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?
> 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.
Best regards,
Tomasz
