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 <[email protected]>
---
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,
};
--
2.25.1
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 <[email protected]>
> ---
> 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.
--
Best regards,
Dmitry
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 <[email protected]>
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
--
0-DAY CI Kernel Test Service
https://01.org/lkp
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 <[email protected]>
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
--
0-DAY CI Kernel Test Service
https://01.org/lkp
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