| Message ID | 20190927222427.5491-1-james.ausmus@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] drm/i915: Move SAGV block time to dev_priv | expand |
On Fri, Sep 27, 2019 at 03:24:26PM -0700, James Ausmus wrote: > In prep for newer platforms having more complicated ways to determine > the SAGV block time, move the variable to dev_priv, and extract the > setting to an initial setup function. While we're at it, update the if > ladder to follow the new gen -> old gen order preference, and warn on > any non-specified gen. > > v2: Shorten the function name (Ville), return directly (Ville), move > sagv_block_time_us value to dev_priv (Ville) > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Signed-off-by: James Ausmus <james.ausmus@intel.com> > --- > > Ville - with the amount of v1..v2 change in this first patch, I wasn't > comfortable applying your R-b, could you take another look? Patch 2 just > has the trivial changes you suggested, so I kept that one. Review ping. :) Thanks! -James > > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_pm.c | 33 ++++++++++++++++++++++++--------- > 2 files changed, 26 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 337d8306416a..87a835a0210b 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1579,6 +1579,8 @@ struct drm_i915_private { > I915_SAGV_NOT_CONTROLLED > } sagv_status; > > + int sagv_block_time_us; > + > struct { > /* > * Raw watermark latency values: > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index bfcf03ab5245..b413a7f3bc5d 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3642,6 +3642,26 @@ intel_has_sagv(struct drm_i915_private *dev_priv) > dev_priv->sagv_status != I915_SAGV_NOT_CONTROLLED; > } > > +static void > +skl_setup_sagv_block_time(struct drm_i915_private *dev_priv) > +{ > + if (IS_GEN(dev_priv, 11)) { > + dev_priv->sagv_block_time_us = 10; > + return; > + } else if (IS_GEN(dev_priv, 10)) { > + dev_priv->sagv_block_time_us = 20; > + return; > + } else if (IS_GEN(dev_priv, 9)) { > + dev_priv->sagv_block_time_us = 30; > + return; > + } else { > + MISSING_CASE(INTEL_GEN(dev_priv)); > + } > + > + /* Default to an unusable block time */ > + dev_priv->sagv_block_time_us = 1000; > +} > + > /* > * SAGV dynamically adjusts the system agent voltage and clock frequencies > * depending on power and performance requirements. The display engine access > @@ -3730,18 +3750,10 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state) > struct intel_crtc_state *crtc_state; > enum pipe pipe; > int level, latency; > - int sagv_block_time_us; > > if (!intel_has_sagv(dev_priv)) > return false; > > - if (IS_GEN(dev_priv, 9)) > - sagv_block_time_us = 30; > - else if (IS_GEN(dev_priv, 10)) > - sagv_block_time_us = 20; > - else > - sagv_block_time_us = 10; > - > /* > * If there are no active CRTCs, no additional checks need be performed > */ > @@ -3788,7 +3800,7 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state) > * incur memory latencies higher than sagv_block_time_us we > * can't enable SAGV. > */ > - if (latency < sagv_block_time_us) > + if (latency < dev_priv->sagv_block_time_us) > return false; > } > > @@ -9013,6 +9025,9 @@ void intel_init_pm(struct drm_i915_private *dev_priv) > else if (IS_GEN(dev_priv, 5)) > i915_ironlake_get_mem_freq(dev_priv); > > + if (IS_GEN9_BC(dev_priv) || INTEL_GEN(dev_priv) >= 10) > + skl_setup_sagv_block_time(dev_priv); > + > /* For FIFO watermark updates */ > if (INTEL_GEN(dev_priv) >= 9) { > skl_setup_wm_latency(dev_priv); > -- > 2.22.1 >
On Fri, Oct 04, 2019 at 01:53:57PM -0700, Lucas De Marchi wrote: > On Fri, Sep 27, 2019 at 03:24:26PM -0700, James Ausmus wrote: > >In prep for newer platforms having more complicated ways to determine > >the SAGV block time, move the variable to dev_priv, and extract the > >setting to an initial setup function. While we're at it, update the if > >ladder to follow the new gen -> old gen order preference, and warn on > >any non-specified gen. > > > >v2: Shorten the function name (Ville), return directly (Ville), move > >sagv_block_time_us value to dev_priv (Ville) > > > >Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > >Cc: Lucas De Marchi <lucas.demarchi@intel.com> > >Signed-off-by: James Ausmus <james.ausmus@intel.com> > >--- > > > >Ville - with the amount of v1..v2 change in this first patch, I wasn't > >comfortable applying your R-b, could you take another look? Patch 2 just > >has the trivial changes you suggested, so I kept that one. > > > > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > > drivers/gpu/drm/i915/intel_pm.c | 33 ++++++++++++++++++++++++--------- > > 2 files changed, 26 insertions(+), 9 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >index 337d8306416a..87a835a0210b 100644 > >--- a/drivers/gpu/drm/i915/i915_drv.h > >+++ b/drivers/gpu/drm/i915/i915_drv.h > >@@ -1579,6 +1579,8 @@ struct drm_i915_private { > > I915_SAGV_NOT_CONTROLLED > > } sagv_status; > > > >+ int sagv_block_time_us; > >+ > > struct { > > /* > > * Raw watermark latency values: > >diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > >index bfcf03ab5245..b413a7f3bc5d 100644 > >--- a/drivers/gpu/drm/i915/intel_pm.c > >+++ b/drivers/gpu/drm/i915/intel_pm.c > >@@ -3642,6 +3642,26 @@ intel_has_sagv(struct drm_i915_private *dev_priv) > > dev_priv->sagv_status != I915_SAGV_NOT_CONTROLLED; > > } > > > >+static void > >+skl_setup_sagv_block_time(struct drm_i915_private *dev_priv) > >+{ > >+ if (IS_GEN(dev_priv, 11)) { > >+ dev_priv->sagv_block_time_us = 10; > >+ return; > >+ } else if (IS_GEN(dev_priv, 10)) { > >+ dev_priv->sagv_block_time_us = 20; > >+ return; > >+ } else if (IS_GEN(dev_priv, 9)) { > >+ dev_priv->sagv_block_time_us = 30; > >+ return; > >+ } else { > >+ MISSING_CASE(INTEL_GEN(dev_priv)); > >+ } > >+ > >+ /* Default to an unusable block time */ > >+ dev_priv->sagv_block_time_us = 1000; > > I would actually make sagv_block_time_us unsigned and assign -1 here. > But making it unsigned would mean cascade that down to the wm > calculations. Humn... Maybe there's a reason for that to be signed that > I'm not seeing. Hmm - yeah, I don't see a reason why it needs to be signed either. Hadn't looked in to it previously, was just following the existing code. :) I'll switch that up and send a v3. > > > >+} > >+ > > /* > > * SAGV dynamically adjusts the system agent voltage and clock frequencies > > * depending on power and performance requirements. The display engine access > >@@ -3730,18 +3750,10 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state) > > struct intel_crtc_state *crtc_state; > > enum pipe pipe; > > int level, latency; > >- int sagv_block_time_us; > > > > if (!intel_has_sagv(dev_priv)) > > return false; > > > >- if (IS_GEN(dev_priv, 9)) > >- sagv_block_time_us = 30; > >- else if (IS_GEN(dev_priv, 10)) > >- sagv_block_time_us = 20; > >- else > >- sagv_block_time_us = 10; > >- > > /* > > * If there are no active CRTCs, no additional checks need be performed > > */ > >@@ -3788,7 +3800,7 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state) > > * incur memory latencies higher than sagv_block_time_us we > > * can't enable SAGV. > > */ > >- if (latency < sagv_block_time_us) > >+ if (latency < dev_priv->sagv_block_time_us) > > return false; > > } > > > >@@ -9013,6 +9025,9 @@ void intel_init_pm(struct drm_i915_private *dev_priv) > > else if (IS_GEN(dev_priv, 5)) > > i915_ironlake_get_mem_freq(dev_priv); > > > >+ if (IS_GEN9_BC(dev_priv) || INTEL_GEN(dev_priv) >= 10) > >+ skl_setup_sagv_block_time(dev_priv); > > Do we want to use intel_has_sagv() here? Yeah, that would make sense, will fix up. Thanks! -James > > >+ > > /* For FIFO watermark updates */ > > if (INTEL_GEN(dev_priv) >= 9) { > > skl_setup_wm_latency(dev_priv); > >-- > >2.22.1 > >
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 337d8306416a..87a835a0210b 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1579,6 +1579,8 @@ struct drm_i915_private { I915_SAGV_NOT_CONTROLLED } sagv_status; + int sagv_block_time_us; + struct { /* * Raw watermark latency values: diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index bfcf03ab5245..b413a7f3bc5d 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3642,6 +3642,26 @@ intel_has_sagv(struct drm_i915_private *dev_priv) dev_priv->sagv_status != I915_SAGV_NOT_CONTROLLED; } +static void +skl_setup_sagv_block_time(struct drm_i915_private *dev_priv) +{ + if (IS_GEN(dev_priv, 11)) { + dev_priv->sagv_block_time_us = 10; + return; + } else if (IS_GEN(dev_priv, 10)) { + dev_priv->sagv_block_time_us = 20; + return; + } else if (IS_GEN(dev_priv, 9)) { + dev_priv->sagv_block_time_us = 30; + return; + } else { + MISSING_CASE(INTEL_GEN(dev_priv)); + } + + /* Default to an unusable block time */ + dev_priv->sagv_block_time_us = 1000; +} + /* * SAGV dynamically adjusts the system agent voltage and clock frequencies * depending on power and performance requirements. The display engine access @@ -3730,18 +3750,10 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state) struct intel_crtc_state *crtc_state; enum pipe pipe; int level, latency; - int sagv_block_time_us; if (!intel_has_sagv(dev_priv)) return false; - if (IS_GEN(dev_priv, 9)) - sagv_block_time_us = 30; - else if (IS_GEN(dev_priv, 10)) - sagv_block_time_us = 20; - else - sagv_block_time_us = 10; - /* * If there are no active CRTCs, no additional checks need be performed */ @@ -3788,7 +3800,7 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state) * incur memory latencies higher than sagv_block_time_us we * can't enable SAGV. */ - if (latency < sagv_block_time_us) + if (latency < dev_priv->sagv_block_time_us) return false; } @@ -9013,6 +9025,9 @@ void intel_init_pm(struct drm_i915_private *dev_priv) else if (IS_GEN(dev_priv, 5)) i915_ironlake_get_mem_freq(dev_priv); + if (IS_GEN9_BC(dev_priv) || INTEL_GEN(dev_priv) >= 10) + skl_setup_sagv_block_time(dev_priv); + /* For FIFO watermark updates */ if (INTEL_GEN(dev_priv) >= 9) { skl_setup_wm_latency(dev_priv);
In prep for newer platforms having more complicated ways to determine the SAGV block time, move the variable to dev_priv, and extract the setting to an initial setup function. While we're at it, update the if ladder to follow the new gen -> old gen order preference, and warn on any non-specified gen. v2: Shorten the function name (Ville), return directly (Ville), move sagv_block_time_us value to dev_priv (Ville) Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> Cc: Lucas De Marchi <lucas.demarchi@intel.com> Signed-off-by: James Ausmus <james.ausmus@intel.com> --- Ville - with the amount of v1..v2 change in this first patch, I wasn't comfortable applying your R-b, could you take another look? Patch 2 just has the trivial changes you suggested, so I kept that one. drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/intel_pm.c | 33 ++++++++++++++++++++++++--------- 2 files changed, 26 insertions(+), 9 deletions(-)