[Intel-gfx] [PATCH v7 13/63] drm/i915: Reject more ioctls for userptr
Jason Ekstrand
jason at jlekstrand.net
Mon Feb 1 19:11:19 UTC 2021
On January 29, 2021 05:42:47 Maarten Lankhorst
<maarten.lankhorst at linux.intel.com> wrote:
> Op 28-01-2021 om 17:47 schreef Jason Ekstrand:
>> On Thu, Jan 28, 2021 at 10:26 AM Maarten Lankhorst
>> <maarten.lankhorst at linux.intel.com> wrote:
>>> There are a couple of ioctl's related to tiling and cache placement,
>>> that make no sense for userptr, reject those:
>>> - i915_gem_set_tiling_ioctl()
>>> Tiling should always be linear for userptr. Changing placement will
>>> fail with -ENXIO.
>>> - i915_gem_set_caching_ioctl()
>>> Userptr memory should always be cached. Changing caching mode will
>>> fail with -ENXIO.
>>> - i915_gem_set_domain_ioctl()
>>> Changed to be equivalent to gem_wait, which is correct for the
>>> cached linear userptr pointers. This is required because we
>>> cannot grab a reference to the pages in the rework, but waiting
>>> for idle will do the same.
>>>
>>> This plus the previous changes have been tested against beignet
>>> by using its own unit tests, and intel-video-compute by using
>>> piglit's opencl tests.
>> Did you test against mesa at all?
>
> I tested it and also looked at the code for manual inspection.
>
> Unfortunately rechecking one more time, it seems I missed bo_alloc_internal
> in mesa. Fortunately it seems not to be capable of allocating userptr.
>
> As far as I can tell, that means the changes to mesa are safe.
>
> I tried to run parts of the vulkan cts as well, but it crashed after a
> while against my distro's vulkan package for non userptr related reasons.
Sounds good. I was mostly concerned by the fact that you called out some of
your testing in the commit message and didn't say you tested Mesa. Made me
wonder if you expected us to do that or if you just didn't list it.
--Jason
>
> ~Maarten
>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>> Reviewed-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>>> Cc: Jason Ekstrand <jason at jlekstrand.net>
>>>
>>> -- Still needs an ack from relevant userspace that it won't break, but
>>> should be good.
>>> ---
>>> drivers/gpu/drm/i915/display/intel_display.c | 2 +-
>>> drivers/gpu/drm/i915/gem/i915_gem_domain.c | 12 ++++++++++--
>>> drivers/gpu/drm/i915/gem/i915_gem_object.h | 6 ++++++
>>> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 3 ++-
>>> 4 files changed, 19 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
>>> b/drivers/gpu/drm/i915/display/intel_display.c
>>> index d013b0fab128..3e24db8b9ad6 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>> @@ -14172,7 +14172,7 @@ static int
>>> intel_user_framebuffer_create_handle(struct drm_framebuffer *fb,
>>> struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>>> struct drm_i915_private *i915 = to_i915(obj->base.dev);
>>>
>>> - if (obj->userptr.mm) {
>>> + if (i915_gem_object_is_userptr(obj)) {
>>> drm_dbg(&i915->drm,
>>> "attempting to use a userptr for a framebuffer, denied\n");
>>> return -EINVAL;
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
>>> b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
>>> index 36f54cedaaeb..3078e9a09f70 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
>>> @@ -335,7 +335,13 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev,
>>> void *data,
>>> * not allowed to be changed by userspace.
>>> */
>>> if (i915_gem_object_is_proxy(obj)) {
>>> - ret = -ENXIO;
>>> + /*
>>> + * Silently allow cached for userptr; the vulkan driver
>>> + * sets all objects to cached
>>> + */
>>> + if (!i915_gem_object_is_userptr(obj) ||
>>> + args->caching != I915_CACHING_CACHED)
>> Thanks for looking out for this case. I just double-checked and, yes,
>> we set caching on userptr but we always set it to CACHED so this
>> should take care of us, assuming it does what it looks like it does.
>>
>> Acked-by: Jason Ekstrand <jason at jlekstrand.net>
>>
>>> + ret = -ENXIO;
>>> goto out;
>>> }
>>>
>>> @@ -533,7 +539,9 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void
>>> *data,
>>> * considered to be outside of any cache domain.
>>> */
>>> if (i915_gem_object_is_proxy(obj)) {
>>> - err = -ENXIO;
>>> + /* silently allow userptr to complete */
>>> + if (!i915_gem_object_is_userptr(obj))
>>> + err = -ENXIO;
>>> goto out;
>>> }
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
>>> b/drivers/gpu/drm/i915/gem/i915_gem_object.h
>>> index e9a8ee96d64c..3f300a1d27ba 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
>>> @@ -574,6 +574,12 @@ void __i915_gem_object_flush_frontbuffer(struct
>>> drm_i915_gem_object *obj,
>>> void __i915_gem_object_invalidate_frontbuffer(struct drm_i915_gem_object *obj,
>>> enum fb_op_origin origin);
>>>
>>> +static inline bool
>>> +i915_gem_object_is_userptr(struct drm_i915_gem_object *obj)
>>> +{
>>> + return obj->userptr.mm;
>>> +}
>>> +
>>> static inline void
>>> i915_gem_object_flush_frontbuffer(struct drm_i915_gem_object *obj,
>>> enum fb_op_origin origin)
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>>> b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>>> index 0c30ca52dee3..c89cf911fb29 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>>> @@ -721,7 +721,8 @@ static const struct drm_i915_gem_object_ops
>>> i915_gem_userptr_ops = {
>>> .name = "i915_gem_object_userptr",
>>> .flags = I915_GEM_OBJECT_IS_SHRINKABLE |
>>> I915_GEM_OBJECT_NO_MMAP |
>>> - I915_GEM_OBJECT_ASYNC_CANCEL,
>>> + I915_GEM_OBJECT_ASYNC_CANCEL |
>>> + I915_GEM_OBJECT_IS_PROXY,
>>> .get_pages = i915_gem_userptr_get_pages,
>>> .put_pages = i915_gem_userptr_put_pages,
>>> .dmabuf_export = i915_gem_userptr_dmabuf_export,
>>> --
>>> 2.30.0
Sent with Aqua Mail for Android
https://www.mobisystems.com/aqua-mail
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20210201/85781445/attachment-0001.htm>
More information about the Intel-gfx
mailing list