Re: [Intel-gfx] [PATCH 10/61] drm/i915: Disable userptr pread/pwrite support.

Ruhl, Michael J
2020-10-19T17:54:21Z

>-----Original Message-----
>From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>Sent: Monday, October 12, 2020 10:14 AM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel-
>gfx@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [PATCH 10/61] drm/i915: Disable userptr pread/pwrite
>support.
>
>Op 02-10-2020 om 22:14 schreef Ruhl, Michael J:
>>> -----Original Message-----
>>> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
>>> Maarten Lankhorst
>>> Sent: Friday, October 2, 2020 8:59 AM
>>> To: intel-gfx@lists.freedesktop.org
>>> Subject: [Intel-gfx] [PATCH 10/61] drm/i915: Disable userptr pread/pwrite
>>> support.
>>>
>>> Userptr should not need the kernel for a userspace memcpy, userspace
>>> needs to call memcpy directly.
>>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> ---
>>> .../gpu/drm/i915/gem/i915_gem_object_types.h | 2 ++
>>> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 20
>>> +++++++++++++++++++
>>> drivers/gpu/drm/i915/i915_gem.c | 5 +++++
>>> 3 files changed, 27 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>>> b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>>> index 62dde3585b51..dbb6f6171165 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>>> @@ -57,6 +57,8 @@ struct drm_i915_gem_object_ops {
>>>
>>> int (*pwrite)(struct drm_i915_gem_object *obj,
>>> const struct drm_i915_gem_pwrite *arg);
>>> + int (*pread)(struct drm_i915_gem_object *obj,
>>> + const struct drm_i915_gem_pread *arg);
>>>
>>> int (*dmabuf_export)(struct drm_i915_gem_object *obj);
>>> void (*release)(struct drm_i915_gem_object *obj);
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>>> b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>>> index 22008948be58..136a589e5d94 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>>> @@ -700,6 +700,24 @@ i915_gem_userptr_dmabuf_export(struct
>>> drm_i915_gem_object *obj)
>>> return i915_gem_userptr_init__mmu_notifier(obj, 0);
>>> }
>>>
>>> +static int
>>> +i915_gem_userptr_pwrite(struct drm_i915_gem_object *obj,
>>> + const struct drm_i915_gem_pwrite *args)
>>> +{
>>> + drm_dbg(obj->base.dev, "pwrite to userptr no longer allowed\n");
>>> +
>>> + return -EINVAL;
>> I have seen ENOSYS used for unsupported pread/pwrite (see
>radeon_gem.c).
>>
>> I have also seen ENOTSUPP for similar return values.
>>
>> Is EINVAL the correct response?
>
>It seems for some other things we use -ENXIO, I don't think it matters in the
>end.
>
>As long as we fail in some recognisable way, I'm fine with it. :)
>
>I chose -EINVAL as we return the same error with r/o objects.

Hi Maarten,

Sorry this got lost in the noise last week. 😐

If my comments are still relevant... 😊

Ok, I understand your reasoning here, but I do see differences.

EINVAL for r/o means I made bad request. ENOTSUPP means that the feature
I want to use is not available. Not terribly different, but slightly more "correct".

I don't see how ENXIO fits other than as a "more unique" error.

I do think that EINVAL is over used, making it difficult to find out why the error
was returned, but I can see its use in this case, so I am not opposed.

Sorry for the lateness of my reply,

Mike