Re: [PATCH 1/3] drm/i915: Enable render context support for Ironlake (gen5)

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Nov 23, 2017 at 04:27:48PM +0000, Chris Wilson wrote:
> Ironlake does 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.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c  | 2 ++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 7 +++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index fede62daf3e1..88ef00faf576 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -175,6 +175,8 @@ __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:
> +			cxt_size = I915_READ(CXT_SIZE);
> +			return round_up(cxt_size * 64, PAGE_SIZE);

I don't think this is correct. It misses the non-pipelined 3D state,
and the ring stuff at the start which IIRC at least SNB still
saved even though it's not used in ring buffer mode. So I think
this needs a 0xb added to the CXT_SIZE value.

But even that doesn't really match the docs. The context image layout
is shown to be 0x3b cachelines long, but 0xb+0x2d only gets us to
0x38. So it looks like CXT_SIZE is off by two for some reason. But
it does't really matter I suppose since we round it up to
a page anyway.

I don't think I ever looked at the context dump to see how big it
really is on ILK. IIRC I did that on SNB and IVB, and maybe HSW.

>  		case 4:
>  		case 3:
>  		case 2:
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index e2085820b586..e649b564b165 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1403,11 +1403,14 @@ static inline int mi_set_context(struct drm_i915_gem_request *rq, u32 flags)
>  		/* These flags are for resource streamer on HSW+ */
>  		flags |= HSW_MI_RS_SAVE_STATE_EN | HSW_MI_RS_RESTORE_STATE_EN;
>  	else
> +		/* We need to save the extended state for powersaving modes */
>  		flags |= MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN;
>  
>  	len = 4;
>  	if (IS_GEN7(i915))
>  		len += 2 + (num_rings ? 4*num_rings + 6 : 0);
> +	if (IS_GEN5(i915))
> +		len += 2;
>  
>  	cs = intel_ring_begin(rq, len);
>  	if (IS_ERR(cs))
> @@ -1430,6 +1433,8 @@ static inline int mi_set_context(struct drm_i915_gem_request *rq, u32 flags)
>  						GEN6_PSMI_SLEEP_MSG_DISABLE);
>  			}
>  		}
> +	} else if (IS_GEN5(i915)) {
> +		*cs++ = MI_SUSPEND_FLUSH | MI_SUSPEND_FLUSH_EN;

Hmm. These are documented for steppings A/B. I wonder what is the
first production stepping for ILK. Latest stepping mentioned in the
w/a db is C2.

Oh, actually w/a db has WaIlkEnableDisableSuspendFlush listed a
"forever". Not sure which source to believe here.

>  	}
>  
>  	*cs++ = MI_NOOP;
> @@ -1464,6 +1469,8 @@ static inline int mi_set_context(struct drm_i915_gem_request *rq, u32 flags)
>  			*cs++ = MI_NOOP;
>  		}
>  		*cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
> +	} else if (IS_GEN5(i915)) {
> +		*cs++ = MI_SUSPEND_FLUSH;
>  	}
>  
>  	intel_ring_advance(rq, cs);
> -- 
> 2.15.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx