Re: [Intel-gfx] [PATCH v2 2/3] drm/i915/dp: Compute port sync crtc states post compute_config()

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

Thanks for the review will make the changes and add your r-b

Manasi

On Wed, Feb 05, 2020 at 04:25:44PM +0200, Ville Syrjälä wrote:
> On Sun, Feb 02, 2020 at 11:47:55PM -0800, Manasi Navare wrote:
> > This patch pushes out the computation of master and slave
> > transcoders in crtc states after encoder's compute_config hook.
> > This ensures that the assigned master slave crtcs have exact same
> > mode and timings which is a requirement for Port sync mode
> > to be enabled.
> > 
> > v2:
> > * Correct indentation
> > * Rename to intel_ddi_port_sync_transcoders (Ville)
> > * remove unwanted debug (Ville)
> > 
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 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_ddi.c     | 109 +++++++++++++
> >  drivers/gpu/drm/i915/display/intel_display.c | 159 +------------------
> >  2 files changed, 110 insertions(+), 158 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index aa066fb9eb00..eb970797cd68 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -4448,6 +4448,114 @@ static int intel_ddi_compute_config(struct intel_encoder *encoder,
> >  	return 0;
> >  }
> >  
> > +static bool mode_equal(const struct drm_display_mode *mode1,
> > +		       const struct drm_display_mode *mode2)
> > +{
> > +	return drm_mode_match(mode1, mode2,
> > +			      DRM_MODE_MATCH_TIMINGS |
> > +			      DRM_MODE_MATCH_FLAGS |
> > +			      DRM_MODE_MATCH_3D_FLAGS) &&
> > +		mode1->clock == mode2->clock; /* we want an exact match */
> > +}
> > +
> > +static bool m_n_equal(const struct intel_link_m_n *m_n_1,
> > +		      const struct intel_link_m_n *m_n_2)
> > +{
> > +	return m_n_1->tu == m_n_2->tu &&
> > +		m_n_1->gmch_m == m_n_2->gmch_m &&
> > +		m_n_1->gmch_n == m_n_2->gmch_n &&
> > +		m_n_1->link_m == m_n_2->link_m &&
> > +		m_n_1->link_n == m_n_2->link_n;
> > +}
> > +
> > +static bool crtcs_port_sync_compatible(const struct intel_crtc_state *crtc_state1,
> > +				       const struct intel_crtc_state *crtc_state2)
> > +{
> > +	return crtc_state1->hw.active && crtc_state2->hw.active &&
> > +		crtc_state1->output_types == crtc_state2->output_types &&
> > +		crtc_state1->output_format == crtc_state2->output_format &&
> > +		crtc_state1->lane_count == crtc_state2->lane_count &&
> > +		crtc_state1->port_clock == crtc_state2->port_clock &&
> > +		mode_equal(&crtc_state1->hw.adjusted_mode,
> > +			   &crtc_state2->hw.adjusted_mode) &&
> > +		m_n_equal(&crtc_state1->dp_m_n, &crtc_state2->dp_m_n);
> > +}
> > +
> > +static u8
> > +intel_ddi_port_sync_transcoders(const struct intel_crtc_state *ref_crtc_state,
> > +				int tile_group_id)
> > +{
> > +	struct drm_connector *connector;
> > +	struct drm_connector_state *conn_state;
> 
> Can be const
> 
> > +	struct drm_i915_private *dev_priv = to_i915(ref_crtc_state->uapi.crtc->dev);
> > +	struct intel_atomic_state *state =
> > +		to_intel_atomic_state(ref_crtc_state->uapi.state);
> > +	u8 transcoders = 0;
> > +	int i;
> > +
> > +	if (INTEL_GEN(dev_priv) < 11)
> > +		return 0;
> > +
> > +	if (!intel_crtc_has_type(ref_crtc_state, INTEL_OUTPUT_DP))
> > +		return 0;
> > +
> > +	for_each_new_connector_in_state(&state->base, connector, conn_state, i) {
> > +		struct intel_crtc *crtc = to_intel_crtc(conn_state->crtc);
> > +		struct intel_crtc_state *crtc_state = NULL;
> 
> Can be const
> 
> Pointless NULL initialization.
> 
> > +
> > +		if (!crtc)
> > +			continue;
> > +
> > +		if (!(connector->has_tile &&
> > +		      connector->tile_group->id ==
> > +		      tile_group_id))
> 
> I'd write that as 
> if (!has_tile || id != group_id)
> 
> Looks like some pointless newlines in there. Makes it look a bit ugly.
> 
> Otherwise lgtm
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > +			continue;
> > +
> > +		crtc_state = intel_atomic_get_new_crtc_state(state,
> > +							     crtc);
> > +		if (!crtcs_port_sync_compatible(ref_crtc_state,
> > +						crtc_state))
> > +			continue;
> > +
> > +		transcoders |= BIT(crtc_state->cpu_transcoder);
> > +	}
> > +
> > +	return transcoders;
> > +}
> > +
> > +static int intel_ddi_compute_config_late(struct intel_encoder *encoder,
> > +					 struct intel_crtc_state *crtc_state,
> > +					 struct drm_connector_state *conn_state)
> > +{
> > +	struct drm_connector *connector = conn_state->connector;
> > +	u8 port_sync_transcoders = 0;
> > +
> > +	DRM_DEBUG_KMS("[ENCODER:%d:%s] [CRTC:%d:%s]",
> > +		      encoder->base.base.id, encoder->base.name,
> > +		      crtc_state->uapi.crtc->base.id, crtc_state->uapi.crtc->name);
> > +
> > +	if (connector->has_tile)
> > +		port_sync_transcoders = intel_ddi_port_sync_transcoders(crtc_state,
> > +									connector->tile_group->id);
> > +
> > +	/*
> > +	 * EDP Transcoders cannot be ensalved
> > +	 * make them a master always when present
> > +	 */
> > +	if (port_sync_transcoders & BIT(TRANSCODER_EDP))
> > +		crtc_state->master_transcoder = TRANSCODER_EDP;
> > +	else
> > +		crtc_state->master_transcoder = ffs(port_sync_transcoders) - 1;
> > +
> > +	if (crtc_state->master_transcoder == crtc_state->cpu_transcoder) {
> > +		crtc_state->master_transcoder = INVALID_TRANSCODER;
> > +		crtc_state->sync_mode_slaves_mask =
> > +			port_sync_transcoders & ~BIT(crtc_state->cpu_transcoder);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static void intel_ddi_encoder_destroy(struct drm_encoder *encoder)
> >  {
> >  	struct intel_digital_port *dig_port = enc_to_dig_port(to_intel_encoder(encoder));
> > @@ -4757,6 +4865,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
> >  	encoder->hotplug = intel_ddi_hotplug;
> >  	encoder->compute_output_type = intel_ddi_compute_output_type;
> >  	encoder->compute_config = intel_ddi_compute_config;
> > +	encoder->compute_config_late = intel_ddi_compute_config_late;
> >  	encoder->enable = intel_enable_ddi;
> >  	encoder->pre_pll_enable = intel_ddi_pre_pll_enable;
> >  	encoder->pre_enable = intel_ddi_pre_enable;
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 2a2c9dd563e5..a943787167de 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -12564,126 +12564,6 @@ 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 bool
> > -intel_atomic_is_master_connector(struct intel_crtc_state *crtc_state)
> > -{
> > -	struct drm_crtc *crtc = crtc_state->uapi.crtc;
> > -	struct drm_atomic_state *state = crtc_state->uapi.state;
> > -	struct drm_connector *connector;
> > -	struct drm_connector_state *connector_state;
> > -	int i;
> > -
> > -	for_each_new_connector_in_state(state, connector, connector_state, i) {
> > -		if (connector_state->crtc != crtc)
> > -			continue;
> > -		if (connector->has_tile &&
> > -		    connector->tile_h_loc == connector->num_h_tile - 1 &&
> > -		    connector->tile_v_loc == connector->num_v_tile - 1)
> > -			return true;
> > -	}
> > -
> > -	return false;
> > -}
> > -
> > -static void reset_port_sync_mode_state(struct intel_crtc_state *crtc_state)
> > -{
> > -	crtc_state->master_transcoder = INVALID_TRANSCODER;
> > -	crtc_state->sync_mode_slaves_mask = 0;
> > -}
> > -
> > -static int icl_compute_port_sync_crtc_state(struct drm_connector *connector,
> > -					    struct intel_crtc_state *crtc_state,
> > -					    int num_tiled_conns)
> > -{
> > -	struct drm_crtc *crtc = crtc_state->uapi.crtc;
> > -	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
> > -	struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> > -	struct drm_connector *master_connector;
> > -	struct drm_connector_list_iter conn_iter;
> > -	struct drm_crtc *master_crtc = NULL;
> > -	struct drm_crtc_state *master_crtc_state;
> > -	struct intel_crtc_state *master_pipe_config;
> > -
> > -	if (INTEL_GEN(dev_priv) < 11)
> > -		return 0;
> > -
> > -	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP))
> > -		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.
> > -	 * If all tiles not present do not make master slave assignments.
> > -	 */
> > -	if (!connector->has_tile ||
> > -	    crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> > -	    crtc_state->hw.mode.vdisplay != connector->tile_v_size ||
> > -	    num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
> > -		reset_port_sync_mode_state(crtc_state);
> > -		return 0;
> > -	}
> > -	/* Last Horizontal and last vertical tile connector is a master
> > -	 * Master's crtc state is already populated in slave for port sync
> > -	 */
> > -	if (connector->tile_h_loc == connector->num_h_tile - 1 &&
> > -	    connector->tile_v_loc == connector->num_v_tile - 1)
> > -		return 0;
> > -
> > -	/* Loop through all connectors and configure the Slave crtc_state
> > -	 * to point to the correct master.
> > -	 */
> > -	drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
> > -	drm_for_each_connector_iter(master_connector, &conn_iter) {
> > -		struct drm_connector_state *master_conn_state = NULL;
> > -
> > -		if (!(master_connector->has_tile &&
> > -		      master_connector->tile_group->id == connector->tile_group->id))
> > -			continue;
> > -		if (master_connector->tile_h_loc != master_connector->num_h_tile - 1 ||
> > -		    master_connector->tile_v_loc != master_connector->num_v_tile - 1)
> > -			continue;
> > -
> > -		master_conn_state = drm_atomic_get_connector_state(&state->base,
> > -								   master_connector);
> > -		if (IS_ERR(master_conn_state)) {
> > -			drm_connector_list_iter_end(&conn_iter);
> > -			return PTR_ERR(master_conn_state);
> > -		}
> > -		if (master_conn_state->crtc) {
> > -			master_crtc = master_conn_state->crtc;
> > -			break;
> > -		}
> > -	}
> > -	drm_connector_list_iter_end(&conn_iter);
> > -
> > -	if (!master_crtc) {
> > -		drm_dbg_kms(&dev_priv->drm,
> > -			    "Could not find Master CRTC for Slave CRTC %d\n",
> > -			    crtc->base.id);
> > -		return -EINVAL;
> > -	}
> > -
> > -	master_crtc_state = drm_atomic_get_crtc_state(&state->base,
> > -						      master_crtc);
> > -	if (IS_ERR(master_crtc_state))
> > -		return PTR_ERR(master_crtc_state);
> > -
> > -	master_pipe_config = to_intel_crtc_state(master_crtc_state);
> > -	crtc_state->master_transcoder = master_pipe_config->cpu_transcoder;
> > -	master_pipe_config->sync_mode_slaves_mask |=
> > -		BIT(crtc_state->cpu_transcoder);
> > -	drm_dbg_kms(&dev_priv->drm,
> > -		    "Master Transcoder = %s added for Slave CRTC = %d, slave transcoder bitmask = %d\n",
> > -		    transcoder_name(crtc_state->master_transcoder),
> > -		    crtc->base.id,
> > -		    master_pipe_config->sync_mode_slaves_mask);
> > -
> > -	return 0;
> > -}
> > -
> >  static u16 hsw_linetime_wm(const struct intel_crtc_state *crtc_state)
> >  {
> >  	const struct drm_display_mode *adjusted_mode =
> > @@ -13332,15 +13212,6 @@ intel_crtc_prepare_cleared_state(struct intel_crtc_state *crtc_state)
> >  	if (IS_G4X(dev_priv) ||
> >  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >  		saved_state->wm = crtc_state->wm;
> > -	/*
> > -	 * Save the slave bitmask which gets filled for master crtc state during
> > -	 * slave atomic check call. For all other CRTCs reset the port sync variables
> > -	 * crtc_state->master_transcoder needs to be set to INVALID
> > -	 */
> > -	reset_port_sync_mode_state(saved_state);
> > -	if (intel_atomic_is_master_connector(crtc_state))
> > -		saved_state->sync_mode_slaves_mask =
> > -			crtc_state->sync_mode_slaves_mask;
> >  
> >  	memcpy(crtc_state, saved_state, sizeof(*crtc_state));
> >  	kfree(saved_state);
> > @@ -13358,8 +13229,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> >  	struct drm_i915_private *i915 = to_i915(pipe_config->uapi.crtc->dev);
> >  	struct drm_connector *connector;
> >  	struct drm_connector_state *connector_state;
> > -	int base_bpp, ret;
> > -	int i, tile_group_id = -1, num_tiled_conns = 0;
> > +	int base_bpp, ret, i;
> >  	bool retry = true;
> >  
> >  	pipe_config->cpu_transcoder =
> > @@ -13431,24 +13301,6 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> >  	drm_mode_set_crtcinfo(&pipe_config->hw.adjusted_mode,
> >  			      CRTC_STEREO_DOUBLE);
> >  
> > -	/* Get tile_group_id of tiled connector */
> > -	for_each_new_connector_in_state(state, connector, connector_state, i) {
> > -		if (connector_state->crtc == crtc &&
> > -		    connector->has_tile) {
> > -			tile_group_id = connector->tile_group->id;
> > -			break;
> > -		}
> > -	}
> > -
> > -	/* Get total number of tiled connectors in state that belong to
> > -	 * this tile group.
> > -	 */
> > -	for_each_new_connector_in_state(state, connector, connector_state, i) {
> > -		if (connector->has_tile &&
> > -		    connector->tile_group->id == tile_group_id)
> > -			num_tiled_conns++;
> > -	}
> > -
> >  	/* Pass our mode to the connectors and the CRTC to give them a chance to
> >  	 * adjust it according to limitations or connector properties, and also
> >  	 * a chance to reject the mode entirely.
> > @@ -13460,15 +13312,6 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> >  		if (connector_state->crtc != crtc)
> >  			continue;
> >  
> > -		ret = icl_compute_port_sync_crtc_state(connector, pipe_config,
> > -						       num_tiled_conns);
> > -		if (ret) {
> > -			drm_dbg_kms(&i915->drm,
> > -				    "Cannot assign Sync Mode CRTCs: %d\n",
> > -				    ret);
> > -			return ret;
> > -		}
> > -
> >  		ret = encoder->compute_config(encoder, pipe_config,
> >  					      connector_state);
> >  		if (ret < 0) {
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx