[PATCH v3 0/5] Add manual request completion to the MediaTek VCodec driver

[PATCH v3 0/5] Add manual request completion to the MediaTek VCodec driver

[Nicolas Dufresne]

This set introduces the manual request completion API by the author Hans
Verkuil and implements it within the MediaTek VCodec driver.

Why is this needed?

The VCodec driver supports a hardware containing two separate cores, the
CORE and the LAT, these are working in a serial manner without this
series. This series solves two issues, the first being that the current
code runs into a problem, that occurs when the last request object is
unbound from the request, before the v4l2_ctrl_request_complete function
is called, causing an implicit transition to the COMPLETE state.
This issues has been found in applications which didn't attach the
controls for the very first request (which is supposed to enable the
driver to sniff out the correct formats, etc.).
The second issue is that the VCodec driver can not utilize the full
performance of both cores, when the LAT core has to wait for the CORE
core to finishing processing the decode. Thus by enabling the LAT core
to process the next bitstream, right after processing the last we can
increase the performance of the driver.
With the manual request completion API, we can separate the
completion of the request objects of a request and from the completion
of the request itself, which allows to send a new bitstream after the
LAT core has processed the previous and while the CORE core decodes the
previous bitstream, so both cores can work in a parallel manner, but
while keeping the request alive during both steps.

A new state machine for the VCodec driver ensures, that all necessary
processing steps are handled in the correct order depending on the
current step in the execution. This state machine has been added to each
request to ensure that new requests do not alter the state of still
ongoing requests.

Additionally, this series adds a small patch to avoid trying to handle a
scenario, which is not supported by the hardware and thus runs into a
timeout.

Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
Changes in v3:
- Patch 1: Applied suggested documentation fixes to the manual completion API
- Patch 4: Moved MTK VCODEC request helper into the decoder driver
- Patch 4: Matched MTK driver namespaces
- Patch 4: Set MTK request to CORE_DONE state if LAT failed
- Patch 4: Dropped Angelo's Rb, its better to review again
- Link to v2: https://lore.kernel.org/r/20250410-sebastianfricke-vcodec_manual_request_completion_with_state_machine-v2-0-5b99ec0450e6@collabora.com

Changes in v2:
- The implementation patch for V1 was using an outdated version of the
  "media: vcodec: Implement manual request completion" patch, update it
  to the most recent one which doesn't use the state machine globally
  but instead per request, thus having no conflicts between multiple
  concurrent threads
- The kernel test robot found an issue because a function which I only
  use locally was defined without the static keyword
- Link to v1: https://lore.kernel.org/r/20250314-sebastianfricke-vcodec_manual_request_completion_with_state_machine-v1-0-5e277a3d695b@collabora.com

---
Hans Verkuil (3):
      media: mc: add manual request completion
      media: vicodec: add support for manual completion
      media: mc: add debugfs node to keep track of requests

Nicolas Dufresne (1):
      media: mtk-vcodec: Don't try to decode 422/444 VP9

Sebastian Fricke (1):
      media: vcodec: Implement manual request completion

 drivers/media/mc/mc-device.c                       |  30 +++++
 drivers/media/mc/mc-devnode.c                      |   5 +
 drivers/media/mc/mc-request.c                      |  44 ++++++-
 .../mediatek/vcodec/decoder/mtk_vcodec_dec.c       |   4 +-
 .../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h   |  29 ++++
 .../vcodec/decoder/mtk_vcodec_dec_stateless.c      | 146 ++++++++++++++++-----
 drivers/media/test-drivers/vicodec/vicodec-core.c  |  21 ++-
 include/media/media-device.h                       |   9 ++
 include/media/media-devnode.h                      |   4 +
 include/media/media-request.h                      |  40 +++++-
 10 files changed, 292 insertions(+), 40 deletions(-)
---
base-commit: 5e1ff2314797bf53636468a97719a8222deca9ae
change-id: 20250312-sebastianfricke-vcodec_manual_request_completion_with_state_machine-6362c7f80a14

Best regards,
-- 
Nicolas Dufresne <nicolas.dufresne@collabora.com>

[PATCH v3 1/5] media: mc: add manual request completion

[Nicolas Dufresne]

From: Hans Verkuil <hverkuil@xs4all.nl>

By default when the last request object is completed, the whole
request completes as well.

But sometimes you want to delay this completion to an arbitrary point in
time so add a manual complete mode for this.

In req_queue the driver marks the request for manual completion by
calling media_request_mark_manual_completion, and when the driver
wants to manually complete the request it calls
media_request_manual_complete().

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/mc/mc-request.c | 38 ++++++++++++++++++++++++++++++++++++--
 include/media/media-request.h | 38 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 73 insertions(+), 3 deletions(-)

diff --git a/drivers/media/mc/mc-request.c b/drivers/media/mc/mc-request.c
index 5edfc2791ce7c7485def5db675bbf53ee223d837..398d0806d1d274eb8c454fc5c37b77476abe1e74 100644
--- a/drivers/media/mc/mc-request.c
+++ b/drivers/media/mc/mc-request.c
@@ -54,6 +54,7 @@ static void media_request_clean(struct media_request *req)
 	req->access_count = 0;
 	WARN_ON(req->num_incomplete_objects);
 	req->num_incomplete_objects = 0;
+	req->manual_completion = false;
 	wake_up_interruptible_all(&req->poll_wait);
 }
 
@@ -313,6 +314,7 @@ int media_request_alloc(struct media_device *mdev, int *alloc_fd)
 	req->mdev = mdev;
 	req->state = MEDIA_REQUEST_STATE_IDLE;
 	req->num_incomplete_objects = 0;
+	req->manual_completion = false;
 	kref_init(&req->kref);
 	INIT_LIST_HEAD(&req->objects);
 	spin_lock_init(&req->lock);
@@ -459,7 +461,7 @@ void media_request_object_unbind(struct media_request_object *obj)
 
 	req->num_incomplete_objects--;
 	if (req->state == MEDIA_REQUEST_STATE_QUEUED &&
-	    !req->num_incomplete_objects) {
+	    !req->num_incomplete_objects && !req->manual_completion) {
 		req->state = MEDIA_REQUEST_STATE_COMPLETE;
 		completed = true;
 		wake_up_interruptible_all(&req->poll_wait);
@@ -488,7 +490,7 @@ void media_request_object_complete(struct media_request_object *obj)
 	    WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
 		goto unlock;
 
-	if (!--req->num_incomplete_objects) {
+	if (!--req->num_incomplete_objects && !req->manual_completion) {
 		req->state = MEDIA_REQUEST_STATE_COMPLETE;
 		wake_up_interruptible_all(&req->poll_wait);
 		completed = true;
@@ -499,3 +501,35 @@ void media_request_object_complete(struct media_request_object *obj)
 		media_request_put(req);
 }
 EXPORT_SYMBOL_GPL(media_request_object_complete);
+
+void media_request_manual_complete(struct media_request *req)
+{
+	unsigned long flags;
+	bool completed = false;
+
+	if (WARN_ON(!req))
+		return;
+	if (WARN_ON(!req->manual_completion))
+		return;
+
+	spin_lock_irqsave(&req->lock, flags);
+	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
+		goto unlock;
+
+	req->manual_completion = false;
+	/*
+	 * It is expected that all other objects in this request are
+	 * completed when this function is called. WARN if that is
+	 * not the case.
+	 */
+	if (!WARN_ON(req->num_incomplete_objects)) {
+		req->state = MEDIA_REQUEST_STATE_COMPLETE;
+		wake_up_interruptible_all(&req->poll_wait);
+		completed = true;
+	}
+unlock:
+	spin_unlock_irqrestore(&req->lock, flags);
+	if (completed)
+		media_request_put(req);
+}
+EXPORT_SYMBOL_GPL(media_request_manual_complete);
diff --git a/include/media/media-request.h b/include/media/media-request.h
index d4ac557678a78372222704400c8c96cf3150b9d9..7f9af68ef19ac6de0184bbb0c0827dc59777c6dc 100644
--- a/include/media/media-request.h
+++ b/include/media/media-request.h
@@ -56,6 +56,9 @@ struct media_request_object;
  * @access_count: count the number of request accesses that are in progress
  * @objects: List of @struct media_request_object request objects
  * @num_incomplete_objects: The number of incomplete objects in the request
+ * @manual_completion: if true, then the request won't be marked as completed
+ * when @num_incomplete_objects reaches 0. Call media_request_manual_complete()
+ * to complete the request after @num_incomplete_objects == 0.
  * @poll_wait: Wait queue for poll
  * @lock: Serializes access to this struct
  */
@@ -68,6 +71,7 @@ struct media_request {
 	unsigned int access_count;
 	struct list_head objects;
 	unsigned int num_incomplete_objects;
+	bool manual_completion;
 	wait_queue_head_t poll_wait;
 	spinlock_t lock;
 };
@@ -218,6 +222,38 @@ media_request_get_by_fd(struct media_device *mdev, int request_fd);
 int media_request_alloc(struct media_device *mdev,
 			int *alloc_fd);
 
+/**
+ * media_request_mark_manual_completion - Enable manual completion
+ *
+ * @req: The request
+ *
+ * Mark that the request has to be manually completed by calling
+ * media_request_manual_complete().
+ *
+ * This function shall be called in the req_queue callback.
+ */
+static inline void
+media_request_mark_manual_completion(struct media_request *req)
+{
+	req->manual_completion = true;
+}
+
+/**
+ * media_request_manual_complete - Mark the request as completed
+ *
+ * @req: The request
+ *
+ * This function completes a request that was marked for manual completion by an
+ * earlier call to media_request_mark_manual_completion(). The request's
+ * @manual_completion flag is reset to false.
+ *
+ * All objects contained in the request must have been completed previously. It
+ * is an error to call this function otherwise. If such an error occurred, the
+ * function will WARN and the object completion will be delayed until
+ * @num_incomplete_objects is 0.
+ */
+void media_request_manual_complete(struct media_request *req);
+
 #else
 
 static inline void media_request_get(struct media_request *req)
@@ -336,7 +372,7 @@ void media_request_object_init(struct media_request_object *obj);
  * @req: The media request
  * @ops: The object ops for this object
  * @priv: A driver-specific priv pointer associated with this object
- * @is_buffer: Set to true if the object a buffer object.
+ * @is_buffer: Set to true if the object is a buffer object.
  * @obj: The object
  *
  * Bind this object to the request and set the ops and priv values of

-- 
2.49.0

[PATCH v3 2/5] media: vicodec: add support for manual completion

[Nicolas Dufresne]

From: Hans Verkuil <hverkuil@xs4all.nl>

Manually complete the requests: this tests the manual completion
code.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/test-drivers/vicodec/vicodec-core.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/media/test-drivers/vicodec/vicodec-core.c b/drivers/media/test-drivers/vicodec/vicodec-core.c
index c45f5cf12ded3c8b57483b148bf7bbffb8a458c5..88cb6e6a713f08bd4f412ca2940b1309bb87513b 100644
--- a/drivers/media/test-drivers/vicodec/vicodec-core.c
+++ b/drivers/media/test-drivers/vicodec/vicodec-core.c
@@ -448,8 +448,10 @@ static void device_run(void *priv)
 	ctx->comp_magic_cnt = 0;
 	ctx->comp_has_frame = false;
 	spin_unlock(ctx->lock);
-	if (ctx->is_stateless && src_req)
+	if (ctx->is_stateless && src_req) {
 		v4l2_ctrl_request_complete(src_req, &ctx->hdl);
+		media_request_manual_complete(src_req);
+	}
 
 	if (ctx->is_enc)
 		v4l2_m2m_job_finish(dev->stateful_enc.m2m_dev, ctx->fh.m2m_ctx);
@@ -1525,8 +1527,12 @@ static void vicodec_return_bufs(struct vb2_queue *q, u32 state)
 			vbuf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
 		if (vbuf == NULL)
 			return;
-		v4l2_ctrl_request_complete(vbuf->vb2_buf.req_obj.req,
-					   &ctx->hdl);
+		if (ctx->is_stateless && V4L2_TYPE_IS_OUTPUT(q->type)) {
+			struct media_request *req = vbuf->vb2_buf.req_obj.req;
+
+			v4l2_ctrl_request_complete(req, &ctx->hdl);
+			media_request_manual_complete(req);
+		}
 		spin_lock(ctx->lock);
 		v4l2_m2m_buf_done(vbuf, state);
 		spin_unlock(ctx->lock);
@@ -1679,6 +1685,7 @@ static void vicodec_buf_request_complete(struct vb2_buffer *vb)
 	struct vicodec_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
 
 	v4l2_ctrl_request_complete(vb->req_obj.req, &ctx->hdl);
+	media_request_manual_complete(vb->req_obj.req);
 }
 
 
@@ -2010,6 +2017,12 @@ static int vicodec_request_validate(struct media_request *req)
 	return vb2_request_validate(req);
 }
 
+static void vicodec_request_queue(struct media_request *req)
+{
+	media_request_mark_manual_completion(req);
+	v4l2_m2m_request_queue(req);
+}
+
 static const struct v4l2_file_operations vicodec_fops = {
 	.owner		= THIS_MODULE,
 	.open		= vicodec_open,
@@ -2030,7 +2043,7 @@ static const struct video_device vicodec_videodev = {
 
 static const struct media_device_ops vicodec_m2m_media_ops = {
 	.req_validate	= vicodec_request_validate,
-	.req_queue	= v4l2_m2m_request_queue,
+	.req_queue	= vicodec_request_queue,
 };
 
 static const struct v4l2_m2m_ops m2m_ops = {

-- 
2.49.0

[PATCH v3 3/5] media: mc: add debugfs node to keep track of requests

[Nicolas Dufresne]

From: Hans Verkuil <hverkuil@xs4all.nl>

Keep track of the number of requests and request objects of a media
device. Helps to verify that all request-related memory is freed.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/mc/mc-device.c  | 30 ++++++++++++++++++++++++++++++
 drivers/media/mc/mc-devnode.c |  5 +++++
 drivers/media/mc/mc-request.c |  6 ++++++
 include/media/media-device.h  |  9 +++++++++
 include/media/media-devnode.h |  4 ++++
 include/media/media-request.h |  2 ++
 6 files changed, 56 insertions(+)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index c0dd4ae5722725f1744bc6fd6282d5c765438059..5a458160200afb540d8014fed42d8bf2dab9c8c3 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -679,6 +679,23 @@ void media_device_unregister_entity(struct media_entity *entity)
 }
 EXPORT_SYMBOL_GPL(media_device_unregister_entity);
 
+#ifdef CONFIG_DEBUG_FS
+/*
+ * Log the state of media requests.
+ * Very useful for debugging.
+ */
+static int media_device_requests(struct seq_file *file, void *priv)
+{
+	struct media_device *dev = dev_get_drvdata(file->private);
+
+	seq_printf(file, "number of requests: %d\n",
+		   atomic_read(&dev->num_requests));
+	seq_printf(file, "number of request objects: %d\n",
+		   atomic_read(&dev->num_request_objects));
+	return 0;
+}
+#endif
+
 void media_device_init(struct media_device *mdev)
 {
 	INIT_LIST_HEAD(&mdev->entities);
@@ -697,6 +714,9 @@ void media_device_init(struct media_device *mdev)
 		media_set_bus_info(mdev->bus_info, sizeof(mdev->bus_info),
 				   mdev->dev);
 
+	atomic_set(&mdev->num_requests, 0);
+	atomic_set(&mdev->num_request_objects, 0);
+
 	dev_dbg(mdev->dev, "Media device initialized\n");
 }
 EXPORT_SYMBOL_GPL(media_device_init);
@@ -748,6 +768,15 @@ int __must_check __media_device_register(struct media_device *mdev,
 
 	dev_dbg(mdev->dev, "Media device registered\n");
 
+#ifdef CONFIG_DEBUG_FS
+	if (!media_debugfs_root)
+		media_debugfs_root = debugfs_create_dir("media", NULL);
+	mdev->media_dir = debugfs_create_dir(dev_name(&devnode->dev),
+					     media_debugfs_root);
+	debugfs_create_devm_seqfile(&devnode->dev, "requests",
+				    mdev->media_dir, media_device_requests);
+#endif
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(__media_device_register);
@@ -824,6 +853,7 @@ void media_device_unregister(struct media_device *mdev)
 
 	dev_dbg(mdev->dev, "Media device unregistered\n");
 
+	debugfs_remove_recursive(mdev->media_dir);
 	device_remove_file(&mdev->devnode->dev, &dev_attr_model);
 	media_devnode_unregister(mdev->devnode);
 	/* devnode free is handled in media_devnode_*() */
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 56444edaf13651874331e7c04e86b0a585067d38..d0a8bcc11dd6350fdbc04add70f62de2c5f01178 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -45,6 +45,9 @@ static dev_t media_dev_t;
 static DEFINE_MUTEX(media_devnode_lock);
 static DECLARE_BITMAP(media_devnode_nums, MEDIA_NUM_DEVICES);
 
+/* debugfs */
+struct dentry *media_debugfs_root;
+
 /* Called when the last user of the media device exits. */
 static void media_devnode_release(struct device *cd)
 {
@@ -236,6 +239,7 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	if (devnode->parent)
 		devnode->dev.parent = devnode->parent;
 	dev_set_name(&devnode->dev, "media%d", devnode->minor);
+	dev_set_drvdata(&devnode->dev, mdev);
 	device_initialize(&devnode->dev);
 
 	/* Part 2: Initialize the character device */
@@ -313,6 +317,7 @@ static int __init media_devnode_init(void)
 
 static void __exit media_devnode_exit(void)
 {
+	debugfs_remove_recursive(media_debugfs_root);
 	bus_unregister(&media_bus_type);
 	unregister_chrdev_region(media_dev_t, MEDIA_NUM_DEVICES);
 }
diff --git a/drivers/media/mc/mc-request.c b/drivers/media/mc/mc-request.c
index 398d0806d1d274eb8c454fc5c37b77476abe1e74..829e35a5d56d41c52cc583cdea1c959bcb4fce60 100644
--- a/drivers/media/mc/mc-request.c
+++ b/drivers/media/mc/mc-request.c
@@ -75,6 +75,7 @@ static void media_request_release(struct kref *kref)
 		mdev->ops->req_free(req);
 	else
 		kfree(req);
+	atomic_dec(&mdev->num_requests);
 }
 
 void media_request_put(struct media_request *req)
@@ -326,6 +327,7 @@ int media_request_alloc(struct media_device *mdev, int *alloc_fd)
 
 	snprintf(req->debug_str, sizeof(req->debug_str), "%u:%d",
 		 atomic_inc_return(&mdev->request_id), fd);
+	atomic_inc(&mdev->num_requests);
 	dev_dbg(mdev->dev, "request: allocated %s\n", req->debug_str);
 
 	fd_install(fd, filp);
@@ -349,10 +351,12 @@ static void media_request_object_release(struct kref *kref)
 	struct media_request_object *obj =
 		container_of(kref, struct media_request_object, kref);
 	struct media_request *req = obj->req;
+	struct media_device *mdev = obj->mdev;
 
 	if (WARN_ON(req))
 		media_request_object_unbind(obj);
 	obj->ops->release(obj);
+	atomic_dec(&mdev->num_request_objects);
 }
 
 struct media_request_object *
@@ -417,6 +421,7 @@ int media_request_object_bind(struct media_request *req,
 	obj->req = req;
 	obj->ops = ops;
 	obj->priv = priv;
+	obj->mdev = req->mdev;
 
 	if (is_buffer)
 		list_add_tail(&obj->list, &req->objects);
@@ -424,6 +429,7 @@ int media_request_object_bind(struct media_request *req,
 		list_add(&obj->list, &req->objects);
 	req->num_incomplete_objects++;
 	ret = 0;
+	atomic_inc(&obj->mdev->num_request_objects);
 
 unlock:
 	spin_unlock_irqrestore(&req->lock, flags);
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 53d2a16a70b0d9d6e5cc28fe1fc5d5ef384410d5..749c327e3c582c3c583e0394468321ccd6160da5 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -11,6 +11,7 @@
 #ifndef _MEDIA_DEVICE_H
 #define _MEDIA_DEVICE_H
 
+#include <linux/atomic.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
 #include <linux/pci.h>
@@ -106,6 +107,9 @@ struct media_device_ops {
  * @ops:	Operation handler callbacks
  * @req_queue_mutex: Serialise the MEDIA_REQUEST_IOC_QUEUE ioctl w.r.t.
  *		     other operations that stop or start streaming.
+ * @num_requests: number of associated requests
+ * @num_request_objects: number of associated request objects
+ * @media_dir:	DebugFS media directory
  * @request_id: Used to generate unique request IDs
  *
  * This structure represents an abstract high-level media device. It allows easy
@@ -179,6 +183,11 @@ struct media_device {
 	const struct media_device_ops *ops;
 
 	struct mutex req_queue_mutex;
+	atomic_t num_requests;
+	atomic_t num_request_objects;
+
+	/* debugfs */
+	struct dentry *media_dir;
 	atomic_t request_id;
 };
 
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index d27c1c646c2805171be3997d72210dd4d1a38e32..dbcabeffcb572ae707f5fe1f51ff719d451c6784 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -20,9 +20,13 @@
 #include <linux/fs.h>
 #include <linux/device.h>
 #include <linux/cdev.h>
+#include <linux/debugfs.h>
 
 struct media_device;
 
+/* debugfs top-level media directory */
+extern struct dentry *media_debugfs_root;
+
 /*
  * Flag to mark the media_devnode struct as registered. Drivers must not touch
  * this flag directly, it will be set and cleared by media_devnode_register and
diff --git a/include/media/media-request.h b/include/media/media-request.h
index 7f9af68ef19ac6de0184bbb0c0827dc59777c6dc..610ccfe8d7b20ec38e166383433f9ee208248640 100644
--- a/include/media/media-request.h
+++ b/include/media/media-request.h
@@ -292,6 +292,7 @@ struct media_request_object_ops {
  * struct media_request_object - An opaque object that belongs to a media
  *				 request
  *
+ * @mdev: Media device this object belongs to
  * @ops: object's operations
  * @priv: object's priv pointer
  * @req: the request this object belongs to (can be NULL)
@@ -303,6 +304,7 @@ struct media_request_object_ops {
  * another struct that contains the actual data for this request object.
  */
 struct media_request_object {
+	struct media_device *mdev;
 	const struct media_request_object_ops *ops;
 	void *priv;
 	struct media_request *req;

-- 
2.49.0

[PATCH v3 4/5] media: vcodec: Implement manual request completion

[Nicolas Dufresne]

From: Sebastian Fricke <sebastian.fricke@collabora.com>

Rework how requests are completed in the MediaTek VCodec driver, by
implementing the new manual request completion feature, which allows to
keep a request open while allowing to add new bitstream data.
This is useful in this case, because the hardware has a LAT and a core
decode work, after the LAT decode the bitstream isn't required anymore
so the source buffer can be set done and the request stays open until
the core decode work finishes.

Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
Co-developed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 .../mediatek/vcodec/decoder/mtk_vcodec_dec.c       |   4 +-
 .../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h   |  29 +++++
 .../vcodec/decoder/mtk_vcodec_dec_stateless.c      | 140 ++++++++++++++++-----
 3 files changed, 140 insertions(+), 33 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
index 98838217b97d45ed2b5431fdf87c94e0ff79fc57..036ad191a9c3e644fe99b4ce25d6a089292f1e57 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
@@ -889,8 +889,10 @@ void vb2ops_vdec_stop_streaming(struct vb2_queue *q)
 					src_buf->vb2_buf.req_obj.req;
 				v4l2_m2m_buf_done(src_buf,
 						VB2_BUF_STATE_ERROR);
-				if (req)
+				if (req) {
 					v4l2_ctrl_request_complete(req, &ctx->ctrl_hdl);
+					media_request_manual_complete(req);
+				}
 			}
 		}
 		return;
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 aececca7ecf8936bb2b3b55c99354af983746b30..6f9728a95720a257dee9201463c5e275d0586a94 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
@@ -64,6 +64,19 @@ enum mtk_vdec_hw_arch {
 	MTK_VDEC_LAT_SINGLE_CORE,
 };
 
+/**
+ * enum mtk_vdec_request_state - Stages of processing a request
+ * @MTK_VDEC_REQUEST_RECEIVED: Hardware prepared for the LAT decode
+ * @MTK_VDEC_REQUEST_LAT_DONE: LAT decode finished, the bitstream is not
+ *		      needed anymore
+ * @MTK_VDEC_REQUEST_CORE_DONE: CORE decode finished
+ */
+enum mtk_vdec_request_state {
+	MTK_VDEC_REQUEST_RECEIVED = 0,
+	MTK_VDEC_REQUEST_LAT_DONE = 1,
+	MTK_VDEC_REQUEST_CORE_DONE = 2,
+};
+
 /**
  * struct vdec_pic_info  - picture size information
  * @pic_w: picture width
@@ -128,6 +141,17 @@ struct mtk_vcodec_dec_pdata {
 	bool uses_stateless_api;
 };
 
+/**
+ * struct mtk_vcodec_dec_request - Media request private data.
+ *
+ * @req_state: Request completion state
+ * @req: Media Request structure
+ */
+struct mtk_vcodec_dec_request {
+	enum mtk_vdec_request_state req_state;
+	struct media_request req;
+};
+
 /**
  * struct mtk_vcodec_dec_ctx - Context (instance) private data.
  *
@@ -319,6 +343,11 @@ static inline struct mtk_vcodec_dec_ctx *ctrl_to_dec_ctx(struct v4l2_ctrl *ctrl)
 	return container_of(ctrl->handler, struct mtk_vcodec_dec_ctx, ctrl_hdl);
 }
 
+static inline struct mtk_vcodec_dec_request *req_to_dec_req(struct media_request *req)
+{
+	return container_of(req, struct mtk_vcodec_dec_request, req);
+}
+
 /* Wake up context wait_queue */
 static inline void
 wake_up_dec_ctx(struct mtk_vcodec_dec_ctx *ctx, unsigned int reason, unsigned int hw_id)
diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
index d873159b9b3069fe3460502c2751f2e8b2714f44..eea0050eacc1c41abd8e0a1cd546c1ecce90a311 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
@@ -242,10 +242,61 @@ static const struct v4l2_frmsize_stepwise stepwise_fhd = {
 	.step_height = 16
 };
 
+static const char *state_to_str(enum mtk_vdec_request_state state)
+{
+	switch (state) {
+	case MTK_VDEC_REQUEST_RECEIVED:
+		return "RECEIVED";
+	case MTK_VDEC_REQUEST_LAT_DONE:
+		return "LAT_DONE";
+	case MTK_VDEC_REQUEST_CORE_DONE:
+		return "CORE_DONE";
+	default:
+		return "UNKNOWN";
+	}
+}
+
+static void mtk_vcodec_dec_request_complete(struct mtk_vcodec_dec_ctx *ctx,
+					    enum mtk_vdec_request_state state,
+					    enum vb2_buffer_state buffer_state,
+					    struct media_request *src_buf_req)
+{
+	struct mtk_vcodec_dec_request *req = req_to_dec_req(src_buf_req);
+	struct vb2_v4l2_buffer *src_buf, *dst_buf;
+
+	mutex_lock(&ctx->lock);
+
+	if (req->req_state >= state) {
+		mutex_unlock(&ctx->lock);
+		return;
+	}
+
+	switch (req->req_state) {
+	case MTK_VDEC_REQUEST_RECEIVED:
+		v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
+		src_buf = v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
+		v4l2_m2m_buf_done(src_buf, buffer_state);
+		if (state == MTK_VDEC_REQUEST_LAT_DONE)
+			break;
+		fallthrough;
+	case MTK_VDEC_REQUEST_LAT_DONE:
+		dst_buf = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
+		v4l2_m2m_buf_done(dst_buf, buffer_state);
+		media_request_manual_complete(src_buf_req);
+		break;
+	default:
+		break;
+	}
+
+	mtk_v4l2_vdec_dbg(3, ctx, "Switch state from %s to %s.\n",
+			  state_to_str(req->req_state), state_to_str(state));
+	req->req_state = state;
+	mutex_unlock(&ctx->lock);
+}
+
 static void mtk_vdec_stateless_cap_to_disp(struct mtk_vcodec_dec_ctx *ctx, int error,
 					   struct media_request *src_buf_req)
 {
-	struct vb2_v4l2_buffer *vb2_dst;
 	enum vb2_buffer_state state;
 
 	if (error)
@@ -253,17 +304,7 @@ static void mtk_vdec_stateless_cap_to_disp(struct mtk_vcodec_dec_ctx *ctx, int e
 	else
 		state = VB2_BUF_STATE_DONE;
 
-	vb2_dst = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
-	if (vb2_dst) {
-		v4l2_m2m_buf_done(vb2_dst, state);
-		mtk_v4l2_vdec_dbg(2, ctx, "free frame buffer id:%d to done list",
-				  vb2_dst->vb2_buf.index);
-	} else {
-		mtk_v4l2_vdec_err(ctx, "dst buffer is NULL");
-	}
-
-	if (src_buf_req)
-		v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
+	mtk_vcodec_dec_request_complete(ctx, MTK_VDEC_REQUEST_CORE_DONE, state, src_buf_req);
 }
 
 static struct vdec_fb *vdec_get_cap_buffer(struct mtk_vcodec_dec_ctx *ctx)
@@ -306,6 +347,7 @@ static void vb2ops_vdec_buf_request_complete(struct vb2_buffer *vb)
 	struct mtk_vcodec_dec_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
 
 	v4l2_ctrl_request_complete(vb->req_obj.req, &ctx->ctrl_hdl);
+	media_request_manual_complete(vb->req_obj.req);
 }
 
 static void mtk_vdec_worker(struct work_struct *work)
@@ -318,7 +360,8 @@ static void mtk_vdec_worker(struct work_struct *work)
 	struct mtk_vcodec_mem *bs_src;
 	struct mtk_video_dec_buf *dec_buf_src;
 	struct media_request *src_buf_req;
-	enum vb2_buffer_state state;
+	enum mtk_vdec_request_state req_state;
+	enum vb2_buffer_state buf_state;
 	bool res_chg = false;
 	int ret;
 
@@ -351,37 +394,43 @@ static void mtk_vdec_worker(struct work_struct *work)
 			  ctx->id, bs_src->va, &bs_src->dma_addr, bs_src->size, vb2_src);
 	/* Apply request controls. */
 	src_buf_req = vb2_src->req_obj.req;
-	if (src_buf_req)
-		v4l2_ctrl_request_setup(src_buf_req, &ctx->ctrl_hdl);
-	else
+	if (WARN_ON(!src_buf_req)) {
+		v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
 		mtk_v4l2_vdec_err(ctx, "vb2 buffer media request is NULL");
+		return;
+	}
+	v4l2_ctrl_request_setup(src_buf_req, &ctx->ctrl_hdl);
 
 	ret = vdec_if_decode(ctx, bs_src, NULL, &res_chg);
-	if (ret && ret != -EAGAIN) {
+
+	if (ret == -EAGAIN) {
+		buf_state = VB2_BUF_STATE_DONE;
+		req_state = MTK_VDEC_REQUEST_RECEIVED;
+	} else if (ret) {
 		mtk_v4l2_vdec_err(ctx,
-				  "[%d] decode src_buf[%d] sz=0x%zx pts=%llu ret=%d res_chg=%d",
+				  "[%d] decode src_buf[%d] sz=0x%zx pts=%llu res_chg=%d ret=%d",
 				  ctx->id, vb2_src->index, bs_src->size,
-				  vb2_src->timestamp, ret, res_chg);
+				  vb2_src->timestamp, res_chg, ret);
 		if (ret == -EIO) {
 			mutex_lock(&ctx->lock);
 			dec_buf_src->error = true;
 			mutex_unlock(&ctx->lock);
 		}
-	}
 
-	state = ret ? VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE;
-	if (!IS_VDEC_LAT_ARCH(dev->vdec_pdata->hw_arch) ||
-	    ctx->current_codec == V4L2_PIX_FMT_VP8_FRAME) {
-		v4l2_m2m_buf_done_and_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx, state);
-		if (src_buf_req)
-			v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
+		buf_state = VB2_BUF_STATE_ERROR;
+		req_state = MTK_VDEC_REQUEST_CORE_DONE;
 	} else {
-		if (ret != -EAGAIN) {
-			v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
-			v4l2_m2m_buf_done(vb2_v4l2_src, state);
-		}
-		v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
+		buf_state = VB2_BUF_STATE_DONE;
+
+		if (!IS_VDEC_LAT_ARCH(dev->vdec_pdata->hw_arch) ||
+		    ctx->current_codec == V4L2_PIX_FMT_VP8_FRAME)
+			req_state = MTK_VDEC_REQUEST_CORE_DONE;
+		else
+			req_state = MTK_VDEC_REQUEST_LAT_DONE;
 	}
+
+	mtk_vcodec_dec_request_complete(ctx, req_state, buf_state, src_buf_req);
+	v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
 }
 
 static void vb2ops_vdec_stateless_buf_queue(struct vb2_buffer *vb)
@@ -709,6 +758,22 @@ static int mtk_vcodec_dec_ctrls_setup(struct mtk_vcodec_dec_ctx *ctx)
 	return 0;
 }
 
+static struct media_request *fops_media_request_alloc(struct media_device *mdev)
+{
+	struct mtk_vcodec_dec_request *req;
+
+	req = kzalloc(sizeof(*req), GFP_KERNEL);
+
+	return &req->req;
+}
+
+static void fops_media_request_free(struct media_request *mreq)
+{
+	struct mtk_vcodec_dec_request *req = req_to_dec_req(mreq);
+
+	kfree(req);
+}
+
 static int fops_media_request_validate(struct media_request *mreq)
 {
 	const unsigned int buffer_cnt = vb2_request_buffer_cnt(mreq);
@@ -729,9 +794,20 @@ static int fops_media_request_validate(struct media_request *mreq)
 	return vb2_request_validate(mreq);
 }
 
+static void fops_media_request_queue(struct media_request *mreq)
+{
+	struct mtk_vcodec_dec_request *req = req_to_dec_req(mreq);
+
+	media_request_mark_manual_completion(mreq);
+	req->req_state = MTK_VDEC_REQUEST_RECEIVED;
+	v4l2_m2m_request_queue(mreq);
+}
+
 const struct media_device_ops mtk_vcodec_media_ops = {
+	.req_alloc      = fops_media_request_alloc,
+	.req_free      = fops_media_request_free,
 	.req_validate	= fops_media_request_validate,
-	.req_queue	= v4l2_m2m_request_queue,
+	.req_queue	= fops_media_request_queue,
 };
 
 static void mtk_vcodec_add_formats(unsigned int fourcc,

-- 
2.49.0

[PATCH v3 5/5] media: mtk-vcodec: Don't try to decode 422/444 VP9

[Nicolas Dufresne]

This is not supported by the hardware and trying to decode
these leads to LAT timeout errors.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 .../platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c     | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
index eea0050eacc1c41abd8e0a1cd546c1ecce90a311..65bd344a12f65d4cb3a81d34f41710f0089a2349 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
@@ -551,6 +551,12 @@ static int mtk_vdec_s_ctrl(struct v4l2_ctrl *ctrl)
 			mtk_v4l2_vdec_err(ctx, "VP9: bit_depth:%d", frame->bit_depth);
 			return -EINVAL;
 		}
+
+		if (!(frame->flags & V4L2_VP9_FRAME_FLAG_X_SUBSAMPLING) ||
+		    !(frame->flags & V4L2_VP9_FRAME_FLAG_Y_SUBSAMPLING)) {
+			mtk_v4l2_vdec_err(ctx, "VP9: only 420 subsampling is supported");
+			return -EINVAL;
+		}
 		break;
 	case V4L2_CID_STATELESS_AV1_SEQUENCE:
 		seq = (struct v4l2_ctrl_av1_sequence *)hdr_ctrl->p_new.p;

-- 
2.49.0

Re: [PATCH v3 1/5] media: mc: add manual request completion

[Sakari Ailus]

Hi Nicolas,

Thanks for the update.

On Wed, Jun 04, 2025 at 04:09:35PM -0400, Nicolas Dufresne wrote:
> From: Hans Verkuil <hverkuil@xs4all.nl>
> 
> By default when the last request object is completed, the whole
> request completes as well.
> 
> But sometimes you want to delay this completion to an arbitrary point in
> time so add a manual complete mode for this.
> 
> In req_queue the driver marks the request for manual completion by
> calling media_request_mark_manual_completion, and when the driver
> wants to manually complete the request it calls
> media_request_manual_complete().
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
>  drivers/media/mc/mc-request.c | 38 ++++++++++++++++++++++++++++++++++++--
>  include/media/media-request.h | 38 +++++++++++++++++++++++++++++++++++++-
>  2 files changed, 73 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/mc/mc-request.c b/drivers/media/mc/mc-request.c
> index 5edfc2791ce7c7485def5db675bbf53ee223d837..398d0806d1d274eb8c454fc5c37b77476abe1e74 100644
> --- a/drivers/media/mc/mc-request.c
> +++ b/drivers/media/mc/mc-request.c
> @@ -54,6 +54,7 @@ static void media_request_clean(struct media_request *req)
>  	req->access_count = 0;
>  	WARN_ON(req->num_incomplete_objects);
>  	req->num_incomplete_objects = 0;
> +	req->manual_completion = false;
>  	wake_up_interruptible_all(&req->poll_wait);
>  }
>  
> @@ -313,6 +314,7 @@ int media_request_alloc(struct media_device *mdev, int *alloc_fd)
>  	req->mdev = mdev;
>  	req->state = MEDIA_REQUEST_STATE_IDLE;
>  	req->num_incomplete_objects = 0;
> +	req->manual_completion = false;
>  	kref_init(&req->kref);
>  	INIT_LIST_HEAD(&req->objects);
>  	spin_lock_init(&req->lock);
> @@ -459,7 +461,7 @@ void media_request_object_unbind(struct media_request_object *obj)
>  
>  	req->num_incomplete_objects--;
>  	if (req->state == MEDIA_REQUEST_STATE_QUEUED &&
> -	    !req->num_incomplete_objects) {
> +	    !req->num_incomplete_objects && !req->manual_completion) {
>  		req->state = MEDIA_REQUEST_STATE_COMPLETE;
>  		completed = true;
>  		wake_up_interruptible_all(&req->poll_wait);
> @@ -488,7 +490,7 @@ void media_request_object_complete(struct media_request_object *obj)
>  	    WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
>  		goto unlock;
>  
> -	if (!--req->num_incomplete_objects) {
> +	if (!--req->num_incomplete_objects && !req->manual_completion) {
>  		req->state = MEDIA_REQUEST_STATE_COMPLETE;
>  		wake_up_interruptible_all(&req->poll_wait);
>  		completed = true;
> @@ -499,3 +501,35 @@ void media_request_object_complete(struct media_request_object *obj)
>  		media_request_put(req);
>  }
>  EXPORT_SYMBOL_GPL(media_request_object_complete);
> +
> +void media_request_manual_complete(struct media_request *req)
> +{
> +	unsigned long flags;

I'd declare flags as last.

> +	bool completed = false;
> +
> +	if (WARN_ON(!req))
> +		return;
> +	if (WARN_ON(!req->manual_completion))
> +		return;

I think I'd use WARN_ON_ONCE() consistently: this is a driver (or
framework) bug and telling once about it is very probably enough.

> +
> +	spin_lock_irqsave(&req->lock, flags);
> +	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
> +		goto unlock;
> +
> +	req->manual_completion = false;
> +	/*
> +	 * It is expected that all other objects in this request are
> +	 * completed when this function is called. WARN if that is
> +	 * not the case.
> +	 */
> +	if (!WARN_ON(req->num_incomplete_objects)) {
> +		req->state = MEDIA_REQUEST_STATE_COMPLETE;
> +		wake_up_interruptible_all(&req->poll_wait);
> +		completed = true;
> +	}

A newline would be nice here.

> +unlock:
> +	spin_unlock_irqrestore(&req->lock, flags);
> +	if (completed)
> +		media_request_put(req);
> +}
> +EXPORT_SYMBOL_GPL(media_request_manual_complete);
> diff --git a/include/media/media-request.h b/include/media/media-request.h
> index d4ac557678a78372222704400c8c96cf3150b9d9..7f9af68ef19ac6de0184bbb0c0827dc59777c6dc 100644
> --- a/include/media/media-request.h
> +++ b/include/media/media-request.h
> @@ -56,6 +56,9 @@ struct media_request_object;
>   * @access_count: count the number of request accesses that are in progress
>   * @objects: List of @struct media_request_object request objects
>   * @num_incomplete_objects: The number of incomplete objects in the request
> + * @manual_completion: if true, then the request won't be marked as completed
> + * when @num_incomplete_objects reaches 0. Call media_request_manual_complete()
> + * to complete the request after @num_incomplete_objects == 0.
>   * @poll_wait: Wait queue for poll
>   * @lock: Serializes access to this struct
>   */
> @@ -68,6 +71,7 @@ struct media_request {
>  	unsigned int access_count;
>  	struct list_head objects;
>  	unsigned int num_incomplete_objects;
> +	bool manual_completion;
>  	wait_queue_head_t poll_wait;
>  	spinlock_t lock;
>  };
> @@ -218,6 +222,38 @@ media_request_get_by_fd(struct media_device *mdev, int request_fd);
>  int media_request_alloc(struct media_device *mdev,
>  			int *alloc_fd);
>  
> +/**
> + * media_request_mark_manual_completion - Enable manual completion
> + *
> + * @req: The request
> + *
> + * Mark that the request has to be manually completed by calling
> + * media_request_manual_complete().
> + *
> + * This function shall be called in the req_queue callback.
> + */
> +static inline void
> +media_request_mark_manual_completion(struct media_request *req)
> +{
> +	req->manual_completion = true;
> +}
> +
> +/**
> + * media_request_manual_complete - Mark the request as completed
> + *
> + * @req: The request
> + *
> + * This function completes a request that was marked for manual completion by an
> + * earlier call to media_request_mark_manual_completion(). The request's
> + * @manual_completion flag is reset to false.

s/flag/field/

> + *
> + * All objects contained in the request must have been completed previously. It
> + * is an error to call this function otherwise. If such an error occurred, the
> + * function will WARN and the object completion will be delayed until
> + * @num_incomplete_objects is 0.
> + */
> +void media_request_manual_complete(struct media_request *req);
> +
>  #else
>  
>  static inline void media_request_get(struct media_request *req)
> @@ -336,7 +372,7 @@ void media_request_object_init(struct media_request_object *obj);
>   * @req: The media request
>   * @ops: The object ops for this object
>   * @priv: A driver-specific priv pointer associated with this object
> - * @is_buffer: Set to true if the object a buffer object.
> + * @is_buffer: Set to true if the object is a buffer object.
>   * @obj: The object
>   *
>   * Bind this object to the request and set the ops and priv values of
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 3/5] media: mc: add debugfs node to keep track of requests

[Sakari Ailus]

Hi Nicolas, Hans,

Thanks for the update.

On Wed, Jun 04, 2025 at 04:09:37PM -0400, Nicolas Dufresne wrote:
> From: Hans Verkuil <hverkuil@xs4all.nl>
> 
> Keep track of the number of requests and request objects of a media
> device. Helps to verify that all request-related memory is freed.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
>  drivers/media/mc/mc-device.c  | 30 ++++++++++++++++++++++++++++++
>  drivers/media/mc/mc-devnode.c |  5 +++++
>  drivers/media/mc/mc-request.c |  6 ++++++
>  include/media/media-device.h  |  9 +++++++++
>  include/media/media-devnode.h |  4 ++++
>  include/media/media-request.h |  2 ++
>  6 files changed, 56 insertions(+)
> 
> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> index c0dd4ae5722725f1744bc6fd6282d5c765438059..5a458160200afb540d8014fed42d8bf2dab9c8c3 100644
> --- a/drivers/media/mc/mc-device.c
> +++ b/drivers/media/mc/mc-device.c
> @@ -679,6 +679,23 @@ void media_device_unregister_entity(struct media_entity *entity)
>  }
>  EXPORT_SYMBOL_GPL(media_device_unregister_entity);
>  
> +#ifdef CONFIG_DEBUG_FS
> +/*
> + * Log the state of media requests.
> + * Very useful for debugging.
> + */

Fits on a single line.

> +static int media_device_requests(struct seq_file *file, void *priv)
> +{
> +	struct media_device *dev = dev_get_drvdata(file->private);
> +
> +	seq_printf(file, "number of requests: %d\n",
> +		   atomic_read(&dev->num_requests));
> +	seq_printf(file, "number of request objects: %d\n",
> +		   atomic_read(&dev->num_request_objects));

Newline here?

> +	return 0;
> +}
> +#endif
> +
>  void media_device_init(struct media_device *mdev)
>  {
>  	INIT_LIST_HEAD(&mdev->entities);
> @@ -697,6 +714,9 @@ void media_device_init(struct media_device *mdev)
>  		media_set_bus_info(mdev->bus_info, sizeof(mdev->bus_info),
>  				   mdev->dev);
>  
> +	atomic_set(&mdev->num_requests, 0);
> +	atomic_set(&mdev->num_request_objects, 0);
> +
>  	dev_dbg(mdev->dev, "Media device initialized\n");
>  }
>  EXPORT_SYMBOL_GPL(media_device_init);
> @@ -748,6 +768,15 @@ int __must_check __media_device_register(struct media_device *mdev,
>  
>  	dev_dbg(mdev->dev, "Media device registered\n");
>  
> +#ifdef CONFIG_DEBUG_FS
> +	if (!media_debugfs_root)
> +		media_debugfs_root = debugfs_create_dir("media", NULL);
> +	mdev->media_dir = debugfs_create_dir(dev_name(&devnode->dev),
> +					     media_debugfs_root);
> +	debugfs_create_devm_seqfile(&devnode->dev, "requests",
> +				    mdev->media_dir, media_device_requests);
> +#endif

I have no objection to this but it would have been great to have the Media
device lifetime set in first and MC device and devnode merged. But maybe
it's too late for that. Well, at least this won't change error handling...

> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(__media_device_register);
> @@ -824,6 +853,7 @@ void media_device_unregister(struct media_device *mdev)
>  
>  	dev_dbg(mdev->dev, "Media device unregistered\n");
>  
> +	debugfs_remove_recursive(mdev->media_dir);
>  	device_remove_file(&mdev->devnode->dev, &dev_attr_model);
>  	media_devnode_unregister(mdev->devnode);
>  	/* devnode free is handled in media_devnode_*() */
> diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
> index 56444edaf13651874331e7c04e86b0a585067d38..d0a8bcc11dd6350fdbc04add70f62de2c5f01178 100644
> --- a/drivers/media/mc/mc-devnode.c
> +++ b/drivers/media/mc/mc-devnode.c
> @@ -45,6 +45,9 @@ static dev_t media_dev_t;
>  static DEFINE_MUTEX(media_devnode_lock);
>  static DECLARE_BITMAP(media_devnode_nums, MEDIA_NUM_DEVICES);
>  
> +/* debugfs */
> +struct dentry *media_debugfs_root;
> +
>  /* Called when the last user of the media device exits. */
>  static void media_devnode_release(struct device *cd)
>  {
> @@ -236,6 +239,7 @@ int __must_check media_devnode_register(struct media_device *mdev,
>  	if (devnode->parent)
>  		devnode->dev.parent = devnode->parent;
>  	dev_set_name(&devnode->dev, "media%d", devnode->minor);
> +	dev_set_drvdata(&devnode->dev, mdev);
>  	device_initialize(&devnode->dev);
>  
>  	/* Part 2: Initialize the character device */
> @@ -313,6 +317,7 @@ static int __init media_devnode_init(void)
>  
>  static void __exit media_devnode_exit(void)
>  {
> +	debugfs_remove_recursive(media_debugfs_root);
>  	bus_unregister(&media_bus_type);
>  	unregister_chrdev_region(media_dev_t, MEDIA_NUM_DEVICES);
>  }
> diff --git a/drivers/media/mc/mc-request.c b/drivers/media/mc/mc-request.c
> index 398d0806d1d274eb8c454fc5c37b77476abe1e74..829e35a5d56d41c52cc583cdea1c959bcb4fce60 100644
> --- a/drivers/media/mc/mc-request.c
> +++ b/drivers/media/mc/mc-request.c
> @@ -75,6 +75,7 @@ static void media_request_release(struct kref *kref)
>  		mdev->ops->req_free(req);
>  	else
>  		kfree(req);
> +	atomic_dec(&mdev->num_requests);
>  }
>  
>  void media_request_put(struct media_request *req)
> @@ -326,6 +327,7 @@ int media_request_alloc(struct media_device *mdev, int *alloc_fd)
>  
>  	snprintf(req->debug_str, sizeof(req->debug_str), "%u:%d",
>  		 atomic_inc_return(&mdev->request_id), fd);
> +	atomic_inc(&mdev->num_requests);
>  	dev_dbg(mdev->dev, "request: allocated %s\n", req->debug_str);
>  
>  	fd_install(fd, filp);
> @@ -349,10 +351,12 @@ static void media_request_object_release(struct kref *kref)
>  	struct media_request_object *obj =
>  		container_of(kref, struct media_request_object, kref);
>  	struct media_request *req = obj->req;
> +	struct media_device *mdev = obj->mdev;
>  
>  	if (WARN_ON(req))
>  		media_request_object_unbind(obj);
>  	obj->ops->release(obj);
> +	atomic_dec(&mdev->num_request_objects);
>  }
>  
>  struct media_request_object *
> @@ -417,6 +421,7 @@ int media_request_object_bind(struct media_request *req,
>  	obj->req = req;
>  	obj->ops = ops;
>  	obj->priv = priv;
> +	obj->mdev = req->mdev;
>  
>  	if (is_buffer)
>  		list_add_tail(&obj->list, &req->objects);
> @@ -424,6 +429,7 @@ int media_request_object_bind(struct media_request *req,
>  		list_add(&obj->list, &req->objects);
>  	req->num_incomplete_objects++;
>  	ret = 0;
> +	atomic_inc(&obj->mdev->num_request_objects);
>  
>  unlock:
>  	spin_unlock_irqrestore(&req->lock, flags);
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index 53d2a16a70b0d9d6e5cc28fe1fc5d5ef384410d5..749c327e3c582c3c583e0394468321ccd6160da5 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -11,6 +11,7 @@
>  #ifndef _MEDIA_DEVICE_H
>  #define _MEDIA_DEVICE_H
>  
> +#include <linux/atomic.h>
>  #include <linux/list.h>
>  #include <linux/mutex.h>
>  #include <linux/pci.h>
> @@ -106,6 +107,9 @@ struct media_device_ops {
>   * @ops:	Operation handler callbacks
>   * @req_queue_mutex: Serialise the MEDIA_REQUEST_IOC_QUEUE ioctl w.r.t.
>   *		     other operations that stop or start streaming.
> + * @num_requests: number of associated requests
> + * @num_request_objects: number of associated request objects
> + * @media_dir:	DebugFS media directory
>   * @request_id: Used to generate unique request IDs
>   *
>   * This structure represents an abstract high-level media device. It allows easy
> @@ -179,6 +183,11 @@ struct media_device {
>  	const struct media_device_ops *ops;
>  
>  	struct mutex req_queue_mutex;
> +	atomic_t num_requests;
> +	atomic_t num_request_objects;
> +
> +	/* debugfs */
> +	struct dentry *media_dir;
>  	atomic_t request_id;
>  };
>  
> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
> index d27c1c646c2805171be3997d72210dd4d1a38e32..dbcabeffcb572ae707f5fe1f51ff719d451c6784 100644
> --- a/include/media/media-devnode.h
> +++ b/include/media/media-devnode.h
> @@ -20,9 +20,13 @@
>  #include <linux/fs.h>
>  #include <linux/device.h>
>  #include <linux/cdev.h>
> +#include <linux/debugfs.h>
>  
>  struct media_device;
>  
> +/* debugfs top-level media directory */
> +extern struct dentry *media_debugfs_root;
> +
>  /*
>   * Flag to mark the media_devnode struct as registered. Drivers must not touch
>   * this flag directly, it will be set and cleared by media_devnode_register and
> diff --git a/include/media/media-request.h b/include/media/media-request.h
> index 7f9af68ef19ac6de0184bbb0c0827dc59777c6dc..610ccfe8d7b20ec38e166383433f9ee208248640 100644
> --- a/include/media/media-request.h
> +++ b/include/media/media-request.h
> @@ -292,6 +292,7 @@ struct media_request_object_ops {
>   * struct media_request_object - An opaque object that belongs to a media
>   *				 request
>   *
> + * @mdev: Media device this object belongs to

This deserves at least a comment what this may be used for: generally once
object is unbound, it's not related to a request anymore (nor a Media
device). This field also adds a new Media device lifetime issue: nothing
guarantees the mdev is not disappearing at a wrong time albeit this is
very, very likely not user-triggerable without physically removing
hardware.

>   * @ops: object's operations
>   * @priv: object's priv pointer
>   * @req: the request this object belongs to (can be NULL)
> @@ -303,6 +304,7 @@ struct media_request_object_ops {
>   * another struct that contains the actual data for this request object.
>   */
>  struct media_request_object {
> +	struct media_device *mdev;
>  	const struct media_request_object_ops *ops;
>  	void *priv;
>  	struct media_request *req;
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 3/5] media: mc: add debugfs node to keep track of requests

[Nicolas Dufresne]

Le mercredi 04 juin 2025 à 21:33 +0000, Sakari Ailus a écrit :
> Hi Nicolas, Hans,
> 
> Thanks for the update.

thanks for the review, these things are precious.

> 
> On Wed, Jun 04, 2025 at 04:09:37PM -0400, Nicolas Dufresne wrote:
> > From: Hans Verkuil <hverkuil@xs4all.nl>
> > 
> > Keep track of the number of requests and request objects of a media
> > device. Helps to verify that all request-related memory is freed.
> > 
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> >  drivers/media/mc/mc-device.c  | 30 ++++++++++++++++++++++++++++++
> >  drivers/media/mc/mc-devnode.c |  5 +++++
> >  drivers/media/mc/mc-request.c |  6 ++++++
> >  include/media/media-device.h  |  9 +++++++++
> >  include/media/media-devnode.h |  4 ++++
> >  include/media/media-request.h |  2 ++
> >  6 files changed, 56 insertions(+)
> > 
> > diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> > index c0dd4ae5722725f1744bc6fd6282d5c765438059..5a458160200afb540d8014fed42d8bf2dab9c8c3 100644
> > --- a/drivers/media/mc/mc-device.c
> > +++ b/drivers/media/mc/mc-device.c
> > @@ -679,6 +679,23 @@ void media_device_unregister_entity(struct media_entity *entity)
> >  }
> >  EXPORT_SYMBOL_GPL(media_device_unregister_entity);
> >  
> > +#ifdef CONFIG_DEBUG_FS
> > +/*
> > + * Log the state of media requests.
> > + * Very useful for debugging.
> > + */
> 
> Fits on a single line.

Ack.

> 
> > +static int media_device_requests(struct seq_file *file, void *priv)
> > +{
> > +	struct media_device *dev = dev_get_drvdata(file->private);
> > +
> > +	seq_printf(file, "number of requests: %d\n",
> > +		   atomic_read(&dev->num_requests));
> > +	seq_printf(file, "number of request objects: %d\n",
> > +		   atomic_read(&dev->num_request_objects));
> 
> Newline here?

I prefer that too.

> 
> > +	return 0;
> > +}
> > +#endif
> > +
> >  void media_device_init(struct media_device *mdev)
> >  {
> >  	INIT_LIST_HEAD(&mdev->entities);
> > @@ -697,6 +714,9 @@ void media_device_init(struct media_device *mdev)
> >  		media_set_bus_info(mdev->bus_info, sizeof(mdev->bus_info),
> >  				   mdev->dev);
> >  
> > +	atomic_set(&mdev->num_requests, 0);
> > +	atomic_set(&mdev->num_request_objects, 0);
> > +
> >  	dev_dbg(mdev->dev, "Media device initialized\n");
> >  }
> >  EXPORT_SYMBOL_GPL(media_device_init);
> > @@ -748,6 +768,15 @@ int __must_check __media_device_register(struct media_device *mdev,
> >  
> >  	dev_dbg(mdev->dev, "Media device registered\n");
> >  
> > +#ifdef CONFIG_DEBUG_FS
> > +	if (!media_debugfs_root)
> > +		media_debugfs_root = debugfs_create_dir("media", NULL);
> > +	mdev->media_dir = debugfs_create_dir(dev_name(&devnode->dev),
> > +					     media_debugfs_root);
> > +	debugfs_create_devm_seqfile(&devnode->dev, "requests",
> > +				    mdev->media_dir, media_device_requests);
> > +#endif
> 
> I have no objection to this but it would have been great to have the Media
> device lifetime set in first and MC device and devnode merged. But maybe
> it's too late for that. Well, at least this won't change error handling...

Since this specific patch is not required to fix the MTK VCODEC issue, I can
delay this a little. Is that comment related to an existing patch ?

> 
> > +
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(__media_device_register);
> > @@ -824,6 +853,7 @@ void media_device_unregister(struct media_device *mdev)
> >  
> >  	dev_dbg(mdev->dev, "Media device unregistered\n");
> >  
> > +	debugfs_remove_recursive(mdev->media_dir);
> >  	device_remove_file(&mdev->devnode->dev, &dev_attr_model);
> >  	media_devnode_unregister(mdev->devnode);
> >  	/* devnode free is handled in media_devnode_*() */
> > diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
> > index 56444edaf13651874331e7c04e86b0a585067d38..d0a8bcc11dd6350fdbc04add70f62de2c5f01178 100644
> > --- a/drivers/media/mc/mc-devnode.c
> > +++ b/drivers/media/mc/mc-devnode.c
> > @@ -45,6 +45,9 @@ static dev_t media_dev_t;
> >  static DEFINE_MUTEX(media_devnode_lock);
> >  static DECLARE_BITMAP(media_devnode_nums, MEDIA_NUM_DEVICES);
> >  
> > +/* debugfs */
> > +struct dentry *media_debugfs_root;
> > +
> >  /* Called when the last user of the media device exits. */
> >  static void media_devnode_release(struct device *cd)
> >  {
> > @@ -236,6 +239,7 @@ int __must_check media_devnode_register(struct media_device *mdev,
> >  	if (devnode->parent)
> >  		devnode->dev.parent = devnode->parent;
> >  	dev_set_name(&devnode->dev, "media%d", devnode->minor);
> > +	dev_set_drvdata(&devnode->dev, mdev);
> >  	device_initialize(&devnode->dev);
> >  
> >  	/* Part 2: Initialize the character device */
> > @@ -313,6 +317,7 @@ static int __init media_devnode_init(void)
> >  
> >  static void __exit media_devnode_exit(void)
> >  {
> > +	debugfs_remove_recursive(media_debugfs_root);
> >  	bus_unregister(&media_bus_type);
> >  	unregister_chrdev_region(media_dev_t, MEDIA_NUM_DEVICES);
> >  }
> > diff --git a/drivers/media/mc/mc-request.c b/drivers/media/mc/mc-request.c
> > index 398d0806d1d274eb8c454fc5c37b77476abe1e74..829e35a5d56d41c52cc583cdea1c959bcb4fce60 100644
> > --- a/drivers/media/mc/mc-request.c
> > +++ b/drivers/media/mc/mc-request.c
> > @@ -75,6 +75,7 @@ static void media_request_release(struct kref *kref)
> >  		mdev->ops->req_free(req);
> >  	else
> >  		kfree(req);
> > +	atomic_dec(&mdev->num_requests);
> >  }
> >  
> >  void media_request_put(struct media_request *req)
> > @@ -326,6 +327,7 @@ int media_request_alloc(struct media_device *mdev, int *alloc_fd)
> >  
> >  	snprintf(req->debug_str, sizeof(req->debug_str), "%u:%d",
> >  		 atomic_inc_return(&mdev->request_id), fd);
> > +	atomic_inc(&mdev->num_requests);
> >  	dev_dbg(mdev->dev, "request: allocated %s\n", req->debug_str);
> >  
> >  	fd_install(fd, filp);
> > @@ -349,10 +351,12 @@ static void media_request_object_release(struct kref *kref)
> >  	struct media_request_object *obj =
> >  		container_of(kref, struct media_request_object, kref);
> >  	struct media_request *req = obj->req;
> > +	struct media_device *mdev = obj->mdev;
> >  
> >  	if (WARN_ON(req))
> >  		media_request_object_unbind(obj);
> >  	obj->ops->release(obj);
> > +	atomic_dec(&mdev->num_request_objects);
> >  }
> >  
> >  struct media_request_object *
> > @@ -417,6 +421,7 @@ int media_request_object_bind(struct media_request *req,
> >  	obj->req = req;
> >  	obj->ops = ops;
> >  	obj->priv = priv;
> > +	obj->mdev = req->mdev;
> >  
> >  	if (is_buffer)
> >  		list_add_tail(&obj->list, &req->objects);
> > @@ -424,6 +429,7 @@ int media_request_object_bind(struct media_request *req,
> >  		list_add(&obj->list, &req->objects);
> >  	req->num_incomplete_objects++;
> >  	ret = 0;
> > +	atomic_inc(&obj->mdev->num_request_objects);
> >  
> >  unlock:
> >  	spin_unlock_irqrestore(&req->lock, flags);
> > diff --git a/include/media/media-device.h b/include/media/media-device.h
> > index 53d2a16a70b0d9d6e5cc28fe1fc5d5ef384410d5..749c327e3c582c3c583e0394468321ccd6160da5 100644
> > --- a/include/media/media-device.h
> > +++ b/include/media/media-device.h
> > @@ -11,6 +11,7 @@
> >  #ifndef _MEDIA_DEVICE_H
> >  #define _MEDIA_DEVICE_H
> >  
> > +#include <linux/atomic.h>
> >  #include <linux/list.h>
> >  #include <linux/mutex.h>
> >  #include <linux/pci.h>
> > @@ -106,6 +107,9 @@ struct media_device_ops {
> >   * @ops:	Operation handler callbacks
> >   * @req_queue_mutex: Serialise the MEDIA_REQUEST_IOC_QUEUE ioctl w.r.t.
> >   *		     other operations that stop or start streaming.
> > + * @num_requests: number of associated requests
> > + * @num_request_objects: number of associated request objects
> > + * @media_dir:	DebugFS media directory
> >   * @request_id: Used to generate unique request IDs
> >   *
> >   * This structure represents an abstract high-level media device. It allows easy
> > @@ -179,6 +183,11 @@ struct media_device {
> >  	const struct media_device_ops *ops;
> >  
> >  	struct mutex req_queue_mutex;
> > +	atomic_t num_requests;
> > +	atomic_t num_request_objects;
> > +
> > +	/* debugfs */
> > +	struct dentry *media_dir;
> >  	atomic_t request_id;
> >  };
> >  
> > diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
> > index d27c1c646c2805171be3997d72210dd4d1a38e32..dbcabeffcb572ae707f5fe1f51ff719d451c6784 100644
> > --- a/include/media/media-devnode.h
> > +++ b/include/media/media-devnode.h
> > @@ -20,9 +20,13 @@
> >  #include <linux/fs.h>
> >  #include <linux/device.h>
> >  #include <linux/cdev.h>
> > +#include <linux/debugfs.h>
> >  
> >  struct media_device;
> >  
> > +/* debugfs top-level media directory */
> > +extern struct dentry *media_debugfs_root;
> > +
> >  /*
> >   * Flag to mark the media_devnode struct as registered. Drivers must not touch
> >   * this flag directly, it will be set and cleared by media_devnode_register and
> > diff --git a/include/media/media-request.h b/include/media/media-request.h
> > index 7f9af68ef19ac6de0184bbb0c0827dc59777c6dc..610ccfe8d7b20ec38e166383433f9ee208248640 100644
> > --- a/include/media/media-request.h
> > +++ b/include/media/media-request.h
> > @@ -292,6 +292,7 @@ struct media_request_object_ops {
> >   * struct media_request_object - An opaque object that belongs to a media
> >   *				 request
> >   *
> > + * @mdev: Media device this object belongs to
> 
> This deserves at least a comment what this may be used for: generally once
> object is unbound, it's not related to a request anymore (nor a Media
> device). This field also adds a new Media device lifetime issue: nothing

We could make it explicit by clearing the mdev pointer ?

> guarantees the mdev is not disappearing at a wrong time albeit this is
> very, very likely not user-triggerable without physically removing
> hardware.

I'm not too familiar with the subject, if the MC knows it has open request
FD(s), why would it allow userspace from unloading its module ?

> 
> >   * @ops: object's operations
> >   * @priv: object's priv pointer
> >   * @req: the request this object belongs to (can be NULL)
> > @@ -303,6 +304,7 @@ struct media_request_object_ops {
> >   * another struct that contains the actual data for this request object.
> >   */
> >  struct media_request_object {
> > +	struct media_device *mdev;
> >  	const struct media_request_object_ops *ops;
> >  	void *priv;
> >  	struct media_request *req;
> > 

Re: [PATCH v3 1/5] media: mc: add manual request completion

[Nicolas Dufresne]

Le mercredi 04 juin 2025 à 21:04 +0000, Sakari Ailus a écrit :
> Hi Nicolas,
> 
> Thanks for the update.
> 
> On Wed, Jun 04, 2025 at 04:09:35PM -0400, Nicolas Dufresne wrote:
> > From: Hans Verkuil <hverkuil@xs4all.nl>
> > 
> > By default when the last request object is completed, the whole
> > request completes as well.
> > 
> > But sometimes you want to delay this completion to an arbitrary point in
> > time so add a manual complete mode for this.
> > 
> > In req_queue the driver marks the request for manual completion by
> > calling media_request_mark_manual_completion, and when the driver
> > wants to manually complete the request it calls
> > media_request_manual_complete().
> > 
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> >  drivers/media/mc/mc-request.c | 38 ++++++++++++++++++++++++++++++++++++--
> >  include/media/media-request.h | 38 +++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 73 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/mc/mc-request.c b/drivers/media/mc/mc-request.c
> > index 5edfc2791ce7c7485def5db675bbf53ee223d837..398d0806d1d274eb8c454fc5c37b77476abe1e74 100644
> > --- a/drivers/media/mc/mc-request.c
> > +++ b/drivers/media/mc/mc-request.c
> > @@ -54,6 +54,7 @@ static void media_request_clean(struct media_request *req)
> >  	req->access_count = 0;
> >  	WARN_ON(req->num_incomplete_objects);
> >  	req->num_incomplete_objects = 0;
> > +	req->manual_completion = false;
> >  	wake_up_interruptible_all(&req->poll_wait);
> >  }
> >  
> > @@ -313,6 +314,7 @@ int media_request_alloc(struct media_device *mdev, int *alloc_fd)
> >  	req->mdev = mdev;
> >  	req->state = MEDIA_REQUEST_STATE_IDLE;
> >  	req->num_incomplete_objects = 0;
> > +	req->manual_completion = false;
> >  	kref_init(&req->kref);
> >  	INIT_LIST_HEAD(&req->objects);
> >  	spin_lock_init(&req->lock);
> > @@ -459,7 +461,7 @@ void media_request_object_unbind(struct media_request_object *obj)
> >  
> >  	req->num_incomplete_objects--;
> >  	if (req->state == MEDIA_REQUEST_STATE_QUEUED &&
> > -	    !req->num_incomplete_objects) {
> > +	    !req->num_incomplete_objects && !req->manual_completion) {
> >  		req->state = MEDIA_REQUEST_STATE_COMPLETE;
> >  		completed = true;
> >  		wake_up_interruptible_all(&req->poll_wait);
> > @@ -488,7 +490,7 @@ void media_request_object_complete(struct media_request_object *obj)
> >  	    WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
> >  		goto unlock;
> >  
> > -	if (!--req->num_incomplete_objects) {
> > +	if (!--req->num_incomplete_objects && !req->manual_completion) {
> >  		req->state = MEDIA_REQUEST_STATE_COMPLETE;
> >  		wake_up_interruptible_all(&req->poll_wait);
> >  		completed = true;
> > @@ -499,3 +501,35 @@ void media_request_object_complete(struct media_request_object *obj)
> >  		media_request_put(req);
> >  }
> >  EXPORT_SYMBOL_GPL(media_request_object_complete);
> > +
> > +void media_request_manual_complete(struct media_request *req)
> > +{
> > +	unsigned long flags;
> 
> I'd declare flags as last.
> 
> > +	bool completed = false;
> > +
> > +	if (WARN_ON(!req))
> > +		return;
> > +	if (WARN_ON(!req->manual_completion))
> > +		return;
> 
> I think I'd use WARN_ON_ONCE() consistently: this is a driver (or
> framework) bug and telling once about it is very probably enough.

Just to be sure, you only mean for the two checks above ? Or did
you mean for the entire function ?

> 
> > +
> > +	spin_lock_irqsave(&req->lock, flags);

In practice, if you call this specific function from two places at the same
time you have a bug, but I realize that moving the the warning on the check
manual_completion inside that lock would massively help detect that case.

What do you think ?

> > +	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
> > +		goto unlock;
> > +
> > +	req->manual_completion = false;
> > +	/*
> > +	 * It is expected that all other objects in this request are
> > +	 * completed when this function is called. WARN if that is
> > +	 * not the case.
> > +	 */
> > +	if (!WARN_ON(req->num_incomplete_objects)) {
> > +		req->state = MEDIA_REQUEST_STATE_COMPLETE;
> > +		wake_up_interruptible_all(&req->poll_wait);
> > +		completed = true;
> > +	}
> 
> A newline would be nice here.
> 
> > +unlock:
> > +	spin_unlock_irqrestore(&req->lock, flags);
> > +	if (completed)
> > +		media_request_put(req);
> > +}
> > +EXPORT_SYMBOL_GPL(media_request_manual_complete);
> > diff --git a/include/media/media-request.h b/include/media/media-request.h
> > index d4ac557678a78372222704400c8c96cf3150b9d9..7f9af68ef19ac6de0184bbb0c0827dc59777c6dc 100644
> > --- a/include/media/media-request.h
> > +++ b/include/media/media-request.h
> > @@ -56,6 +56,9 @@ struct media_request_object;
> >   * @access_count: count the number of request accesses that are in progress
> >   * @objects: List of @struct media_request_object request objects
> >   * @num_incomplete_objects: The number of incomplete objects in the request
> > + * @manual_completion: if true, then the request won't be marked as completed
> > + * when @num_incomplete_objects reaches 0. Call media_request_manual_complete()
> > + * to complete the request after @num_incomplete_objects == 0.
> >   * @poll_wait: Wait queue for poll
> >   * @lock: Serializes access to this struct
> >   */
> > @@ -68,6 +71,7 @@ struct media_request {
> >  	unsigned int access_count;
> >  	struct list_head objects;
> >  	unsigned int num_incomplete_objects;
> > +	bool manual_completion;
> >  	wait_queue_head_t poll_wait;
> >  	spinlock_t lock;
> >  };
> > @@ -218,6 +222,38 @@ media_request_get_by_fd(struct media_device *mdev, int request_fd);
> >  int media_request_alloc(struct media_device *mdev,
> >  			int *alloc_fd);
> >  
> > +/**
> > + * media_request_mark_manual_completion - Enable manual completion
> > + *
> > + * @req: The request
> > + *
> > + * Mark that the request has to be manually completed by calling
> > + * media_request_manual_complete().
> > + *
> > + * This function shall be called in the req_queue callback.
> > + */
> > +static inline void
> > +media_request_mark_manual_completion(struct media_request *req)
> > +{
> > +	req->manual_completion = true;
> > +}
> > +
> > +/**
> > + * media_request_manual_complete - Mark the request as completed
> > + *
> > + * @req: The request
> > + *
> > + * This function completes a request that was marked for manual completion by an
> > + * earlier call to media_request_mark_manual_completion(). The request's
> > + * @manual_completion flag is reset to false.
> 
> s/flag/field/
> 
> > + *
> > + * All objects contained in the request must have been completed previously. It
> > + * is an error to call this function otherwise. If such an error occurred, the
> > + * function will WARN and the object completion will be delayed until
> > + * @num_incomplete_objects is 0.
> > + */
> > +void media_request_manual_complete(struct media_request *req);
> > +
> >  #else
> >  
> >  static inline void media_request_get(struct media_request *req)
> > @@ -336,7 +372,7 @@ void media_request_object_init(struct media_request_object *obj);
> >   * @req: The media request
> >   * @ops: The object ops for this object
> >   * @priv: A driver-specific priv pointer associated with this object
> > - * @is_buffer: Set to true if the object a buffer object.
> > + * @is_buffer: Set to true if the object is a buffer object.
> >   * @obj: The object
> >   *
> >   * Bind this object to the request and set the ops and priv values of
> > 

Re: [PATCH v3 1/5] media: mc: add manual request completion

[Sakari Ailus]

Hi Nicolas,

On Wed, Jun 04, 2025 at 07:19:27PM -0400, Nicolas Dufresne wrote:
> Le mercredi 04 juin 2025 à 21:04 +0000, Sakari Ailus a écrit :
> > Hi Nicolas,
> > 
> > Thanks for the update.
> > 
> > On Wed, Jun 04, 2025 at 04:09:35PM -0400, Nicolas Dufresne wrote:
> > > From: Hans Verkuil <hverkuil@xs4all.nl>
> > > 
> > > By default when the last request object is completed, the whole
> > > request completes as well.
> > > 
> > > But sometimes you want to delay this completion to an arbitrary point in
> > > time so add a manual complete mode for this.
> > > 
> > > In req_queue the driver marks the request for manual completion by
> > > calling media_request_mark_manual_completion, and when the driver
> > > wants to manually complete the request it calls
> > > media_request_manual_complete().
> > > 
> > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > ---
> > >  drivers/media/mc/mc-request.c | 38 ++++++++++++++++++++++++++++++++++++--
> > >  include/media/media-request.h | 38 +++++++++++++++++++++++++++++++++++++-
> > >  2 files changed, 73 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/media/mc/mc-request.c b/drivers/media/mc/mc-request.c
> > > index 5edfc2791ce7c7485def5db675bbf53ee223d837..398d0806d1d274eb8c454fc5c37b77476abe1e74 100644
> > > --- a/drivers/media/mc/mc-request.c
> > > +++ b/drivers/media/mc/mc-request.c
> > > @@ -54,6 +54,7 @@ static void media_request_clean(struct media_request *req)
> > >  	req->access_count = 0;
> > >  	WARN_ON(req->num_incomplete_objects);
> > >  	req->num_incomplete_objects = 0;
> > > +	req->manual_completion = false;
> > >  	wake_up_interruptible_all(&req->poll_wait);
> > >  }
> > >  
> > > @@ -313,6 +314,7 @@ int media_request_alloc(struct media_device *mdev, int *alloc_fd)
> > >  	req->mdev = mdev;
> > >  	req->state = MEDIA_REQUEST_STATE_IDLE;
> > >  	req->num_incomplete_objects = 0;
> > > +	req->manual_completion = false;
> > >  	kref_init(&req->kref);
> > >  	INIT_LIST_HEAD(&req->objects);
> > >  	spin_lock_init(&req->lock);
> > > @@ -459,7 +461,7 @@ void media_request_object_unbind(struct media_request_object *obj)
> > >  
> > >  	req->num_incomplete_objects--;
> > >  	if (req->state == MEDIA_REQUEST_STATE_QUEUED &&
> > > -	    !req->num_incomplete_objects) {
> > > +	    !req->num_incomplete_objects && !req->manual_completion) {
> > >  		req->state = MEDIA_REQUEST_STATE_COMPLETE;
> > >  		completed = true;
> > >  		wake_up_interruptible_all(&req->poll_wait);
> > > @@ -488,7 +490,7 @@ void media_request_object_complete(struct media_request_object *obj)
> > >  	    WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
> > >  		goto unlock;
> > >  
> > > -	if (!--req->num_incomplete_objects) {
> > > +	if (!--req->num_incomplete_objects && !req->manual_completion) {
> > >  		req->state = MEDIA_REQUEST_STATE_COMPLETE;
> > >  		wake_up_interruptible_all(&req->poll_wait);
> > >  		completed = true;
> > > @@ -499,3 +501,35 @@ void media_request_object_complete(struct media_request_object *obj)
> > >  		media_request_put(req);
> > >  }
> > >  EXPORT_SYMBOL_GPL(media_request_object_complete);
> > > +
> > > +void media_request_manual_complete(struct media_request *req)
> > > +{
> > > +	unsigned long flags;
> > 
> > I'd declare flags as last.
> > 
> > > +	bool completed = false;
> > > +
> > > +	if (WARN_ON(!req))
> > > +		return;
> > > +	if (WARN_ON(!req->manual_completion))
> > > +		return;
> > 
> > I think I'd use WARN_ON_ONCE() consistently: this is a driver (or
> > framework) bug and telling once about it is very probably enough.
> 
> Just to be sure, you only mean for the two checks above ? Or did
> you mean for the entire function ?

For the entire function. I thought that if this is user-triggerable, the
amount of data ending up in logs could be very large.

> 
> > 
> > > +
> > > +	spin_lock_irqsave(&req->lock, flags);
> 
> In practice, if you call this specific function from two places at the same
> time you have a bug, but I realize that moving the the warning on the check
> manual_completion inside that lock would massively help detect that case.
> 
> What do you think ?

Seems reasonable to me.

> 
> > > +	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
> > > +		goto unlock;
> > > +
> > > +	req->manual_completion = false;
> > > +	/*
> > > +	 * It is expected that all other objects in this request are
> > > +	 * completed when this function is called. WARN if that is
> > > +	 * not the case.
> > > +	 */
> > > +	if (!WARN_ON(req->num_incomplete_objects)) {
> > > +		req->state = MEDIA_REQUEST_STATE_COMPLETE;
> > > +		wake_up_interruptible_all(&req->poll_wait);
> > > +		completed = true;
> > > +	}
> > 
> > A newline would be nice here.
> > 
> > > +unlock:
> > > +	spin_unlock_irqrestore(&req->lock, flags);
> > > +	if (completed)
> > > +		media_request_put(req);
> > > +}
> > > +EXPORT_SYMBOL_GPL(media_request_manual_complete);
> > > diff --git a/include/media/media-request.h b/include/media/media-request.h
> > > index d4ac557678a78372222704400c8c96cf3150b9d9..7f9af68ef19ac6de0184bbb0c0827dc59777c6dc 100644
> > > --- a/include/media/media-request.h
> > > +++ b/include/media/media-request.h
> > > @@ -56,6 +56,9 @@ struct media_request_object;
> > >   * @access_count: count the number of request accesses that are in progress
> > >   * @objects: List of @struct media_request_object request objects
> > >   * @num_incomplete_objects: The number of incomplete objects in the request
> > > + * @manual_completion: if true, then the request won't be marked as completed
> > > + * when @num_incomplete_objects reaches 0. Call media_request_manual_complete()
> > > + * to complete the request after @num_incomplete_objects == 0.
> > >   * @poll_wait: Wait queue for poll
> > >   * @lock: Serializes access to this struct
> > >   */
> > > @@ -68,6 +71,7 @@ struct media_request {
> > >  	unsigned int access_count;
> > >  	struct list_head objects;
> > >  	unsigned int num_incomplete_objects;
> > > +	bool manual_completion;
> > >  	wait_queue_head_t poll_wait;
> > >  	spinlock_t lock;
> > >  };
> > > @@ -218,6 +222,38 @@ media_request_get_by_fd(struct media_device *mdev, int request_fd);
> > >  int media_request_alloc(struct media_device *mdev,
> > >  			int *alloc_fd);
> > >  
> > > +/**
> > > + * media_request_mark_manual_completion - Enable manual completion
> > > + *
> > > + * @req: The request
> > > + *
> > > + * Mark that the request has to be manually completed by calling
> > > + * media_request_manual_complete().
> > > + *
> > > + * This function shall be called in the req_queue callback.
> > > + */
> > > +static inline void
> > > +media_request_mark_manual_completion(struct media_request *req)
> > > +{
> > > +	req->manual_completion = true;
> > > +}
> > > +
> > > +/**
> > > + * media_request_manual_complete - Mark the request as completed
> > > + *
> > > + * @req: The request
> > > + *
> > > + * This function completes a request that was marked for manual completion by an
> > > + * earlier call to media_request_mark_manual_completion(). The request's
> > > + * @manual_completion flag is reset to false.
> > 
> > s/flag/field/
> > 
> > > + *
> > > + * All objects contained in the request must have been completed previously. It
> > > + * is an error to call this function otherwise. If such an error occurred, the
> > > + * function will WARN and the object completion will be delayed until
> > > + * @num_incomplete_objects is 0.
> > > + */
> > > +void media_request_manual_complete(struct media_request *req);
> > > +
> > >  #else
> > >  
> > >  static inline void media_request_get(struct media_request *req)
> > > @@ -336,7 +372,7 @@ void media_request_object_init(struct media_request_object *obj);
> > >   * @req: The media request
> > >   * @ops: The object ops for this object
> > >   * @priv: A driver-specific priv pointer associated with this object
> > > - * @is_buffer: Set to true if the object a buffer object.
> > > + * @is_buffer: Set to true if the object is a buffer object.
> > >   * @obj: The object
> > >   *
> > >   * Bind this object to the request and set the ops and priv values of
> > > 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 1/5] media: mc: add manual request completion

[Hans Verkuil]

On 6/5/25 08:51, Sakari Ailus wrote:
> Hi Nicolas,
> 
> On Wed, Jun 04, 2025 at 07:19:27PM -0400, Nicolas Dufresne wrote:
>> Le mercredi 04 juin 2025 à 21:04 +0000, Sakari Ailus a écrit :
>>> Hi Nicolas,
>>>
>>> Thanks for the update.
>>>
>>> On Wed, Jun 04, 2025 at 04:09:35PM -0400, Nicolas Dufresne wrote:
>>>> From: Hans Verkuil <hverkuil@xs4all.nl>
>>>>
>>>> By default when the last request object is completed, the whole
>>>> request completes as well.
>>>>
>>>> But sometimes you want to delay this completion to an arbitrary point in
>>>> time so add a manual complete mode for this.
>>>>
>>>> In req_queue the driver marks the request for manual completion by
>>>> calling media_request_mark_manual_completion, and when the driver
>>>> wants to manually complete the request it calls
>>>> media_request_manual_complete().
>>>>
>>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>>> ---
>>>>  drivers/media/mc/mc-request.c | 38 ++++++++++++++++++++++++++++++++++++--
>>>>  include/media/media-request.h | 38 +++++++++++++++++++++++++++++++++++++-
>>>>  2 files changed, 73 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/media/mc/mc-request.c b/drivers/media/mc/mc-request.c
>>>> index 5edfc2791ce7c7485def5db675bbf53ee223d837..398d0806d1d274eb8c454fc5c37b77476abe1e74 100644
>>>> --- a/drivers/media/mc/mc-request.c
>>>> +++ b/drivers/media/mc/mc-request.c
>>>> @@ -54,6 +54,7 @@ static void media_request_clean(struct media_request *req)
>>>>  	req->access_count = 0;
>>>>  	WARN_ON(req->num_incomplete_objects);
>>>>  	req->num_incomplete_objects = 0;
>>>> +	req->manual_completion = false;
>>>>  	wake_up_interruptible_all(&req->poll_wait);
>>>>  }
>>>>  
>>>> @@ -313,6 +314,7 @@ int media_request_alloc(struct media_device *mdev, int *alloc_fd)
>>>>  	req->mdev = mdev;
>>>>  	req->state = MEDIA_REQUEST_STATE_IDLE;
>>>>  	req->num_incomplete_objects = 0;
>>>> +	req->manual_completion = false;
>>>>  	kref_init(&req->kref);
>>>>  	INIT_LIST_HEAD(&req->objects);
>>>>  	spin_lock_init(&req->lock);
>>>> @@ -459,7 +461,7 @@ void media_request_object_unbind(struct media_request_object *obj)
>>>>  
>>>>  	req->num_incomplete_objects--;
>>>>  	if (req->state == MEDIA_REQUEST_STATE_QUEUED &&
>>>> -	    !req->num_incomplete_objects) {
>>>> +	    !req->num_incomplete_objects && !req->manual_completion) {
>>>>  		req->state = MEDIA_REQUEST_STATE_COMPLETE;
>>>>  		completed = true;
>>>>  		wake_up_interruptible_all(&req->poll_wait);
>>>> @@ -488,7 +490,7 @@ void media_request_object_complete(struct media_request_object *obj)
>>>>  	    WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
>>>>  		goto unlock;
>>>>  
>>>> -	if (!--req->num_incomplete_objects) {
>>>> +	if (!--req->num_incomplete_objects && !req->manual_completion) {
>>>>  		req->state = MEDIA_REQUEST_STATE_COMPLETE;
>>>>  		wake_up_interruptible_all(&req->poll_wait);
>>>>  		completed = true;
>>>> @@ -499,3 +501,35 @@ void media_request_object_complete(struct media_request_object *obj)
>>>>  		media_request_put(req);
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(media_request_object_complete);
>>>> +
>>>> +void media_request_manual_complete(struct media_request *req)
>>>> +{
>>>> +	unsigned long flags;
>>>
>>> I'd declare flags as last.
>>>
>>>> +	bool completed = false;
>>>> +
>>>> +	if (WARN_ON(!req))
>>>> +		return;
>>>> +	if (WARN_ON(!req->manual_completion))
>>>> +		return;
>>>
>>> I think I'd use WARN_ON_ONCE() consistently: this is a driver (or
>>> framework) bug and telling once about it is very probably enough.
>>
>> Just to be sure, you only mean for the two checks above ? Or did
>> you mean for the entire function ?
> 
> For the entire function. I thought that if this is user-triggerable, the
> amount of data ending up in logs could be very large.

It's not user-triggerable, if this happens, then it is a driver bug.

Regards,

	Hans

> 
>>
>>>
>>>> +
>>>> +	spin_lock_irqsave(&req->lock, flags);
>>
>> In practice, if you call this specific function from two places at the same
>> time you have a bug, but I realize that moving the the warning on the check
>> manual_completion inside that lock would massively help detect that case.
>>
>> What do you think ?
> 
> Seems reasonable to me.
> 
>>
>>>> +	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
>>>> +		goto unlock;
>>>> +
>>>> +	req->manual_completion = false;
>>>> +	/*
>>>> +	 * It is expected that all other objects in this request are
>>>> +	 * completed when this function is called. WARN if that is
>>>> +	 * not the case.
>>>> +	 */
>>>> +	if (!WARN_ON(req->num_incomplete_objects)) {
>>>> +		req->state = MEDIA_REQUEST_STATE_COMPLETE;
>>>> +		wake_up_interruptible_all(&req->poll_wait);
>>>> +		completed = true;
>>>> +	}
>>>
>>> A newline would be nice here.
>>>
>>>> +unlock:
>>>> +	spin_unlock_irqrestore(&req->lock, flags);
>>>> +	if (completed)
>>>> +		media_request_put(req);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(media_request_manual_complete);
>>>> diff --git a/include/media/media-request.h b/include/media/media-request.h
>>>> index d4ac557678a78372222704400c8c96cf3150b9d9..7f9af68ef19ac6de0184bbb0c0827dc59777c6dc 100644
>>>> --- a/include/media/media-request.h
>>>> +++ b/include/media/media-request.h
>>>> @@ -56,6 +56,9 @@ struct media_request_object;
>>>>   * @access_count: count the number of request accesses that are in progress
>>>>   * @objects: List of @struct media_request_object request objects
>>>>   * @num_incomplete_objects: The number of incomplete objects in the request
>>>> + * @manual_completion: if true, then the request won't be marked as completed
>>>> + * when @num_incomplete_objects reaches 0. Call media_request_manual_complete()
>>>> + * to complete the request after @num_incomplete_objects == 0.
>>>>   * @poll_wait: Wait queue for poll
>>>>   * @lock: Serializes access to this struct
>>>>   */
>>>> @@ -68,6 +71,7 @@ struct media_request {
>>>>  	unsigned int access_count;
>>>>  	struct list_head objects;
>>>>  	unsigned int num_incomplete_objects;
>>>> +	bool manual_completion;
>>>>  	wait_queue_head_t poll_wait;
>>>>  	spinlock_t lock;
>>>>  };
>>>> @@ -218,6 +222,38 @@ media_request_get_by_fd(struct media_device *mdev, int request_fd);
>>>>  int media_request_alloc(struct media_device *mdev,
>>>>  			int *alloc_fd);
>>>>  
>>>> +/**
>>>> + * media_request_mark_manual_completion - Enable manual completion
>>>> + *
>>>> + * @req: The request
>>>> + *
>>>> + * Mark that the request has to be manually completed by calling
>>>> + * media_request_manual_complete().
>>>> + *
>>>> + * This function shall be called in the req_queue callback.
>>>> + */
>>>> +static inline void
>>>> +media_request_mark_manual_completion(struct media_request *req)
>>>> +{
>>>> +	req->manual_completion = true;
>>>> +}
>>>> +
>>>> +/**
>>>> + * media_request_manual_complete - Mark the request as completed
>>>> + *
>>>> + * @req: The request
>>>> + *
>>>> + * This function completes a request that was marked for manual completion by an
>>>> + * earlier call to media_request_mark_manual_completion(). The request's
>>>> + * @manual_completion flag is reset to false.
>>>
>>> s/flag/field/
>>>
>>>> + *
>>>> + * All objects contained in the request must have been completed previously. It
>>>> + * is an error to call this function otherwise. If such an error occurred, the
>>>> + * function will WARN and the object completion will be delayed until
>>>> + * @num_incomplete_objects is 0.
>>>> + */
>>>> +void media_request_manual_complete(struct media_request *req);
>>>> +
>>>>  #else
>>>>  
>>>>  static inline void media_request_get(struct media_request *req)
>>>> @@ -336,7 +372,7 @@ void media_request_object_init(struct media_request_object *obj);
>>>>   * @req: The media request
>>>>   * @ops: The object ops for this object
>>>>   * @priv: A driver-specific priv pointer associated with this object
>>>> - * @is_buffer: Set to true if the object a buffer object.
>>>> + * @is_buffer: Set to true if the object is a buffer object.
>>>>   * @obj: The object
>>>>   *
>>>>   * Bind this object to the request and set the ops and priv values of
>>>>
> 

Re: [PATCH v3 1/5] media: mc: add manual request completion

[Sakari Ailus]

Hi Hans,

On Thu, Jun 05, 2025 at 11:37:54AM +0200, Hans Verkuil wrote:
> >>>> @@ -499,3 +501,35 @@ void media_request_object_complete(struct media_request_object *obj)
> >>>>  		media_request_put(req);
> >>>>  }
> >>>>  EXPORT_SYMBOL_GPL(media_request_object_complete);
> >>>> +
> >>>> +void media_request_manual_complete(struct media_request *req)
> >>>> +{
> >>>> +	unsigned long flags;
> >>>
> >>> I'd declare flags as last.
> >>>
> >>>> +	bool completed = false;
> >>>> +
> >>>> +	if (WARN_ON(!req))
> >>>> +		return;
> >>>> +	if (WARN_ON(!req->manual_completion))
> >>>> +		return;
> >>>
> >>> I think I'd use WARN_ON_ONCE() consistently: this is a driver (or
> >>> framework) bug and telling once about it is very probably enough.
> >>
> >> Just to be sure, you only mean for the two checks above ? Or did
> >> you mean for the entire function ?
> > 
> > For the entire function. I thought that if this is user-triggerable, the
> > amount of data ending up in logs could be very large.
> 
> It's not user-triggerable, if this happens, then it is a driver bug.

If there is a driver bug, it could well be user-triggerable, wouldn't it?
Testing may not uncover all such cases.

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 1/5] media: mc: add manual request completion

[Nicolas Dufresne]

Hi Hans, Sakari,

Le jeudi 05 juin 2025 à 09:48 +0000, Sakari Ailus a écrit :
> > It's not user-triggerable, if this happens, then it is a driver bug.
> 
> If there is a driver bug, it could well be user-triggerable, wouldn't it?
> Testing may not uncover all such cases.

You are both right, if the driver is not used, the warning will never
trigger. I was worried of the hit of a thread safe ONCE implementation,
but WARN_ONCE is simply not, it can warn few time before it stops if
called from multiple CPUs at the same time. In that specific function,
I can move all the checks inside the lock to make it truly once.

Now its up to you, I don't have strong preference. These are driver errors,
and usually quite critical. They are not bug_on simply because we have a
crash free resolution, but its probably not functional anymore.

Nicolas

Re: [PATCH v3 1/5] media: mc: add manual request completion

[Sakari Ailus]

Hi,

On Thu, Jun 05, 2025 at 08:41:45AM -0400, Nicolas Dufresne wrote:
> Hi Hans, Sakari,
> 
> Le jeudi 05 juin 2025 à 09:48 +0000, Sakari Ailus a écrit :
> > > It's not user-triggerable, if this happens, then it is a driver bug.
> > 
> > If there is a driver bug, it could well be user-triggerable, wouldn't it?
> > Testing may not uncover all such cases.
> 
> You are both right, if the driver is not used, the warning will never
> trigger. I was worried of the hit of a thread safe ONCE implementation,
> but WARN_ONCE is simply not, it can warn few time before it stops if
> called from multiple CPUs at the same time. In that specific function,
> I can move all the checks inside the lock to make it truly once.
> 
> Now its up to you, I don't have strong preference. These are driver errors,
> and usually quite critical. They are not bug_on simply because we have a
> crash free resolution, but its probably not functional anymore.

I'd prefer _ONCE variants, I wonder what Hans thinks.

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 1/5] media: mc: add manual request completion

[Hans Verkuil]

On 09/06/2025 11:42, Sakari Ailus wrote:
> Hi,
> 
> On Thu, Jun 05, 2025 at 08:41:45AM -0400, Nicolas Dufresne wrote:
>> Hi Hans, Sakari,
>>
>> Le jeudi 05 juin 2025 à 09:48 +0000, Sakari Ailus a écrit :
>>>> It's not user-triggerable, if this happens, then it is a driver bug.
>>>
>>> If there is a driver bug, it could well be user-triggerable, wouldn't it?
>>> Testing may not uncover all such cases.
>>
>> You are both right, if the driver is not used, the warning will never
>> trigger. I was worried of the hit of a thread safe ONCE implementation,
>> but WARN_ONCE is simply not, it can warn few time before it stops if
>> called from multiple CPUs at the same time. In that specific function,
>> I can move all the checks inside the lock to make it truly once.
>>
>> Now its up to you, I don't have strong preference. These are driver errors,
>> and usually quite critical. They are not bug_on simply because we have a
>> crash free resolution, but its probably not functional anymore.
> 
> I'd prefer _ONCE variants, I wonder what Hans thinks.
> 

I have no particular preference.

Regards,

	Hans

Re: [PATCH v3 3/5] media: mc: add debugfs node to keep track of requests

[Sakari Ailus]

Hi Nicolas,

On Wed, Jun 04, 2025 at 07:08:53PM -0400, Nicolas Dufresne wrote:
> Le mercredi 04 juin 2025 à 21:33 +0000, Sakari Ailus a écrit :
> > Hi Nicolas, Hans,
> > 
> > Thanks for the update.
> 
> thanks for the review, these things are precious.
> 
> > 
> > On Wed, Jun 04, 2025 at 04:09:37PM -0400, Nicolas Dufresne wrote:
> > > From: Hans Verkuil <hverkuil@xs4all.nl>
> > > 
> > > Keep track of the number of requests and request objects of a media
> > > device. Helps to verify that all request-related memory is freed.
> > > 
> > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > ---
> > >  drivers/media/mc/mc-device.c  | 30 ++++++++++++++++++++++++++++++
> > >  drivers/media/mc/mc-devnode.c |  5 +++++
> > >  drivers/media/mc/mc-request.c |  6 ++++++
> > >  include/media/media-device.h  |  9 +++++++++
> > >  include/media/media-devnode.h |  4 ++++
> > >  include/media/media-request.h |  2 ++
> > >  6 files changed, 56 insertions(+)
> > > 
> > > diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> > > index c0dd4ae5722725f1744bc6fd6282d5c765438059..5a458160200afb540d8014fed42d8bf2dab9c8c3 100644
> > > --- a/drivers/media/mc/mc-device.c
> > > +++ b/drivers/media/mc/mc-device.c
> > > @@ -679,6 +679,23 @@ void media_device_unregister_entity(struct media_entity *entity)
> > >  }
> > >  EXPORT_SYMBOL_GPL(media_device_unregister_entity);
> > >  
> > > +#ifdef CONFIG_DEBUG_FS
> > > +/*
> > > + * Log the state of media requests.
> > > + * Very useful for debugging.
> > > + */
> > 
> > Fits on a single line.
> 
> Ack.
> 
> > 
> > > +static int media_device_requests(struct seq_file *file, void *priv)
> > > +{
> > > +	struct media_device *dev = dev_get_drvdata(file->private);
> > > +
> > > +	seq_printf(file, "number of requests: %d\n",
> > > +		   atomic_read(&dev->num_requests));
> > > +	seq_printf(file, "number of request objects: %d\n",
> > > +		   atomic_read(&dev->num_request_objects));
> > 
> > Newline here?
> 
> I prefer that too.
> 
> > 
> > > +	return 0;
> > > +}
> > > +#endif
> > > +
> > >  void media_device_init(struct media_device *mdev)
> > >  {
> > >  	INIT_LIST_HEAD(&mdev->entities);
> > > @@ -697,6 +714,9 @@ void media_device_init(struct media_device *mdev)
> > >  		media_set_bus_info(mdev->bus_info, sizeof(mdev->bus_info),
> > >  				   mdev->dev);
> > >  
> > > +	atomic_set(&mdev->num_requests, 0);
> > > +	atomic_set(&mdev->num_request_objects, 0);
> > > +
> > >  	dev_dbg(mdev->dev, "Media device initialized\n");
> > >  }
> > >  EXPORT_SYMBOL_GPL(media_device_init);
> > > @@ -748,6 +768,15 @@ int __must_check __media_device_register(struct media_device *mdev,
> > >  
> > >  	dev_dbg(mdev->dev, "Media device registered\n");
> > >  
> > > +#ifdef CONFIG_DEBUG_FS
> > > +	if (!media_debugfs_root)
> > > +		media_debugfs_root = debugfs_create_dir("media", NULL);
> > > +	mdev->media_dir = debugfs_create_dir(dev_name(&devnode->dev),
> > > +					     media_debugfs_root);
> > > +	debugfs_create_devm_seqfile(&devnode->dev, "requests",
> > > +				    mdev->media_dir, media_device_requests);
> > > +#endif
> > 
> > I have no objection to this but it would have been great to have the Media
> > device lifetime set in first and MC device and devnode merged. But maybe
> > it's too late for that. Well, at least this won't change error handling...
> 
> Since this specific patch is not required to fix the MTK VCODEC issue, I can
> delay this a little. Is that comment related to an existing patch ?

Yes.

I've pushed the current set here:
<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=media-ref>. I've
rebased it recently but it's still WiP.

...

> > > diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
> > > index d27c1c646c2805171be3997d72210dd4d1a38e32..dbcabeffcb572ae707f5fe1f51ff719d451c6784 100644
> > > --- a/include/media/media-devnode.h
> > > +++ b/include/media/media-devnode.h
> > > @@ -20,9 +20,13 @@
> > >  #include <linux/fs.h>
> > >  #include <linux/device.h>
> > >  #include <linux/cdev.h>
> > > +#include <linux/debugfs.h>
> > >  
> > >  struct media_device;
> > >  
> > > +/* debugfs top-level media directory */
> > > +extern struct dentry *media_debugfs_root;
> > > +
> > >  /*
> > >   * Flag to mark the media_devnode struct as registered. Drivers must not touch
> > >   * this flag directly, it will be set and cleared by media_devnode_register and
> > > diff --git a/include/media/media-request.h b/include/media/media-request.h
> > > index 7f9af68ef19ac6de0184bbb0c0827dc59777c6dc..610ccfe8d7b20ec38e166383433f9ee208248640 100644
> > > --- a/include/media/media-request.h
> > > +++ b/include/media/media-request.h
> > > @@ -292,6 +292,7 @@ struct media_request_object_ops {
> > >   * struct media_request_object - An opaque object that belongs to a media
> > >   *				 request
> > >   *
> > > + * @mdev: Media device this object belongs to
> > 
> > This deserves at least a comment what this may be used for: generally once
> > object is unbound, it's not related to a request anymore (nor a Media
> > device). This field also adds a new Media device lifetime issue: nothing
> 
> We could make it explicit by clearing the mdev pointer ?

That would probably be out of scope of this patch(set). Also see the
patchset I referred to earlier.

> 
> > guarantees the mdev is not disappearing at a wrong time albeit this is
> > very, very likely not user-triggerable without physically removing
> > hardware.
> 
> I'm not too familiar with the subject, if the MC knows it has open request
> FD(s), why would it allow userspace from unloading its module ?

Drivers nor MC currently have a list of request file handles.

Apart from the file handles, that was the original thinking, yes, but
devices can be also unbound without touching the driver (or other) modules.

> 
> > 
> > >   * @ops: object's operations
> > >   * @priv: object's priv pointer
> > >   * @req: the request this object belongs to (can be NULL)
> > > @@ -303,6 +304,7 @@ struct media_request_object_ops {
> > >   * another struct that contains the actual data for this request object.
> > >   */
> > >  struct media_request_object {
> > > +	struct media_device *mdev;
> > >  	const struct media_request_object_ops *ops;
> > >  	void *priv;
> > >  	struct media_request *req;
> > > 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 3/5] media: mc: add debugfs node to keep track of requests

[Hans Verkuil]

On 09/06/2025 12:28, Sakari Ailus wrote:
> Hi Nicolas,
> 
> On Wed, Jun 04, 2025 at 07:08:53PM -0400, Nicolas Dufresne wrote:
>> Le mercredi 04 juin 2025 à 21:33 +0000, Sakari Ailus a écrit :
>>> Hi Nicolas, Hans,
>>>
>>> Thanks for the update.
>>
>> thanks for the review, these things are precious.
>>
>>>
>>> On Wed, Jun 04, 2025 at 04:09:37PM -0400, Nicolas Dufresne wrote:
>>>> From: Hans Verkuil <hverkuil@xs4all.nl>
>>>>
>>>> Keep track of the number of requests and request objects of a media
>>>> device. Helps to verify that all request-related memory is freed.
>>>>
>>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>>> ---
>>>>  drivers/media/mc/mc-device.c  | 30 ++++++++++++++++++++++++++++++
>>>>  drivers/media/mc/mc-devnode.c |  5 +++++
>>>>  drivers/media/mc/mc-request.c |  6 ++++++
>>>>  include/media/media-device.h  |  9 +++++++++
>>>>  include/media/media-devnode.h |  4 ++++
>>>>  include/media/media-request.h |  2 ++
>>>>  6 files changed, 56 insertions(+)
>>>>
>>>> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
>>>> index c0dd4ae5722725f1744bc6fd6282d5c765438059..5a458160200afb540d8014fed42d8bf2dab9c8c3 100644
>>>> --- a/drivers/media/mc/mc-device.c
>>>> +++ b/drivers/media/mc/mc-device.c
>>>> @@ -679,6 +679,23 @@ void media_device_unregister_entity(struct media_entity *entity)
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(media_device_unregister_entity);
>>>>  
>>>> +#ifdef CONFIG_DEBUG_FS
>>>> +/*
>>>> + * Log the state of media requests.
>>>> + * Very useful for debugging.
>>>> + */
>>>
>>> Fits on a single line.
>>
>> Ack.
>>
>>>
>>>> +static int media_device_requests(struct seq_file *file, void *priv)
>>>> +{
>>>> +	struct media_device *dev = dev_get_drvdata(file->private);
>>>> +
>>>> +	seq_printf(file, "number of requests: %d\n",
>>>> +		   atomic_read(&dev->num_requests));
>>>> +	seq_printf(file, "number of request objects: %d\n",
>>>> +		   atomic_read(&dev->num_request_objects));
>>>
>>> Newline here?
>>
>> I prefer that too.
>>
>>>
>>>> +	return 0;
>>>> +}
>>>> +#endif
>>>> +
>>>>  void media_device_init(struct media_device *mdev)
>>>>  {
>>>>  	INIT_LIST_HEAD(&mdev->entities);
>>>> @@ -697,6 +714,9 @@ void media_device_init(struct media_device *mdev)
>>>>  		media_set_bus_info(mdev->bus_info, sizeof(mdev->bus_info),
>>>>  				   mdev->dev);
>>>>  
>>>> +	atomic_set(&mdev->num_requests, 0);
>>>> +	atomic_set(&mdev->num_request_objects, 0);
>>>> +
>>>>  	dev_dbg(mdev->dev, "Media device initialized\n");
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(media_device_init);
>>>> @@ -748,6 +768,15 @@ int __must_check __media_device_register(struct media_device *mdev,
>>>>  
>>>>  	dev_dbg(mdev->dev, "Media device registered\n");
>>>>  
>>>> +#ifdef CONFIG_DEBUG_FS
>>>> +	if (!media_debugfs_root)
>>>> +		media_debugfs_root = debugfs_create_dir("media", NULL);
>>>> +	mdev->media_dir = debugfs_create_dir(dev_name(&devnode->dev),
>>>> +					     media_debugfs_root);
>>>> +	debugfs_create_devm_seqfile(&devnode->dev, "requests",
>>>> +				    mdev->media_dir, media_device_requests);
>>>> +#endif
>>>
>>> I have no objection to this but it would have been great to have the Media
>>> device lifetime set in first and MC device and devnode merged. But maybe
>>> it's too late for that. Well, at least this won't change error handling...
>>
>> Since this specific patch is not required to fix the MTK VCODEC issue, I can
>> delay this a little. Is that comment related to an existing patch ?
> 
> Yes.
> 
> I've pushed the current set here:
> <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=media-ref>. I've
> rebased it recently but it's still WiP.

Since this patch series has been going on for many years now, I do not believe
that that should prevent this specific patch from being merged. It is generally
a bad idea to do that, unless it is absolutely certain that such a patch series
will be merged in a few weeks tops.

I've carried this patch around out-of-tree ever since I started the first
request implementation because without it it is very hard to check that all
requests are properly freed. So getting this in is actually quite useful.

And when it is in I will probably add a test in test-media to improve the stateless
decoder regression test, verifying that all requests are freed.

Regards,

	Hans

> 
> ...
> 
>>>> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
>>>> index d27c1c646c2805171be3997d72210dd4d1a38e32..dbcabeffcb572ae707f5fe1f51ff719d451c6784 100644
>>>> --- a/include/media/media-devnode.h
>>>> +++ b/include/media/media-devnode.h
>>>> @@ -20,9 +20,13 @@
>>>>  #include <linux/fs.h>
>>>>  #include <linux/device.h>
>>>>  #include <linux/cdev.h>
>>>> +#include <linux/debugfs.h>
>>>>  
>>>>  struct media_device;
>>>>  
>>>> +/* debugfs top-level media directory */
>>>> +extern struct dentry *media_debugfs_root;
>>>> +
>>>>  /*
>>>>   * Flag to mark the media_devnode struct as registered. Drivers must not touch
>>>>   * this flag directly, it will be set and cleared by media_devnode_register and
>>>> diff --git a/include/media/media-request.h b/include/media/media-request.h
>>>> index 7f9af68ef19ac6de0184bbb0c0827dc59777c6dc..610ccfe8d7b20ec38e166383433f9ee208248640 100644
>>>> --- a/include/media/media-request.h
>>>> +++ b/include/media/media-request.h
>>>> @@ -292,6 +292,7 @@ struct media_request_object_ops {
>>>>   * struct media_request_object - An opaque object that belongs to a media
>>>>   *				 request
>>>>   *
>>>> + * @mdev: Media device this object belongs to
>>>
>>> This deserves at least a comment what this may be used for: generally once
>>> object is unbound, it's not related to a request anymore (nor a Media
>>> device). This field also adds a new Media device lifetime issue: nothing
>>
>> We could make it explicit by clearing the mdev pointer ?
> 
> That would probably be out of scope of this patch(set). Also see the
> patchset I referred to earlier.
> 
>>
>>> guarantees the mdev is not disappearing at a wrong time albeit this is
>>> very, very likely not user-triggerable without physically removing
>>> hardware.
>>
>> I'm not too familiar with the subject, if the MC knows it has open request
>> FD(s), why would it allow userspace from unloading its module ?
> 
> Drivers nor MC currently have a list of request file handles.
> 
> Apart from the file handles, that was the original thinking, yes, but
> devices can be also unbound without touching the driver (or other) modules.
> 
>>
>>>
>>>>   * @ops: object's operations
>>>>   * @priv: object's priv pointer
>>>>   * @req: the request this object belongs to (can be NULL)
>>>> @@ -303,6 +304,7 @@ struct media_request_object_ops {
>>>>   * another struct that contains the actual data for this request object.
>>>>   */
>>>>  struct media_request_object {
>>>> +	struct media_device *mdev;
>>>>  	const struct media_request_object_ops *ops;
>>>>  	void *priv;
>>>>  	struct media_request *req;
>>>>
> 

Re: [PATCH v3 4/5] media: vcodec: Implement manual request completion

[AngeloGioacchino Del Regno]

Il 04/06/25 22:09, Nicolas Dufresne ha scritto:
> From: Sebastian Fricke <sebastian.fricke@collabora.com>
> 
> Rework how requests are completed in the MediaTek VCodec driver, by
> implementing the new manual request completion feature, which allows to
> keep a request open while allowing to add new bitstream data.
> This is useful in this case, because the hardware has a LAT and a core
> decode work, after the LAT decode the bitstream isn't required anymore
> so the source buffer can be set done and the request stays open until
> the core decode work finishes.
> 
> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> Co-developed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>