On Fri, Jul 20, 2018 at 02:02:34PM +0100, 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.
Doing this kind of global thing from the plane hooks seems a bit
strange. How about just doing this directly from commit_tail()
etc.?
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
