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

[Ruhl, Michael J]

>-----Original Message-----
>From: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>Sent: Monday, October 12, 2020 10:14 AM
>To: Ruhl, Michael J <michael.j.ruhl at intel.com>; intel-
>gfx at 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 at lists.freedesktop.org> On Behalf Of
>>> Maarten Lankhorst
>>> Sent: Friday, October 2, 2020 8:59 AM
>>> To: intel-gfx at 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 at 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