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

[Ville Syrjälä]

On Fri, Mar 01, 2024 at 06:22:19PM +0200, Lisovskiy, Stanislav wrote:
> 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 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.
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala at 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.

Well, that's pretty much it's supposed to be done. As mentioned
I think the current code kinda works more by luck.

I suppose the only reason it works at all is that we do try to order
at least some of the steps via the tricks in
icl_ddi_bigjoiner_pre_enable() and the specific ordering of the crtcs
from the modeset_enables/disables(). But I'm pretty sure there are 
some steps that currently get done in different places for
the master and slace pipes. And that's not by design.

In general it's pretty hard to actually figure out what steps are
being done in which order in the current code.

The "is it OK?" question I think is best answered by asking the
real hardware. If there is some specific ordering requirement
that the current code accidentally gets right but the obvious
code would somehow get wrong, the hardware should be able to
tell us pretty quickly.

-- 
Ville Syrjälä
Intel