[PATCH v2] drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters.

[PATCH v2] drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters.

[Kenneth Graunke]

This patch makes the I915_PARAM_HAS_EXEC_CONSTANTS getparam return 0
(indicating the optional feature is not supported), and makes execbuf
always return -EINVAL if the flags are used.

Apparently, no userspace ever shipped which used this optional feature:
I checked the git history of Mesa, xf86-video-intel, libva, and Beignet,
and there were zero commits showing a use of these flags.  Kernel commit
72bfa19c8deb4 apparently introduced the feature prematurely.

'relative_constants_mode' has always been tracked per-device, but this
has actually been wrong ever since hardware contexts were introduced, as
the INSTPM register is saved (and automatically restored) as part of the
render ring context. The software per-device value could therefore get
out of sync with the hardware per-context value.  This meant that using
them is actually unsafe: a client which tried to use them could damage
the state of other clients, causing the GPU to interpret their BO
offsets as absolute pointers, leading to bogus memory reads.

These flags were also never ported to execlist mode, making them no-ops
on Gen9+ (which requires execlists), and Gen8 in the default mode.

On Gen8+, userspace can write these registers directly, achieving the
same effect.  On Gen6-7.5, it likely makes sense to extend the command
parser to support them.  I don't think anyone wants this on Gen4-5.

Based on a patch by Dave Gordon.

Cc: Dave Gordon <david.s.gordon@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: stable@vger.kernel.org
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92448
Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
---
 drivers/gpu/drm/i915/i915_drv.c            |  2 +-
 drivers/gpu/drm/i915/i915_drv.h            |  2 --
 drivers/gpu/drm/i915/i915_gem.c            |  2 --
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 50 ++----------------------------
 4 files changed, 3 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 309c29c84c54..1a53a4bb09c8 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -276,7 +276,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
 		value = !!dev_priv->engine[VCS2];
 		break;
 	case I915_PARAM_HAS_EXEC_CONSTANTS:
