Re: [PATCH] media: cedrus: Add watchdog for job completion

[Ezequiel Garcia]

Hello,

On Tue, 1 Feb 2022 at 15:34, Jernej Skrabec <jernej.skrabec@xxxxxxxxx> wrote:
>
> Currently, if job is not completed for whatever reason, userspace
> application can hang on ioctl and thus become unkillable.
>
> In order to prevent that, implement watchdog, which will complete job
> after 2 seconds with error state.
>
> Concept is borrowed from hantro driver.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>

This has been very useful developing Hantro,
to get some indication of hardware being programmed wrongly:

Patch looks correct.

Reviewed-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxxxxxxxxx>


> ---
>  drivers/staging/media/sunxi/cedrus/cedrus.c   |  2 ++
>  drivers/staging/media/sunxi/cedrus/cedrus.h   |  3 +++
>  .../staging/media/sunxi/cedrus/cedrus_dec.c   |  4 +++
>  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 25 +++++++++++++++++++
>  .../staging/media/sunxi/cedrus/cedrus_hw.h    |  2 ++
>  5 files changed, 36 insertions(+)
>
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
> index 4a4b714b0f26..68b3dcdb5df3 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> @@ -439,6 +439,8 @@ static int cedrus_probe(struct platform_device *pdev)
>
>         mutex_init(&dev->dev_mutex);
>
> +       INIT_DELAYED_WORK(&dev->watchdog_work, cedrus_watchdog);
> +
>         ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
>         if (ret) {
>                 dev_err(&pdev->dev, "Failed to register V4L2 device\n");
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
> index c345f2984041..3bc094eb497f 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> @@ -24,6 +24,7 @@
>
>  #include <linux/iopoll.h>
>  #include <linux/platform_device.h>
> +#include <linux/workqueue.h>
>
>  #define CEDRUS_NAME                    "cedrus"
>
> @@ -194,6 +195,8 @@ struct cedrus_dev {
>         struct reset_control    *rstc;
>
>         unsigned int            capabilities;
> +
> +       struct delayed_work     watchdog_work;
>  };
>
>  extern struct cedrus_dec_ops cedrus_dec_ops_mpeg2;
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> index a16c1422558f..9c7200299465 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> @@ -97,4 +97,8 @@ void cedrus_device_run(void *priv)
>                 v4l2_ctrl_request_complete(src_req, &ctx->hdl);
>
>         dev->dec_ops[ctx->current_codec]->trigger(ctx);
> +
> +       /* Start the watchdog timer. */
> +       schedule_delayed_work(&dev->watchdog_work,
> +                             msecs_to_jiffies(2000));
>  }
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> index 2d7663726467..a6470a89851e 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> @@ -118,6 +118,13 @@ static irqreturn_t cedrus_irq(int irq, void *data)
>         enum vb2_buffer_state state;
>         enum cedrus_irq_status status;
>
> +       /*
> +        * If cancel_delayed_work returns false it means watchdog already
> +        * executed and finished the job.
> +        */
> +       if (!cancel_delayed_work(&dev->watchdog_work))
> +               return IRQ_HANDLED;
> +
>         ctx = v4l2_m2m_get_curr_priv(dev->m2m_dev);
>         if (!ctx) {
>                 v4l2_err(&dev->v4l2_dev,
> @@ -143,6 +150,24 @@ static irqreturn_t cedrus_irq(int irq, void *data)
>         return IRQ_HANDLED;
>  }
>
> +void cedrus_watchdog(struct work_struct *work)
> +{
> +       struct cedrus_dev *dev;
> +       struct cedrus_ctx *ctx;
> +
> +       dev = container_of(to_delayed_work(work),
> +                          struct cedrus_dev, watchdog_work);
> +
> +       ctx = v4l2_m2m_get_curr_priv(dev->m2m_dev);
> +       if (!ctx)
> +               return;
> +
> +       v4l2_err(&dev->v4l2_dev, "frame processing timed out!\n");
> +       reset_control_reset(dev->rstc);
> +       v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev, ctx->fh.m2m_ctx,
> +                                        VB2_BUF_STATE_ERROR);
> +}
> +
>  int cedrus_hw_suspend(struct device *device)
>  {
>         struct cedrus_dev *dev = dev_get_drvdata(device);
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> index 45f641f0bfa2..7c92f00e36da 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> @@ -28,4 +28,6 @@ int cedrus_hw_resume(struct device *device);
>  int cedrus_hw_probe(struct cedrus_dev *dev);
>  void cedrus_hw_remove(struct cedrus_dev *dev);
>
> +void cedrus_watchdog(struct work_struct *work);
> +
>  #endif
> --
> 2.35.1
>