Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 26 Dec 2022 19:32:10 -0300
From:   Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
To:     Nicolas Dufresne <nicolas.dufresne@collabora.com>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        kernel@collabora.com, linux-media@vger.kernel.org,
        linux-rockchip@lists.infradead.org, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 4/4] rkvdec: Improve error handling
Message-ID: <Y6og6ojA3a/lpAL/@eze-laptop>
References: <20221223193807.914935-1-nicolas.dufresne@collabora.com>
 <20221223193807.914935-5-nicolas.dufresne@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221223193807.914935-5-nicolas.dufresne@collabora.com>
Precedence: bulk

Hi Nicolas,

Thanks a lot for the patchset. I have just some style feedback.

On Fri, Dec 23, 2022 at 02:38:06PM -0500, Nicolas Dufresne wrote:
> There are two ways decoding errors can occure. In one case, the ready
> status is not set and nothing has been written into the destination,
> while in the other case, the buffer is written but may contain a
> certain amount of errors. In order to differentiate these, we set
> the payload for the first case to 0.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
>  drivers/staging/media/rkvdec/rkvdec.c | 31 +++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> index 7e76f8b728854..11e2bbb20aea1 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -27,6 +27,9 @@
>  #include "rkvdec.h"
>  #include "rkvdec-regs.h"
>  
> +static int debug;
> +module_param(debug, int, 0644);
> +
>  static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
> @@ -954,14 +957,34 @@ static irqreturn_t rkvdec_irq_handler(int irq, void *priv)
>  	enum vb2_buffer_state state;
>  	u32 status;
>  
> +	ctx = v4l2_m2m_get_curr_priv(rkvdec->m2m_dev);
>  	status = readl(rkvdec->regs + RKVDEC_REG_INTERRUPT);

Maybe group the I/O together, i.e. the writel would
be right after this readl:

    writel(0, rkvdec->regs + RKVDEC_REG_INTERRUPT);

> -	state = (status & RKVDEC_RDY_STA) ?
> -		VB2_BUF_STATE_DONE : VB2_BUF_STATE_ERROR;
> +
> +	if (!(status & RKVDEC_RDY_STA)) {
> +		struct vb2_v4l2_buffer *dst_buf = NULL;
> +
> +		if (status & RKVDEC_TIMEOUT_STA)
> +			v4l2_dbg(debug, 1, &rkvdec->v4l2_dev,
> +				 "Decoder stopped due to an internal timeout.");
> +		else
> +			v4l2_dbg(debug, 1, &rkvdec->v4l2_dev,
> +				 "Decoder stopped due to an internal error.");

Unless you really want to ensure this string is greppable,
you can do something like:

v4l2_dbg(debug, 1, &rkvdec->v4l2_dev, "Decoder stopped due to an internal %s.", .... ? "error" : "timeout");

> +
> +		/*
> +		 * When this happens, the buffer is left unmodified. As it
> +		 * contains no meaningful data we mark is as empty.
> +		 */
> +		dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> +		vb2_set_plane_payload(&dst_buf->vb2_buf, 0, 0);

Perhaps we can avoid this vb2_set_plane_payload(&dst_buf->vb2_buf, 0, 0);
if we instead set the payload in rkvdec_job_finish_no_pm().
It would change the behavior, as we would be setting payload
only when state is _DONE, so maybe that's not what you want.

> +		state = VB2_BUF_STATE_ERROR;
> +	} else {
> +		state = VB2_BUF_STATE_DONE;
> +	}
>  
>  	writel(0, rkvdec->regs + RKVDEC_REG_INTERRUPT);
> -	ctx = v4l2_m2m_get_curr_priv(rkvdec->m2m_dev);
>  
> -	if (ctx->coded_fmt_desc->ops->check_error_info)
> +	if (ctx->coded_fmt_desc->ops->check_error_info &&
> +	    state == VB2_BUF_STATE_DONE)
>  		state = ctx->coded_fmt_desc->ops->check_error_info(ctx);
>  

How about this:

static irqreturn_t rkvdec_irq_handler(int irq, void *priv)
{
    struct rkvdec_dev *rkvdec = priv;
    struct rkvdec_ctx *ctx;
    enum vb2_buffer_state state = VB2_BUF_STATE_DONE;
    u32 status;

    ctx = v4l2_m2m_get_curr_priv(rkvdec->m2m_dev);
    status = readl(rkvdec->regs + RKVDEC_REG_INTERRUPT);
    writel(0, rkvdec->regs + RKVDEC_REG_INTERRUPT);

    if (!(status & RKVDEC_RDY_STA)) {
        ...
        state = VB2_BUF_STATE_ERROR;
    } else {
        if (ctx->coded_fmt_desc->ops->check_error_info &&
            ctx->coded_fmt_desc->ops->check_error_info(ctx))
            state = VB2_BUF_STATE_ERROR;
    }

    if (cancel_delayed_work(&rkvdec->watchdog_work))
        rkvdec_job_finish(ctx, state);

    return IRQ_HANDLED;
}

So it's clear which paths lead to VB2_BUF_STATE_ERROR.

Thanks!
Ezequiel

>  	if (cancel_delayed_work(&rkvdec->watchdog_work))
> -- 
> 2.38.1
>