[PATCH] drm/cma: correctly handle non-zero offset for mmap

[PATCH] drm/cma: correctly handle non-zero offset for mmap

[Steven Price]

From: Tu Vuong <tu.vuong@arm.com>

When a CMA GEM object is exported via DRM PRIME it should be possible
to mmap the object using an offset. However drm_gem_cma_mmap_obj always
zeroed vm_pgoff.

Fix this by moving the zeroing of vm_pgoff to drm_gem_cma_mmap (which
is only used for non-PRIME mmap) and correct the size parameter in the
call to dma_mmap_wc as the offset may not be non-zero.

Signed-off-by: Tu Vuong <tu.vuong@arm.com>
Signed-off-by: Steven Price <steven.price@arm.com>
Reviewed-by: Brian Starkey <brian.starkey@arm.com>
CC: Brian Starkey <brian.starkey@arm.com>
CC: Liviu Dudau <Liviu.Dudau@arm.com>
---
 drivers/gpu/drm/drm_gem_cma_helper.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
index 373e33f22be4..25828b33c5be 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -276,15 +276,12 @@ static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj,
 	int ret;
 
 	/*
-	 * Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
-	 * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map
-	 * the whole buffer.
+	 * Clear the VM_PFNMAP flag that was set by drm_gem_mmap()
 	 */
 	vma->vm_flags &= ~VM_PFNMAP;
-	vma->vm_pgoff = 0;
 
 	ret = dma_mmap_wc(cma_obj->base.dev->dev, vma, cma_obj->vaddr,
-			  cma_obj->paddr, vma->vm_end - vma->vm_start);
+			  cma_obj->paddr, cma_obj->base.size);
 	if (ret)
 		drm_gem_vm_close(vma);
 
@@ -322,6 +319,12 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
 	gem_obj = vma->vm_private_data;
 	cma_obj = to_drm_gem_cma_obj(gem_obj);
 
+	/*
+	 * Set the vm_pgoff (used as a fake buffer offset by DRM) to 0 as we
+	 * want to map the whole buffer.
+	 */
+	vma->vm_pgoff = 0;
+
 	return drm_gem_cma_mmap_obj(cma_obj, vma);
 }
 EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
-- 
2.11.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/cma: correctly handle non-zero offset for mmap

[Daniel Vetter]

On Wed, Sep 27, 2017 at 4:01 PM, Steven Price <Steven.Price@arm.com> wrote:
> From: Tu Vuong <tu.vuong@arm.com>
>
> When a CMA GEM object is exported via DRM PRIME it should be possible
> to mmap the object using an offset. However drm_gem_cma_mmap_obj always
> zeroed vm_pgoff.

No, at least no one ever had run into this. Pretty much all drivers
work like this.

> Fix this by moving the zeroing of vm_pgoff to drm_gem_cma_mmap (which
> is only used for non-PRIME mmap) and correct the size parameter in the
> call to dma_mmap_wc as the offset may not be non-zero.

Since this is an uabi change we need the corresponding patch to
upstream open source userspace. Reviewed and all that and ready for
acceptance. See

https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements

> Signed-off-by: Tu Vuong <tu.vuong@arm.com>
> Signed-off-by: Steven Price <steven.price@arm.com>
> Reviewed-by: Brian Starkey <brian.starkey@arm.com>
> CC: Brian Starkey <brian.starkey@arm.com>
> CC: Liviu Dudau <Liviu.Dudau@arm.com>

Thanks, Daniel

> ---
>  drivers/gpu/drm/drm_gem_cma_helper.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
> index 373e33f22be4..25828b33c5be 100644
> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -276,15 +276,12 @@ static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj,
>         int ret;
>
>         /*
> -        * Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
> -        * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map
> -        * the whole buffer.
> +        * Clear the VM_PFNMAP flag that was set by drm_gem_mmap()
>          */
>         vma->vm_flags &= ~VM_PFNMAP;
> -       vma->vm_pgoff = 0;
>
>         ret = dma_mmap_wc(cma_obj->base.dev->dev, vma, cma_obj->vaddr,
> -                         cma_obj->paddr, vma->vm_end - vma->vm_start);
> +                         cma_obj->paddr, cma_obj->base.size);
>         if (ret)
>                 drm_gem_vm_close(vma);
>
> @@ -322,6 +319,12 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
>         gem_obj = vma->vm_private_data;
>         cma_obj = to_drm_gem_cma_obj(gem_obj);
>
> +       /*
> +        * Set the vm_pgoff (used as a fake buffer offset by DRM) to 0 as we
> +        * want to map the whole buffer.
> +        */
> +       vma->vm_pgoff = 0;
> +
>         return drm_gem_cma_mmap_obj(cma_obj, vma);
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
> --
> 2.11.0
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/cma: correctly handle non-zero offset for mmap

[Liviu Dudau]

Hi Daniel,

On Thu, Sep 28, 2017 at 09:40:35AM +0200, Daniel Vetter wrote:
> On Wed, Sep 27, 2017 at 4:01 PM, Steven Price <Steven.Price@arm.com> wrote:
> > From: Tu Vuong <tu.vuong@arm.com>
> >
> > When a CMA GEM object is exported via DRM PRIME it should be possible
> > to mmap the object using an offset. However drm_gem_cma_mmap_obj always
> > zeroed vm_pgoff.
> 
> No, at least no one ever had run into this. Pretty much all drivers
> work like this.

Can you be a bit more specific with "work like this"? You mean all
drivers expect mmap-ed calles to have zero offset or the fact that they
all zero vm_pgoff therefore ignore the offset?

> 
> > Fix this by moving the zeroing of vm_pgoff to drm_gem_cma_mmap (which
> > is only used for non-PRIME mmap) and correct the size parameter in the
> > call to dma_mmap_wc as the offset may not be non-zero.
> 
> Since this is an uabi change we need the corresponding patch to
> upstream open source userspace. Reviewed and all that and ready for
> acceptance. See

If ignoring the offset is considered uabi, then I agree. Otherwise it
looks to me more like a fix, rather than an uabi change.

Maybe we should have first a patch warning when vm_pgoff is not zero so
that we can find out how many in the userspace use that?

Best regards,
Liviu

> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
> 
> > Signed-off-by: Tu Vuong <tu.vuong@arm.com>
> > Signed-off-by: Steven Price <steven.price@arm.com>
> > Reviewed-by: Brian Starkey <brian.starkey@arm.com>
> > CC: Brian Starkey <brian.starkey@arm.com>
> > CC: Liviu Dudau <Liviu.Dudau@arm.com>
> 
> Thanks, Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_gem_cma_helper.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
> > index 373e33f22be4..25828b33c5be 100644
> > --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> > @@ -276,15 +276,12 @@ static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj,
> >         int ret;
> >
> >         /*
> > -        * Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
> > -        * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map
> > -        * the whole buffer.
> > +        * Clear the VM_PFNMAP flag that was set by drm_gem_mmap()
> >          */
> >         vma->vm_flags &= ~VM_PFNMAP;
> > -       vma->vm_pgoff = 0;
> >
> >         ret = dma_mmap_wc(cma_obj->base.dev->dev, vma, cma_obj->vaddr,
> > -                         cma_obj->paddr, vma->vm_end - vma->vm_start);
> > +                         cma_obj->paddr, cma_obj->base.size);
> >         if (ret)
> >                 drm_gem_vm_close(vma);
> >
> > @@ -322,6 +319,12 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
> >         gem_obj = vma->vm_private_data;
> >         cma_obj = to_drm_gem_cma_obj(gem_obj);
> >
> > +       /*
> > +        * Set the vm_pgoff (used as a fake buffer offset by DRM) to 0 as we
> > +        * want to map the whole buffer.
> > +        */
> > +       vma->vm_pgoff = 0;
> > +
> >         return drm_gem_cma_mmap_obj(cma_obj, vma);
> >  }
> >  EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
> > --
> > 2.11.0
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/cma: correctly handle non-zero offset for mmap

[Daniel Vetter]

On Thu, Sep 28, 2017 at 10:23 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> Hi Daniel,
>
> On Thu, Sep 28, 2017 at 09:40:35AM +0200, Daniel Vetter wrote:
>> On Wed, Sep 27, 2017 at 4:01 PM, Steven Price <Steven.Price@arm.com> wrote:
>> > From: Tu Vuong <tu.vuong@arm.com>
>> >
>> > When a CMA GEM object is exported via DRM PRIME it should be possible
>> > to mmap the object using an offset. However drm_gem_cma_mmap_obj always
>> > zeroed vm_pgoff.
>>
>> No, at least no one ever had run into this. Pretty much all drivers
>> work like this.
>
> Can you be a bit more specific with "work like this"? You mean all
> drivers expect mmap-ed calles to have zero offset or the fact that they
> all zero vm_pgoff therefore ignore the offset?

Iirc some drivers even outright reject the mmap if it's not aligned to
the full obj.

>> > Fix this by moving the zeroing of vm_pgoff to drm_gem_cma_mmap (which
>> > is only used for non-PRIME mmap) and correct the size parameter in the
>> > call to dma_mmap_wc as the offset may not be non-zero.
>>
>> Since this is an uabi change we need the corresponding patch to
>> upstream open source userspace. Reviewed and all that and ready for
>> acceptance. See
>
> If ignoring the offset is considered uabi, then I agree. Otherwise it
> looks to me more like a fix, rather than an uabi change.
>
> Maybe we should have first a patch warning when vm_pgoff is not zero so
> that we can find out how many in the userspace use that?

