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