| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <64ad3d66-4edf-4a0b-8992-0db0e90a32b0@oss.nxp.com>
Date: Mon, 1 Dec 2025 09:52:19 +0800
From: "Ming Qian(OSS)" <ming.qian@....nxp.com>
To: Nicolas Dufresne <nicolas@...fresne.ca>, linux-media@...r.kernel.org
Cc: mchehab@...nel.org, hverkuil-cisco@...all.nl,
benjamin.gaignard@...labora.com, p.zabel@...gutronix.de,
sebastian.fricke@...labora.com, shawnguo@...nel.org, ulf.hansson@...aro.org,
s.hauer@...gutronix.de, kernel@...gutronix.de, festevam@...il.com,
linux-imx@....com, l.stach@...gutronix.de, Frank.li@....com,
peng.fan@....com, eagle.zhou@....com, imx@...ts.linux.dev,
linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 2/2] media: verisilicon: Avoid G2 bus error while
decoding H.264 and HEVC
Hi Nicolas,
On 11/29/2025 5:28 AM, Nicolas Dufresne wrote:
> Hi,
>
> Le vendredi 28 novembre 2025 à 10:51 +0800, ming.qian@....nxp.com a écrit :
>> From: Ming Qian <ming.qian@....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@....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 = {
Powered by blists - more mailing lists