Re: [PATCH 10/19] drm/i915: Extract intel_cdclk_update_hw_state()

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

On Tue, Mar 04, 2025 at 02:04:02PM +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>
> > 
> > Hoist the cdclk stuff into a separate function from
> > intel_modeset_readout_hw_state() so that the details
> > are better hidden inside intel_cdclk.c.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.c    | 29 ++++++++++++++++++-
> >  drivers/gpu/drm/i915/display/intel_cdclk.h    |  2 +-
> >  .../drm/i915/display/intel_modeset_setup.c    | 16 ++--------
> >  3 files changed, 31 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index 4b7058e65588..947833a96ab7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -2788,7 +2788,7 @@ static int intel_planes_min_cdclk(const struct intel_crtc_state *crtc_state)
> >  	return min_cdclk;
> >  }
> >  
> > -int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> > +static int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> >  {
> >  	int min_cdclk;
> >  
> > @@ -3340,6 +3340,33 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
> >  	return 0;
> >  }
> >  
> > +void intel_cdclk_update_hw_state(struct intel_display *display)
> > +{
> > +	struct intel_cdclk_state *cdclk_state =
> > +		to_intel_cdclk_state(display->cdclk.obj.state);
> > +	struct intel_crtc *crtc;
> > +
> > +	cdclk_state->active_pipes = 0;
> 
> As active_pipes are already calculated in intel_modeset_readout_hw_state, wonder will it be useful
> if we pass active_pipes as a parameter to this function and use it above? Same applies to bwstate-
> >active_pipes as well in couple of patches later.

We want to make things more independent, not dependent. So everyone 
should handle their own stuff as much as reasonably possible.

> 
> Anyway, 
> 
> Reviewed-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> 
> > +
> > +	for_each_intel_crtc(display->drm, crtc) {
> > +		const struct intel_crtc_state *crtc_state =
> > +			to_intel_crtc_state(crtc->base.state);
> > +		enum pipe pipe = crtc->pipe;
> > +		int min_cdclk = 0;
> > +
> > +		if (crtc_state->hw.active) {
> > +			cdclk_state->active_pipes |= BIT(pipe);
> > +
> > +			min_cdclk = intel_crtc_compute_min_cdclk(crtc_state);
> > +			if (drm_WARN_ON(display->drm, min_cdclk < 0))
> > +				min_cdclk = 0;
> > +		}
> > +
> > +		cdclk_state->min_cdclk[pipe] = min_cdclk;
> > +		cdclk_state->min_voltage_level[pipe] = crtc_state->min_voltage_level;
> > +	}
> > +}
> > +
> >  void intel_cdclk_crtc_disable_noatomic(struct intel_crtc *crtc)
> >  {
> >  	struct intel_display *display = to_intel_display(crtc);
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h
> > b/drivers/gpu/drm/i915/display/intel_cdclk.h
> > index 689e12e2196b..a1cefd455d92 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> > @@ -59,7 +59,6 @@ struct intel_cdclk_state {
> >  	bool disable_pipes;
> >  };
> >  
> > -int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state);
> >  void intel_cdclk_init_hw(struct intel_display *display);
> >  void intel_cdclk_uninit_hw(struct intel_display *display);
> >  void intel_init_cdclk_hooks(struct intel_display *display);
> > @@ -84,6 +83,7 @@ int intel_cdclk_atomic_check(struct intel_atomic_state *state,
> >  int intel_cdclk_state_set_joined_mbus(struct intel_atomic_state *state, bool joined_mbus);
> >  struct intel_cdclk_state *
> >  intel_atomic_get_cdclk_state(struct intel_atomic_state *state);
> > +void intel_cdclk_update_hw_state(struct intel_display *display);
> >  void intel_cdclk_crtc_disable_noatomic(struct intel_crtc *crtc);
> >  
> >  #define to_intel_cdclk_state(global_state) \
> > diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> > b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> > index e9b0533526f6..1cfa03bd3224 100644
> > --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> > +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> > @@ -693,8 +693,6 @@ static void readout_plane_state(struct drm_i915_private *i915)
> >  static void intel_modeset_readout_hw_state(struct drm_i915_private *i915)
> >  {
> >  	struct intel_display *display = &i915->display;
> > -	struct intel_cdclk_state *cdclk_state =
> > -		to_intel_cdclk_state(i915->display.cdclk.obj.state);
> >  	struct intel_dbuf_state *dbuf_state =
> >  		to_intel_dbuf_state(i915->display.dbuf.obj.state);
> >  	struct intel_pmdemand_state *pmdemand_state =
> > @@ -730,7 +728,6 @@ static void intel_modeset_readout_hw_state(struct drm_i915_private *i915)
> >  			    str_enabled_disabled(crtc_state->hw.active));
> >  	}
> >  
> > -	cdclk_state->active_pipes = active_pipes;
> >  	dbuf_state->active_pipes = active_pipes;
> >  
> >  	readout_plane_state(i915);
> > @@ -833,7 +830,6 @@ static void intel_modeset_readout_hw_state(struct drm_i915_private *i915)
> >  		struct intel_crtc_state *crtc_state =
> >  			to_intel_crtc_state(crtc->base.state);
> >  		struct intel_plane *plane;
> > -		int min_cdclk = 0;
> >  
> >  		if (crtc_state->hw.active) {
> >  			/*
> > @@ -882,22 +878,14 @@ static void intel_modeset_readout_hw_state(struct drm_i915_private *i915)
> >  				    crtc_state->min_cdclk[plane->id]);
> >  		}
> >  
> > -		if (crtc_state->hw.active) {
> > -			min_cdclk = intel_crtc_compute_min_cdclk(crtc_state);
> > -			if (drm_WARN_ON(&i915->drm, min_cdclk < 0))
> > -				min_cdclk = 0;
> > -		}
> > -
> > -		cdclk_state->min_cdclk[crtc->pipe] = min_cdclk;
> > -		cdclk_state->min_voltage_level[crtc->pipe] =
> > -			crtc_state->min_voltage_level;
> > -
> >  		intel_pmdemand_update_port_clock(display, pmdemand_state, pipe,
> >  						 crtc_state->port_clock);
> >  
> >  		intel_bw_crtc_update(bw_state, crtc_state);
> >  	}
> >  
> > +	intel_cdclk_update_hw_state(display);
> > +
> >  	intel_pmdemand_init_pmdemand_params(display, pmdemand_state);
> >  }
> >  
> 

-- 
Ville Syrjälä
Intel