[v3] media: videobuf2-dma-sg: fix vmap and vunmap callbacks

  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(+)
  

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?

>         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
  

Tomasz,

Can you review this?

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,
>  };
  

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