[Intel-gfx] [PATCH v3 2/5] drm/i915: Force recalculate min_cdclk if planes config changed

[Lisovskiy, Stanislav]

On Mon, Mar 30, 2020 at 07:18:57PM +0300, Ville Syrjälä wrote:
> On Mon, Mar 30, 2020 at 03:23:51PM +0300, Stanislav Lisovskiy wrote:
> > In Gen11+ whenever we might exceed DBuf bandwidth we might need to
> > recalculate CDCLK which DBuf bandwidth is scaled with.
> > Total Dbuf bw used might change based on particular plane needs.
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 17d83f37f49f..9fd32d61ebfe 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -14623,7 +14623,7 @@ static bool active_planes_affects_min_cdclk(struct drm_i915_private *dev_priv)
> >  	/* See {hsw,vlv,ivb}_plane_ratio() */
> >  	return IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv) ||
> >  		IS_CHERRYVIEW(dev_priv) || IS_VALLEYVIEW(dev_priv) ||
> > -		IS_IVYBRIDGE(dev_priv);
> > +		IS_IVYBRIDGE(dev_priv) || (INTEL_GEN(dev_priv) >= 11);
> >  }
> >  
> >  static int intel_atomic_check_planes(struct intel_atomic_state *state,
> > @@ -14669,7 +14669,13 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state,
> >  		old_active_planes = old_crtc_state->active_planes & ~BIT(PLANE_CURSOR);
> >  		new_active_planes = new_crtc_state->active_planes & ~BIT(PLANE_CURSOR);
> >  
> > -		if (hweight8(old_active_planes) == hweight8(new_active_planes))
> > +		/*
> > +		 * Not only the number of planes, but if the plane configuration had
> > +		 * changed might already mean we need to recompute min CDCLK,
> > +		 * because different planes might consume different amount of Dbuf bandwidth
> > +		 * according to formula: Bw per plane = Pixel rate * bpp * pipe/plane scale factor
> > +		 */
> 
> The set of of active planes doesn't dictate the bandwidth since it
> doesn't consider most of the parameters you listed above. Ie. doesn't
> seem to be the right place for this stuff.
> 
> The decision to bump the cdclk should really come from the dbuf code
> not from some totally distinct piece of code. But to get this right
> I have a feeling we'll need the dbuf state and totally decouple cdclk
> from any_ms.

My idea was that if active plane configuration had changed - it means that we need
to recalculate bandwidth used by those. Once we now the bandwidth used per slice/per pipe
stored in bw_state - so we recalculate only those in state and that should be fine.
if recalculated bandwidth results in a different CDCLK - then we need to change it.

Or do you mean we need to recalculate bandwidth constantly as part of DBuf state?
I kind of don't like recalculating it everytime, so filtering the case when it will
obviously be the same seems to a good idea, may be this check is not correct but I would 
prefer to recalc used bw only if the planes had changed.

Also I'm fine with tracking it in DBuf state, we first need to land DBuf state patches
(need to look at those again), but meanwhile we could store it in a bw_state.

> 
> > +		if (old_active_planes == new_active_planes)
> >  			continue;
> >  
> >  		ret = intel_crtc_add_planes_to_state(state, crtc, new_active_planes);
> > -- 
> > 2.24.1.485.gad05a3d8e5
> 
> -- 
> Ville Syrjälä
> Intel