[PATCH] drm/cma: correctly handle non-zero offset for mmap
Steven Price
Steven.Price at arm.com
Thu Sep 28 10:19:36 UTC 2017
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 at 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 at arm.c
> > > om> 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?
>
> 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 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! /
> > ---------------
> > ¯\_(ツ)_/¯
>
>
>
More information about the dri-devel
mailing list