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

[Michel Thierry <michel.thierry@intel.com>]

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