[Intel-gfx] [PATCH v7 13/63] drm/i915: Reject more ioctls for userptr

[Maarten Lankhorst]

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.

~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
>>