udmabuf: add vmap method to udmabuf_ops

  The reason behind that patch is associated with videobuf2 subsystem
(or more genrally with v4l2 framework) and user created
dma buffers (udmabuf). In some circumstances
when dealing with V4L2_MEMORY_DMABUF buffers videobuf2 subsystem
wants to use dma_buf_vmap() method on the attached dma buffer.
As udmabuf does not have .vmap operation implemented,
such dma_buf_vmap() natually fails.

videobuf2_common: [cap-000000003473b2f1] __vb2_queue_alloc: allocated 3 buffers, 1 plane(s) each
videobuf2_common: [cap-000000003473b2f1] __prepare_dmabuf: buffer for plane 0 changed
videobuf2_common: [cap-000000003473b2f1] __prepare_dmabuf: failed to map dmabuf for plane 0
videobuf2_common: [cap-000000003473b2f1] __buf_prepare: buffer preparation failed: -14

The patch itself seems to be strighforward.
It adds implementation of .vmap method to 'struct dma_buf_ops udmabuf_ops'.
.vmap method itself uses vm_map_ram() to map pages linearly
into the kernel virtual address space (only if such mapping
hasn't been created yet).

Signed-off-by: Lukasz Wiecaszek <lukasz.wiecaszek@gmail.com>
---
 drivers/dma-buf/udmabuf.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
  

Adding Dmitry as well.

Am 11.11.22 um 12:45 schrieb Lukasz Wiecaszek:
> The reason behind that patch is associated with videobuf2 subsystem
> (or more genrally with v4l2 framework) and user created
> dma buffers (udmabuf). In some circumstances
> when dealing with V4L2_MEMORY_DMABUF buffers videobuf2 subsystem
> wants to use dma_buf_vmap() method on the attached dma buffer.
> As udmabuf does not have .vmap operation implemented,
> such dma_buf_vmap() natually fails.
>
> videobuf2_common: [cap-000000003473b2f1] __vb2_queue_alloc: allocated 3 buffers, 1 plane(s) each
> videobuf2_common: [cap-000000003473b2f1] __prepare_dmabuf: buffer for plane 0 changed
> videobuf2_common: [cap-000000003473b2f1] __prepare_dmabuf: failed to map dmabuf for plane 0
> videobuf2_common: [cap-000000003473b2f1] __buf_prepare: buffer preparation failed: -14
>
> The patch itself seems to be strighforward.
> It adds implementation of .vmap method to 'struct dma_buf_ops udmabuf_ops'.
> .vmap method itself uses vm_map_ram() to map pages linearly
> into the kernel virtual address space (only if such mapping
> hasn't been created yet).

Of hand that sounds sane to me.

You should probably mention somewhere in a code comment that the cached 
vaddr is protected by the reservation lock being taken. That's not 
necessary obvious to everybody.

Apart from that looks good to me.

Regards,
Christian.

>
> Signed-off-by: Lukasz Wiecaszek <lukasz.wiecaszek@gmail.com>
> ---
>   drivers/dma-buf/udmabuf.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index 2bcdb935a3ac..8649fcbd05c4 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -12,6 +12,7 @@
>   #include <linux/slab.h>
>   #include <linux/udmabuf.h>
>   #include <linux/hugetlb.h>
> +#include <linux/vmalloc.h>
>   
>   static int list_limit = 1024;
>   module_param(list_limit, int, 0644);
> @@ -26,6 +27,7 @@ struct udmabuf {
>   	struct page **pages;
>   	struct sg_table *sg;
>   	struct miscdevice *device;
> +	void *vaddr;
>   };
>   
>   static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf)
> @@ -57,6 +59,21 @@ static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma)
>   	return 0;
>   }
>   
> +static int vmap_udmabuf(struct dma_buf *buf, struct dma_buf_map *map)
> +{
> +	struct udmabuf *ubuf = buf->priv;
> +
> +	if (!ubuf->vaddr) {
> +		ubuf->vaddr = vm_map_ram(ubuf->pages, ubuf->pagecount, -1);
> +		if (!ubuf->vaddr)
> +			return -EINVAL;
> +	}
> +
> +	dma_buf_map_set_vaddr(map, ubuf->vaddr);
> +
> +	return 0;
> +}
> +
>   static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf,
>   				     enum dma_data_direction direction)
>   {
> @@ -159,6 +176,7 @@ static const struct dma_buf_ops udmabuf_ops = {
>   	.unmap_dma_buf	   = unmap_udmabuf,
>   	.release	   = release_udmabuf,
>   	.mmap		   = mmap_udmabuf,
> +	.vmap		   = vmap_udmabuf,
>   	.begin_cpu_access  = begin_cpu_udmabuf,
>   	.end_cpu_access    = end_cpu_udmabuf,
>   };
  

On 11/11/22 15:05, Christian König wrote:
> Adding Dmitry as well.
> 
> Am 11.11.22 um 12:45 schrieb Lukasz Wiecaszek:
>> The reason behind that patch is associated with videobuf2 subsystem
>> (or more genrally with v4l2 framework) and user created
>> dma buffers (udmabuf). In some circumstances
>> when dealing with V4L2_MEMORY_DMABUF buffers videobuf2 subsystem
>> wants to use dma_buf_vmap() method on the attached dma buffer.
>> As udmabuf does not have .vmap operation implemented,
>> such dma_buf_vmap() natually fails.
>>
>> videobuf2_common: [cap-000000003473b2f1] __vb2_queue_alloc: allocated
>> 3 buffers, 1 plane(s) each
>> videobuf2_common: [cap-000000003473b2f1] __prepare_dmabuf: buffer for
>> plane 0 changed
>> videobuf2_common: [cap-000000003473b2f1] __prepare_dmabuf: failed to
>> map dmabuf for plane 0
>> videobuf2_common: [cap-000000003473b2f1] __buf_prepare: buffer
>> preparation failed: -14
>>
>> The patch itself seems to be strighforward.
>> It adds implementation of .vmap method to 'struct dma_buf_ops
>> udmabuf_ops'.
>> .vmap method itself uses vm_map_ram() to map pages linearly
>> into the kernel virtual address space (only if such mapping
>> hasn't been created yet).
> 
> Of hand that sounds sane to me.
> 
> You should probably mention somewhere in a code comment that the cached
> vaddr is protected by the reservation lock being taken. That's not
> necessary obvious to everybody.
> 
> Apart from that looks good to me.

Adding a comment won't hurt.

We have the dma_resv_assert_held() in dma_buf_vmap() that will help
spotting a missing lock at runtime by developers. While the
dmbuf_ops->vmap() shouldn't be ever used directly by importers.
  

Hi Lukasz,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master v6.1-rc4 next-20221111]
[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/Lukasz-Wiecaszek/udmabuf-add-vmap-method-to-udmabuf_ops/20221111-194718
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20221111114528.608801-1-lukasz.wiecaszek%40gmail.com
patch subject: [PATCH] udmabuf: add vmap method to udmabuf_ops
config: m68k-allyesconfig
compiler: m68k-linux-gcc (GCC) 12.1.0
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/abc7204aeb6f9de98f5f614611551d3c471f79d3
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Lukasz-Wiecaszek/udmabuf-add-vmap-method-to-udmabuf_ops/20221111-194718
        git checkout abc7204aeb6f9de98f5f614611551d3c471f79d3
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/

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 >>):

>> drivers/dma-buf/udmabuf.c:62:53: warning: 'struct dma_buf_map' declared inside parameter list will not be visible outside of this definition or declaration
      62 | static int vmap_udmabuf(struct dma_buf *buf, struct dma_buf_map *map)
         |                                                     ^~~~~~~~~~~
   drivers/dma-buf/udmabuf.c: In function 'vmap_udmabuf':
   drivers/dma-buf/udmabuf.c:72:9: error: implicit declaration of function 'dma_buf_map_set_vaddr'; did you mean 'iosys_map_set_vaddr'? [-Werror=implicit-function-declaration]
      72 |         dma_buf_map_set_vaddr(map, ubuf->vaddr);
         |         ^~~~~~~~~~~~~~~~~~~~~
         |         iosys_map_set_vaddr
   drivers/dma-buf/udmabuf.c: At top level:
   drivers/dma-buf/udmabuf.c:179:30: error: initialization of 'int (*)(struct dma_buf *, struct iosys_map *)' from incompatible pointer type 'int (*)(struct dma_buf *, struct dma_buf_map *)' [-Werror=incompatible-pointer-types]
     179 |         .vmap              = vmap_udmabuf,
         |                              ^~~~~~~~~~~~
   drivers/dma-buf/udmabuf.c:179:30: note: (near initialization for 'udmabuf_ops.vmap')
   cc1: some warnings being treated as errors


vim +62 drivers/dma-buf/udmabuf.c

    61	
  > 62	static int vmap_udmabuf(struct dma_buf *buf, struct dma_buf_map *map)
    63	{
    64		struct udmabuf *ubuf = buf->priv;
    65	
    66		if (!ubuf->vaddr) {
    67			ubuf->vaddr = vm_map_ram(ubuf->pages, ubuf->pagecount, -1);
    68			if (!ubuf->vaddr)
    69				return -EINVAL;
    70		}
    71	
    72		dma_buf_map_set_vaddr(map, ubuf->vaddr);
    73	
    74		return 0;
    75	}
    76
  

Hi Lukasz,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on linus/master v6.1-rc4 next-20221111]
[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/Lukasz-Wiecaszek/udmabuf-add-vmap-method-to-udmabuf_ops/20221111-194718
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20221111114528.608801-1-lukasz.wiecaszek%40gmail.com
patch subject: [PATCH] udmabuf: add vmap method to udmabuf_ops
config: x86_64-rhel-8.3-kselftests
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/abc7204aeb6f9de98f5f614611551d3c471f79d3
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Lukasz-Wiecaszek/udmabuf-add-vmap-method-to-udmabuf_ops/20221111-194718
        git checkout abc7204aeb6f9de98f5f614611551d3c471f79d3
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/

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

All errors (new ones prefixed by >>):

   drivers/dma-buf/udmabuf.c:62:53: warning: 'struct dma_buf_map' declared inside parameter list will not be visible outside of this definition or declaration
      62 | static int vmap_udmabuf(struct dma_buf *buf, struct dma_buf_map *map)
         |                                                     ^~~~~~~~~~~
   drivers/dma-buf/udmabuf.c: In function 'vmap_udmabuf':
>> drivers/dma-buf/udmabuf.c:72:9: error: implicit declaration of function 'dma_buf_map_set_vaddr'; did you mean 'iosys_map_set_vaddr'? [-Werror=implicit-function-declaration]
      72 |         dma_buf_map_set_vaddr(map, ubuf->vaddr);
         |         ^~~~~~~~~~~~~~~~~~~~~
         |         iosys_map_set_vaddr
   drivers/dma-buf/udmabuf.c: At top level:
>> drivers/dma-buf/udmabuf.c:179:30: error: initialization of 'int (*)(struct dma_buf *, struct iosys_map *)' from incompatible pointer type 'int (*)(struct dma_buf *, struct dma_buf_map *)' [-Werror=incompatible-pointer-types]
     179 |         .vmap              = vmap_udmabuf,
         |                              ^~~~~~~~~~~~
   drivers/dma-buf/udmabuf.c:179:30: note: (near initialization for 'udmabuf_ops.vmap')
   cc1: some warnings being treated as errors


vim +72 drivers/dma-buf/udmabuf.c

    61	
    62	static int vmap_udmabuf(struct dma_buf *buf, struct dma_buf_map *map)
    63	{
    64		struct udmabuf *ubuf = buf->priv;
    65	
    66		if (!ubuf->vaddr) {
    67			ubuf->vaddr = vm_map_ram(ubuf->pages, ubuf->pagecount, -1);
    68			if (!ubuf->vaddr)
    69				return -EINVAL;
    70		}
    71	
  > 72		dma_buf_map_set_vaddr(map, ubuf->vaddr);
    73	
    74		return 0;
    75	}
    76	
    77	static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf,
    78					     enum dma_data_direction direction)
    79	{
    80		struct udmabuf *ubuf = buf->priv;
    81		struct sg_table *sg;
    82		int ret;
    83	
    84		sg = kzalloc(sizeof(*sg), GFP_KERNEL);
    85		if (!sg)
    86			return ERR_PTR(-ENOMEM);
    87		ret = sg_alloc_table_from_pages(sg, ubuf->pages, ubuf->pagecount,
    88						0, ubuf->pagecount << PAGE_SHIFT,
    89						GFP_KERNEL);
    90		if (ret < 0)
    91			goto err;
    92		ret = dma_map_sgtable(dev, sg, direction, 0);
    93		if (ret < 0)
    94			goto err;
    95		return sg;
    96	
    97	err:
    98		sg_free_table(sg);
    99		kfree(sg);
   100		return ERR_PTR(ret);
   101	}
   102	
   103	static void put_sg_table(struct device *dev, struct sg_table *sg,
   104				 enum dma_data_direction direction)
   105	{
   106		dma_unmap_sgtable(dev, sg, direction, 0);
   107		sg_free_table(sg);
   108		kfree(sg);
   109	}
   110	
   111	static struct sg_table *map_udmabuf(struct dma_buf_attachment *at,
   112					    enum dma_data_direction direction)
   113	{
   114		return get_sg_table(at->dev, at->dmabuf, direction);
   115	}
   116	
   117	static void unmap_udmabuf(struct dma_buf_attachment *at,
   118				  struct sg_table *sg,
   119				  enum dma_data_direction direction)
   120	{
   121		return put_sg_table(at->dev, sg, direction);
   122	}
   123	
   124	static void release_udmabuf(struct dma_buf *buf)
   125	{
   126		struct udmabuf *ubuf = buf->priv;
   127		struct device *dev = ubuf->device->this_device;
   128		pgoff_t pg;
   129	
   130		if (ubuf->sg)
   131			put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL);
   132	
   133		for (pg = 0; pg < ubuf->pagecount; pg++)
   134			put_page(ubuf->pages[pg]);
   135		kfree(ubuf->pages);
   136		kfree(ubuf);
   137	}
   138	
   139	static int begin_cpu_udmabuf(struct dma_buf *buf,
   140				     enum dma_data_direction direction)
   141	{
   142		struct udmabuf *ubuf = buf->priv;
   143		struct device *dev = ubuf->device->this_device;
   144		int ret = 0;
   145	
   146		if (!ubuf->sg) {
   147			ubuf->sg = get_sg_table(dev, buf, direction);
   148			if (IS_ERR(ubuf->sg)) {
   149				ret = PTR_ERR(ubuf->sg);
   150				ubuf->sg = NULL;
   151			}
   152		} else {
   153			dma_sync_sg_for_cpu(dev, ubuf->sg->sgl, ubuf->sg->nents,
   154					    direction);
   155		}
   156	
   157		return ret;
   158	}
   159	
   160	static int end_cpu_udmabuf(struct dma_buf *buf,
   161				   enum dma_data_direction direction)
   162	{
   163		struct udmabuf *ubuf = buf->priv;
   164		struct device *dev = ubuf->device->this_device;
   165	
   166		if (!ubuf->sg)
   167			return -EINVAL;
   168	
   169		dma_sync_sg_for_device(dev, ubuf->sg->sgl, ubuf->sg->nents, direction);
   170		return 0;
   171	}
   172	
   173	static const struct dma_buf_ops udmabuf_ops = {
   174		.cache_sgt_mapping = true,
   175		.map_dma_buf	   = map_udmabuf,
   176		.unmap_dma_buf	   = unmap_udmabuf,
   177		.release	   = release_udmabuf,
   178		.mmap		   = mmap_udmabuf,
 > 179		.vmap		   = vmap_udmabuf,
   180		.begin_cpu_access  = begin_cpu_udmabuf,
   181		.end_cpu_access    = end_cpu_udmabuf,
   182	};
   183
  

