| Message ID | 20191207011832.422566-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/i915/tgl: Select master transcoder for MST stream | expand |
On Fri, Dec 06, 2019 at 05:18:29PM -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 first > 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 > scenario will be handled in the next patch. > > 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_ddi.c | 14 +- > drivers/gpu/drm/i915/display/intel_display.c | 15 +- > .../drm/i915/display/intel_display_types.h | 3 + > drivers/gpu/drm/i915/display/intel_dp_mst.c | 163 ++++++++++++++++++ > drivers/gpu/drm/i915/display/intel_dp_mst.h | 5 + > 5 files changed, 196 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index 3cacb1e279c1..be5bc506d4d3 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -1903,8 +1903,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); I'd drop the WARN_ON(). If we keep adding these for every little thing we'll drown in them. > + temp |= TRANS_DDI_MST_TRANSPORT_SELECT(master); > + } > } else { > temp |= TRANS_DDI_MODE_SELECT_DP_SST; > temp |= DDI_PORT_WIDTH(crtc_state->lane_count); > @@ -4372,6 +4377,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 821ba8053f9d..f89494c849ce 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" > @@ -12542,6 +12543,11 @@ 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); > > + if (INTEL_GEN(dev_priv) >= 12 && > + intel_crtc_has_type(pipe_config, INTEL_OUTPUT_DP_MST)) Could probably print this unconditionally to keep the code less messy. > + DRM_DEBUG_KMS("MST master transcoder: %s\n", > + transcoder_name(pipe_config->mst_master_transcoder)); > + > dump_planes: > if (!state) > return; > @@ -13318,6 +13324,10 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config, > PIPE_CONF_CHECK_I(sync_mode_slaves_mask); > PIPE_CONF_CHECK_I(master_transcoder); > > + if (INTEL_GEN(dev_priv) >= 12 && > + intel_crtc_has_type(pipe_config, INTEL_OUTPUT_DP_MST)) These checks are definitely pointless. > + PIPE_CONF_CHECK_I(mst_master_transcoder); > + > #undef PIPE_CONF_CHECK_X > #undef PIPE_CONF_CHECK_I > #undef PIPE_CONF_CHECK_BOOL > @@ -14406,7 +14416,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)) > @@ -14420,7 +14430,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 926e49f449a6..49f1518e3ab9 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -87,6 +87,49 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > return 0; > } > > +/* > + * Iterate over all the CRTCs and set mst_master_transcoder to the first active > + * transcoder that shares the same MST connector. > + * > + * TODO: maybe this could be optimized to keep the old master transcoder > + */ > +static int > +intel_dp_mst_master_trans_compute(struct intel_encoder *encoder, > + struct intel_crtc_state *pipe_config, s/pipe_config/crtc_state/ > + struct drm_connector_state *conn_state) > +{ > + struct intel_atomic_state *state = to_intel_atomic_state(pipe_config->uapi.state); > + struct intel_connector *conn = to_intel_connector(conn_state->connector); ocd: s/conn/connector/ all over. > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > + struct intel_digital_connector_state *iter_conn_state; > + struct intel_connector *iter_conn; > + int i; > + > + if (INTEL_GEN(dev_priv) < 12) > + return 0; > + > + for_each_new_intel_connector_in_state(state, iter_conn, > + iter_conn_state, i) { > + struct intel_crtc_state *iter_crtc_state; > + struct intel_crtc *iter_crtc; > + > + if (conn->mst_port != iter_conn->mst_port || > + !iter_conn_state->base.crtc) > + continue; > + > + iter_crtc = to_intel_crtc(iter_conn_state->base.crtc); > + iter_crtc_state = intel_atomic_get_new_crtc_state(state, > + iter_crtc); > + if (!iter_crtc_state->uapi.active) > + continue; > + > + pipe_config->mst_master_transcoder = iter_crtc_state->cpu_transcoder; > + break; So we're going to pick this based on the connector order. How does that fit in with the port sync, or did we not have port sync for MST yet? To keep everything consistent and easy my idea was to pick the first pipe as the master for both MST and port sync. Although I can easily imagine topologies where even that would land us in some interesting mess. For example: pipe A -> MST port B -> ... -> whatever : MST master pipe B -> MST port B -> ... -> tile 0 : port sync master / MST slave pipe C -> MST port C -> ... -> tile 1 : port sync slave / MST master pipe D -> MST port C -> ... -> whatever : MST slave pipe A -> MST port B -> ... -> whatever : MST master pipe B -> MST port B -> ... -> tile 0 : port sync master / MST slave pipe C -> MST port C -> ... -> whatever : MST master pipe D -> MST port C -> ... -> tile 1 : port sync slave / MST slave No idea if the hw can even do something like that. Needs more thought if/when we enable port sync with MST (assuming we're not already doing that). > + } > + > + return 0; > +} > + > static int intel_dp_mst_compute_config(struct intel_encoder *encoder, > struct intel_crtc_state *pipe_config, > struct drm_connector_state *conn_state) > @@ -154,6 +197,98 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder, > > intel_ddi_compute_min_voltage_level(dev_priv, pipe_config); > > + ret = intel_dp_mst_master_trans_compute(encoder, pipe_config, > + conn_state); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static int > +intel_dp_mst_atomic_master_trans_check(struct drm_connector *connector, > + struct drm_atomic_state *state) Just pass in intel_ types pls. > +{ > + struct drm_i915_private *dev_priv = to_i915(connector->dev); > + struct drm_connector_state *new_conn_state, *old_conn_state; > + struct drm_connector_list_iter conn_list_iter; ocd: s/conn_list_iter/conn_iter/ > + struct intel_crtc_state *intel_crtc_state; Just 'crtc_state' > + struct intel_connector *intel_conn; > + struct drm_crtc_state *crtc_state; > + struct drm_connector *conn_iter; iter_foo vs. foo_iter in this vs. the previous function. Not that I really like that naming convention since it can get confusing with the 'struct drm_connector_list_iter conn_iter'. > + > + if (INTEL_GEN(dev_priv) < 12) > + return 0; > + > + intel_conn = to_intel_connector(connector); > + new_conn_state = drm_atomic_get_new_connector_state(state, connector); > + old_conn_state = drm_atomic_get_old_connector_state(state, connector); These could be done when declaring the variables. > + > + if (!old_conn_state->crtc && !new_conn_state->crtc) > + return 0; > + > + /* > + * 3 cases that needs be handled here: > + * - connector going from disabled to enabled: > + * nothing else need to be done, drm helpers already set mode_changed in > + * the CRTCs needed > + * - connector going from enabled to disabled: > + * if this transcoder was the master, all slaves needs a modeset > + * - connector going from enabled to enabled but it needs a modeset: > + * if this transcoder was the master, all slaves also needs a modeset > + */ Too may special cases IMO. I suggest just blindly marking everything on the same mst_port for modeset if this connector needs a modeset. IIRC we have a helper for that check even... Ah yes, intel_connector_needs_modeset(). > + > + /* disabled -> enabled */ > + if (!old_conn_state->crtc && new_conn_state->crtc) > + return 0; > + > + /* enabled -> enabled(modeset)? */ > + if (new_conn_state->crtc) { > + crtc_state = drm_atomic_get_new_crtc_state(state, > + new_conn_state->crtc); > + if (!drm_atomic_crtc_needs_modeset(crtc_state)) > + return 0; > + } > + > + /* handling enabled -> enabled(modeset) and enabled -> disabled */ > + crtc_state = drm_atomic_get_old_crtc_state(state, old_conn_state->crtc); > + intel_crtc_state = to_intel_crtc_state(crtc_state); > + > + /* If not master, nothing else needs to be handled */ > + if (!intel_dp_mst_is_master_trans(intel_crtc_state)) > + return 0; > + > + /* It is master, add and mark all other CRTCs in the stream as changed */ > + drm_connector_list_iter_begin(state->dev, &conn_list_iter); > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) { > + struct drm_connector_state *conn_iter_state; > + struct intel_connector *intel_conn_iter; > + > + intel_conn_iter = to_intel_connector(conn_iter); > + if (intel_conn_iter->mst_port != intel_conn->mst_port) > + continue; > + > + conn_iter_state = drm_atomic_get_connector_state(state, > + conn_iter); > + if (IS_ERR(conn_iter_state)) { > + drm_connector_list_iter_end(&conn_list_iter); > + return PTR_ERR(conn_iter_state); > + } > + > + if (!conn_iter_state->crtc) > + continue; > + > + crtc_state = drm_atomic_get_crtc_state(state, > + conn_iter_state->crtc); > + if (IS_ERR(crtc_state)) { > + drm_connector_list_iter_end(&conn_list_iter); > + return PTR_ERR(conn_iter_state); > + } > + > + crtc_state->mode_changed = true; > + } > + drm_connector_list_iter_end(&conn_list_iter); > + > return 0; > } > > @@ -175,6 +310,10 @@ intel_dp_mst_atomic_check(struct drm_connector *connector, > if (ret) > return ret; > > + ret = intel_dp_mst_atomic_master_trans_check(connector, state); > + if (ret) > + return ret; > + > if (!old_conn_state->crtc) > return 0; > > @@ -241,6 +380,9 @@ 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)); > + > /* > * From TGL spec: "If multi-stream slave transcoder: Configure > * Transcoder Clock Select to direct no clock to the transcoder" > @@ -321,6 +463,9 @@ static void intel_mst_pre_enable_dp(struct intel_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); > > if (first_mst_stream) > @@ -726,3 +871,21 @@ 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) > +{ > + struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); > + > + return INTEL_GEN(dev_priv) >= 12 && > + intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST) && > + crtc_state->mst_master_transcoder == crtc_state->cpu_transcoder; All but the last line seem redundant. > +} > + > +bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); > + > + return INTEL_GEN(dev_priv) >= 12 && > + intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST) && > + 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..e40767f78343 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 <stdbool.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.0
On Mon, 2019-12-09 at 18:40 +0200, Ville Syrjälä wrote: > On Fri, Dec 06, 2019 at 05:18:29PM -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 first > > 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 > > scenario will be handled in the next patch. > > > > 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_ddi.c | 14 +- > > drivers/gpu/drm/i915/display/intel_display.c | 15 +- > > .../drm/i915/display/intel_display_types.h | 3 + > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 163 > > ++++++++++++++++++ > > drivers/gpu/drm/i915/display/intel_dp_mst.h | 5 + > > 5 files changed, 196 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > index 3cacb1e279c1..be5bc506d4d3 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > @@ -1903,8 +1903,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); > > I'd drop the WARN_ON(). If we keep adding these for every little > thing > we'll drown in them. > > > + temp |= TRANS_DDI_MST_TRANSPORT_SELECT(master); > > + } > > } else { > > temp |= TRANS_DDI_MODE_SELECT_DP_SST; > > temp |= DDI_PORT_WIDTH(crtc_state->lane_count); > > @@ -4372,6 +4377,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 821ba8053f9d..f89494c849ce 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" > > @@ -12542,6 +12543,11 @@ 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); > > > > + if (INTEL_GEN(dev_priv) >= 12 && > > + intel_crtc_has_type(pipe_config, INTEL_OUTPUT_DP_MST)) > > Could probably print this unconditionally to keep the code less > messy. I'm not setting mst_master_transcoder = INVALID_TRANSCODER in the other code paths, so it would print transcoder A for HDMI, DP SST and to DP MST in gen < 12. That is fine? Should I add mst_master_transcoder = INVALID_TRANSCODER to all those code paths? For me the best option is keep this checks. > > > + DRM_DEBUG_KMS("MST master transcoder: %s\n", > > + transcoder_name(pipe_config- > > >mst_master_transcoder)); > > + > > dump_planes: > > if (!state) > > return; > > @@ -13318,6 +13324,10 @@ intel_pipe_config_compare(const struct > > intel_crtc_state *current_config, > > PIPE_CONF_CHECK_I(sync_mode_slaves_mask); > > PIPE_CONF_CHECK_I(master_transcoder); > > > > + if (INTEL_GEN(dev_priv) >= 12 && > > + intel_crtc_has_type(pipe_config, INTEL_OUTPUT_DP_MST)) > > These checks are definitely pointless. Similar with the above, it would not cause mismatch because for non gen 12 DP MST it would compare trans A with trans A, will change that depending on your answer to the above. > > > + PIPE_CONF_CHECK_I(mst_master_transcoder); > > + > > #undef PIPE_CONF_CHECK_X > > #undef PIPE_CONF_CHECK_I > > #undef PIPE_CONF_CHECK_BOOL > > @@ -14406,7 +14416,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)) > > @@ -14420,7 +14430,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 926e49f449a6..49f1518e3ab9 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > @@ -87,6 +87,49 @@ static int > > intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > > return 0; > > } > > > > +/* > > + * Iterate over all the CRTCs and set mst_master_transcoder to the > > first active > > + * transcoder that shares the same MST connector. > > + * > > + * TODO: maybe this could be optimized to keep the old master > > transcoder > > + */ > > +static int > > +intel_dp_mst_master_trans_compute(struct intel_encoder *encoder, > > + struct intel_crtc_state *pipe_config, > > s/pipe_config/crtc_state/ Okay > > > + struct drm_connector_state > > *conn_state) > > +{ > > + struct intel_atomic_state *state = > > to_intel_atomic_state(pipe_config->uapi.state); > > + struct intel_connector *conn = to_intel_connector(conn_state- > > >connector); > > ocd: s/conn/connector/ all over. Okay > > > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > + struct intel_digital_connector_state *iter_conn_state; > > + struct intel_connector *iter_conn; > > + int i; > > + > > + if (INTEL_GEN(dev_priv) < 12) > > + return 0; > > + > > + for_each_new_intel_connector_in_state(state, iter_conn, > > + iter_conn_state, i) { > > + struct intel_crtc_state *iter_crtc_state; > > + struct intel_crtc *iter_crtc; > > + > > + if (conn->mst_port != iter_conn->mst_port || > > + !iter_conn_state->base.crtc) > > + continue; > > + > > + iter_crtc = to_intel_crtc(iter_conn_state->base.crtc); > > + iter_crtc_state = > > intel_atomic_get_new_crtc_state(state, > > + iter_ > > crtc); > > + if (!iter_crtc_state->uapi.active) > > + continue; > > + > > + pipe_config->mst_master_transcoder = iter_crtc_state- > > >cpu_transcoder; > > + break; > > So we're going to pick this based on the connector order. How does > that > fit in with the port sync, or did we not have port sync for MST yet? > > To keep everything consistent and easy my idea was to pick the first > pipe as the master for both MST and port sync. > > Although I can easily imagine topologies where even that would > land us in some interesting mess. For example: > pipe A -> MST port B -> ... -> whatever : MST master > pipe B -> MST port B -> ... -> tile 0 : port sync master / MST slave > pipe C -> MST port C -> ... -> tile 1 : port sync slave / MST master > pipe D -> MST port C -> ... -> whatever : MST slave > > pipe A -> MST port B -> ... -> whatever : MST master > pipe B -> MST port B -> ... -> tile 0 : port sync master / MST slave > pipe C -> MST port C -> ... -> whatever : MST master > pipe D -> MST port C -> ... -> tile 1 : port sync slave / MST slave > > No idea if the hw can even do something like that. Needs more thought > if/when we enable port sync with MST (assuming we're not already > doing > that). According to BSpec is possible to have port sync with MST but it is not implemented. But sure it can be easily fixed, will loop throgh all the connectors updating mst_master_transcoder with the smallest pipe. > > > + } > > + > > + return 0; > > +} > > + > > static int intel_dp_mst_compute_config(struct intel_encoder > > *encoder, > > struct intel_crtc_state > > *pipe_config, > > struct drm_connector_state > > *conn_state) > > @@ -154,6 +197,98 @@ static int intel_dp_mst_compute_config(struct > > intel_encoder *encoder, > > > > intel_ddi_compute_min_voltage_level(dev_priv, pipe_config); > > > > + ret = intel_dp_mst_master_trans_compute(encoder, pipe_config, > > + conn_state); > > + if (ret) > > + return ret; > > + > > + return 0; > > +} > > + > > +static int > > +intel_dp_mst_atomic_master_trans_check(struct drm_connector > > *connector, > > + struct drm_atomic_state *state) > > Just pass in intel_ types pls. Okay > > > +{ > > + struct drm_i915_private *dev_priv = to_i915(connector->dev); > > + struct drm_connector_state *new_conn_state, *old_conn_state; > > + struct drm_connector_list_iter conn_list_iter; > > ocd: s/conn_list_iter/conn_iter/ Okay > > > + struct intel_crtc_state *intel_crtc_state; > > Just 'crtc_state' Okay > > > + struct intel_connector *intel_conn; > > + struct drm_crtc_state *crtc_state; > > + struct drm_connector *conn_iter; > > iter_foo vs. foo_iter in this vs. the previous function. Not that I > really like that naming convention since it can get confusing with > the 'struct drm_connector_list_iter conn_iter'. > > > + > > + if (INTEL_GEN(dev_priv) < 12) > > + return 0; > > + > > + intel_conn = to_intel_connector(connector); > > + new_conn_state = drm_atomic_get_new_connector_state(state, > > connector); > > + old_conn_state = drm_atomic_get_old_connector_state(state, > > connector); > > These could be done when declaring the variables. > > > + > > + if (!old_conn_state->crtc && !new_conn_state->crtc) > > + return 0; > > + > > + /* > > + * 3 cases that needs be handled here: > > + * - connector going from disabled to enabled: > > + * nothing else need to be done, drm helpers already set > > mode_changed in > > + * the CRTCs needed > > + * - connector going from enabled to disabled: > > + * if this transcoder was the master, all slaves needs a > > modeset > > + * - connector going from enabled to enabled but it needs a > > modeset: > > + * if this transcoder was the master, all slaves also needs a > > modeset > > + */ > > Too may special cases IMO. I suggest just blindly marking > everything on the same mst_port for modeset if this connector > needs a modeset. IIRC we have a helper for that check even... > Ah yes, intel_connector_needs_modeset(). Oooh cool, will use it. > > > + > > + /* disabled -> enabled */ > > + if (!old_conn_state->crtc && new_conn_state->crtc) > > + return 0; > > + > > + /* enabled -> enabled(modeset)? */ > > + if (new_conn_state->crtc) { > > + crtc_state = drm_atomic_get_new_crtc_state(state, > > + new_conn_sta > > te->crtc); > > + if (!drm_atomic_crtc_needs_modeset(crtc_state)) > > + return 0; > > + } > > + > > + /* handling enabled -> enabled(modeset) and enabled -> disabled > > */ > > + crtc_state = drm_atomic_get_old_crtc_state(state, > > old_conn_state->crtc); > > + intel_crtc_state = to_intel_crtc_state(crtc_state); > > + > > + /* If not master, nothing else needs to be handled */ > > + if (!intel_dp_mst_is_master_trans(intel_crtc_state)) > > + return 0; > > + > > + /* It is master, add and mark all other CRTCs in the stream as > > changed */ > > + drm_connector_list_iter_begin(state->dev, &conn_list_iter); > > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) { > > + struct drm_connector_state *conn_iter_state; > > + struct intel_connector *intel_conn_iter; > > + > > + intel_conn_iter = to_intel_connector(conn_iter); > > + if (intel_conn_iter->mst_port != intel_conn->mst_port) > > + continue; > > + > > + conn_iter_state = drm_atomic_get_connector_state(state, > > + conn_i > > ter); > > + if (IS_ERR(conn_iter_state)) { > > + drm_connector_list_iter_end(&conn_list_iter); > > + return PTR_ERR(conn_iter_state); > > + } > > + > > + if (!conn_iter_state->crtc) > > + continue; > > + > > + crtc_state = drm_atomic_get_crtc_state(state, > > + conn_iter_state- > > >crtc); > > + if (IS_ERR(crtc_state)) { > > + drm_connector_list_iter_end(&conn_list_iter); > > + return PTR_ERR(conn_iter_state); > > + } > > + > > + crtc_state->mode_changed = true; > > + } > > + drm_connector_list_iter_end(&conn_list_iter); > > + > > return 0; > > } > > > > @@ -175,6 +310,10 @@ intel_dp_mst_atomic_check(struct drm_connector > > *connector, > > if (ret) > > return ret; > > > > + ret = intel_dp_mst_atomic_master_trans_check(connector, state); > > + if (ret) > > + return ret; > > + > > if (!old_conn_state->crtc) > > return 0; > > > > @@ -241,6 +380,9 @@ 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)); > > + > > /* > > * From TGL spec: "If multi-stream slave transcoder: Configure > > * Transcoder Clock Select to direct no clock to the > > transcoder" > > @@ -321,6 +463,9 @@ static void intel_mst_pre_enable_dp(struct > > intel_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); > > > > if (first_mst_stream) > > @@ -726,3 +871,21 @@ 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) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(crtc_state- > > >uapi.crtc->dev); > > + > > + return INTEL_GEN(dev_priv) >= 12 && > > + intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST) && > > + crtc_state->mst_master_transcoder == crtc_state- > > >cpu_transcoder; > > All but the last line seem redundant. Similar to my first answer and even if INVALID_TRANSCODER is set in other code paths, removing the other checks would cause us to need to check for gen 12 and MST when enabling or disabling pipes, that is why I kept everything here. > > > +} > > + > > +bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state > > *crtc_state) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(crtc_state- > > >uapi.crtc->dev); > > + > > + return INTEL_GEN(dev_priv) >= 12 && > > + intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST) && > > + 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..e40767f78343 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 <stdbool.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.0
On Mon, Dec 09, 2019 at 06:45:43PM +0000, Souza, Jose wrote: > On Mon, 2019-12-09 at 18:40 +0200, Ville Syrjälä wrote: > > On Fri, Dec 06, 2019 at 05:18:29PM -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 first > > > 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 > > > scenario will be handled in the next patch. > > > > > > 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_ddi.c | 14 +- > > > drivers/gpu/drm/i915/display/intel_display.c | 15 +- > > > .../drm/i915/display/intel_display_types.h | 3 + > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 163 > > > ++++++++++++++++++ > > > drivers/gpu/drm/i915/display/intel_dp_mst.h | 5 + > > > 5 files changed, 196 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > > index 3cacb1e279c1..be5bc506d4d3 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > > @@ -1903,8 +1903,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); > > > > I'd drop the WARN_ON(). If we keep adding these for every little > > thing > > we'll drown in them. > > > > > + temp |= TRANS_DDI_MST_TRANSPORT_SELECT(master); > > > + } > > > } else { > > > temp |= TRANS_DDI_MODE_SELECT_DP_SST; > > > temp |= DDI_PORT_WIDTH(crtc_state->lane_count); > > > @@ -4372,6 +4377,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 821ba8053f9d..f89494c849ce 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" > > > @@ -12542,6 +12543,11 @@ 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); > > > > > > + if (INTEL_GEN(dev_priv) >= 12 && > > > + intel_crtc_has_type(pipe_config, INTEL_OUTPUT_DP_MST)) > > > > Could probably print this unconditionally to keep the code less > > messy. > > I'm not setting mst_master_transcoder = INVALID_TRANSCODER in the other > code paths, so it would print transcoder A for HDMI, DP SST and to DP > MST in gen < 12. > That is fine? Should I add mst_master_transcoder = INVALID_TRANSCODER > to all those code paths? For me the best option is keep this checks. I think setting to invalid would be nice. With https://patchwork.freedesktop.org/series/69129/ we could even do it in a nice central place just the once. > > > > > > > + DRM_DEBUG_KMS("MST master transcoder: %s\n", > > > + transcoder_name(pipe_config- > > > >mst_master_transcoder)); > > > + > > > dump_planes: > > > if (!state) > > > return; > > > @@ -13318,6 +13324,10 @@ intel_pipe_config_compare(const struct > > > intel_crtc_state *current_config, > > > PIPE_CONF_CHECK_I(sync_mode_slaves_mask); > > > PIPE_CONF_CHECK_I(master_transcoder); > > > > > > + if (INTEL_GEN(dev_priv) >= 12 && > > > + intel_crtc_has_type(pipe_config, INTEL_OUTPUT_DP_MST)) > > > > These checks are definitely pointless. > > Similar with the above, it would not cause mismatch because for non gen > 12 DP MST it would compare trans A with trans A, will change that > depending on your answer to the above. > > > > > > > + PIPE_CONF_CHECK_I(mst_master_transcoder); > > > + > > > #undef PIPE_CONF_CHECK_X > > > #undef PIPE_CONF_CHECK_I > > > #undef PIPE_CONF_CHECK_BOOL > > > @@ -14406,7 +14416,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)) > > > @@ -14420,7 +14430,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 926e49f449a6..49f1518e3ab9 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > @@ -87,6 +87,49 @@ static int > > > intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > > > return 0; > > > } > > > > > > +/* > > > + * Iterate over all the CRTCs and set mst_master_transcoder to the > > > first active > > > + * transcoder that shares the same MST connector. > > > + * > > > + * TODO: maybe this could be optimized to keep the old master > > > transcoder > > > + */ > > > +static int > > > +intel_dp_mst_master_trans_compute(struct intel_encoder *encoder, > > > + struct intel_crtc_state *pipe_config, > > > > s/pipe_config/crtc_state/ > > Okay > > > > > > + struct drm_connector_state > > > *conn_state) > > > +{ > > > + struct intel_atomic_state *state = > > > to_intel_atomic_state(pipe_config->uapi.state); > > > + struct intel_connector *conn = to_intel_connector(conn_state- > > > >connector); > > > > ocd: s/conn/connector/ all over. > > Okay > > > > > > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > > + struct intel_digital_connector_state *iter_conn_state; > > > + struct intel_connector *iter_conn; > > > + int i; > > > + > > > + if (INTEL_GEN(dev_priv) < 12) > > > + return 0; > > > + > > > + for_each_new_intel_connector_in_state(state, iter_conn, > > > + iter_conn_state, i) { > > > + struct intel_crtc_state *iter_crtc_state; > > > + struct intel_crtc *iter_crtc; > > > + > > > + if (conn->mst_port != iter_conn->mst_port || > > > + !iter_conn_state->base.crtc) > > > + continue; > > > + > > > + iter_crtc = to_intel_crtc(iter_conn_state->base.crtc); > > > + iter_crtc_state = > > > intel_atomic_get_new_crtc_state(state, > > > + iter_ > > > crtc); > > > + if (!iter_crtc_state->uapi.active) > > > + continue; > > > + > > > + pipe_config->mst_master_transcoder = iter_crtc_state- > > > >cpu_transcoder; > > > + break; > > > > So we're going to pick this based on the connector order. How does > > that > > fit in with the port sync, or did we not have port sync for MST yet? > > > > To keep everything consistent and easy my idea was to pick the first > > pipe as the master for both MST and port sync. > > > > Although I can easily imagine topologies where even that would > > land us in some interesting mess. For example: > > pipe A -> MST port B -> ... -> whatever : MST master > > pipe B -> MST port B -> ... -> tile 0 : port sync master / MST slave > > pipe C -> MST port C -> ... -> tile 1 : port sync slave / MST master > > pipe D -> MST port C -> ... -> whatever : MST slave > > > > pipe A -> MST port B -> ... -> whatever : MST master > > pipe B -> MST port B -> ... -> tile 0 : port sync master / MST slave > > pipe C -> MST port C -> ... -> whatever : MST master > > pipe D -> MST port C -> ... -> tile 1 : port sync slave / MST slave > > > > No idea if the hw can even do something like that. Needs more thought > > if/when we enable port sync with MST (assuming we're not already > > doing > > that). > > According to BSpec is possible to have port sync with MST but it is not > implemented. But sure it can be easily fixed, will loop throgh all the > connectors updating mst_master_transcoder with the smallest pipe. I don't think it's quite that easy since the ordering constraints of MST vs. port sync might conflict as in my examples above. But let's just ignore that particular can of worms for now.
On Mon, 2019-12-09 at 21:43 +0200, Ville Syrjälä wrote: > On Mon, Dec 09, 2019 at 06:45:43PM +0000, Souza, Jose wrote: > > On Mon, 2019-12-09 at 18:40 +0200, Ville Syrjälä wrote: > > > On Fri, Dec 06, 2019 at 05:18:29PM -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 first > > > > 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 > > > > scenario will be handled in the next patch. > > > > > > > > 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_ddi.c | 14 +- > > > > drivers/gpu/drm/i915/display/intel_display.c | 15 +- > > > > .../drm/i915/display/intel_display_types.h | 3 + > > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 163 > > > > ++++++++++++++++++ > > > > drivers/gpu/drm/i915/display/intel_dp_mst.h | 5 + > > > > 5 files changed, 196 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > > > index 3cacb1e279c1..be5bc506d4d3 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > > > @@ -1903,8 +1903,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); > > > > > > I'd drop the WARN_ON(). If we keep adding these for every little > > > thing > > > we'll drown in them. > > > > > > > + temp |= > > > > TRANS_DDI_MST_TRANSPORT_SELECT(master); > > > > + } > > > > } else { > > > > temp |= TRANS_DDI_MODE_SELECT_DP_SST; > > > > temp |= DDI_PORT_WIDTH(crtc_state->lane_count); > > > > @@ -4372,6 +4377,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 821ba8053f9d..f89494c849ce 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" > > > > @@ -12542,6 +12543,11 @@ 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); > > > > > > > > + if (INTEL_GEN(dev_priv) >= 12 && > > > > + intel_crtc_has_type(pipe_config, > > > > INTEL_OUTPUT_DP_MST)) > > > > > > Could probably print this unconditionally to keep the code less > > > messy. > > > > I'm not setting mst_master_transcoder = INVALID_TRANSCODER in the > > other > > code paths, so it would print transcoder A for HDMI, DP SST and to > > DP > > MST in gen < 12. > > That is fine? Should I add mst_master_transcoder = > > INVALID_TRANSCODER > > to all those code paths? For me the best option is keep this > > checks. > > I think setting to invalid would be nice. > > With https://patchwork.freedesktop.org/series/69129/ > we could even do it in a nice central place just the once. Awesome, just rvb it. It still apply without conflicts and compile, moving this MST patches on top. > > > > > > > + DRM_DEBUG_KMS("MST master transcoder: %s\n", > > > > + transcoder_name(pipe_config- > > > > > mst_master_transcoder)); > > > > + > > > > dump_planes: > > > > if (!state) > > > > return; > > > > @@ -13318,6 +13324,10 @@ intel_pipe_config_compare(const struct > > > > intel_crtc_state *current_config, > > > > PIPE_CONF_CHECK_I(sync_mode_slaves_mask); > > > > PIPE_CONF_CHECK_I(master_transcoder); > > > > > > > > + if (INTEL_GEN(dev_priv) >= 12 && > > > > + intel_crtc_has_type(pipe_config, > > > > INTEL_OUTPUT_DP_MST)) > > > > > > These checks are definitely pointless. > > > > Similar with the above, it would not cause mismatch because for non > > gen > > 12 DP MST it would compare trans A with trans A, will change that > > depending on your answer to the above. > > > > > > > > + PIPE_CONF_CHECK_I(mst_master_transcoder); > > > > + > > > > #undef PIPE_CONF_CHECK_X > > > > #undef PIPE_CONF_CHECK_I > > > > #undef PIPE_CONF_CHECK_BOOL > > > > @@ -14406,7 +14416,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)) > > > > @@ -14420,7 +14430,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 926e49f449a6..49f1518e3ab9 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > @@ -87,6 +87,49 @@ static int > > > > intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > > > > return 0; > > > > } > > > > > > > > +/* > > > > + * Iterate over all the CRTCs and set mst_master_transcoder to > > > > the > > > > first active > > > > + * transcoder that shares the same MST connector. > > > > + * > > > > + * TODO: maybe this could be optimized to keep the old master > > > > transcoder > > > > + */ > > > > +static int > > > > +intel_dp_mst_master_trans_compute(struct intel_encoder > > > > *encoder, > > > > + struct intel_crtc_state > > > > *pipe_config, > > > > > > s/pipe_config/crtc_state/ > > > > Okay > > > > > > + struct drm_connector_state > > > > *conn_state) > > > > +{ > > > > + struct intel_atomic_state *state = > > > > to_intel_atomic_state(pipe_config->uapi.state); > > > > + struct intel_connector *conn = > > > > to_intel_connector(conn_state- > > > > > connector); > > > > > > ocd: s/conn/connector/ all over. > > > > Okay > > > > > > + struct drm_i915_private *dev_priv = to_i915(encoder- > > > > >base.dev); > > > > + struct intel_digital_connector_state *iter_conn_state; > > > > + struct intel_connector *iter_conn; > > > > + int i; > > > > + > > > > + if (INTEL_GEN(dev_priv) < 12) > > > > + return 0; > > > > + > > > > + for_each_new_intel_connector_in_state(state, iter_conn, > > > > + iter_conn_state, > > > > i) { > > > > + struct intel_crtc_state *iter_crtc_state; > > > > + struct intel_crtc *iter_crtc; > > > > + > > > > + if (conn->mst_port != iter_conn->mst_port || > > > > + !iter_conn_state->base.crtc) > > > > + continue; > > > > + > > > > + iter_crtc = to_intel_crtc(iter_conn_state- > > > > >base.crtc); > > > > + iter_crtc_state = > > > > intel_atomic_get_new_crtc_state(state, > > > > + > > > > iter_ > > > > crtc); > > > > + if (!iter_crtc_state->uapi.active) > > > > + continue; > > > > + > > > > + pipe_config->mst_master_transcoder = > > > > iter_crtc_state- > > > > > cpu_transcoder; > > > > + break; > > > > > > So we're going to pick this based on the connector order. How > > > does > > > that > > > fit in with the port sync, or did we not have port sync for MST > > > yet? > > > > > > To keep everything consistent and easy my idea was to pick the > > > first > > > pipe as the master for both MST and port sync. > > > > > > Although I can easily imagine topologies where even that would > > > land us in some interesting mess. For example: > > > pipe A -> MST port B -> ... -> whatever : MST master > > > pipe B -> MST port B -> ... -> tile 0 : port sync master / MST > > > slave > > > pipe C -> MST port C -> ... -> tile 1 : port sync slave / MST > > > master > > > pipe D -> MST port C -> ... -> whatever : MST slave > > > > > > pipe A -> MST port B -> ... -> whatever : MST master > > > pipe B -> MST port B -> ... -> tile 0 : port sync master / MST > > > slave > > > pipe C -> MST port C -> ... -> whatever : MST master > > > pipe D -> MST port C -> ... -> tile 1 : port sync slave / MST > > > slave > > > > > > No idea if the hw can even do something like that. Needs more > > > thought > > > if/when we enable port sync with MST (assuming we're not already > > > doing > > > that). > > > > According to BSpec is possible to have port sync with MST but it is > > not > > implemented. But sure it can be easily fixed, will loop throgh all > > the > > connectors updating mst_master_transcoder with the smallest pipe. > > I don't think it's quite that easy since the ordering constraints of > MST vs. port sync might conflict as in my examples above. But let's > just ignore that particular can of worms for now. >
On Tue, Dec 10, 2019 at 02:16:23AM +0000, Souza, Jose wrote: > On Mon, 2019-12-09 at 21:43 +0200, Ville Syrjälä wrote: > > On Mon, Dec 09, 2019 at 06:45:43PM +0000, Souza, Jose wrote: > > > On Mon, 2019-12-09 at 18:40 +0200, Ville Syrjälä wrote: > > > > On Fri, Dec 06, 2019 at 05:18:29PM -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 first > > > > > 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 > > > > > scenario will be handled in the next patch. > > > > > > > > > > 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_ddi.c | 14 +- > > > > > drivers/gpu/drm/i915/display/intel_display.c | 15 +- > > > > > .../drm/i915/display/intel_display_types.h | 3 + > > > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 163 > > > > > ++++++++++++++++++ > > > > > drivers/gpu/drm/i915/display/intel_dp_mst.h | 5 + > > > > > 5 files changed, 196 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > index 3cacb1e279c1..be5bc506d4d3 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > @@ -1903,8 +1903,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); > > > > > > > > I'd drop the WARN_ON(). If we keep adding these for every little > > > > thing > > > > we'll drown in them. > > > > > > > > > + temp |= > > > > > TRANS_DDI_MST_TRANSPORT_SELECT(master); > > > > > + } > > > > > } else { > > > > > temp |= TRANS_DDI_MODE_SELECT_DP_SST; > > > > > temp |= DDI_PORT_WIDTH(crtc_state->lane_count); > > > > > @@ -4372,6 +4377,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 821ba8053f9d..f89494c849ce 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" > > > > > @@ -12542,6 +12543,11 @@ 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); > > > > > > > > > > + if (INTEL_GEN(dev_priv) >= 12 && > > > > > + intel_crtc_has_type(pipe_config, > > > > > INTEL_OUTPUT_DP_MST)) > > > > > > > > Could probably print this unconditionally to keep the code less > > > > messy. > > > > > > I'm not setting mst_master_transcoder = INVALID_TRANSCODER in the > > > other > > > code paths, so it would print transcoder A for HDMI, DP SST and to > > > DP > > > MST in gen < 12. > > > That is fine? Should I add mst_master_transcoder = > > > INVALID_TRANSCODER > > > to all those code paths? For me the best option is keep this > > > checks. > > > > I think setting to invalid would be nice. > > > > With https://patchwork.freedesktop.org/series/69129/ > > we could even do it in a nice central place just the once. > > Awesome, just rvb it. > It still apply without conflicts and compile, moving this MST patches > on top. Thanks. Oh, now I remember. That series didn't pass BAT. The fix for that is the remainder of https://patchwork.freedesktop.org/series/69366/
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 3cacb1e279c1..be5bc506d4d3 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -1903,8 +1903,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); @@ -4372,6 +4377,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 821ba8053f9d..f89494c849ce 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" @@ -12542,6 +12543,11 @@ 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); + if (INTEL_GEN(dev_priv) >= 12 && + intel_crtc_has_type(pipe_config, INTEL_OUTPUT_DP_MST)) + DRM_DEBUG_KMS("MST master transcoder: %s\n", + transcoder_name(pipe_config->mst_master_transcoder)); + dump_planes: if (!state) return; @@ -13318,6 +13324,10 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config, PIPE_CONF_CHECK_I(sync_mode_slaves_mask); PIPE_CONF_CHECK_I(master_transcoder); + if (INTEL_GEN(dev_priv) >= 12 && + intel_crtc_has_type(pipe_config, INTEL_OUTPUT_DP_MST)) + PIPE_CONF_CHECK_I(mst_master_transcoder); + #undef PIPE_CONF_CHECK_X #undef PIPE_CONF_CHECK_I #undef PIPE_CONF_CHECK_BOOL @@ -14406,7 +14416,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)) @@ -14420,7 +14430,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 926e49f449a6..49f1518e3ab9 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -87,6 +87,49 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, return 0; } +/* + * Iterate over all the CRTCs and set mst_master_transcoder to the first active + * transcoder that shares the same MST connector. + * + * TODO: maybe this could be optimized to keep the old master transcoder + */ +static int +intel_dp_mst_master_trans_compute(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(pipe_config->uapi.state); + struct intel_connector *conn = to_intel_connector(conn_state->connector); + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); + struct intel_digital_connector_state *iter_conn_state; + struct intel_connector *iter_conn; + int i; + + if (INTEL_GEN(dev_priv) < 12) + return 0; + + for_each_new_intel_connector_in_state(state, iter_conn, + iter_conn_state, i) { + struct intel_crtc_state *iter_crtc_state; + struct intel_crtc *iter_crtc; + + if (conn->mst_port != iter_conn->mst_port || + !iter_conn_state->base.crtc) + continue; + + iter_crtc = to_intel_crtc(iter_conn_state->base.crtc); + iter_crtc_state = intel_atomic_get_new_crtc_state(state, + iter_crtc); + if (!iter_crtc_state->uapi.active) + continue; + + pipe_config->mst_master_transcoder = iter_crtc_state->cpu_transcoder; + break; + } + + return 0; +} + static int intel_dp_mst_compute_config(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config, struct drm_connector_state *conn_state) @@ -154,6 +197,98 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder, intel_ddi_compute_min_voltage_level(dev_priv, pipe_config); + ret = intel_dp_mst_master_trans_compute(encoder, pipe_config, + conn_state); + if (ret) + return ret; + + return 0; +} + +static int +intel_dp_mst_atomic_master_trans_check(struct drm_connector *connector, + struct drm_atomic_state *state) +{ + struct drm_i915_private *dev_priv = to_i915(connector->dev); + struct drm_connector_state *new_conn_state, *old_conn_state; + struct drm_connector_list_iter conn_list_iter; + struct intel_crtc_state *intel_crtc_state; + struct intel_connector *intel_conn; + struct drm_crtc_state *crtc_state; + struct drm_connector *conn_iter; + + if (INTEL_GEN(dev_priv) < 12) + return 0; + + intel_conn = to_intel_connector(connector); + new_conn_state = drm_atomic_get_new_connector_state(state, connector); + old_conn_state = drm_atomic_get_old_connector_state(state, connector); + + if (!old_conn_state->crtc && !new_conn_state->crtc) + return 0; + + /* + * 3 cases that needs be handled here: + * - connector going from disabled to enabled: + * nothing else need to be done, drm helpers already set mode_changed in + * the CRTCs needed + * - connector going from enabled to disabled: + * if this transcoder was the master, all slaves needs a modeset + * - connector going from enabled to enabled but it needs a modeset: + * if this transcoder was the master, all slaves also needs a modeset + */ + + /* disabled -> enabled */ + if (!old_conn_state->crtc && new_conn_state->crtc) + return 0; + + /* enabled -> enabled(modeset)? */ + if (new_conn_state->crtc) { + crtc_state = drm_atomic_get_new_crtc_state(state, + new_conn_state->crtc); + if (!drm_atomic_crtc_needs_modeset(crtc_state)) + return 0; + } + + /* handling enabled -> enabled(modeset) and enabled -> disabled */ + crtc_state = drm_atomic_get_old_crtc_state(state, old_conn_state->crtc); + intel_crtc_state = to_intel_crtc_state(crtc_state); + + /* If not master, nothing else needs to be handled */ + if (!intel_dp_mst_is_master_trans(intel_crtc_state)) + return 0; + + /* It is master, add and mark all other CRTCs in the stream as changed */ + drm_connector_list_iter_begin(state->dev, &conn_list_iter); + drm_for_each_connector_iter(conn_iter, &conn_list_iter) { + struct drm_connector_state *conn_iter_state; + struct intel_connector *intel_conn_iter; + + intel_conn_iter = to_intel_connector(conn_iter); + if (intel_conn_iter->mst_port != intel_conn->mst_port) + continue; + + conn_iter_state = drm_atomic_get_connector_state(state, + conn_iter); + if (IS_ERR(conn_iter_state)) { + drm_connector_list_iter_end(&conn_list_iter); + return PTR_ERR(conn_iter_state); + } + + if (!conn_iter_state->crtc) + continue; + + crtc_state = drm_atomic_get_crtc_state(state, + conn_iter_state->crtc); + if (IS_ERR(crtc_state)) { + drm_connector_list_iter_end(&conn_list_iter); + return PTR_ERR(conn_iter_state); + } + + crtc_state->mode_changed = true; + } + drm_connector_list_iter_end(&conn_list_iter); + return 0; } @@ -175,6 +310,10 @@ intel_dp_mst_atomic_check(struct drm_connector *connector, if (ret) return ret; + ret = intel_dp_mst_atomic_master_trans_check(connector, state); + if (ret) + return ret; + if (!old_conn_state->crtc) return 0; @@ -241,6 +380,9 @@ 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)); + /* * From TGL spec: "If multi-stream slave transcoder: Configure * Transcoder Clock Select to direct no clock to the transcoder" @@ -321,6 +463,9 @@ static void intel_mst_pre_enable_dp(struct intel_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); if (first_mst_stream) @@ -726,3 +871,21 @@ 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) +{ + struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); + + return INTEL_GEN(dev_priv) >= 12 && + intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST) && + crtc_state->mst_master_transcoder == crtc_state->cpu_transcoder; +} + +bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state) +{ + struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); + + return INTEL_GEN(dev_priv) >= 12 && + intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST) && + 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..e40767f78343 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 <stdbool.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 first 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 scenario will be handled in the next patch. 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_ddi.c | 14 +- drivers/gpu/drm/i915/display/intel_display.c | 15 +- .../drm/i915/display/intel_display_types.h | 3 + drivers/gpu/drm/i915/display/intel_dp_mst.c | 163 ++++++++++++++++++ drivers/gpu/drm/i915/display/intel_dp_mst.h | 5 + 5 files changed, 196 insertions(+), 4 deletions(-)