Re: [PATCH v2 05/14] drm/i915: Avoid triggering unwanted cdclk changes due to dbuf bandwidth changes

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

On Thu, Mar 27, 2025 at 10:00:19AM +0200, Jani Nikula wrote:
> On Wed, 26 Mar 2025, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Currently intel_bw_calc_min_cdclk() always adds the bw_state
> > to the atomic state. Not only does it result in potentially
> > redundant work later, it's also currently causing unwanted cdclk
> > changes during driver load.
> 
> Can you elaborate how we currently end up changing cdclk even if the
> dbuf bw isn't changing? Different rules wrt to GOP?

Older GOP versions always use the max cdclk. IIRC newer ones
might be using a more optimal value.

> 
> Anyway, the change make sense no matter what,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> 
> >
> > Check if the dbuf bw is actually changing before we decide to
> > pull in the bw state.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_bw.c | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> > index 67d088da1f38..19b516084fac 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > @@ -1294,7 +1294,8 @@ int intel_bw_calc_min_cdclk(struct intel_atomic_state *state,
> >  	struct intel_bw_state *new_bw_state = NULL;
> >  	const struct intel_bw_state *old_bw_state = NULL;
> >  	const struct intel_cdclk_state *cdclk_state;
> > -	const struct intel_crtc_state *crtc_state;
> > +	const struct intel_crtc_state *old_crtc_state;
> > +	const struct intel_crtc_state *new_crtc_state;
> >  	int old_min_cdclk, new_min_cdclk;
> >  	struct intel_crtc *crtc;
> >  	int i;
> > @@ -1302,15 +1303,23 @@ int intel_bw_calc_min_cdclk(struct intel_atomic_state *state,
> >  	if (DISPLAY_VER(display) < 9)
> >  		return 0;
> >  
> > -	for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
> > +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > +					    new_crtc_state, i) {
> > +		struct intel_dbuf_bw old_dbuf_bw, new_dbuf_bw;
> > +
> > +		skl_crtc_calc_dbuf_bw(&old_dbuf_bw, old_crtc_state);
> > +		skl_crtc_calc_dbuf_bw(&new_dbuf_bw, new_crtc_state);
> > +
> > +		if (!intel_dbuf_bw_changed(display, &old_dbuf_bw, &new_dbuf_bw))
> > +			continue;
> > +
> >  		new_bw_state = intel_atomic_get_bw_state(state);
> >  		if (IS_ERR(new_bw_state))
> >  			return PTR_ERR(new_bw_state);
> >  
> >  		old_bw_state = intel_atomic_get_old_bw_state(state);
> >  
> > -		skl_crtc_calc_dbuf_bw(&new_bw_state->dbuf_bw[crtc->pipe],
> > -				      crtc_state);
> > +		new_bw_state->dbuf_bw[crtc->pipe] = new_dbuf_bw;
> >  	}
> >  
> >  	if (!old_bw_state)
> 
> -- 
> Jani Nikula, Intel

-- 
Ville Syrjälä
Intel