Not sure warning is a good idea, because it still means we get to
audit everything. Might as well go ahead an reject it with -EINVAL,
and make sure that's consistent across drivers. But that's all work,
so what exactly is the use-case you have and want to do sub-mmapping
for? Once we have that it's much easier to discuss what to do, instead
of abstraction discussions without the full stack at hand.

Thanks, Daniel

> Best regards,
> Liviu
>
>>
>> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
>>
>> > Signed-off-by: Tu Vuong <tu.vuong@arm.com>
>> > Signed-off-by: Steven Price <steven.price@arm.com>
>> > Reviewed-by: Brian Starkey <brian.starkey@arm.com>
>> > CC: Brian Starkey <brian.starkey@arm.com>
>> > CC: Liviu Dudau <Liviu.Dudau@arm.com>
>>
>> Thanks, Daniel
>>
>> > ---
>> >  drivers/gpu/drm/drm_gem_cma_helper.c | 13 ++++++++-----
>> >  1 file changed, 8 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
>> > index 373e33f22be4..25828b33c5be 100644
>> > --- a/drivers/gpu/drm/drm_gem_cma_helper.c
>> > +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
>> > @@ -276,15 +276,12 @@ static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj,
>> >         int ret;
>> >
>> >         /*
>> > -        * Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
>> > -        * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map
>> > -        * the whole buffer.
>> > +        * Clear the VM_PFNMAP flag that was set by drm_gem_mmap()
>> >          */
>> >         vma->vm_flags &= ~VM_PFNMAP;
>> > -       vma->vm_pgoff = 0;
>> >
>> >         ret = dma_mmap_wc(cma_obj->base.dev->dev, vma, cma_obj->vaddr,
>> > -                         cma_obj->paddr, vma->vm_end - vma->vm_start);
>> > +                         cma_obj->paddr, cma_obj->base.size);
>> >         if (ret)
>> >                 drm_gem_vm_close(vma);
>> >
>> > @@ -322,6 +319,12 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
>> >         gem_obj = vma->vm_private_data;
>> >         cma_obj = to_drm_gem_cma_obj(gem_obj);
>> >
>> > +       /*
>> > +        * Set the vm_pgoff (used as a fake buffer offset by DRM) to 0 as we
>> > +        * want to map the whole buffer.
>> > +        */
>> > +       vma->vm_pgoff = 0;
>> > +
>> >         return drm_gem_cma_mmap_obj(cma_obj, vma);
>> >  }
>> >  EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
>> > --
>> > 2.11.0
>> > _______________________________________________
>> > dri-devel mailing list
>> > dri-devel@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>>
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/cma: correctly handle non-zero offset for mmap

[Steven Price]

On Thu, 2017-09-28 at 10:29 +0200, Daniel Vetter wrote:
> On Thu, Sep 28, 2017 at 10:23 AM, Liviu Dudau <Liviu.Dudau@arm.com>
> wrote:
> > Hi Daniel,
> > 
> > On Thu, Sep 28, 2017 at 09:40:35AM +0200, Daniel Vetter wrote:
> > > On Wed, Sep 27, 2017 at 4:01 PM, Steven Price <Steven.Price@arm.c
> > > om> wrote:
> > > > From: Tu Vuong <tu.vuong@arm.com>
> > > > 
> > > > When a CMA GEM object is exported via DRM PRIME it should be
> > > > possible
> > > > to mmap the object using an offset. However
> > > > drm_gem_cma_mmap_obj always
> > > > zeroed vm_pgoff.
> > > 
> > > No, at least no one ever had run into this. Pretty much all
> > > drivers
> > > work like this.
> > 
> > Can you be a bit more specific with "work like this"? You mean all
> > drivers expect mmap-ed calles to have zero offset or the fact that
> > they
> > all zero vm_pgoff therefore ignore the offset?
> 
> Iirc some drivers even outright reject the mmap if it's not aligned
> to
> the full obj.

The mmap directly on the GEM object should behave as you say, however
if user space has used the PRIME ioctl to export the object to a
dma_buf object then surely it should behave like any other dma_buf
object?

The problem here is that the dma_buf object might be handed off to
another piece of code which has no idea where it has come from and
expects to be able to map with an offset (as it would be able to if the
dma_buf had come from another driver).

> > > > Fix this by moving the zeroing of vm_pgoff to drm_gem_cma_mmap
> > > > (which
> > > > is only used for non-PRIME mmap) and correct the size parameter
> > > > in the
> > > > call to dma_mmap_wc as the offset may not be non-zero.
> > > 
> > > Since this is an uabi change we need the corresponding patch to
> > > upstream open source userspace. Reviewed and all that and ready
> > > for
> > > acceptance. See
> > 
> > If ignoring the offset is considered uabi, then I agree. Otherwise
> > it
> > looks to me more like a fix, rather than an uabi change.
> > 
> > Maybe we should have first a patch warning when vm_pgoff is not
> > zero so
> > that we can find out how many in the userspace use that?
> 
> Not sure warning is a good idea, because it still means we get to
> audit everything. Might as well go ahead an reject it with -EINVAL,
> and make sure that's consistent across drivers. But that's all work,
> so what exactly is the use-case you have and want to do sub-mmapping
> for? Once we have that it's much easier to discuss what to do,
> instead
> of abstraction discussions without the full stack at hand.

This was initially discovered with the Mali Midgard driver. It uses
sub-mmapping when there are multiple 'frames' within one buffer (e.g.
using a display driver that only supports panning around a single pre-
allocated buffer). It may also make sense in some cases for the likes
of glReadPixels if the region specified is small and in a subset of the
pages.

Currently there's no way the caller of mmap can find out that the
offset has been ignored. One option would be to "fix" this by simply
rejecting attempts to mmap with an offset (return -EINVAL). However I
do feel this is a useful feature as it avoids the need to mmap a large
region when the caller knows only a small part of it will be needed.

I don't believe this is a uabi change as such because the current
functionality is "useless" - although obviously it is possible that
some driver is feeding garbage in to the offset and happens to work
because of the current implementation. Unless any user space drivers
have bugs of this sort there wouldn't be any need to change them (and
they would already be broken with other dma_buf exporters).

Thanks,

Steven

> Thanks, Daniel
> 
> > Best regards,
> > Liviu
> > 
> > > 
> > > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-sourc
> > > e-userspace-requirements
> > > 
> > > > Signed-off-by: Tu Vuong <tu.vuong@arm.com>
> > > > Signed-off-by: Steven Price <steven.price@arm.com>
> > > > Reviewed-by: Brian Starkey <brian.starkey@arm.com>
> > > > CC: Brian Starkey <brian.starkey@arm.com>
> > > > CC: Liviu Dudau <Liviu.Dudau@arm.com>
> > > 
> > > Thanks, Daniel
> > > 
> > > > ---
> > > >  drivers/gpu/drm/drm_gem_cma_helper.c | 13 ++++++++-----
> > > >  1 file changed, 8 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c
> > > > b/drivers/gpu/drm/drm_gem_cma_helper.c
> > > > index 373e33f22be4..25828b33c5be 100644
> > > > --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> > > > +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> > > > @@ -276,15 +276,12 @@ static int drm_gem_cma_mmap_obj(struct
> > > > drm_gem_cma_object *cma_obj,
> > > >         int ret;
> > > > 
> > > >         /*
> > > > -        * Clear the VM_PFNMAP flag that was set by
> > > > drm_gem_mmap(), and set the
> > > > -        * vm_pgoff (used as a fake buffer offset by DRM) to 0
> > > > as we want to map
> > > > -        * the whole buffer.
> > > > +        * Clear the VM_PFNMAP flag that was set by
> > > > drm_gem_mmap()
> > > >          */
> > > >         vma->vm_flags &= ~VM_PFNMAP;
> > > > -       vma->vm_pgoff = 0;
> > > > 
> > > >         ret = dma_mmap_wc(cma_obj->base.dev->dev, vma, cma_obj-
> > > > >vaddr,
> > > > -                         cma_obj->paddr, vma->vm_end - vma-
> > > > >vm_start);
> > > > +                         cma_obj->paddr, cma_obj->base.size);
> > > >         if (ret)
> > > >                 drm_gem_vm_close(vma);
> > > > 
> > > > @@ -322,6 +319,12 @@ int drm_gem_cma_mmap(struct file *filp,
> > > > struct vm_area_struct *vma)
> > > >         gem_obj = vma->vm_private_data;
> > > >         cma_obj = to_drm_gem_cma_obj(gem_obj);
> > > > 
> > > > +       /*
> > > > +        * Set the vm_pgoff (used as a fake buffer offset by
> > > > DRM) to 0 as we
> > > > +        * want to map the whole buffer.
> > > > +        */
> > > > +       vma->vm_pgoff = 0;
> > > > +
> > > >         return drm_gem_cma_mmap_obj(cma_obj, vma);
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
> > > > --
> > > > 2.11.0
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > 
> > > 
> > > 
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > 
> > --
> > ====================
> > > I would like to |
> > > fix the world,  |
> > > but they're not |
> > > giving me the   |
> > 
> >  \ source code!  /
> >   ---------------
> >     ¯\_(ツ)_/¯
> 
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel