Re: [PATCH 4/4] drm/i915: Enable render context support for gen4 (Broadwater to Cantiga)

[Kenneth Graunke <kenneth@whitecape.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4965 bytes --]

On Friday, April 19, 2019 4:16:01 AM PDT Chris Wilson wrote:
> Broadwater and the rest of gen4  do support being able to saving and
> reloading context specific registers between contexts, providing isolation
> of the basic GPU state (as programmable by userspace). This allows
> userspace to assume that the GPU retains their state from one batch to the
> next, minimising the amount of state it needs to reload and manually save
> across batches.
> 
> v2: CONSTANT_BUFFER woes
> 
> Running through piglit turned up an interesting issue, a GPU hang inside
> the context load. The context image includes the CONSTANT_BUFFER command
> that loads an address into a on-gpu buffer, and the context load was
> executing that immediately. However, since it was reading from the GTT
> there is no guarantee that the GTT retains the same configuration as
> when the context was saved, resulting in stray reads and a GPU hang.
> 
> Having tried issuing a CONSTANT_BUFFER (to disable the command) from the
> ring before saving the context to no avail, we resort to patching out
> the instruction inside the context image before loading.
> 
> This does impose that gen4 always reissues CONSTANT_BUFFER commands on
> each batch, but due to the use of a shared GTT that was and will remain
> a requirement.
> 
> v3: ECOSPKD to the rescue
> 
> Ville found the magic bit in the ECOSPKD to disable saving and restoring
> the CONSTANT_BUFFER from the context image, thereby completely avoiding
> the GPU hangs from chasing invalid pointers. This appears to be the
> default behaviour for gen5, and so we just need to tweak gen4 to match.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Kenneth Graunke <kenneth@whitecape.org>
> ---
>  drivers/gpu/drm/i915/i915_reg.h         |  3 +++
>  drivers/gpu/drm/i915/intel_engine_cs.c  |  2 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 14 ++++++++++++++
>  3 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b74824f0b5b1..5815703ac35f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2665,6 +2665,9 @@ enum i915_power_well_id {
>  # define MODE_IDLE					(1 << 9)
>  # define STOP_RING					(1 << 8)
>  
> +#define ECOSPKD		_MMIO(0x21d0)
> +# define CONSTANT_BUFFER_SR_DISABLE BIT(4)
> +

The name of this register is ECOSKPD (Scratch Pad, or SK PD).

The G45 PRM says it's DevBW-C1+.  I can't recall if earlier ones
shipped or not.  If so, we might be in trouble.  But I've seen a
lot of DevBW-A/B warnings that I'm pretty sure I've safely ignored...

With the typo fixed, this patch is:
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

>  #define GEN6_GT_MODE	_MMIO(0x20d0)
>  #define GEN7_GT_MODE	_MMIO(0x7008)
>  #define   GEN6_WIZ_HASHING(hi, lo)			(((hi) << 9) | ((lo) << 7))
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index fc8be2fcb4e6..f9db2e0bca12 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -211,6 +211,7 @@ __intel_engine_context_size(struct drm_i915_private *dev_priv, u8 class)
>  			return round_up(GEN6_CXT_TOTAL_SIZE(cxt_size) * 64,
>  					PAGE_SIZE);
>  		case 5:
> +		case 4:
>  			/*
>  			 * There is a discrepancy here between the size reported
>  			 * by the register and the size of the context layout
> @@ -227,7 +228,6 @@ __intel_engine_context_size(struct drm_i915_private *dev_priv, u8 class)
>  					 cxt_size * 64,
>  					 cxt_size - 1);
>  			return round_up(cxt_size * 64, PAGE_SIZE);
> -		case 4:
>  		case 3:
>  		case 2:
>  		/* For the special day when i810 gets merged. */
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 2d2e33cd3fae..26b276ed00b3 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -832,6 +832,20 @@ static int init_render_ring(struct intel_engine_cs *engine)
>  {
>  	struct drm_i915_private *dev_priv = engine->i915;
>  
> +	/*
> +	 * Disable CONSTANT_BUFFER before it is loaded from the context
> +	 * image. For as it is loaded, it is executed and the stored
> +	 * address may no longer be valid, leading to a GPU hang.
> +	 *
> +	 * This imposes the requirement that userspace reload their
> +	 * CONSTANT_BUFFER on every batch, fortunately a requirement
> +	 * they are already accustomed to from before contexts were
> +	 * enabled.
> +	 */
> +	if (IS_GEN(dev_priv, 4))
> +		I915_WRITE(ECOSPKD,
> +			   _MASKED_BIT_ENABLE(CONSTANT_BUFFER_SR_DISABLE));
> +
>  	/* WaTimedSingleVertexDispatch:cl,bw,ctg,elk,ilk,snb */
>  	if (IS_GEN_RANGE(dev_priv, 4, 6))
>  		I915_WRITE(MI_MODE, _MASKED_BIT_ENABLE(VS_TIMER_DISPATCH));
> 


[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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