| Message ID | 20231031230435.3356103-1-m.grzeschik@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] media: videobuf2-dma-sg: fix vmap and vunmap callbacks | expand |
Hi Hans, On Mon, Nov 20, 2023 at 10:27 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > Tomasz, > > Can you review this? I did review it a few weeks ago, with some comments pending: https://patchwork.kernel.org/project/linux-media/patch/20231031230435.3356103-1-m.grzeschik@pengutronix.de/#25577798 Best, Tomasz > > Michael, I have one comment below: > > On 01/11/2023 00:04, Michael Grzeschik wrote: > > For dmabuf import users to be able to use the vaddr from another > > videobuf2-dma-sg source, the exporter needs to set a proper vaddr on > > vb2_dma_sg_dmabuf_ops_vmap callback. > > > > This patch adds vmap on map if buf->vaddr was not set, and also > > adds the vb2_dma_sg_dmabuf_ops_vunmap function to remove the mapping > > afterwards. > > > > Cc: stable@kernel.org > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > > > > --- > > v2 -> v3: resend as a single patch > > v1 -> v2: using vmap and vunmap instead of vm_map_ram and vm_unmap_ram > > > > .../media/common/videobuf2/videobuf2-dma-sg.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > > index 28f3fdfe23a298..05b43edda94a7e 100644 > > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c > > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > > @@ -489,11 +489,26 @@ static int vb2_dma_sg_dmabuf_ops_vmap(struct dma_buf *dbuf, > > { > > struct vb2_dma_sg_buf *buf = dbuf->priv; > > > > + if (!buf->vaddr) > > + buf->vaddr = vmap(buf->pages, buf->num_pages, VM_MAP, > > + PAGE_KERNEL); > > Shouldn't this check for success and return an error code if it fails? > > Regards, > > Hans > > > + > > iosys_map_set_vaddr(map, buf->vaddr); > > > > return 0; > > } > > > > +static void vb2_dma_sg_dmabuf_ops_vunmap(struct dma_buf *dbuf, > > + struct iosys_map *map) > > +{ > > + struct vb2_dma_sg_buf *buf = dbuf->priv; > > + > > + if (buf->vaddr) > > + vunmap(buf->vaddr); > > + > > + buf->vaddr = NULL; > > +} > > + > > static int vb2_dma_sg_dmabuf_ops_mmap(struct dma_buf *dbuf, > > struct vm_area_struct *vma) > > { > > @@ -508,6 +523,7 @@ static const struct dma_buf_ops vb2_dma_sg_dmabuf_ops = { > > .begin_cpu_access = vb2_dma_sg_dmabuf_ops_begin_cpu_access, > > .end_cpu_access = vb2_dma_sg_dmabuf_ops_end_cpu_access, > > .vmap = vb2_dma_sg_dmabuf_ops_vmap, > > + .vunmap = vb2_dma_sg_dmabuf_ops_vunmap, > > .mmap = vb2_dma_sg_dmabuf_ops_mmap, > > .release = vb2_dma_sg_dmabuf_ops_release, > > }; >
On Wed, Nov 01, 2023 at 12:48:21PM +0900, Tomasz Figa wrote: >Hi Michael, > > >On Wed, Nov 1, 2023 at 8:04 AM Michael Grzeschik ><m.grzeschik@pengutronix.de> wrote: >> >> For dmabuf import users to be able to use the vaddr from another >> videobuf2-dma-sg source, the exporter needs to set a proper vaddr on >> vb2_dma_sg_dmabuf_ops_vmap callback. >> >> This patch adds vmap on map if buf->vaddr was not set, and also >> adds the vb2_dma_sg_dmabuf_ops_vunmap function to remove the mapping >> afterwards. >> >> Cc: stable@kernel.org >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >> >> --- >> v2 -> v3: resend as a single patch >> v1 -> v2: using vmap and vunmap instead of vm_map_ram and vm_unmap_ram >> > >First of all, thanks for the patch! > >Please check my comments inline. > >> .../media/common/videobuf2/videobuf2-dma-sg.c | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c >> index 28f3fdfe23a298..05b43edda94a7e 100644 >> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c >> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c >> @@ -489,11 +489,26 @@ static int vb2_dma_sg_dmabuf_ops_vmap(struct dma_buf *dbuf, >> { >> struct vb2_dma_sg_buf *buf = dbuf->priv; >> >> + if (!buf->vaddr) >> + buf->vaddr = vmap(buf->pages, buf->num_pages, VM_MAP, >> + PAGE_KERNEL); >> + > >Would it make sense to use vb2_dma_sg_vaddr() instead of open coding >the mapping again? > So in the end the whole patch would look like this: buf->vaddr = vb2_dma_sg_vaddr(NULL, buf); if (!buf->vaddr) return -ENOMEM; >> iosys_map_set_vaddr(map, buf->vaddr); >> >> return 0; >> } >> >> +static void vb2_dma_sg_dmabuf_ops_vunmap(struct dma_buf *dbuf, >> + struct iosys_map *map) >> +{ >> + struct vb2_dma_sg_buf *buf = dbuf->priv; >> + >> + if (buf->vaddr) >> + vunmap(buf->vaddr); >> + >> + buf->vaddr = NULL; >> +} > >This could be potentially dangerous. Please consider the situation >when the exporting V4L2 driver needs the CPU mapping for its own >usage. It would call vb2_dma_sg_vaddr(), which would return the >mapping. Then the importer could call vunmap, which would destroy the >mapping that is still in use by the exporter internally. > >The idea of the current implementation is that we just create a kernel >mapping when it's needed first and just keep it around until the >entire buffer is destroyed. In this patch I will drop the while vunmap callback then. Michael
On Tue, Nov 21, 2023 at 10:30 PM Michael Grzeschik <mgr@pengutronix.de> wrote: > > On Wed, Nov 01, 2023 at 12:48:21PM +0900, Tomasz Figa wrote: > >Hi Michael, > > > > > >On Wed, Nov 1, 2023 at 8:04 AM Michael Grzeschik > ><m.grzeschik@pengutronix.de> wrote: > >> > >> For dmabuf import users to be able to use the vaddr from another > >> videobuf2-dma-sg source, the exporter needs to set a proper vaddr on > >> vb2_dma_sg_dmabuf_ops_vmap callback. > >> > >> This patch adds vmap on map if buf->vaddr was not set, and also > >> adds the vb2_dma_sg_dmabuf_ops_vunmap function to remove the mapping > >> afterwards. > >> > >> Cc: stable@kernel.org > >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > >> > >> --- > >> v2 -> v3: resend as a single patch > >> v1 -> v2: using vmap and vunmap instead of vm_map_ram and vm_unmap_ram > >> > > > >First of all, thanks for the patch! > > > >Please check my comments inline. > > > >> .../media/common/videobuf2/videobuf2-dma-sg.c | 16 ++++++++++++++++ > >> 1 file changed, 16 insertions(+) > >> > >> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > >> index 28f3fdfe23a298..05b43edda94a7e 100644 > >> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c > >> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > >> @@ -489,11 +489,26 @@ static int vb2_dma_sg_dmabuf_ops_vmap(struct dma_buf *dbuf, > >> { > >> struct vb2_dma_sg_buf *buf = dbuf->priv; > >> > >> + if (!buf->vaddr) > >> + buf->vaddr = vmap(buf->pages, buf->num_pages, VM_MAP, > >> + PAGE_KERNEL); > >> + > > > >Would it make sense to use vb2_dma_sg_vaddr() instead of open coding > >the mapping again? > > > > So in the end the whole patch would look like this: > > buf->vaddr = vb2_dma_sg_vaddr(NULL, buf); > if (!buf->vaddr) > return -ENOMEM; Yes, but buf->vaddr is already updated by vb2_dma_sg_vaddr(), so we should just save the return value to a local variable. > > >> iosys_map_set_vaddr(map, buf->vaddr); > >> > >> return 0; > >> } > >> > >> +static void vb2_dma_sg_dmabuf_ops_vunmap(struct dma_buf *dbuf, > >> + struct iosys_map *map) > >> +{ > >> + struct vb2_dma_sg_buf *buf = dbuf->priv; > >> + > >> + if (buf->vaddr) > >> + vunmap(buf->vaddr); > >> + > >> + buf->vaddr = NULL; > >> +} > > > >This could be potentially dangerous. Please consider the situation > >when the exporting V4L2 driver needs the CPU mapping for its own > >usage. It would call vb2_dma_sg_vaddr(), which would return the > >mapping. Then the importer could call vunmap, which would destroy the > >mapping that is still in use by the exporter internally. > > > >The idea of the current implementation is that we just create a kernel > >mapping when it's needed first and just keep it around until the > >entire buffer is destroyed. > > In this patch I will drop the while vunmap callback then. Ack. I think in the long term we may want to rework vb2_plane_vaddr() semantics so that the mapping can be reference counted, but that would require quite a bit of a change around the vb2 framework and existing drivers. (Has been on my list for a while...) Best regards, Tomasz
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c index 28f3fdfe23a298..05b43edda94a7e 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c @@ -489,11 +489,26 @@ static int vb2_dma_sg_dmabuf_ops_vmap(struct dma_buf *dbuf, { struct vb2_dma_sg_buf *buf = dbuf->priv; + if (!buf->vaddr) + buf->vaddr = vmap(buf->pages, buf->num_pages, VM_MAP, + PAGE_KERNEL); + iosys_map_set_vaddr(map, buf->vaddr); return 0; } +static void vb2_dma_sg_dmabuf_ops_vunmap(struct dma_buf *dbuf, + struct iosys_map *map) +{ + struct vb2_dma_sg_buf *buf = dbuf->priv; + + if (buf->vaddr) + vunmap(buf->vaddr); + + buf->vaddr = NULL; +} + static int vb2_dma_sg_dmabuf_ops_mmap(struct dma_buf *dbuf, struct vm_area_struct *vma) { @@ -508,6 +523,7 @@ static const struct dma_buf_ops vb2_dma_sg_dmabuf_ops = { .begin_cpu_access = vb2_dma_sg_dmabuf_ops_begin_cpu_access, .end_cpu_access = vb2_dma_sg_dmabuf_ops_end_cpu_access, .vmap = vb2_dma_sg_dmabuf_ops_vmap, + .vunmap = vb2_dma_sg_dmabuf_ops_vunmap, .mmap = vb2_dma_sg_dmabuf_ops_mmap, .release = vb2_dma_sg_dmabuf_ops_release, };
For dmabuf import users to be able to use the vaddr from another videobuf2-dma-sg source, the exporter needs to set a proper vaddr on vb2_dma_sg_dmabuf_ops_vmap callback. This patch adds vmap on map if buf->vaddr was not set, and also adds the vb2_dma_sg_dmabuf_ops_vunmap function to remove the mapping afterwards. Cc: stable@kernel.org Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- v2 -> v3: resend as a single patch v1 -> v2: using vmap and vunmap instead of vm_map_ram and vm_unmap_ram .../media/common/videobuf2/videobuf2-dma-sg.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)