Re: [Intel-gfx] [PATCH 2/6] drm/i915/gt: Move rps.enabled/active to flags

2020-04-27 Thread Chris Wilson
Quoting Andi Shyti (2020-04-27 00:15:56)
> Hi Chris,
> 
> >   intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
> >   if (IS_CHERRYVIEW(i915))
> > - rps->enabled = chv_rps_enable(rps);
> > + enabled = chv_rps_enable(rps);
> >   else if (IS_VALLEYVIEW(i915))
> > - rps->enabled = vlv_rps_enable(rps);
> > + enabled = vlv_rps_enable(rps);
> >   else if (INTEL_GEN(i915) >= 9)
> > - rps->enabled = gen9_rps_enable(rps);
> > + enabled = gen9_rps_enable(rps);
> >   else if (INTEL_GEN(i915) >= 8)
> > - rps->enabled = gen8_rps_enable(rps);
> > + enabled = gen8_rps_enable(rps);
> >   else if (INTEL_GEN(i915) >= 6)
> > - rps->enabled = gen6_rps_enable(rps);
> > + enabled = gen6_rps_enable(rps);
> >   else if (IS_IRONLAKE_M(i915))
> > - rps->enabled = gen5_rps_enable(rps);
> > + enabled = gen5_rps_enable(rps);
> >   intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL);
> > - if (!rps->enabled)
> > + if (!enabled || rps->max_freq <= rps->min_freq)
> 
> isn't this a bit out of context? I don't think the above
> functions have any effect on max_freq and min freq.

The question is whether or not we want to enable dynamic reclocking if
the range is 0. The answer to that, is definitely no -- just look at all
the selftests that do not work (because the frequency cannot be changed)!

An alternative would be to

if (rps->max_freq <= rpq->min_freq)
/* leave disabled, no room for dynamic reclocking */
else if ...
else
MISSING_CASE(INTEL_GEN(i915));
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH 2/6] drm/i915/gt: Move rps.enabled/active to flags

2020-04-26 Thread Andi Shyti
Hi Chris,

>   intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
>   if (IS_CHERRYVIEW(i915))
> - rps->enabled = chv_rps_enable(rps);
> + enabled = chv_rps_enable(rps);
>   else if (IS_VALLEYVIEW(i915))
> - rps->enabled = vlv_rps_enable(rps);
> + enabled = vlv_rps_enable(rps);
>   else if (INTEL_GEN(i915) >= 9)
> - rps->enabled = gen9_rps_enable(rps);
> + enabled = gen9_rps_enable(rps);
>   else if (INTEL_GEN(i915) >= 8)
> - rps->enabled = gen8_rps_enable(rps);
> + enabled = gen8_rps_enable(rps);
>   else if (INTEL_GEN(i915) >= 6)
> - rps->enabled = gen6_rps_enable(rps);
> + enabled = gen6_rps_enable(rps);
>   else if (IS_IRONLAKE_M(i915))
> - rps->enabled = gen5_rps_enable(rps);
> + enabled = gen5_rps_enable(rps);
>   intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL);
> - if (!rps->enabled)
> + if (!enabled || rps->max_freq <= rps->min_freq)

isn't this a bit out of context? I don't think the above
functions have any effect on max_freq and min freq.

just if (!enable) should do.

Andi

>   return;
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


[Intel-gfx] [PATCH 2/6] drm/i915/gt: Move rps.enabled/active to flags

2020-04-25 Thread Chris Wilson
Pull the boolean intel_rps.enabled and intel_rps.active into a single
flags field, in preparation for more.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/gt/debugfs_gt_pm.c   |  5 +--
 drivers/gpu/drm/i915/gt/intel_rps.c   | 37 +--
 drivers/gpu/drm/i915/gt/intel_rps.h   | 30 ++
 drivers/gpu/drm/i915/gt/intel_rps_types.h |  8 +++--
 drivers/gpu/drm/i915/gt/selftest_rps.c| 20 ++--
 drivers/gpu/drm/i915/i915_debugfs.c   |  5 +--
 6 files changed, 73 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/debugfs_gt_pm.c 
b/drivers/gpu/drm/i915/gt/debugfs_gt_pm.c
index 3d3ef62ed89f..3fd33cd1a400 100644
--- a/drivers/gpu/drm/i915/gt/debugfs_gt_pm.c
+++ b/drivers/gpu/drm/i915/gt/debugfs_gt_pm.c
@@ -556,7 +556,8 @@ static int rps_boost_show(struct seq_file *m, void *data)
struct drm_i915_private *i915 = gt->i915;
struct intel_rps *rps = >->rps;
 
-   seq_printf(m, "RPS enabled? %d\n", rps->enabled);
+   seq_printf(m, "RPS enabled? %s\n", yesno(intel_rps_is_enabled(rps)));
+   seq_printf(m, "RPS active? %s\n", yesno(intel_rps_is_active(rps)));
seq_printf(m, "GPU busy? %s\n", yesno(gt->awake));
seq_printf(m, "Boosts outstanding? %d\n",
   atomic_read(&rps->num_waiters));
@@ -576,7 +577,7 @@ static int rps_boost_show(struct seq_file *m, void *data)
 
seq_printf(m, "Wait boosts: %d\n", atomic_read(&rps->boosts));
 
-   if (INTEL_GEN(i915) >= 6 && rps->enabled && gt->awake) {
+   if (INTEL_GEN(i915) >= 6 && intel_rps_is_active(rps)) {
struct intel_uncore *uncore = gt->uncore;
u32 rpup, rpupei;
u32 rpdown, rpdownei;
diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c 
b/drivers/gpu/drm/i915/gt/intel_rps.c
index 2ce006e58b4a..05410d19dbc0 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.c
+++ b/drivers/gpu/drm/i915/gt/intel_rps.c
@@ -648,7 +648,7 @@ void intel_rps_mark_interactive(struct intel_rps *rps, bool 
interactive)
 
mutex_lock(&rps->power.mutex);
if (interactive) {
-   if (!rps->power.interactive++ && READ_ONCE(rps->active))
+   if (!rps->power.interactive++ && intel_rps_is_active(rps))
rps_set_power(rps, HIGH_POWER);
} else {
GEM_BUG_ON(!rps->power.interactive);
@@ -721,7 +721,7 @@ static int rps_set(struct intel_rps *rps, u8 val, bool 
update)
 
 void intel_rps_unpark(struct intel_rps *rps)
 {
-   if (!rps->enabled)
+   if (!intel_rps_is_enabled(rps))
return;
 
GT_TRACE(rps_to_gt(rps), "unpark:%x\n", rps->cur_freq);
@@ -732,8 +732,7 @@ void intel_rps_unpark(struct intel_rps *rps)
 */
mutex_lock(&rps->lock);
 
-   WRITE_ONCE(rps->active, true);
-
+   intel_rps_set_active(rps);
intel_rps_set(rps,
  clamp(rps->cur_freq,
rps->min_freq_softlimit,
@@ -754,13 +753,12 @@ void intel_rps_park(struct intel_rps *rps)
 {
struct drm_i915_private *i915 = rps_to_i915(rps);
 
-   if (!rps->enabled)
+   if (!intel_rps_clear_active(rps))
return;
 
if (INTEL_GEN(i915) >= 6)
rps_disable_interrupts(rps);
 
-   WRITE_ONCE(rps->active, false);
if (rps->last_freq <= rps->idle_freq)
return;
 
@@ -802,7 +800,7 @@ void intel_rps_boost(struct i915_request *rq)
struct intel_rps *rps = &READ_ONCE(rq->engine)->gt->rps;
unsigned long flags;
 
-   if (i915_request_signaled(rq) || !READ_ONCE(rps->active))
+   if (i915_request_signaled(rq) || !intel_rps_is_active(rps))
return;
 
/* Serializes with i915_request_retire() */
@@ -831,7 +829,7 @@ int intel_rps_set(struct intel_rps *rps, u8 val)
GEM_BUG_ON(val > rps->max_freq);
GEM_BUG_ON(val < rps->min_freq);
 
-   if (rps->active) {
+   if (intel_rps_is_active(rps)) {
err = rps_set(rps, val, true);
if (err)
return err;
@@ -1219,6 +1217,7 @@ void intel_rps_enable(struct intel_rps *rps)
 {
struct drm_i915_private *i915 = rps_to_i915(rps);
struct intel_uncore *uncore = rps_to_uncore(rps);
+   bool enabled = false;
 
if (!HAS_RPS(i915))
return;
@@ -1227,19 +1226,19 @@ void intel_rps_enable(struct intel_rps *rps)
 
intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
if (IS_CHERRYVIEW(i915))
-   rps->enabled = chv_rps_enable(rps);
+   enabled = chv_rps_enable(rps);
else if (IS_VALLEYVIEW(i915))
-   rps->enabled = vlv_rps_enable(rps);
+   enabled = vlv_rps_enable(rps);
else if (INTEL_GEN(i915) >= 9)
-   rps->enabled = gen9_rps_enable(rps);
+   enabled = gen9_rps_enable(rps);
else if (INTEL_GEN(i915) >= 8)
-   rps->enabled = gen8_rps_enable(