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);
>>
>
>
next prev parent reply other threads:[~2022-04-14 16:07 UTC|newest]
Thread overview: 11+ 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 ` [PATCH v2 1/5] drm/i915: instantiate ttm ranger manager for stolen memory Robert Beckett
2022-04-12 15:18 ` [PATCH v2 2/5] drm/i915: sanitize mem_flags for stolen buffers Robert Beckett
2022-04-14 13:58 ` Thomas Hellström
2022-04-12 15:18 ` [PATCH v2 3/5] drm/i915: ttm move/clear logic fix Robert Beckett
2022-04-14 14:00 ` 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-14 14:05 ` Thomas Hellström
2022-04-14 16:13 ` Robert Beckett [this message]
2022-04-27 10:14 ` Thomas Hellström
2022-04-12 15:18 ` [PATCH v2 5/5] drm/i915: stolen memory use ttm backend Robert Beckett
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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).