[PATCH] media: imx-jpeg: Fix JPEG encoder ready race condition

[Ming Qian(OSS)]

Hi Marek,

On 8/21/2025 12:29 AM, Marek Vasut wrote:
> [You don't often get email from marek.vasut at mailbox.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> The current mxc_jpeg_job_ready() implementation works for JPEG decode
> side of this IP, it does not work at all for the JPEG encode side. The
> JPEG encode side does not change ctx->source_change at all, therefore
> the mxc_jpeg_source_change() always returns right away and the encode
> side somehow works.
> 
> However, this is susceptible to a race condition between mxc_jpeg_dec_irq()
> and mxc_jpeg_start_streaming(), where mxc_jpeg_start_streaming() might
> start decoding another frame before mxc_jpeg_dec_irq() indicates completion
> of encoding of current frame. Add new state, MXC_JPEG_ENC_DONE, which is
> set in three locations, first when streaming starts to indicate the encoder
> is ready to start processing a frame, second in mxc_jpeg_dec_irq() when the
> encoder finishes encoding current frame, and third in mxc_jpeg_dec_irq() in
> case of an error so the encoder can proceed with encoding another frame.
> 
> Update mxc_jpeg_job_ready() to check whether the encoder is in this new
> MXC_JPEG_ENC_DONE state before reporting the encoder is ready to encode
> new frame.
> 

The jpeg encoder and jpeg decoder are 2 different devices, so they won't
compete with each other.

For encoder, we always want mxc_jpeg_job_ready() to return true.

And v4l2_m2m has ensured that there will be no race condition between
mxc_jpeg_dec_irq() and mxc_jpeg_start_streaming().

After v4l2_m2m_job_finish() is called in the mxc_jpeg_dec_irq(),
then it is possible to start encoding the next frame.

So I don't think it's necessary to add a new state MXC_JPEG_ENC_DONE,
as It is almost equivalent to the completion status of the v4l2 m2m job.

Regards,
Ming

> Fixes: b4e1fb8643da ("media: imx-jpeg: Support dynamic resolution change")
> Signed-off-by: Marek Vasut <marek.vasut at mailbox.org>
> ---
> Cc: Fabio Estevam <festevam at gmail.com>
> Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Cc: Mauro Carvalho Chehab <mchehab at kernel.org>
> Cc: Ming Qian <ming.qian at oss.nxp.com>
> Cc: Mirela Rabulea <mirela.rabulea at nxp.com>
> Cc: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> Cc: Pengutronix Kernel Team <kernel at pengutronix.de>
> Cc: Sascha Hauer <s.hauer at pengutronix.de>
> Cc: Shawn Guo <shawnguo at kernel.org>
> Cc: imx at lists.linux.dev
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: linux-media at vger.kernel.org
> ---
>   drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 18 ++++++++++++++++--
>   drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h |  1 +
>   2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> index df3ccdf767baf..aef1d6473eb8d 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> @@ -1009,6 +1009,7 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
> 
>                  dev_err(dev, "Encoder/decoder error, dec_ret = 0x%08x, status=0x%08x",
>                          dec_ret, ret);
> +               ctx->enc_state = MXC_JPEG_ENC_DONE;
>                  mxc_jpeg_clr_desc(reg, slot);
>                  mxc_jpeg_sw_reset(reg);
>                  buf_state = VB2_BUF_STATE_ERROR;
> @@ -1062,9 +1063,16 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
> 
>   buffers_done:
>          mxc_jpeg_job_finish(ctx, buf_state, false);
> -       spin_unlock(&jpeg->hw_lock);
>          cancel_delayed_work(&ctx->task_timer);
> +
> +       if (jpeg->mode == MXC_JPEG_ENCODE && ctx->enc_state == MXC_JPEG_ENCODING)
> +               ctx->enc_state = MXC_JPEG_ENC_DONE;
> +
>          v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
> +
> +       spin_unlock(&jpeg->hw_lock);
> +
> +
>          return IRQ_HANDLED;
>   job_unlock:
>          spin_unlock(&jpeg->hw_lock);
> @@ -1488,8 +1496,12 @@ static bool mxc_jpeg_source_change(struct mxc_jpeg_ctx *ctx,
>   static int mxc_jpeg_job_ready(void *priv)
>   {
>          struct mxc_jpeg_ctx *ctx = priv;
> +       struct mxc_jpeg_dev *jpeg = ctx->mxc_jpeg;
> 
> -       return ctx->source_change ? 0 : 1;
> +       if (jpeg->mode == MXC_JPEG_ENCODE)
> +               return ctx->enc_state == MXC_JPEG_ENC_DONE;
> +       else
> +               return ctx->source_change ? 0 : 1;
>   }
> 
>   static void mxc_jpeg_device_run_timeout(struct work_struct *work)
> @@ -1713,6 +1725,8 @@ static int mxc_jpeg_start_streaming(struct vb2_queue *q, unsigned int count)
> 
>          if (ctx->mxc_jpeg->mode == MXC_JPEG_DECODE && V4L2_TYPE_IS_CAPTURE(q->type))
>                  ctx->source_change = 0;
> +       if (ctx->mxc_jpeg->mode == MXC_JPEG_ENCODE)
> +               ctx->enc_state = MXC_JPEG_ENC_DONE;
>          dev_dbg(ctx->mxc_jpeg->dev, "Start streaming ctx=%p", ctx);
>          q_data->sequence = 0;
> 
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> index 44e46face6d1d..7f0910fc9b47e 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> @@ -35,6 +35,7 @@
>   enum mxc_jpeg_enc_state {
>          MXC_JPEG_ENCODING       = 0, /* jpeg encode phase */
>          MXC_JPEG_ENC_CONF       = 1, /* jpeg encoder config phase */
> +       MXC_JPEG_ENC_DONE       = 2, /* jpeg encoder done/ready phase */
>   };
> 
>   enum mxc_jpeg_mode {
> --
> 2.50.1
>