Re: [RFC] drm/i915: Allow dmabuf mmap forwarding

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 13/12/2023 14:13, Christian König wrote:
> Am 13.12.23 um 12:46 schrieb Tvrtko Ursulin:
> > Hi,
> >
> > On 12/12/2023 14:10, Christian König wrote:
> > > Hi Tvrtko,
> > >
> > > Thanks for pointing this mail out once more, I've totally missed it.
> >
> > That's okay, if it was really urgent I would have re-raised the thread 
> > earlier. :) As it stands so far it is only about acceptance test 
> > suites failing and no known real use cases affected.
> >
> > > Am 12.12.23 um 11:37 schrieb Tvrtko Ursulin:
> > > > On 25/09/2023 14:16, Tvrtko Ursulin wrote:
> > > > > From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > > > >
> > > > > Allow mmap forwarding for imported buffers in order to allow 
> > > > > minigbm mmap
> > > > > to work on aperture-less platforms such as Meteorlake.
> > > > >
> > > > > So far i915 did not allow mmap on imported buffers but from minigbm
> > > > > perspective that worked because of the DRM_IOCTL_I915_GEM_MMAP_GTT 
> > > > > fall-
> > > > > back would then be attempted, and would be successful.
> > > > >
> > > > > This stops working on Meteorlake since there is no aperture.
> > > > >
> > > > > Allow i915 to mmap imported buffers using forwarding via 
> > > > > dma_buf_mmap(),
> > > > > which allows the primary minigbm path of 
> > > > > DRM_IOCTL_I915_GEM_MMAP_OFFSET /
> > > > > I915_MMAP_OFFSET_WB to work.
> > > > >
> > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > > > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > > > Cc: Christian König <christian.koenig@xxxxxxx>
> > > > > Cc: Matthew Auld <matthew.auld@xxxxxxxxx>
> > > > > Cc: Nirmoy Das <nirmoy.das@xxxxxxxxx>
> > > > > ---
> > > > > 1)
> > > > > It is unclear to me if any real userspace depends on this, but 
> > > > > there are
> > > > > certainly compliance suites which fail.
> > >
> > > Well that is actually intentional, but see below.
> > >
> > > > > 2)
> > > > > It is also a bit unclear to me if dma_buf_mmap() is exactly 
> > > > > intended for
> > > > > this kind of use. It seems that it is, but I also found some old 
> > > > > mailing
> > > > > list discussions suggesting there might be some unresolved questions
> > > > > around VMA revocation.
> > >
> > > I actually solved those a few years back by introducing the 
> > > vma_set_file() function which standardized the dance necessary for 
> > > the dma_buf_mmap() function.
> > >
> > > > > 1 + 2 = RFC for now.
> > > > >
> > > > > Daniel and Christian were involved in 2) in the past so comments would
> > > > > be appreciated.
> > > >
> > > > Any comments on this one? I don't have all the historical knowledge 
> > > > of when this was maybe attempted before and what problems were hit, 
> > > > or something. So would there be downsides or it is fine to forward it.
> > >
> > > It works technically inside the kernel and Thomas Zimmerman suggested 
> > > a patch set which made it possible to use for all DRM drivers.
> > >
> > > But IIRC this patch set was rejected with the rational that while 
> > > doing an mmap() on an imported DMA-buf works when userspace actually 
> > > does this then there is a bug in userspace. The UMD doesn't seems to 
> > > be aware of the fact that the buffer is imported and so for example 
> > > needs to call dma_buf_begin_cpu_access() and dma_buf_end_cpu_access().
> > >
> > > UMDs can trivially work around this by doing the mmap() on the 
> > > DMA-buf file descriptor instead (potentially after re-exporting it), 
> > > but the kernel really shouldn't help hide userspace bugs.
> >
> > Hm right, however why does drm_gem_shmem_mmap:
> >
> >     if (obj->import_attach) {
> >         ret = dma_buf_mmap(obj->dma_buf, vma, 0);
>
> Honestly I have absolutely no idea.
>
> > Isn't that allowing drivers which use the helper to to forward to 
> > dma_buf_mmap?
>
> Yes, Daniel mentioned that some drivers did this before we found that 
> it's actually not a good idea. It could be that this code piece was 
> meant with that and we only allow it to avoid breaking UAPI.
>
> Never the less I think we should add documentation for this.
>
> > Maybe I am getting lost in the forest of callbacks in this area.. 
> > Because it is supposed to be about shmem objects, but drivers which 
> > use the helper and rely on common prime import look and also use 
> > drm_gem_shmem_prime_import_sg_table can get there.
>
> I don't fully understand it either of hand.

Right, I will leave untangling the jungle of callbacks and helpers in my 
mind for later too. For now I think the rationale that the API does not 
allow correctnes via dma_buf_begin_cpu_access/end is I think good enough 
to pass on as will-not-implement. Thanks for the clarifications!

Regards,

Tvrtko

> Regards,
> Christian.
>
> > Regards,
> >
> > Tvrtko
> >
> > > > > Test-with: 20230925131539.32743-1-tvrtko.ursulin@xxxxxxxxxxxxxxx
> > > > >
> > > > > ---
> > > > >   drivers/gpu/drm/i915/gem/i915_gem_mman.c      | 78 
> > > > > +++++++++++++++----
> > > > >   .../gpu/drm/i915/gem/i915_gem_object_types.h  |  1 +
> > > > >   2 files changed, 65 insertions(+), 14 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c 
> > > > > b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > > > index aa4d842d4c5a..78c84c0a8b08 100644
> > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > > > @@ -5,6 +5,7 @@
> > > > >    */
> > > > >     #include <linux/anon_inodes.h>
> > > > > +#include <linux/dma-buf.h>
> > > > >   #include <linux/mman.h>
> > > > >   #include <linux/pfn_t.h>
> > > > >   #include <linux/sizes.h>
> > > > > @@ -664,6 +665,7 @@ insert_mmo(struct drm_i915_gem_object *obj, 
> > > > > struct i915_mmap_offset *mmo)
> > > > >   static struct i915_mmap_offset *
> > > > >   mmap_offset_attach(struct drm_i915_gem_object *obj,
> > > > >              enum i915_mmap_type mmap_type,
> > > > > +           bool forward_mmap,
> > > > >              struct drm_file *file)
> > > > >   {
> > > > >       struct drm_i915_private *i915 = to_i915(obj->base.dev);
> > > > > @@ -682,6 +684,7 @@ mmap_offset_attach(struct drm_i915_gem_object 
> > > > > *obj,
> > > > >         mmo->obj = obj;
> > > > >       mmo->mmap_type = mmap_type;
> > > > > +    mmo->forward_mmap = forward_mmap;
> > > > >       drm_vma_node_reset(&mmo->vma_node);
> > > > >         err = drm_vma_offset_add(obj->base.dev->vma_offset_manager,
> > > > > @@ -714,12 +717,25 @@ mmap_offset_attach(struct drm_i915_gem_object 
> > > > > *obj,
> > > > >       return ERR_PTR(err);
> > > > >   }
> > > > >   +static bool
> > > > > +should_forward_mmap(struct drm_i915_gem_object *obj,
> > > > > +            enum i915_mmap_type mmap_type)
> > > > > +{
> > > > > +    if (!obj->base.import_attach)
> > > > > +        return false;
> > > > > +
> > > > > +    return mmap_type == I915_MMAP_TYPE_WB ||
> > > > > +           mmap_type == I915_MMAP_TYPE_WC ||
> > > > > +           mmap_type == I915_MMAP_TYPE_UC;
> > > > > +}
> > > > > +
> > > > >   static int
> > > > >   __assign_mmap_offset(struct drm_i915_gem_object *obj,
> > > > >                enum i915_mmap_type mmap_type,
> > > > >                u64 *offset, struct drm_file *file)
> > > > >   {
> > > > >       struct i915_mmap_offset *mmo;
> > > > > +    bool should_forward;
> > > > >         if (i915_gem_object_never_mmap(obj))
> > > > >           return -ENODEV;
> > > > > @@ -735,12 +751,15 @@ __assign_mmap_offset(struct 
> > > > > drm_i915_gem_object *obj,
> > > > >       if (mmap_type == I915_MMAP_TYPE_FIXED)
> > > > >           return -ENODEV;
> > > > >   +    should_forward = should_forward_mmap(obj, mmap_type);
> > > > > +
> > > > >       if (mmap_type != I915_MMAP_TYPE_GTT &&
> > > > >           !i915_gem_object_has_struct_page(obj) &&
> > > > > -        !i915_gem_object_has_iomem(obj))
> > > > > +        !i915_gem_object_has_iomem(obj) &&
> > > > > +        !should_forward)
> > > > >           return -ENODEV;
> > > > >   -    mmo = mmap_offset_attach(obj, mmap_type, file);
> > > > > +    mmo = mmap_offset_attach(obj, mmap_type, should_forward, file);
> > > > >       if (IS_ERR(mmo))
> > > > >           return PTR_ERR(mmo);
> > > > >   @@ -936,6 +955,32 @@ static struct file *mmap_singleton(struct 
> > > > > drm_i915_private *i915)
> > > > >       return file;
> > > > >   }
> > > > >   +static void
> > > > > +__vma_mmap_pgprot(struct vm_area_struct *vma, enum i915_mmap_type 
> > > > > mmap_type)
> > > > > +{
> > > > > +    const pgprot_t pgprot =vm_get_page_prot(vma->vm_flags);
> > > > > +
> > > > > +    switch (mmap_type) {
> > > > > +    case I915_MMAP_TYPE_WC:
> > > > > +        vma->vm_page_prot = pgprot_writecombine(pgprot);
> > > > > +        break;
> > > > > +    case I915_MMAP_TYPE_FIXED:
> > > > > +        GEM_WARN_ON(1);
> > > > > +        fallthrough;
> > > > > +    case I915_MMAP_TYPE_WB:
> > > > > +        vma->vm_page_prot = pgprot;
> > > > > +        break;
> > > > > +    case I915_MMAP_TYPE_UC:
> > > > > +        vma->vm_page_prot = pgprot_noncached(pgprot);
> > > > > +        break;
> > > > > +    case I915_MMAP_TYPE_GTT:
> > > > > +        vma->vm_page_prot = pgprot_writecombine(pgprot);
> > > > > +        break;
> > > > > +    }
> > > > > +
> > > > > +    vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> > > > > +}
> > > > > +
> > > > >   static int
> > > > >   i915_gem_object_mmap(struct drm_i915_gem_object *obj,
> > > > >                struct i915_mmap_offset *mmo,
> > > > > @@ -953,6 +998,20 @@ i915_gem_object_mmap(struct 
> > > > > drm_i915_gem_object *obj,
> > > > >           vm_flags_clear(vma, VM_MAYWRITE);
> > > > >       }
> > > > >   +    /* dma-buf import */
> > > > > +    if (mmo && mmo->forward_mmap) {
> > > > > +        __vma_mmap_pgprot(vma, mmo->mmap_type);
> > > > > +        vm_flags_set(vma, VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP 
> > > > > | VM_IO);
> > > > > +
> > > > > +        /*
> > > > > +         * Don't have our vm_ops to drop the reference in this 
> > > > > case so
> > > > > +         * drop it now and if object goes away userspace will fault.
> > > > > +         */
> > > > > +        i915_gem_object_put(mmo->obj);
> > > > > +
> > > > > +        return dma_buf_mmap(obj->base.dma_buf, vma, 0);
> > > > > +    }
> > > > > +
> > > > >       anon = mmap_singleton(to_i915(dev));
> > > > >       if (IS_ERR(anon)) {
> > > > >           i915_gem_object_put(obj);
> > > > > @@ -982,34 +1041,25 @@ i915_gem_object_mmap(struct 
> > > > > drm_i915_gem_object *obj,
> > > > >         vma->vm_private_data = mmo;
> > > > >   +    __vma_mmap_pgprot(vma, mmo->mmap_type);
> > > > > +
> > > > >       switch (mmo->mmap_type) {
> > > > >       case I915_MMAP_TYPE_WC:
> > > > > -        vma->vm_page_prot =
> > > > > - pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> > > > >           vma->vm_ops = &vm_ops_cpu;
> > > > >           break;
> > > > > -
> > > > >       case I915_MMAP_TYPE_FIXED:
> > > > >           GEM_WARN_ON(1);
> > > > >           fallthrough;
> > > > >       case I915_MMAP_TYPE_WB:
> > > > > -        vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> > > > >           vma->vm_ops = &vm_ops_cpu;
> > > > >           break;
> > > > > -
> > > > >       case I915_MMAP_TYPE_UC:
> > > > > -        vma->vm_page_prot =
> > > > > - pgprot_noncached(vm_get_page_prot(vma->vm_flags));
> > > > >           vma->vm_ops = &vm_ops_cpu;
> > > > >           break;
> > > > > -
> > > > >       case I915_MMAP_TYPE_GTT:
> > > > > -        vma->vm_page_prot =
> > > > > - pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> > > > >           vma->vm_ops = &vm_ops_gtt;
> > > > >           break;
> > > > >       }
> > > > > -    vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> > > > >         return 0;
> > > > >   }
> > > > > @@ -1084,7 +1134,7 @@ int i915_gem_fb_mmap(struct 
> > > > > drm_i915_gem_object *obj, struct vm_area_struct *vma
> > > > >       } else {
> > > > >           /* handle stolen and smem objects */
> > > > >           mmap_type = i915_ggtt_has_aperture(ggtt) ? 
> > > > > I915_MMAP_TYPE_GTT : I915_MMAP_TYPE_WC;
> > > > > -        mmo = mmap_offset_attach(obj, mmap_type, NULL);
> > > > > +        mmo = mmap_offset_attach(obj, mmap_type, false, NULL);
> > > > >           if (IS_ERR(mmo))
> > > > >               return PTR_ERR(mmo);
> > > > >       }
> > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h 
> > > > > b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> > > > > index 0c5cdab278b6..b4f86fa020aa 100644
> > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> > > > > @@ -225,6 +225,7 @@ struct i915_mmap_offset {
> > > > >       struct drm_vma_offset_node vma_node;
> > > > >       struct drm_i915_gem_object *obj;
> > > > >       enum i915_mmap_type mmap_type;
> > > > > +    bool forward_mmap;
> > > > >         struct rb_node offset;
> > > > >   };