From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kenneth Graunke Subject: Re: [PATCH 4/4] drm/i915: Enable render context support for gen4 (Broadwater to Cantiga) Date: Fri, 19 Apr 2019 09:54:22 -0700 Message-ID: <1757609.LCLJ7C3CGE@kirito> References: <20190419111601.18656-1-chris@chris-wilson.co.uk> <20190419111601.18656-4-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1763179781==" Return-path: Received: from smtp121.iad3b.emailsrvr.com (smtp121.iad3b.emailsrvr.com [146.20.161.121]) by gabe.freedesktop.org (Postfix) with ESMTPS id A994B891E2 for ; Fri, 19 Apr 2019 17:01:45 +0000 (UTC) In-Reply-To: <20190419111601.18656-4-chris@chris-wilson.co.uk> 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 --===============1763179781== Content-Type: multipart/signed; boundary="nextPart18754889.Bo5nT1cl9Q"; micalg="pgp-sha256"; protocol="application/pgp-signature" --nextPart18754889.Bo5nT1cl9Q Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" On Friday, April 19, 2019 4:16:01 AM PDT Chris Wilson wrote: > Broadwater and the rest of gen4 do support being able to saving and > reloading context specific registers between contexts, providing isolation > of the basic GPU state (as programmable by userspace). This allows > userspace to assume that the GPU retains their state from one batch to the > next, minimising the amount of state it needs to reload and manually save > across batches. >=20 > v2: CONSTANT_BUFFER woes >=20 > Running through piglit turned up an interesting issue, a GPU hang inside > the context load. The context image includes the CONSTANT_BUFFER command > that loads an address into a on-gpu buffer, and the context load was > executing that immediately. However, since it was reading from the GTT > there is no guarantee that the GTT retains the same configuration as > when the context was saved, resulting in stray reads and a GPU hang. >=20 > Having tried issuing a CONSTANT_BUFFER (to disable the command) from the > ring before saving the context to no avail, we resort to patching out > the instruction inside the context image before loading. >=20 > This does impose that gen4 always reissues CONSTANT_BUFFER commands on > each batch, but due to the use of a shared GTT that was and will remain > a requirement. >=20 > v3: ECOSPKD to the rescue >=20 > Ville found the magic bit in the ECOSPKD to disable saving and restoring > the CONSTANT_BUFFER from the context image, thereby completely avoiding > the GPU hangs from chasing invalid pointers. This appears to be the > default behaviour for gen5, and so we just need to tweak gen4 to match. >=20 > Signed-off-by: Chris Wilson > Cc: Ville Syrj=E4l=E4 > Cc: Kenneth Graunke > --- > drivers/gpu/drm/i915/i915_reg.h | 3 +++ > drivers/gpu/drm/i915/intel_engine_cs.c | 2 +- > drivers/gpu/drm/i915/intel_ringbuffer.c | 14 ++++++++++++++ > 3 files changed, 18 insertions(+), 1 deletion(-) >=20 > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_= reg.h > index b74824f0b5b1..5815703ac35f 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -2665,6 +2665,9 @@ enum i915_power_well_id { > # define MODE_IDLE (1 << 9) > # define STOP_RING (1 << 8) > =20 > +#define ECOSPKD _MMIO(0x21d0) > +# define CONSTANT_BUFFER_SR_DISABLE BIT(4) > + The name of this register is ECOSKPD (Scratch Pad, or SK PD). The G45 PRM says it's DevBW-C1+. I can't recall if earlier ones shipped or not. If so, we might be in trouble. But I've seen a lot of DevBW-A/B warnings that I'm pretty sure I've safely ignored... With the typo fixed, this patch is: Reviewed-by: Kenneth Graunke > #define GEN6_GT_MODE _MMIO(0x20d0) > #define GEN7_GT_MODE _MMIO(0x7008) > #define GEN6_WIZ_HASHING(hi, lo) (((hi) << 9) | ((lo) << 7)) > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i91= 5/intel_engine_cs.c > index fc8be2fcb4e6..f9db2e0bca12 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -211,6 +211,7 @@ __intel_engine_context_size(struct drm_i915_private *= dev_priv, u8 class) > return round_up(GEN6_CXT_TOTAL_SIZE(cxt_size) * 64, > PAGE_SIZE); > case 5: > + case 4: > /* > * There is a discrepancy here between the size reported > * by the register and the size of the context layout > @@ -227,7 +228,6 @@ __intel_engine_context_size(struct drm_i915_private *= dev_priv, u8 class) > cxt_size * 64, > cxt_size - 1); > return round_up(cxt_size * 64, PAGE_SIZE); > - case 4: > case 3: > case 2: > /* For the special day when i810 gets merged. */ > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i9= 15/intel_ringbuffer.c > index 2d2e33cd3fae..26b276ed00b3 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -832,6 +832,20 @@ static int init_render_ring(struct intel_engine_cs *= engine) > { > struct drm_i915_private *dev_priv =3D engine->i915; > =20 > + /* > + * Disable CONSTANT_BUFFER before it is loaded from the context > + * image. For as it is loaded, it is executed and the stored > + * address may no longer be valid, leading to a GPU hang. > + * > + * This imposes the requirement that userspace reload their > + * CONSTANT_BUFFER on every batch, fortunately a requirement > + * they are already accustomed to from before contexts were > + * enabled. > + */ > + if (IS_GEN(dev_priv, 4)) > + I915_WRITE(ECOSPKD, > + _MASKED_BIT_ENABLE(CONSTANT_BUFFER_SR_DISABLE)); > + > /* WaTimedSingleVertexDispatch:cl,bw,ctg,elk,ilk,snb */ > if (IS_GEN_RANGE(dev_priv, 4, 6)) > I915_WRITE(MI_MODE, _MASKED_BIT_ENABLE(VS_TIMER_DISPATCH)); >=20 --nextPart18754889.Bo5nT1cl9Q Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEE6OtbNAgc4e6ibv4ZW1vaBx1JzDgFAly5/T4ACgkQW1vaBx1J zDiUzBAAk3X3uLVnwVUZVADOFx9Z759+fqcTB1boOacGEmAv3vGgrsdF6LQ7Bosd 4v/IcxEblLNnAmfxKFkSzpyidCfbVSj+08KZOl1eFTD8C356YueJEeIWgNPJQWDI 8ZMUbHpnvIkDOFw2h7B1tEAh4i9+UKH+yldzsb+klymyjFNhbsSIT6BbOPYIqiR2 z2QyNj7nV2ik44J54LAmR8RXUI5QiFIMSClS3GifAG6Z7LgR/o00Z3J+TnEPVp/I hr1CSceQVRkVaWnT0ylVKivXZQ0yOv2Rt6TZgKOkqbC3MhD5CQAIniPHpKkiGSdB yUjoFIO7OEHISMaqRoDlhR18Q+4XEVFElSRW/w2zQzLyE2GU4SouaO/bM4oSlLVD 5xdbp4Z1TD51CYBopWkHtebuFHQ2BbAVGHAjUA1VizhN6I1HK5RzLNVcxKBw1vAj QMTGdoD/q1rNaMqnhwkiVenGCSnqPD+8ALswGh6Evw/m0xifRUOHi9HrWC15LkdY sqZBRX4USDh4XVgUsc+ZSOrTmLajWsFkbeLp0xDyMKQYFHT8IWUweyCW2GNzwcRb QhkcFwoK6bh35RnEKUvngvmBkXyFoxBeseS/Is+muaBsJKAKahe1KKB/kRtTo3Wy /td6i63xYBRUE3Br5PkCEoZmVfxCN3RIdLqZE3yyrU6neF885xk= =0LDc -----END PGP SIGNATURE----- --nextPart18754889.Bo5nT1cl9Q-- --===============1763179781== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4 --===============1763179781==--