[PATCH v2 1/2] pmdomain: imx8m-blk-ctrl: Remove separate rst and clk mask for 8mq vpu

[PATCH v2 1/2] pmdomain: imx8m-blk-ctrl: Remove separate rst and clk mask for 8mq vpu

[ming.qian]

From: Ming Qian <ming.qian@oss.nxp.com>

For i.MX8MQ platform, the ADB in the VPUMIX domain has no separate reset
and clock enable bits, but is ungated and reset together with the VPUs.
So we can't reset G1 or G2 separately, it may led to the system hang.
Remove rst_mask and clk_mask of imx8mq_vpu_blk_ctl_domain_data.
Let imx8mq_vpu_power_notifier() do really vpu reset.

Fixes: 608d7c325e85 ("soc: imx: imx8m-blk-ctrl: add i.MX8MQ VPU blk-ctrl")
Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
Reviewed-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
v2
- Update commit message

 drivers/pmdomain/imx/imx8m-blk-ctrl.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/pmdomain/imx/imx8m-blk-ctrl.c b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
index 5c83e5599f1e..1f07ff041295 100644
--- a/drivers/pmdomain/imx/imx8m-blk-ctrl.c
+++ b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
@@ -852,16 +852,12 @@ static const struct imx8m_blk_ctrl_domain_data imx8mq_vpu_blk_ctl_domain_data[]
 		.clk_names = (const char *[]){ "g1", },
 		.num_clks = 1,
 		.gpc_name = "g1",
-		.rst_mask = BIT(1),
-		.clk_mask = BIT(1),
 	},
 	[IMX8MQ_VPUBLK_PD_G2] = {
 		.name = "vpublk-g2",
 		.clk_names = (const char *[]){ "g2", },
 		.num_clks = 1,
 		.gpc_name = "g2",
-		.rst_mask = BIT(0),
-		.clk_mask = BIT(0),
 	},
 };
 
-- 
2.52.0

[PATCH v2 2/2] media: verisilicon: Avoid G2 bus error while decoding H.264 and HEVC

[ming.qian]

From: Ming Qian <ming.qian@oss.nxp.com>

For the i.MX8MQ platform, there is a hardware limitation: the g1 VPU and
g2 VPU cannot decode simultaneously; otherwise, it will cause below bus
error and produce corrupted pictures, even led to system hang.

[  110.527986] hantro-vpu 38310000.video-codec: frame decode timed out.
[  110.583517] hantro-vpu 38310000.video-codec: bus error detected.

Therefore, it is necessary to ensure that g1 and g2 operate alternately.
Then this allows for successful multi-instance decoding of H.264 and HEVC.

To achieve this, we can have g1 and g2 share the same v4l2_m2m_dev, and
then the v4l2_m2m_dev can handle the scheduling.

Fixes: cb5dd5a0fa518 ("media: hantro: Introduce G2/HEVC decoder")
Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
---
v2
- Abandon the waiting approach.
- Switch to a shared v4l2_m2m_dev solution.

 drivers/media/platform/verisilicon/hantro.h   |  2 +
 .../media/platform/verisilicon/hantro_drv.c   | 41 +++++++++++++++++--
 .../media/platform/verisilicon/imx8m_vpu_hw.c |  2 +
 3 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/verisilicon/hantro.h b/drivers/media/platform/verisilicon/hantro.h
