| Message ID | 20180801092431.29316-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915: Drop stray clearing of rps->last_adj | expand |
Chris Wilson <chris@chris-wilson.co.uk> writes: > We used to reset last_adj to 0 on crossing a power domain boundary, to > slow down our rate of change. However, commit 60548c554be2 ("drm/i915: > Interactive RPS mode") accidentally caused it to be reset on every > frequency update, nerfing the fast response granted by the slow start > algorithm. > > Fixes: 60548c554be2 ("drm/i915: Interactive RPS mode") > Testcase: igt/pm_rps/mix-max-config-loaded > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 2531eb75bdce..f90a3c7f1c40 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -6371,7 +6371,6 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val) > new_power = HIGH_POWER; > rps_set_power(dev_priv, new_power); > mutex_unlock(&rps->power.mutex); > - rps->last_adj = 0; To follow the old logic, you should zero it in rps_set_power ? -Mika > } > > void intel_rps_mark_interactive(struct drm_i915_private *i915, bool interactive) > -- > 2.18.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Mika Kuoppala (2018-08-01 10:38:55) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > We used to reset last_adj to 0 on crossing a power domain boundary, to > > slow down our rate of change. However, commit 60548c554be2 ("drm/i915: > > Interactive RPS mode") accidentally caused it to be reset on every > > frequency update, nerfing the fast response granted by the slow start > > algorithm. > > > > Fixes: 60548c554be2 ("drm/i915: Interactive RPS mode") > > Testcase: igt/pm_rps/mix-max-config-loaded > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 2531eb75bdce..f90a3c7f1c40 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -6371,7 +6371,6 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val) > > new_power = HIGH_POWER; > > rps_set_power(dev_priv, new_power); > > mutex_unlock(&rps->power.mutex); > > - rps->last_adj = 0; > > To follow the old logic, you should zero it in rps_set_power ? I was opting for a new logic, no more nerf on domain crossing. The domain crossing is just a rate change, so shouldn't really impact on the slow start algorithm; what I really want is to detect a null EI and use that as the backoff. That would help with the overshoot, but the effect of dampening is likely to be firmly in the noise. -Chris
Quoting Mika Kuoppala (2018-08-01 10:38:55) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > We used to reset last_adj to 0 on crossing a power domain boundary, to > > slow down our rate of change. However, commit 60548c554be2 ("drm/i915: > > Interactive RPS mode") accidentally caused it to be reset on every > > frequency update, nerfing the fast response granted by the slow start > > algorithm. > > > > Fixes: 60548c554be2 ("drm/i915: Interactive RPS mode") > > Testcase: igt/pm_rps/mix-max-config-loaded > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 2531eb75bdce..f90a3c7f1c40 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -6371,7 +6371,6 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val) > > new_power = HIGH_POWER; > > rps_set_power(dev_priv, new_power); > > mutex_unlock(&rps->power.mutex); > > - rps->last_adj = 0; > > To follow the old logic, you should zero it in rps_set_power ? Also note I didn't do that originally because we now have an alternative path that would then modify last_adj outside of the rps worker, conflicting with the previous logic as well. -Chris
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 2531eb75bdce..f90a3c7f1c40 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -6371,7 +6371,6 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val) new_power = HIGH_POWER; rps_set_power(dev_priv, new_power); mutex_unlock(&rps->power.mutex); - rps->last_adj = 0; } void intel_rps_mark_interactive(struct drm_i915_private *i915, bool interactive)
We used to reset last_adj to 0 on crossing a power domain boundary, to slow down our rate of change. However, commit 60548c554be2 ("drm/i915: Interactive RPS mode") accidentally caused it to be reset on every frequency update, nerfing the fast response granted by the slow start algorithm. Fixes: 60548c554be2 ("drm/i915: Interactive RPS mode") Testcase: igt/pm_rps/mix-max-config-loaded Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 1 - 1 file changed, 1 deletion(-)