From: Robert Beckett <bob.beckett@collabora.com> To: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>, dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, "Jani Nikula" <jani.nikula@linux.intel.com>, "Joonas Lahtinen" <joonas.lahtinen@linux.intel.com>, "Rodrigo Vivi" <rodrigo.vivi@intel.com>, "Tvrtko Ursulin" <tvrtko.ursulin@linux.intel.com>, "David Airlie" <airlied@linux.ie>, "Daniel Vetter" <daniel@ffwll.ch> Cc: Matthew Auld <matthew.auld@intel.com>, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 4/5] drm/i915: ttm backend dont provide mmap_offset for kernel buffers Date: Thu, 14 Apr 2022 17:13:21 +0100 [thread overview] Message-ID: <cc56e644-f20f-27ae-7a21-e40c173dfe1e@collabora.com> (raw) In-Reply-To: <07e5b1dc442e0b318ee0314f90a433216ed38dcb.camel@linux.intel.com> 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); >> > >
WARNING: multiple messages have this Message-ID (diff)
From: Robert Beckett <bob.beckett@collabora.com> To: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>, dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, "Jani Nikula" <jani.nikula@linux.intel.com>, "Joonas Lahtinen" <joonas.lahtinen@linux.intel.com>, "Rodrigo Vivi" <rodrigo.vivi@intel.com>, "Tvrtko Ursulin" <tvrtko.ursulin@linux.intel.com>, "David Airlie" <airlied@linux.ie>, "Daniel Vetter" <daniel@ffwll.ch> Cc: Matthew Auld <matthew.auld@intel.com>, linux-kernel@vger.kernel.org Subject: Re: [Intel-gfx] [PATCH v2 4/5] drm/i915: ttm backend dont provide mmap_offset for kernel buffers Date: Thu, 14 Apr 2022 17:13:21 +0100 [thread overview] Message-ID: <cc56e644-f20f-27ae-7a21-e40c173dfe1e@collabora.com> (raw) In-Reply-To: <07e5b1dc442e0b318ee0314f90a433216ed38dcb.camel@linux.intel.com> 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); >> > >
next prev parent reply other threads:[~2022-04-14 16:35 UTC|newest] Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-04-12 15:18 [PATCH v2 0/5] drm/i915: ttm for stolen region Robert Beckett 2022-04-12 15:18 ` [Intel-gfx] " Robert Beckett 2022-04-12 15:18 ` [PATCH v2 1/5] drm/i915: instantiate ttm ranger manager for stolen memory Robert Beckett 2022-04-12 15:18 ` [Intel-gfx] " Robert Beckett 2022-04-12 15:18 ` Robert Beckett 2022-04-12 15:18 ` [PATCH v2 2/5] drm/i915: sanitize mem_flags for stolen buffers Robert Beckett 2022-04-12 15:18 ` [Intel-gfx] " Robert Beckett 2022-04-12 15:18 ` Robert Beckett 2022-04-14 13:58 ` Thomas Hellström 2022-04-14 13:58 ` [Intel-gfx] " Thomas Hellström 2022-04-12 15:18 ` [PATCH v2 3/5] drm/i915: ttm move/clear logic fix Robert Beckett 2022-04-12 15:18 ` [Intel-gfx] " Robert Beckett 2022-04-12 15:18 ` Robert Beckett 2022-04-14 14:00 ` Thomas Hellström 2022-04-14 14:00 ` [Intel-gfx] " Thomas Hellström 2022-04-12 15:18 ` [PATCH v2 4/5] drm/i915: ttm backend dont provide mmap_offset for kernel buffers Robert Beckett 2022-04-12 15:18 ` [Intel-gfx] " Robert Beckett 2022-04-12 15:18 ` Robert Beckett 2022-04-14 14:05 ` Thomas Hellström 2022-04-14 14:05 ` [Intel-gfx] " Thomas Hellström 2022-04-14 16:13 ` Robert Beckett [this message] 2022-04-14 16:13 ` Robert Beckett 2022-04-27 10:14 ` Thomas Hellström 2022-04-27 10:14 ` [Intel-gfx] " Thomas Hellström 2022-04-12 15:18 ` [PATCH v2 5/5] drm/i915: stolen memory use ttm backend Robert Beckett 2022-04-12 15:18 ` [Intel-gfx] " Robert Beckett 2022-04-12 15:18 ` Robert Beckett 2022-04-12 15:34 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915: ttm for stolen region (rev2) Patchwork 2022-04-12 15:48 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=cc56e644-f20f-27ae-7a21-e40c173dfe1e@collabora.com \ --to=bob.beckett@collabora.com \ --cc=airlied@linux.ie \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=jani.nikula@linux.intel.com \ --cc=joonas.lahtinen@linux.intel.com \ --cc=linux-kernel@vger.kernel.org \ --cc=matthew.auld@intel.com \ --cc=rodrigo.vivi@intel.com \ --cc=thomas.hellstrom@linux.intel.com \ --cc=tvrtko.ursulin@linux.intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.