-		value = INTEL_GEN(dev_priv) >= 4;
+		value = 0;
 		break;
 	case I915_PARAM_HAS_LLC:
 		value = HAS_LLC(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 918118a268b8..8a6ba63d3f74 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2077,8 +2077,6 @@ struct drm_i915_private {
 
 	const struct intel_device_info info;
 
-	int relative_constants_mode;
-
 	void __iomem *regs;
 
 	struct intel_uncore uncore;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 71297920fdf4..c9be0285c7cf 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4656,8 +4656,6 @@ i915_gem_load_init(struct drm_i915_private *dev_priv)
 	init_waitqueue_head(&dev_priv->gpu_error.wait_queue);
 	init_waitqueue_head(&dev_priv->gpu_error.reset_queue);
 
-	dev_priv->relative_constants_mode = I915_EXEC_CONSTANTS_REL_GENERAL;
-
 	init_waitqueue_head(&dev_priv->pending_flip_queue);
 
 	dev_priv->mm.interruptible = true;
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index da0846fe2ad6..febb067903e9 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1412,10 +1412,7 @@ execbuf_submit(struct i915_execbuffer_params *params,
 	       struct drm_i915_gem_execbuffer2 *args,
 	       struct list_head *vmas)
 {
-	struct drm_i915_private *dev_priv = params->request->i915;
 	u64 exec_start, exec_len;
-	int instp_mode;
-	u32 instp_mask, *cs;
 	int ret;
 
 	ret = i915_gem_execbuffer_move_to_gpu(params->request, vmas);
@@ -1426,54 +1423,11 @@ execbuf_submit(struct i915_execbuffer_params *params,
 	if (ret)
 		return ret;
 
-	instp_mode = args->flags & I915_EXEC_CONSTANTS_MASK;
-	instp_mask = I915_EXEC_CONSTANTS_MASK;
-	switch (instp_mode) {
-	case I915_EXEC_CONSTANTS_REL_GENERAL:
-	case I915_EXEC_CONSTANTS_ABSOLUTE:
-	case I915_EXEC_CONSTANTS_REL_SURFACE:
-		if (instp_mode != 0 && params->engine->id != RCS) {
-			DRM_DEBUG("non-0 rel constants mode on non-RCS\n");
-			return -EINVAL;
-		}
-
-		if (instp_mode != dev_priv->relative_constants_mode) {
-			if (INTEL_INFO(dev_priv)->gen < 4) {
-				DRM_DEBUG("no rel constants on pre-gen4\n");
-				return -EINVAL;
-			}
-
-			if (INTEL_INFO(dev_priv)->gen > 5 &&
-			    instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
-				DRM_DEBUG("rel surface constants mode invalid on gen5+\n");
-				return -EINVAL;
-			}
-
-			/* The HW changed the meaning on this bit on gen6 */
-			if (INTEL_INFO(dev_priv)->gen >= 6)
-				instp_mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
-		}
-		break;
-	default:
-		DRM_DEBUG("execbuf with unknown constants: %d\n", instp_mode);
+	if (args->flags & I915_EXEC_CONSTANTS_MASK) {
+		DRM_DEBUG("I915_EXEC_CONSTANTS_* unsupported\n");
 		return -EINVAL;
 	}
 
-	if (params->engine->id == RCS &&
-	    instp_mode != dev_priv->relative_constants_mode) {
-		cs = intel_ring_begin(params->request, 4);
-		if (IS_ERR(cs))
-			return PTR_ERR(cs);
-
-		*cs++ = MI_NOOP;
-		*cs++ = MI_LOAD_REGISTER_IMM(1);
-		*cs++ = i915_mmio_reg_offset(INSTPM);
-		*cs++ = instp_mask << 16 | instp_mode;
-		intel_ring_advance(params->request, cs);
-
-		dev_priv->relative_constants_mode = instp_mode;
-	}
-
 	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
 		ret = i915_reset_gen7_sol_offsets(params->request);
 		if (ret)
-- 
2.11.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters. (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters. (rev2)
URL   : https://patchwork.freedesktop.org/series/19676/
State : success

== Summary ==

Series 19676v2 drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters.
https://patchwork.freedesktop.org/api/1.0/series/19676/revisions/2/mbox/

Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> INCOMPLETE (fi-byt-n2820) fdo#99740

fdo#99740 https://bugs.freedesktop.org/show_bug.cgi?id=99740

fi-bdw-5557u     total:252  pass:241  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:252  pass:213  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:252  pass:233  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:252  pass:225  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:215  pass:184  dwarn:0   dfail:0   fail:0   skip:30 
fi-hsw-4770      total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:252  pass:202  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:252  pass:235  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:252  pass:230  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:252  pass:224  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:252  pass:223  dwarn:0   dfail:0   fail:0   skip:29 

e80b772bf85da72c46b582e7c4f7b0d861cb4669 drm-tip: 2017y-02m-14d-22h-44m-17s UTC integration manifest
8ea1846 drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3816/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters.

[Joonas Lahtinen]

On ti, 2017-02-14 at 20:17 -0800, Kenneth Graunke wrote:
> This patch makes the I915_PARAM_HAS_EXEC_CONSTANTS getparam return 0
> (indicating the optional feature is not supported), and makes execbuf
> always return -EINVAL if the flags are used.
> 
> Apparently, no userspace ever shipped which used this optional feature:
> I checked the git history of Mesa, xf86-video-intel, libva, and Beignet,
> and there were zero commits showing a use of these flags.  Kernel commit
> 72bfa19c8deb4 apparently introduced the feature prematurely.
> 
> 'relative_constants_mode' has always been tracked per-device, but this
> has actually been wrong ever since hardware contexts were introduced, as
> the INSTPM register is saved (and automatically restored) as part of the
> render ring context. The software per-device value could therefore get
> out of sync with the hardware per-context value.  This meant that using
> them is actually unsafe: a client which tried to use them could damage
> the state of other clients, causing the GPU to interpret their BO
> offsets as absolute pointers, leading to bogus memory reads.
> 
> These flags were also never ported to execlist mode, making them no-ops
> on Gen9+ (which requires execlists), and Gen8 in the default mode.
> 
> On Gen8+, userspace can write these registers directly, achieving the
> same effect.  On Gen6-7.5, it likely makes sense to extend the command
> parser to support them.  I don't think anyone wants this on Gen4-5.
> 
> Based on a patch by Dave Gordon.
> 
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@vger.kernel.org
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92448
> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>

Explanation makes sense, only Google hits seem to be on Mesa detecting
the feature and later, removing the detection when unused.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters.

[Chris Wilson]

On Tue, Feb 14, 2017 at 08:17:51PM -0800, Kenneth Graunke wrote:
> This patch makes the I915_PARAM_HAS_EXEC_CONSTANTS getparam return 0
> (indicating the optional feature is not supported), and makes execbuf
> always return -EINVAL if the flags are used.
> 
> Apparently, no userspace ever shipped which used this optional feature:
> I checked the git history of Mesa, xf86-video-intel, libva, and Beignet,
> and there were zero commits showing a use of these flags.  Kernel commit
> 72bfa19c8deb4 apparently introduced the feature prematurely.

It was actually cairo where we had patches to use it first. And we then
realised the use was broken for gen6.
 
> 'relative_constants_mode' has always been tracked per-device, but this
> has actually been wrong ever since hardware contexts were introduced, as
> the INSTPM register is saved (and automatically restored) as part of the
> render ring context. The software per-device value could therefore get
> out of sync with the hardware per-context value.  This meant that using
> them is actually unsafe: a client which tried to use them could damage
> the state of other clients, causing the GPU to interpret their BO
> offsets as absolute pointers, leading to bogus memory reads.
> 
> These flags were also never ported to execlist mode, making them no-ops
> on Gen9+ (which requires execlists), and Gen8 in the default mode.
> 
> On Gen8+, userspace can write these registers directly, achieving the
> same effect.  On Gen6-7.5, it likely makes sense to extend the command
> parser to support them.  I don't think anyone wants this on Gen4-5.
> 
> Based on a patch by Dave Gordon.
> 
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@vger.kernel.org
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92448
> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.c            |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h            |  2 --
>  drivers/gpu/drm/i915/i915_gem.c            |  2 --
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 50 ++----------------------------
>  4 files changed, 3 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 309c29c84c54..1a53a4bb09c8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -276,7 +276,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
>  		value = !!dev_priv->engine[VCS2];
>  		break;
>  	case I915_PARAM_HAS_EXEC_CONSTANTS:
> -		value = INTEL_GEN(dev_priv) >= 4;
> +		value = 0;

The style for obsolete param is to return -ENODEV. If that makes sense
we can move this to the start of this block with the other dead params.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2] drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters.

[Kenneth Graunke]

[-- Attachment #1.1: Type: text/plain, Size: 3173 bytes --]

On Wednesday, February 15, 2017 12:12:50 AM PST Chris Wilson wrote:
> On Tue, Feb 14, 2017 at 08:17:51PM -0800, Kenneth Graunke wrote:
> > This patch makes the I915_PARAM_HAS_EXEC_CONSTANTS getparam return 0
> > (indicating the optional feature is not supported), and makes execbuf
> > always return -EINVAL if the flags are used.
> > 
> > Apparently, no userspace ever shipped which used this optional feature:
> > I checked the git history of Mesa, xf86-video-intel, libva, and Beignet,
> > and there were zero commits showing a use of these flags.  Kernel commit
> > 72bfa19c8deb4 apparently introduced the feature prematurely.
> 
> It was actually cairo where we had patches to use it first. And we then
> realised the use was broken for gen6.

Ahhh, that makes sense.  I knew I was forgetting about some userspace.
Thanks for reminding me about cairo-drm.

> > 'relative_constants_mode' has always been tracked per-device, but this
> > has actually been wrong ever since hardware contexts were introduced, as
> > the INSTPM register is saved (and automatically restored) as part of the
> > render ring context. The software per-device value could therefore get
> > out of sync with the hardware per-context value.  This meant that using
> > them is actually unsafe: a client which tried to use them could damage
> > the state of other clients, causing the GPU to interpret their BO
> > offsets as absolute pointers, leading to bogus memory reads.
> > 
> > These flags were also never ported to execlist mode, making them no-ops
> > on Gen9+ (which requires execlists), and Gen8 in the default mode.
> > 
> > On Gen8+, userspace can write these registers directly, achieving the
> > same effect.  On Gen6-7.5, it likely makes sense to extend the command
> > parser to support them.  I don't think anyone wants this on Gen4-5.
> > 
> > Based on a patch by Dave Gordon.
> > 
> > Cc: Dave Gordon <david.s.gordon@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: stable@vger.kernel.org
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92448
> > Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c            |  2 +-
> >  drivers/gpu/drm/i915/i915_drv.h            |  2 --
> >  drivers/gpu/drm/i915/i915_gem.c            |  2 --
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 50 ++----------------------------
> >  4 files changed, 3 insertions(+), 53 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 309c29c84c54..1a53a4bb09c8 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -276,7 +276,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
> >  		value = !!dev_priv->engine[VCS2];
> >  		break;
> >  	case I915_PARAM_HAS_EXEC_CONSTANTS:
> > -		value = INTEL_GEN(dev_priv) >= 4;
> > +		value = 0;
> 
> The style for obsolete param is to return -ENODEV. If that makes sense
> we can move this to the start of this block with the other dead params.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris

Thanks, I missed that!  I'll change this and send a v3.

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx