[PATCH] drm/i915/execlists: WaDisableCtxRestoreArbitration is only needed in gen8

[PATCH] drm/i915/execlists: WaDisableCtxRestoreArbitration is only needed in gen8

[Michel Thierry]

WaDisableCtxRestoreArbitration was only applied for bdw and chv, but
this changed after the code got moved to gen8_emit_bb_start (and, at
least in my tree, there is no gen9_emit_bb_start).

Fixes: 3ad7b52d962e ("drm/i915/execlists: Move bdw GPGPU w/a to emit_bb")
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index c5b76082d695..d2b7eac0777c 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1619,7 +1619,10 @@ static int gen8_emit_bb_start(struct drm_i915_gem_request *req,
 		return PTR_ERR(cs);
 
 	/* WaDisableCtxRestoreArbitration:bdw,chv */
-	*cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
+	if (IS_GEN8(req->i915))
+		*cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
+	else
+		*cs++ = MI_NOOP;
 
 	/* FIXME(BDW): Address space and security selectors. */
 	*cs++ = MI_BATCH_BUFFER_START_GEN8 |
-- 
2.14.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/execlists: WaDisableCtxRestoreArbitration is only needed in gen8

[Chris Wilson]

Quoting Michel Thierry (2017-10-05 19:19:27)
> WaDisableCtxRestoreArbitration was only applied for bdw and chv, but
> this changed after the code got moved to gen8_emit_bb_start (and, at
> least in my tree, there is no gen9_emit_bb_start).

But I need to make sure we enable arbitration at some point. :)
It may not need the w/a name, but I was making very sure we didn't start
a BB without ARB, or context-switch into the middle of a batch without
ARB. That was my thinking, that there was no harm in overkill if we set
it more often than strictly necessary.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/execlists: WaDisableCtxRestoreArbitration is only needed in gen8

[Michel Thierry]

On 10/5/2017 11:28 AM, Chris Wilson wrote:
> Quoting Michel Thierry (2017-10-05 19:19:27)
>> WaDisableCtxRestoreArbitration was only applied for bdw and chv, but
>> this changed after the code got moved to gen8_emit_bb_start (and, at
>> least in my tree, there is no gen9_emit_bb_start).
> 
> But I need to make sure we enable arbitration at some point. :)
> It may not need the w/a name, but I was making very sure we didn't start
> a BB without ARB, or context-switch into the middle of a batch without
> ARB. That was my thinking, that there was no harm in overkill if we set
> it more often than strictly necessary.

Ok, thanks for the detailed explanation and yes, probably no harm in 
setting MI_ARB_ON.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915/execlists: Add a comment for the extra MI_ARB_ENABLE

[Chris Wilson]

Michel Thierry noticed that we were applying WaDisableCtxRestoreArbitration
even to gen9, which does not require the w/a. The rationale is that we
need to enable MI arbitration for execlists to work, and to be safe we
do that before every batch (in addition to every context switch into the
batch). Since this is not clear from the single line comment suggesting
the MI_ARB_ENABLE is solely for the w/a, add a little more detail.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 476881f63505..fa66e4f58642 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1608,7 +1608,23 @@ static int gen8_emit_bb_start(struct drm_i915_gem_request *req,
 	if (IS_ERR(cs))
 		return PTR_ERR(cs);
 
-	/* WaDisableCtxRestoreArbitration:bdw,chv */
+	/*
+	 * WaDisableCtxRestoreArbitration:bdw,chv
+	 *
+	 * We don't need to perform MI_ARB_ENABLE as often as we do (in
+	 * particular all the gen that do not need the w/a at all!), if we
+	 * took care to make sure that on every switch into this context
+	 * (both ordinary and for preemption) that arbitrartion was enabled
+	 * we would be fine. However, there doesn't seem to be a downside to
+	 * being paranoid and making sure it is set before each batch and
+	 * every context-switch.
+	 *
+	 * Note that if we fail to enable arbitration before the request
+	 * is complete, then we do not see the context-switch interrupt and
+	 * the engine hangs (with RING_HEAD == RING_TAIL).
+	 *
+	 * That satisfies both the GPGPU w/a and our heavy-handed paranoia.
+	 */
 	*cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
 
 	/* FIXME(BDW): Address space and security selectors. */
-- 
2.14.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/execlists: Add a comment for the extra MI_ARB_ENABLE

[Michel Thierry]

On 10/5/2017 12:10 PM, Chris Wilson wrote:
> Michel Thierry noticed that we were applying WaDisableCtxRestoreArbitration
> even to gen9, which does not require the w/a. The rationale is that we
> need to enable MI arbitration for execlists to work, and to be safe we
> do that before every batch (in addition to every context switch into the
> batch). Since this is not clear from the single line comment suggesting
> the MI_ARB_ENABLE is solely for the w/a, add a little more detail.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>

It can't be clearer. Thanks!

Reviewed-by: Michel Thierry <michel.thierry@intel.com>

> ---
>   drivers/gpu/drm/i915/intel_lrc.c | 18 +++++++++++++++++-
>   1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 476881f63505..fa66e4f58642 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1608,7 +1608,23 @@ static int gen8_emit_bb_start(struct drm_i915_gem_request *req,
>   	if (IS_ERR(cs))
>   		return PTR_ERR(cs);
>   
> -	/* WaDisableCtxRestoreArbitration:bdw,chv */
> +	/*
> +	 * WaDisableCtxRestoreArbitration:bdw,chv
> +	 *
> +	 * We don't need to perform MI_ARB_ENABLE as often as we do (in
> +	 * particular all the gen that do not need the w/a at all!), if we
> +	 * took care to make sure that on every switch into this context
> +	 * (both ordinary and for preemption) that arbitrartion was enabled
> +	 * we would be fine. However, there doesn't seem to be a downside to
> +	 * being paranoid and making sure it is set before each batch and
> +	 * every context-switch.
> +	 *
> +	 * Note that if we fail to enable arbitration before the request
> +	 * is complete, then we do not see the context-switch interrupt and
> +	 * the engine hangs (with RING_HEAD == RING_TAIL).
> +	 *
> +	 * That satisfies both the GPGPU w/a and our heavy-handed paranoia.
> +	 */
>   	*cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
>   
>   	/* FIXME(BDW): Address space and security selectors. */
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/execlists: WaDisableCtxRestoreArbitration is only needed in gen8

[Joonas Lahtinen]

On Thu, 2017-10-05 at 11:54 -0700, Michel Thierry wrote:
> On 10/5/2017 11:28 AM, Chris Wilson wrote:
> > Quoting Michel Thierry (2017-10-05 19:19:27)
> > > WaDisableCtxRestoreArbitration was only applied for bdw and chv, but
> > > this changed after the code got moved to gen8_emit_bb_start (and, at
> > > least in my tree, there is no gen9_emit_bb_start).
> > 
> > But I need to make sure we enable arbitration at some point. :)
> > It may not need the w/a name, but I was making very sure we didn't start
> > a BB without ARB, or context-switch into the middle of a batch without
> > ARB. That was my thinking, that there was no harm in overkill if we set
> > it more often than strictly necessary.
> 
> Ok, thanks for the detailed explanation and yes, probably no harm in 
> setting MI_ARB_ON.

Please add a comment if this is the case. W/As implementing required
logic for platforms not covered by the W/A is confusing when they are
being reviewed or double-checked.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm/i915/execlists: WaDisableCtxRestoreArbitration is only needed in gen8 (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/execlists: WaDisableCtxRestoreArbitration is only needed in gen8 (rev2)
URL   : https://patchwork.freedesktop.org/series/31452/
State : warning

== Summary ==

Series 31452v2 drm/i915/execlists: WaDisableCtxRestoreArbitration is only needed in gen8
https://patchwork.freedesktop.org/api/1.0/series/31452/revisions/2/mbox/

Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> DMESG-FAIL (fi-kbl-r) fdo#102846 +2
                pass       -> DMESG-WARN (fi-cfl-s) fdo#103026
Test gem_flink_basic:
        Subgroup bad-flink:
                pass       -> DMESG-WARN (fi-kbl-r)
        Subgroup bad-open:
                pass       -> DMESG-WARN (fi-kbl-r)
        Subgroup basic:
                pass       -> DMESG-WARN (fi-kbl-r)
        Subgroup double-flink:
                pass       -> DMESG-WARN (fi-kbl-r)
        Subgroup flink-lifetime:
                pass       -> DMESG-WARN (fi-kbl-r)

fdo#102846 https://bugs.freedesktop.org/show_bug.cgi?id=102846
fdo#103026 https://bugs.freedesktop.org/show_bug.cgi?id=103026

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:460s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:472s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:395s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:575s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:287s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:529s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:534s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:546s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:541s
fi-cfl-s         total:289  pass:256  dwarn:1   dfail:0   fail:0   skip:32  time:568s
fi-cnl-y         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:619s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:436s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:598s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:437s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:422s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:463s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:509s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:475s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:507s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:581s
fi-kbl-7567u     total:289  pass:265  dwarn:4   dfail:0   fail:0   skip:20  time:499s
fi-kbl-r         total:125  pass:97   dwarn:5   dfail:2   fail:0   skip:20 
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:655s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:478s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:665s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:530s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:583s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:469s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:579s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:431s

97c9e99b242fe40bbda48ba2bcaed07c47fba085 drm-tip: 2017y-10m-06d-09h-07m-21s UTC integration manifest
a9197bdfd0c3 drm/i915/execlists: Add a comment for the extra MI_ARB_ENABLE

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5918/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/execlists: Add a comment for the extra MI_ARB_ENABLE

[Chris Wilson]

Quoting Michel Thierry (2017-10-05 20:41:40)
> On 10/5/2017 12:10 PM, Chris Wilson wrote:
> > Michel Thierry noticed that we were applying WaDisableCtxRestoreArbitration
> > even to gen9, which does not require the w/a. The rationale is that we
> > need to enable MI arbitration for execlists to work, and to be safe we
> > do that before every batch (in addition to every context switch into the
> > batch). Since this is not clear from the single line comment suggesting
> > the MI_ARB_ENABLE is solely for the w/a, add a little more detail.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Michel Thierry <michel.thierry@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Michał Winiarski <michal.winiarski@intel.com>
> 
> It can't be clearer. Thanks!
> 
> Reviewed-by: Michel Thierry <michel.thierry@intel.com>

Thanks for asking, and checking what I wrote made sense!
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx