[Intel-gfx] [PATCH v2 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset

[Ville Syrjälä]

On Fri, Dec 20, 2019 at 09:04:00AM -0800, Manasi Navare wrote:
> On Fri, Dec 20, 2019 at 06:47:46PM +0200, Ville Syrjälä wrote:
> > On Fri, Dec 20, 2019 at 08:35:58AM -0800, Manasi Navare wrote:
> > > On Fri, Dec 20, 2019 at 03:17:27PM +0200, Ville Syrjälä wrote:
> > > > On Thu, Dec 19, 2019 at 01:51:15PM -0800, Manasi Navare wrote:
> > > > > In case of tiled displays, all the tiles are linke dto each other
> > > > > for transcoder port sync. So in intel_atomic_check() we need to make
> > > > > sure that we add all the tiles to the modeset and if one of the
> > > > > tiles needs a full modeset then mark all other tiles for a full modeset.
> > > > > 
> > > > > v2:
> > > > > * Change crtc_state scope, remove tile_grp_id (Ville)
> > > > > * Use intel_connector_needs_modeset() (Ville)
> > > > > * Add modeset_synced_crtcs (Ville)
> > > > > * Make sure synced crtcs are forced full modeset
> > > > > after fastset check (Ville)
> > > > > 
> > > > > Suggested-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > Cc: José Roberto de Souza <jose.souza at intel.com>
> > > > > Cc: Matt Roper <matthew.d.roper at intel.com>
> > > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > > > Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_display.c | 143 +++++++++++++++++--
> > > > >  1 file changed, 131 insertions(+), 12 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > index a3f9430493ae..00608d8cef50 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > @@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
> > > > >  	new_crtc_state->uapi.mode_changed = false;
> > > > >  	new_crtc_state->update_pipe = true;
> > > > >  
> > > > > -	/*
> > > > > -	 * If we're not doing the full modeset we want to
> > > > > -	 * keep the current M/N values as they may be
> > > > > -	 * sufficiently different to the computed values
> > > > > -	 * to cause problems.
> > > > > -	 *
> > > > > -	 * FIXME: should really copy more fuzzy state here
> > > > > -	 */
> > > > > -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > > > > -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > > > > -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > > > > -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > > > >  }
> > > > 
> > > > The check vs. copy should really be a separate patch. Otherwise we risk
> > > > having to revert the whole thing if something goes wrong.
> > > >
> > > 
> > > Yes will sepaarte it out , also I think Jose's series might get merged before mine so..
> > > 
> > >  
> > > > >  
> > > > >  static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
> > > > > @@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > +static void
> > > > > +intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state)
> > > > 
> > > > I would remove "dp" from the names of all these functions. The fact
> > > > that we only enable port sync on DP outputs is just an implementation
> > > > detail we don't have/want to care about in higher level code like this.
> > > >
> > > 
> > > Okay will rename
> > >  
> > > > > +{
> > > > > +	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 (is_trans_port_sync_mode(new_crtc_state)) {
> > > > > +			new_crtc_state->uapi.mode_changed = true;
> > > > > +			new_crtc_state->update_pipe = false;
> > > > > +		}
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +static void
> > > > > +intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state)
> > > > > +{
> > > > > +	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 (!is_trans_port_sync_mode(new_crtc_state) ||
> > > > > +		    !needs_modeset(new_crtc_state))
> > > > > +			continue;
> > > > > +
> > > > > +		intel_dp_modeset_synced_crtcs(state);
> > > > > +	}
> > > > 
> > > > This is not sufficient for the pre-compute_config() check. The point is
> > > > that we don't yet necessarily have the synced crtcs in the atomic state
> > > > so we have to add them. Please have a look in my branch for what I mean
> > > > exactly.
> > > >
> > > 
> > > But if I use old crtc state or new crtc state here, I do see that since we havent
> > > cleared the state yet, we do have all synced crtcs in the state , like here after I disconnect 1 conn,
> > > I do see both conns in trans port sync mode and i can set the mode changed to true for both.
> > > 
> > > I dont understand why what you mean by we would beed to add them?
> > 
> > for_each_new_intel_crtc_in_state() only iterates the crtcs we've already
> > added to the state, not all crtcs. The whole point of the
> > pre-compute_config() check is that we add the ones that were previously
> > synced but weren't yet added to the state. This for the case where the
> > tile info has gone poof so the function that adds stuff based on the
> > tile info will have missed them.
> >
> 
> But it would have been previously synced and that info would still be in new_crtc_state->master_trans and slve_mask
> since this is before intel_crtc_prepare_cleared_state()

You don't even have that new_crtc_state yet for the crtcs that were
synced previously but whose tile info vanished in the meantime.
That's the whole point here.

-- 
Ville Syrjälä
Intel