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

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

On Fri, Mar 01, 2024 at 06:10:25PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 01, 2024 at 06:04:27PM +0200, Lisovskiy, Stanislav wrote:
> > On Fri, Mar 01, 2024 at 04:36:00PM +0200, Ville Syrjala wrote:
> > > 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.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 64 ++++++++++++--------
> > >  1 file changed, 38 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 1df3923cc30d..07239c1ce9df 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);
> > >  }
> > 
> > Okay the only difference from hsw_crtc_disable part from my patch is that
> > I don't have intel_crtc_joined_pipe_mask and encoder calls are outside the pipe
> > loop. Ok. You could of course just communicate this to me, it is quite a small
> > thing to change.
> > 
> > And still there is a question about how to handle the crtc enable side, since
> > extracting transcoder programming from the pipe loop, will break the sequence,
> > as I described. Either it is ok that we will partly program slave/master pipe, then
> > program transcoder then again program slave/master pipes or it has to be
> > in a pipe loop.
> 
> Transcoder stuff shouldn't be in pipe loops. That's what
> I've been saying all along.

Yep, I realize you kept saying this and I described you the problem what happens if 
we extract it from there.
Either it is ok to have 2 loops and have transcoder programming in between or you
first program pipes then program the transcoder - in both cases that would change
the sequence of how it is done now.
My question was if this is ok or not.

Stan

> 
> -- 
> Ville Syrjälä
> Intel