[PATCH 18/22] drm/i915: Handle joined pipes inside hsw_crtc_disable()

[Ville Syrjälä]

On Mon, Apr 01, 2024 at 06:46:20AM +0000, Murthy, Arun R wrote:
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of Ville
> > Syrjala
> > Sent: Friday, March 29, 2024 6:43 AM
> > To: intel-gfx at lists.freedesktop.org
> > Subject: [PATCH 18/22] drm/i915: Handle joined pipes inside hsw_crtc_disable()
> > 
> > From: Ville Syrjälä <ville.syrjala at 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: Use the name 'pipe_crtc' for the per-pipe crtc pointer
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 64 ++++++++++++--------
> >  1 file changed, 39 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 58ee40786d5c..c15ea046c62a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -1791,29 +1791,28 @@ static void hsw_crtc_disable(struct
> > intel_atomic_state *state,
> >  	const struct intel_crtc_state *old_crtc_state =
> >  		intel_atomic_get_old_crtc_state(state, crtc);
> >  	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> > +	struct intel_crtc *pipe_crtc;
> > 
> >  	/*
> >  	 * FIXME collapse everything to one hook.
> >  	 * Need care with mst->ddi interactions.
> >  	 */
> > -	if (!intel_crtc_is_bigjoiner_slave(old_crtc_state)) {
> > -		intel_encoders_disable(state, crtc);
> > -		intel_encoders_post_disable(state, crtc);
> > -	}
> > -
> > -	intel_disable_shared_dpll(old_crtc_state);
> > +	intel_encoders_disable(state, crtc);
> > +	intel_encoders_post_disable(state, crtc);
> > 
> > -	if (!intel_crtc_is_bigjoiner_slave(old_crtc_state)) {
> > -		struct intel_crtc *slave_crtc;
> > +	for_each_intel_crtc_in_pipe_mask(&i915->drm, pipe_crtc,
> > +
> > intel_crtc_joined_pipe_mask(old_crtc_state)) {
> > +		const struct intel_crtc_state *old_pipe_crtc_state =
> > +			intel_atomic_get_old_crtc_state(state, pipe_crtc);
> > 
> > -		intel_encoders_post_pll_disable(state, crtc);
> > +		intel_disable_shared_dpll(old_pipe_crtc_state);
> > +	}
> 
> As per the sequence is considered, should the pll be disabled prior to disabling the encoders and then followed by post_pll_disable?

The correct disable order is:
1. encoder disable()
2. disable transcoder/etc. (nop for hsw+ as that stuff
   has been sucked into the encoder hooks)
3. encoder post_disable()
4. pll disable
5. encoder post_pll_disable()

which we should be following here, thouh the diff is
rather hard to read due to the indentation changes.

-- 
Ville Syrjälä
Intel