Messages in this thread | | | Date | Thu, 14 Apr 2022 17:13:21 +0100 | Subject | Re: [PATCH v2 4/5] drm/i915: ttm backend dont provide mmap_offset for kernel buffers | From | 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@collabora.com> >> --- >> 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); >> > >
| |