[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 at arm.com> wrote:
> > From: Tu Vuong <tu.vuong at 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 at arm.com>
> > Signed-off-by: Steven Price <steven.price at arm.com>
> > Reviewed-by: Brian Starkey <brian.starkey at arm.com>
> > CC: Brian Starkey <brian.starkey at arm.com>
> > CC: Liviu Dudau <Liviu.Dudau at 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 at 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!  /
  ---------------
    ¯\_(ツ)_/¯