Re: [PATCH] media: mediatek: vcodec: Use spinlock for context list protection lock

[Chen-Yu Tsai]

On Thu, Aug 14, 2025 at 4:59 PM Fei Shao <fshao@xxxxxxxxxxxx> wrote:
>
> On Thu, Aug 14, 2025 at 4:38 PM Chen-Yu Tsai <wenst@xxxxxxxxxxxx> wrote:
> >
> > Previously a mutex was added to protect the encoder and decoder context
> > lists from unexpected changes originating from the SCP IP block, causing
> > the context pointer to go invalid, resulting in a NULL pointer
> > dereference in the IPI handler.
> >
> > Turns out on the MT8173, the VPU IPI handler is called from hard IRQ
> > context. This causes a big warning from the scheduler. This was first
> > reported downstream on the ChromeOS kernels, but is also reproducible
> > on mainline using Fluster with the FFmpeg v4l2m2m decoders. Even though
> > the actual capture format is not supported, the affected code paths
> > are triggered.
> >
> > Since this lock just protects the context list and operations on it are
> > very fast, it should be OK to switch to a spinlock.
> >
> > Fixes: 6467cda18c9f ("media: mediatek: vcodec: adding lock to protect decoder context list")
> > Fixes: afaaf3a0f647 ("media: mediatek: vcodec: adding lock to protect encoder context list")
> > Cc: Yunfei Dong <yunfei.dong@xxxxxxxxxxxx>
> > Signed-off-by: Chen-Yu Tsai <wenst@xxxxxxxxxxxx>
> > ---
> >  .../mediatek/vcodec/common/mtk_vcodec_fw_vpu.c       | 10 ++++++----
> >  .../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c     | 12 +++++++-----
> >  .../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h     |  2 +-
> >  .../platform/mediatek/vcodec/decoder/vdec_vpu_if.c   |  4 ++--
> >  .../mediatek/vcodec/encoder/mtk_vcodec_enc_drv.c     | 12 +++++++-----
> >  .../mediatek/vcodec/encoder/mtk_vcodec_enc_drv.h     |  2 +-
> >  .../platform/mediatek/vcodec/encoder/venc_vpu_if.c   |  4 ++--
> >  7 files changed, 26 insertions(+), 20 deletions(-)
> >
>
> [...]
>
> > diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c
> > index 145958206e38..e9b5cac9c63b 100644
> > --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c
> > +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c
> > @@ -77,14 +77,14 @@ static bool vpu_dec_check_ap_inst(struct mtk_vcodec_dec_dev *dec_dev, struct vde
> >         struct mtk_vcodec_dec_ctx *ctx;
> >         int ret = false;
> >
> > -       mutex_lock(&dec_dev->dev_ctx_lock);
> > +       spin_lock(&dec_dev->dev_ctx_lock);
>
> Do you mean spin_lock_irqsave()?

This function is only called from the handler below (outside the diff
context), which itself is called from hard IRQ context. This is mentioned
in the comment above the handler.

> >         list_for_each_entry(ctx, &dec_dev->ctx_list, list) {
> >                 if (!IS_ERR_OR_NULL(ctx) && ctx->vpu_inst == vpu) {
> >                         ret = true;
> >                         break;
> >                 }
> >         }
> > -       mutex_unlock(&dec_dev->dev_ctx_lock);
> > +       spin_unlock(&dec_dev->dev_ctx_lock);
> >
> >         return ret;
> >  }
>
> [...]
>
> > diff --git a/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c b/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> > index 51bb7ee141b9..79a91283da78 100644
> > --- a/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> > +++ b/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> > @@ -47,14 +47,14 @@ static bool vpu_enc_check_ap_inst(struct mtk_vcodec_enc_dev *enc_dev, struct ven
> >         struct mtk_vcodec_enc_ctx *ctx;
> >         int ret = false;
> >
> > -       mutex_lock(&enc_dev->dev_ctx_lock);
> > +       spin_lock(&enc_dev->dev_ctx_lock);
>
> Also here.

Same reasoning applies here as well.

ChenYu

> Regards,
> Fei
>
> >         list_for_each_entry(ctx, &enc_dev->ctx_list, list) {
> >                 if (!IS_ERR_OR_NULL(ctx) && ctx->vpu_inst == vpu) {
> >                         ret = true;
> >                         break;
> >                 }
> >         }
> > -       mutex_unlock(&enc_dev->dev_ctx_lock);
> > +       spin_unlock(&enc_dev->dev_ctx_lock);
> >
> >         return ret;
> >  }
> > --
> > 2.51.0.rc1.163.g2494970778-goog
> >
> >