drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup

Commit Message

Navare, Manasi March 15, 2022, 11:38 p.m. UTC

This patch abstracts pieces of hsw_crtc_enable corresponding to different
Bspec enable sequence steps into separate functions.
This helps to call them in a specific order for bigjoiner master/slave
in a cleaner fashion.

Cc: Ville Syrjälä <[e-mail]>
Cc: Animesh Manna <[e-mail]>
Signed-off-by: Manasi Navare <[e-mail]>
---
 drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
 1 file changed, 66 insertions(+), 59 deletions(-)

Comments

Jani Nikula March 16, 2022, 7:48 a.m. UTC | #1

On Tue, 15 Mar 2022, Manasi Navare <[e-mail]> wrote:
> This patch abstracts pieces of hsw_crtc_enable corresponding to different
> Bspec enable sequence steps into separate functions.
> This helps to call them in a specific order for bigjoiner master/slave
> in a cleaner fashion.

So does this contain only refactoring or functional changes also? One or
the other at a time, please, not both.

Also looks like hsw_crtc_* have accumulated just way too many checks for
platforms instead of having a clean break at e.g. display 9 and/or 11.

Adding references to "enable sequence step 8" is not helpful because
it's platform specific. (Yeah, I know there are existing references like
this, but they're also suspect.)


BR,
Jani.


>
> Cc: Ville Syrjälä <[e-mail]>
> Cc: Animesh Manna <[e-mail]>
> Signed-off-by: Manasi Navare <[e-mail]>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
>  1 file changed, 66 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index eb49973621f0..d8e6466c9fa0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
>  	intel_de_write(dev_priv, reg, val);
>  }
>  
> -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
> -					 const struct intel_crtc_state *crtc_state)
> -{
> -	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> -
> -	/*
> -	 * Enable sequence steps 1-7 on bigjoiner master
> -	 */
> -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> -		intel_encoders_pre_pll_enable(state, master_crtc);
> -
> -	if (crtc_state->shared_dpll)
> -		intel_enable_shared_dpll(crtc_state);
> -
> -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> -		intel_encoders_pre_enable(state, master_crtc);
> -}
> -
>  static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
>  	hsw_set_transconf(crtc_state);
>  }
>  
> -static void hsw_crtc_enable(struct intel_atomic_state *state,
> -			    struct intel_crtc *crtc)
> +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> +				    const struct intel_crtc_state *crtc_state)
>  {
> -	const struct intel_crtc_state *new_crtc_state =
> -		intel_atomic_get_new_crtc_state(state, crtc);
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> -	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> -	bool psl_clkgate_wa;
> -
> -	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> -		return;
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  
> -	if (!new_crtc_state->bigjoiner_pipes) {
> -		intel_encoders_pre_pll_enable(state, crtc);
> +	/*
> +	 * Enable sequence steps 1 - 7 on all pipes
> +	 */
> +	intel_encoders_pre_pll_enable(state, crtc);
> +	if (crtc_state->shared_dpll)
> +		intel_enable_shared_dpll(crtc_state);
>  
> -		if (new_crtc_state->shared_dpll)
> -			intel_enable_shared_dpll(new_crtc_state);
> +	intel_encoders_pre_enable(state, crtc);
> +}
>  
> -		intel_encoders_pre_enable(state, crtc);
> -	} else {
> -		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> -	}
> +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> +				     const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> +	bool psl_clkgate_wa;
>  
> -	intel_dsc_enable(new_crtc_state);
> +	/*
> +	 * Enable sequence step 8
> +	 */
> +	intel_dsc_enable(crtc_state);
>  
>  	if (DISPLAY_VER(dev_priv) >= 13)
> -		intel_uncompressed_joiner_enable(new_crtc_state);
> +		intel_uncompressed_joiner_enable(crtc_state);
>  
> -	intel_set_pipe_src_size(new_crtc_state);
> +	intel_set_pipe_src_size(crtc_state);
>  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> -		bdw_set_pipemisc(new_crtc_state);
> +		bdw_set_pipemisc(crtc_state);
>  
> -	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> +	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
>  	    !transcoder_is_dsi(cpu_transcoder))
> -		hsw_configure_cpu_transcoder(new_crtc_state);
> +		hsw_configure_cpu_transcoder(crtc_state);
>  
>  	crtc->active = true;
>  
>  	/* Display WA #1180: WaDisableScalarClockGating: glk */
>  	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
> -		new_crtc_state->pch_pfit.enabled;
> +		crtc_state->pch_pfit.enabled;
>  	if (psl_clkgate_wa)
>  		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
>  
>  	if (DISPLAY_VER(dev_priv) >= 9)
> -		skl_pfit_enable(new_crtc_state);
> +		skl_pfit_enable(crtc_state);
>  	else
> -		ilk_pfit_enable(new_crtc_state);
> +		ilk_pfit_enable(crtc_state);
>  
>  	/*
>  	 * On ILK+ LUT must be loaded before the pipe is running but with
>  	 * clocks enabled
>  	 */
> -	intel_color_load_luts(new_crtc_state);
> -	intel_color_commit(new_crtc_state);
> +	intel_color_load_luts(crtc_state);
> +	intel_color_commit(crtc_state);
>  	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
>  	if (DISPLAY_VER(dev_priv) < 9)
> -		intel_disable_primary_plane(new_crtc_state);
> +		intel_disable_primary_plane(crtc_state);
>  
> -	hsw_set_linetime_wm(new_crtc_state);
> +	hsw_set_linetime_wm(crtc_state);
>  
>  	if (DISPLAY_VER(dev_priv) >= 11)
> -		icl_set_pipe_chicken(new_crtc_state);
> +		icl_set_pipe_chicken(crtc_state);
>  
>  	intel_initial_watermarks(state, crtc);
>  
> @@ -1984,8 +1969,8 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>  		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
>  	}
>  
> -	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> -		intel_crtc_vblank_on(new_crtc_state);
> +	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> +		intel_crtc_vblank_on(crtc_state);
>  
>  	intel_encoders_enable(state, crtc);
>  
> @@ -1996,7 +1981,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>  
>  	/* If we change the relative order between pipe/planes enabling, we need
>  	 * to change the workaround. */
> -	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
> +	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
>  	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
>  		struct intel_crtc *wa_crtc;
>  
> @@ -2007,6 +1992,29 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>  	}
>  }
>  
> +static void hsw_crtc_enable(struct intel_atomic_state *state,
> +			    struct intel_crtc *crtc)
> +{
> +	const struct intel_crtc_state *new_crtc_state =
> +		intel_atomic_get_new_crtc_state(state, crtc);
> +	struct intel_crtc *slave_crtc;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +
> +	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> +		return;
> +
> +	hsw_crtc_pre_pll_enable(state, new_crtc_state);
> +
> +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
> +					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
> +		struct intel_crtc_state *slave_crtc_state =
> +			intel_atomic_get_new_crtc_state(state, slave_crtc);
> +
> +		hsw_crtc_post_pll_enable(state, slave_crtc_state);
> +	}
> +	hsw_crtc_post_pll_enable(state, new_crtc_state);
> +}
> +
>  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> @@ -8122,11 +8130,11 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
>  
>  	intel_crtc_update_active_timings(new_crtc_state);
>  
> -	dev_priv->display->crtc_enable(state, crtc);
> -
>  	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
>  		return;
>  
> +	dev_priv->display->crtc_enable(state, crtc);
> +
>  	intel_drrs_enable(new_crtc_state);
>  
>  	/* vblanks work again, re-enable pipe CRC. */
> @@ -8360,8 +8368,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>  			continue;
>  
>  		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> -		    is_trans_port_sync_master(new_crtc_state) ||
> -		    intel_crtc_is_bigjoiner_master(new_crtc_state))
> +		    is_trans_port_sync_master(new_crtc_state))
>  			continue;
>  
>  		modeset_pipes &= ~BIT(pipe);
> @@ -8371,7 +8378,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>  
>  	/*
>  	 * Then we enable all remaining pipes that depend on other
> -	 * pipes: MST slaves and port sync masters, big joiner master
> +	 * pipes: MST slaves and port sync masters
>  	 */
>  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
>  		enum pipe pipe = crtc->pipe;

Navare, Manasi March 16, 2022, 5:10 p.m. UTC | #2

On Wed, Mar 16, 2022 at 09:48:17AM +0200, Jani Nikula wrote:
> On Tue, 15 Mar 2022, Manasi Navare <[e-mail]> wrote:
> > This patch abstracts pieces of hsw_crtc_enable corresponding to different
> > Bspec enable sequence steps into separate functions.
> > This helps to call them in a specific order for bigjoiner master/slave
> > in a cleaner fashion.
> 
> So does this contain only refactoring or functional changes also? One or
> the other at a time, please, not both.

No this is only refactor, no functional changes here

> 
> Also looks like hsw_crtc_* have accumulated just way too many checks for
> platforms instead of having a clean break at e.g. display 9 and/or 11.

These checks were already part of hsw_crtc_enable() function that I have just moved to a separate
function. How do you recommend removing these checks?

> 
> Adding references to "enable sequence step 8" is not helpful because
> it's platform specific. (Yeah, I know there are existing references like
> this, but they're also suspect.)

Yes agree, may be I will add the comment for what actually the step 7/8 does?

Manasi

> 
> 
> BR,
> Jani.
> 
> 
> >
> > Cc: Ville Syrjälä <[e-mail]>
> > Cc: Animesh Manna <[e-mail]>
> > Signed-off-by: Manasi Navare <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
> >  1 file changed, 66 insertions(+), 59 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index eb49973621f0..d8e6466c9fa0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
> >  	intel_de_write(dev_priv, reg, val);
> >  }
> >  
> > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
> > -					 const struct intel_crtc_state *crtc_state)
> > -{
> > -	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> > -
> > -	/*
> > -	 * Enable sequence steps 1-7 on bigjoiner master
> > -	 */
> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > -		intel_encoders_pre_pll_enable(state, master_crtc);
> > -
> > -	if (crtc_state->shared_dpll)
> > -		intel_enable_shared_dpll(crtc_state);
> > -
> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > -		intel_encoders_pre_enable(state, master_crtc);
> > -}
> > -
> >  static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
> >  {
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
> >  	hsw_set_transconf(crtc_state);
> >  }
> >  
> > -static void hsw_crtc_enable(struct intel_atomic_state *state,
> > -			    struct intel_crtc *crtc)
> > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> > +				    const struct intel_crtc_state *crtc_state)
> >  {
> > -	const struct intel_crtc_state *new_crtc_state =
> > -		intel_atomic_get_new_crtc_state(state, crtc);
> > -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > -	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > -	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> > -	bool psl_clkgate_wa;
> > -
> > -	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > -		return;
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >  
> > -	if (!new_crtc_state->bigjoiner_pipes) {
> > -		intel_encoders_pre_pll_enable(state, crtc);
> > +	/*
> > +	 * Enable sequence steps 1 - 7 on all pipes
> > +	 */
> > +	intel_encoders_pre_pll_enable(state, crtc);
> > +	if (crtc_state->shared_dpll)
> > +		intel_enable_shared_dpll(crtc_state);
> >  
> > -		if (new_crtc_state->shared_dpll)
> > -			intel_enable_shared_dpll(new_crtc_state);
> > +	intel_encoders_pre_enable(state, crtc);
> > +}
> >  
> > -		intel_encoders_pre_enable(state, crtc);
> > -	} else {
> > -		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> > -	}
> > +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> > +				     const struct intel_crtc_state *crtc_state)
> > +{
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > +	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > +	bool psl_clkgate_wa;
> >  
> > -	intel_dsc_enable(new_crtc_state);
> > +	/*
> > +	 * Enable sequence step 8
> > +	 */
> > +	intel_dsc_enable(crtc_state);
> >  
> >  	if (DISPLAY_VER(dev_priv) >= 13)
> > -		intel_uncompressed_joiner_enable(new_crtc_state);
> > +		intel_uncompressed_joiner_enable(crtc_state);
> >  
> > -	intel_set_pipe_src_size(new_crtc_state);
> > +	intel_set_pipe_src_size(crtc_state);
> >  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > -		bdw_set_pipemisc(new_crtc_state);
> > +		bdw_set_pipemisc(crtc_state);
> >  
> > -	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> > +	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
> >  	    !transcoder_is_dsi(cpu_transcoder))
> > -		hsw_configure_cpu_transcoder(new_crtc_state);
> > +		hsw_configure_cpu_transcoder(crtc_state);
> >  
> >  	crtc->active = true;
> >  
> >  	/* Display WA #1180: WaDisableScalarClockGating: glk */
> >  	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
> > -		new_crtc_state->pch_pfit.enabled;
> > +		crtc_state->pch_pfit.enabled;
> >  	if (psl_clkgate_wa)
> >  		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
> >  
> >  	if (DISPLAY_VER(dev_priv) >= 9)
> > -		skl_pfit_enable(new_crtc_state);
> > +		skl_pfit_enable(crtc_state);
> >  	else
> > -		ilk_pfit_enable(new_crtc_state);
> > +		ilk_pfit_enable(crtc_state);
> >  
> >  	/*
> >  	 * On ILK+ LUT must be loaded before the pipe is running but with
> >  	 * clocks enabled
> >  	 */
> > -	intel_color_load_luts(new_crtc_state);
> > -	intel_color_commit(new_crtc_state);
> > +	intel_color_load_luts(crtc_state);
> > +	intel_color_commit(crtc_state);
> >  	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
> >  	if (DISPLAY_VER(dev_priv) < 9)
> > -		intel_disable_primary_plane(new_crtc_state);
> > +		intel_disable_primary_plane(crtc_state);
> >  
> > -	hsw_set_linetime_wm(new_crtc_state);
> > +	hsw_set_linetime_wm(crtc_state);
> >  
> >  	if (DISPLAY_VER(dev_priv) >= 11)
> > -		icl_set_pipe_chicken(new_crtc_state);
> > +		icl_set_pipe_chicken(crtc_state);
> >  
> >  	intel_initial_watermarks(state, crtc);
> >  
> > @@ -1984,8 +1969,8 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> >  		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
> >  	}
> >  
> > -	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> > -		intel_crtc_vblank_on(new_crtc_state);
> > +	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > +		intel_crtc_vblank_on(crtc_state);
> >  
> >  	intel_encoders_enable(state, crtc);
> >  
> > @@ -1996,7 +1981,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> >  
> >  	/* If we change the relative order between pipe/planes enabling, we need
> >  	 * to change the workaround. */
> > -	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
> > +	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
> >  	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
> >  		struct intel_crtc *wa_crtc;
> >  
> > @@ -2007,6 +1992,29 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> >  	}
> >  }
> >  
> > +static void hsw_crtc_enable(struct intel_atomic_state *state,
> > +			    struct intel_crtc *crtc)
> > +{
> > +	const struct intel_crtc_state *new_crtc_state =
> > +		intel_atomic_get_new_crtc_state(state, crtc);
> > +	struct intel_crtc *slave_crtc;
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > +
> > +	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > +		return;
> > +
> > +	hsw_crtc_pre_pll_enable(state, new_crtc_state);
> > +
> > +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
> > +					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
> > +		struct intel_crtc_state *slave_crtc_state =
> > +			intel_atomic_get_new_crtc_state(state, slave_crtc);
> > +
> > +		hsw_crtc_post_pll_enable(state, slave_crtc_state);
> > +	}
> > +	hsw_crtc_post_pll_enable(state, new_crtc_state);
> > +}
> > +
> >  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
> >  {
> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> > @@ -8122,11 +8130,11 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
> >  
> >  	intel_crtc_update_active_timings(new_crtc_state);
> >  
> > -	dev_priv->display->crtc_enable(state, crtc);
> > -
> >  	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> >  		return;
> >  
> > +	dev_priv->display->crtc_enable(state, crtc);
> > +
> >  	intel_drrs_enable(new_crtc_state);
> >  
> >  	/* vblanks work again, re-enable pipe CRC. */
> > @@ -8360,8 +8368,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> >  			continue;
> >  
> >  		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > -		    is_trans_port_sync_master(new_crtc_state) ||
> > -		    intel_crtc_is_bigjoiner_master(new_crtc_state))
> > +		    is_trans_port_sync_master(new_crtc_state))
> >  			continue;
> >  
> >  		modeset_pipes &= ~BIT(pipe);
> > @@ -8371,7 +8378,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> >  
> >  	/*
> >  	 * Then we enable all remaining pipes that depend on other
> > -	 * pipes: MST slaves and port sync masters, big joiner master
> > +	 * pipes: MST slaves and port sync masters
> >  	 */
> >  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> >  		enum pipe pipe = crtc->pipe;
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

