[PATCH 1/2] drm/i915/lrc: Clear context restore/save inhibit flags for new contexts

[PATCH 1/2] drm/i915/lrc: Clear context restore/save inhibit flags for new contexts

[Chris Wilson]

CTX_CONTEXT_CONTROL (CTX_SR_CTL) operates as a masked register and so
will only apply the bits that are selected by the upper half. In the
case of selectively enabling sr inhibit, this may mean the context keeps
the current setting (so forgetting to save the context later, eventually
leading to a very upset GPU!).

Fixes: d2b4b97933f5 ("drm/i915: Record the default hw state after reset upon load")
Fixes: 517aaffe0c1b ("drm/i915/execlists: Inhibit context save/restore for the fake preempt context")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 89e92defbcfe..29b14d7d4b07 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -456,6 +456,12 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
 	ce->ring->tail &= (ce->ring->size - 1);
 	ce->lrc_reg_state[CTX_RING_TAIL+1] = ce->ring->tail;
 
+	GEM_BUG_ON((ce->lrc_reg_state[CTX_CONTEXT_CONTROL + 1] &
+		    _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
+				       CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT)) !=
+		   _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
+				      CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT));
+
 	GEM_TRACE("%s\n", engine->name);
 	for (n = execlists_num_ports(&engine->execlists); --n; )
 		elsp_write(0, engine->execlists.elsp);
@@ -2118,6 +2124,8 @@ static void execlists_init_reg_state(u32 *regs,
 				 MI_LRI_FORCE_POSTED;
 
 	CTX_REG(regs, CTX_CONTEXT_CONTROL, RING_CONTEXT_CONTROL(engine),
+		_MASKED_BIT_DISABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
+				    CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT) |
 		_MASKED_BIT_ENABLE(CTX_CTRL_INHIBIT_SYN_CTX_SWITCH |
 				   (HAS_RESOURCE_STREAMER(dev_priv) ?
 				   CTX_CTRL_RS_CTX_ENABLE : 0)));
-- 
2.15.1

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

[PATCH 2/2] drm/i915/execlists: Remove the ring advancement under preemption

[Chris Wilson]

Load an empty ringbuffer for preemption, ignoring the lite-restore
workaround as we now the preempt context is always idle before preemption.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 29b14d7d4b07..d6758838311f 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -449,13 +449,6 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
 	unsigned int n;
 
 	GEM_BUG_ON(engine->i915->preempt_context->hw_id != PREEMPT_ID);
-	GEM_BUG_ON(!IS_ALIGNED(ce->ring->size, WA_TAIL_BYTES));
-
-	memset(ce->ring->vaddr + ce->ring->tail, 0, WA_TAIL_BYTES);
-	ce->ring->tail += WA_TAIL_BYTES;
-	ce->ring->tail &= (ce->ring->size - 1);
-	ce->lrc_reg_state[CTX_RING_TAIL+1] = ce->ring->tail;
-
 	GEM_BUG_ON((ce->lrc_reg_state[CTX_CONTEXT_CONTROL + 1] &
 		    _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
 				       CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT)) !=
-- 
2.15.1

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

Re: [PATCH 2/2] drm/i915/execlists: Remove the ring advancement under preemption

[Michel Thierry]

On 1/25/2018 3:24 AM, Chris Wilson wrote:
> Load an empty ringbuffer for preemption, ignoring the lite-restore
> workaround as we now the preempt context is always idle before preemption.

s/we now/we know/?

Looks ok to me; the restriction is to avoid a lite-restore with 
HEAD==TAIL, and as you said the preempt ctx could not be running at this 
point.

Michal(s)/Tvrtko/Mika, any thoughts?

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_lrc.c | 7 -------
>   1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 29b14d7d4b07..d6758838311f 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -449,13 +449,6 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
>   	unsigned int n;
>   
>   	GEM_BUG_ON(engine->i915->preempt_context->hw_id != PREEMPT_ID);
> -	GEM_BUG_ON(!IS_ALIGNED(ce->ring->size, WA_TAIL_BYTES));
> -
> -	memset(ce->ring->vaddr + ce->ring->tail, 0, WA_TAIL_BYTES);
> -	ce->ring->tail += WA_TAIL_BYTES;
> -	ce->ring->tail &= (ce->ring->size - 1);
> -	ce->lrc_reg_state[CTX_RING_TAIL+1] = ce->ring->tail;
> -
>   	GEM_BUG_ON((ce->lrc_reg_state[CTX_CONTEXT_CONTROL + 1] &
>   		    _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
>   				       CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT)) !=
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/lrc: Clear context restore/save inhibit flags for new contexts

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/lrc: Clear context restore/save inhibit flags for new contexts
URL   : https://patchwork.freedesktop.org/series/37099/
State : success

== Summary ==

Series 37099v1 series starting with [1/2] drm/i915/lrc: Clear context restore/save inhibit flags for new contexts
https://patchwork.freedesktop.org/api/1.0/series/37099/revisions/1/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                dmesg-warn -> DMESG-FAIL (fi-elk-e7500) fdo#103989
Test gem_exec_reloc:
        Subgroup basic-gtt-read:
                pass       -> INCOMPLETE (fi-byt-j1900) fdo#102657
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                incomplete -> PASS       (fi-snb-2520m) fdo#103713

fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fdo#102657 https://bugs.freedesktop.org/show_bug.cgi?id=102657
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:419s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:435s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:375s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:488s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:282s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:481s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:488s
fi-byt-j1900     total:76   pass:66   dwarn:0   dfail:0   fail:0   skip:9  
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:461s
fi-elk-e7500     total:224  pass:168  dwarn:9   dfail:1   fail:0   skip:45 
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:278s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:514s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:392s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:400s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:416s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:453s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:417s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:463s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:497s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:453s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:504s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:580s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:423s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:513s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:533s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:490s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:492s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:417s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:432s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:533s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:402s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:566s
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:474s

9d8467fe5626095314bc34449457798dae066fbb drm-tip: 2018y-01m-24d-19h-59m-41s UTC integration manifest
3badc56541db drm/i915/execlists: Remove the ring advancement under preemption
d569899c3d45 drm/i915/lrc: Clear context restore/save inhibit flags for new contexts

== Logs ==

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

✗ Fi.CI.IGT: failure for series starting with [1/2] drm/i915/lrc: Clear context restore/save inhibit flags for new contexts

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/lrc: Clear context restore/save inhibit flags for new contexts
URL   : https://patchwork.freedesktop.org/series/37099/
State : failure

== Summary ==

Test kms_flip:
        Subgroup plain-flip-fb-recreate-interruptible:
                fail       -> PASS       (shard-hsw) fdo#100368
        Subgroup 2x-flip-vs-absolute-wf_vblank:
                pass       -> FAIL       (shard-hsw)
        Subgroup 2x-plain-flip-fb-recreate-interruptible:
                pass       -> FAIL       (shard-hsw)
        Subgroup 2x-flip-vs-modeset-interruptible:
                dmesg-warn -> PASS       (shard-hsw) fdo#102614
Test kms_sysfs_edid_timing:
                pass       -> WARN       (shard-apl) fdo#100047
Test gem_exec_schedule:
        Subgroup deep-vebox:
                skip       -> FAIL       (shard-apl) fdo#102848
Test kms_cursor_legacy:
        Subgroup flip-vs-cursor-crc-atomic:
                fail       -> PASS       (shard-apl) fdo#102670

fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
fdo#102848 https://bugs.freedesktop.org/show_bug.cgi?id=102848
fdo#102670 https://bugs.freedesktop.org/show_bug.cgi?id=102670

shard-apl        total:2838 pass:1752 dwarn:1   dfail:0   fail:23  skip:1061 time:12603s
shard-hsw        total:2838 pass:1733 dwarn:1   dfail:0   fail:13  skip:1090 time:11934s
shard-snb        total:2838 pass:1330 dwarn:1   dfail:0   fail:10  skip:1497 time:6644s
Blacklisted hosts:
shard-kbl        total:2838 pass:1870 dwarn:3   dfail:0   fail:24  skip:941 time:9644s

== Logs ==

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

Re: [PATCH 1/2] drm/i915/lrc: Clear context restore/save inhibit flags for new contexts

[Michel Thierry]

On 1/25/2018 3:24 AM, Chris Wilson wrote:
> CTX_CONTEXT_CONTROL (CTX_SR_CTL) operates as a masked register and so
> will only apply the bits that are selected by the upper half. In the
> case of selectively enabling sr inhibit, this may mean the context keeps
> the current setting (so forgetting to save the context later, eventually
> leading to a very upset GPU!).

Oops, true no one would clear Context Save Inhibit once it's set.

> 
> Fixes: d2b4b97933f5 ("drm/i915: Record the default hw state after reset upon load")
Does it really fixes this one ^^^?

Restore Inhibit has this note: "This is not a true register bit.... This 
bit will always be in clear state on a context save of this bit". Maybe 
that's why didn't see any problems. But it doesn't hurt being paranoid.

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


