[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 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.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?

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 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!  /
>   ---------------
>     ¯\_(ツ)_/¯



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch