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@xxxxxxxxxxxxxxx > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92448 > Signed-off-by: Kenneth Graunke <kenneth@xxxxxxxxxxxxx> > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> [v2] > Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> [v2] Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
