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

[Imre Deak <imre.deak@intel.com>]

On Thu, Jul 06, 2023 at 11:24:21AM +0300, Lisovskiy, Stanislav wrote:
> 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.
> 
> Well, as I pointed out the BSpec 49190 instructs us to disable master
> first, then slave. Current code skips all non-slaves in first cycle,
> i.e it disables first slaves and then masters. Which is _wrong_.

This is correct at the moment, followed in the encoder's disable hook
which is only assigned to the master CRTC.

> Anything else in particular, where do you need clarifications?
> 
> Stan
> 
> > 
> > >  			continue;
> > >  
> > >  		intel_old_crtc_state_disables(state, old_crtc_state,
> > > -- 
> > > 2.37.3
> > >