On Tue, Nov 21, 2023 at 10:30 PM Michael Grzeschik <mgr@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxx
> >> Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx>
> >>
> >> ---
> >> 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
