Message-ID: <99f3a6d843afb1c5c47b14e414b917920c3a5041.camel@ndufresne.ca>
Date: Tue, 02 Dec 2025 15:57:43 -0500
From: Nicolas Dufresne <nicolas@...fresne.ca>
To: "Ming Qian(OSS)" <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

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@...labora.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@...labora.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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists