From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kenneth Graunke Subject: Re: [PATCH v2] drm/i915: Drop support for I915_EXEC_CONSTANTS_* execbuf parameters. Date: Wed, 15 Feb 2017 01:33:06 -0800 Message-ID: <3497501.vkApMHG6fz@eiger> References: <20170215041751.11441-1-kenneth@whitecape.org> <20170215081250.GR18048@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0422441325==" Return-path: Received: from smtp65.ord1c.emailsrvr.com (smtp65.ord1c.emailsrvr.com [108.166.43.65]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2682E6E1FE for ; Wed, 15 Feb 2017 09:33:12 +0000 (UTC) In-Reply-To: <20170215081250.GR18048@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============0422441325== Content-Type: multipart/signed; boundary="nextPart6237101.4ovtmMvyfI"; micalg="pgp-sha256"; protocol="application/pgp-signature" --nextPart6237101.4ovtmMvyfI Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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 > > Cc: Chris Wilson > > Cc: stable@vger.kernel.org > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92448 > > Signed-off-by: Kenneth Graunke > > --- > > 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 Thanks, I missed that! I'll change this and send a v3. --nextPart6237101.4ovtmMvyfI Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEE6OtbNAgc4e6ibv4ZW1vaBx1JzDgFAlikIFIACgkQW1vaBx1J zDhVgA//XOPMFVTBbamK46CwVUMTTe8v0msTwqSzhK7QxXBziLbvh6VvGqOPG5py iEUuWIZAUBJqVP74+QGBYPVnjuJhf2F+Mj8MxA4izgRHAQtd2h75co0xb6/M0/3u r2K6qLYBAgCqqOhInaellXbXNkj15UnvBgRuDcEB2trKxG4L750hKsh1gJp+zZnM JH0btVVrhaButKTUsHACgUWPJzgZh00MplE7V47U192QbULvvf8jnM6FdsslouNE hQqhfrCaM+y40Hh54thsSwy3GOsqW7twtJG+Y1YM0nKhvdLYTLvmwxMcKxM2ccGl Ztab1mtCFNNi3qBFfIF4xrZPCzSK3blIUWJPvmPSwat7Ked8cPau9LGlUOYzeriT cxuTp75HR2LbTqPb6KUYv1dtDT/ZNGnMj8ZhSeV1X1iHadO5mFQbL3vVViQIjfp4 InLwr/9RbBo/T+PgzhCF/60ZF/DR8UxFieNwaoceK4BggP2pbHA7cq/h2aTxBKRF s2pV1kl6pEQZYt7/AWFWid1Ssr7FYseel5UIab41C94dQlP1jopAuNMhDpW+Jd9J bbo6Zptj6KfEpMVKSKZIgEXXrig0do3WblJxtj/xW1TUCMs1vS+NBm07GCPZ7xMi FXa2mZB9HO504yy4xh7kblDEPLmROklTbDaZ+7RQwCYYoKUILOE= =rj9H -----END PGP SIGNATURE----- --nextPart6237101.4ovtmMvyfI-- --===============0422441325== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== --===============0422441325==--