drm/i915: request ring to be pinned above GUC_WOPCM_TOP
diff mbox

Message ID 1482436502-7965-1-git-send-email-daniele.ceraolospurio@intel.com
State New
Headers show

Commit Message

Daniele Ceraolo Spurio Dec. 22, 2016, 7:55 p.m. UTC
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>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Anusha Srivatsa Dec. 22, 2016, 8:58 p.m. UTC | #1
>-----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
Daniele Ceraolo Spurio Dec. 22, 2016, 11:15 p.m. UTC | #2
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/
>
Chris Wilson Dec. 23, 2016, 7:44 a.m. UTC | #3
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

Patch
diff mbox

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;