Re: [PATCH 2/2] media: uvcvideo: expose dma-heap hint to userspace

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Christian,

Thank you for the patch.

On Mon, Jan 23, 2023 at 01:37:56PM +0100, Christian König wrote:
> Expose an indicator to let userspace know from which dma_heap
> to allocate for buffers of this device.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index e4bcb5011360..b247026b68c5 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/atomic.h>
> +#include <linux/dma-heap.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
> @@ -1909,6 +1910,8 @@ static void uvc_unregister_video(struct uvc_device *dev)
>  
>  	if (dev->vdev.dev)
>  		v4l2_device_unregister(&dev->vdev);
> +	dma_heap_remove_device_link(&dev->udev->dev);
> +

Could we avoid having to call this explicitly in drivers, possibly using
devres in dma_heap_create_device_link() ?

>  #ifdef CONFIG_MEDIA_CONTROLLER
>  	if (media_devnode_is_registered(dev->mdev.devnode))
>  		media_device_unregister(&dev->mdev);
> @@ -2181,6 +2184,14 @@ static int uvc_probe(struct usb_interface *intf,
>  			 dev->uvc_version >> 8, dev->uvc_version & 0xff);
>  	}
>  
> +	/*
> +	 * UVC exports DMA-buf buffers with dirty CPU caches. For compatibility
> +	 * with device which can't snoop the CPU cache it's best practice to
> +	 * allocate DMA-bufs from the system DMA-heap.
> +	 */
> +	if (dma_heap_create_device_link(&dev->udev->dev, "system"))

I don't think this is the right device. A UVC device is usually a
composite USB device with an audio (UAC) function in addition to UVC,
and that may require a different heap (at least conceptually). Wouldn't
the video_device be a better candidate to expose the link ? This would
create a race condition though, as the link will be created after
userspace gets notified of the device being available.

> +		goto error;
> +
>  	/* Register the V4L2 device. */
>  	if (v4l2_device_register(&intf->dev, &dev->vdev) < 0)
>  		goto error;

-- 
Regards,

Laurent Pinchart