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ä <[email protected]> > > > > 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ä <[email protected]> > > --- > > 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 <[email protected]> > > > + > > + 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
