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@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.
> > 
> > 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@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.
> > > 
> > > > + * 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