* [PATCH] media: mediatek: vcodec: Use spinlock for context list protection lock
@ 2025-08-14 8:31 Chen-Yu Tsai
2025-08-14 8:58 ` Fei Shao
0 siblings, 1 reply; 5+ messages in thread
From: Chen-Yu Tsai @ 2025-08-14 8:31 UTC (permalink / raw)
To: Yunfei Dong, Mauro Carvalho Chehab, Hans Verkuil,
Matthias Brugger, AngeloGioacchino Del Regno
Cc: Chen-Yu Tsai, linux-media, linux-kernel, linux-mediatek
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@mediatek.com>
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
.../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/common/mtk_vcodec_fw_vpu.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c
index d7027d600208..223fb2294894 100644
--- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c
+++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c
@@ -47,30 +47,32 @@ static void mtk_vcodec_vpu_reset_dec_handler(void *priv)
{
struct mtk_vcodec_dec_dev *dev = priv;
struct mtk_vcodec_dec_ctx *ctx;
+ unsigned long flags;
dev_err(&dev->plat_dev->dev, "Watchdog timeout!!");
- mutex_lock(&dev->dev_ctx_lock);
+ spin_lock_irqsave(&dev->dev_ctx_lock, flags);
list_for_each_entry(ctx, &dev->ctx_list, list) {
ctx->state = MTK_STATE_ABORT;
mtk_v4l2_vdec_dbg(0, ctx, "[%d] Change to state MTK_STATE_ABORT", ctx->id);
}
- mutex_unlock(&dev->dev_ctx_lock);
+ spin_unlock_irqrestore(&dev->dev_ctx_lock, flags);
}
static void mtk_vcodec_vpu_reset_enc_handler(void *priv)
{
struct mtk_vcodec_enc_dev *dev = priv;
struct mtk_vcodec_enc_ctx *ctx;
+ unsigned long flags;
dev_err(&dev->plat_dev->dev, "Watchdog timeout!!");
- mutex_lock(&dev->dev_ctx_lock);
+ spin_lock_irqsave(&dev->dev_ctx_lock, flags);
list_for_each_entry(ctx, &dev->ctx_list, list) {
ctx->state = MTK_STATE_ABORT;
mtk_v4l2_vdec_dbg(0, ctx, "[%d] Change to state MTK_STATE_ABORT", ctx->id);
}
- mutex_unlock(&dev->dev_ctx_lock);
+ spin_unlock_irqrestore(&dev->dev_ctx_lock, flags);
}
static const struct mtk_vcodec_fw_ops mtk_vcodec_vpu_msg = {
diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
index 46d176e6de63..3b81fae9f913 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
@@ -198,6 +198,7 @@ static int fops_vcodec_open(struct file *file)
struct mtk_vcodec_dec_ctx *ctx = NULL;
int ret = 0, i, hw_count;
struct vb2_queue *src_vq;
+ unsigned long flags;
ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
if (!ctx)
@@ -267,9 +268,9 @@ static int fops_vcodec_open(struct file *file)
ctx->dev->vdec_pdata->init_vdec_params(ctx);
- mutex_lock(&dev->dev_ctx_lock);
+ spin_lock_irqsave(&dev->dev_ctx_lock, flags);
list_add(&ctx->list, &dev->ctx_list);
- mutex_unlock(&dev->dev_ctx_lock);
+ spin_unlock_irqrestore(&dev->dev_ctx_lock, flags);
mtk_vcodec_dbgfs_create(ctx);
mutex_unlock(&dev->dev_mutex);
@@ -294,6 +295,7 @@ static int fops_vcodec_release(struct file *file)
{
struct mtk_vcodec_dec_dev *dev = video_drvdata(file);
struct mtk_vcodec_dec_ctx *ctx = file_to_dec_ctx(file);
+ unsigned long flags;
mtk_v4l2_vdec_dbg(0, ctx, "[%d] decoder", ctx->id);
mutex_lock(&dev->dev_mutex);
@@ -312,9 +314,9 @@ static int fops_vcodec_release(struct file *file)
v4l2_ctrl_handler_free(&ctx->ctrl_hdl);
mtk_vcodec_dbgfs_remove(dev, ctx->id);
- mutex_lock(&dev->dev_ctx_lock);
+ spin_lock_irqsave(&dev->dev_ctx_lock, flags);
list_del_init(&ctx->list);
- mutex_unlock(&dev->dev_ctx_lock);
+ spin_unlock_irqrestore(&dev->dev_ctx_lock, flags);
kfree(ctx);
mutex_unlock(&dev->dev_mutex);
return 0;
@@ -407,7 +409,7 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
for (i = 0; i < MTK_VDEC_HW_MAX; i++)
mutex_init(&dev->dec_mutex[i]);
mutex_init(&dev->dev_mutex);
- mutex_init(&dev->dev_ctx_lock);
+ spin_lock_init(&dev->dev_ctx_lock);
spin_lock_init(&dev->irqlock);
snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name), "%s",
diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
index d047d7c580fb..9d68808e8f9c 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
@@ -285,7 +285,7 @@ struct mtk_vcodec_dec_dev {
/* decoder hardware mutex lock */
struct mutex dec_mutex[MTK_VDEC_HW_MAX];
struct mutex dev_mutex;
- struct mutex dev_ctx_lock;
+ spinlock_t dev_ctx_lock;
struct workqueue_struct *decode_workqueue;
spinlock_t irqlock;
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);
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/mtk_vcodec_enc_drv.c b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv.c
index fb1c3bdc2dae..82b8ff38e8f1 100644
--- a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv.c
+++ b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv.c
@@ -117,6 +117,7 @@ static int fops_vcodec_open(struct file *file)
struct mtk_vcodec_enc_ctx *ctx = NULL;
int ret = 0;
struct vb2_queue *src_vq;
+ unsigned long flags;
ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
if (!ctx)
@@ -176,9 +177,9 @@ static int fops_vcodec_open(struct file *file)
mtk_v4l2_venc_dbg(2, ctx, "Create instance [%d]@%p m2m_ctx=%p ",
ctx->id, ctx, ctx->m2m_ctx);
- mutex_lock(&dev->dev_ctx_lock);
+ spin_lock_irqsave(&dev->dev_ctx_lock, flags);
list_add(&ctx->list, &dev->ctx_list);
- mutex_unlock(&dev->dev_ctx_lock);
+ spin_unlock_irqrestore(&dev->dev_ctx_lock, flags);
mutex_unlock(&dev->dev_mutex);
mtk_v4l2_venc_dbg(0, ctx, "%s encoder [%d]", dev_name(&dev->plat_dev->dev),
@@ -203,6 +204,7 @@ static int fops_vcodec_release(struct file *file)
{
struct mtk_vcodec_enc_dev *dev = video_drvdata(file);
struct mtk_vcodec_enc_ctx *ctx = file_to_enc_ctx(file);
+ unsigned long flags;
mtk_v4l2_venc_dbg(1, ctx, "[%d] encoder", ctx->id);
mutex_lock(&dev->dev_mutex);
@@ -213,9 +215,9 @@ static int fops_vcodec_release(struct file *file)
v4l2_fh_exit(&ctx->fh);
v4l2_ctrl_handler_free(&ctx->ctrl_hdl);
- mutex_lock(&dev->dev_ctx_lock);
+ spin_lock_irqsave(&dev->dev_ctx_lock, flags);
list_del_init(&ctx->list);
- mutex_unlock(&dev->dev_ctx_lock);
+ spin_unlock_irqrestore(&dev->dev_ctx_lock, flags);
kfree(ctx);
mutex_unlock(&dev->dev_mutex);
return 0;
@@ -297,7 +299,7 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
mutex_init(&dev->enc_mutex);
mutex_init(&dev->dev_mutex);
- mutex_init(&dev->dev_ctx_lock);
+ spin_lock_init(&dev->dev_ctx_lock);
spin_lock_init(&dev->irqlock);
snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name), "%s",
diff --git a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv.h b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv.h
index 5b304a551236..0cddfa13594f 100644
--- a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv.h
+++ b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv.h
@@ -206,7 +206,7 @@ struct mtk_vcodec_enc_dev {
/* encoder hardware mutex lock */
struct mutex enc_mutex;
struct mutex dev_mutex;
- struct mutex dev_ctx_lock;
+ spinlock_t dev_ctx_lock;
struct workqueue_struct *encode_workqueue;
int enc_irq;
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);
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] media: mediatek: vcodec: Use spinlock for context list protection lock
2025-08-14 8:31 [PATCH] media: mediatek: vcodec: Use spinlock for context list protection lock Chen-Yu Tsai
@ 2025-08-14 8:58 ` Fei Shao
2025-08-14 9:06 ` Chen-Yu Tsai
0 siblings, 1 reply; 5+ messages in thread
From: Fei Shao @ 2025-08-14 8:58 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Yunfei Dong, Mauro Carvalho Chehab, Hans Verkuil,
Matthias Brugger, AngeloGioacchino Del Regno, linux-media,
linux-kernel, linux-mediatek
On Thu, Aug 14, 2025 at 4:38 PM Chen-Yu Tsai <wenst@chromium.org> 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@mediatek.com>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> .../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()?
> 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.
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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: mediatek: vcodec: Use spinlock for context list protection lock
2025-08-14 8:58 ` Fei Shao
@ 2025-08-14 9:06 ` Chen-Yu Tsai
2025-08-14 9:47 ` Fei Shao
0 siblings, 1 reply; 5+ messages in thread
From: Chen-Yu Tsai @ 2025-08-14 9:06 UTC (permalink / raw)
To: Fei Shao
Cc: Yunfei Dong, Mauro Carvalho Chehab, Hans Verkuil,
Matthias Brugger, AngeloGioacchino Del Regno, linux-media,
linux-kernel, linux-mediatek
On Thu, Aug 14, 2025 at 4:59 PM Fei Shao <fshao@chromium.org> wrote:
>
> On Thu, Aug 14, 2025 at 4:38 PM Chen-Yu Tsai <wenst@chromium.org> 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@mediatek.com>
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> > .../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
> >
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: mediatek: vcodec: Use spinlock for context list protection lock
2025-08-14 9:06 ` Chen-Yu Tsai
@ 2025-08-14 9:47 ` Fei Shao
2025-08-20 5:15 ` Tomasz Figa
0 siblings, 1 reply; 5+ messages in thread
From: Fei Shao @ 2025-08-14 9:47 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Yunfei Dong, Mauro Carvalho Chehab, Hans Verkuil,
Matthias Brugger, AngeloGioacchino Del Regno, linux-media,
linux-kernel, linux-mediatek
On Thu, Aug 14, 2025 at 5:06 PM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> On Thu, Aug 14, 2025 at 4:59 PM Fei Shao <fshao@chromium.org> wrote:
> >
> > On Thu, Aug 14, 2025 at 4:38 PM Chen-Yu Tsai <wenst@chromium.org> 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@mediatek.com>
> > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > > ---
> > > .../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.
I see. I only searched here but didn't check the full source.
Leaving a comment would be clearer if a revision is made, but it's not
worth resending just for that alone.
Reviewed-by: Fei Shao <fshao@chromium.org>
On Thu, Aug 14, 2025 at 5:06 PM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> On Thu, Aug 14, 2025 at 4:59 PM Fei Shao <fshao@chromium.org> wrote:
> >
> > On Thu, Aug 14, 2025 at 4:38 PM Chen-Yu Tsai <wenst@chromium.org> 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@mediatek.com>
> > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > > ---
> > > .../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
> > >
> > >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: mediatek: vcodec: Use spinlock for context list protection lock
2025-08-14 9:47 ` Fei Shao
@ 2025-08-20 5:15 ` Tomasz Figa
0 siblings, 0 replies; 5+ messages in thread
From: Tomasz Figa @ 2025-08-20 5:15 UTC (permalink / raw)
To: Fei Shao
Cc: Chen-Yu Tsai, Yunfei Dong, Mauro Carvalho Chehab, Hans Verkuil,
Matthias Brugger, AngeloGioacchino Del Regno, linux-media,
linux-kernel, linux-mediatek
On Thu, Aug 14, 2025 at 6:52 PM Fei Shao <fshao@chromium.org> wrote:
>
> On Thu, Aug 14, 2025 at 5:06 PM Chen-Yu Tsai <wenst@chromium.org> wrote:
> >
> > On Thu, Aug 14, 2025 at 4:59 PM Fei Shao <fshao@chromium.org> wrote:
> > >
> > > On Thu, Aug 14, 2025 at 4:38 PM Chen-Yu Tsai <wenst@chromium.org> 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@mediatek.com>
> > > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > > > ---
> > > > .../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.
>
> I see. I only searched here but didn't check the full source.
> Leaving a comment would be clearer if a revision is made, but it's not
> worth resending just for that alone.
Hmm, I feel like this could make it easy to introduce further locking
bugs in the future, because someone may just decide to call this
function from a different context. Also having the _irqsave variants
consistently used for the lock make it clear that it's used to
synchronize with an IRQ handler regardless of which place in the code
one looks at.
My recommendation would be to still amend the patch to do that.
>
> Reviewed-by: Fei Shao <fshao@chromium.org>
>
>
> On Thu, Aug 14, 2025 at 5:06 PM Chen-Yu Tsai <wenst@chromium.org> wrote:
> >
> > On Thu, Aug 14, 2025 at 4:59 PM Fei Shao <fshao@chromium.org> wrote:
> > >
> > > On Thu, Aug 14, 2025 at 4:38 PM Chen-Yu Tsai <wenst@chromium.org> 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@mediatek.com>
> > > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > > > ---
> > > > .../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
> > > >
> > > >
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-08-20 5:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-14 8:31 [PATCH] media: mediatek: vcodec: Use spinlock for context list protection lock Chen-Yu Tsai
2025-08-14 8:58 ` Fei Shao
2025-08-14 9:06 ` Chen-Yu Tsai
2025-08-14 9:47 ` Fei Shao
2025-08-20 5:15 ` Tomasz Figa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).