On 20/07/2018 14:59, Chris Wilson wrote:
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.
Putting it all under the lock works for me then. But okay, if you so
like the override idea, which I wouldn't call neat, but unusual, it's
not the end of the world.
+{
+ 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.
True, it only catches the imbalance in one direction quickly. If suspend
idea works go with that, but what's so bad about some log messages?
Assuming leak towards the overflow direction on each flip it could be
reached in ~18 hours which is realistic to get bug reports of.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
