| Message ID | 20191218185910.303540-2-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/6] drm/i915/display: Share intel_connector_needs_modeset() | expand |
On Wed, Dec 18, 2019 at 10:59:06AM -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 > > 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 | 13 +- > .../drm/i915/display/intel_display_types.h | 3 + > drivers/gpu/drm/i915/display/intel_dp_mst.c | 143 ++++++++++++++++-- > drivers/gpu/drm/i915/display/intel_dp_mst.h | 5 + > 7 files changed, 183 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 fa40ba7cbcad..9d99ec82d072 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); > @@ -4400,6 +4405,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 25bf084427bf..59b3bfe8b721 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" > @@ -11630,6 +11631,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; > } > > /* Returns the currently programmed mode of the given encoder. */ > @@ -12477,6 +12479,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; > @@ -12618,6 +12623,7 @@ intel_crtc_prepare_cleared_state(struct intel_crtc_state *crtc_state) > memcpy(saved_state->icl_port_dplls, crtc_state->icl_port_dplls, > sizeof(saved_state->icl_port_dplls)); > saved_state->crc_enabled = crtc_state->crc_enabled; > + saved_state->mst_master_transcoder = INVALID_TRANSCODER; That's redundant now? > if (IS_G4X(dev_priv) || > IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > saved_state->wm = crtc_state->wm; > @@ -13257,6 +13263,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 > @@ -14341,7 +14349,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)) > @@ -14355,7 +14363,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..710137984c71 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -87,6 +87,54 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > return 0; > } > > +/* > + * Iterate over all connectors and set mst_master_transcoder to the smallest > + * transcoder id that shares the same MST connector. > + */ > +static enum transcoder > +intel_dp_mst_master_trans_compute(struct intel_encoder *encoder, > + struct intel_crtc_state *crtc_state, > + struct drm_connector_state *connector_state) > +{ > + struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state); Could grab that from the connector state and not pass in crtc_state at all? Hmm. Might be even better to just pass inin intel_atomic_state+mst_port so we wouldn't have the clash with the iterators and could use the normal names for those. Shrug. > + struct intel_connector *connector = to_intel_connector(connector_state->connector); > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > + struct intel_digital_connector_state *iter_connector_state; Could sprinkle some consts on the crtc/connector states now that we're pure. > + struct intel_connector *iter_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, iter_connector, > + iter_connector_state, i) { > + struct intel_crtc_state *iter_crtc_state; > + struct intel_crtc *iter_crtc; > + > + if (connector->mst_port != iter_connector->mst_port || > + !iter_connector_state->base.crtc) > + continue; > + > + iter_crtc = to_intel_crtc(iter_connector_state->base.crtc); > + iter_crtc_state = intel_atomic_get_new_crtc_state(state, > + iter_crtc); > + if (!iter_crtc_state->uapi.active) > + continue; > + > + /* > + * Using crtc->pipe because crtc_state->cpu_transcoder is > + * computed, so others pipe cpu_transcoder could have being > + * computed yet > + */ > + if (iter_crtc->pipe < ret) > + ret = iter_crtc->pipe; > + } What if we have no active pipes? Aren't we going to come here with ret==MAX_PIPES? I guess we can't really change the !active check to !enabled because we surely can't use an inactive transcoder as the master. So !active does seem like the right thing to check. > + > + /* Simple cast works because TGL don't have a eDP transcoder */ > + return (enum transcoder)ret; I'd have probably extracted the thing to compute the transcoder. But doesn't matter for MST since it can't be going over the EDP transcoder. Port sync will be a different story. We can cross that bridge when we come to it. > +} > + > static int intel_dp_mst_compute_config(struct intel_encoder *encoder, > struct intel_crtc_state *pipe_config, > struct drm_connector_state *conn_state) > @@ -154,24 +202,89 @@ 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(encoder, > + pipe_config, > + conn_state); > + if (ret) > + return ret; ret? > + > + return 0; > +} > + > +/* > + * If one of the connectors in a MST stream needs a modeset, mark all CRTCs > + * that have a connector in the same MST stream as mode changed, > + * intel_modeset_pipe_config()+intel_crtc_check_fastset() will take 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; > + > + 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); > + } > + > + crtc_state->uapi.mode_changed = true; We might need drm_atomic_add_affected_planes() here. The helper won't do that for us if the connector->atomic_check() was called from the second loop. The rest lgtm. > + } > + 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 +295,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 +306,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 +350,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 +429,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 +836,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
On Wed, 2019-12-18 at 21:24 +0200, Ville Syrjälä wrote: > On Wed, Dec 18, 2019 at 10:59:06AM -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 > > > > 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 | 13 +- > > .../drm/i915/display/intel_display_types.h | 3 + > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 143 > > ++++++++++++++++-- > > drivers/gpu/drm/i915/display/intel_dp_mst.h | 5 + > > 7 files changed, 183 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 fa40ba7cbcad..9d99ec82d072 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); > > @@ -4400,6 +4405,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_TRA > > NSPORT_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 25bf084427bf..59b3bfe8b721 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" > > @@ -11630,6 +11631,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; > > } > > > > /* Returns the currently programmed mode of the given encoder. */ > > @@ -12477,6 +12479,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; > > @@ -12618,6 +12623,7 @@ intel_crtc_prepare_cleared_state(struct > > intel_crtc_state *crtc_state) > > memcpy(saved_state->icl_port_dplls, crtc_state->icl_port_dplls, > > sizeof(saved_state->icl_port_dplls)); > > saved_state->crc_enabled = crtc_state->crc_enabled; > > + saved_state->mst_master_transcoder = INVALID_TRANSCODER; > > That's redundant now? No, we don't call intel_crtc_state_reset() when computing state. Did not changed that because we are copying a bunch of stuff from the saved_state. > > > if (IS_G4X(dev_priv) || > > IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > saved_state->wm = crtc_state->wm; > > @@ -13257,6 +13263,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 > > @@ -14341,7 +14349,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)) > > @@ -14355,7 +14363,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..710137984c71 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > @@ -87,6 +87,54 @@ static int > > intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > > return 0; > > } > > > > +/* > > + * Iterate over all connectors and set mst_master_transcoder to > > the smallest > > + * transcoder id that shares the same MST connector. > > + */ > > +static enum transcoder > > +intel_dp_mst_master_trans_compute(struct intel_encoder *encoder, > > + struct intel_crtc_state *crtc_state, > > + struct drm_connector_state > > *connector_state) > > +{ > > + struct intel_atomic_state *state = > > to_intel_atomic_state(crtc_state->uapi.state); > > Could grab that from the connector state and not pass in crtc_state > at > all? > > Hmm. Might be even better to just pass inin > intel_atomic_state+mst_port > so we wouldn't have the clash with the iterators and could use the > normal names for those. Shrug. Okay > > > > + struct intel_connector *connector = > > to_intel_connector(connector_state->connector); > > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > + struct intel_digital_connector_state *iter_connector_state; > > Could sprinkle some consts on the crtc/connector states now that > we're pure. > > > + struct intel_connector *iter_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, iter_connector, > > + iter_connector_state, i) > > { > > + struct intel_crtc_state *iter_crtc_state; > > + struct intel_crtc *iter_crtc; > > + > > + if (connector->mst_port != iter_connector->mst_port || > > + !iter_connector_state->base.crtc) > > + continue; > > + > > + iter_crtc = to_intel_crtc(iter_connector_state- > > >base.crtc); > > + iter_crtc_state = > > intel_atomic_get_new_crtc_state(state, > > + iter_ > > crtc); > > + if (!iter_crtc_state->uapi.active) > > + continue; > > + > > + /* > > + * Using crtc->pipe because crtc_state->cpu_transcoder > > is > > + * computed, so others pipe cpu_transcoder could have > > being > > + * computed yet > > + */ > > + if (iter_crtc->pipe < ret) > > + ret = iter_crtc->pipe; > > + } > > What if we have no active pipes? Aren't we going to come here with > ret==MAX_PIPES? > > I guess we can't really change the !active check to !enabled > because we surely can't use an inactive transcoder as the > master. So !active does seem like the right thing to check. > > > + > > + /* Simple cast works because TGL don't have a eDP transcoder */ > > + return (enum transcoder)ret; > > I'd have probably extracted the thing to compute the transcoder. > But doesn't matter for MST since it can't be going over the EDP > transcoder. Port sync will be a different story. We can cross that > bridge when we come to it. > > > +} > > + > > static int intel_dp_mst_compute_config(struct intel_encoder > > *encoder, > > struct intel_crtc_state > > *pipe_config, > > struct drm_connector_state > > *conn_state) > > @@ -154,24 +202,89 @@ 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(encoder, > > + > > pipe_config, > > + > > conn_state); > > + if (ret) > > + return ret; > > ret? My bad, it is left over from the change to return the master transcoder. > > > + > > + return 0; > > +} > > + > > +/* > > + * If one of the connectors in a MST stream needs a modeset, mark > > all CRTCs > > + * that have a connector in the same MST stream as mode changed, > > + * intel_modeset_pipe_config()+intel_crtc_check_fastset() will > > take 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; > > + > > + 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_ite > > r); > > + 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_ite > > r); > > + return PTR_ERR(crtc_state); > > + } > > + > > + crtc_state->uapi.mode_changed = true; > > We might need drm_atomic_add_affected_planes() here. The helper won't > do > that for us if the connector->atomic_check() was called from the > second > loop. Done > > The rest lgtm. > > > + } > > + 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 +295,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 +306,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 +350,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 +429,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 +836,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
On Wed, Dec 18, 2019 at 08:06:34PM +0000, Souza, Jose wrote: > On Wed, 2019-12-18 at 21:24 +0200, Ville Syrjälä wrote: > > On Wed, Dec 18, 2019 at 10:59:06AM -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 > > > > > > 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 | 13 +- > > > .../drm/i915/display/intel_display_types.h | 3 + > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 143 > > > ++++++++++++++++-- > > > drivers/gpu/drm/i915/display/intel_dp_mst.h | 5 + > > > 7 files changed, 183 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 fa40ba7cbcad..9d99ec82d072 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); > > > @@ -4400,6 +4405,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_TRA > > > NSPORT_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 25bf084427bf..59b3bfe8b721 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" > > > @@ -11630,6 +11631,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; > > > } > > > > > > /* Returns the currently programmed mode of the given encoder. */ > > > @@ -12477,6 +12479,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; > > > @@ -12618,6 +12623,7 @@ intel_crtc_prepare_cleared_state(struct > > > intel_crtc_state *crtc_state) > > > memcpy(saved_state->icl_port_dplls, crtc_state->icl_port_dplls, > > > sizeof(saved_state->icl_port_dplls)); > > > saved_state->crc_enabled = crtc_state->crc_enabled; > > > + saved_state->mst_master_transcoder = INVALID_TRANSCODER; > > > > That's redundant now? > > No, we don't call intel_crtc_state_reset() when computing state. > Did not changed that because we are copying a bunch of stuff from the > saved_state. Doh. I think we should be able just call the reset thing in saved_state here? Oh, and we seem to have another one in vlv_force_pll_on(). Maybe we should even add a intel_crtc_state_alloc() or something which would give us a properly initialized fresh state.
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 fa40ba7cbcad..9d99ec82d072 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); @@ -4400,6 +4405,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 25bf084427bf..59b3bfe8b721 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" @@ -11630,6 +11631,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; } /* Returns the currently programmed mode of the given encoder. */ @@ -12477,6 +12479,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; @@ -12618,6 +12623,7 @@ intel_crtc_prepare_cleared_state(struct intel_crtc_state *crtc_state) memcpy(saved_state->icl_port_dplls, crtc_state->icl_port_dplls, sizeof(saved_state->icl_port_dplls)); saved_state->crc_enabled = crtc_state->crc_enabled; + saved_state->mst_master_transcoder = INVALID_TRANSCODER; if (IS_G4X(dev_priv) || IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) saved_state->wm = crtc_state->wm; @@ -13257,6 +13263,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 @@ -14341,7 +14349,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)) @@ -14355,7 +14363,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..710137984c71 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -87,6 +87,54 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, return 0; } +/* + * Iterate over all connectors and set mst_master_transcoder to the smallest + * transcoder id that shares the same MST connector. + */ +static enum transcoder +intel_dp_mst_master_trans_compute(struct intel_encoder *encoder, + struct intel_crtc_state *crtc_state, + struct drm_connector_state *connector_state) +{ + struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state); + struct intel_connector *connector = to_intel_connector(connector_state->connector); + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); + struct intel_digital_connector_state *iter_connector_state; + struct intel_connector *iter_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, iter_connector, + iter_connector_state, i) { + struct intel_crtc_state *iter_crtc_state; + struct intel_crtc *iter_crtc; + + if (connector->mst_port != iter_connector->mst_port || + !iter_connector_state->base.crtc) + continue; + + iter_crtc = to_intel_crtc(iter_connector_state->base.crtc); + iter_crtc_state = intel_atomic_get_new_crtc_state(state, + iter_crtc); + if (!iter_crtc_state->uapi.active) + continue; + + /* + * Using crtc->pipe because crtc_state->cpu_transcoder is + * computed, so others pipe cpu_transcoder could have being + * computed yet + */ + if (iter_crtc->pipe < ret) + ret = iter_crtc->pipe; + } + + /* 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) @@ -154,24 +202,89 @@ 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(encoder, + pipe_config, + conn_state); + if (ret) + return ret; + + return 0; +} + +/* + * If one of the connectors in a MST stream needs a modeset, mark all CRTCs + * that have a connector in the same MST stream as mode changed, + * intel_modeset_pipe_config()+intel_crtc_check_fastset() will take 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; + + 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); + } + + 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 +295,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 +306,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 +350,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 +429,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 +836,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__ */
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 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 | 13 +- .../drm/i915/display/intel_display_types.h | 3 + drivers/gpu/drm/i915/display/intel_dp_mst.c | 143 ++++++++++++++++-- drivers/gpu/drm/i915/display/intel_dp_mst.h | 5 + 7 files changed, 183 insertions(+), 13 deletions(-)