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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
