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@xxxxxxxxx>
>>>
>>> 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@xxxxxxxxx>
>>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>
>>> ---
>>>  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