> Fixes: 517aaffe0c1b ("drm/i915/execlists: Inhibit context save/restore for the fake preempt context")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_lrc.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 89e92defbcfe..29b14d7d4b07 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -456,6 +456,12 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
>   	ce->ring->tail &= (ce->ring->size - 1);
>   	ce->lrc_reg_state[CTX_RING_TAIL+1] = ce->ring->tail;
>   
> +	GEM_BUG_ON((ce->lrc_reg_state[CTX_CONTEXT_CONTROL + 1] &
> +		    _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
> +				       CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT)) !=
> +		   _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
> +				      CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT));
> +
>   	GEM_TRACE("%s\n", engine->name);
>   	for (n = execlists_num_ports(&engine->execlists); --n; )
>   		elsp_write(0, engine->execlists.elsp);
> @@ -2118,6 +2124,8 @@ static void execlists_init_reg_state(u32 *regs,
>   				 MI_LRI_FORCE_POSTED;
>   
>   	CTX_REG(regs, CTX_CONTEXT_CONTROL, RING_CONTEXT_CONTROL(engine),
> +		_MASKED_BIT_DISABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
> +				    CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT) |
>   		_MASKED_BIT_ENABLE(CTX_CTRL_INHIBIT_SYN_CTX_SWITCH |
>   				   (HAS_RESOURCE_STREAMER(dev_priv) ?
>   				   CTX_CTRL_RS_CTX_ENABLE : 0)));
> 

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

Re: [PATCH 1/2] drm/i915/lrc: Clear context restore/save inhibit flags for new contexts

[Chris Wilson]

Quoting Michel Thierry (2018-01-25 17:49:49)
> On 1/25/2018 3:24 AM, Chris Wilson wrote:
> > CTX_CONTEXT_CONTROL (CTX_SR_CTL) operates as a masked register and so
> > will only apply the bits that are selected by the upper half. In the
> > case of selectively enabling sr inhibit, this may mean the context keeps
> > the current setting (so forgetting to save the context later, eventually
> > leading to a very upset GPU!).
> 
> Oops, true no one would clear Context Save Inhibit once it's set.
> 
> > 
> > Fixes: d2b4b97933f5 ("drm/i915: Record the default hw state after reset upon load")
> Does it really fixes this one ^^^?

It's a stretch, I don't have an observed bug, but I felt it was sensible
for completeness.
 
> Restore Inhibit has this note: "This is not a true register bit.... This 
> bit will always be in clear state on a context save of this bit". Maybe 
> that's why didn't see any problems. But it doesn't hurt being paranoid.

Paranoid is my middle name. Well it would be if the HW wasn't out to get
me.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/lrc: Clear context restore/save inhibit flags for new contexts

[Chris Wilson]

Quoting Michel Thierry (2018-01-25 17:49:49)
> On 1/25/2018 3:24 AM, Chris Wilson wrote:
> > CTX_CONTEXT_CONTROL (CTX_SR_CTL) operates as a masked register and so
> > will only apply the bits that are selected by the upper half. In the
> > case of selectively enabling sr inhibit, this may mean the context keeps
> > the current setting (so forgetting to save the context later, eventually
> > leading to a very upset GPU!).
> 
> Oops, true no one would clear Context Save Inhibit once it's set.
> 
> > 
> > Fixes: d2b4b97933f5 ("drm/i915: Record the default hw state after reset upon load")
> Does it really fixes this one ^^^?

Actually, having added the BUG_ON, I had better not recommend back
porting to the earlier fixes.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/lrc: Clear context restore/save inhibit flags for new contexts

[Chris Wilson]

Quoting Michel Thierry (2018-01-25 17:49:49)
> On 1/25/2018 3:24 AM, Chris Wilson wrote:
> > CTX_CONTEXT_CONTROL (CTX_SR_CTL) operates as a masked register and so
> > will only apply the bits that are selected by the upper half. In the
> > case of selectively enabling sr inhibit, this may mean the context keeps
> > the current setting (so forgetting to save the context later, eventually
> > leading to a very upset GPU!).
> 
> Oops, true no one would clear Context Save Inhibit once it's set.
> 
> > 
> > Fixes: d2b4b97933f5 ("drm/i915: Record the default hw state after reset upon load")
> Does it really fixes this one ^^^?
> 
> Restore Inhibit has this note: "This is not a true register bit.... This 
> bit will always be in clear state on a context save of this bit". Maybe 
> that's why didn't see any problems. But it doesn't hurt being paranoid.
> 
> Reviewed-by: Michel Thierry <michel.thierry@intel.com>

Trimmed the earlier fixes and pushed. Thanks for the review!
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx