| 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: <46e9a5a881946d879d1b2af3421d574b26256ae3.camel@ndufresne.ca>
Date: Fri, 28 Nov 2025 16:28:39 -0500
From: Nicolas Dufresne <nicolas@...fresne.ca>
To: ming.qian@....nxp.com, 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,
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
> +
> 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 = {
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists