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@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
>
> > +
> > + 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
