linux-kernel - Re: [PATCH 1/7] RFC: media: Add media_request

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <42b656180e4b3f9f720173787075e165509942c1.camel@ndufresne.ca>
Date: Mon, 06 Jan 2025 15:32:04 -0500
From: Nicolas Dufresne <nicolas@...fresne.ca>
To: Dave Stevenson <dave.stevenson@...pberrypi.com>, Sakari Ailus	
 <sakari.ailus@...ux.intel.com>, Laurent Pinchart	
 <laurent.pinchart@...asonboard.com>, Mauro Carvalho Chehab
 <mchehab@...nel.org>,  Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,  Florian Fainelli
 <florian.fainelli@...adcom.com>, Broadcom internal kernel review list	
 <bcm-kernel-feedback-list@...adcom.com>, John Cox
 <john.cox@...pberrypi.com>,  Dom Cobley <dom@...pberrypi.com>, review list
 <kernel-list@...pberrypi.com>, Ezequiel Garcia	
 <ezequiel@...guardiasur.com.ar>
Cc: John Cox <jc@...esim.co.uk>, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-rpi-kernel@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org,
  John Cox <john.cox@...pberypi.com>
Subject: Re: [PATCH 1/7] RFC: media: Add media_request_{pin,unpin} API

Hi Dave,

I'm very happy to see this effort being resumed.

Le vendredi 20 décembre 2024 à 16:21 +0000, Dave Stevenson a écrit :
> From: Ezequiel Garcia <ezequiel@...guardiasur.com.ar>
> 
> This is probably not the API we will want to add, but it
> should show what semantics are needed by drivers.
> 
> The goal is to allow the OUTPUT (aka source) buffer and the
> controls associated to a request to be released from the request,
> and in particular return the OUTPUT buffer back to userspace,
> without signalling the media request fd.

My impression is that Hans proposal for media_request_manual_complete() is
better align to the direction we want to take to solve this issue. Mediatek and
Sebastian are currently experimenting with it for the MTK VCODEC driver, which
also have two stages (but in two cores).

https://lore.kernel.org/linux-media/cover.1724928939.git.hverkuil-cisco@xs4all.nl/

I'm not sure if you noticed this work, but having from more feedback on it would
be nice. So far the main challenge we have hit is that it can be difficult to
use when the first stages completes before the device_run function finishes
(reentrant). But we believe its MTK VCODEC software design that introduce this
complexity. With the 2 stage single core design, that issue won't exist, so my
impression is that this should be straightforward for you.

Nicolas

> 
> This is useful for devices that are able to pre-process
> the OUTPUT buffer, therefore able to release it before
> the decoding is finished. These drivers should signal
> the media request fd only after the CAPTURE buffer is done.
> 
> Tested-by: John Cox <john.cox@...pberypi.com>
> Signed-off-by: Ezequiel Garcia <ezequiel@...guardiasur.com.ar>
> Signed-off-by: Dave Stevenson <dave.stevenson@...pberrypi.com>
> ---
>  drivers/media/mc/mc-request.c | 35 +++++++++++++++++++++++++++++++++++
>  include/media/media-request.h | 12 ++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/drivers/media/mc/mc-request.c b/drivers/media/mc/mc-request.c
> index 5edfc2791ce7..b5334389d846 100644
> --- a/drivers/media/mc/mc-request.c
> +++ b/drivers/media/mc/mc-request.c
> @@ -499,3 +499,38 @@ void media_request_object_complete(struct media_request_object *obj)
>  		media_request_put(req);
>  }
>  EXPORT_SYMBOL_GPL(media_request_object_complete);
> +
> +void media_request_pin(struct media_request *req)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&req->lock, flags);
> +	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
> +		goto unlock;
> +	req->num_incomplete_objects++;
> +unlock:
> +	spin_unlock_irqrestore(&req->lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(media_request_pin);
> +
> +void media_request_unpin(struct media_request *req)
> +{
> +	unsigned long flags;
> +	bool completed = false;
> +
> +	spin_lock_irqsave(&req->lock, flags);
> +	if (WARN_ON(!req->num_incomplete_objects) ||
> +	    WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
> +		goto unlock;
> +
> +	if (!--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_unpin);
> diff --git a/include/media/media-request.h b/include/media/media-request.h
> index d4ac557678a7..c48cfb710959 100644
> --- a/include/media/media-request.h
> +++ b/include/media/media-request.h
> @@ -189,6 +189,10 @@ static inline void media_request_get(struct media_request *req)
>   */
>  void media_request_put(struct media_request *req);
>  
> +void media_request_pin(struct media_request *req);
> +
> +void media_request_unpin(struct media_request *req);
> +
>  /**
>   * media_request_get_by_fd - Get a media request by fd
>   *
> @@ -228,6 +232,14 @@ static inline void media_request_put(struct media_request *req)
>  {
>  }
>  
> +static inline void media_request_pin(struct media_request *req)
> +{
> +}
> +
> +static inline void media_request_unpin(struct media_request *req)
> +{
> +}
> +
>  static inline struct media_request *
>  media_request_get_by_fd(struct media_device *mdev, int request_fd)
>  {
>