Re: [PATCH v2 4/5] drm/i915: ttm backend dont provide mmap_offset for kernel buffers

[Robert Beckett]

On 14/04/2022 15:05, Thomas Hellström wrote:
> On Tue, 2022-04-12 at 15:18 +0000, Robert Beckett wrote:
> > stolen/kernel buffers should not be mmapable by userland.
> > do not provide callbacks to facilitate this for these buffers.
> >
> > Signed-off-by: Robert Beckett <bob.beckett@xxxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 32 +++++++++++++++++++++--
> > --
> >   1 file changed, 27 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > index a878910a563c..b20f81836c54 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > @@ -1092,8 +1092,8 @@ static void i915_ttm_unmap_virtual(struct
> > drm_i915_gem_object *obj)
> >          ttm_bo_unmap_virtual(i915_gem_to_ttm(obj));
> >   }
> >   
> > -static const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = {
> > -       .name = "i915_gem_object_ttm",
> > +static const struct drm_i915_gem_object_ops
> > i915_gem_ttm_user_obj_ops = {
> > +       .name = "i915_gem_object_ttm_user",
> >          .flags = I915_GEM_OBJECT_IS_SHRINKABLE |
> >                   I915_GEM_OBJECT_SELF_MANAGED_SHRINK_LIST,
> >   
> > @@ -1111,6 +1111,21 @@ static const struct drm_i915_gem_object_ops
> > i915_gem_ttm_obj_ops = {
> >          .mmap_ops = &vm_ops_ttm,
> >   };
> >   
> > +static const struct drm_i915_gem_object_ops
> > i915_gem_ttm_kern_obj_ops = {
> > +       .name = "i915_gem_object_ttm_kern",
> > +       .flags = I915_GEM_OBJECT_IS_SHRINKABLE |
> > +                I915_GEM_OBJECT_SELF_MANAGED_SHRINK_LIST,
> > +
> > +       .get_pages = i915_ttm_get_pages,
> > +       .put_pages = i915_ttm_put_pages,
> > +       .truncate = i915_ttm_truncate,
> > +       .shrink = i915_ttm_shrink,
> > +
> > +       .adjust_lru = i915_ttm_adjust_lru,
> > +       .delayed_free = i915_ttm_delayed_free,
> > +       .migrate = i915_ttm_migrate,
> > +};
>
> Do we really need two different ops here?
>
> Since if we don't have mmap ops, basically that tells GEM it should do
> the mmapping rather than TTM.
>
> That might of course come in handy for the shmem backend, but I don't
> fully follow why we need this for stolen.

the main rationale for doing this was to avoid 
drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c:can_mmap() presuming 
that is can use I915_MMAP_TYPE_FIXED

As the original backend also did not have mmap_offset handlers for 
stolen, this seemed like a reasonable design.

If desired, we could add a special case for the testing logic, but those 
special cases have tendency to multiply.

> Also for the framebuffer handed over from BIOS to fbdev, Does that need
> mmapping and if so, how do we handle that?

I'm not sure of the usecase there. Do you know of any igt test that 
tests this? I can investigate further if you do not.

> /Thomas
>
>
>
>
> > +
> >   void i915_ttm_bo_destroy(struct ttm_buffer_object *bo)
> >   {
> >          struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
> > @@ -1165,10 +1180,19 @@ int __i915_gem_ttm_object_init(struct
> > intel_memory_region *mem,
> >                  .no_wait_gpu = false,
> >          };
> >          enum ttm_bo_type bo_type;
> > +       const struct drm_i915_gem_object_ops *ops;
> >          int ret;
> >   
> >          drm_gem_private_object_init(&i915->drm, &obj->base, size);
> > -       i915_gem_object_init(obj, &i915_gem_ttm_obj_ops, &lock_class,
> > flags);
> > +
> > +       if (flags & I915_BO_ALLOC_USER &&
> > intel_region_to_ttm_type(mem) != I915_PL_STOLEN) {
> > +               bo_type = ttm_bo_type_device;
> > +               ops = &i915_gem_ttm_user_obj_ops;
> > +       } else {
> > +               bo_type = ttm_bo_type_kernel;
> > +               ops = &i915_gem_ttm_kern_obj_ops;
> > +       }
> > +       i915_gem_object_init(obj, ops, &lock_class, flags);
> >   
> >          obj->bo_offset = offset;
> >   
> > @@ -1178,8 +1202,6 @@ int __i915_gem_ttm_object_init(struct
> > intel_memory_region *mem,
> >   
> >          INIT_RADIX_TREE(&obj->ttm.get_io_page.radix, GFP_KERNEL |
> > __GFP_NOWARN);
> >          mutex_init(&obj->ttm.get_io_page.lock);
> > -       bo_type = (obj->flags & I915_BO_ALLOC_USER) ?
> > ttm_bo_type_device :
> > -               ttm_bo_type_kernel;
> >   
> >          obj->base.vma_node.driver_private = i915_gem_to_ttm(obj);
> >