| Message ID | 1431937916-11686-3-git-send-email-abdiel.janulgue@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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.
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
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
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. */
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(-)