Jani Nikula March 17, 2022, 3:35 p.m. UTC | #3

On Wed, 16 Mar 2022, "Navare, Manasi" <[e-mail]> wrote:
> On Wed, Mar 16, 2022 at 09:48:17AM +0200, Jani Nikula wrote:
>> On Tue, 15 Mar 2022, Manasi Navare <[e-mail]> wrote:
>> > This patch abstracts pieces of hsw_crtc_enable corresponding to different
>> > Bspec enable sequence steps into separate functions.
>> > This helps to call them in a specific order for bigjoiner master/slave
>> > in a cleaner fashion.
>> 
>> So does this contain only refactoring or functional changes also? One or
>> the other at a time, please, not both.
>
> No this is only refactor, no functional changes here
>
>> 
>> Also looks like hsw_crtc_* have accumulated just way too many checks for
>> platforms instead of having a clean break at e.g. display 9 and/or 11.
>
> These checks were already part of hsw_crtc_enable() function that I have just moved to a separate
> function. How do you recommend removing these checks?

We use hsw_crtc_enable for all DDI platforms and later. We do have the
difference between skl_display_funcs and ddi_display_funcs, but they
both point to hsw_crtc_enable. Maybe it's time for them to have their
own functions that don't have to take so many platform differences into
account.

BR,
Jani.




>
>> 
>> Adding references to "enable sequence step 8" is not helpful because
>> it's platform specific. (Yeah, I know there are existing references like
>> this, but they're also suspect.)
>
> Yes agree, may be I will add the comment for what actually the step 7/8 does?
>
> Manasi
>
>> 
>> 
>> BR,
>> Jani.
>> 
>> 
>> >
>> > Cc: Ville Syrjälä <[e-mail]>
>> > Cc: Animesh Manna <[e-mail]>
>> > Signed-off-by: Manasi Navare <[e-mail]>
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
>> >  1 file changed, 66 insertions(+), 59 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> > index eb49973621f0..d8e6466c9fa0 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
>> >  	intel_de_write(dev_priv, reg, val);
>> >  }
>> >  
>> > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
>> > -					 const struct intel_crtc_state *crtc_state)
>> > -{
>> > -	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
>> > -
>> > -	/*
>> > -	 * Enable sequence steps 1-7 on bigjoiner master
>> > -	 */
>> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
>> > -		intel_encoders_pre_pll_enable(state, master_crtc);
>> > -
>> > -	if (crtc_state->shared_dpll)
>> > -		intel_enable_shared_dpll(crtc_state);
>> > -
>> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
>> > -		intel_encoders_pre_enable(state, master_crtc);
>> > -}
>> > -
>> >  static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
>> >  {
>> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
>> >  	hsw_set_transconf(crtc_state);
>> >  }
>> >  
>> > -static void hsw_crtc_enable(struct intel_atomic_state *state,
>> > -			    struct intel_crtc *crtc)
>> > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
>> > +				    const struct intel_crtc_state *crtc_state)
>> >  {
>> > -	const struct intel_crtc_state *new_crtc_state =
>> > -		intel_atomic_get_new_crtc_state(state, crtc);
>> > -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> > -	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
>> > -	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
>> > -	bool psl_clkgate_wa;
>> > -
>> > -	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
>> > -		return;
>> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> >  
>> > -	if (!new_crtc_state->bigjoiner_pipes) {
>> > -		intel_encoders_pre_pll_enable(state, crtc);
>> > +	/*
>> > +	 * Enable sequence steps 1 - 7 on all pipes
>> > +	 */
>> > +	intel_encoders_pre_pll_enable(state, crtc);
>> > +	if (crtc_state->shared_dpll)
>> > +		intel_enable_shared_dpll(crtc_state);
>> >  
>> > -		if (new_crtc_state->shared_dpll)
>> > -			intel_enable_shared_dpll(new_crtc_state);
>> > +	intel_encoders_pre_enable(state, crtc);
>> > +}
>> >  
>> > -		intel_encoders_pre_enable(state, crtc);
>> > -	} else {
>> > -		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
>> > -	}
>> > +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
>> > +				     const struct intel_crtc_state *crtc_state)
>> > +{
>> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> > +	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
>> > +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>> > +	bool psl_clkgate_wa;
>> >  
>> > -	intel_dsc_enable(new_crtc_state);
>> > +	/*
>> > +	 * Enable sequence step 8
>> > +	 */
>> > +	intel_dsc_enable(crtc_state);
>> >  
>> >  	if (DISPLAY_VER(dev_priv) >= 13)
>> > -		intel_uncompressed_joiner_enable(new_crtc_state);
>> > +		intel_uncompressed_joiner_enable(crtc_state);
>> >  
>> > -	intel_set_pipe_src_size(new_crtc_state);
>> > +	intel_set_pipe_src_size(crtc_state);
>> >  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
>> > -		bdw_set_pipemisc(new_crtc_state);
>> > +		bdw_set_pipemisc(crtc_state);
>> >  
>> > -	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
>> > +	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
>> >  	    !transcoder_is_dsi(cpu_transcoder))
>> > -		hsw_configure_cpu_transcoder(new_crtc_state);
>> > +		hsw_configure_cpu_transcoder(crtc_state);
>> >  
>> >  	crtc->active = true;
>> >  
>> >  	/* Display WA #1180: WaDisableScalarClockGating: glk */
>> >  	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
>> > -		new_crtc_state->pch_pfit.enabled;
>> > +		crtc_state->pch_pfit.enabled;
>> >  	if (psl_clkgate_wa)
>> >  		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
>> >  
>> >  	if (DISPLAY_VER(dev_priv) >= 9)
>> > -		skl_pfit_enable(new_crtc_state);
>> > +		skl_pfit_enable(crtc_state);
>> >  	else
>> > -		ilk_pfit_enable(new_crtc_state);
>> > +		ilk_pfit_enable(crtc_state);
>> >  
>> >  	/*
>> >  	 * On ILK+ LUT must be loaded before the pipe is running but with
>> >  	 * clocks enabled
>> >  	 */
>> > -	intel_color_load_luts(new_crtc_state);
>> > -	intel_color_commit(new_crtc_state);
>> > +	intel_color_load_luts(crtc_state);
>> > +	intel_color_commit(crtc_state);
>> >  	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
>> >  	if (DISPLAY_VER(dev_priv) < 9)
>> > -		intel_disable_primary_plane(new_crtc_state);
>> > +		intel_disable_primary_plane(crtc_state);
>> >  
>> > -	hsw_set_linetime_wm(new_crtc_state);
>> > +	hsw_set_linetime_wm(crtc_state);
>> >  
>> >  	if (DISPLAY_VER(dev_priv) >= 11)
>> > -		icl_set_pipe_chicken(new_crtc_state);
>> > +		icl_set_pipe_chicken(crtc_state);
>> >  
>> >  	intel_initial_watermarks(state, crtc);
>> >  
>> > @@ -1984,8 +1969,8 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>> >  		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
>> >  	}
>> >  
>> > -	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
>> > -		intel_crtc_vblank_on(new_crtc_state);
>> > +	if (intel_crtc_is_bigjoiner_slave(crtc_state))
>> > +		intel_crtc_vblank_on(crtc_state);
>> >  
>> >  	intel_encoders_enable(state, crtc);
>> >  
>> > @@ -1996,7 +1981,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>> >  
>> >  	/* If we change the relative order between pipe/planes enabling, we need
>> >  	 * to change the workaround. */
>> > -	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
>> > +	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
>> >  	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
>> >  		struct intel_crtc *wa_crtc;
>> >  
>> > @@ -2007,6 +1992,29 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>> >  	}
>> >  }
>> >  
>> > +static void hsw_crtc_enable(struct intel_atomic_state *state,
>> > +			    struct intel_crtc *crtc)
>> > +{
>> > +	const struct intel_crtc_state *new_crtc_state =
>> > +		intel_atomic_get_new_crtc_state(state, crtc);
>> > +	struct intel_crtc *slave_crtc;
>> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> > +
>> > +	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
>> > +		return;
>> > +
>> > +	hsw_crtc_pre_pll_enable(state, new_crtc_state);
>> > +
>> > +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
>> > +					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
>> > +		struct intel_crtc_state *slave_crtc_state =
>> > +			intel_atomic_get_new_crtc_state(state, slave_crtc);
>> > +
>> > +		hsw_crtc_post_pll_enable(state, slave_crtc_state);
>> > +	}
>> > +	hsw_crtc_post_pll_enable(state, new_crtc_state);
>> > +}
>> > +
>> >  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
>> >  {
>> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
>> > @@ -8122,11 +8130,11 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
>> >  
>> >  	intel_crtc_update_active_timings(new_crtc_state);
>> >  
>> > -	dev_priv->display->crtc_enable(state, crtc);
>> > -
>> >  	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
>> >  		return;
>> >  
>> > +	dev_priv->display->crtc_enable(state, crtc);
>> > +
>> >  	intel_drrs_enable(new_crtc_state);
>> >  
>> >  	/* vblanks work again, re-enable pipe CRC. */
>> > @@ -8360,8 +8368,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>> >  			continue;
>> >  
>> >  		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
>> > -		    is_trans_port_sync_master(new_crtc_state) ||
>> > -		    intel_crtc_is_bigjoiner_master(new_crtc_state))
>> > +		    is_trans_port_sync_master(new_crtc_state))
>> >  			continue;
>> >  
>> >  		modeset_pipes &= ~BIT(pipe);
>> > @@ -8371,7 +8378,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>> >  
>> >  	/*
>> >  	 * Then we enable all remaining pipes that depend on other
>> > -	 * pipes: MST slaves and port sync masters, big joiner master
>> > +	 * pipes: MST slaves and port sync masters
>> >  	 */
>> >  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
>> >  		enum pipe pipe = crtc->pipe;
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center

Navare, Manasi March 17, 2022, 6:28 p.m. UTC | #4

