Re: [Intel-gfx] [PATCH 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present

[Manasi Navare <manasi.d.navare@intel.com>]

On Thu, Dec 12, 2019 at 05:03:07PM -0800, Matt Roper wrote:
> On Wed, Dec 11, 2019 at 01:14:24PM -0800, Manasi Navare wrote:
> > Add an extra check before making master slave assignments for tiled
> > displays to make sure we make these assignments only if all tiled
> > connectors are present. If not then initialize the state to defaults
> > so it does a normal non tiled modeset without transcoder port sync.
> > 
> > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 28 ++++++++++++++++++--
> >  1 file changed, 26 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 7263eaa66cda..c0a2dab3fe67 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -12048,6 +12048,12 @@ static bool c8_planes_changed(const struct intel_crtc_state *new_crtc_state)
> >  	return !old_crtc_state->c8_planes != !new_crtc_state->c8_planes;
> >  }
> >  
> > +static void initialize_trans_port_sync_mode_state(struct intel_crtc_state *crtc_state)
> 
> "reset" rather than "initialize" might be more consistent with similar
> things elsewhere in the driver?

Yes I was considering other names like default/reset/initialize, but yes I think
reset sounds better, will change that

> 
> > +{
> > +	crtc_state->master_transcoder = INVALID_TRANSCODER;
> > +	crtc_state->sync_mode_slaves_mask = 0;
> > +}
> > +
> >  static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
> >  {
> >  	struct drm_crtc *crtc = crtc_state->uapi.crtc;
> > @@ -12059,11 +12065,22 @@ static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
> >  	struct drm_crtc *master_crtc = NULL;
> >  	struct drm_crtc_state *master_crtc_state;
> >  	struct intel_crtc_state *master_pipe_config;
> > -	int i, tile_group_id;
> > +	int i, tile_group_id = 0, num_tiled_conns = 0;
> >  
> >  	if (INTEL_GEN(dev_priv) < 11)
> >  		return 0;
> >  
> > +	/* If all tiles not present do not make master slave assignments
> 
> This comment seems like it should go farther down the function; this
> block isn't directly responsible for master/slave assignments, so it's a
> bit confusing here.  Also, you might want to clarify what "present"
> means in this case.  E.g., explain that since you've already added all
> tiles of the same monitor to the transaction (earlier in
> intel_atomic_check), then if the number of tiles in the tile group is
> smaller than the size of the tile group it means that at least one of
> the tiles isn't active.
>

Yes will move this comment down
 
> > +	 * Here we assume all tiles belong to the same tile group for now.

Will add a FIXME
I will make these changes and add your r-b, thanks Matt

Manasi


> 
> Should this sentence be a FIXME:?  If we plug in 2x 2-tile monitors on
> TGL, this would be problematic, right?
> 
> The logic changes seem correct (if we assume that only a single tiled
> monitor is in use), so
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> 
> Matt
> 
> > +	 */
> > +	for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> > +		if (connector->has_tile) {
> > +			if (!tile_group_id)
> > +				tile_group_id = connector->tile_group->id;
> > +			num_tiled_conns++;
> > +		}
> > +	}
> > +
> >  	/*
> >  	 * In case of tiled displays there could be one or more slaves but there is
> >  	 * only one master. Lets make the CRTC used by the connector corresponding
> > @@ -12077,8 +12094,15 @@ static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
> >  		if (!connector->has_tile)
> >  			continue;
> >  		if (crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> > -		    crtc_state->hw.mode.vdisplay != connector->tile_v_size)
> > +		    crtc_state->hw.mode.vdisplay != connector->tile_v_size) {
> > +			initialize_trans_port_sync_mode_state(crtc_state);
> >  			return 0;
> > +		}
> > +		if (connector->tile_group->id == tile_group_id &&
> > +		    num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
> > +			initialize_trans_port_sync_mode_state(crtc_state);
> > +			return 0;
> > +		}
> >  		if (connector->tile_h_loc == connector->num_h_tile - 1 &&
> >  		    connector->tile_v_loc == connector->num_v_tile - 1)
> >  			continue;
> > -- 
> > 2.19.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> (916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx