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

[PATCH v2 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 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/common/mtk_vcodec_cmn_drv.h    | 13 ++++
 .../mediatek/vcodec/decoder/mtk_vcodec_dec.c       |  4 +-
 .../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c   | 50 ++++++++++++++++
 .../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h   | 19 ++++++
 .../vcodec/decoder/mtk_vcodec_dec_stateless.c      | 69 ++++++++++++++--------
 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                      | 38 +++++++++++-
 12 files changed, 274 insertions(+), 32 deletions(-)
---
base-commit: 9ddc3d6c16ea2587898a315f20f7b8fbd791dc1b
change-id: 20250312-sebastianfricke-vcodec_manual_request_completion_with_state_machine-6362c7f80a14

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

[PATCH v2 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 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>
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 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 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..645d18907be7148ca50dcc9248ff06bd8ccdf953 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.49.0

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

[Laurent Pinchart]

Hi Nicolas,

Thank you for the patch.

On Thu, Apr 10, 2025 at 11:39:56AM -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 manually complete a request in a driver,
> so add a manual complete mode for this.

I didn't immediately understand this was about delaying completion of
the request. It would be nice to make that more explicit in the commit
message and in the documentation of
media_request_mark_manual_completion(). A sample use case would also
help.

> 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 | 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 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..645d18907be7148ca50dcc9248ff06bd8ccdf953 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

I'd drop "set this field to false and " here.

> + * if @num_incomplete_objects == 0.

 * after @num_incomplete_objects reaches 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.

s/should/shall/ unless it's not a hard requirement. Any way to catch
incorrect call patterns ?

> + */
> +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

The main purpose of the function is to complete the request, not setting
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.

If that's an error then I'd document it more explicitly, as the first
sentence makes it sound that both cases are valid. Maybe

 * 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. The request will not be
 * completed in that case, and the function will WARN.

> + */
> +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

-- 
Regards,

Laurent Pinchart

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

[Nicolas Dufresne]

Hi Hans, 

another question for you below.

Le jeudi 10 avril 2025 à 20:50 +0300, Laurent Pinchart a écrit :
> Hi Nicolas,
> 
> Thank you for the patch.
> 
> On Thu, Apr 10, 2025 at 11:39:56AM -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 manually complete a request in a driver,
> > so add a manual complete mode for this.
> 
> I didn't immediately understand this was about delaying completion of
> the request. It would be nice to make that more explicit in the
> commit
> message and in the documentation of
> media_request_mark_manual_completion(). A sample use case would also
> help.
> 
> > 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>

The CI is not happy that the Sob did not match the author, shall we
ignore the CI or edit one of the two ?

Nicolas

> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> >  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
> > 5edfc2791ce7c7485def5db675bbf53ee223d837..398d0806d1d274eb8c454fc5c
> > 37b77476abe1e74 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..645d18907be7148ca50dcc924
> > 8ff06bd8ccdf953 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
> 
> I'd drop "set this field to false and " here.
> 
> > + * if @num_incomplete_objects == 0.
> 
>  * after @num_incomplete_objects reaches 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.
> 
> s/should/shall/ unless it's not a hard requirement. Any way to catch
> incorrect call patterns ?
> 
> > + */
> > +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
> 
> The main purpose of the function is to complete the request, not
> setting
> 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.
> 
> If that's an error then I'd document it more explicitly, as the first
> sentence makes it sound that both cases are valid. Maybe
> 
>  * 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. The request will not
> be
>  * completed in that case, and the function will WARN.
> 
> > + */
> > +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

-- 
Nicolas Dufresne
Principal Engineer at Collabora

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

[Hans Verkuil]

On 10/04/2025 20:26, Nicolas Dufresne wrote:
> Hi Hans, 
> 
> another question for you below.
> 
> Le jeudi 10 avril 2025 à 20:50 +0300, Laurent Pinchart a écrit :
>> Hi Nicolas,
>>
>> Thank you for the patch.
>>
>> On Thu, Apr 10, 2025 at 11:39:56AM -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 manually complete a request in a driver,
>>> so add a manual complete mode for this.
>>
>> I didn't immediately understand this was about delaying completion of
>> the request. It would be nice to make that more explicit in the
>> commit
>> message and in the documentation of
>> media_request_mark_manual_completion(). A sample use case would also
>> help.
>>
>>> 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>
> 
> The CI is not happy that the Sob did not match the author, shall we
> ignore the CI or edit one of the two ?

It's an old patch, so just edit the Sob and drop the '-cisco' part.
I stopped using that alias since it caused problems like this.

Regards,

	Hans

> 
> Nicolas
> 
>>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>> ---
>>>  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
>>> 5edfc2791ce7c7485def5db675bbf53ee223d837..398d0806d1d274eb8c454fc5c
>>> 37b77476abe1e74 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..645d18907be7148ca50dcc924
>>> 8ff06bd8ccdf953 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
>>
>> I'd drop "set this field to false and " here.
>>
>>> + * if @num_incomplete_objects == 0.
>>
>>  * after @num_incomplete_objects reaches 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.
>>
>> s/should/shall/ unless it's not a hard requirement. Any way to catch
>> incorrect call patterns ?
>>
>>> + */
>>> +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
>>
>> The main purpose of the function is to complete the request, not
>> setting
>> 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.
>>
>> If that's an error then I'd document it more explicitly, as the first
>> sentence makes it sound that both cases are valid. Maybe
>>
>>  * 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. The request will not
>> be
>>  * completed in that case, and the function will WARN.
>>
>>> + */
>>> +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
> 

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

[Nicolas Dufresne]

Hi Laurent,

Le jeudi 10 avril 2025 à 20:50 +0300, Laurent Pinchart a écrit :
> Hi Nicolas,
> 
> Thank you for the patch.
> 
> On Thu, Apr 10, 2025 at 11:39:56AM -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 manually complete a request in a driver,
> > so add a manual complete mode for this.
> 
> I didn't immediately understand this was about delaying completion of
> the request. It would be nice to make that more explicit in the commit
> message and in the documentation of
> media_request_mark_manual_completion(). A sample use case would also
> help.

I have never considered this a "delay", but an explicit completion
function. In short you want to use that if you are not satisfied with
the existing implicit logic that currently delays and complete the
request based on the media_request_object attachments.

We can add this multi-core CODEC use case as an example.

The only alternative to that would have been to allocate a driver
specific media_request_object and store it in request. This is a rather
expensive and complicated way to do this. I even got to a point I
considered having a media_request_object in the driver specific
media_request, so avoid the runtime allocs, but found the explicit
completion a lot easier to read and think about.

My only remaining thought, is why do we keep the complicated explicit
completion in the first place, perhaps because its a lot of work to
undo ? The request_object still have a purpose, since we still need to
store data in the request. And it has some benefit, that instead of
silently never completing, the complete() call will warn if you have
left over objects at an unexpected point in time.

Nicolas


> > 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 | 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 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..645d18907be7148ca50dcc9248ff06bd8ccdf953 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
> 
> I'd drop "set this field to false and " here.
> 
> > + * if @num_incomplete_objects == 0.
> 
>  * after @num_incomplete_objects reaches 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.
> 
> s/should/shall/ unless it's not a hard requirement. Any way to catch
> incorrect call patterns ?
> 
> > + */
> > +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
> 
> The main purpose of the function is to complete the request, not setting
> 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.
> 
> If that's an error then I'd document it more explicitly, as the first
> sentence makes it sound that both cases are valid. Maybe
> 
>  * 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. The request will not be
>  * completed in that case, and the function will WARN.
> 
> > + */
> > +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

-- 
Nicolas Dufresne
Principal Engineer at Collabora

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

[Laurent Pinchart]

On Thu, Apr 10, 2025 at 02:41:36PM -0400, Nicolas Dufresne wrote:
> Le jeudi 10 avril 2025 à 20:50 +0300, Laurent Pinchart a écrit :
> > On Thu, Apr 10, 2025 at 11:39:56AM -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 manually complete a request in a driver,
> > > so add a manual complete mode for this.
> > 
> > I didn't immediately understand this was about delaying completion of
> > the request. It would be nice to make that more explicit in the commit
> > message and in the documentation of
> > media_request_mark_manual_completion(). A sample use case would also
> > help.
> 
> I have never considered this a "delay", but an explicit completion
> function. In short you want to use that if you are not satisfied with
> the existing implicit logic that currently delays and complete the
> request based on the media_request_object attachments.

But you can only delay completion with this mechanism, not complete the
request earlier, right ? As far as I understand, this mechanism is about
telling the framework that the driver holds an extra "reference" to the
request, and that the request shouldn't be completed until that
"reference" is released.

If we think about this as an extra reference, do we need to forbid
dropping it while there are still objects in the request ? Could there
be use cases where driver may want to drop their "reference" earlier ?

> We can add this multi-core CODEC use case as an example.
> 
> The only alternative to that would have been to allocate a driver
> specific media_request_object and store it in request. This is a rather
> expensive and complicated way to do this. I even got to a point I
> considered having a media_request_object in the driver specific
> media_request, so avoid the runtime allocs, but found the explicit
> completion a lot easier to read and think about.

I agree that handling this with a "fake" media_request_object is
probably not nice solution, but that's an implementation detail anyway.
API-wise, I think it would be best to document the behaviour, and hide
the fact that it's implementated with an extra boolean flag.
media_request_mark_manual_completion() shouldn't mention
manual_completion, and drivers should never touch that field, so the
implementation could be reworked later if needed.

> My only remaining thought, is why do we keep the complicated explicit
> completion in the first place, perhaps because its a lot of work to
> undo ? The request_object still have a purpose, since we still need to
> store data in the request. And it has some benefit, that instead of
> silently never completing, the complete() call will warn if you have
> left over objects at an unexpected point in time.

I'm not following you when you write "keep the complicated explcit
completion". media_request_manual_complete() is the explicit completion,
isn't it ?

> > > 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 | 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 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..645d18907be7148ca50dcc9248ff06bd8ccdf953 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
> > 
> > I'd drop "set this field to false and " here.
> > 
> > > + * if @num_incomplete_objects == 0.
> > 
> >  * after @num_incomplete_objects reaches 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.
> > 
> > s/should/shall/ unless it's not a hard requirement. Any way to catch
> > incorrect call patterns ?
> > 
> > > + */
> > > +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
> > 
> > The main purpose of the function is to complete the request, not setting
> > 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.
> > 
> > If that's an error then I'd document it more explicitly, as the first
> > sentence makes it sound that both cases are valid. Maybe
> > 
> >  * 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. The request will not be
> >  * completed in that case, and the function will WARN.
> > 
> > > + */
> > > +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

-- 
Regards,

Laurent Pinchart

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

[Nicolas Dufresne]

Le jeudi 10 avril 2025 à 22:05 +0300, Laurent Pinchart a écrit :
> On Thu, Apr 10, 2025 at 02:41:36PM -0400, Nicolas Dufresne wrote:
> > Le jeudi 10 avril 2025 à 20:50 +0300, Laurent Pinchart a écrit :
> > > On Thu, Apr 10, 2025 at 11:39:56AM -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 manually complete a request in a driver,
> > > > so add a manual complete mode for this.
> > > 
> > > I didn't immediately understand this was about delaying completion of
> > > the request. It would be nice to make that more explicit in the commit
> > > message and in the documentation of
> > > media_request_mark_manual_completion(). A sample use case would also
> > > help.
> > 
> > I have never considered this a "delay", but an explicit completion
> > function. In short you want to use that if you are not satisfied with
> > the existing implicit logic that currently delays and complete the
> > request based on the media_request_object attachments.
> 
> But you can only delay completion with this mechanism, not complete the
> request earlier, right ? As far as I understand, this mechanism is about
> telling the framework that the driver holds an extra "reference" to the
> request, and that the request shouldn't be completed until that
> "reference" is released.

There is a lot of nuance to that, notably you can remove all object,
put back objects, etc.

> 
> If we think about this as an extra reference, do we need to forbid
> dropping it while there are still objects in the request ? Could there
> be use cases where driver may want to drop their "reference" earlier ?

Not that I'm aware of.

> 
> > We can add this multi-core CODEC use case as an example.
> > 
> > The only alternative to that would have been to allocate a driver
> > specific media_request_object and store it in request. This is a rather
> > expensive and complicated way to do this. I even got to a point I
> > considered having a media_request_object in the driver specific
> > media_request, so avoid the runtime allocs, but found the explicit
> > completion a lot easier to read and think about.
> 
> I agree that handling this with a "fake" media_request_object is
> probably not nice solution, but that's an implementation detail anyway.
> API-wise, I think it would be best to document the behaviour, and hide
> the fact that it's implementated with an extra boolean flag.
> media_request_mark_manual_completion() shouldn't mention
> manual_completion, and drivers should never touch that field, so the
> implementation could be reworked later if needed.

I don't follow this part, I did missed some part of the first reply, so
I'll get back to that and just ignore this one.

> 
> > My only remaining thought, is why do we keep the complicated explicit
> > completion in the first place, perhaps because its a lot of work to
> > undo ? The request_object still have a purpose, since we still need to
> > store data in the request. And it has some benefit, that instead of
> > silently never completing, the complete() call will warn if you have
> > left over objects at an unexpected point in time.
> 
> I'm not following you when you write "keep the complicated explcit
> completion". media_request_manual_complete() is the explicit completion,
> isn't it ?

Its my typo, "keep the implicit ...". Anyway, I really meant that this
is a lot more work to go all the way explicit in all drivers, and
meanwhile we have random splats when playing youtube on MTK SoC.

Nicolas

> 
> > > > 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 | 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 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..645d18907be7148ca50dcc9248ff06bd8ccdf953 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
> > > 
> > > I'd drop "set this field to false and " here.
> > > 
> > > > + * if @num_incomplete_objects == 0.
> > > 
> > >  * after @num_incomplete_objects reaches 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.
> > > 
> > > s/should/shall/ unless it's not a hard requirement. Any way to catch
> > > incorrect call patterns ?
> > > 
> > > > + */
> > > > +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
> > > 
> > > The main purpose of the function is to complete the request, not setting
> > > 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.
> > > 
> > > If that's an error then I'd document it more explicitly, as the first
> > > sentence makes it sound that both cases are valid. Maybe
> > > 
> > >  * 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. The request will not be
> > >  * completed in that case, and the function will WARN.
> > > 
> > > > + */
> > > > +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

-- 
Nicolas Dufresne
Principal Engineer at Collabora

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

[Nicolas Dufresne]

Replying on the code comment, sorry for missing some bit earlier ...

Le jeudi 10 avril 2025 à 20:50 +0300, Laurent Pinchart a écrit :
> Hi Nicolas,
> 
> Thank you for the patch.
> 
> On Thu, Apr 10, 2025 at 11:39:56AM -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 manually complete a request in a driver,
> > so add a manual complete mode for this.
> 
> I didn't immediately understand this was about delaying completion of
> the request. It would be nice to make that more explicit in the commit
> message and in the documentation of
> media_request_mark_manual_completion(). A sample use case would also
> help.
> 
> > 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 | 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 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..645d18907be7148ca50dcc9248ff06bd8ccdf953 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
> 
> I'd drop "set this field to false and " here.

I agree, it sounds like an invitation to write it directly, we don't
really want this. If Hans is fine I'll drop it.

> 
> > + * if @num_incomplete_objects == 0.
> 
>  * after @num_incomplete_objects reaches 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.
> 
> s/should/shall/ unless it's not a hard requirement. Any way to catch
> incorrect call patterns ?

I think we should be more strict, I can edit to shall.

About ways to check, perhaps I can add a lockdep_assert_held() on the
mdev->req_queue_mutex along with checking that the state is QUEUED. The
state check would catch trying to do so in req_validate().

> 
> > + */
> > +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
> 
> The main purpose of the function is to complete the request, not setting
> manual_completion to false.

Indeed, that's documenting an implementation detail.

> 
> > + *
> > + * @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.
> 
> If that's an error then I'd document it more explicitly, as the first
> sentence makes it sound that both cases are valid. Maybe

Its a programming error, I should rephrase this in the next version.

> 
>  * 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. The request will not be
>  * completed in that case, and the function will WARN.
> 
> > + */
> > +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

thanks a lot for the review,

-- 
Nicolas Dufresne
Principal Engineer at Collabora

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

[Hans Verkuil]

On 10/04/2025 22:31, Nicolas Dufresne wrote:
> Replying on the code comment, sorry for missing some bit earlier ...
> 
> Le jeudi 10 avril 2025 à 20:50 +0300, Laurent Pinchart a écrit :
>> Hi Nicolas,
>>
>> Thank you for the patch.
>>
>> On Thu, Apr 10, 2025 at 11:39:56AM -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 manually complete a request in a driver,
>>> so add a manual complete mode for this.
>>
>> I didn't immediately understand this was about delaying completion of
>> the request. It would be nice to make that more explicit in the commit
>> message and in the documentation of
>> media_request_mark_manual_completion(). A sample use case would also
>> help.
>>
>>> 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 | 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 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..645d18907be7148ca50dcc9248ff06bd8ccdf953 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
>>
>> I'd drop "set this field to false and " here.
> 
> I agree, it sounds like an invitation to write it directly, we don't
> really want this. If Hans is fine I'll drop it.

Yes, that's better. Just drop that part.

> 
>>
>>> + * if @num_incomplete_objects == 0.
>>
>>  * after @num_incomplete_objects reaches 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.
>>
>> s/should/shall/ unless it's not a hard requirement. Any way to catch
>> incorrect call patterns ?
> 
> I think we should be more strict, I can edit to shall.

Yes, that should be 'shall'.

> 
> About ways to check, perhaps I can add a lockdep_assert_held() on the
> mdev->req_queue_mutex along with checking that the state is QUEUED. The
> state check would catch trying to do so in req_validate().

A WARN_ON doesn't hurt.

> 
>>
>>> + */
>>> +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
>>
>> The main purpose of the function is to complete the request, not setting
>> manual_completion to false.
> 
> Indeed, that's documenting an implementation detail.

Change to: "Enable manual completion"

> 
>>
>>> + *
>>> + * @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.
>>
>> If that's an error then I'd document it more explicitly, as the first
>> sentence makes it sound that both cases are valid. Maybe
> 
> Its a programming error, I should rephrase this in the next version.

suggests -> is

> 
>>
>>  * 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. The request will not be
>>  * completed in that case, and the function will WARN.
>>
>>> + */
>>> +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
> 
> thanks a lot for the review,
> 

Regards,

	Hans

[PATCH v2 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 v2 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 645d18907be7148ca50dcc9248ff06bd8ccdf953..c8dad380c40767f192f30dcf1c69b9ad1310f449 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.49.0

[PATCH v2 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/common/mtk_vcodec_cmn_drv.h    | 13 +++++
 .../mediatek/vcodec/decoder/mtk_vcodec_dec.c       |  4 +-
 .../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c   | 50 +++++++++++++++++
 .../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h   | 19 +++++++
 .../vcodec/decoder/mtk_vcodec_dec_stateless.c      | 63 +++++++++++++---------
 5 files changed, 124 insertions(+), 25 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_cmn_drv.h b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_cmn_drv.h
index 6087e27bd604d24e5d37b48de5bb37eab86fc1ab..c5fd37cb60ca0cc5fd09c9243b36fbc716c56454 100644
--- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_cmn_drv.h
+++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_cmn_drv.h
@@ -105,6 +105,19 @@ enum mtk_instance_state {
 	MTK_STATE_ABORT = 4,
 };
 
+/**
+ * enum mtk_request_state - Stages of processing a request
+ * @MTK_REQUEST_RECEIVED: Hardware prepared for the LAT decode
+ * @MTK_REQUEST_LAT_DONE: LAT decode finished, the bitstream is not
+ *		      needed anymore
+ * @MTK_REQUEST_CORE_DONE: CORE decode finished
+ */
+enum mtk_request_state {
+	MTK_REQUEST_RECEIVED = 0,
+	MTK_REQUEST_LAT_DONE = 1,
+	MTK_REQUEST_CORE_DONE = 2,
+};
+
 enum mtk_fmt_type {
 	MTK_FMT_DEC = 0,
 	MTK_FMT_ENC = 1,
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.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
index 9247d92d431d8570609423156b989878f7901f1c..c80c1db509eaadd449bfd183c5eb9db0a1fc22bd 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
@@ -26,6 +26,56 @@
 #include "mtk_vcodec_dec_pm.h"
 #include "../common/mtk_vcodec_intr.h"
 
+static const char *state_to_str(enum mtk_request_state state)
+{
+	switch (state) {
+	case MTK_REQUEST_RECEIVED:
+		return "RECEIVED";
+	case MTK_REQUEST_LAT_DONE:
+		return "LAT_DONE";
+	case MTK_REQUEST_CORE_DONE:
+		return "CORE_DONE";
+	default:
+		return "UNKNOWN";
+	}
+}
+
+void mtk_request_complete(struct mtk_vcodec_dec_ctx *ctx, enum mtk_request_state state,
+			  enum vb2_buffer_state buffer_state, struct media_request *src_buf_req)
+{
+	struct mtk_request *req = req_to_mtk_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_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_REQUEST_LAT_DONE)
+			break;
+		fallthrough;
+	case MTK_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 int mtk_vcodec_get_hw_count(struct mtk_vcodec_dec_ctx *ctx, struct mtk_vcodec_dec_dev *dev)
 {
 	switch (dev->vdec_pdata->hw_arch) {
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 ac568ed14fa257d25b533b6fd6b3cd341227ecc2..cd61bf46de6918c27ed39ba64162e5f2637f93b2 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
@@ -126,6 +126,17 @@ struct mtk_vcodec_dec_pdata {
 	bool uses_stateless_api;
 };
 
+/**
+ * struct mtk_request - Media request private data.
+ *
+ * @req_state: Request completion state
+ * @req: Media Request structure
+ */
+struct mtk_request {
+	enum mtk_request_state req_state;
+	struct media_request req;
+};
+
 /**
  * struct mtk_vcodec_dec_ctx - Context (instance) private data.
  *
@@ -317,6 +328,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_request *req_to_mtk_req(struct media_request *req)
+{
+	return container_of(req, struct mtk_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)
@@ -326,6 +342,9 @@ wake_up_dec_ctx(struct mtk_vcodec_dec_ctx *ctx, unsigned int reason, unsigned in
 	wake_up_interruptible(&ctx->queue[hw_id]);
 }
 
+void mtk_request_complete(struct mtk_vcodec_dec_ctx *ctx, enum mtk_request_state state,
+			  enum vb2_buffer_state buffer_state, struct media_request *src_buf_req);
+
 #define mtk_vdec_err(ctx, fmt, args...)                               \
 	mtk_vcodec_err((ctx)->id, (ctx)->dev->plat_dev, fmt, ##args)
 
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 afa224da0f4165cf5701d6861f1f787c6317bfe4..1b08f95ba04ee137b46d61d866b030857f439429 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
@@ -247,7 +247,6 @@ static const struct v4l2_frmsize_stepwise stepwise_fhd = {
 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)
@@ -255,17 +254,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_request_complete(ctx, MTK_REQUEST_CORE_DONE, state, src_buf_req);
 }
 
 static struct vdec_fb *vdec_get_cap_buffer(struct mtk_vcodec_dec_ctx *ctx)
@@ -308,6 +297,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)
@@ -359,11 +349,12 @@ static void mtk_vdec_worker(struct work_struct *work)
 		mtk_v4l2_vdec_err(ctx, "vb2 buffer media request is NULL");
 
 	ret = vdec_if_decode(ctx, bs_src, NULL, &res_chg);
+
 	if (ret && ret != -EAGAIN) {
 		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;
@@ -372,18 +363,15 @@ static void mtk_vdec_worker(struct work_struct *work)
 	}
 
 	state = ret ? VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE;
+	if (ret != -EAGAIN)
+		mtk_request_complete(ctx, MTK_REQUEST_LAT_DONE, state, src_buf_req);
+
 	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);
-	} 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);
+		mtk_request_complete(ctx, MTK_REQUEST_CORE_DONE, 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)
@@ -711,6 +699,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_request *req;
+
+	req = kzalloc(sizeof(*req), GFP_KERNEL);
+
+	return &req->req;
+}
+
+static void fops_media_request_free(struct media_request *mreq)
+{
+	struct mtk_request *req = req_to_mtk_req(mreq);
+
+	kfree(req);
+}
+
 static int fops_media_request_validate(struct media_request *mreq)
 {
 	const unsigned int buffer_cnt = vb2_request_buffer_cnt(mreq);
@@ -731,9 +735,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_request *req = req_to_mtk_req(mreq);
+
+	media_request_mark_manual_completion(mreq);
+	req->req_state = MTK_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

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

[AngeloGioacchino Del Regno]

Il 10/04/25 17:39, 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>

This patch is great - but looks like it's worsening naming consistency across the
driver.

Please look below.

> ---
>   .../mediatek/vcodec/common/mtk_vcodec_cmn_drv.h    | 13 +++++
>   .../mediatek/vcodec/decoder/mtk_vcodec_dec.c       |  4 +-
>   .../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c   | 50 +++++++++++++++++
>   .../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h   | 19 +++++++
>   .../vcodec/decoder/mtk_vcodec_dec_stateless.c      | 63 +++++++++++++---------
>   5 files changed, 124 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_cmn_drv.h b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_cmn_drv.h
> index 6087e27bd604d24e5d37b48de5bb37eab86fc1ab..c5fd37cb60ca0cc5fd09c9243b36fbc716c56454 100644
> --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_cmn_drv.h
> +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_cmn_drv.h
> @@ -105,6 +105,19 @@ enum mtk_instance_state {
>   	MTK_STATE_ABORT = 4,
>   };
>   
> +/**
> + * enum mtk_request_state - Stages of processing a request
> + * @MTK_REQUEST_RECEIVED: Hardware prepared for the LAT decode
> + * @MTK_REQUEST_LAT_DONE: LAT decode finished, the bitstream is not
> + *		      needed anymore
> + * @MTK_REQUEST_CORE_DONE: CORE decode finished
> + */
> +enum mtk_request_state {
> +	MTK_REQUEST_RECEIVED = 0,
> +	MTK_REQUEST_LAT_DONE = 1,
> +	MTK_REQUEST_CORE_DONE = 2,
> +};
> +
>   enum mtk_fmt_type {
>   	MTK_FMT_DEC = 0,
>   	MTK_FMT_ENC = 1,
> 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.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
> index 9247d92d431d8570609423156b989878f7901f1c..c80c1db509eaadd449bfd183c5eb9db0a1fc22bd 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
> @@ -26,6 +26,56 @@
>   #include "mtk_vcodec_dec_pm.h"
>   #include "../common/mtk_vcodec_intr.h"
>   
> +static const char *state_to_str(enum mtk_request_state state)

static const char *mtk_vcodec_req_state_to_str(enum mtk_request_state state)

> +{
> +	switch (state) {
> +	case MTK_REQUEST_RECEIVED:
> +		return "RECEIVED";
> +	case MTK_REQUEST_LAT_DONE:
> +		return "LAT_DONE";
> +	case MTK_REQUEST_CORE_DONE:
> +		return "CORE_DONE";
> +	default:
> +		return "UNKNOWN";
> +	}
> +}
> +
> +void mtk_request_complete(struct mtk_vcodec_dec_ctx *ctx, enum mtk_request_state state,

void mtk_vcodec_request_complete( ....)


> +			  enum vb2_buffer_state buffer_state, struct media_request *src_buf_req)
> +{
> +	struct mtk_request *req = req_to_mtk_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_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_REQUEST_LAT_DONE)
> +			break;
> +		fallthrough;
> +	case MTK_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 int mtk_vcodec_get_hw_count(struct mtk_vcodec_dec_ctx *ctx, struct mtk_vcodec_dec_dev *dev)
>   {
>   	switch (dev->vdec_pdata->hw_arch) {
> 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 ac568ed14fa257d25b533b6fd6b3cd341227ecc2..cd61bf46de6918c27ed39ba64162e5f2637f93b2 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
> @@ -126,6 +126,17 @@ struct mtk_vcodec_dec_pdata {
>   	bool uses_stateless_api;
>   };
>   
> +/**
> + * struct mtk_request - Media request private data.
> + *
> + * @req_state: Request completion state
> + * @req: Media Request structure
> + */
> +struct mtk_request {

Maybe mtk_vcodec_dec_request? :-)

> +	enum mtk_request_state req_state;
> +	struct media_request req;
> +};
> +
>   /**
>    * struct mtk_vcodec_dec_ctx - Context (instance) private data.
>    *
> @@ -317,6 +328,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_request *req_to_mtk_req(struct media_request *req)

...and this could become req_to_dec_req ... but no strong opinions on this one
specifically, so feel free to keep this as it is.

> +{
> +	return container_of(req, struct mtk_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)

After which....

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

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

[Nicolas Dufresne]

Le lundi 14 avril 2025 à 11:11 +0200, AngeloGioacchino Del Regno a écrit :
> Il 10/04/25 17:39, 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>
> 
> This patch is great - but looks like it's worsening naming consistency across the
> driver.
> 
> Please look below.
> 
> > ---
> >   .../mediatek/vcodec/common/mtk_vcodec_cmn_drv.h    | 13 +++++
> >   .../mediatek/vcodec/decoder/mtk_vcodec_dec.c       |  4 +-
> >   .../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c   | 50 +++++++++++++++++
> >   .../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h   | 19 +++++++
> >   .../vcodec/decoder/mtk_vcodec_dec_stateless.c      | 63 +++++++++++++---------
> >   5 files changed, 124 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_cmn_drv.h b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_cmn_drv.h
> > index 6087e27bd604d24e5d37b48de5bb37eab86fc1ab..c5fd37cb60ca0cc5fd09c9243b36fbc716c56454 100644
> > --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_cmn_drv.h
> > +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_cmn_drv.h
> > @@ -105,6 +105,19 @@ enum mtk_instance_state {
> >   	MTK_STATE_ABORT = 4,
> >   };
> >   
> > +/**
> > + * enum mtk_request_state - Stages of processing a request
> > + * @MTK_REQUEST_RECEIVED: Hardware prepared for the LAT decode
> > + * @MTK_REQUEST_LAT_DONE: LAT decode finished, the bitstream is not
> > + *		      needed anymore
> > + * @MTK_REQUEST_CORE_DONE: CORE decode finished
> > + */
> > +enum mtk_request_state {
> > +	MTK_REQUEST_RECEIVED = 0,
> > +	MTK_REQUEST_LAT_DONE = 1,
> > +	MTK_REQUEST_CORE_DONE = 2,
> > +};
> > +
> >   enum mtk_fmt_type {
> >   	MTK_FMT_DEC = 0,
> >   	MTK_FMT_ENC = 1,
> > 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.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
> > index 9247d92d431d8570609423156b989878f7901f1c..c80c1db509eaadd449bfd183c5eb9db0a1fc22bd 100644
> > --- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
> > +++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
> > @@ -26,6 +26,56 @@
> >   #include "mtk_vcodec_dec_pm.h"
> >   #include "../common/mtk_vcodec_intr.h"
> >   
> > +static const char *state_to_str(enum mtk_request_state state)
> 
> static const char *mtk_vcodec_req_state_to_str(enum mtk_request_state state)

I'd keep this one an exception, its local to this C file.

> 
> > +{
> > +	switch (state) {
> > +	case MTK_REQUEST_RECEIVED:
> > +		return "RECEIVED";
> > +	case MTK_REQUEST_LAT_DONE:
> > +		return "LAT_DONE";
> > +	case MTK_REQUEST_CORE_DONE:
> > +		return "CORE_DONE";
> > +	default:
> > +		return "UNKNOWN";
> > +	}
> > +}
> > +
> > +void mtk_request_complete(struct mtk_vcodec_dec_ctx *ctx, enum mtk_request_state state,
> 
> void mtk_vcodec_request_complete( ....)

Ack, though the namspace here seems to be "mtk_vcodec_dec", so I'll opt
for that.

> 
> 
> > +			  enum vb2_buffer_state buffer_state, struct media_request *src_buf_req)
> > +{
> > +	struct mtk_request *req = req_to_mtk_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_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_REQUEST_LAT_DONE)
> > +			break;
> > +		fallthrough;
> > +	case MTK_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 int mtk_vcodec_get_hw_count(struct mtk_vcodec_dec_ctx *ctx, struct mtk_vcodec_dec_dev *dev)
> >   {
> >   	switch (dev->vdec_pdata->hw_arch) {
> > 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 ac568ed14fa257d25b533b6fd6b3cd341227ecc2..cd61bf46de6918c27ed39ba64162e5f2637f93b2 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
> > @@ -126,6 +126,17 @@ struct mtk_vcodec_dec_pdata {
> >   	bool uses_stateless_api;
> >   };
> >   
> > +/**
> > + * struct mtk_request - Media request private data.
> > + *
> > + * @req_state: Request completion state
> > + * @req: Media Request structure
> > + */
> > +struct mtk_request {
> 
> Maybe mtk_vcodec_dec_request? :-)

Ack.

> 
> > +	enum mtk_request_state req_state;
> > +	struct media_request req;
> > +};
> > +
> >   /**
> >    * struct mtk_vcodec_dec_ctx - Context (instance) private data.
> >    *
> > @@ -317,6 +328,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_request *req_to_mtk_req(struct media_request *req)
> 
> ...and this could become req_to_dec_req ... but no strong opinions on this one
> specifically, so feel free to keep this as it is.

Something like that, very subtle at this point.

> 
> > +{
> > +	return container_of(req, struct mtk_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)
> 
> After which....
> 
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

thanks,

-- 
Nicolas Dufresne
Principal Engineer at Collabora

[PATCH v2 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.

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 1b08f95ba04ee137b46d61d866b030857f439429..ab6ab9ef33dbd0d4735f82e74526b376f2502550 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
@@ -492,6 +492,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 v2 5/5] media: mtk-vcodec: Don't try to decode 422/444 VP9

[AngeloGioacchino Del Regno]

Il 10/04/25 17:40, Nicolas Dufresne ha scritto:
> This is not supported by the hardware and trying to decode
> these leads to LAT timeout errors.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

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