Re: [PATCH 1/2] drm/i915/icl: Assign genlock CRTC pointer in all slave CRTC states for tiled displays

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

On Thu, Mar 28, 2019 at 11:32:18AM +0200, Jani Nikula wrote:
> On Thu, 21 Mar 2019, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > In case of tiled displays when the two tiles are sent across two CRTCs
> > over two separate DP SST connectors, we need a mechanism to synchronize
> > the two CRTCs and their corresponding transcoders.
> > So use the master-slave mode where there is one master corresponding
> > to last horizontal and vertical tile that needs to be genlocked with
> > all other slave tiles.
> > This patch identifies saves the master CRTC pointer in all the slave
> > CRTC states. This pointer is needed to select the master CRTC/transcoder
> > while configuring transcoder port sync for the corresponding slaves.
> >
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 84 ++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h     |  3 +
> >  2 files changed, 87 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 8ff7aa8cb3cf..9980a4ed8c9c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11281,6 +11281,86 @@ static int icl_check_nv12_planes(struct intel_crtc_state *crtc_state)
> >  	return 0;
> >  }
> >  
> > +static int icl_add_genlock_crtcs(struct drm_device *dev,
> > +				 struct drm_atomic_state *state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct drm_connector *genlock_connector, *connector;
> > +	struct drm_connector_state *connector_state;
> > +	struct drm_connector_list_iter conn_iter;
> > +	struct drm_crtc *genlock_crtc = NULL;
> > +	struct drm_crtc_state *genlock_crtc_state;
> > +	struct intel_crtc_state *slave_crtc_state;
> > +	int i, tile_group_id;
> > +
> > +	if (INTEL_GEN(dev_priv) < 11)
> > +		return 0;
> > +
> > +	/*
> > +	 * 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
> > +	 * to the last horizonal and last vertical tile a master/genlock CRTC.
> > +	 * All the other CRTCs corresponding to other tiles of the same Tile group
> > +	 * are the slave CRTCs and hold a pointer to their genlock CRTC.
> > +	 */
> > +	for_each_new_connector_in_state(state, connector, connector_state, i) {
> > +		if (!connector_state->crtc)
> > +			continue;
> > +		if (!connector->has_tile)
> > +			continue;
> > +		if (connector->tile_h_loc == connector->num_h_tile - 1 &&
> > +		    connector->tile_v_loc == connector->num_v_tile - 1)
> > +			continue;
> > +		slave_crtc_state = to_intel_crtc_state(
> > +			drm_atomic_get_new_crtc_state(state,
> > +						      connector_state->crtc));
> > +		slave_crtc_state->genlock_crtc = NULL;
> > +		tile_group_id = connector->tile_group->id;
> > +		drm_connector_list_iter_begin(dev, &conn_iter);
> > +		drm_for_each_connector_iter(genlock_connector, &conn_iter) {
> > +			struct drm_connector_state *genlock_conn_state = NULL;
> > +
> > +			if (!genlock_connector->has_tile)
> > +				continue;
> > +			if (genlock_connector->tile_h_loc != genlock_connector->num_h_tile - 1 ||
> > +			    genlock_connector->tile_v_loc != genlock_connector->num_v_tile - 1)
> > +				continue;
> > +			if (genlock_connector->tile_group->id != tile_group_id)
> > +				continue;
> > +
> > +			genlock_conn_state = drm_atomic_get_connector_state(state,
> > +									    genlock_connector);
> > +			if (IS_ERR(genlock_conn_state)) {
> > +				drm_connector_list_iter_end(&conn_iter);
> > +				return PTR_ERR(genlock_conn_state);
> > +			}
> > +			if (genlock_conn_state->crtc) {
> > +				genlock_crtc = genlock_conn_state->crtc;
> > +				break;
> > +			}
> > +		}
> > +		drm_connector_list_iter_end(&conn_iter);
> 
> The above loop would benefit from being abstracted to a separate
> function. "find genlock master based on tile info"
> 
> I wonder if it would work to have each relevant encoder ->compute_config
> hook look for its genlock master instead of adding another top level
> loop.

So are you suggesting adding this directly inside say compute_config() hook
for DP encoder so inside intel_dp_compute_config(), or would it be better to add
just the inner find_genlock_master() function in the existing for_each_connector() in intel_modeset_pipe_config()
before calling into the compute_config encoder hook?

Regards
Manasi
> 
> BR,
> Jani.
> 
> 
> > +
> > +		if (!genlock_crtc) {
> > +			DRM_DEBUG_KMS("Could not add Genlock CRTC for Slave CRTC %d\n",
> > +				      connector_state->crtc->base.id);
> > +			return -EINVAL;
> > +		}
> > +
> > +		genlock_crtc_state = drm_atomic_get_crtc_state(state,
> > +							       genlock_crtc);
> > +		if (IS_ERR(genlock_crtc_state))
> > +			return PTR_ERR(genlock_crtc_state);
> > +
> > +		slave_crtc_state->genlock_crtc = to_intel_crtc(genlock_crtc);
> > +		DRM_DEBUG_KMS("Genlock CRTC = %d added for Slave CRTC = %d\n",
> > +			      genlock_crtc->base.id,
> > +			      slave_crtc_state->base.crtc->base.id);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> >  				   struct drm_crtc_state *crtc_state)
> >  {
> > @@ -13098,6 +13178,10 @@ static int intel_atomic_check(struct drm_device *dev,
> >  	if (ret)
> >  		return ret;
> >  
> > +	ret = icl_add_genlock_crtcs(dev, state);
> > +	if (ret)
> > +		return ret;
> > +
> >  	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) {
> >  		struct intel_crtc_state *pipe_config =
> >  			to_intel_crtc_state(crtc_state);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 4d7ae579fc92..b41728946b73 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1074,6 +1074,9 @@ struct intel_crtc_state {
> >  
> >  	/* Forward Error correction State */
> >  	bool fec_enable;
> > +
> > +	/* Pointer to master/genlock crtc in case of tiled displays */
> > +	struct intel_crtc *genlock_crtc;
> >  };
> >  
> >  struct intel_crtc {
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx