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

[Kenneth Graunke <kenneth@whitecape.org>]

[-- 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