Re: [Intel-gfx] [PATCH] drm/i915: Recalculate CDCLK if plane scaling ratio changes

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Wed, Jan 12, 2022 at 03:50:05PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 11, 2022 at 06:08:12PM +0200, Stanislav Lisovskiy wrote:
> > Currently we only recalculate CDCLK if active plane mask changes
> > or if we do a full modeset, however according to BSpec
> > required Dbuf bandwidth calculations also depend on pipe/plane
> > scaling ratio, which means that CDCLK must be recalculated
> > everytime plane scaling ratio changes,
> 
> Already handled by the plane min_cdclk stuff.

Problem is that plane min_cdclk will only be called for those
which are added to the state.
In intel_atomic_check_planes we call intel_crtc_add_planes_to_state
only if active_planes_affects_min_cdclk is true and active_planes
mask got changed.
However if we got one of planes scaling ratio changed, we need to
recalculate CDCLK once again and make sure we have all the active
planes in state for that. Don't we need all active planes 
in state to calculate it properly? 

> 
> > because it affects
> > display buffer bandwidth requirements.
> 
> Yes, the dbuf bw code is borked. I have old patches on the list that 
> started to fix up all the data rate related stuff, but IIRC I didn't
> finish it because I ran out of time at the time. I think I have a
> branch that has a bit more but I'll need to check how far along I
> actually got in fixing it all...

Yeah, it kind of tries to take into account that multiple BSpec
requirements into account, however as I remember at the moment when
this was committed we were still not even sure, we interpret BSpec
properly here. By the way we are still missing cumulative bpp W/A here 
in upstream.

Stan

> 
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 63 +++++++++++++++++++-
> >  1 file changed, 60 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index bf7ce684dd8e..2c616348e993 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -7499,13 +7499,65 @@ static int intel_bigjoiner_add_affected_planes(struct intel_atomic_state *state)
> >  	return 0;
> >  }
> >  
> > +static bool scaling_affects_cdclk(struct intel_plane_state *old_plane_state,
> > +				  struct intel_plane_state *new_plane_state)
> > +{
> > +	int old_src_w = drm_rect_width(&old_plane_state->uapi.src) >> 16;
> > +	int old_src_h = drm_rect_height(&old_plane_state->uapi.src) >> 16;
> > +	int old_dst_w = drm_rect_width(&old_plane_state->uapi.dst);
> > +	int old_dst_h = drm_rect_height(&old_plane_state->uapi.dst);
> > +	int new_src_w = drm_rect_width(&new_plane_state->uapi.src) >> 16;
> > +	int new_src_h = drm_rect_height(&new_plane_state->uapi.src) >> 16;
> > +	int new_dst_w = drm_rect_width(&new_plane_state->uapi.dst);
> > +	int new_dst_h = drm_rect_height(&new_plane_state->uapi.dst);
> > +	int old_hscale_ratio, new_hscale_ratio;
> > +	int old_vscale_ratio, new_vscale_ratio;
> > +
> > +	if (needs_scaling(old_plane_state) != needs_scaling(new_plane_state))
> > +		return true;
> > +
> > +	if (!old_dst_w || !old_dst_h)
> > +		return true;
> > +
> > +	DRM_DEBUG_KMS("old_dst_w %d old_dst_h %d\n", old_dst_w, old_dst_h);
> > +
> > +	old_hscale_ratio = DIV_ROUND_UP(old_src_w, old_dst_w);
> > +	old_vscale_ratio = DIV_ROUND_UP(old_src_h, old_dst_h);
> > +
> > +	if (!new_dst_w || !new_dst_h)
> > +		return true;
> > +
> > +	DRM_DEBUG_KMS("new_dst_w %d new_dst_h %d\n", new_dst_w, new_dst_h);
> > +
> > +	new_hscale_ratio = DIV_ROUND_UP(new_src_w, new_dst_w);
> > +	new_vscale_ratio = DIV_ROUND_UP(new_src_h, new_dst_h);
> > +
> > +	DRM_DEBUG_KMS("new_hscale_ratio %d new_vscale_ratio %d "
> > +		      "old_hscale_ratio %d old_vscale_ratio %d\n",
> > +		      new_hscale_ratio, new_vscale_ratio,
> > +		      old_hscale_ratio, old_vscale_ratio);
> > +
> > +	if ((old_hscale_ratio != new_hscale_ratio) ||
> > +	    (old_vscale_ratio != new_vscale_ratio)) {
> > +		DRM_DEBUG_KMS("Scaling ratios changed from %dx%d"
> > +			      " to %dx%d - need cdclk recalc\n",
> > +			      old_hscale_ratio, old_vscale_ratio,
> > +			      new_hscale_ratio, new_vscale_ratio);
> > +		return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  static int intel_atomic_check_planes(struct intel_atomic_state *state)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> >  	struct intel_crtc_state *old_crtc_state, *new_crtc_state;
> >  	struct intel_plane_state *plane_state;
> > +	struct intel_plane_state *old_plane_state;
> >  	struct intel_plane *plane;
> >  	struct intel_crtc *crtc;
> > +	bool need_cdclk_calc = false;
> >  	int i, ret;
> >  
> >  	ret = icl_add_linked_planes(state);
> > @@ -7516,7 +7568,7 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state)
> >  	if (ret)
> >  		return ret;
> >  
> > -	for_each_new_intel_plane_in_state(state, plane, plane_state, i) {
> > +	for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state, plane_state, i) {
> >  		ret = intel_plane_atomic_check(state, plane);
> >  		if (ret) {
> >  			drm_dbg_atomic(&dev_priv->drm,
> > @@ -7524,6 +7576,9 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state)
> >  				       plane->base.base.id, plane->base.name);
> >  			return ret;
> >  		}
> > +
> > +		if (scaling_affects_cdclk(old_plane_state, plane_state))
> > +			need_cdclk_calc = true;
> >  	}
> >  
> >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > @@ -7539,18 +7594,20 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state)
> >  		 * the planes' minimum cdclk calculation. Add such planes
> >  		 * to the state before we compute the minimum cdclk.
> >  		 */
> > -		if (!active_planes_affects_min_cdclk(dev_priv))
> > +		if (!active_planes_affects_min_cdclk(dev_priv) && !need_cdclk_calc)
> >  			continue;
> >  
> >  		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))
> > +		if ((hweight8(old_active_planes) == hweight8(new_active_planes)) && !need_cdclk_calc)
> >  			continue;
> >  
> >  		ret = intel_crtc_add_planes_to_state(state, crtc, new_active_planes);
> >  		if (ret)
> >  			return ret;
> > +
> > +
> >  	}
> >  
> >  	return 0;
> > -- 
> > 2.24.1.485.gad05a3d8e5
> 
> -- 
> Ville Syrjälä
> Intel