Re: [RESEND] drm/i915: Interactive RPS mode

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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