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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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