Re: [PATCH 13/19] drm/i915: Skip some bw_state readout on pre-icl

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

On Tue, Mar 04, 2025 at 02:20:51PM +0000, Govindapillai, Vinod wrote:
> On Tue, 2025-02-18 at 23:19 +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We only compute bw_state->data_rate and bw_state->num_active_planes
> > on icl+. Do the same during readout so that we don't leave random
> > junk inside the state.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_bw.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> > index 30e6f33d6f30..2a669b8535e0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > @@ -809,13 +809,16 @@ static int intel_bw_crtc_min_cdclk(const struct intel_crtc_state
> > *crtc_state)
> >  static void intel_bw_crtc_update(struct intel_bw_state *bw_state,
> >  				 const struct intel_crtc_state *crtc_state)
> >  {
> > +	struct intel_display *display = to_intel_display(crtc_state);
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >  	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> >  
> > -	bw_state->data_rate[crtc->pipe] =
> > -		intel_bw_crtc_data_rate(crtc_state);
> > -	bw_state->num_active_planes[crtc->pipe] =
> > -		intel_bw_crtc_num_active_planes(crtc_state);
> > +	if (DISPLAY_VER(display) >= 11) {
> > +		bw_state->data_rate[crtc->pipe] =
> > +			intel_bw_crtc_data_rate(crtc_state);
> > +		bw_state->num_active_planes[crtc->pipe] =
> > +			intel_bw_crtc_num_active_planes(crtc_state);
> > +	}
> >  	bw_state->force_check_qgv = true;
> 
> This force_check_qgv can be within the display >= 11 as well.

Hmm, yeah looks that way. Though I'll be removing force_check_qgv
later anyway so doesn't matter all that much.

> 
> Also does the below debug message need a tweak too to explain why those will be 0 in display < 11?
> Or call this intel_bw_crtc_update() only for display >= 11 ?

I do add some other stuff there later, but it would perhaps be
cleaner to add it directly to the caller instead.

These debug messages probably should be replaced with some kind of
more sensible state dump support instead. But that's a bigger topic
so not relevant right now.

> 
> With those sorted out,
> 
> Reviewed-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> 
> 
> >  
> >  	drm_dbg_kms(&i915->drm, "pipe %c data rate %u num active planes %u\n",
> 

-- 
Ville Syrjälä
Intel