On Thu, Mar 17, 2022 at 05:35:43PM +0200, Jani Nikula wrote:
> On Wed, 16 Mar 2022, "Navare, Manasi" <[e-mail]> wrote:
> > On Wed, Mar 16, 2022 at 09:48:17AM +0200, Jani Nikula wrote:
> >> On Tue, 15 Mar 2022, Manasi Navare <[e-mail]> wrote:
> >> > This patch abstracts pieces of hsw_crtc_enable corresponding to different
> >> > Bspec enable sequence steps into separate functions.
> >> > This helps to call them in a specific order for bigjoiner master/slave
> >> > in a cleaner fashion.
> >> 
> >> So does this contain only refactoring or functional changes also? One or
> >> the other at a time, please, not both.
> >
> > No this is only refactor, no functional changes here
> >
> >> 
> >> Also looks like hsw_crtc_* have accumulated just way too many checks for
> >> platforms instead of having a clean break at e.g. display 9 and/or 11.
> >
> > These checks were already part of hsw_crtc_enable() function that I have just moved to a separate
> > function. How do you recommend removing these checks?
> 
> We use hsw_crtc_enable for all DDI platforms and later. We do have the
> difference between skl_display_funcs and ddi_display_funcs, but they
> both point to hsw_crtc_enable. Maybe it's time for them to have their
> own functions that don't have to take so many platform differences into
> account.

Yes I could perhaps have a separate skl_crtc_enable() function which would then have all parts that apply to DISPLAy >=9
everything else can stay in hsw_crtc_enable

That makes sense?

Manasi

> 
> BR,
> Jani.
> 
> 
> 
> 
> >
> >> 
> >> Adding references to "enable sequence step 8" is not helpful because
> >> it's platform specific. (Yeah, I know there are existing references like
> >> this, but they're also suspect.)
> >
> > Yes agree, may be I will add the comment for what actually the step 7/8 does?
> >
> > Manasi
> >
> >> 
> >> 
> >> BR,
> >> Jani.
> >> 
> >> 
> >> >
> >> > Cc: Ville Syrjälä <[e-mail]>
> >> > Cc: Animesh Manna <[e-mail]>
> >> > Signed-off-by: Manasi Navare <[e-mail]>
> >> > ---
> >> >  drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
> >> >  1 file changed, 66 insertions(+), 59 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> >> > index eb49973621f0..d8e6466c9fa0 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >> > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
> >> >  	intel_de_write(dev_priv, reg, val);
> >> >  }
> >> >  
> >> > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
> >> > -					 const struct intel_crtc_state *crtc_state)
> >> > -{
> >> > -	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> >> > -
> >> > -	/*
> >> > -	 * Enable sequence steps 1-7 on bigjoiner master
> >> > -	 */
> >> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> >> > -		intel_encoders_pre_pll_enable(state, master_crtc);
> >> > -
> >> > -	if (crtc_state->shared_dpll)
> >> > -		intel_enable_shared_dpll(crtc_state);
> >> > -
> >> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> >> > -		intel_encoders_pre_enable(state, master_crtc);
> >> > -}
> >> > -
> >> >  static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
> >> >  {
> >> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >> > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
> >> >  	hsw_set_transconf(crtc_state);
> >> >  }
> >> >  
> >> > -static void hsw_crtc_enable(struct intel_atomic_state *state,
> >> > -			    struct intel_crtc *crtc)
> >> > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> >> > +				    const struct intel_crtc_state *crtc_state)
> >> >  {
> >> > -	const struct intel_crtc_state *new_crtc_state =
> >> > -		intel_atomic_get_new_crtc_state(state, crtc);
> >> > -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >> > -	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> >> > -	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> >> > -	bool psl_clkgate_wa;
> >> > -
> >> > -	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> >> > -		return;
> >> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >> >  
> >> > -	if (!new_crtc_state->bigjoiner_pipes) {
> >> > -		intel_encoders_pre_pll_enable(state, crtc);
> >> > +	/*
> >> > +	 * Enable sequence steps 1 - 7 on all pipes
> >> > +	 */
> >> > +	intel_encoders_pre_pll_enable(state, crtc);
> >> > +	if (crtc_state->shared_dpll)
> >> > +		intel_enable_shared_dpll(crtc_state);
> >> >  
> >> > -		if (new_crtc_state->shared_dpll)
> >> > -			intel_enable_shared_dpll(new_crtc_state);
> >> > +	intel_encoders_pre_enable(state, crtc);
> >> > +}
> >> >  
> >> > -		intel_encoders_pre_enable(state, crtc);
> >> > -	} else {
> >> > -		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> >> > -	}
> >> > +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> >> > +				     const struct intel_crtc_state *crtc_state)
> >> > +{
> >> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >> > +	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> >> > +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> >> > +	bool psl_clkgate_wa;
> >> >  
> >> > -	intel_dsc_enable(new_crtc_state);
> >> > +	/*
> >> > +	 * Enable sequence step 8
> >> > +	 */
> >> > +	intel_dsc_enable(crtc_state);
> >> >  
> >> >  	if (DISPLAY_VER(dev_priv) >= 13)
> >> > -		intel_uncompressed_joiner_enable(new_crtc_state);
> >> > +		intel_uncompressed_joiner_enable(crtc_state);
> >> >  
> >> > -	intel_set_pipe_src_size(new_crtc_state);
> >> > +	intel_set_pipe_src_size(crtc_state);
> >> >  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> >> > -		bdw_set_pipemisc(new_crtc_state);
> >> > +		bdw_set_pipemisc(crtc_state);
> >> >  
> >> > -	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> >> > +	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
> >> >  	    !transcoder_is_dsi(cpu_transcoder))
> >> > -		hsw_configure_cpu_transcoder(new_crtc_state);
> >> > +		hsw_configure_cpu_transcoder(crtc_state);
> >> >  
> >> >  	crtc->active = true;
> >> >  
> >> >  	/* Display WA #1180: WaDisableScalarClockGating: glk */
> >> >  	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
> >> > -		new_crtc_state->pch_pfit.enabled;
> >> > +		crtc_state->pch_pfit.enabled;
> >> >  	if (psl_clkgate_wa)
> >> >  		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
> >> >  
> >> >  	if (DISPLAY_VER(dev_priv) >= 9)
> >> > -		skl_pfit_enable(new_crtc_state);
> >> > +		skl_pfit_enable(crtc_state);
> >> >  	else
> >> > -		ilk_pfit_enable(new_crtc_state);
> >> > +		ilk_pfit_enable(crtc_state);
> >> >  
> >> >  	/*
> >> >  	 * On ILK+ LUT must be loaded before the pipe is running but with
> >> >  	 * clocks enabled
> >> >  	 */
> >> > -	intel_color_load_luts(new_crtc_state);
> >> > -	intel_color_commit(new_crtc_state);
> >> > +	intel_color_load_luts(crtc_state);
> >> > +	intel_color_commit(crtc_state);
> >> >  	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
> >> >  	if (DISPLAY_VER(dev_priv) < 9)
> >> > -		intel_disable_primary_plane(new_crtc_state);
> >> > +		intel_disable_primary_plane(crtc_state);
> >> >  
> >> > -	hsw_set_linetime_wm(new_crtc_state);
> >> > +	hsw_set_linetime_wm(crtc_state);
> >> >  
> >> >  	if (DISPLAY_VER(dev_priv) >= 11)
> >> > -		icl_set_pipe_chicken(new_crtc_state);
> >> > +		icl_set_pipe_chicken(crtc_state);
> >> >  
> >> >  	intel_initial_watermarks(state, crtc);
> >> >  
> >> > @@ -1984,8 +1969,8 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> >> >  		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
> >> >  	}
> >> >  
> >> > -	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> >> > -		intel_crtc_vblank_on(new_crtc_state);
> >> > +	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> >> > +		intel_crtc_vblank_on(crtc_state);
> >> >  
> >> >  	intel_encoders_enable(state, crtc);
> >> >  
> >> > @@ -1996,7 +1981,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> >> >  
> >> >  	/* If we change the relative order between pipe/planes enabling, we need
> >> >  	 * to change the workaround. */
> >> > -	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
> >> > +	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
> >> >  	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
> >> >  		struct intel_crtc *wa_crtc;
> >> >  
> >> > @@ -2007,6 +1992,29 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> >> >  	}
> >> >  }
> >> >  
> >> > +static void hsw_crtc_enable(struct intel_atomic_state *state,
> >> > +			    struct intel_crtc *crtc)
> >> > +{
> >> > +	const struct intel_crtc_state *new_crtc_state =
> >> > +		intel_atomic_get_new_crtc_state(state, crtc);
> >> > +	struct intel_crtc *slave_crtc;
> >> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >> > +
> >> > +	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> >> > +		return;
> >> > +
> >> > +	hsw_crtc_pre_pll_enable(state, new_crtc_state);
> >> > +
> >> > +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
> >> > +					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
> >> > +		struct intel_crtc_state *slave_crtc_state =
> >> > +			intel_atomic_get_new_crtc_state(state, slave_crtc);
> >> > +
> >> > +		hsw_crtc_post_pll_enable(state, slave_crtc_state);
> >> > +	}
> >> > +	hsw_crtc_post_pll_enable(state, new_crtc_state);
> >> > +}
> >> > +
> >> >  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
> >> >  {
> >> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> >> > @@ -8122,11 +8130,11 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
> >> >  
> >> >  	intel_crtc_update_active_timings(new_crtc_state);
> >> >  
> >> > -	dev_priv->display->crtc_enable(state, crtc);
> >> > -
> >> >  	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> >> >  		return;
> >> >  
> >> > +	dev_priv->display->crtc_enable(state, crtc);
> >> > +
> >> >  	intel_drrs_enable(new_crtc_state);
> >> >  
> >> >  	/* vblanks work again, re-enable pipe CRC. */
> >> > @@ -8360,8 +8368,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> >> >  			continue;
> >> >  
> >> >  		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> >> > -		    is_trans_port_sync_master(new_crtc_state) ||
> >> > -		    intel_crtc_is_bigjoiner_master(new_crtc_state))
> >> > +		    is_trans_port_sync_master(new_crtc_state))
> >> >  			continue;
> >> >  
> >> >  		modeset_pipes &= ~BIT(pipe);
> >> > @@ -8371,7 +8378,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> >> >  
> >> >  	/*
> >> >  	 * Then we enable all remaining pipes that depend on other
> >> > -	 * pipes: MST slaves and port sync masters, big joiner master
> >> > +	 * pipes: MST slaves and port sync masters
> >> >  	 */
> >> >  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> >> >  		enum pipe pipe = crtc->pipe;
> >> 
> >> -- 
> >> Jani Nikula, Intel Open Source Graphics Center
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

Navare, Manasi March 17, 2022, 6:31 p.m. UTC | #5

On Thu, Mar 17, 2022 at 11:28:03AM -0700, Navare, Manasi wrote:
> On Thu, Mar 17, 2022 at 05:35:43PM +0200, Jani Nikula wrote:
> > On Wed, 16 Mar 2022, "Navare, Manasi" <[e-mail]> wrote:
> > > On Wed, Mar 16, 2022 at 09:48:17AM +0200, Jani Nikula wrote:
> > >> On Tue, 15 Mar 2022, Manasi Navare <[e-mail]> wrote:
> > >> > This patch abstracts pieces of hsw_crtc_enable corresponding to different
> > >> > Bspec enable sequence steps into separate functions.
> > >> > This helps to call them in a specific order for bigjoiner master/slave
> > >> > in a cleaner fashion.
> > >> 
> > >> So does this contain only refactoring or functional changes also? One or
> > >> the other at a time, please, not both.
> > >
> > > No this is only refactor, no functional changes here
> > >
> > >> 
> > >> Also looks like hsw_crtc_* have accumulated just way too many checks for
> > >> platforms instead of having a clean break at e.g. display 9 and/or 11.
> > >
> > > These checks were already part of hsw_crtc_enable() function that I have just moved to a separate
> > > function. How do you recommend removing these checks?
> > 
> > We use hsw_crtc_enable for all DDI platforms and later. We do have the
> > difference between skl_display_funcs and ddi_display_funcs, but they
> > both point to hsw_crtc_enable. Maybe it's time for them to have their
> > own functions that don't have to take so many platform differences into
> > account.
> 
> Yes I could perhaps have a separate skl_crtc_enable() function which would then have all parts that apply to DISPLAy >=9
> everything else can stay in hsw_crtc_enable
> 
> That makes sense?

Or actually we could have a separate one for >=11 starting ICL, since there are a lot of
checks for >=11. Also bigjoiner support only from ICL, so then this bigjoiner sequence then only becomes part
of icl_crtc_enable()
Which one of the above 2 do you recommend?

Manasi

> 
> Manasi
> 
> > 
> > BR,
> > Jani.
> > 
> > 
> > 
> > 
> > >
> > >> 
> > >> Adding references to "enable sequence step 8" is not helpful because
> > >> it's platform specific. (Yeah, I know there are existing references like
> > >> this, but they're also suspect.)
> > >
> > > Yes agree, may be I will add the comment for what actually the step 7/8 does?
> > >
> > > Manasi
> > >
> > >> 
> > >> 
> > >> BR,
> > >> Jani.
> > >> 
> > >> 
> > >> >
> > >> > Cc: Ville Syrjälä <[e-mail]>
> > >> > Cc: Animesh Manna <[e-mail]>
> > >> > Signed-off-by: Manasi Navare <[e-mail]>
> > >> > ---
> > >> >  drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
> > >> >  1 file changed, 66 insertions(+), 59 deletions(-)
> > >> >
> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > >> > index eb49973621f0..d8e6466c9fa0 100644
> > >> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > >> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > >> > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
> > >> >  	intel_de_write(dev_priv, reg, val);
> > >> >  }
> > >> >  
> > >> > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
> > >> > -					 const struct intel_crtc_state *crtc_state)
> > >> > -{
> > >> > -	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> > >> > -
> > >> > -	/*
> > >> > -	 * Enable sequence steps 1-7 on bigjoiner master
> > >> > -	 */
> > >> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > >> > -		intel_encoders_pre_pll_enable(state, master_crtc);
> > >> > -
> > >> > -	if (crtc_state->shared_dpll)
> > >> > -		intel_enable_shared_dpll(crtc_state);
> > >> > -
> > >> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > >> > -		intel_encoders_pre_enable(state, master_crtc);
> > >> > -}
> > >> > -
> > >> >  static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
> > >> >  {
> > >> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > >> > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
> > >> >  	hsw_set_transconf(crtc_state);
> > >> >  }
> > >> >  
> > >> > -static void hsw_crtc_enable(struct intel_atomic_state *state,
> > >> > -			    struct intel_crtc *crtc)
> > >> > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> > >> > +				    const struct intel_crtc_state *crtc_state)
> > >> >  {
> > >> > -	const struct intel_crtc_state *new_crtc_state =
> > >> > -		intel_atomic_get_new_crtc_state(state, crtc);
> > >> > -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > >> > -	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > >> > -	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> > >> > -	bool psl_clkgate_wa;
> > >> > -
> > >> > -	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > >> > -		return;
> > >> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > >> >  
> > >> > -	if (!new_crtc_state->bigjoiner_pipes) {
> > >> > -		intel_encoders_pre_pll_enable(state, crtc);
> > >> > +	/*
> > >> > +	 * Enable sequence steps 1 - 7 on all pipes
> > >> > +	 */
> > >> > +	intel_encoders_pre_pll_enable(state, crtc);
> > >> > +	if (crtc_state->shared_dpll)
> > >> > +		intel_enable_shared_dpll(crtc_state);
> > >> >  
> > >> > -		if (new_crtc_state->shared_dpll)
> > >> > -			intel_enable_shared_dpll(new_crtc_state);
> > >> > +	intel_encoders_pre_enable(state, crtc);
> > >> > +}
> > >> >  
> > >> > -		intel_encoders_pre_enable(state, crtc);
> > >> > -	} else {
> > >> > -		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> > >> > -	}
> > >> > +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> > >> > +				     const struct intel_crtc_state *crtc_state)
> > >> > +{
> > >> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > >> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > >> > +	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > >> > +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > >> > +	bool psl_clkgate_wa;
> > >> >  
> > >> > -	intel_dsc_enable(new_crtc_state);
> > >> > +	/*
> > >> > +	 * Enable sequence step 8
> > >> > +	 */
> > >> > +	intel_dsc_enable(crtc_state);
> > >> >  
> > >> >  	if (DISPLAY_VER(dev_priv) >= 13)
> > >> > -		intel_uncompressed_joiner_enable(new_crtc_state);
> > >> > +		intel_uncompressed_joiner_enable(crtc_state);
> > >> >  
> > >> > -	intel_set_pipe_src_size(new_crtc_state);
> > >> > +	intel_set_pipe_src_size(crtc_state);
> > >> >  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > >> > -		bdw_set_pipemisc(new_crtc_state);
> > >> > +		bdw_set_pipemisc(crtc_state);
> > >> >  
> > >> > -	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> > >> > +	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
> > >> >  	    !transcoder_is_dsi(cpu_transcoder))
> > >> > -		hsw_configure_cpu_transcoder(new_crtc_state);
> > >> > +		hsw_configure_cpu_transcoder(crtc_state);
> > >> >  
> > >> >  	crtc->active = true;
> > >> >  
> > >> >  	/* Display WA #1180: WaDisableScalarClockGating: glk */
> > >> >  	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
> > >> > -		new_crtc_state->pch_pfit.enabled;
> > >> > +		crtc_state->pch_pfit.enabled;
> > >> >  	if (psl_clkgate_wa)
> > >> >  		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
> > >> >  
> > >> >  	if (DISPLAY_VER(dev_priv) >= 9)
> > >> > -		skl_pfit_enable(new_crtc_state);
> > >> > +		skl_pfit_enable(crtc_state);
> > >> >  	else
> > >> > -		ilk_pfit_enable(new_crtc_state);
> > >> > +		ilk_pfit_enable(crtc_state);
> > >> >  
> > >> >  	/*
> > >> >  	 * On ILK+ LUT must be loaded before the pipe is running but with
> > >> >  	 * clocks enabled
> > >> >  	 */
> > >> > -	intel_color_load_luts(new_crtc_state);
> > >> > -	intel_color_commit(new_crtc_state);
> > >> > +	intel_color_load_luts(crtc_state);
> > >> > +	intel_color_commit(crtc_state);
> > >> >  	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
> > >> >  	if (DISPLAY_VER(dev_priv) < 9)
> > >> > -		intel_disable_primary_plane(new_crtc_state);
> > >> > +		intel_disable_primary_plane(crtc_state);
> > >> >  
> > >> > -	hsw_set_linetime_wm(new_crtc_state);
> > >> > +	hsw_set_linetime_wm(crtc_state);
> > >> >  
> > >> >  	if (DISPLAY_VER(dev_priv) >= 11)
> > >> > -		icl_set_pipe_chicken(new_crtc_state);
> > >> > +		icl_set_pipe_chicken(crtc_state);
> > >> >  
> > >> >  	intel_initial_watermarks(state, crtc);
> > >> >  
> > >> > @@ -1984,8 +1969,8 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > >> >  		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
> > >> >  	}
> > >> >  
> > >> > -	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> > >> > -		intel_crtc_vblank_on(new_crtc_state);
> > >> > +	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > >> > +		intel_crtc_vblank_on(crtc_state);
> > >> >  
> > >> >  	intel_encoders_enable(state, crtc);
> > >> >  
> > >> > @@ -1996,7 +1981,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > >> >  
> > >> >  	/* If we change the relative order between pipe/planes enabling, we need
> > >> >  	 * to change the workaround. */
> > >> > -	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
> > >> > +	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
> > >> >  	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
> > >> >  		struct intel_crtc *wa_crtc;
> > >> >  
> > >> > @@ -2007,6 +1992,29 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > >> >  	}
> > >> >  }
> > >> >  
> > >> > +static void hsw_crtc_enable(struct intel_atomic_state *state,
> > >> > +			    struct intel_crtc *crtc)
> > >> > +{
> > >> > +	const struct intel_crtc_state *new_crtc_state =
> > >> > +		intel_atomic_get_new_crtc_state(state, crtc);
> > >> > +	struct intel_crtc *slave_crtc;
> > >> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > >> > +
> > >> > +	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > >> > +		return;
> > >> > +
> > >> > +	hsw_crtc_pre_pll_enable(state, new_crtc_state);
> > >> > +
> > >> > +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
> > >> > +					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
> > >> > +		struct intel_crtc_state *slave_crtc_state =
> > >> > +			intel_atomic_get_new_crtc_state(state, slave_crtc);
> > >> > +
> > >> > +		hsw_crtc_post_pll_enable(state, slave_crtc_state);
> > >> > +	}
> > >> > +	hsw_crtc_post_pll_enable(state, new_crtc_state);
> > >> > +}
> > >> > +
> > >> >  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
> > >> >  {
> > >> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> > >> > @@ -8122,11 +8130,11 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
> > >> >  
> > >> >  	intel_crtc_update_active_timings(new_crtc_state);
> > >> >  
> > >> > -	dev_priv->display->crtc_enable(state, crtc);
> > >> > -
> > >> >  	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> > >> >  		return;
> > >> >  
> > >> > +	dev_priv->display->crtc_enable(state, crtc);
> > >> > +
> > >> >  	intel_drrs_enable(new_crtc_state);
> > >> >  
> > >> >  	/* vblanks work again, re-enable pipe CRC. */
> > >> > @@ -8360,8 +8368,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > >> >  			continue;
> > >> >  
> > >> >  		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > >> > -		    is_trans_port_sync_master(new_crtc_state) ||
> > >> > -		    intel_crtc_is_bigjoiner_master(new_crtc_state))
> > >> > +		    is_trans_port_sync_master(new_crtc_state))
> > >> >  			continue;
> > >> >  
> > >> >  		modeset_pipes &= ~BIT(pipe);
> > >> > @@ -8371,7 +8378,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > >> >  
> > >> >  	/*
> > >> >  	 * Then we enable all remaining pipes that depend on other
> > >> > -	 * pipes: MST slaves and port sync masters, big joiner master
> > >> > +	 * pipes: MST slaves and port sync masters
> > >> >  	 */
> > >> >  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> > >> >  		enum pipe pipe = crtc->pipe;
> > >> 
> > >> -- 
> > >> Jani Nikula, Intel Open Source Graphics Center
> > 
> > -- 
> > Jani Nikula, Intel Open Source Graphics Center

Ville Syrjälä March 17, 2022, 6:52 p.m. UTC | #6

On Tue, Mar 15, 2022 at 04:38:56PM -0700, Manasi Navare wrote:
> This patch abstracts pieces of hsw_crtc_enable corresponding to different
> Bspec enable sequence steps into separate functions.
> This helps to call them in a specific order for bigjoiner master/slave
> in a cleaner fashion.
> 
> Cc: Ville Syrjälä <[e-mail]>
> Cc: Animesh Manna <[e-mail]>
> Signed-off-by: Manasi Navare <[e-mail]>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
>  1 file changed, 66 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index eb49973621f0..d8e6466c9fa0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
>  	intel_de_write(dev_priv, reg, val);
>  }
>  
> -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
> -					 const struct intel_crtc_state *crtc_state)
> -{
> -	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> -
> -	/*
> -	 * Enable sequence steps 1-7 on bigjoiner master
> -	 */
> -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> -		intel_encoders_pre_pll_enable(state, master_crtc);
> -
> -	if (crtc_state->shared_dpll)
> -		intel_enable_shared_dpll(crtc_state);
> -
> -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> -		intel_encoders_pre_enable(state, master_crtc);
> -}
> -
>  static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
>  	hsw_set_transconf(crtc_state);
>  }
>  
> -static void hsw_crtc_enable(struct intel_atomic_state *state,
> -			    struct intel_crtc *crtc)
> +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> +				    const struct intel_crtc_state *crtc_state)
>  {
> -	const struct intel_crtc_state *new_crtc_state =
> -		intel_atomic_get_new_crtc_state(state, crtc);
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> -	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> -	bool psl_clkgate_wa;
> -
> -	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> -		return;
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  
> -	if (!new_crtc_state->bigjoiner_pipes) {
> -		intel_encoders_pre_pll_enable(state, crtc);
> +	/*
> +	 * Enable sequence steps 1 - 7 on all pipes
> +	 */
> +	intel_encoders_pre_pll_enable(state, crtc);
> +	if (crtc_state->shared_dpll)
> +		intel_enable_shared_dpll(crtc_state);
>  
> -		if (new_crtc_state->shared_dpll)
> -			intel_enable_shared_dpll(new_crtc_state);
> +	intel_encoders_pre_enable(state, crtc);
> +}
>  
> -		intel_encoders_pre_enable(state, crtc);
> -	} else {
> -		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> -	}
> +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> +				     const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> +	bool psl_clkgate_wa;
>  
> -	intel_dsc_enable(new_crtc_state);
> +	/*
> +	 * Enable sequence step 8
> +	 */
> +	intel_dsc_enable(crtc_state);
>  
>  	if (DISPLAY_VER(dev_priv) >= 13)
> -		intel_uncompressed_joiner_enable(new_crtc_state);
> +		intel_uncompressed_joiner_enable(crtc_state);
>  
> -	intel_set_pipe_src_size(new_crtc_state);
> +	intel_set_pipe_src_size(crtc_state);
>  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> -		bdw_set_pipemisc(new_crtc_state);
> +		bdw_set_pipemisc(crtc_state);
>  
> -	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> +	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
>  	    !transcoder_is_dsi(cpu_transcoder))
> -		hsw_configure_cpu_transcoder(new_crtc_state);
> +		hsw_configure_cpu_transcoder(crtc_state);
>  
>  	crtc->active = true;
>  
>  	/* Display WA #1180: WaDisableScalarClockGating: glk */
>  	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
> -		new_crtc_state->pch_pfit.enabled;
> +		crtc_state->pch_pfit.enabled;
>  	if (psl_clkgate_wa)
>  		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
>  
>  	if (DISPLAY_VER(dev_priv) >= 9)
> -		skl_pfit_enable(new_crtc_state);
> +		skl_pfit_enable(crtc_state);
>  	else
> -		ilk_pfit_enable(new_crtc_state);
> +		ilk_pfit_enable(crtc_state);
>  
>  	/*
>  	 * On ILK+ LUT must be loaded before the pipe is running but with
>  	 * clocks enabled
>  	 */
> -	intel_color_load_luts(new_crtc_state);
> -	intel_color_commit(new_crtc_state);
> +	intel_color_load_luts(crtc_state);
> +	intel_color_commit(crtc_state);
>  	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
>  	if (DISPLAY_VER(dev_priv) < 9)
> -		intel_disable_primary_plane(new_crtc_state);
> +		intel_disable_primary_plane(crtc_state);
>  
> -	hsw_set_linetime_wm(new_crtc_state);
> +	hsw_set_linetime_wm(crtc_state);
>  
>  	if (DISPLAY_VER(dev_priv) >= 11)
> -		icl_set_pipe_chicken(new_crtc_state);
> +		icl_set_pipe_chicken(crtc_state);
>  
>  	intel_initial_watermarks(state, crtc);
>  
> @@ -1984,8 +1969,8 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>  		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
>  	}
>  
> -	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> -		intel_crtc_vblank_on(new_crtc_state);
> +	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> +		intel_crtc_vblank_on(crtc_state);
>  
>  	intel_encoders_enable(state, crtc);
>  
> @@ -1996,7 +1981,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>  
>  	/* If we change the relative order between pipe/planes enabling, we need
>  	 * to change the workaround. */
> -	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
> +	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
>  	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
>  		struct intel_crtc *wa_crtc;
>  
> @@ -2007,6 +1992,29 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>  	}
>  }
>  
> +static void hsw_crtc_enable(struct intel_atomic_state *state,
> +			    struct intel_crtc *crtc)
> +{
> +	const struct intel_crtc_state *new_crtc_state =
> +		intel_atomic_get_new_crtc_state(state, crtc);
> +	struct intel_crtc *slave_crtc;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +
> +	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> +		return;
> +
> +	hsw_crtc_pre_pll_enable(state, new_crtc_state);
> +
> +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
> +					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
> +		struct intel_crtc_state *slave_crtc_state =
> +			intel_atomic_get_new_crtc_state(state, slave_crtc);
> +
> +		hsw_crtc_post_pll_enable(state, slave_crtc_state);
> +	}
> +	hsw_crtc_post_pll_enable(state, new_crtc_state);
> +}

I suspect this is far too high level for bigjoiner. Eg. there's a bunch
of things already in the disable sequence that seem to need much more
low level sequencing between the joined pipes. So my gut feeling still
is that we want to continue the per-pipe vs. per-transcoder split and
do the joiner loops in lower level code.

> +
>  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> @@ -8122,11 +8130,11 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
>  
>  	intel_crtc_update_active_timings(new_crtc_state);
>  
> -	dev_priv->display->crtc_enable(state, crtc);
> -
>  	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
>  		return;
>  
> +	dev_priv->display->crtc_enable(state, crtc);
> +
>  	intel_drrs_enable(new_crtc_state);
>  
>  	/* vblanks work again, re-enable pipe CRC. */
> @@ -8360,8 +8368,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>  			continue;
>  
>  		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> -		    is_trans_port_sync_master(new_crtc_state) ||
> -		    intel_crtc_is_bigjoiner_master(new_crtc_state))
> +		    is_trans_port_sync_master(new_crtc_state))
>  			continue;
>  
>  		modeset_pipes &= ~BIT(pipe);
> @@ -8371,7 +8378,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>  
>  	/*
>  	 * Then we enable all remaining pipes that depend on other
> -	 * pipes: MST slaves and port sync masters, big joiner master
> +	 * pipes: MST slaves and port sync masters
>  	 */
>  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
>  		enum pipe pipe = crtc->pipe;
> -- 
> 2.19.1

Navare, Manasi March 17, 2022, 7:05 p.m. UTC | #7

On Thu, Mar 17, 2022 at 08:52:52PM +0200, Ville Syrjälä wrote:
> On Tue, Mar 15, 2022 at 04:38:56PM -0700, Manasi Navare wrote:
> > This patch abstracts pieces of hsw_crtc_enable corresponding to different
> > Bspec enable sequence steps into separate functions.
> > This helps to call them in a specific order for bigjoiner master/slave
> > in a cleaner fashion.
> > 
> > Cc: Ville Syrjälä <[e-mail]>
> > Cc: Animesh Manna <[e-mail]>
> > Signed-off-by: Manasi Navare <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
> >  1 file changed, 66 insertions(+), 59 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index eb49973621f0..d8e6466c9fa0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
> >  	intel_de_write(dev_priv, reg, val);
> >  }
> >  
> > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
> > -					 const struct intel_crtc_state *crtc_state)
> > -{
> > -	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> > -
> > -	/*
> > -	 * Enable sequence steps 1-7 on bigjoiner master
> > -	 */
> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > -		intel_encoders_pre_pll_enable(state, master_crtc);
> > -
> > -	if (crtc_state->shared_dpll)
> > -		intel_enable_shared_dpll(crtc_state);
> > -
> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > -		intel_encoders_pre_enable(state, master_crtc);
> > -}
> > -
> >  static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
> >  {
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
> >  	hsw_set_transconf(crtc_state);
> >  }
> >  
> > -static void hsw_crtc_enable(struct intel_atomic_state *state,
> > -			    struct intel_crtc *crtc)
> > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> > +				    const struct intel_crtc_state *crtc_state)
> >  {
> > -	const struct intel_crtc_state *new_crtc_state =
> > -		intel_atomic_get_new_crtc_state(state, crtc);
> > -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > -	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > -	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> > -	bool psl_clkgate_wa;
> > -
> > -	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > -		return;
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >  
> > -	if (!new_crtc_state->bigjoiner_pipes) {
> > -		intel_encoders_pre_pll_enable(state, crtc);
> > +	/*
> > +	 * Enable sequence steps 1 - 7 on all pipes
> > +	 */
> > +	intel_encoders_pre_pll_enable(state, crtc);
> > +	if (crtc_state->shared_dpll)
> > +		intel_enable_shared_dpll(crtc_state);
> >  
> > -		if (new_crtc_state->shared_dpll)
> > -			intel_enable_shared_dpll(new_crtc_state);
> > +	intel_encoders_pre_enable(state, crtc);
> > +}
> >  
> > -		intel_encoders_pre_enable(state, crtc);
> > -	} else {
> > -		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> > -	}
> > +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> > +				     const struct intel_crtc_state *crtc_state)
> > +{
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > +	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > +	bool psl_clkgate_wa;
> >  
> > -	intel_dsc_enable(new_crtc_state);
> > +	/*
> > +	 * Enable sequence step 8
> > +	 */
> > +	intel_dsc_enable(crtc_state);
> >  
> >  	if (DISPLAY_VER(dev_priv) >= 13)
> > -		intel_uncompressed_joiner_enable(new_crtc_state);
> > +		intel_uncompressed_joiner_enable(crtc_state);
> >  
> > -	intel_set_pipe_src_size(new_crtc_state);
> > +	intel_set_pipe_src_size(crtc_state);
> >  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > -		bdw_set_pipemisc(new_crtc_state);
> > +		bdw_set_pipemisc(crtc_state);
> >  
> > -	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> > +	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
> >  	    !transcoder_is_dsi(cpu_transcoder))
> > -		hsw_configure_cpu_transcoder(new_crtc_state);
> > +		hsw_configure_cpu_transcoder(crtc_state);
> >  
> >  	crtc->active = true;
> >  
> >  	/* Display WA #1180: WaDisableScalarClockGating: glk */
> >  	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
> > -		new_crtc_state->pch_pfit.enabled;
> > +		crtc_state->pch_pfit.enabled;
> >  	if (psl_clkgate_wa)
> >  		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
> >  
> >  	if (DISPLAY_VER(dev_priv) >= 9)
> > -		skl_pfit_enable(new_crtc_state);
> > +		skl_pfit_enable(crtc_state);
> >  	else
> > -		ilk_pfit_enable(new_crtc_state);
> > +		ilk_pfit_enable(crtc_state);
> >  
> >  	/*
> >  	 * On ILK+ LUT must be loaded before the pipe is running but with
> >  	 * clocks enabled
> >  	 */
> > -	intel_color_load_luts(new_crtc_state);
> > -	intel_color_commit(new_crtc_state);
> > +	intel_color_load_luts(crtc_state);
> > +	intel_color_commit(crtc_state);
> >  	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
> >  	if (DISPLAY_VER(dev_priv) < 9)
> > -		intel_disable_primary_plane(new_crtc_state);
> > +		intel_disable_primary_plane(crtc_state);
> >  
> > -	hsw_set_linetime_wm(new_crtc_state);
> > +	hsw_set_linetime_wm(crtc_state);
> >  
> >  	if (DISPLAY_VER(dev_priv) >= 11)
> > -		icl_set_pipe_chicken(new_crtc_state);
> > +		icl_set_pipe_chicken(crtc_state);
> >  
> >  	intel_initial_watermarks(state, crtc);
> >  
> > @@ -1984,8 +1969,8 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> >  		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
> >  	}
> >  
> > -	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> > -		intel_crtc_vblank_on(new_crtc_state);
> > +	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > +		intel_crtc_vblank_on(crtc_state);
> >  
> >  	intel_encoders_enable(state, crtc);
> >  
> > @@ -1996,7 +1981,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> >  
> >  	/* If we change the relative order between pipe/planes enabling, we need
> >  	 * to change the workaround. */
> > -	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
> > +	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
> >  	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
> >  		struct intel_crtc *wa_crtc;
> >  
> > @@ -2007,6 +1992,29 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> >  	}
> >  }
> >  
> > +static void hsw_crtc_enable(struct intel_atomic_state *state,
> > +			    struct intel_crtc *crtc)
> > +{
> > +	const struct intel_crtc_state *new_crtc_state =
> > +		intel_atomic_get_new_crtc_state(state, crtc);
> > +	struct intel_crtc *slave_crtc;
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > +
> > +	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > +		return;
> > +
> > +	hsw_crtc_pre_pll_enable(state, new_crtc_state);
> > +
> > +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
> > +					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
> > +		struct intel_crtc_state *slave_crtc_state =
> > +			intel_atomic_get_new_crtc_state(state, slave_crtc);
> > +
> > +		hsw_crtc_post_pll_enable(state, slave_crtc_state);
> > +	}
> > +	hsw_crtc_post_pll_enable(state, new_crtc_state);
> > +}
> 
> I suspect this is far too high level for bigjoiner. Eg. there's a bunch
> of things already in the disable sequence that seem to need much more
> low level sequencing between the joined pipes. So my gut feeling still
> is that we want to continue the per-pipe vs. per-transcoder split and
> do the joiner loops in lower level code.
>

But for enable sequence upto all pre_pll_enable needs to happen for master and then post_pll_enable for all slaves first and then master.
Hence added the sequencing here. Because like I have split here the post_pll_enable covers Bspec step 8 and hence that needs to happen for all
slaves first and then master.
I believe this cleans up a lot of bigjoiner special casing.

Further like Jani said, I can just make a separate icl_crtc_enable hook and only that will have this sequence.

Does this make sense as a first refacoring step? This should to scale the code to multiple joiners.

Manasi
 
> > +
> >  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
> >  {
> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> > @@ -8122,11 +8130,11 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
> >  
> >  	intel_crtc_update_active_timings(new_crtc_state);
> >  
> > -	dev_priv->display->crtc_enable(state, crtc);
> > -
> >  	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> >  		return;
> >  
> > +	dev_priv->display->crtc_enable(state, crtc);
> > +
> >  	intel_drrs_enable(new_crtc_state);
> >  
> >  	/* vblanks work again, re-enable pipe CRC. */
> > @@ -8360,8 +8368,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> >  			continue;
> >  
> >  		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > -		    is_trans_port_sync_master(new_crtc_state) ||
> > -		    intel_crtc_is_bigjoiner_master(new_crtc_state))
> > +		    is_trans_port_sync_master(new_crtc_state))
> >  			continue;
> >  
> >  		modeset_pipes &= ~BIT(pipe);
> > @@ -8371,7 +8378,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> >  
> >  	/*
> >  	 * Then we enable all remaining pipes that depend on other
> > -	 * pipes: MST slaves and port sync masters, big joiner master
> > +	 * pipes: MST slaves and port sync masters
> >  	 */
> >  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> >  		enum pipe pipe = crtc->pipe;
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel

Ville Syrjälä March 17, 2022, 7:14 p.m. UTC | #8

On Thu, Mar 17, 2022 at 12:05:47PM -0700, Navare, Manasi wrote:
> On Thu, Mar 17, 2022 at 08:52:52PM +0200, Ville Syrjälä wrote:
> > On Tue, Mar 15, 2022 at 04:38:56PM -0700, Manasi Navare wrote:
> > > This patch abstracts pieces of hsw_crtc_enable corresponding to different
> > > Bspec enable sequence steps into separate functions.
> > > This helps to call them in a specific order for bigjoiner master/slave
> > > in a cleaner fashion.
> > > 
> > > Cc: Ville Syrjälä <[e-mail]>
> > > Cc: Animesh Manna <[e-mail]>
> > > Signed-off-by: Manasi Navare <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
> > >  1 file changed, 66 insertions(+), 59 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index eb49973621f0..d8e6466c9fa0 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
> > >  	intel_de_write(dev_priv, reg, val);
> > >  }
> > >  
> > > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
> > > -					 const struct intel_crtc_state *crtc_state)
> > > -{
> > > -	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> > > -
> > > -	/*
> > > -	 * Enable sequence steps 1-7 on bigjoiner master
> > > -	 */
> > > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > > -		intel_encoders_pre_pll_enable(state, master_crtc);
> > > -
> > > -	if (crtc_state->shared_dpll)
> > > -		intel_enable_shared_dpll(crtc_state);
> > > -
> > > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > > -		intel_encoders_pre_enable(state, master_crtc);
> > > -}
> > > -
> > >  static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
> > >  {
> > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
> > >  	hsw_set_transconf(crtc_state);
> > >  }
> > >  
> > > -static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > -			    struct intel_crtc *crtc)
> > > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> > > +				    const struct intel_crtc_state *crtc_state)
> > >  {
> > > -	const struct intel_crtc_state *new_crtc_state =
> > > -		intel_atomic_get_new_crtc_state(state, crtc);
> > > -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > -	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > > -	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> > > -	bool psl_clkgate_wa;
> > > -
> > > -	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > > -		return;
> > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > >  
> > > -	if (!new_crtc_state->bigjoiner_pipes) {
> > > -		intel_encoders_pre_pll_enable(state, crtc);
> > > +	/*
> > > +	 * Enable sequence steps 1 - 7 on all pipes
> > > +	 */
> > > +	intel_encoders_pre_pll_enable(state, crtc);
> > > +	if (crtc_state->shared_dpll)
> > > +		intel_enable_shared_dpll(crtc_state);
> > >  
> > > -		if (new_crtc_state->shared_dpll)
> > > -			intel_enable_shared_dpll(new_crtc_state);
> > > +	intel_encoders_pre_enable(state, crtc);
> > > +}
> > >  
> > > -		intel_encoders_pre_enable(state, crtc);
> > > -	} else {
> > > -		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> > > -	}
> > > +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> > > +				     const struct intel_crtc_state *crtc_state)
> > > +{
> > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > +	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > > +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > > +	bool psl_clkgate_wa;
> > >  
> > > -	intel_dsc_enable(new_crtc_state);
> > > +	/*
> > > +	 * Enable sequence step 8
> > > +	 */
> > > +	intel_dsc_enable(crtc_state);
> > >  
> > >  	if (DISPLAY_VER(dev_priv) >= 13)
> > > -		intel_uncompressed_joiner_enable(new_crtc_state);
> > > +		intel_uncompressed_joiner_enable(crtc_state);
> > >  
> > > -	intel_set_pipe_src_size(new_crtc_state);
> > > +	intel_set_pipe_src_size(crtc_state);
> > >  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > > -		bdw_set_pipemisc(new_crtc_state);
> > > +		bdw_set_pipemisc(crtc_state);
> > >  
> > > -	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> > > +	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
> > >  	    !transcoder_is_dsi(cpu_transcoder))
> > > -		hsw_configure_cpu_transcoder(new_crtc_state);
> > > +		hsw_configure_cpu_transcoder(crtc_state);
> > >  
> > >  	crtc->active = true;
> > >  
> > >  	/* Display WA #1180: WaDisableScalarClockGating: glk */
> > >  	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
> > > -		new_crtc_state->pch_pfit.enabled;
> > > +		crtc_state->pch_pfit.enabled;
> > >  	if (psl_clkgate_wa)
> > >  		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
> > >  
> > >  	if (DISPLAY_VER(dev_priv) >= 9)
> > > -		skl_pfit_enable(new_crtc_state);
> > > +		skl_pfit_enable(crtc_state);
> > >  	else
> > > -		ilk_pfit_enable(new_crtc_state);
> > > +		ilk_pfit_enable(crtc_state);
> > >  
> > >  	/*
> > >  	 * On ILK+ LUT must be loaded before the pipe is running but with
> > >  	 * clocks enabled
> > >  	 */
> > > -	intel_color_load_luts(new_crtc_state);
> > > -	intel_color_commit(new_crtc_state);
> > > +	intel_color_load_luts(crtc_state);
> > > +	intel_color_commit(crtc_state);
> > >  	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
> > >  	if (DISPLAY_VER(dev_priv) < 9)
> > > -		intel_disable_primary_plane(new_crtc_state);
> > > +		intel_disable_primary_plane(crtc_state);
> > >  
> > > -	hsw_set_linetime_wm(new_crtc_state);
> > > +	hsw_set_linetime_wm(crtc_state);
> > >  
> > >  	if (DISPLAY_VER(dev_priv) >= 11)
> > > -		icl_set_pipe_chicken(new_crtc_state);
> > > +		icl_set_pipe_chicken(crtc_state);
> > >  
> > >  	intel_initial_watermarks(state, crtc);
> > >  
> > > @@ -1984,8 +1969,8 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > >  		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
> > >  	}
> > >  
> > > -	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> > > -		intel_crtc_vblank_on(new_crtc_state);
> > > +	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > > +		intel_crtc_vblank_on(crtc_state);
> > >  
> > >  	intel_encoders_enable(state, crtc);
> > >  
> > > @@ -1996,7 +1981,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > >  
> > >  	/* If we change the relative order between pipe/planes enabling, we need
> > >  	 * to change the workaround. */
> > > -	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
> > > +	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
> > >  	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
> > >  		struct intel_crtc *wa_crtc;
> > >  
> > > @@ -2007,6 +1992,29 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > >  	}
> > >  }
> > >  
> > > +static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > +			    struct intel_crtc *crtc)
> > > +{
> > > +	const struct intel_crtc_state *new_crtc_state =
> > > +		intel_atomic_get_new_crtc_state(state, crtc);
> > > +	struct intel_crtc *slave_crtc;
> > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > +
> > > +	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > > +		return;
> > > +
> > > +	hsw_crtc_pre_pll_enable(state, new_crtc_state);
> > > +
> > > +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
> > > +					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
> > > +		struct intel_crtc_state *slave_crtc_state =
> > > +			intel_atomic_get_new_crtc_state(state, slave_crtc);
> > > +
> > > +		hsw_crtc_post_pll_enable(state, slave_crtc_state);
> > > +	}
> > > +	hsw_crtc_post_pll_enable(state, new_crtc_state);
> > > +}
> > 
> > I suspect this is far too high level for bigjoiner. Eg. there's a bunch
> > of things already in the disable sequence that seem to need much more
> > low level sequencing between the joined pipes. So my gut feeling still
> > is that we want to continue the per-pipe vs. per-transcoder split and
> > do the joiner loops in lower level code.
> >
> 
> But for enable sequence upto all pre_pll_enable needs to happen for master and then post_pll_enable for all slaves first and then master.

We want the enable and disable to be as symmetric as possible. So you
can't just blindly stare at the enable sequence and ignore what's going
on in the disable sequence. Otherwise the end result will be a confusing
mess.

> Hence added the sequencing here. Because like I have split here the post_pll_enable covers Bspec step 8 and hence that needs to happen for all
> slaves first and then master.
> I believe this cleans up a lot of bigjoiner special casing.
> 
> Further like Jani said, I can just make a separate icl_crtc_enable hook and only that will have this sequence.

That doesn't seem particularly helpful if it just means copy-pasting
the whole hsw_crtc_enable() into two places and removing one or two
little things from the original.

> 
> Does this make sense as a first refacoring step? This should to scale the code to multiple joiners.
> 
> Manasi
>  
> > > +
> > >  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
> > >  {
> > >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> > > @@ -8122,11 +8130,11 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
> > >  
> > >  	intel_crtc_update_active_timings(new_crtc_state);
> > >  
> > > -	dev_priv->display->crtc_enable(state, crtc);
> > > -
> > >  	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> > >  		return;
> > >  
> > > +	dev_priv->display->crtc_enable(state, crtc);
> > > +
> > >  	intel_drrs_enable(new_crtc_state);
> > >  
> > >  	/* vblanks work again, re-enable pipe CRC. */
> > > @@ -8360,8 +8368,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > >  			continue;
> > >  
> > >  		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > > -		    is_trans_port_sync_master(new_crtc_state) ||
> > > -		    intel_crtc_is_bigjoiner_master(new_crtc_state))
> > > +		    is_trans_port_sync_master(new_crtc_state))
> > >  			continue;
> > >  
> > >  		modeset_pipes &= ~BIT(pipe);
> > > @@ -8371,7 +8378,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > >  
> > >  	/*
> > >  	 * Then we enable all remaining pipes that depend on other
> > > -	 * pipes: MST slaves and port sync masters, big joiner master
> > > +	 * pipes: MST slaves and port sync masters
> > >  	 */
> > >  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> > >  		enum pipe pipe = crtc->pipe;
> > > -- 
> > > 2.19.1
> > 
> > -- 
> > Ville Syrjälä
> > Intel

Jani Nikula March 17, 2022, 7:15 p.m. UTC | #9

On Thu, 17 Mar 2022, "Navare, Manasi" <[e-mail]> wrote:
> On Thu, Mar 17, 2022 at 11:28:03AM -0700, Navare, Manasi wrote:
>> On Thu, Mar 17, 2022 at 05:35:43PM +0200, Jani Nikula wrote:
>> > On Wed, 16 Mar 2022, "Navare, Manasi" <[e-mail]> wrote:
>> > > On Wed, Mar 16, 2022 at 09:48:17AM +0200, Jani Nikula wrote:
>> > >> On Tue, 15 Mar 2022, Manasi Navare <[e-mail]> wrote:
>> > >> > This patch abstracts pieces of hsw_crtc_enable corresponding to different
>> > >> > Bspec enable sequence steps into separate functions.
>> > >> > This helps to call them in a specific order for bigjoiner master/slave
>> > >> > in a cleaner fashion.
>> > >> 
>> > >> So does this contain only refactoring or functional changes also? One or
>> > >> the other at a time, please, not both.
>> > >
>> > > No this is only refactor, no functional changes here
>> > >
>> > >> 
>> > >> Also looks like hsw_crtc_* have accumulated just way too many checks for
>> > >> platforms instead of having a clean break at e.g. display 9 and/or 11.
>> > >
>> > > These checks were already part of hsw_crtc_enable() function that I have just moved to a separate
>> > > function. How do you recommend removing these checks?
>> > 
>> > We use hsw_crtc_enable for all DDI platforms and later. We do have the
>> > difference between skl_display_funcs and ddi_display_funcs, but they
>> > both point to hsw_crtc_enable. Maybe it's time for them to have their
>> > own functions that don't have to take so many platform differences into
>> > account.
>> 
>> Yes I could perhaps have a separate skl_crtc_enable() function which would then have all parts that apply to DISPLAy >=9
>> everything else can stay in hsw_crtc_enable
>> 
>> That makes sense?
>
> Or actually we could have a separate one for >=11 starting ICL, since there are a lot of
> checks for >=11. Also bigjoiner support only from ICL, so then this bigjoiner sequence then only becomes part
> of icl_crtc_enable()
> Which one of the above 2 do you recommend?

Could be both! Depends on the improved clarity vs. amount of code
duplication.

BR,
Jani.


>
> Manasi
>
>> 
>> Manasi
>> 
>> > 
>> > BR,
>> > Jani.
>> > 
>> > 
>> > 
>> > 
>> > >
>> > >> 
>> > >> Adding references to "enable sequence step 8" is not helpful because
>> > >> it's platform specific. (Yeah, I know there are existing references like
>> > >> this, but they're also suspect.)
>> > >
>> > > Yes agree, may be I will add the comment for what actually the step 7/8 does?
>> > >
>> > > Manasi
>> > >
>> > >> 
>> > >> 
>> > >> BR,
>> > >> Jani.
>> > >> 
>> > >> 
>> > >> >
>> > >> > Cc: Ville Syrjälä <[e-mail]>
>> > >> > Cc: Animesh Manna <[e-mail]>
>> > >> > Signed-off-by: Manasi Navare <[e-mail]>
>> > >> > ---
>> > >> >  drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
>> > >> >  1 file changed, 66 insertions(+), 59 deletions(-)
>> > >> >
>> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> > >> > index eb49973621f0..d8e6466c9fa0 100644
>> > >> > --- a/drivers/gpu/drm/i915/display/intel_display.c
>> > >> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> > >> > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
>> > >> >  	intel_de_write(dev_priv, reg, val);
>> > >> >  }
>> > >> >  
>> > >> > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
>> > >> > -					 const struct intel_crtc_state *crtc_state)
>> > >> > -{
>> > >> > -	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
>> > >> > -
>> > >> > -	/*
>> > >> > -	 * Enable sequence steps 1-7 on bigjoiner master
>> > >> > -	 */
>> > >> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
>> > >> > -		intel_encoders_pre_pll_enable(state, master_crtc);
>> > >> > -
>> > >> > -	if (crtc_state->shared_dpll)
>> > >> > -		intel_enable_shared_dpll(crtc_state);
>> > >> > -
>> > >> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
>> > >> > -		intel_encoders_pre_enable(state, master_crtc);
>> > >> > -}
>> > >> > -
>> > >> >  static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
>> > >> >  {
>> > >> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> > >> > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
>> > >> >  	hsw_set_transconf(crtc_state);
>> > >> >  }
>> > >> >  
>> > >> > -static void hsw_crtc_enable(struct intel_atomic_state *state,
>> > >> > -			    struct intel_crtc *crtc)
>> > >> > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
>> > >> > +				    const struct intel_crtc_state *crtc_state)
>> > >> >  {
>> > >> > -	const struct intel_crtc_state *new_crtc_state =
>> > >> > -		intel_atomic_get_new_crtc_state(state, crtc);
>> > >> > -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> > >> > -	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
>> > >> > -	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
>> > >> > -	bool psl_clkgate_wa;
>> > >> > -
>> > >> > -	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
>> > >> > -		return;
>> > >> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> > >> >  
>> > >> > -	if (!new_crtc_state->bigjoiner_pipes) {
>> > >> > -		intel_encoders_pre_pll_enable(state, crtc);
>> > >> > +	/*
>> > >> > +	 * Enable sequence steps 1 - 7 on all pipes
>> > >> > +	 */
>> > >> > +	intel_encoders_pre_pll_enable(state, crtc);
>> > >> > +	if (crtc_state->shared_dpll)
>> > >> > +		intel_enable_shared_dpll(crtc_state);
>> > >> >  
>> > >> > -		if (new_crtc_state->shared_dpll)
>> > >> > -			intel_enable_shared_dpll(new_crtc_state);
>> > >> > +	intel_encoders_pre_enable(state, crtc);
>> > >> > +}
>> > >> >  
>> > >> > -		intel_encoders_pre_enable(state, crtc);
>> > >> > -	} else {
>> > >> > -		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
>> > >> > -	}
>> > >> > +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
>> > >> > +				     const struct intel_crtc_state *crtc_state)
>> > >> > +{
>> > >> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> > >> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> > >> > +	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
>> > >> > +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>> > >> > +	bool psl_clkgate_wa;
>> > >> >  
>> > >> > -	intel_dsc_enable(new_crtc_state);
>> > >> > +	/*
>> > >> > +	 * Enable sequence step 8
>> > >> > +	 */
>> > >> > +	intel_dsc_enable(crtc_state);
>> > >> >  
>> > >> >  	if (DISPLAY_VER(dev_priv) >= 13)
>> > >> > -		intel_uncompressed_joiner_enable(new_crtc_state);
>> > >> > +		intel_uncompressed_joiner_enable(crtc_state);
>> > >> >  
>> > >> > -	intel_set_pipe_src_size(new_crtc_state);
>> > >> > +	intel_set_pipe_src_size(crtc_state);
>> > >> >  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
>> > >> > -		bdw_set_pipemisc(new_crtc_state);
>> > >> > +		bdw_set_pipemisc(crtc_state);
>> > >> >  
>> > >> > -	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
>> > >> > +	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
>> > >> >  	    !transcoder_is_dsi(cpu_transcoder))
>> > >> > -		hsw_configure_cpu_transcoder(new_crtc_state);
>> > >> > +		hsw_configure_cpu_transcoder(crtc_state);
>> > >> >  
>> > >> >  	crtc->active = true;
>> > >> >  
>> > >> >  	/* Display WA #1180: WaDisableScalarClockGating: glk */
>> > >> >  	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
>> > >> > -		new_crtc_state->pch_pfit.enabled;
>> > >> > +		crtc_state->pch_pfit.enabled;
>> > >> >  	if (psl_clkgate_wa)
>> > >> >  		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
>> > >> >  
>> > >> >  	if (DISPLAY_VER(dev_priv) >= 9)
>> > >> > -		skl_pfit_enable(new_crtc_state);
>> > >> > +		skl_pfit_enable(crtc_state);
>> > >> >  	else
>> > >> > -		ilk_pfit_enable(new_crtc_state);
>> > >> > +		ilk_pfit_enable(crtc_state);
>> > >> >  
>> > >> >  	/*
>> > >> >  	 * On ILK+ LUT must be loaded before the pipe is running but with
>> > >> >  	 * clocks enabled
>> > >> >  	 */
>> > >> > -	intel_color_load_luts(new_crtc_state);
>> > >> > -	intel_color_commit(new_crtc_state);
>> > >> > +	intel_color_load_luts(crtc_state);
>> > >> > +	intel_color_commit(crtc_state);
>> > >> >  	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
>> > >> >  	if (DISPLAY_VER(dev_priv) < 9)
>> > >> > -		intel_disable_primary_plane(new_crtc_state);
>> > >> > +		intel_disable_primary_plane(crtc_state);
>> > >> >  
>> > >> > -	hsw_set_linetime_wm(new_crtc_state);
>> > >> > +	hsw_set_linetime_wm(crtc_state);
>> > >> >  
>> > >> >  	if (DISPLAY_VER(dev_priv) >= 11)
>> > >> > -		icl_set_pipe_chicken(new_crtc_state);
>> > >> > +		icl_set_pipe_chicken(crtc_state);
>> > >> >  
>> > >> >  	intel_initial_watermarks(state, crtc);
>> > >> >  
>> > >> > @@ -1984,8 +1969,8 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>> > >> >  		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
>> > >> >  	}
>> > >> >  
>> > >> > -	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
>> > >> > -		intel_crtc_vblank_on(new_crtc_state);
>> > >> > +	if (intel_crtc_is_bigjoiner_slave(crtc_state))
>> > >> > +		intel_crtc_vblank_on(crtc_state);
>> > >> >  
>> > >> >  	intel_encoders_enable(state, crtc);
>> > >> >  
>> > >> > @@ -1996,7 +1981,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>> > >> >  
>> > >> >  	/* If we change the relative order between pipe/planes enabling, we need
>> > >> >  	 * to change the workaround. */
>> > >> > -	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
>> > >> > +	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
>> > >> >  	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
>> > >> >  		struct intel_crtc *wa_crtc;
>> > >> >  
>> > >> > @@ -2007,6 +1992,29 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>> > >> >  	}
>> > >> >  }
>> > >> >  
>> > >> > +static void hsw_crtc_enable(struct intel_atomic_state *state,
>> > >> > +			    struct intel_crtc *crtc)
>> > >> > +{
>> > >> > +	const struct intel_crtc_state *new_crtc_state =
>> > >> > +		intel_atomic_get_new_crtc_state(state, crtc);
>> > >> > +	struct intel_crtc *slave_crtc;
>> > >> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> > >> > +
>> > >> > +	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
>> > >> > +		return;
>> > >> > +
>> > >> > +	hsw_crtc_pre_pll_enable(state, new_crtc_state);
>> > >> > +
>> > >> > +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
>> > >> > +					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
>> > >> > +		struct intel_crtc_state *slave_crtc_state =
>> > >> > +			intel_atomic_get_new_crtc_state(state, slave_crtc);
>> > >> > +
>> > >> > +		hsw_crtc_post_pll_enable(state, slave_crtc_state);
>> > >> > +	}
>> > >> > +	hsw_crtc_post_pll_enable(state, new_crtc_state);
>> > >> > +}
>> > >> > +
>> > >> >  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
>> > >> >  {
>> > >> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
>> > >> > @@ -8122,11 +8130,11 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
>> > >> >  
>> > >> >  	intel_crtc_update_active_timings(new_crtc_state);
>> > >> >  
>> > >> > -	dev_priv->display->crtc_enable(state, crtc);
>> > >> > -
>> > >> >  	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
>> > >> >  		return;
>> > >> >  
>> > >> > +	dev_priv->display->crtc_enable(state, crtc);
>> > >> > +
>> > >> >  	intel_drrs_enable(new_crtc_state);
>> > >> >  
>> > >> >  	/* vblanks work again, re-enable pipe CRC. */
>> > >> > @@ -8360,8 +8368,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>> > >> >  			continue;
>> > >> >  
>> > >> >  		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
>> > >> > -		    is_trans_port_sync_master(new_crtc_state) ||
>> > >> > -		    intel_crtc_is_bigjoiner_master(new_crtc_state))
>> > >> > +		    is_trans_port_sync_master(new_crtc_state))
>> > >> >  			continue;
>> > >> >  
>> > >> >  		modeset_pipes &= ~BIT(pipe);
>> > >> > @@ -8371,7 +8378,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>> > >> >  
>> > >> >  	/*
>> > >> >  	 * Then we enable all remaining pipes that depend on other
>> > >> > -	 * pipes: MST slaves and port sync masters, big joiner master
>> > >> > +	 * pipes: MST slaves and port sync masters
>> > >> >  	 */
>> > >> >  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
>> > >> >  		enum pipe pipe = crtc->pipe;
>> > >> 
>> > >> -- 
>> > >> Jani Nikula, Intel Open Source Graphics Center
>> > 
>> > -- 
>> > Jani Nikula, Intel Open Source Graphics Center

Navare, Manasi March 30, 2022, midnight UTC | #10

Hi Ville,

I was looking at your suggestion of extracting the per pipe stuff out.
Currently in hsw_crtc_enable: the Only non per pipe stuff which gets enabled for the encoders is :
encoder specific is pre_pll_enable(), enable_shared_dpll, encoders_pre_enable and configure_cpu_transcoder() - All of this 
can be put in a function hsw_encoder_configure() or something that can still be called from with hsw_crtc_enable

Then the remaining can go into a per pipe function that can be called for each slave pipe

But it means still pretty much splitting the current hsw_crtc_enable into 2 separate functions

Does this refactoring make sense?

Manasi


