[Intel-gfx] [PATCH 30/36] drm/i915: Refactor frequency bounds computation

[Chris Wilson]

Quoting Sagar Arun Kamble (2018-03-17 15:10:08)
> 
> 
> On 3/14/2018 3:07 PM, Chris Wilson wrote:
> > When choosing the initial frequency in intel_gt_pm_busy() we also need
> > to calculate the current min/max bounds. As this calculation is going to
> > become more complex with the intersection of several different limits,
> > refactor it to a common function. The alternative wold be to feed the
> typo
> > initial reclocking through the RPS worker, but the latency in this case
> > is undesirable.
> >
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/intel_gt_pm.c | 58 +++++++++++++++-----------------------
> >   1 file changed, 22 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_gt_pm.c b/drivers/gpu/drm/i915/intel_gt_pm.c
> > index 8630c30a7e48..f8e029b4a8a7 100644
> > --- a/drivers/gpu/drm/i915/intel_gt_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_gt_pm.c
> > @@ -382,15 +382,25 @@ static int __intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
> >               return 0;
> >   }
> >   
> > -static int intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
> > +static int adjust_rps(struct drm_i915_private *dev_priv, int freq, int adj)
> >   {
> >       struct intel_rps *rps = &dev_priv->gt_pm.rps;
> > +     int min, max, val;
> Can we move to u8 type in this patch itself

No. Check the math, that presumes int clamping, so just use native
register types until after the clamping.

> >       int err;
> >   
> >       lockdep_assert_held(&rps->lock);
> >       GEM_BUG_ON(!rps->active);
> > -     GEM_BUG_ON(val > rps->max_freq);
> > -     GEM_BUG_ON(val < rps->min_freq);
> > +
> > +     min = rps->min_freq_softlimit;
> > +     max = rps->max_freq_softlimit;
> > +     if (atomic_read(&rps->num_waiters) && max < rps->boost_freq)
> > +             max = rps->boost_freq;
> > +
> > +     GEM_BUG_ON(min < rps->min_freq);
> > +     GEM_BUG_ON(max > rps->max_freq);
> > +     GEM_BUG_ON(max < min);
> > +
> > +     val = clamp(freq + adj, min, max);
> >   
> >       err = __intel_set_rps(dev_priv, val);
> >       if (err)
> > @@ -401,6 +411,8 @@ static int intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
> >               rps->cur_freq = val;
> >       }
> >   
> > +     rps->last_adj = val == freq ? adj : 0;
> > +
> I think this should be:
> rps->last_adj = val == freq ? 0 : adj;

If we make the adjustment, store the new adj; if we overrule the
selection, then cancel the adj so that we don't keep on accumulating adj
upon hitting the bounds.

Hmm, should have been val == freq + adj.
-Chris