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

From: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v3 3/3] drm/i915: Enable Resource Streamer state save/restore in HSW
Date: Tue, 19 May 2015 11:31:21 +0300	[thread overview]
Message-ID: <555AF4D9.7080807@linux.intel.com> (raw)
In-Reply-To: <20150519082612.GK15256@phenom.ffwll.local>

On 05/19/2015 11:26 AM, Daniel Vetter wrote:
> On Tue, May 19, 2015 at 09:58:52AM +0300, Abdiel Janulgue wrote:
>>
>>
>> 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@chris-wilson.co.uk>
>>>>>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@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@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.
> 
> If the bit has been renamed it might be good to add at least a new #define
> with a _HSw suffix and better name, just to make it clear what's going on.
> gcc will see through this and fold down the different conditions to just
> one.

I'll do that in the next revision and also add the igt tag you require.
In the meantime, please check the igt approach I sent is the correct one.

Thanks!
-Abdiel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx