Re: [Intel-gfx] [PATCH 1/5] drm/i915: Fix post-fastset modeset check for port sync

["Souza, Jose" <jose.souza@intel.com>]

On Thu, 2020-01-16 at 12:56 +0200, Ville Syrjälä wrote:
> On Thu, Jan 16, 2020 at 12:05:43AM +0000, Souza, Jose wrote:
> > On Wed, 2020-01-15 at 21:08 +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > The post-fastset "does anyone still need a full modeset?" for
> > > port sync looks busted. The outer loop bails out of a full
> > > modeset
> > > is still needed by the current crtc which, and then we skip
> > > forcing
> > > a full modeset on the related crtcs. That's totally the opposite
> > > of what we want.
> > 
> > Ops yeah, it is always doing a full modeset.
> > 
> > > The MST path has the logic mostly the other way around so it
> > > looks correct. To fix the port sync case let's follow the MST
> > > logic for both. So, if the current crtc already needs a modeset
> > > we do nothing. otherwise we check if any of the related crtcs
> > > needs a modeset, and if so we force a full modeset for the
> > > current crtc.
> > > 
> > > And while at let's change the else if to a plain if to so
> > > we don't have needless coupling between the MST and port sync
> > > checks.
> > > 
> > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > Fixes: 05a8e45136ca ("drm/i915/display: Use external dependency
> > > loop
> > > for port sync")
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 43 ++++++++----
> > > ----
> > > ----
> > >  1 file changed, 17 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index dd03987cc24f..b397816ce253 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -14469,37 +14469,23 @@ static int
> > > intel_atomic_check_crtcs(struct
> > > intel_atomic_state *state)
> > >  	return 0;
> > >  }
> > >  
> > > -static bool intel_cpu_transcoder_needs_modeset(struct
> > > intel_atomic_state *state,
> > > -					       enum transcoder
> > > transcoder)
> > > +static bool intel_cpu_transcoders_need_modeset(struct
> > > intel_atomic_state *state,
> > > +					       u8 transcoders)
> > >  {
> > > -	struct intel_crtc_state *new_crtc_state;
> > > +	const struct intel_crtc_state *new_crtc_state;
> > >  	struct intel_crtc *crtc;
> > >  	int i;
> > >  
> > > -	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > > i)
> > > -		if (new_crtc_state->cpu_transcoder == transcoder)
> > > -			return needs_modeset(new_crtc_state);
> > > +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > > i) {
> > > +		if (new_crtc_state->hw.enable &&
> > 
> > The intel_pipe_config_compare() will force a modeset for MST and
> > port
> > sync but this hw.enable could skip modeset in future features when
> > one
> > of the pipes are going from enabled to disabled.
> 
> We can't trust what's in .cpu_transcoder when enable==false. It can
> be some stale junk left over from the previous state. Well, until
> the next patch at least.
> 
> If the disabled pipe was the master then the mst/port sync master
> transcoder will change and the fastset check won't pass on any of the
> slaves. So the enable check is fine for that case.
> 
> If the disabled pipe was one of the port sync slaves the relevant
> bitmask will change for the master, again making the fastset check
> fail. And thus this code here will force a modeset for all the
> other slaves.
> 
> So can't see how having the check could be a problem.

I know this will not affect MST and port sync, I was thinking in future
users of this function but okay, we can leave that to be worked when
such users shows up.

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> 
> > Removing it:
> > 
> > Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> > 
> > > +		    transcoders & BIT(new_crtc_state->cpu_transcoder)
> > > &&
> > > +		    needs_modeset(new_crtc_state))
> > > +			return true;
> > > +	}
> > >  
> > >  	return false;
> > >  }
> > >  
> > > -static void
> > > -intel_modeset_synced_crtcs(struct intel_atomic_state *state,
> > > -			   u8 transcoders)
> > > -{
> > > -	struct intel_crtc_state *new_crtc_state;
> > > -	struct intel_crtc *crtc;
> > > -	int i;
> > > -
> > > -	for_each_new_intel_crtc_in_state(state, crtc,
> > > -					 new_crtc_state, i) {
> > > -		if (transcoders & BIT(new_crtc_state->cpu_transcoder))
> > > {
> > > -			new_crtc_state->uapi.mode_changed = true;
> > > -			new_crtc_state->update_pipe = false;
> > > -		}
> > > -	}
> > > -}
> > > -
> > >  static int
> > >  intel_modeset_all_tiles(struct intel_atomic_state *state, int
> > > tile_grp_id)
> > >  {
> > > @@ -14655,15 +14641,20 @@ static int intel_atomic_check(struct
> > > drm_device *dev,
> > >  		if (intel_dp_mst_is_slave_trans(new_crtc_state)) {
> > >  			enum transcoder master = new_crtc_state-
> > > > mst_master_transcoder;
> > >  
> > > -			if (intel_cpu_transcoder_needs_modeset(state,
> > > master)) {
> > > +			if (intel_cpu_transcoders_need_modeset(state,
> > > BIT(master))) {
> > >  				new_crtc_state->uapi.mode_changed =
> > > true;
> > >  				new_crtc_state->update_pipe = false;
> > >  			}
> > > -		} else if (is_trans_port_sync_mode(new_crtc_state)) {
> > > +		}
> > > +
> > > +		if (is_trans_port_sync_mode(new_crtc_state)) {
> > >  			u8 trans = new_crtc_state-
> > > > sync_mode_slaves_mask |
> > >  				   BIT(new_crtc_state-
> > > > master_transcoder);
> > >  
> > > -			intel_modeset_synced_crtcs(state, trans);
> > > +			if (intel_cpu_transcoders_need_modeset(state,
> > > trans)) {
> > > +				new_crtc_state->uapi.mode_changed =
> > > true;
> > > +				new_crtc_state->update_pipe = false;
> > > +			}
> > >  		}
> > >  	}
> > >  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx