lkml.org 
[lkml]   [2022]   [Dec]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 4/4] rkvdec: Improve error handling
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
>

\
 
 \ /
  Last update: 2023-03-26 23:21    [W:0.032 / U:1.304 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site