[Intel-gfx] [PATCH v3 3/3] drm/i915: Enable Resource Streamer state save/restore in HSW

[Abdiel Janulgue]

On 05/18/2015 07:07 PM, Ville Syrjälä wrote:
> On Mon, May 18, 2015 at 04:41:51PM +0100, Chris Wilson wrote:
>> On Mon, May 18, 2015 at 06:36:18PM +0300, Ville Syrjälä wrote:
>>> On Mon, May 18, 2015 at 11:31:56AM +0300, Abdiel Janulgue wrote:
>>>> Also clarify comments on context size that the extra state for
>>>> Resource Streamer is included.
>>>>
>>>> Suggested-by: Chris Wilson <chris at chris-wilson.co.uk>
>>>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue at linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
>>>>  drivers/gpu/drm/i915/i915_reg.h         | 3 ++-
>>>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
>>>> index f3e84c4..1db107a 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>>>> @@ -509,7 +509,7 @@ mi_set_context(struct intel_engine_cs *ring,
>>>>  	}
>>>>  
>>>>  	/* These flags are for resource streamer on HSW+ */
>>>> -	if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
>>>> +	if (IS_HASWELL(ring->dev))
>>>>  		flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
>>>
>>> I don't get it. Previously we told the hardware to save the extended
>>> context on !hsw, and now we don't. That doesn't seem correct to me.
>>
>> We don't use the extended state elsewhere.
> 
> Umm. On SNB at least 3DSTATE_POLY_STIPPLE_PATTERN seems to be part of
> the extended state, and on IVB/VLV SOL state is there. Mesa uses all of
> that.
> 
>> I'd always been dubious of
>> the origins/intentions of this line of code since it claims only to be
>> for enabling RS on HSW...
>>
>> i.e. commit e80f14b6d36e3e07111cf2ab084ef8dd5d015ce2
>> Author: Ben Widawsky <benjamin.widawsky at intel.com>
>> Date:   Mon Aug 18 10:35:28 2014 -0700
>>
>>     drm/i915: Don't save/restore RS when not used
>>  
>> was backwards.
> 
> ? It did exactly what it said, ie. avoid setting the RS save/restore
> bits on HSW+ while leaving the ext save/restore enabled on older
> platforms.
> 

Another option is to enable extended state save restore for both HSW and
the older platforms so we would get both features (RS save/restore and
Extended State Save enable)?

Seems the reason for this confusion is that the we have the exact same
bit positions in MI_SET_CONTEXT but it got renamed starting from HSW and up.