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

[Tvrtko Ursulin]

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:
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 7998e70a3174..5809366ff9f0 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -13104,6 +13104,19 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>>>                add_rps_boost_after_vblank(new_state->crtc, new_state->fence);
>>>        }
>>>    
>>> +     /*
>>> +      * We declare pageflips to be interactive and so merit a small bias
>>> +      * towards upclocking to deliver the frame on time. By only changing
>>> +      * the RPS thresholds to sample more regularly and aim for higher
>>> +      * clocks we can hopefully deliver low power workloads (like kodi)
>>> +      * that are not quite steady state without resorting to forcing
>>> +      * maximum clocks following a vblank miss (see do_rps_boost()).
>>> +      */
>>> +     if (!intel_state->rps_interactive) {
>>> +             intel_rps_set_interactive(dev_priv, true);
>>> +             intel_state->rps_interactive = true;
>>> +     }
>>> +
>>>        return 0;
>>>    }
>>>    
>>> @@ -13120,8 +13133,15 @@ void
>>>    intel_cleanup_plane_fb(struct drm_plane *plane,
>>>                       struct drm_plane_state *old_state)
>>>    {
>>> +     struct intel_atomic_state *intel_state =
>>> +             to_intel_atomic_state(old_state->state);
>>>        struct drm_i915_private *dev_priv = to_i915(plane->dev);
>>>    
>>> +     if (intel_state->rps_interactive) {
>>> +             intel_rps_set_interactive(dev_priv, false);
>>> +             intel_state->rps_interactive = false;
>>> +     }
>>
>> Why is the rps_intearctive flag needed in plane state? I wanted to
>> assume prepare & cleanup hooks are fully symmetric, but this flags makes
>> me unsure. A reviewer with more display knowledge will be needed here I
>> think.
> 
> It's so that when we call intel_cleanup_plane_fb() on something that
> wasn't first prepared, we don't decrement the counter. There's a little
> bit of asymmetry at the start where we inherit the plane state.
> 
>>> +
>>>        /* Should only be called after a successful intel_prepare_plane_fb()! */
>>>        mutex_lock(&dev_priv->drm.struct_mutex);
>>>        intel_plane_unpin_fb(to_intel_plane_state(old_state));
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index 61e715ddd0d5..544812488821 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -482,6 +482,8 @@ struct intel_atomic_state {
>>>         */
>>>        bool skip_intermediate_wm;
>>>    
>>> +     bool rps_interactive;
>>
>> Should the name at this level be something more tied to the subsystem
>> and not implying wider relationships? Like page flip pending? fb_prepared?
> 
> But we are in the plane state so anything plane/flip is implied. I think
> saying whether or not we've called out to rps is a reasonable name for
> the state.

Okay fair enough. Maybe just call it rps_interactive_requested?

>>> +     /* Max/min bins are special */
>>> +     if (val <= rps->min_freq_softlimit)
>>> +             new_power = LOW_POWER;
>>> +     if (val >= rps->max_freq_softlimit)
>>> +             new_power = HIGH_POWER;
>>> +
>>> +     mutex_lock(&rps->power_lock);
>>
>> Is it worth avoiding the atomic here when GEN < 6?
> 
> We don't get here when not !RPS. You mean GEN < 5 ;)

No, okay, just incomplete picture based on locking in the other helper. 
Ignore.
> 
>>> +     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 (...)
}

mutex_lock
...
mutex_unlock


> 
>>> +void intel_rps_set_interactive(struct drm_i915_private *dev_priv, bool state)
>>
>> s/state/interactive/ for more self-documenting function body?
>>
>> And not s/dev_priv/i915/ ?!? :)
>>
>>> +{
>>> +     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.

Regards,

Tvrtko