diff mbox series

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

Message ID 20230123123756.401692-3-christian.koenig@amd.com (mailing list archive)
State New
Delegated to: Laurent Pinchart
Headers
Series [1/2] dma-heap: add device link and unlink functions |

Commit Message

Christian König Jan. 23, 2023, 12:37 p.m. UTC 
  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(+)

Comments

Laurent Pinchart Jan. 23, 2023, 2 p.m. UTC | #1 
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;
kernel test robot Jan. 23, 2023, 11:58 p.m. UTC | #2 
Hi Christian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on media-tree/master drm-tip/drm-tip linus/master v6.2-rc5]
[cannot apply to next-20230123]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-K-nig/media-uvcvideo-expose-dma-heap-hint-to-userspace/20230123-213836
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20230123123756.401692-3-christian.koenig%40amd.com
patch subject: [PATCH 2/2] media: uvcvideo: expose dma-heap hint to userspace
config: hexagon-randconfig-r032-20230123 (https://download.01.org/0day-ci/archive/20230124/202301240717.tim1ggHo-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 4196ca3278f78c6e19246e54ab0ecb364e37d66a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/adc04dccd892eec7f84c6ec112b48df376172e48
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Christian-K-nig/media-uvcvideo-expose-dma-heap-hint-to-userspace/20230123-213836
        git checkout adc04dccd892eec7f84c6ec112b48df376172e48
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/media/usb/uvc/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/media/usb/uvc/uvc_driver.c:10:
>> include/linux/dma-heap.h:92:5: warning: no previous prototype for function 'dma_heap_create_device_link' [-Wmissing-prototypes]
   int dma_heap_create_device_link(struct device *dev, const char *heap)
       ^
   include/linux/dma-heap.h:92:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int dma_heap_create_device_link(struct device *dev, const char *heap)
   ^
   static 
>> include/linux/dma-heap.h:97:6: warning: no previous prototype for function 'dma_heap_remove_device_link' [-Wmissing-prototypes]
   void dma_heap_remove_device_link(struct device *dev)
        ^
   include/linux/dma-heap.h:97:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void dma_heap_remove_device_link(struct device *dev)
   ^
   static 
   In file included from drivers/media/usb/uvc/uvc_driver.c:16:
   In file included from include/linux/usb.h:16:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/media/usb/uvc/uvc_driver.c:16:
   In file included from include/linux/usb.h:16:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/media/usb/uvc/uvc_driver.c:16:
   In file included from include/linux/usb.h:16:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   8 warnings generated.


vim +/dma_heap_create_device_link +92 include/linux/dma-heap.h

4ce5c5c0cf31f4 Christian König 2023-01-23   91  
4ce5c5c0cf31f4 Christian König 2023-01-23  @92  int dma_heap_create_device_link(struct device *dev, const char *heap)
4ce5c5c0cf31f4 Christian König 2023-01-23   93  {
4ce5c5c0cf31f4 Christian König 2023-01-23   94  	return 0;
4ce5c5c0cf31f4 Christian König 2023-01-23   95  }
4ce5c5c0cf31f4 Christian König 2023-01-23   96  
4ce5c5c0cf31f4 Christian König 2023-01-23  @97  void dma_heap_remove_device_link(struct device *dev)
4ce5c5c0cf31f4 Christian König 2023-01-23   98  {
4ce5c5c0cf31f4 Christian König 2023-01-23   99  }
4ce5c5c0cf31f4 Christian König 2023-01-23  100
kernel test robot Jan. 24, 2023, 3:44 a.m. UTC | #3 
Hi Christian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on media-tree/master drm-tip/drm-tip linus/master v6.2-rc5]
[cannot apply to next-20230123]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-K-nig/media-uvcvideo-expose-dma-heap-hint-to-userspace/20230123-213836
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20230123123756.401692-3-christian.koenig%40amd.com
patch subject: [PATCH 2/2] media: uvcvideo: expose dma-heap hint to userspace
config: i386-randconfig-a002-20230123 (https://download.01.org/0day-ci/archive/20230124/202301241137.qT2rnQ5T-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/adc04dccd892eec7f84c6ec112b48df376172e48
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Christian-K-nig/media-uvcvideo-expose-dma-heap-hint-to-userspace/20230123-213836
        git checkout adc04dccd892eec7f84c6ec112b48df376172e48
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 olddefconfig
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/media/usb/uvc/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/media/usb/uvc/uvc_driver.c:10:
>> include/linux/dma-heap.h:92:5: warning: no previous prototype for 'dma_heap_create_device_link' [-Wmissing-prototypes]
      92 | int dma_heap_create_device_link(struct device *dev, const char *heap)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/dma-heap.h:97:6: warning: no previous prototype for 'dma_heap_remove_device_link' [-Wmissing-prototypes]
      97 | void dma_heap_remove_device_link(struct device *dev)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/dma_heap_create_device_link +92 include/linux/dma-heap.h

4ce5c5c0cf31f4 Christian König 2023-01-23   91  
4ce5c5c0cf31f4 Christian König 2023-01-23  @92  int dma_heap_create_device_link(struct device *dev, const char *heap)
4ce5c5c0cf31f4 Christian König 2023-01-23   93  {
4ce5c5c0cf31f4 Christian König 2023-01-23   94  	return 0;
4ce5c5c0cf31f4 Christian König 2023-01-23   95  }
4ce5c5c0cf31f4 Christian König 2023-01-23   96  
4ce5c5c0cf31f4 Christian König 2023-01-23  @97  void dma_heap_remove_device_link(struct device *dev)
4ce5c5c0cf31f4 Christian König 2023-01-23   98  {
4ce5c5c0cf31f4 Christian König 2023-01-23   99  }
4ce5c5c0cf31f4 Christian König 2023-01-23  100
diff mbox series

Patch

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);
+
 #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"))
+		goto error;
+
 	/* Register the V4L2 device. */
 	if (v4l2_device_register(&intf->dev, &dev->vdev) < 0)
 		goto error;