| Message ID | 1482436502-7965-1-git-send-email-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>-----Original Message----- >From: Ceraolo Spurio, Daniele >Sent: Thursday, December 22, 2016 11:55 AM >To: intel-gfx@lists.freedesktop.org >Cc: Ceraolo Spurio, Daniele <daniele.ceraolospurio@intel.com>; Chris Wilson ><chris@chris-wilson.co.uk>; Wajdeczko, Michal <Michal.Wajdeczko@intel.com>; >Hiler, Arkadiusz <arkadiusz.hiler@intel.com>; Srivatsa, Anusha ><anusha.srivatsa@intel.com>; Winiarski, Michal <michal.winiarski@intel.com> >Subject: [PATCH] drm/i915: request ring to be pinned above GUC_WOPCM_TOP > >From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >GuC will validate the ring offset and fail if it is in the [0, GUC_WOPCM_TOP) >range. > >Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >Cc: Chris Wilson <chris@chris-wilson.co.uk> >Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> >Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> >Cc: Michał Winiarski <michal.winiarski@intel.com> Tested-by: Anusha Srivatsa <anusha.srivatsa@intel.com> >--- > drivers/gpu/drm/i915/intel_ringbuffer.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > >diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c >b/drivers/gpu/drm/i915/intel_ringbuffer.c >index 69ccf4f..8f97b2e 100644 >--- a/drivers/gpu/drm/i915/intel_ringbuffer.c >+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c >@@ -1807,8 +1807,11 @@ static int init_phys_status_page(struct >intel_engine_cs *engine) > > int intel_ring_pin(struct intel_ring *ring) { >- /* Ring wraparound at offset 0 sometimes hangs. No idea why. */ >- unsigned int flags = PIN_GLOBAL | PIN_OFFSET_BIAS | 4096; >+ /* Need a bias for 2 reasons: >+ * 1: ring wraparound at offset 0 sometimes hangs. No idea why. >+ * 2: GuC requires the ring to be placed above GUC_WOPCM_TOP >+ */ >+ unsigned int flags = PIN_GLOBAL | PIN_OFFSET_BIAS | >GUC_WOPCM_TOP; > enum i915_map_type map; > struct i915_vma *vma = ring->vma; > void *addr; >-- >1.9.1
On 22/12/16 14:23, Patchwork wrote: > == Series Details == > > Series: drm/i915: request ring to be pinned above GUC_WOPCM_TOP > URL : https://patchwork.freedesktop.org/series/17147/ > State : failure > > == Summary == > > Series 17147v1 drm/i915: request ring to be pinned above GUC_WOPCM_TOP > https://patchwork.freedesktop.org/api/1.0/series/17147/revisions/1/mbox/ > > Test gem_busy: > Subgroup basic-busy-default: > pass -> FAIL (fi-hsw-4770) > pass -> FAIL (fi-hsw-4770r) > pass -> FAIL (fi-byt-j1900) > Subgroup basic-hang-default: > pass -> FAIL (fi-hsw-4770) > pass -> FAIL (fi-hsw-4770r) > Test gem_wait: > Subgroup basic-busy-all: > pass -> FAIL (fi-ivb-3770) > pass -> FAIL (fi-hsw-4770) > pass -> FAIL (fi-ivb-3520m) > Subgroup basic-wait-all: > pass -> FAIL (fi-ivb-3770) > pass -> FAIL (fi-hsw-4770) > pass -> FAIL (fi-byt-j1900) Clearly moving the ring for legacy submission as well was a bad idea, although by looking at the logs I'm not clear as of why we're getting these hangs and I don't have any pre-Gen8 device to try and reproduce locally. I'll wait until tomorrow to see if there are any comments on the patch and then I'll submit v2 with the change in offset gated by HAS_GUC_SCHED. Daniele > > fi-bdw-5557u total:246 pass:232 dwarn:0 dfail:0 fail:0 skip:14 > fi-bsw-n3050 total:246 pass:207 dwarn:0 dfail:0 fail:0 skip:39 > fi-bxt-j4205 total:246 pass:224 dwarn:0 dfail:0 fail:0 skip:22 > fi-bxt-t5700 total:82 pass:69 dwarn:0 dfail:0 fail:0 skip:12 > fi-byt-j1900 total:246 pass:217 dwarn:0 dfail:0 fail:2 skip:27 > fi-byt-n2820 total:246 pass:215 dwarn:0 dfail:0 fail:0 skip:31 > fi-hsw-4770 total:246 pass:223 dwarn:0 dfail:0 fail:4 skip:19 > fi-hsw-4770r total:246 pass:225 dwarn:0 dfail:0 fail:2 skip:19 > fi-ivb-3520m total:246 pass:224 dwarn:0 dfail:0 fail:1 skip:21 > fi-ivb-3770 total:246 pass:223 dwarn:0 dfail:0 fail:2 skip:21 > fi-kbl-7500u total:246 pass:225 dwarn:0 dfail:0 fail:0 skip:21 > fi-skl-6260u total:246 pass:233 dwarn:0 dfail:0 fail:0 skip:13 > fi-skl-6700hq total:246 pass:226 dwarn:0 dfail:0 fail:0 skip:20 > fi-skl-6700k total:246 pass:222 dwarn:3 dfail:0 fail:0 skip:21 > fi-skl-6770hq total:246 pass:233 dwarn:0 dfail:0 fail:0 skip:13 > fi-snb-2600 total:246 pass:214 dwarn:0 dfail:0 fail:0 skip:32 > > 7165fdc7f0b0b536557fcd0a222d083a901be57c drm-tip: 2016y-12m-22d-19h-42m-25s UTC integration manifest > 9d0d15f drm/i915: request ring to be pinned above GUC_WOPCM_TOP > > == Logs == > > For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3380/ >
On Thu, Dec 22, 2016 at 03:15:03PM -0800, Daniele Ceraolo Spurio wrote: > > > On 22/12/16 14:23, Patchwork wrote: > >== Series Details == > > > >Series: drm/i915: request ring to be pinned above GUC_WOPCM_TOP > >URL : https://patchwork.freedesktop.org/series/17147/ > >State : failure > > > >== Summary == > > > >Series 17147v1 drm/i915: request ring to be pinned above GUC_WOPCM_TOP > >https://patchwork.freedesktop.org/api/1.0/series/17147/revisions/1/mbox/ > > > >Test gem_busy: > > Subgroup basic-busy-default: > > pass -> FAIL (fi-hsw-4770) > > pass -> FAIL (fi-hsw-4770r) > > pass -> FAIL (fi-byt-j1900) > > Subgroup basic-hang-default: > > pass -> FAIL (fi-hsw-4770) > > pass -> FAIL (fi-hsw-4770r) > >Test gem_wait: > > Subgroup basic-busy-all: > > pass -> FAIL (fi-ivb-3770) > > pass -> FAIL (fi-hsw-4770) > > pass -> FAIL (fi-ivb-3520m) > > Subgroup basic-wait-all: > > pass -> FAIL (fi-ivb-3770) > > pass -> FAIL (fi-hsw-4770) > > pass -> FAIL (fi-byt-j1900) > > Clearly moving the ring for legacy submission as well was a bad > idea, although by looking at the logs I'm not clear as of why we're > getting these hangs and I don't have any pre-Gen8 device to try and > reproduce locally. I'll wait until tomorrow to see if there are any > comments on the patch and then I'll submit v2 with the change in > offset gated by HAS_GUC_SCHED. Well they are all gen7 which had the issue of MI_STORE_DWORD_IMM stopping after a ring wrap to address 0, i.e. why there's a 4096 bias in there. Now, it is likely that you've made a hole large enough for something else to creep into address 0 which is equally upsetting to the GPU. No rational explanation, just a theory. But for the sake of not fragmenting our aperture space needlessly (and these will be allocated from bottom-up in the future) we should compute the bias per-gen. Compute a value in the context, alongside ring->size, and pass it in would be my approach. kernel_context would then have a conservative bias due to enable_execlists being unresolved atm, but later contexts would not be as restricted (for !guc). It should then be possible to apply that bias to both contexts/rings equally, or at least consistently. -Chris
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 69ccf4f..8f97b2e 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1807,8 +1807,11 @@ static int init_phys_status_page(struct intel_engine_cs *engine) int intel_ring_pin(struct intel_ring *ring) { - /* Ring wraparound at offset 0 sometimes hangs. No idea why. */ - unsigned int flags = PIN_GLOBAL | PIN_OFFSET_BIAS | 4096; + /* Need a bias for 2 reasons: + * 1: ring wraparound at offset 0 sometimes hangs. No idea why. + * 2: GuC requires the ring to be placed above GUC_WOPCM_TOP + */ + unsigned int flags = PIN_GLOBAL | PIN_OFFSET_BIAS | GUC_WOPCM_TOP; enum i915_map_type map; struct i915_vma *vma = ring->vma; void *addr;