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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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