index e0fdc4535b2d..8a0583f95417 100644
--- a/drivers/media/platform/verisilicon/hantro.h
+++ b/drivers/media/platform/verisilicon/hantro.h
@@ -77,6 +77,7 @@ struct hantro_irq {
  * @double_buffer:		core needs double buffering
  * @legacy_regs:		core uses legacy register set
  * @late_postproc:		postproc must be set up at the end of the job
+ * @shared_resource:		flag of shared resource
  */
 struct hantro_variant {
 	unsigned int enc_offset;
@@ -101,6 +102,7 @@ struct hantro_variant {
 	unsigned int double_buffer : 1;
 	unsigned int legacy_regs : 1;
 	unsigned int late_postproc : 1;
+	unsigned int shared_resource : 1;
 };
 
 /**
diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
index 60b95b5d8565..f42b2df86806 100644
--- a/drivers/media/platform/verisilicon/hantro_drv.c
+++ b/drivers/media/platform/verisilicon/hantro_drv.c
@@ -35,6 +35,10 @@ module_param_named(debug, hantro_debug, int, 0644);
 MODULE_PARM_DESC(debug,
 		 "Debug level - higher value produces more verbose messages");
 
+static DEFINE_MUTEX(shared_dev_lock);
+static atomic_t shared_dev_ref_cnt = ATOMIC_INIT(0);
+static struct v4l2_m2m_dev *shared_m2m_dev;
+
 void *hantro_get_ctrl(struct hantro_ctx *ctx, u32 id)
 {
 	struct v4l2_ctrl *ctrl;
@@ -1035,6 +1039,37 @@ static int hantro_disable_multicore(struct hantro_dev *vpu)
 	return 0;
 }
 
+static struct v4l2_m2m_dev *hantro_get_v4l2_m2m_dev(struct hantro_dev *vpu)
+{
+	if (!vpu->variant || !vpu->variant->shared_resource)
+		return v4l2_m2m_init(&vpu_m2m_ops);
+
+	scoped_guard(mutex, &shared_dev_lock) {
+		if (atomic_inc_return(&shared_dev_ref_cnt) == 1) {
+			shared_m2m_dev = v4l2_m2m_init(&vpu_m2m_ops);
+			if (IS_ERR(shared_m2m_dev))
+				atomic_dec(&shared_dev_ref_cnt);
+		}
+	}
+
+	return shared_m2m_dev;
+}
+
+static void hantro_put_v4l2_m2m_dev(struct hantro_dev *vpu)
+{
+	if (!vpu->variant || !vpu->variant->shared_resource)
+		v4l2_m2m_release(vpu->m2m_dev);
+
+	scoped_guard(mutex, &shared_dev_lock) {
+		if (!atomic_dec_return(&shared_dev_ref_cnt)) {
+			if (!IS_ERR(shared_m2m_dev)) {
+				v4l2_m2m_release(shared_m2m_dev);
+				shared_m2m_dev = NULL;
+			}
+		}
+	}
+}
+
 static int hantro_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *match;
@@ -1186,7 +1221,7 @@ static int hantro_probe(struct platform_device *pdev)
 	}
 	platform_set_drvdata(pdev, vpu);
 
-	vpu->m2m_dev = v4l2_m2m_init(&vpu_m2m_ops);
+	vpu->m2m_dev = hantro_get_v4l2_m2m_dev(vpu);
 	if (IS_ERR(vpu->m2m_dev)) {
 		v4l2_err(&vpu->v4l2_dev, "Failed to init mem2mem device\n");
 		ret = PTR_ERR(vpu->m2m_dev);
@@ -1225,7 +1260,7 @@ static int hantro_probe(struct platform_device *pdev)
 	hantro_remove_enc_func(vpu);
 err_m2m_rel:
 	media_device_cleanup(&vpu->mdev);
-	v4l2_m2m_release(vpu->m2m_dev);
+	hantro_put_v4l2_m2m_dev(vpu);
 err_v4l2_unreg:
 	v4l2_device_unregister(&vpu->v4l2_dev);
 err_clk_unprepare:
@@ -1248,7 +1283,7 @@ static void hantro_remove(struct platform_device *pdev)
 	hantro_remove_dec_func(vpu);
 	hantro_remove_enc_func(vpu);
 	media_device_cleanup(&vpu->mdev);
-	v4l2_m2m_release(vpu->m2m_dev);
+	hantro_put_v4l2_m2m_dev(vpu);
 	v4l2_device_unregister(&vpu->v4l2_dev);
 	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
 	reset_control_assert(vpu->resets);
diff --git a/drivers/media/platform/verisilicon/imx8m_vpu_hw.c b/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
index 5be0e2e76882..5111ce5c36f3 100644
--- a/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
+++ b/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
@@ -356,6 +356,7 @@ const struct hantro_variant imx8mq_vpu_g1_variant = {
 	.num_irqs = ARRAY_SIZE(imx8mq_irqs),
 	.clk_names = imx8mq_g1_clk_names,
 	.num_clocks = ARRAY_SIZE(imx8mq_g1_clk_names),
+	.shared_resource = 1,
 };
 
 const struct hantro_variant imx8mq_vpu_g2_variant = {
@@ -371,6 +372,7 @@ const struct hantro_variant imx8mq_vpu_g2_variant = {
 	.num_irqs = ARRAY_SIZE(imx8mq_g2_irqs),
 	.clk_names = imx8mq_g2_clk_names,
 	.num_clocks = ARRAY_SIZE(imx8mq_g2_clk_names),
+	.shared_resource = 1,
 };
 
 const struct hantro_variant imx8mm_vpu_g1_variant = {
-- 
2.52.0

RE: [PATCH v2 1/2] pmdomain: imx8m-blk-ctrl: Remove separate rst and clk mask for 8mq vpu

[Peng Fan]

> Subject: [PATCH v2 1/2] pmdomain: imx8m-blk-ctrl: Remove separate
> rst and clk mask for 8mq vpu
> 
> From: Ming Qian <ming.qian@oss.nxp.com>
> 
> For i.MX8MQ platform, the ADB in the VPUMIX domain has no
> separate reset and clock enable bits, but is ungated and reset together
> with the VPUs.
> So we can't reset G1 or G2 separately, it may led to the system hang.
> Remove rst_mask and clk_mask of imx8mq_vpu_blk_ctl_domain_data.
> Let imx8mq_vpu_power_notifier() do really vpu reset.
> 
> Fixes: 608d7c325e85 ("soc: imx: imx8m-blk-ctrl: add i.MX8MQ VPU
> blk-ctrl")
> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> Reviewed-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

Re: [PATCH v2 1/2] pmdomain: imx8m-blk-ctrl: Remove separate rst and clk mask for 8mq vpu

[Lucas Stach]

Am Freitag, dem 28.11.2025 um 10:51 +0800 schrieb
ming.qian@oss.nxp.com:
> From: Ming Qian <ming.qian@oss.nxp.com>
> 
> For i.MX8MQ platform, the ADB in the VPUMIX domain has no separate reset
> and clock enable bits, but is ungated and reset together with the VPUs.
> So we can't reset G1 or G2 separately, it may led to the system hang.
> Remove rst_mask and clk_mask of imx8mq_vpu_blk_ctl_domain_data.
> Let imx8mq_vpu_power_notifier() do really vpu reset.
> 
> Fixes: 608d7c325e85 ("soc: imx: imx8m-blk-ctrl: add i.MX8MQ VPU blk-ctrl")
> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> Reviewed-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> v2
> - Update commit message
> 
>  drivers/pmdomain/imx/imx8m-blk-ctrl.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/pmdomain/imx/imx8m-blk-ctrl.c b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
> index 5c83e5599f1e..1f07ff041295 100644
> --- a/drivers/pmdomain/imx/imx8m-blk-ctrl.c
> +++ b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
> @@ -852,16 +852,12 @@ static const struct imx8m_blk_ctrl_domain_data imx8mq_vpu_blk_ctl_domain_data[]
>  		.clk_names = (const char *[]){ "g1", },
>  		.num_clks = 1,
>  		.gpc_name = "g1",
> -		.rst_mask = BIT(1),
> -		.clk_mask = BIT(1),

Change itself looks okay to me. Can you please leave a small comment
here and for the G2 domain to document why the clk and reset bits are
removed, so one doesn't need to dig into the git history when reading
the driver code?

Regards,
Lucas

>  	},
>  	[IMX8MQ_VPUBLK_PD_G2] = {
>  		.name = "vpublk-g2",
>  		.clk_names = (const char *[]){ "g2", },
>  		.num_clks = 1,
>  		.gpc_name = "g2",
> -		.rst_mask = BIT(0),
> -		.clk_mask = BIT(0),
>  	},
>  };
>  

Re: [PATCH v2 2/2] media: verisilicon: Avoid G2 bus error while decoding H.264 and HEVC

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 6604 bytes --]

Hi,

Le vendredi 28 novembre 2025 à 10:51 +0800, ming.qian@oss.nxp.com a écrit :
> From: Ming Qian <ming.qian@oss.nxp.com>
> 
> For the i.MX8MQ platform, there is a hardware limitation: the g1 VPU and
> g2 VPU cannot decode simultaneously; otherwise, it will cause below bus
> error and produce corrupted pictures, even led to system hang.
> 
> [  110.527986] hantro-vpu 38310000.video-codec: frame decode timed out.
> [  110.583517] hantro-vpu 38310000.video-codec: bus error detected.
> 
> Therefore, it is necessary to ensure that g1 and g2 operate alternately.
> Then this allows for successful multi-instance decoding of H.264 and HEVC.
> 
> To achieve this, we can have g1 and g2 share the same v4l2_m2m_dev, and
> then the v4l2_m2m_dev can handle the scheduling.
> 
> Fixes: cb5dd5a0fa518 ("media: hantro: Introduce G2/HEVC decoder")
> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>

I like where this is going.
> ---
> v2
> - Abandon the waiting approach.
> - Switch to a shared v4l2_m2m_dev solution.
> 
>  drivers/media/platform/verisilicon/hantro.h   |  2 +
>  .../media/platform/verisilicon/hantro_drv.c   | 41 +++++++++++++++++--
>  .../media/platform/verisilicon/imx8m_vpu_hw.c |  2 +
>  3 files changed, 42 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/verisilicon/hantro.h b/drivers/media/platform/verisilicon/hantro.h
> index e0fdc4535b2d..8a0583f95417 100644
> --- a/drivers/media/platform/verisilicon/hantro.h
> +++ b/drivers/media/platform/verisilicon/hantro.h
> @@ -77,6 +77,7 @@ struct hantro_irq {
>   * @double_buffer:		core needs double buffering
>   * @legacy_regs:		core uses legacy register set
>   * @late_postproc:		postproc must be set up at the end of the job
> + * @shared_resource:		flag of shared resource
>   */
>  struct hantro_variant {
>  	unsigned int enc_offset;
> @@ -101,6 +102,7 @@ struct hantro_variant {
>  	unsigned int double_buffer : 1;
>  	unsigned int legacy_regs : 1;
>  	unsigned int late_postproc : 1;
> +	unsigned int shared_resource : 1;

Instead of that, I'd make an array of compatible node we are going to share the
with.

>  };
>  
>  /**
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index 60b95b5d8565..f42b2df86806 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -35,6 +35,10 @@ module_param_named(debug, hantro_debug, int, 0644);
>  MODULE_PARM_DESC(debug,
>  		 "Debug level - higher value produces more verbose messages");
>  
> +static DEFINE_MUTEX(shared_dev_lock);
> +static atomic_t shared_dev_ref_cnt = ATOMIC_INIT(0);
> +static struct v4l2_m2m_dev *shared_m2m_dev;
> +
>  void *hantro_get_ctrl(struct hantro_ctx *ctx, u32 id)
>  {
>  	struct v4l2_ctrl *ctrl;
> @@ -1035,6 +1039,37 @@ static int hantro_disable_multicore(struct hantro_dev *vpu)
>  	return 0;
>  }
>  
> +static struct v4l2_m2m_dev *hantro_get_v4l2_m2m_dev(struct hantro_dev *vpu)
> +{
> +	if (!vpu->variant || !vpu->variant->shared_resource)
> +		return v4l2_m2m_init(&vpu_m2m_ops);
> +
> +	scoped_guard(mutex, &shared_dev_lock) {
> +		if (atomic_inc_return(&shared_dev_ref_cnt) == 1) {
> +			shared_m2m_dev = v4l2_m2m_init(&vpu_m2m_ops);
> +			if (IS_ERR(shared_m2m_dev))
> +				atomic_dec(&shared_dev_ref_cnt);
> +		}
> +	}
> +
> +	return shared_m2m_dev;
> +}
> +
> +static void hantro_put_v4l2_m2m_dev(struct hantro_dev *vpu)
> +{
> +	if (!vpu->variant || !vpu->variant->shared_resource)
> +		v4l2_m2m_release(vpu->m2m_dev);
> +
> +	scoped_guard(mutex, &shared_dev_lock) {
> +		if (!atomic_dec_return(&shared_dev_ref_cnt)) {
> +			if (!IS_ERR(shared_m2m_dev)) {
> +				v4l2_m2m_release(shared_m2m_dev);
> +				shared_m2m_dev = NULL;
> +			}
> +		}
> +	}
> +}

Then instead of this, I would like to add a kref to v4l2_m2m_dec, I checked
already and this is both safe and backward compatible.

Then in the get function, you will walk over the compatible that are different
from the currently probe node. If you find one that is initialized, you will
call v4l2_m2m_get() to obtained a shared reference. In _remove() you will simply
do v4l2_m2m_put() instead of v4l2_m2m_release().

Hope the others are happy with this. For Hantro drivers, this will make it a
much more powerfull mechanism.

Nicolas

> +
>  static int hantro_probe(struct platform_device *pdev)
>  {
>  	const struct of_device_id *match;
> @@ -1186,7 +1221,7 @@ static int hantro_probe(struct platform_device *pdev)
>  	}
>  	platform_set_drvdata(pdev, vpu);
>  
> -	vpu->m2m_dev = v4l2_m2m_init(&vpu_m2m_ops);
> +	vpu->m2m_dev = hantro_get_v4l2_m2m_dev(vpu);
>  	if (IS_ERR(vpu->m2m_dev)) {
>  		v4l2_err(&vpu->v4l2_dev, "Failed to init mem2mem device\n");
>  		ret = PTR_ERR(vpu->m2m_dev);
> @@ -1225,7 +1260,7 @@ static int hantro_probe(struct platform_device *pdev)
>  	hantro_remove_enc_func(vpu);
>  err_m2m_rel:
>  	media_device_cleanup(&vpu->mdev);
> -	v4l2_m2m_release(vpu->m2m_dev);
> +	hantro_put_v4l2_m2m_dev(vpu)

>  err_v4l2_unreg:
>  	v4l2_device_unregister(&vpu->v4l2_dev);
>  err_clk_unprepare:
> @@ -1248,7 +1283,7 @@ static void hantro_remove(struct platform_device *pdev)
>  	hantro_remove_dec_func(vpu);
>  	hantro_remove_enc_func(vpu);
>  	media_device_cleanup(&vpu->mdev);
> -	v4l2_m2m_release(vpu->m2m_dev);
> +	hantro_put_v4l2_m2m_dev(vpu);
>  	v4l2_device_unregister(&vpu->v4l2_dev);
>  	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
>  	reset_control_assert(vpu->resets);
> diff --git a/drivers/media/platform/verisilicon/imx8m_vpu_hw.c b/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
> index 5be0e2e76882..5111ce5c36f3 100644
> --- a/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
> +++ b/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
> @@ -356,6 +356,7 @@ const struct hantro_variant imx8mq_vpu_g1_variant = {
>  	.num_irqs = ARRAY_SIZE(imx8mq_irqs),
>  	.clk_names = imx8mq_g1_clk_names,
>  	.num_clocks = ARRAY_SIZE(imx8mq_g1_clk_names),
> +	.shared_resource = 1,
>  };
>  
>  const struct hantro_variant imx8mq_vpu_g2_variant = {
> @@ -371,6 +372,7 @@ const struct hantro_variant imx8mq_vpu_g2_variant = {
>  	.num_irqs = ARRAY_SIZE(imx8mq_g2_irqs),
>  	.clk_names = imx8mq_g2_clk_names,
>  	.num_clocks = ARRAY_SIZE(imx8mq_g2_clk_names),
> +	.shared_resource = 1,
>  };
>  
>  const struct hantro_variant imx8mm_vpu_g1_variant = {

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/2] pmdomain: imx8m-blk-ctrl: Remove separate rst and clk mask for 8mq vpu

[Ming Qian(OSS)]

Hi Lucas,

On 11/28/2025 6:38 PM, Lucas Stach wrote:
> Am Freitag, dem 28.11.2025 um 10:51 +0800 schrieb
> ming.qian@oss.nxp.com:
>> From: Ming Qian <ming.qian@oss.nxp.com>
>>
>> For i.MX8MQ platform, the ADB in the VPUMIX domain has no separate reset
>> and clock enable bits, but is ungated and reset together with the VPUs.
>> So we can't reset G1 or G2 separately, it may led to the system hang.
>> Remove rst_mask and clk_mask of imx8mq_vpu_blk_ctl_domain_data.
>> Let imx8mq_vpu_power_notifier() do really vpu reset.
>>
>> Fixes: 608d7c325e85 ("soc: imx: imx8m-blk-ctrl: add i.MX8MQ VPU blk-ctrl")
>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>> Reviewed-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>> v2
>> - Update commit message
>>
>>   drivers/pmdomain/imx/imx8m-blk-ctrl.c | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/drivers/pmdomain/imx/imx8m-blk-ctrl.c b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
>> index 5c83e5599f1e..1f07ff041295 100644
>> --- a/drivers/pmdomain/imx/imx8m-blk-ctrl.c
>> +++ b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
>> @@ -852,16 +852,12 @@ static const struct imx8m_blk_ctrl_domain_data imx8mq_vpu_blk_ctl_domain_data[]
>>   		.clk_names = (const char *[]){ "g1", },
>>   		.num_clks = 1,
>>   		.gpc_name = "g1",
>> -		.rst_mask = BIT(1),
>> -		.clk_mask = BIT(1),
> 
> Change itself looks okay to me. Can you please leave a small comment
> here and for the G2 domain to document why the clk and reset bits are
> removed, so one doesn't need to dig into the git history when reading
> the driver code?
> 
> Regards,
> Lucas

Sure, will do in v3

Regards,
Ming

> 
>>   	},
>>   	[IMX8MQ_VPUBLK_PD_G2] = {
>>   		.name = "vpublk-g2",
>>   		.clk_names = (const char *[]){ "g2", },
>>   		.num_clks = 1,
>>   		.gpc_name = "g2",
>> -		.rst_mask = BIT(0),
>> -		.clk_mask = BIT(0),
>>   	},
>>   };
>>   
> 

Re: [PATCH v2 2/2] media: verisilicon: Avoid G2 bus error while decoding H.264 and HEVC

[Ming Qian(OSS)]

Hi Nicolas,

On 11/29/2025 5:28 AM, Nicolas Dufresne wrote:
> Hi,
> 
> Le vendredi 28 novembre 2025 à 10:51 +0800, ming.qian@oss.nxp.com a écrit :
>> From: Ming Qian <ming.qian@oss.nxp.com>
>>
>> For the i.MX8MQ platform, there is a hardware limitation: the g1 VPU and
>> g2 VPU cannot decode simultaneously; otherwise, it will cause below bus
>> error and produce corrupted pictures, even led to system hang.
>>
>> [  110.527986] hantro-vpu 38310000.video-codec: frame decode timed out.
>> [  110.583517] hantro-vpu 38310000.video-codec: bus error detected.
>>
>> Therefore, it is necessary to ensure that g1 and g2 operate alternately.
>> Then this allows for successful multi-instance decoding of H.264 and HEVC.
>>
>> To achieve this, we can have g1 and g2 share the same v4l2_m2m_dev, and
>> then the v4l2_m2m_dev can handle the scheduling.
>>
>> Fixes: cb5dd5a0fa518 ("media: hantro: Introduce G2/HEVC decoder")
>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> 
> I like where this is going.
>> ---
>> v2
>> - Abandon the waiting approach.
>> - Switch to a shared v4l2_m2m_dev solution.
>>
>>   drivers/media/platform/verisilicon/hantro.h   |  2 +
>>   .../media/platform/verisilicon/hantro_drv.c   | 41 +++++++++++++++++--
>>   .../media/platform/verisilicon/imx8m_vpu_hw.c |  2 +
>>   3 files changed, 42 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/verisilicon/hantro.h b/drivers/media/platform/verisilicon/hantro.h
>> index e0fdc4535b2d..8a0583f95417 100644
>> --- a/drivers/media/platform/verisilicon/hantro.h
>> +++ b/drivers/media/platform/verisilicon/hantro.h
>> @@ -77,6 +77,7 @@ struct hantro_irq {
>>    * @double_buffer:		core needs double buffering
>>    * @legacy_regs:		core uses legacy register set
>>    * @late_postproc:		postproc must be set up at the end of the job
>> + * @shared_resource:		flag of shared resource
>>    */
>>   struct hantro_variant {
>>   	unsigned int enc_offset;
>> @@ -101,6 +102,7 @@ struct hantro_variant {
>>   	unsigned int double_buffer : 1;
>>   	unsigned int legacy_regs : 1;
>>   	unsigned int late_postproc : 1;
>> +	unsigned int shared_resource : 1;
> 
> Instead of that, I'd make an array of compatible node we are going to share the
> with.
> 
>>   };
>>   
>>   /**
>> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
>> index 60b95b5d8565..f42b2df86806 100644
>> --- a/drivers/media/platform/verisilicon/hantro_drv.c
>> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
>> @@ -35,6 +35,10 @@ module_param_named(debug, hantro_debug, int, 0644);
>>   MODULE_PARM_DESC(debug,
>>   		 "Debug level - higher value produces more verbose messages");
>>   
>> +static DEFINE_MUTEX(shared_dev_lock);
>> +static atomic_t shared_dev_ref_cnt = ATOMIC_INIT(0);
>> +static struct v4l2_m2m_dev *shared_m2m_dev;
>> +
>>   void *hantro_get_ctrl(struct hantro_ctx *ctx, u32 id)
>>   {
>>   	struct v4l2_ctrl *ctrl;
>> @@ -1035,6 +1039,37 @@ static int hantro_disable_multicore(struct hantro_dev *vpu)
>>   	return 0;
>>   }
>>   
>> +static struct v4l2_m2m_dev *hantro_get_v4l2_m2m_dev(struct hantro_dev *vpu)
>> +{
>> +	if (!vpu->variant || !vpu->variant->shared_resource)
>> +		return v4l2_m2m_init(&vpu_m2m_ops);
>> +
>> +	scoped_guard(mutex, &shared_dev_lock) {
>> +		if (atomic_inc_return(&shared_dev_ref_cnt) == 1) {
>> +			shared_m2m_dev = v4l2_m2m_init(&vpu_m2m_ops);
>> +			if (IS_ERR(shared_m2m_dev))
>> +				atomic_dec(&shared_dev_ref_cnt);
>> +		}
>> +	}
>> +
>> +	return shared_m2m_dev;
>> +}
>> +
>> +static void hantro_put_v4l2_m2m_dev(struct hantro_dev *vpu)
>> +{
>> +	if (!vpu->variant || !vpu->variant->shared_resource)
>> +		v4l2_m2m_release(vpu->m2m_dev);
>> +
>> +	scoped_guard(mutex, &shared_dev_lock) {
>> +		if (!atomic_dec_return(&shared_dev_ref_cnt)) {
>> +			if (!IS_ERR(shared_m2m_dev)) {
>> +				v4l2_m2m_release(shared_m2m_dev);
>> +				shared_m2m_dev = NULL;
>> +			}
>> +		}
>> +	}
>> +}
> 
> Then instead of this, I would like to add a kref to v4l2_m2m_dec, I checked
> already and this is both safe and backward compatible.
> 
> Then in the get function, you will walk over the compatible that are different
> from the currently probe node. If you find one that is initialized, you will
> call v4l2_m2m_get() to obtained a shared reference. In _remove() you will simply
> do v4l2_m2m_put() instead of v4l2_m2m_release().
> 
> Hope the others are happy with this. For Hantro drivers, this will make it a
> much more powerfull mechanism.
> 
> Nicolas
> 

For v4l2_m2m_get() and v4l2_m2m_put(), do you mean defining these two
functions in v4l2 m2m, instead of just adding them in the hantro driver?

Regards,
Ming

>> +
>>   static int hantro_probe(struct platform_device *pdev)
>>   {
>>   	const struct of_device_id *match;
>> @@ -1186,7 +1221,7 @@ static int hantro_probe(struct platform_device *pdev)
>>   	}
>>   	platform_set_drvdata(pdev, vpu);
>>   
>> -	vpu->m2m_dev = v4l2_m2m_init(&vpu_m2m_ops);
>> +	vpu->m2m_dev = hantro_get_v4l2_m2m_dev(vpu);
>>   	if (IS_ERR(vpu->m2m_dev)) {
>>   		v4l2_err(&vpu->v4l2_dev, "Failed to init mem2mem device\n");
>>   		ret = PTR_ERR(vpu->m2m_dev);
>> @@ -1225,7 +1260,7 @@ static int hantro_probe(struct platform_device *pdev)
>>   	hantro_remove_enc_func(vpu);
>>   err_m2m_rel:
>>   	media_device_cleanup(&vpu->mdev);
>> -	v4l2_m2m_release(vpu->m2m_dev);
>> +	hantro_put_v4l2_m2m_dev(vpu)
> 
>>   err_v4l2_unreg:
>>   	v4l2_device_unregister(&vpu->v4l2_dev);
>>   err_clk_unprepare:
>> @@ -1248,7 +1283,7 @@ static void hantro_remove(struct platform_device *pdev)
>>   	hantro_remove_dec_func(vpu);
>>   	hantro_remove_enc_func(vpu);
>>   	media_device_cleanup(&vpu->mdev);
>> -	v4l2_m2m_release(vpu->m2m_dev);
>> +	hantro_put_v4l2_m2m_dev(vpu);
>>   	v4l2_device_unregister(&vpu->v4l2_dev);
>>   	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
>>   	reset_control_assert(vpu->resets);
>> diff --git a/drivers/media/platform/verisilicon/imx8m_vpu_hw.c b/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
>> index 5be0e2e76882..5111ce5c36f3 100644
>> --- a/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
>> +++ b/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
>> @@ -356,6 +356,7 @@ const struct hantro_variant imx8mq_vpu_g1_variant = {
>>   	.num_irqs = ARRAY_SIZE(imx8mq_irqs),
>>   	.clk_names = imx8mq_g1_clk_names,
>>   	.num_clocks = ARRAY_SIZE(imx8mq_g1_clk_names),
>> +	.shared_resource = 1,
>>   };
>>   
>>   const struct hantro_variant imx8mq_vpu_g2_variant = {
>> @@ -371,6 +372,7 @@ const struct hantro_variant imx8mq_vpu_g2_variant = {
>>   	.num_irqs = ARRAY_SIZE(imx8mq_g2_irqs),
>>   	.clk_names = imx8mq_g2_clk_names,
>>   	.num_clocks = ARRAY_SIZE(imx8mq_g2_clk_names),
>> +	.shared_resource = 1,
>>   };
>>   
>>   const struct hantro_variant imx8mm_vpu_g1_variant = {

Re: [PATCH v2 2/2] media: verisilicon: Avoid G2 bus error while decoding H.264 and HEVC

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 1511 bytes --]

Hi Ming,

Le lundi 01 décembre 2025 à 09:52 +0800, Ming Qian(OSS) a écrit :
> > Then instead of this, I would like to add a kref to v4l2_m2m_dec, I checked
> > already and this is both safe and backward compatible.
> > 
> > Then in the get function, you will walk over the compatible that are
> > different
> > from the currently probe node. If you find one that is initialized, you will
> > call v4l2_m2m_get() to obtained a shared reference. In _remove() you will
> > simply
> > do v4l2_m2m_put() instead of v4l2_m2m_release().
> > 
> > Hope the others are happy with this. For Hantro drivers, this will make it a
> > much more powerfull mechanism.
> > 
> > Nicolas
> > 
> 
> For v4l2_m2m_get() and v4l2_m2m_put(), do you mean defining these two
> functions in v4l2 m2m, instead of just adding them in the hantro driver?

my idea was to add a kref to be able to share v4l2_m2m_dev. One thing I notice
is that its the v4l2_m2m dev that creates the media controller on all other
drivers. Note sure why Hantro does it own still, maybe it predates, maybe its
something else.

Its quite weirdly glued together, since the v4l2_m2m_release() function just
happily free the m2m_dev without even trying to cleanup the media controller. It
totally orthogonal to the job queue, maybe it shouldn't be there.

I'll make an RFC for the kref anyway, if that has a change, I'll try and split
out the v4l2_m2m_mc_dev on it own, so driver can share a m2m and use the
helpers.

Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 2/2] media: verisilicon: Avoid G2 bus error while decoding H.264 and HEVC

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 6491 bytes --]

Le mardi 02 décembre 2025 à 15:21 -0500, Nicolas Dufresne a écrit :
> Hi Ming,
> 
> Le lundi 01 décembre 2025 à 09:52 +0800, Ming Qian(OSS) a écrit :
> > > Then instead of this, I would like to add a kref to v4l2_m2m_dec, I checked
> > > already and this is both safe and backward compatible.
> > > 
> > > Then in the get function, you will walk over the compatible that are
> > > different
> > > from the currently probe node. If you find one that is initialized, you will
> > > call v4l2_m2m_get() to obtained a shared reference. In _remove() you will
> > > simply
> > > do v4l2_m2m_put() instead of v4l2_m2m_release().
> > > 
> > > Hope the others are happy with this. For Hantro drivers, this will make it a
> > > much more powerfull mechanism.
> > > 
> > > Nicolas
> > > 
> > 
> > For v4l2_m2m_get() and v4l2_m2m_put(), do you mean defining these two
> > functions in v4l2 m2m, instead of just adding them in the hantro driver?
> 
> my idea was to add a kref to be able to share v4l2_m2m_dev. One thing I notice
> is that its the v4l2_m2m dev that creates the media controller on all other
> drivers. Note sure why Hantro does it own still, maybe it predates, maybe its
> something else.
> 
> Its quite weirdly glued together, since the v4l2_m2m_release() function just
> happily free the m2m_dev without even trying to cleanup the media controller. It
> totally orthogonal to the job queue, maybe it shouldn't be there.
> 
> I'll make an RFC for the kref anyway, if that has a change, I'll try and split
> out the v4l2_m2m_mc_dev on it own, so driver can share a m2m and use the
> helpers.
> 
> Nicolas

Here's down below what I had in mind, this is for illustration. I can send a
proper RFC, while at it I will move the media controller part out and port
Hantro to use the helper.

The idea is that instead of using global, you can have a list of devices that
must be combined in the Hantro variant. You then walk the nodes, and if you find
a match, you reference the v4l2_m2m_dev, isntead of creating a new one.

This is slightly more elegant, and can be reused quickly later if we have other
hantro complications like this. One use case could be for modern integration of
the Nano decoder/encoder combo (H1/G1 combo). These are known to share the same
internal SRAM, and cannot be run concurrently. This was defined as a single
device node on RK3588, but I think this badly describe the hardware, and having
two nodes would be more clear.

A bonus would be to describe this dependency in the DT, but since this is
invisible hardware, its pretty hard to design. And in the case of imx8mq, its
not clear this was by design, but likely more an errata that should be added to
the pretty long list for this SoC.

cheers,
Nicolas

---
From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date: Tue, 2 Dec 2025 15:40:37 -0500
Subject: [PATCH] media: v4l2-mem2mem: Add a kref to the v4l2_m2m_dev structure

Adding a reference count to the v4l2_m2m_dev structure allow safely sharing it
across multiple hardware nodes. This can be used to prevent running jobs
concurrently on m2m cores that have some internal resource sharing.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 22 ++++++++++++++++++++++
 include/media/v4l2-mem2mem.h           | 21 +++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index fec93c1a92317..b6940bab641d3 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -90,6 +90,7 @@ static const char * const m2m_entity_name[] = {
  * @job_work:		worker to run queued jobs.
  * @job_queue_flags:	flags of the queue status, %QUEUE_PAUSED.
  * @m2m_ops:		driver callbacks
+ * @kref:		device reference count
  */
 struct v4l2_m2m_dev {
 	struct v4l2_m2m_ctx	*curr_ctx;
@@ -109,6 +110,8 @@ struct v4l2_m2m_dev {
 	unsigned long		job_queue_flags;
 
 	const struct v4l2_m2m_ops *m2m_ops;
+
+	struct kref kref;
 };
 
 static struct v4l2_m2m_queue_ctx *get_queue_ctx(struct v4l2_m2m_ctx *m2m_ctx,
@@ -1200,6 +1203,7 @@ struct v4l2_m2m_dev *v4l2_m2m_init(const struct v4l2_m2m_ops *m2m_ops)
 	INIT_LIST_HEAD(&m2m_dev->job_queue);
 	spin_lock_init(&m2m_dev->job_spinlock);
 	INIT_WORK(&m2m_dev->job_work, v4l2_m2m_device_run_work);
+	kref_init(&m2m_dev->kref);
 
 	return m2m_dev;
 }
@@ -1211,6 +1215,24 @@ void v4l2_m2m_release(struct v4l2_m2m_dev *m2m_dev)
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_release);
 
+void v4l2_m2m_get(struct v4l2_m2m_dev *m2m_dev)
+{
+	kref_get(&m2m_dev->kref);
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_get);
+
+static void v4l2_m2m_release_from_kref(struct kref *kref)
+{
+	struct v4l2_m2m_dev *m2m_dev = container_of(kref, struct v4l2_m2m_dev, kref);
+	v4l2_m2m_release(m2m_dev);
+}
+
+void v4l2_m2m_put(struct v4l2_m2m_dev *m2m_dev)
+{
+	kref_put(&m2m_dev->kref, v4l2_m2m_release_from_kref);
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_put);
+
 struct v4l2_m2m_ctx *v4l2_m2m_ctx_init(struct v4l2_m2m_dev *m2m_dev,
 		void *drv_priv,
 		int (*queue_init)(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq))
diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
index bf6a09a04dcf8..ca295c660c7f9 100644
--- a/include/media/v4l2-mem2mem.h
+++ b/include/media/v4l2-mem2mem.h
@@ -547,6 +547,27 @@ v4l2_m2m_register_media_controller(struct v4l2_m2m_dev *m2m_dev,
  */
 void v4l2_m2m_release(struct v4l2_m2m_dev *m2m_dev);
 
+/**
+ * v4l2_m2m_get() - take a reference to the m2m_dev structure
+ *
+ * @m2m_dev: opaque pointer to the internal data to handle M2M context
+ *
+ * This is used to share the M2M device across multiple devices. This
+ * can be used to avoid scheduling two hardware nodes concurrently.
+ */
+void v4l2_m2m_get(struct v4l2_m2m_dev *m2m_dev);
+
+/**
+ * v4l2_m2m_put() - remove a reference to the m2m_dev structure
+ *
+ * @m2m_dev: opaque pointer to the internal data to handle M2M context
+ *
+ * Once the M2M device have no more references, v4l2_m2m_realse() will be
+ * called automatically. Users of this method should never call
+ * v4l2_m2m_release() directly. See v4l2_m2m_get() for more details.
+ */
+void v4l2_m2m_put(struct v4l2_m2m_dev *m2m_dev);
+
 /**
  * v4l2_m2m_ctx_init() - allocate and initialize a m2m context
  *
-- 
2.51.0

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]