Re: [Intel-gfx] [PATCH v5 3/7] drm/i915/tgl: Select master transcoder for MST stream

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Dec 20, 2019 at 07:29:50AM -0800, José Roberto de Souza wrote:
> On TGL the blending of all the streams have moved from DDI to
> transcoder, so now every transcoder working over the same MST port must
> send its stream to a master transcoder and master will send to DDI
> respecting the time slots.
> 
> So here adding all the CRTCs that shares the same MST stream if
> needed and computing their state again, it will pick the lowest
> pipe/transcoder among the ones in the same stream to be master.
> 
> Most of the time skl_commit_modeset_enables() enables pipes in a
> crescent order but due DDB overlapping it might not happen, this
> scenarios will be handled in the next patch.
> 
> v2:
> - Using recently added intel_crtc_state_reset() to set
> mst_master_transcoder to invalid transcoder for all non gen12 & MST
> code paths
> - Setting lowest pipe/transcoder as master, previously it was the
> first one but setting a predictable one will help in future MST e
> port sync integration
> - Moving to intel type as much as we can
> 
> v3:
> - Now intel_dp_mst_master_trans_compute() returns the MST master transcoder
> - Replaced stdbool.h by linux/types.h
> - Skip the connector being checked in
> intel_dp_mst_atomic_master_trans_check()
> - Using pipe instead of transcoder to compute MST master
> 
> v4:
> - renamed connector_state to conn_state
> 
> v5:
> - Improved the parameters of intel_dp_mst_master_trans_compute() to
> simply code
> - Added call drm_atomic_add_affected_planes() in
> intel_dp_mst_atomic_master_trans_check() as helper could not do it
> for us
> - Removed "if (ret)" left over from v3 changes
> 
> BSpec: 50493
> BSpec: 49190
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_atomic.c   |  14 ++
>  drivers/gpu/drm/i915/display/intel_atomic.h   |   4 +
>  drivers/gpu/drm/i915/display/intel_ddi.c      |  14 +-
>  drivers/gpu/drm/i915/display/intel_display.c  |  12 +-
>  .../drm/i915/display/intel_display_types.h    |   3 +
>  drivers/gpu/drm/i915/display/intel_dp_mst.c   | 140 ++++++++++++++++--
>  drivers/gpu/drm/i915/display/intel_dp_mst.h   |   5 +
>  7 files changed, 179 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> index b7dda18b6f29..0eb973f65977 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -192,6 +192,20 @@ intel_connector_needs_modeset(struct intel_atomic_state *state,
>  									    new_conn_state->crtc)));
>  }
>  
> +struct intel_digital_connector_state *
> +intel_atomic_get_digital_connector_state(struct intel_atomic_state *state,
> +					 struct intel_connector *connector)
> +{
> +	struct drm_connector_state *conn_state;
> +
> +	conn_state = drm_atomic_get_connector_state(&state->base,
> +						    &connector->base);
> +	if (IS_ERR(conn_state))
> +		return ERR_CAST(conn_state);
> +
> +	return to_intel_digital_connector_state(conn_state);
> +}
> +
>  /**
>   * intel_crtc_duplicate_state - duplicate crtc state
>   * @crtc: drm crtc
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h b/drivers/gpu/drm/i915/display/intel_atomic.h
> index a7d1a8576c48..74c749dbfb4f 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> @@ -17,6 +17,7 @@ struct drm_device;
>  struct drm_i915_private;
>  struct drm_property;
>  struct intel_atomic_state;
> +struct intel_connector;
>  struct intel_crtc;
>  struct intel_crtc_state;
>  
> @@ -34,6 +35,9 @@ struct drm_connector_state *
>  intel_digital_connector_duplicate_state(struct drm_connector *connector);
>  bool intel_connector_needs_modeset(struct intel_atomic_state *state,
>  				   struct drm_connector *connector);
> +struct intel_digital_connector_state *
> +intel_atomic_get_digital_connector_state(struct intel_atomic_state *state,
> +					 struct intel_connector *connector);
>  
>  struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc);
>  void intel_crtc_destroy_state(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index c9ba7d7f3787..c3ac950e79a8 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -1899,8 +1899,13 @@ intel_ddi_transcoder_func_reg_val_get(const struct intel_crtc_state *crtc_state)
>  		temp |= TRANS_DDI_MODE_SELECT_DP_MST;
>  		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
>  
> -		if (INTEL_GEN(dev_priv) >= 12)
> -			temp |= TRANS_DDI_MST_TRANSPORT_SELECT(crtc_state->cpu_transcoder);
> +		if (INTEL_GEN(dev_priv) >= 12) {
> +			enum transcoder master;
> +
> +			master = crtc_state->mst_master_transcoder;
> +			WARN_ON(master == INVALID_TRANSCODER);
> +			temp |= TRANS_DDI_MST_TRANSPORT_SELECT(master);
> +		}
>  	} else {
>  		temp |= TRANS_DDI_MODE_SELECT_DP_SST;
>  		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> @@ -4405,6 +4410,11 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>  		pipe_config->output_types |= BIT(INTEL_OUTPUT_DP_MST);
>  		pipe_config->lane_count =
>  			((temp & DDI_PORT_WIDTH_MASK) >> DDI_PORT_WIDTH_SHIFT) + 1;
> +
> +		if (INTEL_GEN(dev_priv) >= 12)
> +			pipe_config->mst_master_transcoder =
> +					REG_FIELD_GET(TRANS_DDI_MST_TRANSPORT_SELECT_MASK, temp);
> +
>  		intel_dp_get_m_n(intel_crtc, pipe_config);
>  		break;
>  	default:
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index fc77829ea958..eb97ad562c96 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -46,6 +46,7 @@
>  #include "display/intel_crt.h"
>  #include "display/intel_ddi.h"
>  #include "display/intel_dp.h"
> +#include "display/intel_dp_mst.h"
>  #include "display/intel_dsi.h"
>  #include "display/intel_dvo.h"
>  #include "display/intel_gmbus.h"
> @@ -11627,6 +11628,7 @@ static void intel_crtc_state_reset(struct intel_crtc_state *crtc_state,
>  	crtc_state->hsw_workaround_pipe = INVALID_PIPE;
>  	crtc_state->output_format = INTEL_OUTPUT_FORMAT_INVALID;
>  	crtc_state->scaler_state.scaler_id = -1;
> +	crtc_state->mst_master_transcoder = INVALID_TRANSCODER;
>  }
>  
>  static struct intel_crtc_state *intel_crtc_state_alloc(struct intel_crtc *crtc)
> @@ -12484,6 +12486,9 @@ static void intel_dump_pipe_config(const struct intel_crtc_state *pipe_config,
>  			      pipe_config->csc_mode, pipe_config->gamma_mode,
>  			      pipe_config->gamma_enable, pipe_config->csc_enable);
>  
> +	DRM_DEBUG_KMS("MST master transcoder: %s\n",
> +		      transcoder_name(pipe_config->mst_master_transcoder));
> +
>  dump_planes:
>  	if (!state)
>  		return;
> @@ -13264,6 +13269,8 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  	PIPE_CONF_CHECK_I(dsc.dsc_split);
>  	PIPE_CONF_CHECK_I(dsc.compressed_bpp);
>  
> +	PIPE_CONF_CHECK_I(mst_master_transcoder);
> +
>  #undef PIPE_CONF_CHECK_X
>  #undef PIPE_CONF_CHECK_I
>  #undef PIPE_CONF_CHECK_BOOL
> @@ -14348,7 +14355,7 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
>  	u32 handled = 0;
>  	int i;
>  
> -	/* Only disable port sync slaves */
> +	/* Only disable port sync and MST slaves */
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  					    new_crtc_state, i) {
>  		if (!needs_modeset(new_crtc_state))
> @@ -14362,7 +14369,8 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
>  		 * slave CRTCs are disabled first and then master CRTC since
>  		 * Slave vblanks are masked till Master Vblanks.
>  		 */
> -		if (!is_trans_port_sync_slave(old_crtc_state))
> +		if (!is_trans_port_sync_slave(old_crtc_state) &&
> +		    !intel_dp_mst_is_slave_trans(old_crtc_state))
>  			continue;
>  
>  		intel_pre_plane_update(state, crtc);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 83ea04149b77..630a94892b7b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1054,6 +1054,9 @@ struct intel_crtc_state {
>  
>  	/* Bitmask to indicate slaves attached */
>  	u8 sync_mode_slaves_mask;
> +
> +	/* Only valid on TGL+ */
> +	enum transcoder mst_master_transcoder;
>  };
>  
>  struct intel_crtc {
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 7aa0975c33b7..a6237da9ac52 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -87,10 +87,53 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
>  	return 0;
>  }
>  
> +/*
> + * Iterate over all connectors and return the smallest transcoder in the MST
> + * stream
> + */
> +static enum transcoder
> +intel_dp_mst_master_trans_compute(struct intel_atomic_state *state,
> +				  struct intel_dp *mst_port)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	struct intel_digital_connector_state *conn_state;
> +	struct intel_connector *connector;
> +	enum pipe ret = I915_MAX_PIPES;
> +	int i;
> +
> +	if (INTEL_GEN(dev_priv) < 12)
> +		return INVALID_TRANSCODER;
> +
> +	for_each_new_intel_connector_in_state(state, connector, conn_state, i) {
> +		struct intel_crtc_state *crtc_state;
> +		struct intel_crtc *crtc;
> +
> +		if (connector->mst_port != mst_port || !conn_state->base.crtc)
> +			continue;
> +
> +		crtc = to_intel_crtc(conn_state->base.crtc);
> +		crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
> +		if (!crtc_state->uapi.active)
> +			continue;
> +
> +		/*
> +		 * Using crtc->pipe because crtc_state->cpu_transcoder is
> +		 * computed, so others CRTCs could have non-computed
> +		 * cpu_transcoder
> +		 */
> +		if (crtc->pipe < ret)
> +			ret = crtc->pipe;
> +	}

Still has the problem that ret==I915_MAX_PIPES if no active crtcs in the
state.

I guess a simple
if (ret == MAX_PIPES)
	return INVALID;
should do here. Or something similar.

With that fixed this is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Future thoughts based on what I prototyped in
git://github.com/vsyrjala/linux.git port_sync_stuff
would be to simply compute the bitmask of transcoders here, and then
let the caller pick the first with ffs()-1. We could potentially
optimize that approach a bit by only computing each distinct bitmask
only once and storing them temporarult in eg. intel_atomic_state.
Then each encoder would just assign:
mst_master = ffs(precomputed_mst_bitmask[cpu_transcoder]) - 1;


> +
> +	/* Simple cast works because TGL don't have a eDP transcoder */
> +	return (enum transcoder)ret;
> +}
> +
>  static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  				       struct intel_crtc_state *pipe_config,
>  				       struct drm_connector_state *conn_state)
>  {
> +	struct intel_atomic_state *state = to_intel_atomic_state(conn_state->state);
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
>  	struct intel_dp *intel_dp = &intel_mst->primary->dp;
> @@ -154,24 +197,91 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  
>  	intel_ddi_compute_min_voltage_level(dev_priv, pipe_config);
>  
> +	pipe_config->mst_master_transcoder = intel_dp_mst_master_trans_compute(state, intel_dp);
> +
> +	return 0;
> +}
> +
> +/*
> + * If one of the connectors in a MST stream needs a modeset, mark all CRTCs
> + * that shares the same MST stream as mode changed,
> + * intel_modeset_pipe_config()+intel_crtc_check_fastset() will take care to do
> + * a fastset when possible.
> + */
> +static int
> +intel_dp_mst_atomic_master_trans_check(struct intel_connector *connector,
> +				       struct intel_atomic_state *state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	struct drm_connector_list_iter connector_list_iter;
> +	struct intel_connector *connector_iter;
> +
> +	if (INTEL_GEN(dev_priv) < 12)
> +		return  0;
> +
> +	if (!intel_connector_needs_modeset(state, &connector->base))
> +		return 0;
> +
> +	drm_connector_list_iter_begin(&dev_priv->drm, &connector_list_iter);
> +	for_each_intel_connector_iter(connector_iter, &connector_list_iter) {
> +		struct intel_digital_connector_state *conn_iter_state;
> +		struct intel_crtc_state *crtc_state;
> +		struct intel_crtc *crtc;
> +		int ret;
> +
> +		if (connector_iter->mst_port != connector->mst_port ||
> +		    connector_iter == connector)
> +			continue;
> +
> +		conn_iter_state = intel_atomic_get_digital_connector_state(state,
> +									   connector_iter);
> +		if (IS_ERR(conn_iter_state)) {
> +			drm_connector_list_iter_end(&connector_list_iter);
> +			return PTR_ERR(conn_iter_state);
> +		}
> +
> +		if (!conn_iter_state->base.crtc)
> +			continue;
> +
> +		crtc = to_intel_crtc(conn_iter_state->base.crtc);
> +		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
> +		if (IS_ERR(crtc_state)) {
> +			drm_connector_list_iter_end(&connector_list_iter);
> +			return PTR_ERR(crtc_state);
> +		}
> +
> +		ret = drm_atomic_add_affected_planes(&state->base, &crtc->base);
> +		if (ret) {
> +			drm_connector_list_iter_end(&connector_list_iter);
> +			return ret;
> +		}
> +		crtc_state->uapi.mode_changed = true;
> +	}
> +	drm_connector_list_iter_end(&connector_list_iter);
> +
>  	return 0;
>  }
>  
>  static int
>  intel_dp_mst_atomic_check(struct drm_connector *connector,
> -			  struct drm_atomic_state *state)
> +			  struct drm_atomic_state *_state)
>  {
> +	struct intel_atomic_state *state = to_intel_atomic_state(_state);
>  	struct drm_connector_state *new_conn_state =
> -		drm_atomic_get_new_connector_state(state, connector);
> +		drm_atomic_get_new_connector_state(&state->base, connector);
>  	struct drm_connector_state *old_conn_state =
> -		drm_atomic_get_old_connector_state(state, connector);
> +		drm_atomic_get_old_connector_state(&state->base, connector);
>  	struct intel_connector *intel_connector =
>  		to_intel_connector(connector);
>  	struct drm_crtc *new_crtc = new_conn_state->crtc;
>  	struct drm_dp_mst_topology_mgr *mgr;
>  	int ret;
>  
> -	ret = intel_digital_connector_atomic_check(connector, state);
> +	ret = intel_digital_connector_atomic_check(connector, &state->base);
> +	if (ret)
> +		return ret;
> +
> +	ret = intel_dp_mst_atomic_master_trans_check(intel_connector, state);
>  	if (ret)
>  		return ret;
>  
> @@ -182,12 +292,9 @@ intel_dp_mst_atomic_check(struct drm_connector *connector,
>  	 * connector
>  	 */
>  	if (new_crtc) {
> -		struct intel_atomic_state *intel_state =
> -			to_intel_atomic_state(state);
>  		struct intel_crtc *intel_crtc = to_intel_crtc(new_crtc);
>  		struct intel_crtc_state *crtc_state =
> -			intel_atomic_get_new_crtc_state(intel_state,
> -							intel_crtc);
> +			intel_atomic_get_new_crtc_state(state, intel_crtc);
>  
>  		if (!crtc_state ||
>  		    !drm_atomic_crtc_needs_modeset(&crtc_state->uapi) ||
> @@ -196,7 +303,7 @@ intel_dp_mst_atomic_check(struct drm_connector *connector,
>  	}
>  
>  	mgr = &enc_to_mst(old_conn_state->best_encoder)->primary->dp.mst_mgr;
> -	ret = drm_dp_atomic_release_vcpi_slots(state, mgr,
> +	ret = drm_dp_atomic_release_vcpi_slots(&state->base, mgr,
>  					       intel_connector->port);
>  
>  	return ret;
> @@ -240,6 +347,8 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
>  
>  	intel_dp->active_mst_links--;
>  	last_mst_stream = intel_dp->active_mst_links == 0;
> +	WARN_ON(INTEL_GEN(dev_priv) >= 12 && last_mst_stream &&
> +		!intel_dp_mst_is_master_trans(old_crtc_state));
>  
>  	intel_crtc_vblank_off(old_crtc_state);
>  
> @@ -317,6 +426,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>  	connector->encoder = encoder;
>  	intel_mst->connector = connector;
>  	first_mst_stream = intel_dp->active_mst_links == 0;
> +	WARN_ON(INTEL_GEN(dev_priv) >= 12 && first_mst_stream &&
> +		!intel_dp_mst_is_master_trans(pipe_config));
>  
>  	DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
>  
> @@ -722,3 +833,14 @@ intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port)
>  	drm_dp_mst_topology_mgr_destroy(&intel_dp->mst_mgr);
>  	/* encoders will get killed by normal cleanup */
>  }
> +
> +bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state)
> +{
> +	return crtc_state->mst_master_transcoder == crtc_state->cpu_transcoder;
> +}
> +
> +bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state)
> +{
> +	return crtc_state->mst_master_transcoder != INVALID_TRANSCODER &&
> +	       crtc_state->mst_master_transcoder != crtc_state->cpu_transcoder;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> index f660ad80db04..854724f68f09 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> @@ -6,10 +6,15 @@
>  #ifndef __INTEL_DP_MST_H__
>  #define __INTEL_DP_MST_H__
>  
> +#include <linux/types.h>
> +
>  struct intel_digital_port;
> +struct intel_crtc_state;
>  
>  int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
>  void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port);
>  int intel_dp_mst_encoder_active_links(struct intel_digital_port *intel_dig_port);
> +bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state);
> +bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state);
>  
>  #endif /* __INTEL_DP_MST_H__ */
> -- 
> 2.24.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx