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

[Chris Wilson]

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.

> > +     /* 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 ;)

> > +     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.

> > +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.
-Chris