* [PATCH] drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters.
@ 2017-02-15 9:34 Kenneth Graunke
2017-02-15 10:40 ` Chris Wilson
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Kenneth Graunke @ 2017-02-15 9:34 UTC (permalink / raw)
To: intel-gfx; +Cc: Kenneth Graunke, stable
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. According
to Chris, the intention was to use this in cairo-drm, but "the use was
broken for gen6", so I don't think it ever happened.
'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.
v3: Return -ENODEV for the getparam, as this is what we do for other
obsolete features. Suggested by Chris Wilson.
Cc: stable@vger.kernel.org
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92448
Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> [v2]
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> [v2]
---
drivers/gpu/drm/i915/i915_drv.c | 4 +--
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(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 309c29c84c54..448768cef1f6 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -249,6 +249,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
case I915_PARAM_IRQ_ACTIVE:
case I915_PARAM_ALLOW_BATCHBUFFER:
case I915_PARAM_LAST_DISPATCH:
+ case I915_PARAM_HAS_EXEC_CONSTANTS:
/* Reject all old ums/dri params. */
return -ENODEV;
case I915_PARAM_CHIPSET_ID:
@@ -275,9 +276,6 @@ static int i915_getparam(struct drm_device *dev, void *data,
case I915_PARAM_HAS_BSD2:
value = !!dev_priv->engine[VCS2];
break;
- case I915_PARAM_HAS_EXEC_CONSTANTS:
- value = INTEL_GEN(dev_priv) >= 4;
- break;
case I915_PARAM_HAS_LLC:
value = HAS_LLC(dev_priv);
break;
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters.
2017-02-15 9:34 [PATCH] drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters Kenneth Graunke
@ 2017-02-15 10:40 ` Chris Wilson
2017-02-15 12:59 ` Joonas Lahtinen
2017-02-15 12:22 ` ✓ Fi.CI.BAT: success for drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters. (rev3) Patchwork
2017-02-24 17:53 ` [PATCH] drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters Chris Wilson
2 siblings, 1 reply; 7+ messages in thread
From: Chris Wilson @ 2017-02-15 10:40 UTC (permalink / raw)
To: Kenneth Graunke; +Cc: intel-gfx, joonas.lahtinen, stable, daniel.vetter
On Wed, Feb 15, 2017 at 01:34:46AM -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. According
> to Chris, the intention was to use this in cairo-drm, but "the use was
> broken for gen6", so I don't think it ever happened.
>
> '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.
>
> v3: Return -ENODEV for the getparam, as this is what we do for other
> obsolete features. Suggested by Chris Wilson.
>
> Cc: stable@vger.kernel.org
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92448
> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> [v2]
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> [v2]
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Just give Daniel a chance to ack the ABI revert if he cares...
-Chris
> ---
> drivers/gpu/drm/i915/i915_drv.c | 4 +--
> 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(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 309c29c84c54..448768cef1f6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -249,6 +249,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
> case I915_PARAM_IRQ_ACTIVE:
> case I915_PARAM_ALLOW_BATCHBUFFER:
> case I915_PARAM_LAST_DISPATCH:
> + case I915_PARAM_HAS_EXEC_CONSTANTS:
> /* Reject all old ums/dri params. */
> return -ENODEV;
> case I915_PARAM_CHIPSET_ID:
> @@ -275,9 +276,6 @@ static int i915_getparam(struct drm_device *dev, void *data,
> case I915_PARAM_HAS_BSD2:
> value = !!dev_priv->engine[VCS2];
> break;
> - case I915_PARAM_HAS_EXEC_CONSTANTS:
> - value = INTEL_GEN(dev_priv) >= 4;
> - break;
> case I915_PARAM_HAS_LLC:
> value = HAS_LLC(dev_priv);
> break;
> 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
>
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters.
2017-02-15 10:40 ` Chris Wilson
@ 2017-02-15 12:59 ` Joonas Lahtinen
0 siblings, 0 replies; 7+ messages in thread
From: Joonas Lahtinen @ 2017-02-15 12:59 UTC (permalink / raw)
To: Chris Wilson, Kenneth Graunke; +Cc: daniel.vetter, intel-gfx, stable
On ke, 2017-02-15 at 10:40 +0000, Chris Wilson wrote:
> On Wed, Feb 15, 2017 at 01:34:46AM -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. According
> > to Chris, the intention was to use this in cairo-drm, but "the use was
> > broken for gen6", so I don't think it ever happened.
> >
> > '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.
> >
> > v3: Return -ENODEV for the getparam, as this is what we do for other
> > obsolete features. Suggested by Chris Wilson.
> >
> > Cc: stable@vger.kernel.org
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92448
> > Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
> > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> [v2]
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> [v2]
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
<SNIP>
> Just give Daniel a chance to ack the ABI revert if he cares...
I'm just thinking this might break an application which does the
feature detection and only understands 0 or 1.
Either way, with A-b from Daniel, this too is;
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters. (rev3)
2017-02-15 9:34 [PATCH] drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters Kenneth Graunke
2017-02-15 10:40 ` Chris Wilson
@ 2017-02-15 12:22 ` Patchwork
2017-02-24 17:53 ` [PATCH] drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters Chris Wilson
2 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2017-02-15 12:22 UTC (permalink / raw)
To: Kenneth Graunke; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters. (rev3)
URL : https://patchwork.freedesktop.org/series/19676/
State : success
== Summary ==
Series 19676v3 drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters.
https://patchwork.freedesktop.org/api/1.0/series/19676/revisions/3/mbox/
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:252 pass:221 dwarn:0 dfail:0 fail:0 skip:31
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
78640dff39ba539190c519d5b210c9e28fb886bb drm-tip: 2017y-02m-15d-10h-08m-38s UTC integration manifest
89a57c1 drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters.
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3819/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters.
2017-02-15 9:34 [PATCH] drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters Kenneth Graunke
2017-02-15 10:40 ` Chris Wilson
2017-02-15 12:22 ` ✓ Fi.CI.BAT: success for drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters. (rev3) Patchwork
@ 2017-02-24 17:53 ` Chris Wilson
2 siblings, 0 replies; 7+ messages in thread
From: Chris Wilson @ 2017-02-24 17:53 UTC (permalink / raw)
To: Kenneth Graunke; +Cc: intel-gfx, joonas.lahtinen, stable
On Wed, Feb 15, 2017 at 01:34:46AM -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. According
> to Chris, the intention was to use this in cairo-drm, but "the use was
> broken for gen6", so I don't think it ever happened.
>
> '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.
>
> v3: Return -ENODEV for the getparam, as this is what we do for other
> obsolete features. Suggested by Chris Wilson.
>
> Cc: stable@vger.kernel.org
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92448
> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> [v2]
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> [v2]
Acked by Daniel (irc), so scrubbed from the history books.
Thanks for the patch,
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 7+ messages in thread
* Fixes that failed to backport to v4.11-rc1
@ 2017-03-13 16:14 Jani Nikula
2017-03-13 17:04 ` [PATCH] drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters Chris Wilson
0 siblings, 1 reply; 7+ messages in thread
From: Jani Nikula @ 2017-03-13 16:14 UTC (permalink / raw)
To: Chris Wilson, Conselvan De Oliveira, Ander, Kenneth Graunke,
Ville Syrjälä
Cc: Daniel Vetter, intel-gfx
I'm already scripting my fixes backports quite a bit, and frankly don't
really manually backport anything that doesn't apply cleanly. I'm
thinking of automating some "failed to backport" reporting to authors,
not unlike the failed stable backport reports.
This is a manual report that the following commits have been marked as
Cc: stable or fixing something in v4.11-rc1, but failed to cherry-pick
to drm-intel-fixes. Please see if they are worth backporting, and please
do so if they are.
Feedback about the idea of this reporting is also appreciated.
BR,
Jani.
1f7b847d72c3 ("drm/i915: Disable engine->irq_tasklet around resets")
370a81fb89cb ("drm/i915: Remove unused function intel_ddi_get_link_dpll()")
262fd485ac6b ("drm/i915: Only enable hotplug interrupts if the display interrupts are enabled")
ef0f411f5147 ("drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters.")
c998e8a0f43a ("drm/i915: Hold rpm during GEM suspend in driver unload/suspend")
a5509abda48e ("drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW")
5be6e3340099 ("drm/i915: Do .init_clock_gating() earlier to avoid it clobbering watermarks")
d9321a03efcd ("drm/i915/glk: Remove MODULE_FIRMWARE() tag from Geminilake's DMC")
e0e8c7cb6eb6 ("drm/i915: Stop using RP_DOWN_EI on Baytrail")
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters.
2017-03-13 16:14 Fixes that failed to backport to v4.11-rc1 Jani Nikula
@ 2017-03-13 17:04 ` Chris Wilson
0 siblings, 0 replies; 7+ messages in thread
From: Chris Wilson @ 2017-03-13 17:04 UTC (permalink / raw)
To: intel-gfx; +Cc: jani.nikula, Kenneth Graunke, stable, Chris Wilson
From: Kenneth Graunke <kenneth@whitecape.org>
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. According
to Chris, the intention was to use this in cairo-drm, but "the use was
broken for gen6", so I don't think it ever happened.
'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.
v3: Return -ENODEV for the getparam, as this is what we do for other
obsolete features. Suggested by Chris Wilson.
Cherry-pick: ef0f411f5147 ("drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters.")
Cc: stable@vger.kernel.org
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92448
Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Link: http://patchwork.freedesktop.org/patch/msgid/20170215093446.21291-1-kenneth@whitecape.org
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.c | 4 +--
drivers/gpu/drm/i915/i915_drv.h | 2 --
drivers/gpu/drm/i915/i915_gem.c | 2 --
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 52 ++----------------------------
4 files changed, 3 insertions(+), 57 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2093d203665d..6cd78bb2064d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -248,6 +248,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
case I915_PARAM_IRQ_ACTIVE:
case I915_PARAM_ALLOW_BATCHBUFFER:
case I915_PARAM_LAST_DISPATCH:
+ case I915_PARAM_HAS_EXEC_CONSTANTS:
/* Reject all old ums/dri params. */
return -ENODEV;
case I915_PARAM_CHIPSET_ID:
@@ -274,9 +275,6 @@ static int i915_getparam(struct drm_device *dev, void *data,
case I915_PARAM_HAS_BSD2:
value = !!dev_priv->engine[VCS2];
break;
- case I915_PARAM_HAS_EXEC_CONSTANTS:
- value = INTEL_GEN(dev_priv) >= 4;
- break;
case I915_PARAM_HAS_LLC:
value = HAS_LLC(dev_priv);
break;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1d20c2d00f42..80be09831a52 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2064,8 +2064,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 1051fdf37d20..67b1fc5a0331 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4694,8 +4694,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 d02cfaefe1c8..30e0675fd7da 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1408,10 +1408,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;
int ret;
ret = i915_gem_execbuffer_move_to_gpu(params->request, vmas);
@@ -1422,56 +1419,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) {
- struct intel_ring *ring = params->request->ring;
-
- ret = intel_ring_begin(params->request, 4);
- if (ret)
- return ret;
-
- intel_ring_emit(ring, MI_NOOP);
- intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
- intel_ring_emit_reg(ring, INSTPM);
- intel_ring_emit(ring, instp_mask << 16 | instp_mode);
- intel_ring_advance(ring);
-
- 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.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters.
@ 2017-02-15 2:33 Kenneth Graunke
0 siblings, 0 replies; 7+ messages in thread
From: Kenneth Graunke @ 2017-02-15 2:33 UTC (permalink / raw)
To: intel-gfx; +Cc: Kenneth Graunke, Dave Gordon, Chris Wilson, stable
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, 4 insertions(+), 52 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..dabdeca55fb3 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1414,8 +1414,7 @@ execbuf_submit(struct i915_execbuffer_params *params,
{
struct drm_i915_private *dev_priv = params->request->i915;
u64 exec_start, exec_len;
- int instp_mode;
- u32 instp_mask, *cs;
+ u32 *cs;
int ret;
ret = i915_gem_execbuffer_move_to_gpu(params->request, vmas);
@@ -1426,54 +1425,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
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-03-13 17:04 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-15 9:34 [PATCH] drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters Kenneth Graunke
2017-02-15 10:40 ` Chris Wilson
2017-02-15 12:59 ` Joonas Lahtinen
2017-02-15 12:22 ` ✓ Fi.CI.BAT: success for drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters. (rev3) Patchwork
2017-02-24 17:53 ` [PATCH] drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters Chris Wilson
-- strict thread matches above, loose matches on Subject: below --
2017-03-13 16:14 Fixes that failed to backport to v4.11-rc1 Jani Nikula
2017-03-13 17:04 ` [PATCH] drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters Chris Wilson
2017-02-15 2:33 Kenneth Graunke
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox