Re: [Intel-gfx] [PATCH] drm/i915: Fix the disabling sequence for Bigjoiner

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

On Wed, Jul 05, 2023 at 06:32:51PM +0300, Imre Deak wrote:
> On Thu, May 25, 2023 at 01:10:36PM +0300, Stanislav Lisovskiy wrote:
> > According to BSpec 49190, when enabling crtcs, we first setup
> > slave and then master crtc, however for disabling it should go
> > vice versa, i.e first master, then slave, however current code
> > does disabling in a same way as enabling. Fix this, by skipping
> > non-master crtcs, instead of non-slaves.
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 0490c6412ab5..68958ba0ef49 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -6662,7 +6662,7 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
> >  		 */
> >  		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_crtc_is_bigjoiner_master(old_crtc_state))
> 
> I don't see what does this fix. The sequence is correct at the moment
> and this change would break it, leaving the encoder PLL enabled
> incorrectly when the encoder->post_pll_disable() hook is called. Hence
> it's NAK from side.

Citing BSpec:

Follow the steps to disable Planes, Pipe, and Transcoder for pipe B and transcoder B(master)
Follow the steps to Disable Planes and Pipe for pipe C(slave)
Disable port associated with Transcoder B

however because of !intel_crtc_is_bigjoiner_slave(old_crtc_state), we are skipping masters
and do disabling for the slave crtc first.

If fixing that going to break something - that is another story(anyway doesn't mean we shouldn't fix)

Stan

> 
> >  			continue;
> >  
> >  		intel_old_crtc_state_disables(state, old_crtc_state,
> > -- 
> > 2.37.3
> >