Re: [Intel-gfx] [PATCH v2 1/2] drm/i915: Move SAGV block time to dev_priv
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ä
> >Cc: Stanislav Lisovskiy
> >Cc: Lucas De Marchi
> >Signed-off-by: James Ausmus
> >---
> >
> >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
> >
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 1/2] drm/i915: Move SAGV block time to dev_priv
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ä
Cc: Stanislav Lisovskiy
Cc: Lucas De Marchi
Signed-off-by: James Ausmus
---
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.
+}
+
/*
* 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?
+
/* For FIFO watermark updates */
if (INTEL_GEN(dev_priv) >= 9) {
skl_setup_wm_latency(dev_priv);
--
2.22.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 1/2] drm/i915: Move SAGV block time to dev_priv
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ä
> Cc: Stanislav Lisovskiy
> Cc: Lucas De Marchi
> Signed-off-by: James Ausmus
> ---
>
> 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
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