On Fri, Nov 11, 2022 at 03:31:15PM +0300, Dmitry Osipenko wrote:
> On 11/11/22 15:05, Christian König wrote:
> > Adding Dmitry as well.
> > 
> > Am 11.11.22 um 12:45 schrieb Lukasz Wiecaszek:
> >> The reason behind that patch is associated with videobuf2 subsystem
> >> (or more genrally with v4l2 framework) and user created
> >> dma buffers (udmabuf). In some circumstances
> >> when dealing with V4L2_MEMORY_DMABUF buffers videobuf2 subsystem
> >> wants to use dma_buf_vmap() method on the attached dma buffer.
> >> As udmabuf does not have .vmap operation implemented,
> >> such dma_buf_vmap() natually fails.
> >>
> >> videobuf2_common: [cap-000000003473b2f1] __vb2_queue_alloc: allocated
> >> 3 buffers, 1 plane(s) each
> >> videobuf2_common: [cap-000000003473b2f1] __prepare_dmabuf: buffer for
> >> plane 0 changed
> >> videobuf2_common: [cap-000000003473b2f1] __prepare_dmabuf: failed to
> >> map dmabuf for plane 0
> >> videobuf2_common: [cap-000000003473b2f1] __buf_prepare: buffer
> >> preparation failed: -14
> >>
> >> The patch itself seems to be strighforward.
> >> It adds implementation of .vmap method to 'struct dma_buf_ops
> >> udmabuf_ops'.
> >> .vmap method itself uses vm_map_ram() to map pages linearly
> >> into the kernel virtual address space (only if such mapping
> >> hasn't been created yet).
> > 
> > Of hand that sounds sane to me.
> > 
> > You should probably mention somewhere in a code comment that the cached
> > vaddr is protected by the reservation lock being taken. That's not
> > necessary obvious to everybody.
> > 
> > Apart from that looks good to me.
> 
> Adding a comment won't hurt.
> 
> We have the dma_resv_assert_held() in dma_buf_vmap() that will help
> spotting a missing lock at runtime by developers. While the
> dmbuf_ops->vmap() shouldn't be ever used directly by importers.
> 
> -- 
> Best regards,
> Dmitry
>

Give me some time guys. I need to prepare patch agains 6.1. And this is
my first time, so now it hurts. 

Lukasz