[PATCH 2/2] drm/etnaviv: properly implement mmaping of imported buffers

[Daniel Vetter]

On Tue, May 29, 2018 at 11:08 AM, Lucas Stach <l.stach at pengutronix.de> wrote:
> Am Dienstag, den 29.05.2018, 10:20 +0200 schrieb Daniel Vetter:
>> On Fri, May 25, 2018 at 03:42:54PM +0200, Lucas Stach wrote:
>> > The intention of the existing code was to deflect the actual work
>> > of mmaping a dma-buf to the exporter, as that one probably knows best
>> > how to handle the buffer. Unfortunately the call to drm_gem_mmap did
>> > more than what etnaviv needs in this case by actually setting up the
>> > mapping.
>> >
>> > Move mapping setup to the shm buffer type mmap implementation so we
>> > only need to look up the BO and call the buffer type mmap function
>> > from the handler.
>> >
>> > Fixes mmap behavior with dma-buf imported and userptr buffers.
>>
>> You allow mmap on userptr buffers? That sounds really nasty ...
>
> No, we don't because that's obviously crazy, even more so on ARM with
> it's special rules about aliasing mappings. The current code is buggy
> in that it first sets up the mapping and then tells userspace the mmap
> failed. After this patch we properly reject userptr mmap outright.

Ah, I didn't realize that. It sounded more like making userptr mmap
work, not rejecting it. Can't you instead just never register the mmap
offset for userptr objects? That would catch the invalid request even
earlier, and make it more obvious that mmap is not ok for userptr.
Would also remove the need to hand-roll an etnaviv version of
drm_gem_obj_mmap.

>> Also not really thrilled about dma-buf mmap forwarding either, since you
>> don't seem to forward the begin/end_cpu_access stuff either.
>
> Yeah, that's still missing, but IMHO more of a correctness fix (which
> can be done in a follow on patch), distinct of the bugfix in this
> patch.

Yeah drm_gem_obj_mmap should check for obj->import_attach imo and
scream. Maybe we should even have that check when allocating the mmap
offset, since having an mmap offset for something you can never mmap
is silly. And obj->import_attach isn't allowed to change over the
lifetime of an object.
-Daniel

>
> Regards,
> Lucas
>> -Daniel
>>
>> > Signed-off-by: Lucas Stach <l.stach at pengutronix.de>
>> > ---
>> >  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 30 ++++++++++++++++++++-------
>> >  1 file changed, 23 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> > index fcc969fa0e69..f38989960d7f 100644
>> > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> > @@ -138,6 +138,13 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj,
>> >             struct vm_area_struct *vma)
>> >  {
>> >     pgprot_t vm_page_prot;
>> > +   int ret;
>> > +
>> > +   ret = drm_gem_mmap_obj(&etnaviv_obj->base,
>> > +           drm_vma_node_size(&etnaviv_obj->base.vma_node) << PAGE_SHIFT,
>> > +           vma);
>> > +   if (ret)
>> > +           return ret;
>> >
>> >     vma->vm_flags &= ~VM_PFNMAP;
>> >     vma->vm_flags |= VM_MIXEDMAP;
>> > @@ -167,17 +174,26 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj,
>> >
>> >  int etnaviv_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>> >  {
>> > -   struct etnaviv_gem_object *obj;
>> > +   struct drm_file *priv = filp->private_data;
>> > +   struct etnaviv_gem_object *etnaviv_obj;
>> > +   struct drm_gem_object *obj;
>> >     int ret;
>> >
>> > -   ret = drm_gem_mmap(filp, vma);
>> > -   if (ret) {
>> > -           DBG("mmap failed: %d", ret);
>> > -           return ret;
>> > +   obj = drm_gem_bo_vm_lookup(filp, vma);
>> > +   if (!obj)
>> > +           return -EINVAL;
>> > +
>> > +   if (!drm_vma_node_is_allowed(&obj->vma_node, priv)) {
>> > +           drm_gem_object_put_unlocked(obj);
>> > +           return -EACCES;
>> >     }
>> >
>> > -   obj = to_etnaviv_bo(vma->vm_private_data);
>> > -   return obj->ops->mmap(obj, vma);
>> > +   etnaviv_obj = to_etnaviv_bo(obj);
>> > +
>> > +   ret = etnaviv_obj->ops->mmap(etnaviv_obj, vma);
>> > +   drm_gem_object_put_unlocked(obj);
>> > +
>> > +   return ret;
>> >  }
>> >
>> >  int etnaviv_gem_fault(struct vm_fault *vmf)
>> > --
>> > 2.17.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