[Intel-gfx] [RESEND] drm/i915: Interactive RPS mode

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-07-20 14:29:52)
> 
> On 20/07/2018 14:02, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-07-20 13:49:09)
> >>
> >> On 12/07/2018 18:38, Chris Wilson wrote:
> >>> +     if (rps->interactive)
> >>> +             new_power = HIGH_POWER;
> >>> +     rps_set_power(dev_priv, new_power);
> >>> +     mutex_unlock(&rps->power_lock);
> >>   >
> >>   >      rps->last_adj = 0;
> >>
> >> This block should go up to the beginning of the function since when
> >> rps->interactive all preceding calculations are for nothing. And can
> >> immediately return then.
> > 
> > We have to lock around rps_set_power, so you're not going to avoid
> > taking the lock here, so I don't think it makes any difference.
> > Certainly neater than locking everything.
> 
> Not to avoid the lock but to avoid all the trouble of calculating 
> new_power to override it all if rps->interactive is set. Just looks a 
> bit weird. I suspect read of rps->interactive doesn't even need to be 
> under the lock so maybe:
> 
> if (rps->interactive)
>         new_power = HIGH_POWER;
> } else {
>         switch (...)
> }

There is a race there, so you do need to explain why we wouldn't care.
(There is a reasonable argument about it being periodic and we don't care
about the first vs interactive.) I thought it came out quite neat as the
atomic override.

> >>> +{
> >>> +     struct intel_rps *rps = &dev_priv->gt_pm.rps;
> >>> +
> >>> +     if (INTEL_GEN(dev_priv) < 6)
> >>> +             return;
> >>> +
> >>> +     mutex_lock(&rps->power_lock);
> >>> +     if (state) {
> >>> +             if (!rps->interactive++ && READ_ONCE(dev_priv->gt.awake))
> >>> +                     rps_set_power(dev_priv, HIGH_POWER);
> >>> +     } else {
> >>> +             GEM_BUG_ON(!rps->interactive);
> >>> +             rps->interactive--;
> >>
> >> Hm maybe protect this in production code so some missed code flows in
> >> the atomic paths do not cause underflow and so permanent interactive mode.
> > 
> > Are you suggesting testing is inadequate? ;) Underflow here isn't a big
> > deal, it just locks in the HIGH_POWER (faster sampling, bias towards
> > upclocking). Or worse not allow HIGH_POWER, status quo returned.
> 
> Testing for this probably is inadequate, no? Would need to be looking at 
> the new debugfs flag from many test cases. And in real world probably 
> quite difficult to debug too.
> 
> Whether or not it would be too much to add a DRM_WARN for this.. I am 
> leaning towards saying to have it, since it is about two systems 
> communicating together so it would be easier to notice a broken 
> contract. But I don't insist on it.

Just checking underflow is not going to catch many problems. If we can
identify some points where we know what the value should be... I wonder
if we can assert it is zero at runtime/system suspend.
-Chris