Re: [PATCH v2 8/8] drm/i915: Handle joined pipes inside hsw_crtc_disable()

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Mon, Mar 04, 2024 at 08:44:35AM +0200, Srinivas, Vidya wrote:
> Thank you very much Ville and Stan.
> With https://patchwork.freedesktop.org/series/130619/ and https://patchwork.freedesktop.org/series/130449/ tested that 6K works
> Tested-by: Vidya Srinivas <vidya.srinivas@intel.com>

The thing is that we still don't handle crtc enable(i.e actual enabling of displays)
here at all, only disabling part. So fact that it works could be also related to this.

Ville, should I use your series, plus the things we had discussed in my series to implement
hsw_crtc_enable on top of your series?
Of course things related to transcoder have to be clarified still. 
Or do you plan to do it yourself?

Stan

> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville
> > Syrjala
> > Sent: Friday, March 1, 2024 10:54 PM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: Lisovskiy, Stanislav <stanislav.lisovskiy@intel.com>
> > Subject: [PATCH v2 8/8] drm/i915: Handle joined pipes inside
> > hsw_crtc_disable()
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Reorganize the crtc disable path to only deal with the master
> > pipes/transcoders in intel_old_crtc_state_disables() and offload the handling
> > of joined pipes to hsw_crtc_disable().
> > This makes the whole thing much more sensible since we can actually control
> > the order in which we do the per-pipe vs.
> > per-transcoder modeset steps.
> > 
> > v2: Pass the correct crtc pointer to .crtc_disable()
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 66 ++++++++++++--------
> >  1 file changed, 39 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 1df3923cc30d..e01536983303 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -1793,29 +1793,27 @@ static void hsw_crtc_disable(struct
> > intel_atomic_state *state,
> >  	const struct intel_crtc_state *old_master_crtc_state =
> >  		intel_atomic_get_old_crtc_state(state, master_crtc);
> >  	struct drm_i915_private *i915 = to_i915(master_crtc->base.dev);
> > +	u8 pipe_mask = intel_crtc_joined_pipe_mask(old_master_crtc_state);
> > +	struct intel_crtc *crtc;
> > 
> >  	/*
> >  	 * FIXME collapse everything to one hook.
> >  	 * Need care with mst->ddi interactions.
> >  	 */
> > -	if (!intel_crtc_is_bigjoiner_slave(old_master_crtc_state)) {
> > -		intel_encoders_disable(state, master_crtc);
> > -		intel_encoders_post_disable(state, master_crtc);
> > -	}
> > -
> > -	intel_disable_shared_dpll(old_master_crtc_state);
> > +	intel_encoders_disable(state, master_crtc);
> > +	intel_encoders_post_disable(state, master_crtc);
> > 
> > -	if (!intel_crtc_is_bigjoiner_slave(old_master_crtc_state)) {
> > -		struct intel_crtc *slave_crtc;
> > +	for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, pipe_mask) {
> > +		const struct intel_crtc_state *old_crtc_state =
> > +			intel_atomic_get_old_crtc_state(state, crtc);
> > 
> > -		intel_encoders_post_pll_disable(state, master_crtc);
> > +		intel_disable_shared_dpll(old_crtc_state);
> > +	}
> > 
> > -		intel_dmc_disable_pipe(i915, master_crtc->pipe);
> > +	intel_encoders_post_pll_disable(state, master_crtc);
> > 
> > -		for_each_intel_crtc_in_pipe_mask(&i915->drm, slave_crtc,
> > -
> > intel_crtc_bigjoiner_slave_pipes(old_master_crtc_state))
> > -			intel_dmc_disable_pipe(i915, slave_crtc->pipe);
> > -	}
> > +	for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, pipe_mask)
> > +		intel_dmc_disable_pipe(i915, crtc->pipe);
> >  }
> > 
> >  static void i9xx_pfit_enable(const struct intel_crtc_state *crtc_state) @@ -
> > 6753,24 +6751,33 @@ static void intel_update_crtc(struct intel_atomic_state
> > *state,  }
> > 
> >  static void intel_old_crtc_state_disables(struct intel_atomic_state *state,
> > -					  struct intel_crtc *crtc)
> > +					  struct intel_crtc *master_crtc)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > -	const struct intel_crtc_state *new_crtc_state =
> > -		intel_atomic_get_new_crtc_state(state, crtc);
> > +	const struct intel_crtc_state *old_master_crtc_state =
> > +		intel_atomic_get_old_crtc_state(state, master_crtc);
> > +	u8 pipe_mask = intel_crtc_joined_pipe_mask(old_master_crtc_state);
> > +	struct intel_crtc *crtc;
> > 
> >  	/*
> >  	 * We need to disable pipe CRC before disabling the pipe,
> >  	 * or we race against vblank off.
> >  	 */
> > -	intel_crtc_disable_pipe_crc(crtc);
> > +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, crtc, pipe_mask)
> > +		intel_crtc_disable_pipe_crc(crtc);
> > 
> > -	dev_priv->display.funcs.display->crtc_disable(state, crtc);
> > -	crtc->active = false;
> > -	intel_fbc_disable(crtc);
> > +	dev_priv->display.funcs.display->crtc_disable(state, master_crtc);
> > 
> > -	if (!new_crtc_state->hw.active)
> > -		intel_initial_watermarks(state, crtc);
> > +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, crtc, pipe_mask)
> > {
> > +		const struct intel_crtc_state *new_crtc_state =
> > +			intel_atomic_get_new_crtc_state(state, crtc);
> > +
> > +		crtc->active = false;
> > +		intel_fbc_disable(crtc);
> > +
> > +		if (!new_crtc_state->hw.active)
> > +			intel_initial_watermarks(state, crtc);
> > +	}
> >  }
> > 
> >  static void intel_commit_modeset_disables(struct intel_atomic_state *state)
> > @@ -6810,19 +6817,21 @@ static void
> > intel_commit_modeset_disables(struct intel_atomic_state *state)
> >  		if ((disable_pipes & BIT(crtc->pipe)) == 0)
> >  			continue;
> > 
> > +		if (intel_crtc_is_bigjoiner_slave(old_crtc_state))
> > +			continue;
> > +
> >  		/* In case of Transcoder port Sync master slave CRTCs can be
> >  		 * assigned in any order and we need to make sure that
> >  		 * slave CRTCs are disabled first and then master CRTC since
> >  		 * Slave vblanks are masked till Master Vblanks.
> >  		 */
> >  		if (!is_trans_port_sync_slave(old_crtc_state) &&
> > -		    !intel_dp_mst_is_slave_trans(old_crtc_state) &&
> > -		    !intel_crtc_is_bigjoiner_slave(old_crtc_state))
> > +		    !intel_dp_mst_is_slave_trans(old_crtc_state))
> >  			continue;
> > 
> >  		intel_old_crtc_state_disables(state, crtc);
> > 
> > -		disable_pipes &= ~BIT(crtc->pipe);
> > +		disable_pipes &=
> > ~intel_crtc_joined_pipe_mask(old_crtc_state);
> >  	}
> > 
> >  	/* Disable everything else left on */
> > @@ -6830,9 +6839,12 @@ static void intel_commit_modeset_disables(struct
> > intel_atomic_state *state)
> >  		if ((disable_pipes & BIT(crtc->pipe)) == 0)
> >  			continue;
> > 
> > +		if (intel_crtc_is_bigjoiner_slave(old_crtc_state))
> > +			continue;
> > +
> >  		intel_old_crtc_state_disables(state, crtc);
> > 
> > -		disable_pipes &= ~BIT(crtc->pipe);
> > +		disable_pipes &=
> > ~intel_crtc_joined_pipe_mask(old_crtc_state);
> >  	}
> > 
> >  	drm_WARN_ON(&i915->drm, disable_pipes);
> > --
> > 2.43.0
>