[PATCH v3 1/5] media: mc: add manual request completion
Hans Verkuil
hans at jjverkuil.nl
Thu Jun 5 02:37:54 PDT 2025
On 6/5/25 08:51, Sakari Ailus wrote:
> Hi Nicolas,
>
> On Wed, Jun 04, 2025 at 07:19:27PM -0400, Nicolas Dufresne wrote:
>> Le mercredi 04 juin 2025 à 21:04 +0000, Sakari Ailus a écrit :
>>> Hi Nicolas,
>>>
>>> Thanks for the update.
>>>
>>> On Wed, Jun 04, 2025 at 04:09:35PM -0400, Nicolas Dufresne wrote:
>>>> From: Hans Verkuil <hverkuil at xs4all.nl>
>>>>
>>>> By default when the last request object is completed, the whole
>>>> request completes as well.
>>>>
>>>> But sometimes you want to delay this completion to an arbitrary point in
>>>> time so add a manual complete mode for this.
>>>>
>>>> In req_queue the driver marks the request for manual completion by
>>>> calling media_request_mark_manual_completion, and when the driver
>>>> wants to manually complete the request it calls
>>>> media_request_manual_complete().
>>>>
>>>> Signed-off-by: Hans Verkuil <hverkuil-cisco at xs4all.nl>
>>>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
>>>> ---
>>>> drivers/media/mc/mc-request.c | 38 ++++++++++++++++++++++++++++++++++++--
>>>> include/media/media-request.h | 38 +++++++++++++++++++++++++++++++++++++-
>>>> 2 files changed, 73 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/media/mc/mc-request.c b/drivers/media/mc/mc-request.c
>>>> index 5edfc2791ce7c7485def5db675bbf53ee223d837..398d0806d1d274eb8c454fc5c37b77476abe1e74 100644
>>>> --- a/drivers/media/mc/mc-request.c
>>>> +++ b/drivers/media/mc/mc-request.c
>>>> @@ -54,6 +54,7 @@ static void media_request_clean(struct media_request *req)
>>>> req->access_count = 0;
>>>> WARN_ON(req->num_incomplete_objects);
>>>> req->num_incomplete_objects = 0;
>>>> + req->manual_completion = false;
>>>> wake_up_interruptible_all(&req->poll_wait);
>>>> }
>>>>
>>>> @@ -313,6 +314,7 @@ int media_request_alloc(struct media_device *mdev, int *alloc_fd)
>>>> req->mdev = mdev;
>>>> req->state = MEDIA_REQUEST_STATE_IDLE;
>>>> req->num_incomplete_objects = 0;
>>>> + req->manual_completion = false;
>>>> kref_init(&req->kref);
>>>> INIT_LIST_HEAD(&req->objects);
>>>> spin_lock_init(&req->lock);
>>>> @@ -459,7 +461,7 @@ void media_request_object_unbind(struct media_request_object *obj)
>>>>
>>>> req->num_incomplete_objects--;
>>>> if (req->state == MEDIA_REQUEST_STATE_QUEUED &&
>>>> - !req->num_incomplete_objects) {
>>>> + !req->num_incomplete_objects && !req->manual_completion) {
>>>> req->state = MEDIA_REQUEST_STATE_COMPLETE;
>>>> completed = true;
>>>> wake_up_interruptible_all(&req->poll_wait);
>>>> @@ -488,7 +490,7 @@ void media_request_object_complete(struct media_request_object *obj)
>>>> WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
>>>> goto unlock;
>>>>
>>>> - if (!--req->num_incomplete_objects) {
>>>> + if (!--req->num_incomplete_objects && !req->manual_completion) {
>>>> req->state = MEDIA_REQUEST_STATE_COMPLETE;
>>>> wake_up_interruptible_all(&req->poll_wait);
>>>> completed = true;
>>>> @@ -499,3 +501,35 @@ void media_request_object_complete(struct media_request_object *obj)
>>>> media_request_put(req);
>>>> }
>>>> EXPORT_SYMBOL_GPL(media_request_object_complete);
>>>> +
>>>> +void media_request_manual_complete(struct media_request *req)
>>>> +{
>>>> + unsigned long flags;
>>>
>>> I'd declare flags as last.
>>>
>>>> + bool completed = false;
>>>> +
>>>> + if (WARN_ON(!req))
>>>> + return;
>>>> + if (WARN_ON(!req->manual_completion))
>>>> + return;
>>>
>>> I think I'd use WARN_ON_ONCE() consistently: this is a driver (or
>>> framework) bug and telling once about it is very probably enough.
>>
>> Just to be sure, you only mean for the two checks above ? Or did
>> you mean for the entire function ?
>
> For the entire function. I thought that if this is user-triggerable, the
> amount of data ending up in logs could be very large.
It's not user-triggerable, if this happens, then it is a driver bug.
Regards,
Hans
>
>>
>>>
>>>> +
>>>> + spin_lock_irqsave(&req->lock, flags);
>>
>> In practice, if you call this specific function from two places at the same
>> time you have a bug, but I realize that moving the the warning on the check
>> manual_completion inside that lock would massively help detect that case.
>>
>> What do you think ?
>
> Seems reasonable to me.
>
>>
>>>> + if (WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
>>>> + goto unlock;
>>>> +
>>>> + req->manual_completion = false;
>>>> + /*
>>>> + * It is expected that all other objects in this request are
>>>> + * completed when this function is called. WARN if that is
>>>> + * not the case.
>>>> + */
>>>> + if (!WARN_ON(req->num_incomplete_objects)) {
>>>> + req->state = MEDIA_REQUEST_STATE_COMPLETE;
>>>> + wake_up_interruptible_all(&req->poll_wait);
>>>> + completed = true;
>>>> + }
>>>
>>> A newline would be nice here.
>>>
>>>> +unlock:
>>>> + spin_unlock_irqrestore(&req->lock, flags);
>>>> + if (completed)
>>>> + media_request_put(req);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(media_request_manual_complete);
>>>> diff --git a/include/media/media-request.h b/include/media/media-request.h
>>>> index d4ac557678a78372222704400c8c96cf3150b9d9..7f9af68ef19ac6de0184bbb0c0827dc59777c6dc 100644
>>>> --- a/include/media/media-request.h
>>>> +++ b/include/media/media-request.h
>>>> @@ -56,6 +56,9 @@ struct media_request_object;
>>>> * @access_count: count the number of request accesses that are in progress
>>>> * @objects: List of @struct media_request_object request objects
>>>> * @num_incomplete_objects: The number of incomplete objects in the request
>>>> + * @manual_completion: if true, then the request won't be marked as completed
>>>> + * when @num_incomplete_objects reaches 0. Call media_request_manual_complete()
>>>> + * to complete the request after @num_incomplete_objects == 0.
>>>> * @poll_wait: Wait queue for poll
>>>> * @lock: Serializes access to this struct
>>>> */
>>>> @@ -68,6 +71,7 @@ struct media_request {
>>>> unsigned int access_count;
>>>> struct list_head objects;
>>>> unsigned int num_incomplete_objects;
>>>> + bool manual_completion;
>>>> wait_queue_head_t poll_wait;
>>>> spinlock_t lock;
>>>> };
>>>> @@ -218,6 +222,38 @@ media_request_get_by_fd(struct media_device *mdev, int request_fd);
>>>> int media_request_alloc(struct media_device *mdev,
>>>> int *alloc_fd);
>>>>
>>>> +/**
>>>> + * media_request_mark_manual_completion - Enable manual completion
>>>> + *
>>>> + * @req: The request
>>>> + *
>>>> + * Mark that the request has to be manually completed by calling
>>>> + * media_request_manual_complete().
>>>> + *
>>>> + * This function shall be called in the req_queue callback.
>>>> + */
>>>> +static inline void
>>>> +media_request_mark_manual_completion(struct media_request *req)
>>>> +{
>>>> + req->manual_completion = true;
>>>> +}
>>>> +
>>>> +/**
>>>> + * media_request_manual_complete - Mark the request as completed
>>>> + *
>>>> + * @req: The request
>>>> + *
>>>> + * This function completes a request that was marked for manual completion by an
>>>> + * earlier call to media_request_mark_manual_completion(). The request's
>>>> + * @manual_completion flag is reset to false.
>>>
>>> s/flag/field/
>>>
>>>> + *
>>>> + * All objects contained in the request must have been completed previously. It
>>>> + * is an error to call this function otherwise. If such an error occurred, the
>>>> + * function will WARN and the object completion will be delayed until
>>>> + * @num_incomplete_objects is 0.
>>>> + */
>>>> +void media_request_manual_complete(struct media_request *req);
>>>> +
>>>> #else
>>>>
>>>> static inline void media_request_get(struct media_request *req)
>>>> @@ -336,7 +372,7 @@ void media_request_object_init(struct media_request_object *obj);
>>>> * @req: The media request
>>>> * @ops: The object ops for this object
>>>> * @priv: A driver-specific priv pointer associated with this object
>>>> - * @is_buffer: Set to true if the object a buffer object.
>>>> + * @is_buffer: Set to true if the object is a buffer object.
>>>> * @obj: The object
>>>> *
>>>> * Bind this object to the request and set the ops and priv values of
>>>>
>
More information about the Linux-mediatek
mailing list