diff mbox series

[1/1] drm/i915: Disable pread/pwrite ioctl's for future platforms (v2)

Message ID 20210311202017.1133327-2-jason@jlekstrand.net (mailing list archive)
State New, archived
Headers show
Series drm/i915: Disable pread/pwrite ioctl's for future platforms (v2) | expand

Commit Message

Jason Ekstrand March 11, 2021, 8:20 p.m. UTC 
From: Ashutosh Dixit <ashutosh.dixit@intel.com>

The rationale for this change is roughly as follows:

 1. The functionality can be done entirely in userspace with a
    combination of mmap + memcpy

 2. The only reason anyone in userspace is still using it is because
    someone implemented bo_subdata that way in libdrm ages ago and
    they're all too lazy to write the 5 lines of code to do a map.

 3. This falls cleanly into the category of things which will only get
    more painful with local memory support.

These ioctls aren't used much anymore by "real" userspace drivers.
Vulkan has never used them and neither has the iris GL driver.  The old
i965 GL driver does use PWRITE for glBufferSubData but it only supports
up through Gen11; Gen12 was never enabled in i965.  The compute driver
has never used PREAD/PWRITE.  The only remaining user is the media
driver which uses it exactly twice and they're easily removed [1] so
expecting them to drop it going forward is reasonable.

IGT changes which handle this kernel change have also been submitted [2].

[1] https://github.com/intel/media-driver/pull/1160
[2] https://patchwork.freedesktop.org/series/81384/

v2 (Jason Ekstrand):
 - Improved commit message with the status of all usermode drivers
 - A more future-proof platform check

Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_gem.c | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+)

Comments

Dixit, Ashutosh March 12, 2021, 3:28 a.m. UTC | #1 
On Thu, 11 Mar 2021 12:20:17 -0800, Jason Ekstrand wrote:
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index b2e3b5cfccb4a..78ad5a9dd4784 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -374,10 +374,19 @@ int
>  i915_gem_pread_ioctl(struct drm_device *dev, void *data,
>		     struct drm_file *file)
>  {
> +	struct drm_i915_private *i915 = to_i915(dev);
>	struct drm_i915_gem_pread *args = data;
>	struct drm_i915_gem_object *obj;
>	int ret;
>
> +	/* Pread is disallowed for all platforms after TGL-LP */
> +	if (INTEL_GEN(i915) >= 12 && !IS_TIGERLAKE(i915))
> +		return -EOPNOTSUPP;
> +
> +	/* All discrete memory platforms are Gen12 or above */
> +	if (WARN_ON(HAS_LMEM(i915)))
> +		return -EOPNOTSUPP;

Not sure but you are probably trying to make it explicit that pread/pwrite
are truly gone on dGfx? Because real dGfx are Gen12+ the code will return
from the first if statement and never get to the second if statement. And
there's talk on the relocation thread about tripping fake LMEM here for
platforms prior to Gen12.

So I'd suggest get rid of this second if statement and only retain the
first (for both pread and pwrite) since that seems to be entirely
sufficient.
Maarten Lankhorst March 12, 2021, 11:37 a.m. UTC | #2 
Op 2021-03-12 om 04:28 schreef Dixit, Ashutosh:
> On Thu, 11 Mar 2021 12:20:17 -0800, Jason Ekstrand wrote:
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index b2e3b5cfccb4a..78ad5a9dd4784 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -374,10 +374,19 @@ int
>>  i915_gem_pread_ioctl(struct drm_device *dev, void *data,
>> 		     struct drm_file *file)
>>  {
>> +	struct drm_i915_private *i915 = to_i915(dev);
>> 	struct drm_i915_gem_pread *args = data;
>> 	struct drm_i915_gem_object *obj;
>> 	int ret;
>>
>> +	/* Pread is disallowed for all platforms after TGL-LP */
>> +	if (INTEL_GEN(i915) >= 12 && !IS_TIGERLAKE(i915))
>> +		return -EOPNOTSUPP;
>> +
>> +	/* All discrete memory platforms are Gen12 or above */
>> +	if (WARN_ON(HAS_LMEM(i915)))
>> +		return -EOPNOTSUPP;
> Not sure but you are probably trying to make it explicit that pread/pwrite
> are truly gone on dGfx? Because real dGfx are Gen12+ the code will return
> from the first if statement and never get to the second if statement. And
> there's talk on the relocation thread about tripping fake LMEM here for
> platforms prior to Gen12.
>
> So I'd suggest get rid of this second if statement and only retain the
> first (for both pread and pwrite) since that seems to be entirely
> sufficient.
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

I think this should probably be a -ENODEV return code, otherwise patch looks good to me.

We probably don't want to break fake lmem until it's removed..

Cc drm maintainers on next version?
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1d45d7492d10d..3f74188fa8090 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1633,6 +1633,7 @@  tgl_stepping_get(struct drm_i915_private *dev_priv)
 #define HAS_SECURE_BATCHES(dev_priv) (INTEL_GEN(dev_priv) < 6)
 #define HAS_WT(dev_priv)	HAS_EDRAM(dev_priv)
 
+#define HAS_PREAD_PWRITE(dev_priv)	(!INTEL_INFO(dev_priv)->no_pread_pwrite)
 #define HWS_NEEDS_PHYSICAL(dev_priv)	(INTEL_INFO(dev_priv)->hws_needs_physical)
 
 #define HAS_LOGICAL_RING_CONTEXTS(dev_priv) \
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b2e3b5cfccb4a..78ad5a9dd4784 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -374,10 +374,19 @@  int
 i915_gem_pread_ioctl(struct drm_device *dev, void *data,
 		     struct drm_file *file)
 {
+	struct drm_i915_private *i915 = to_i915(dev);
 	struct drm_i915_gem_pread *args = data;
 	struct drm_i915_gem_object *obj;
 	int ret;
 
+	/* Pread is disallowed for all platforms after TGL-LP */
+	if (INTEL_GEN(i915) >= 12 && !IS_TIGERLAKE(i915))
+		return -EOPNOTSUPP;
+
+	/* All discrete memory platforms are Gen12 or above */
+	if (WARN_ON(HAS_LMEM(i915)))
+		return -EOPNOTSUPP;
+
 	if (args->size == 0)
 		return 0;
 
@@ -675,10 +684,19 @@  int
 i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
 		      struct drm_file *file)
 {
+	struct drm_i915_private *i915 = to_i915(dev);
 	struct drm_i915_gem_pwrite *args = data;
 	struct drm_i915_gem_object *obj;
 	int ret;
 
+	/* Pwrite is disallowed for all platforms after TGL-LP */
+	if (INTEL_GEN(i915) >= 12 && !IS_TIGERLAKE(i915))
+		return -EOPNOTSUPP;
+
+	/* All discrete memory platforms are Gen12 or above */
+	if (WARN_ON(HAS_LMEM(i915)))
+		return -EOPNOTSUPP;
+
 	if (args->size == 0)
 		return 0;