[v3,3/3] drm/i915: Enable Resource Streamer state save/restore in HSW
diff mbox

Message ID 1431937916-11686-3-git-send-email-abdiel.janulgue@linux.intel.com
State New
Headers show

Commit Message

Abdiel Janulgue May 18, 2015, 8:31 a.m. UTC
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(-)

Comments

Ville Syrjälä May 18, 2015, 3:36 p.m. UTC | #1
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.

>  
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 238bb25..3db0596 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2498,7 +2498,8 @@ enum skl_disp_power_wells {
>   * valid. Now, docs explain in dwords what is in the context object. The full
>   * size is 70720 bytes, however, the power context and execlist context will
>   * never be saved (power context is stored elsewhere, and execlists don't work
> - * on HSW) - so the final size is 66944 bytes, which rounds to 17 pages.
> + * on HSW) - so the final size, including the extra state required for the
> + * Resource Streamer, is 66944 bytes, which rounds to 17 pages.
>   */
>  #define HSW_CXT_TOTAL_SIZE		(17 * PAGE_SIZE)
>  /* Same as Haswell, but 72064 bytes now. */
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson May 18, 2015, 3:41 p.m. UTC | #2
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. 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.
-Chris
Ville Syrjälä May 18, 2015, 4:07 p.m. UTC | #3
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.
Abdiel Janulgue May 19, 2015, 6:58 a.m. UTC | #4
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.
Daniel Vetter May 19, 2015, 8:26 a.m. UTC | #5
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.
-Daniel
Abdiel Janulgue May 19, 2015, 8:31 a.m. UTC | #6
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

Patch
diff mbox

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);
 
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 238bb25..3db0596 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2498,7 +2498,8 @@  enum skl_disp_power_wells {
  * valid. Now, docs explain in dwords what is in the context object. The full
  * size is 70720 bytes, however, the power context and execlist context will
  * never be saved (power context is stored elsewhere, and execlists don't work
- * on HSW) - so the final size is 66944 bytes, which rounds to 17 pages.
+ * on HSW) - so the final size, including the extra state required for the
+ * Resource Streamer, is 66944 bytes, which rounds to 17 pages.
  */
 #define HSW_CXT_TOTAL_SIZE		(17 * PAGE_SIZE)
 /* Same as Haswell, but 72064 bytes now. */