[PATCHv3 0/3] media: mc: add manual request completion support

[PATCHv3 0/3] media: mc: add manual request completion support

[Hans Verkuil]

Normally a request contains one or more request objects, and once all
objects are marked as 'completed' the request itself is completed and
userspace gets a signal that the request is complete.

Calling vb2_buffer_done will complete a buffer object, and
v4l2_ctrl_request_complete will complete a control handler object.

In some cases (e.g. VP9 codecs) there is only a buffer object, so
as soon as the buffer is marked done, the request is marked as
completed. But in the case of mediatek, while the buffer is done
(i.e. the data is consumed by the hardware), the request isn't
completed yet as the data is still being processed. Once the
data is fully processed, the driver wants to call
v4l2_ctrl_request_complete() which will either update an existing
control handler object, or add a new control handler object to the
request containing the latest control values. But since the
request is already completed, calling v4l2_ctrl_request_complete()
will fail.

One option is to simply postpone calling vb2_buffer_done() and do
it after the call to v4l2_ctrl_request_complete(). However, in some
use-cases (e.g. secure memory) the number of available buffers is
very limited and you really want to return a buffer as soon as
possible.

In that case you want to postpone request completion until you
know the request is really ready.

Originally I thought the best way would be to make a dummy request
object, but that turned out to be overly complicated. So instead
I just add a bool manual_completion, which you set to true in
req_queue, and you call media_request_manual_complete() when you
know the request is complete. That was a lot less complicated.

The first patch adds this new manual completion code, the second
patch adds this to vicodec so it is included in regression testing,
and the last patch is an updated old patch of mine that adds debugfs
code to check if all requests and request objects are properly freed.
Without it it is really hard to verify that there are no dangling
requests or objects.

I prefer to merge this third patch as well, but if there are
objections, then I can live without it.

Regards,

	Hans

Changes since v2:
- fixed use-after-free bug in the third patch in media_request_object_release().

Changes since the RFC:

- Added WARN_ONs
- vicodec was calling media_request_manual_complete() without
  checking that it was the stateless output queue first.
- Some minor cleanups in patch 3.

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

 drivers/media/mc/mc-device.c                  | 30 +++++++++++++
 drivers/media/mc/mc-devnode.c                 |  5 +++
 drivers/media/mc/mc-request.c                 | 44 ++++++++++++++++++-
 .../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                 | 38 +++++++++++++++-
 7 files changed, 144 insertions(+), 7 deletions(-)

-- 
2.43.0

[PATCHv3 1/3] media: mc: add manual request completion

[Hans Verkuil]

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

But sometimes you want to manually complete a request in a driver,
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>
---
 drivers/media/mc/mc-request.c | 38 +++++++++++++++++++++++++++++++++--
 include/media/media-request.h | 36 ++++++++++++++++++++++++++++++++-
 2 files changed, 71 insertions(+), 3 deletions(-)

diff --git a/drivers/media/mc/mc-request.c b/drivers/media/mc/mc-request.c
index addb8f2d8939..1ad725522a7d 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);
 }
 
@@ -319,6 +320,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);
@@ -465,7 +467,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);
@@ -494,7 +496,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;
@@ -505,3 +507,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 3cd25a2717ce..6434758ab597 100644
--- a/include/media/media-request.h
+++ b/include/media/media-request.h
@@ -56,6 +56,10 @@ 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 set this field to false and complete the request
+ * if @num_incomplete_objects == 0.
  * @poll_wait: Wait queue for poll
  * @lock: Serializes access to this struct
  */
@@ -68,6 +72,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 +223,35 @@ 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 - Set manual_completion to true
+ *
+ * @req: The request
+ *
+ * Mark that the request has to be manually completed by calling
+ * media_request_manual_complete().
+ *
+ * This function should 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 - Set manual_completion to false
+ *
+ * @req: The request
+ *
+ * Set @manual_completion to false, and if @num_incomplete_objects
+ * is 0, then mark the request as completed.
+ *
+ * If there are still incomplete objects in the request, then
+ * WARN for that since that suggests a driver error.
+ */
+void media_request_manual_complete(struct media_request *req);
+
 #else
 
 static inline void media_request_get(struct media_request *req)
@@ -336,7 +370,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.43.0

[PATCHv3 2/3] media: vicodec: add support for manual completion

[Hans Verkuil]

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

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 .../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 846e90c06291..2870fa3b529c 100644
--- a/drivers/media/test-drivers/vicodec/vicodec-core.c
+++ b/drivers/media/test-drivers/vicodec/vicodec-core.c
@@ -446,8 +446,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);
@@ -1523,8 +1525,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);
@@ -1677,6 +1683,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);
 }
 
 
@@ -2001,6 +2008,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,
@@ -2021,7 +2034,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.43.0

[PATCHv3 3/3] media: mc: add debugfs node to keep track of requests

[Hans Verkuil]

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>
---
 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 c0dd4ae57227..5a458160200a 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 318e267e798e..e01764fc1491 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)
 {
@@ -237,6 +240,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 */
@@ -314,6 +318,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 1ad725522a7d..dcf43ddca368 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)
@@ -332,6 +333,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);
@@ -355,10 +357,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 *
@@ -423,6 +427,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);
@@ -430,6 +435,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 53d2a16a70b0..749c327e3c58 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 d27c1c646c28..dbcabeffcb57 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 6434758ab597..db0d9edfb38f 100644
--- a/include/media/media-request.h
+++ b/include/media/media-request.h
@@ -290,6 +290,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)
@@ -301,6 +302,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.43.0

Re: [PATCHv3 0/3] media: mc: add manual request completion support

[Steve Cho]

Hi Hans,

Few questions here.
After this patch, is there any need to reflect in the Request API doc?

On Thu, Aug 29, 2024 at 3:58 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> Normally a request contains one or more request objects, and once all
> objects are marked as 'completed' the request itself is completed and
> userspace gets a signal that the request is complete.
>
> Calling vb2_buffer_done will complete a buffer object, and
> v4l2_ctrl_request_complete will complete a control handler object.
>
> In some cases (e.g. VP9 codecs) there is only a buffer object, so
What codecs have the same behavior here?
How are things different with other codecs?

> as soon as the buffer is marked done, the request is marked as
> completed. But in the case of mediatek, while the buffer is done
> (i.e. the data is consumed by the hardware), the request isn't
> completed yet as the data is still being processed. Once the
This sounds like standard HW behavior to me...
How is it different on rockchip, which I heard was not reproducing this issue?

> data is fully processed, the driver wants to call
> v4l2_ctrl_request_complete() which will either update an existing
> control handler object, or add a new control handler object to the
> request containing the latest control values. But since the
> request is already completed, calling v4l2_ctrl_request_complete()
> will fail.
>
> One option is to simply postpone calling vb2_buffer_done() and do
> it after the call to v4l2_ctrl_request_complete(). However, in some
> use-cases (e.g. secure memory) the number of available buffers is
> very limited and you really want to return a buffer as soon as
> possible.
>
> In that case you want to postpone request completion until you
> know the request is really ready.
>
> Originally I thought the best way would be to make a dummy request
> object, but that turned out to be overly complicated. So instead
> I just add a bool manual_completion, which you set to true in
> req_queue, and you call media_request_manual_complete() when you
> know the request is complete. That was a lot less complicated.
>
> The first patch adds this new manual completion code, the second
> patch adds this to vicodec so it is included in regression testing,
> and the last patch is an updated old patch of mine that adds debugfs
> code to check if all requests and request objects are properly freed.
> Without it it is really hard to verify that there are no dangling
> requests or objects.
>
> I prefer to merge this third patch as well, but if there are
> objections, then I can live without it.
>
> Regards,
>
>         Hans
>
> Changes since v2:
> - fixed use-after-free bug in the third patch in media_request_object_release().
>
> Changes since the RFC:
>
> - Added WARN_ONs
> - vicodec was calling media_request_manual_complete() without
>   checking that it was the stateless output queue first.
> - Some minor cleanups in patch 3.
>
> 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
>
>  drivers/media/mc/mc-device.c                  | 30 +++++++++++++
>  drivers/media/mc/mc-devnode.c                 |  5 +++
>  drivers/media/mc/mc-request.c                 | 44 ++++++++++++++++++-
>  .../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                 | 38 +++++++++++++++-
>  7 files changed, 144 insertions(+), 7 deletions(-)
>
> --
> 2.43.0
>
>

Re: [PATCHv3 0/3] media: mc: add manual request completion support

[Nicolas Dufresne]

Hi Steve,

Le jeudi 03 octobre 2024 à 15:03 -0700, Steve Cho a écrit :
> Hi Hans,
> 
> Few questions here.
> After this patch, is there any need to reflect in the Request API doc?
> 
> On Thu, Aug 29, 2024 at 3:58 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> > 
> > Normally a request contains one or more request objects, and once all
> > objects are marked as 'completed' the request itself is completed and
> > userspace gets a signal that the request is complete.
> > 
> > Calling vb2_buffer_done will complete a buffer object, and
> > v4l2_ctrl_request_complete will complete a control handler object.
> > 
> > In some cases (e.g. VP9 codecs) there is only a buffer object, so
> What codecs have the same behavior here?
> How are things different with other codecs?

CODECs with single header are typically affected. VP9 have no sequence header of
any sort. Userspace will have to set an initial VP9 Frame header out of request
in order to negotiate the format with the driver. VP8 and AV1 are also possible
candidates.

> 
> > as soon as the buffer is marked done, the request is marked as
> > completed. But in the case of mediatek, while the buffer is done
> > (i.e. the data is consumed by the hardware), the request isn't
> > completed yet as the data is still being processed. Once the
> This sounds like standard HW behavior to me...
> How is it different on rockchip, which I heard was not reproducing this issue?

This only affects MTK VCODEC, and only very recent version of Chromium. Event if
Hantro and RKVDEC are not affected, at the time RK3399 support was dropped by
ChromeOS team, Chromium was still sending all the controls into all the request.
Incidently, we never tested the effect of having request without controls.

The intention here is to give drivers the control over when the following steps
occurs. In practice, the most logical event signalling order would be:

- Apply the new controls to the global state
- Mark the bitstream buffer done (bitstream buffers can immediately be refilled)
- Mark the picture buffer done
- Signal the request completion

But all drivers, except MTK, uses the helper v4l2_m2m_buf_done_and_job_finish(),
which imply

- Apply the new controls to the global state
- Mark the picture buffer done
- Mark the bitstream buffer done
- After which the last object in the request is dropped and the implicit
signalling triggers

In order to render the bistream buffers available earlier to user space, the MTK
driver is currently delaying the application of controls. But if there is no
control object in the request, this mechanism is not working and lead to the
splat we are trying to fix in a clean way.

Nicolas

> 
> > data is fully processed, the driver wants to call
> > v4l2_ctrl_request_complete() which will either update an existing
> > control handler object, or add a new control handler object to the
> > request containing the latest control values. But since the
> > request is already completed, calling v4l2_ctrl_request_complete()
> > will fail.
> > 
> > One option is to simply postpone calling vb2_buffer_done() and do
> > it after the call to v4l2_ctrl_request_complete(). However, in some
> > use-cases (e.g. secure memory) the number of available buffers is
> > very limited and you really want to return a buffer as soon as
> > possible.
> > 
> > In that case you want to postpone request completion until you
> > know the request is really ready.
> > 
> > Originally I thought the best way would be to make a dummy request
> > object, but that turned out to be overly complicated. So instead
> > I just add a bool manual_completion, which you set to true in
> > req_queue, and you call media_request_manual_complete() when you
> > know the request is complete. That was a lot less complicated.
> > 
> > The first patch adds this new manual completion code, the second
> > patch adds this to vicodec so it is included in regression testing,
> > and the last patch is an updated old patch of mine that adds debugfs
> > code to check if all requests and request objects are properly freed.
> > Without it it is really hard to verify that there are no dangling
> > requests or objects.
> > 
> > I prefer to merge this third patch as well, but if there are
> > objections, then I can live without it.
> > 
> > Regards,
> > 
> >         Hans
> > 
> > Changes since v2:
> > - fixed use-after-free bug in the third patch in media_request_object_release().
> > 
> > Changes since the RFC:
> > 
> > - Added WARN_ONs
> > - vicodec was calling media_request_manual_complete() without
> >   checking that it was the stateless output queue first.
> > - Some minor cleanups in patch 3.
> > 
> > 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
> > 
> >  drivers/media/mc/mc-device.c                  | 30 +++++++++++++
> >  drivers/media/mc/mc-devnode.c                 |  5 +++
> >  drivers/media/mc/mc-request.c                 | 44 ++++++++++++++++++-
> >  .../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                 | 38 +++++++++++++++-
> >  7 files changed, 144 insertions(+), 7 deletions(-)
> > 
> > --
> > 2.43.0
> > 
> > 

Re: [PATCHv3 0/3] media: mc: add manual request completion support

[Steve Cho]

Thank you Nicolas for the explanation.

On Fri, Oct 4, 2024 at 7:54 PM Nicolas Dufresne
<nicolas.dufresne@collabora.com> wrote:
>
> Hi Steve,
>
> Le jeudi 03 octobre 2024 à 15:03 -0700, Steve Cho a écrit :
> > Hi Hans,
> >
> > Few questions here.
> > After this patch, is there any need to reflect in the Request API doc?
> >
> > On Thu, Aug 29, 2024 at 3:58 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> > >
> > > Normally a request contains one or more request objects, and once all
> > > objects are marked as 'completed' the request itself is completed and
> > > userspace gets a signal that the request is complete.
> > >
> > > Calling vb2_buffer_done will complete a buffer object, and
> > > v4l2_ctrl_request_complete will complete a control handler object.
> > >
> > > In some cases (e.g. VP9 codecs) there is only a buffer object, so
> > What codecs have the same behavior here?
> > How are things different with other codecs?
>
> CODECs with single header are typically affected. VP9 have no sequence header of
> any sort. Userspace will have to set an initial VP9 Frame header out of request
> in order to negotiate the format with the driver. VP8 and AV1 are also possible
> candidates.
>
> >
> > > as soon as the buffer is marked done, the request is marked as
> > > completed. But in the case of mediatek, while the buffer is done
> > > (i.e. the data is consumed by the hardware), the request isn't
> > > completed yet as the data is still being processed. Once the
> > This sounds like standard HW behavior to me...
> > How is it different on rockchip, which I heard was not reproducing this issue?
>
> This only affects MTK VCODEC, and only very recent version of Chromium. Event if
I assume you are referring to what we call "v4l2 flat stateless"
implementation.
This was developed, but it was not enabled.

> Hantro and RKVDEC are not affected, at the time RK3399 support was dropped by
> ChromeOS team, Chromium was still sending all the controls into all the request.
> Incidently, we never tested the effect of having request without controls.
>
> The intention here is to give drivers the control over when the following steps
> occurs. In practice, the most logical event signalling order would be:
>
> - Apply the new controls to the global state
> - Mark the bitstream buffer done (bitstream buffers can immediately be refilled)
> - Mark the picture buffer done
> - Signal the request completion
>
> But all drivers, except MTK, uses the helper v4l2_m2m_buf_done_and_job_finish(),
> which imply
>
> - Apply the new controls to the global state
> - Mark the picture buffer done
> - Mark the bitstream buffer done
> - After which the last object in the request is dropped and the implicit
> signalling triggers
>
> In order to render the bistream buffers available earlier to user space, the MTK
> driver is currently delaying the application of controls. But if there is no
> control object in the request, this mechanism is not working and lead to the
> splat we are trying to fix in a clean way.
>
> Nicolas
>
> >
> > > data is fully processed, the driver wants to call
> > > v4l2_ctrl_request_complete() which will either update an existing
> > > control handler object, or add a new control handler object to the
> > > request containing the latest control values. But since the
> > > request is already completed, calling v4l2_ctrl_request_complete()
> > > will fail.
> > >
> > > One option is to simply postpone calling vb2_buffer_done() and do
> > > it after the call to v4l2_ctrl_request_complete(). However, in some
> > > use-cases (e.g. secure memory) the number of available buffers is
> > > very limited and you really want to return a buffer as soon as
> > > possible.
> > >
> > > In that case you want to postpone request completion until you
> > > know the request is really ready.
> > >
> > > Originally I thought the best way would be to make a dummy request
> > > object, but that turned out to be overly complicated. So instead
> > > I just add a bool manual_completion, which you set to true in
> > > req_queue, and you call media_request_manual_complete() when you
> > > know the request is complete. That was a lot less complicated.
> > >
> > > The first patch adds this new manual completion code, the second
> > > patch adds this to vicodec so it is included in regression testing,
> > > and the last patch is an updated old patch of mine that adds debugfs
> > > code to check if all requests and request objects are properly freed.
> > > Without it it is really hard to verify that there are no dangling
> > > requests or objects.
> > >
> > > I prefer to merge this third patch as well, but if there are
> > > objections, then I can live without it.
> > >
> > > Regards,
> > >
> > >         Hans
> > >
> > > Changes since v2:
> > > - fixed use-after-free bug in the third patch in media_request_object_release().
> > >
> > > Changes since the RFC:
> > >
> > > - Added WARN_ONs
> > > - vicodec was calling media_request_manual_complete() without
> > >   checking that it was the stateless output queue first.
> > > - Some minor cleanups in patch 3.
> > >
> > > 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
> > >
> > >  drivers/media/mc/mc-device.c                  | 30 +++++++++++++
> > >  drivers/media/mc/mc-devnode.c                 |  5 +++
> > >  drivers/media/mc/mc-request.c                 | 44 ++++++++++++++++++-
> > >  .../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                 | 38 +++++++++++++++-
> > >  7 files changed, 144 insertions(+), 7 deletions(-)
> > >
> > > --
> > > 2.43.0
> > >
> > >
>