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(+)
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, };
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.
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
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-... base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next patch link: https://lore.kernel.org/r/20221111114528.608801-1-lukasz.wiecaszek%40gmail.c... 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/abc7204aeb6f9de98f5f614611551d... 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-... base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next patch link: https://lore.kernel.org/r/20221111114528.608801-1-lukasz.wiecaszek%40gmail.c... 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/abc7204aeb6f9de98f5f614611551d... 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
linaro-mm-sig@lists.linaro.org
-
Christian König -
Dmitry Osipenko -
kernel test robot -
Lukasz Wiecaszek