On Thu, Mar 17, 2022 at 09:14:16PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 17, 2022 at 12:05:47PM -0700, Navare, Manasi wrote:
> > On Thu, Mar 17, 2022 at 08:52:52PM +0200, Ville Syrjälä wrote:
> > > On Tue, Mar 15, 2022 at 04:38:56PM -0700, Manasi Navare wrote:
> > > > This patch abstracts pieces of hsw_crtc_enable corresponding to different
> > > > Bspec enable sequence steps into separate functions.
> > > > This helps to call them in a specific order for bigjoiner master/slave
> > > > in a cleaner fashion.
> > > > 
> > > > Cc: Ville Syrjälä <[e-mail]>
> > > > Cc: Animesh Manna <[e-mail]>
> > > > Signed-off-by: Manasi Navare <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
> > > >  1 file changed, 66 insertions(+), 59 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index eb49973621f0..d8e6466c9fa0 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
> > > >  	intel_de_write(dev_priv, reg, val);
> > > >  }
> > > >  
> > > > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
> > > > -					 const struct intel_crtc_state *crtc_state)
> > > > -{
> > > > -	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> > > > -
> > > > -	/*
> > > > -	 * Enable sequence steps 1-7 on bigjoiner master
> > > > -	 */
> > > > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > > > -		intel_encoders_pre_pll_enable(state, master_crtc);
> > > > -
> > > > -	if (crtc_state->shared_dpll)
> > > > -		intel_enable_shared_dpll(crtc_state);
> > > > -
> > > > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > > > -		intel_encoders_pre_enable(state, master_crtc);
> > > > -}
> > > > -
> > > >  static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
> > > >  {
> > > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
> > > >  	hsw_set_transconf(crtc_state);
> > > >  }
> > > >  
> > > > -static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > > -			    struct intel_crtc *crtc)
> > > > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> > > > +				    const struct intel_crtc_state *crtc_state)
> > > >  {
> > > > -	const struct intel_crtc_state *new_crtc_state =
> > > > -		intel_atomic_get_new_crtc_state(state, crtc);
> > > > -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > > -	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > > > -	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> > > > -	bool psl_clkgate_wa;
> > > > -
> > > > -	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > > > -		return;
> > > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > >  
> > > > -	if (!new_crtc_state->bigjoiner_pipes) {
> > > > -		intel_encoders_pre_pll_enable(state, crtc);
> > > > +	/*
> > > > +	 * Enable sequence steps 1 - 7 on all pipes
> > > > +	 */
> > > > +	intel_encoders_pre_pll_enable(state, crtc);
> > > > +	if (crtc_state->shared_dpll)
> > > > +		intel_enable_shared_dpll(crtc_state);
> > > >  
> > > > -		if (new_crtc_state->shared_dpll)
> > > > -			intel_enable_shared_dpll(new_crtc_state);
> > > > +	intel_encoders_pre_enable(state, crtc);
> > > > +}
> > > >  
> > > > -		intel_encoders_pre_enable(state, crtc);
> > > > -	} else {
> > > > -		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> > > > -	}
> > > > +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> > > > +				     const struct intel_crtc_state *crtc_state)
> > > > +{
> > > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > > +	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > > > +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > > > +	bool psl_clkgate_wa;
> > > >  
> > > > -	intel_dsc_enable(new_crtc_state);
> > > > +	/*
> > > > +	 * Enable sequence step 8
> > > > +	 */
> > > > +	intel_dsc_enable(crtc_state);
> > > >  
> > > >  	if (DISPLAY_VER(dev_priv) >= 13)
> > > > -		intel_uncompressed_joiner_enable(new_crtc_state);
> > > > +		intel_uncompressed_joiner_enable(crtc_state);
> > > >  
> > > > -	intel_set_pipe_src_size(new_crtc_state);
> > > > +	intel_set_pipe_src_size(crtc_state);
> > > >  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > > > -		bdw_set_pipemisc(new_crtc_state);
> > > > +		bdw_set_pipemisc(crtc_state);
> > > >  
> > > > -	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> > > > +	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
> > > >  	    !transcoder_is_dsi(cpu_transcoder))
> > > > -		hsw_configure_cpu_transcoder(new_crtc_state);
> > > > +		hsw_configure_cpu_transcoder(crtc_state);
> > > >  
> > > >  	crtc->active = true;
> > > >  
> > > >  	/* Display WA #1180: WaDisableScalarClockGating: glk */
> > > >  	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
> > > > -		new_crtc_state->pch_pfit.enabled;
> > > > +		crtc_state->pch_pfit.enabled;
> > > >  	if (psl_clkgate_wa)
> > > >  		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
> > > >  
> > > >  	if (DISPLAY_VER(dev_priv) >= 9)
> > > > -		skl_pfit_enable(new_crtc_state);
> > > > +		skl_pfit_enable(crtc_state);
> > > >  	else
> > > > -		ilk_pfit_enable(new_crtc_state);
> > > > +		ilk_pfit_enable(crtc_state);
> > > >  
> > > >  	/*
> > > >  	 * On ILK+ LUT must be loaded before the pipe is running but with
> > > >  	 * clocks enabled
> > > >  	 */
> > > > -	intel_color_load_luts(new_crtc_state);
> > > > -	intel_color_commit(new_crtc_state);
> > > > +	intel_color_load_luts(crtc_state);
> > > > +	intel_color_commit(crtc_state);
> > > >  	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
> > > >  	if (DISPLAY_VER(dev_priv) < 9)
> > > > -		intel_disable_primary_plane(new_crtc_state);
> > > > +		intel_disable_primary_plane(crtc_state);
> > > >  
> > > > -	hsw_set_linetime_wm(new_crtc_state);
> > > > +	hsw_set_linetime_wm(crtc_state);
> > > >  
> > > >  	if (DISPLAY_VER(dev_priv) >= 11)
> > > > -		icl_set_pipe_chicken(new_crtc_state);
> > > > +		icl_set_pipe_chicken(crtc_state);
> > > >  
> > > >  	intel_initial_watermarks(state, crtc);
> > > >  
> > > > @@ -1984,8 +1969,8 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > >  		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
> > > >  	}
> > > >  
> > > > -	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> > > > -		intel_crtc_vblank_on(new_crtc_state);
> > > > +	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > > > +		intel_crtc_vblank_on(crtc_state);
> > > >  
> > > >  	intel_encoders_enable(state, crtc);
> > > >  
> > > > @@ -1996,7 +1981,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > >  
> > > >  	/* If we change the relative order between pipe/planes enabling, we need
> > > >  	 * to change the workaround. */
> > > > -	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
> > > > +	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
> > > >  	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
> > > >  		struct intel_crtc *wa_crtc;
> > > >  
> > > > @@ -2007,6 +1992,29 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > >  	}
> > > >  }
> > > >  
> > > > +static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > > +			    struct intel_crtc *crtc)
> > > > +{
> > > > +	const struct intel_crtc_state *new_crtc_state =
> > > > +		intel_atomic_get_new_crtc_state(state, crtc);
> > > > +	struct intel_crtc *slave_crtc;
> > > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > > +
> > > > +	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > > > +		return;
> > > > +
> > > > +	hsw_crtc_pre_pll_enable(state, new_crtc_state);
> > > > +
> > > > +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
> > > > +					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
> > > > +		struct intel_crtc_state *slave_crtc_state =
> > > > +			intel_atomic_get_new_crtc_state(state, slave_crtc);
> > > > +
> > > > +		hsw_crtc_post_pll_enable(state, slave_crtc_state);
> > > > +	}
> > > > +	hsw_crtc_post_pll_enable(state, new_crtc_state);
> > > > +}
> > > 
> > > I suspect this is far too high level for bigjoiner. Eg. there's a bunch
> > > of things already in the disable sequence that seem to need much more
> > > low level sequencing between the joined pipes. So my gut feeling still
> > > is that we want to continue the per-pipe vs. per-transcoder split and
> > > do the joiner loops in lower level code.
> > >
> > 
> > But for enable sequence upto all pre_pll_enable needs to happen for master and then post_pll_enable for all slaves first and then master.
> 
> We want the enable and disable to be as symmetric as possible. So you
> can't just blindly stare at the enable sequence and ignore what's going
> on in the disable sequence. Otherwise the end result will be a confusing
> mess.
> 
> > Hence added the sequencing here. Because like I have split here the post_pll_enable covers Bspec step 8 and hence that needs to happen for all
> > slaves first and then master.
> > I believe this cleans up a lot of bigjoiner special casing.
> > 
> > Further like Jani said, I can just make a separate icl_crtc_enable hook and only that will have this sequence.
> 
> That doesn't seem particularly helpful if it just means copy-pasting
> the whole hsw_crtc_enable() into two places and removing one or two
> little things from the original.
> 
> > 
> > Does this make sense as a first refacoring step? This should to scale the code to multiple joiners.
> > 
> > Manasi
> >  
> > > > +
> > > >  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
> > > >  {
> > > >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> > > > @@ -8122,11 +8130,11 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
> > > >  
> > > >  	intel_crtc_update_active_timings(new_crtc_state);
> > > >  
> > > > -	dev_priv->display->crtc_enable(state, crtc);
> > > > -
> > > >  	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> > > >  		return;
> > > >  
> > > > +	dev_priv->display->crtc_enable(state, crtc);
> > > > +
> > > >  	intel_drrs_enable(new_crtc_state);
> > > >  
> > > >  	/* vblanks work again, re-enable pipe CRC. */
> > > > @@ -8360,8 +8368,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > > >  			continue;
> > > >  
> > > >  		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > > > -		    is_trans_port_sync_master(new_crtc_state) ||
> > > > -		    intel_crtc_is_bigjoiner_master(new_crtc_state))
> > > > +		    is_trans_port_sync_master(new_crtc_state))
> > > >  			continue;
> > > >  
> > > >  		modeset_pipes &= ~BIT(pipe);
> > > > @@ -8371,7 +8378,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > > >  
> > > >  	/*
> > > >  	 * Then we enable all remaining pipes that depend on other
> > > > -	 * pipes: MST slaves and port sync masters, big joiner master
> > > > +	 * pipes: MST slaves and port sync masters
> > > >  	 */
> > > >  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> > > >  		enum pipe pipe = crtc->pipe;
> > > > -- 
> > > > 2.19.1
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel

Ville Syrjälä March 30, 2022, 9:53 a.m. UTC | #11

On Tue, Mar 29, 2022 at 05:00:39PM -0700, Navare, Manasi wrote:
> Hi Ville,
> 
> I was looking at your suggestion of extracting the per pipe stuff out.
> Currently in hsw_crtc_enable: the Only non per pipe stuff which gets enabled for the encoders is :
> encoder specific is pre_pll_enable(), enable_shared_dpll, encoders_pre_enable and configure_cpu_transcoder() - All of this 
> can be put in a function hsw_encoder_configure() or something that can still be called from with hsw_crtc_enable

I don't see a need to move that stuff anywhere. Just leave it
in hsw_crtc_enable() IMO.

> 
> Then the remaining can go into a per pipe function that can be called for each slave pipe

This is what I have been suggesting.

But I think there's also some per-pipe stuff currently in the
the encoder hooks, and some of that is only done for the master
there whereaas the slave part I think are somewhere else. We
should attempt to fix that as well so that every step is done
in a consistent manner for every pipe be it master or slave.

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index eb49973621f0..d8e6466c9fa0 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -1865,24 +1865,6 @@  static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
 	intel_de_write(dev_priv, reg, val);
 }
 
-static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
-					 const struct intel_crtc_state *crtc_state)
-{
-	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
-
-	/*
-	 * Enable sequence steps 1-7 on bigjoiner master
-	 */
-	if (intel_crtc_is_bigjoiner_slave(crtc_state))
-		intel_encoders_pre_pll_enable(state, master_crtc);
-
-	if (crtc_state->shared_dpll)
-		intel_enable_shared_dpll(crtc_state);
-
-	if (intel_crtc_is_bigjoiner_slave(crtc_state))
-		intel_encoders_pre_enable(state, master_crtc);
-}
-
 static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
@@ -1910,70 +1892,73 @@  static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
 	hsw_set_transconf(crtc_state);
 }
 
-static void hsw_crtc_enable(struct intel_atomic_state *state,
-			    struct intel_crtc *crtc)
+static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
+				    const struct intel_crtc_state *crtc_state)
 {
-	const struct intel_crtc_state *new_crtc_state =
-		intel_atomic_get_new_crtc_state(state, crtc);
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
-	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
-	bool psl_clkgate_wa;
-
-	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
-		return;
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 
-	if (!new_crtc_state->bigjoiner_pipes) {
-		intel_encoders_pre_pll_enable(state, crtc);
+	/*
+	 * Enable sequence steps 1 - 7 on all pipes
+	 */
+	intel_encoders_pre_pll_enable(state, crtc);
+	if (crtc_state->shared_dpll)
+		intel_enable_shared_dpll(crtc_state);
 
-		if (new_crtc_state->shared_dpll)
-			intel_enable_shared_dpll(new_crtc_state);
+	intel_encoders_pre_enable(state, crtc);
+}
 
-		intel_encoders_pre_enable(state, crtc);
-	} else {
-		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
-	}
+static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
+				     const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
+	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
+	bool psl_clkgate_wa;
 
-	intel_dsc_enable(new_crtc_state);
+	/*
+	 * Enable sequence step 8
+	 */
+	intel_dsc_enable(crtc_state);
 
 	if (DISPLAY_VER(dev_priv) >= 13)
-		intel_uncompressed_joiner_enable(new_crtc_state);
+		intel_uncompressed_joiner_enable(crtc_state);
 
-	intel_set_pipe_src_size(new_crtc_state);
+	intel_set_pipe_src_size(crtc_state);
 	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
-		bdw_set_pipemisc(new_crtc_state);
+		bdw_set_pipemisc(crtc_state);
 
-	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
+	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
 	    !transcoder_is_dsi(cpu_transcoder))
-		hsw_configure_cpu_transcoder(new_crtc_state);
+		hsw_configure_cpu_transcoder(crtc_state);
 
 	crtc->active = true;
 
 	/* Display WA #1180: WaDisableScalarClockGating: glk */
 	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
-		new_crtc_state->pch_pfit.enabled;
+		crtc_state->pch_pfit.enabled;
 	if (psl_clkgate_wa)
 		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
 
 	if (DISPLAY_VER(dev_priv) >= 9)
-		skl_pfit_enable(new_crtc_state);
+		skl_pfit_enable(crtc_state);
 	else
-		ilk_pfit_enable(new_crtc_state);
+		ilk_pfit_enable(crtc_state);
 
 	/*
 	 * On ILK+ LUT must be loaded before the pipe is running but with
 	 * clocks enabled
 	 */
-	intel_color_load_luts(new_crtc_state);
-	intel_color_commit(new_crtc_state);
+	intel_color_load_luts(crtc_state);
+	intel_color_commit(crtc_state);
 	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
 	if (DISPLAY_VER(dev_priv) < 9)
-		intel_disable_primary_plane(new_crtc_state);
+		intel_disable_primary_plane(crtc_state);
 
-	hsw_set_linetime_wm(new_crtc_state);
+	hsw_set_linetime_wm(crtc_state);
 
 	if (DISPLAY_VER(dev_priv) >= 11)
-		icl_set_pipe_chicken(new_crtc_state);
+		icl_set_pipe_chicken(crtc_state);
 
 	intel_initial_watermarks(state, crtc);
 
@@ -1984,8 +1969,8 @@  static void hsw_crtc_enable(struct intel_atomic_state *state,
 		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
 	}
 
-	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
-		intel_crtc_vblank_on(new_crtc_state);
+	if (intel_crtc_is_bigjoiner_slave(crtc_state))
+		intel_crtc_vblank_on(crtc_state);
 
 	intel_encoders_enable(state, crtc);
 
@@ -1996,7 +1981,7 @@  static void hsw_crtc_enable(struct intel_atomic_state *state,
 
 	/* If we change the relative order between pipe/planes enabling, we need
 	 * to change the workaround. */
-	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
+	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
 	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
 		struct intel_crtc *wa_crtc;
 
@@ -2007,6 +1992,29 @@  static void hsw_crtc_enable(struct intel_atomic_state *state,
 	}
 }
 
+static void hsw_crtc_enable(struct intel_atomic_state *state,
+			    struct intel_crtc *crtc)
+{
+	const struct intel_crtc_state *new_crtc_state =
+		intel_atomic_get_new_crtc_state(state, crtc);
+	struct intel_crtc *slave_crtc;
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+
+	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
+		return;
+
+	hsw_crtc_pre_pll_enable(state, new_crtc_state);
+
+	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
+					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
+		struct intel_crtc_state *slave_crtc_state =
+			intel_atomic_get_new_crtc_state(state, slave_crtc);
+
+		hsw_crtc_post_pll_enable(state, slave_crtc_state);
+	}
+	hsw_crtc_post_pll_enable(state, new_crtc_state);
+}
+
 void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
@@ -8122,11 +8130,11 @@  static void intel_enable_crtc(struct intel_atomic_state *state,
 
 	intel_crtc_update_active_timings(new_crtc_state);
 
-	dev_priv->display->crtc_enable(state, crtc);
-
 	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
 		return;
 
+	dev_priv->display->crtc_enable(state, crtc);
+
 	intel_drrs_enable(new_crtc_state);
 
 	/* vblanks work again, re-enable pipe CRC. */
@@ -8360,8 +8368,7 @@  static void skl_commit_modeset_enables(struct intel_atomic_state *state)
 			continue;
 
 		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
-		    is_trans_port_sync_master(new_crtc_state) ||
-		    intel_crtc_is_bigjoiner_master(new_crtc_state))
+		    is_trans_port_sync_master(new_crtc_state))
 			continue;
 
 		modeset_pipes &= ~BIT(pipe);
@@ -8371,7 +8378,7 @@  static void skl_commit_modeset_enables(struct intel_atomic_state *state)
 
 	/*
 	 * Then we enable all remaining pipes that depend on other
-	 * pipes: MST slaves and port sync masters, big joiner master
+	 * pipes: MST slaves and port sync masters
 	 */
 	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
 		enum pipe pipe = crtc->pipe;