| Message ID | 20191123005459.155383-3-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] drm/i915/display: Refactor intel_commit_modeset_disables() | expand |
On Fri, Nov 22, 2019 at 04:54:55PM -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. > > A previous approach was using the lowest pipe/transcoder as master > transcoder but as the comment in skl_commit_modeset_enables() states, > that is not always true. > > So here promoting the first pipe/transcoder of the stream as master. > That caused several other problems as during the commit phase the > state computed should not be changed. > > So the master transcoder is store into intel_dp and the modeset in > slave pipes/transcoders is forced using mst_master_trans_pending. > > v2: > - added missing config compute to trigger fullmodeset in slave > transcoders > > 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 | 10 +- > drivers/gpu/drm/i915/display/intel_display.c | 58 ++++++- > .../drm/i915/display/intel_display_types.h | 3 + > drivers/gpu/drm/i915/display/intel_dp.c | 1 + > drivers/gpu/drm/i915/display/intel_dp_mst.c | 149 +++++++++++++++++- > drivers/gpu/drm/i915/display/intel_dp_mst.h | 2 + > 6 files changed, 216 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index a976606d21c7..d2f0d393d3ee 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -35,6 +35,7 @@ > #include "intel_display_types.h" > #include "intel_dp.h" > #include "intel_dp_link_training.h" > +#include "intel_dp_mst.h" > #include "intel_dpio_phy.h" > #include "intel_dsi.h" > #include "intel_fifo_underrun.h" > @@ -1903,8 +1904,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 = intel_dp_mst_master_trans_get(encoder); Why isn't that just stored in the crtc state like everything else? I'm thinking we should maybe do it just like port sync and have both master + slave_mask. That way it should be pretty trivial to add all the relevant crtcs to the state when needed. > + 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); > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 801b975c7d39..35a59108194e 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" > @@ -5365,6 +5366,36 @@ intel_get_crtc_new_encoder(const struct intel_atomic_state *state, > return encoder; > } > > +/* > + * Finds the encoder associated with the given CRTC. This can only be > + * used when we know that the CRTC isn't feeding multiple encoders! > + */ > +static struct intel_encoder * > +intel_get_crtc_old_encoder(const struct intel_atomic_state *state, > + const struct intel_crtc_state *crtc_state) > +{ > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > + const struct drm_connector_state *connector_state; > + const struct drm_connector *connector; > + struct intel_encoder *encoder = NULL; > + int num_encoders = 0; > + int i; > + > + for_each_old_connector_in_state(&state->base, connector, > + connector_state, i) { > + if (connector_state->crtc != &crtc->base) > + continue; > + > + encoder = to_intel_encoder(connector_state->best_encoder); > + num_encoders++; > + } > + > + WARN(num_encoders != 1, "%d encoders for pipe %c\n", > + num_encoders, pipe_name(crtc->pipe)); > + > + return encoder; > +} Argh. I was hoping to kill the other one of these. Got it down to 1 remaining user now I think. > + > /* > * Enable PCH resources required for PCH ports: > * - PCH PLLs > @@ -13365,6 +13396,12 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config, > #undef PIPE_CONF_CHECK_COLOR_LUT > #undef PIPE_CONF_QUIRK > > + if (fastset && pipe_config->mst_master_trans_pending) { > + DRM_DEBUG_KMS("[CRTC:%d:%s] fastset mismatch in mst_master_trans\n", > + crtc->base.base.id, crtc->base.name); > + ret = false; > + } > + > return ret; > } > > @@ -14449,22 +14486,35 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state) > struct intel_crtc *crtc; > 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) || !crtc->active) > continue; > > + if (!is_trans_port_sync_mode(new_crtc_state) && > + !intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST)) > + continue; > + > /* In case of Transcoder port Sync master slave CRTCs can be > * assigned in any order and we need to make sure that > * slave CRTCs are disabled first and then master CRTC since > * Slave vblanks are masked till Master Vblanks. > */ > - if (!is_trans_port_sync_mode(old_crtc_state)) > - continue; > - if (is_trans_port_sync_master(old_crtc_state)) > + if (is_trans_port_sync_mode(new_crtc_state) && > + is_trans_port_sync_master(new_crtc_state)) > continue; > > + if (intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST)) { > + struct intel_encoder *encoder; > + > + encoder = intel_get_crtc_old_encoder(state, > + old_crtc_state); > + if (intel_dp_mst_master_trans_get(encoder) == > + old_crtc_state->cpu_transcoder) > + continue; > + } > + > intel_pre_plane_update(old_crtc_state, new_crtc_state); > intel_old_crtc_state_disables(state, old_crtc_state, > new_crtc_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..23d747cdca64 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -1052,6 +1052,8 @@ struct intel_crtc_state { > /* Pointer to master transcoder in case of tiled displays */ > enum transcoder master_transcoder; > > + bool mst_master_trans_pending; > + > /* Bitmask to indicate slaves attached */ > u8 sync_mode_slaves_mask; > }; > @@ -1284,6 +1286,7 @@ struct intel_dp { > bool can_mst; /* this port supports mst */ > bool is_mst; > int active_mst_links; > + enum transcoder mst_master_transcoder; /* Only used in TGL+ */ > > /* > * DP_TP_* registers may be either on port or transcoder register space. > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 3123958e2081..ceff6901451a 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -7424,6 +7424,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > intel_dp->reset_link_params = true; > intel_dp->pps_pipe = INVALID_PIPE; > intel_dp->active_pipe = INVALID_PIPE; > + intel_dp->mst_master_transcoder = INVALID_TRANSCODER; > > /* Preserve the current hw state. */ > intel_dp->DP = I915_READ(intel_dp->output_reg); > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index f8a350359346..9731c3c1d3f2 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -87,6 +87,47 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > return 0; > } > > +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 drm_i915_private *dev_priv = to_i915(encoder->base.dev); > + struct intel_crtc_state *new_crtc_state; > + struct intel_crtc *crtc; > + enum transcoder master; > + int i; > + > + if (INTEL_GEN(dev_priv) < 12) > + return 0; > + > + if (!conn_state->crtc) > + return 0; > + > + master = intel_dp_mst_master_trans_get(encoder); > + if (master == INVALID_TRANSCODER) > + return 0; > + > + for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) { > + /* > + * cpu_transcoder is set when computing CRTC state if it will > + * be disabled it will not happen, so checking pipe instead > + */ > + if (crtc->pipe != (enum pipe)master) > + continue; > + > + if (!drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi) && > + new_crtc_state->uapi.enable) > + continue; > + > + pipe_config->mst_master_trans_pending = true; > + 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 +195,95 @@ 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 intel_connector *intel_conn = to_intel_connector(connector); > + 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 drm_crtc_state *crtc_state; > + struct drm_connector *conn_iter; > + > + if (INTEL_GEN(dev_priv) < 12) > + return 0; > + > + 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 > + * - 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_conn->mst_port->mst_master_transcoder != > + intel_crtc_state->cpu_transcoder) > + return 0; > + > + /* Is master, mark all other CRTCs as needing a modeset */ > + drm_connector_list_iter_begin(state->dev, &conn_list_iter); > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) { > + struct intel_connector *intel_conn_iter; > + struct drm_connector_state *conn_iter_state; > + > + 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); > + } > + > + intel_crtc_state = to_intel_crtc_state(crtc_state); > + crtc_state->mode_changed = true; > + } > + drm_connector_list_iter_end(&conn_list_iter); > + > return 0; > } > > @@ -175,6 +305,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; > > @@ -259,6 +393,7 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder, > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); > intel_dig_port->base.post_disable(&intel_dig_port->base, > old_crtc_state, NULL); > + intel_dp->mst_master_transcoder = INVALID_TRANSCODER; > } > > DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links); > @@ -314,8 +449,11 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder, > > DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links); > > - if (first_mst_stream) > + if (first_mst_stream) { > + WARN_ON(intel_dp->mst_master_transcoder != INVALID_TRANSCODER); > + intel_dp->mst_master_transcoder = pipe_config->cpu_transcoder; > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > + } > > drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port, true); > > @@ -717,3 +855,12 @@ 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 */ > } > + > +enum transcoder > +intel_dp_mst_master_trans_get(struct intel_encoder *encoder) > +{ > + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base); > + struct intel_dp *intel_dp = &intel_mst->primary->dp; > + > + return intel_dp->mst_master_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..e6f28a517182 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h > @@ -7,9 +7,11 @@ > #define __INTEL_DP_MST_H__ > > struct intel_digital_port; > +struct intel_encoder; > > 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); > +enum transcoder intel_dp_mst_master_trans_get(struct intel_encoder *encoder); > > #endif /* __INTEL_DP_MST_H__ */ > -- > 2.24.0
On Tue, 2019-11-26 at 22:05 +0200, Ville Syrjälä wrote: > On Fri, Nov 22, 2019 at 04:54:55PM -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. > > > > A previous approach was using the lowest pipe/transcoder as master > > transcoder but as the comment in skl_commit_modeset_enables() > > states, > > that is not always true. > > > > So here promoting the first pipe/transcoder of the stream as > > master. > > That caused several other problems as during the commit phase the > > state computed should not be changed. > > > > So the master transcoder is store into intel_dp and the modeset in > > slave pipes/transcoders is forced using mst_master_trans_pending. > > > > v2: > > - added missing config compute to trigger fullmodeset in slave > > transcoders > > > > 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 | 10 +- > > drivers/gpu/drm/i915/display/intel_display.c | 58 ++++++- > > .../drm/i915/display/intel_display_types.h | 3 + > > drivers/gpu/drm/i915/display/intel_dp.c | 1 + > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 149 > > +++++++++++++++++- > > drivers/gpu/drm/i915/display/intel_dp_mst.h | 2 + > > 6 files changed, 216 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > index a976606d21c7..d2f0d393d3ee 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > @@ -35,6 +35,7 @@ > > #include "intel_display_types.h" > > #include "intel_dp.h" > > #include "intel_dp_link_training.h" > > +#include "intel_dp_mst.h" > > #include "intel_dpio_phy.h" > > #include "intel_dsi.h" > > #include "intel_fifo_underrun.h" > > @@ -1903,8 +1904,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 = > > intel_dp_mst_master_trans_get(encoder); > > Why isn't that just stored in the crtc state like everything else? > > I'm thinking we should maybe do it just like port sync and have both > master + slave_mask. That way it should be pretty trivial to add all > the relevant crtcs to the state when needed. I guess port sync is not doing the right thing and it could cause underruns. When it is going to enable the master CRTC of the port sync it forcibly enables the slave first, what could cause underruns because of overlap in ddb allocations(that is what I understood from the comment in skl_commit_modeset_enables()). So for MST we only know who is the master in the commit phase and at this point we should not modify the computed state. > > > + 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); > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > b/drivers/gpu/drm/i915/display/intel_display.c > > index 801b975c7d39..35a59108194e 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" > > @@ -5365,6 +5366,36 @@ intel_get_crtc_new_encoder(const struct > > intel_atomic_state *state, > > return encoder; > > } > > > > +/* > > + * Finds the encoder associated with the given CRTC. This can only > > be > > + * used when we know that the CRTC isn't feeding multiple > > encoders! > > + */ > > +static struct intel_encoder * > > +intel_get_crtc_old_encoder(const struct intel_atomic_state *state, > > + const struct intel_crtc_state *crtc_state) > > +{ > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > + const struct drm_connector_state *connector_state; > > + const struct drm_connector *connector; > > + struct intel_encoder *encoder = NULL; > > + int num_encoders = 0; > > + int i; > > + > > + for_each_old_connector_in_state(&state->base, connector, > > + connector_state, i) { > > + if (connector_state->crtc != &crtc->base) > > + continue; > > + > > + encoder = to_intel_encoder(connector_state- > > >best_encoder); > > + num_encoders++; > > + } > > + > > + WARN(num_encoders != 1, "%d encoders for pipe %c\n", > > + num_encoders, pipe_name(crtc->pipe)); > > + > > + return encoder; > > +} > > Argh. I was hoping to kill the other one of these. Got it down to 1 > remaining user now I think. > > > + > > /* > > * Enable PCH resources required for PCH ports: > > * - PCH PLLs > > @@ -13365,6 +13396,12 @@ intel_pipe_config_compare(const struct > > intel_crtc_state *current_config, > > #undef PIPE_CONF_CHECK_COLOR_LUT > > #undef PIPE_CONF_QUIRK > > > > + if (fastset && pipe_config->mst_master_trans_pending) { > > + DRM_DEBUG_KMS("[CRTC:%d:%s] fastset mismatch in > > mst_master_trans\n", > > + crtc->base.base.id, crtc->base.name); > > + ret = false; > > + } > > + > > return ret; > > } > > > > @@ -14449,22 +14486,35 @@ static void > > intel_commit_modeset_disables(struct intel_atomic_state *state) > > struct intel_crtc *crtc; > > 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) || !crtc->active) > > continue; > > > > + if (!is_trans_port_sync_mode(new_crtc_state) && > > + !intel_crtc_has_type(old_crtc_state, > > INTEL_OUTPUT_DP_MST)) > > + continue; > > + > > /* In case of Transcoder port Sync master slave CRTCs > > can be > > * assigned in any order and we need to make sure that > > * slave CRTCs are disabled first and then master CRTC > > since > > * Slave vblanks are masked till Master Vblanks. > > */ > > - if (!is_trans_port_sync_mode(old_crtc_state)) > > - continue; > > - if (is_trans_port_sync_master(old_crtc_state)) > > + if (is_trans_port_sync_mode(new_crtc_state) && > > + is_trans_port_sync_master(new_crtc_state)) > > continue; > > > > + if (intel_crtc_has_type(old_crtc_state, > > INTEL_OUTPUT_DP_MST)) { > > + struct intel_encoder *encoder; > > + > > + encoder = intel_get_crtc_old_encoder(state, > > + old_crtc_s > > tate); > > + if (intel_dp_mst_master_trans_get(encoder) == > > + old_crtc_state->cpu_transcoder) > > + continue; > > + } > > + > > intel_pre_plane_update(old_crtc_state, new_crtc_state); > > intel_old_crtc_state_disables(state, old_crtc_state, > > new_crtc_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..23d747cdca64 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > @@ -1052,6 +1052,8 @@ struct intel_crtc_state { > > /* Pointer to master transcoder in case of tiled displays */ > > enum transcoder master_transcoder; > > > > + bool mst_master_trans_pending; > > + > > /* Bitmask to indicate slaves attached */ > > u8 sync_mode_slaves_mask; > > }; > > @@ -1284,6 +1286,7 @@ struct intel_dp { > > bool can_mst; /* this port supports mst */ > > bool is_mst; > > int active_mst_links; > > + enum transcoder mst_master_transcoder; /* Only used in TGL+ */ > > > > /* > > * DP_TP_* registers may be either on port or transcoder > > register space. > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > b/drivers/gpu/drm/i915/display/intel_dp.c > > index 3123958e2081..ceff6901451a 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -7424,6 +7424,7 @@ intel_dp_init_connector(struct > > intel_digital_port *intel_dig_port, > > intel_dp->reset_link_params = true; > > intel_dp->pps_pipe = INVALID_PIPE; > > intel_dp->active_pipe = INVALID_PIPE; > > + intel_dp->mst_master_transcoder = INVALID_TRANSCODER; > > > > /* Preserve the current hw state. */ > > intel_dp->DP = I915_READ(intel_dp->output_reg); > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > index f8a350359346..9731c3c1d3f2 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > @@ -87,6 +87,47 @@ static int > > intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > > return 0; > > } > > > > +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 drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > + struct intel_crtc_state *new_crtc_state; > > + struct intel_crtc *crtc; > > + enum transcoder master; > > + int i; > > + > > + if (INTEL_GEN(dev_priv) < 12) > > + return 0; > > + > > + if (!conn_state->crtc) > > + return 0; > > + > > + master = intel_dp_mst_master_trans_get(encoder); > > + if (master == INVALID_TRANSCODER) > > + return 0; > > + > > + for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, > > i) { > > + /* > > + * cpu_transcoder is set when computing CRTC state if > > it will > > + * be disabled it will not happen, so checking pipe > > instead > > + */ > > + if (crtc->pipe != (enum pipe)master) > > + continue; > > + > > + if (!drm_atomic_crtc_needs_modeset(&new_crtc_state- > > >uapi) && > > + new_crtc_state->uapi.enable) > > + continue; > > + > > + pipe_config->mst_master_trans_pending = true; > > + 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 +195,95 @@ 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 intel_connector *intel_conn = > > to_intel_connector(connector); > > + 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 drm_crtc_state *crtc_state; > > + struct drm_connector *conn_iter; > > + > > + if (INTEL_GEN(dev_priv) < 12) > > + return 0; > > + > > + 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 > > + * - 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_conn->mst_port->mst_master_transcoder != > > + intel_crtc_state->cpu_transcoder) > > + return 0; > > + > > + /* Is master, mark all other CRTCs as needing a modeset */ > > + drm_connector_list_iter_begin(state->dev, &conn_list_iter); > > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) { > > + struct intel_connector *intel_conn_iter; > > + struct drm_connector_state *conn_iter_state; > > + > > + 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); > > + } > > + > > + intel_crtc_state = to_intel_crtc_state(crtc_state); > > + crtc_state->mode_changed = true; > > + } > > + drm_connector_list_iter_end(&conn_list_iter); > > + > > return 0; > > } > > > > @@ -175,6 +305,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; > > > > @@ -259,6 +393,7 @@ static void intel_mst_post_disable_dp(struct > > intel_encoder *encoder, > > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); > > intel_dig_port->base.post_disable(&intel_dig_port- > > >base, > > old_crtc_state, > > NULL); > > + intel_dp->mst_master_transcoder = INVALID_TRANSCODER; > > } > > > > DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links); > > @@ -314,8 +449,11 @@ static void intel_mst_pre_enable_dp(struct > > intel_encoder *encoder, > > > > DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links); > > > > - if (first_mst_stream) > > + if (first_mst_stream) { > > + WARN_ON(intel_dp->mst_master_transcoder != > > INVALID_TRANSCODER); > > + intel_dp->mst_master_transcoder = pipe_config- > > >cpu_transcoder; > > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > > + } > > > > drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector- > > >port, true); > > > > @@ -717,3 +855,12 @@ 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 */ > > } > > + > > +enum transcoder > > +intel_dp_mst_master_trans_get(struct intel_encoder *encoder) > > +{ > > + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder- > > >base); > > + struct intel_dp *intel_dp = &intel_mst->primary->dp; > > + > > + return intel_dp->mst_master_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..e6f28a517182 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h > > @@ -7,9 +7,11 @@ > > #define __INTEL_DP_MST_H__ > > > > struct intel_digital_port; > > +struct intel_encoder; > > > > 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); > > +enum transcoder intel_dp_mst_master_trans_get(struct intel_encoder > > *encoder); > > > > #endif /* __INTEL_DP_MST_H__ */ > > -- > > 2.24.0
On Tue, Nov 26, 2019 at 08:30:31PM +0000, Souza, Jose wrote: > On Tue, 2019-11-26 at 22:05 +0200, Ville Syrjälä wrote: > > On Fri, Nov 22, 2019 at 04:54:55PM -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. > > > > > > A previous approach was using the lowest pipe/transcoder as master > > > transcoder but as the comment in skl_commit_modeset_enables() > > > states, > > > that is not always true. > > > > > > So here promoting the first pipe/transcoder of the stream as > > > master. > > > That caused several other problems as during the commit phase the > > > state computed should not be changed. > > > > > > So the master transcoder is store into intel_dp and the modeset in > > > slave pipes/transcoders is forced using mst_master_trans_pending. > > > > > > v2: > > > - added missing config compute to trigger fullmodeset in slave > > > transcoders > > > > > > 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 | 10 +- > > > drivers/gpu/drm/i915/display/intel_display.c | 58 ++++++- > > > .../drm/i915/display/intel_display_types.h | 3 + > > > drivers/gpu/drm/i915/display/intel_dp.c | 1 + > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 149 > > > +++++++++++++++++- > > > drivers/gpu/drm/i915/display/intel_dp_mst.h | 2 + > > > 6 files changed, 216 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > > index a976606d21c7..d2f0d393d3ee 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > > @@ -35,6 +35,7 @@ > > > #include "intel_display_types.h" > > > #include "intel_dp.h" > > > #include "intel_dp_link_training.h" > > > +#include "intel_dp_mst.h" > > > #include "intel_dpio_phy.h" > > > #include "intel_dsi.h" > > > #include "intel_fifo_underrun.h" > > > @@ -1903,8 +1904,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 = > > > intel_dp_mst_master_trans_get(encoder); > > > > Why isn't that just stored in the crtc state like everything else? > > > > I'm thinking we should maybe do it just like port sync and have both > > master + slave_mask. That way it should be pretty trivial to add all > > the relevant crtcs to the state when needed. > > I guess port sync is not doing the right thing and it could cause > underruns. > When it is going to enable the master CRTC of the port sync it forcibly > enables the slave first, what could cause underruns because of overlap > in ddb allocations(that is what I understood from the comment in > skl_commit_modeset_enables()). Not necessarily just underruns but even a system hang. The fix should be a trivial "check the slave for ddb overlap as well", but apparently I failed at convicing people to do that. I've actually been pondering about decoupling the plane updates from the crtc enable stuff entirely. At least theoretically crtc enable should be able to excute in any order as long we don't enable any new planes. But none of that really matters for the discussion at hand. Though there are other problems with the port sync stuff that would need to be handled better. Eg. I think it now adds both crtcs to the state always which is going to cut the fps in half. Also the place where it does that stuff is rather suspicious. All that stuff should be somewhere a bit higher up IMO. > > So for MST we only know who is the master in the commit phase and at > this point we should not modify the computed state. I'm not suggesting modifying anything during commit phase. I think you are effectiely doing that right now by stuffing that mst master transcoder into intel_dp. > > > > > > + 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); > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > index 801b975c7d39..35a59108194e 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" > > > @@ -5365,6 +5366,36 @@ intel_get_crtc_new_encoder(const struct > > > intel_atomic_state *state, > > > return encoder; > > > } > > > > > > +/* > > > + * Finds the encoder associated with the given CRTC. This can only > > > be > > > + * used when we know that the CRTC isn't feeding multiple > > > encoders! > > > + */ > > > +static struct intel_encoder * > > > +intel_get_crtc_old_encoder(const struct intel_atomic_state *state, > > > + const struct intel_crtc_state *crtc_state) > > > +{ > > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > > + const struct drm_connector_state *connector_state; > > > + const struct drm_connector *connector; > > > + struct intel_encoder *encoder = NULL; > > > + int num_encoders = 0; > > > + int i; > > > + > > > + for_each_old_connector_in_state(&state->base, connector, > > > + connector_state, i) { > > > + if (connector_state->crtc != &crtc->base) > > > + continue; > > > + > > > + encoder = to_intel_encoder(connector_state- > > > >best_encoder); > > > + num_encoders++; > > > + } > > > + > > > + WARN(num_encoders != 1, "%d encoders for pipe %c\n", > > > + num_encoders, pipe_name(crtc->pipe)); > > > + > > > + return encoder; > > > +} > > > > Argh. I was hoping to kill the other one of these. Got it down to 1 > > remaining user now I think. > > > > > + > > > /* > > > * Enable PCH resources required for PCH ports: > > > * - PCH PLLs > > > @@ -13365,6 +13396,12 @@ intel_pipe_config_compare(const struct > > > intel_crtc_state *current_config, > > > #undef PIPE_CONF_CHECK_COLOR_LUT > > > #undef PIPE_CONF_QUIRK > > > > > > + if (fastset && pipe_config->mst_master_trans_pending) { > > > + DRM_DEBUG_KMS("[CRTC:%d:%s] fastset mismatch in > > > mst_master_trans\n", > > > + crtc->base.base.id, crtc->base.name); > > > + ret = false; > > > + } > > > + > > > return ret; > > > } > > > > > > @@ -14449,22 +14486,35 @@ static void > > > intel_commit_modeset_disables(struct intel_atomic_state *state) > > > struct intel_crtc *crtc; > > > 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) || !crtc->active) > > > continue; > > > > > > + if (!is_trans_port_sync_mode(new_crtc_state) && > > > + !intel_crtc_has_type(old_crtc_state, > > > INTEL_OUTPUT_DP_MST)) > > > + continue; > > > + > > > /* In case of Transcoder port Sync master slave CRTCs > > > can be > > > * assigned in any order and we need to make sure that > > > * slave CRTCs are disabled first and then master CRTC > > > since > > > * Slave vblanks are masked till Master Vblanks. > > > */ > > > - if (!is_trans_port_sync_mode(old_crtc_state)) > > > - continue; > > > - if (is_trans_port_sync_master(old_crtc_state)) > > > + if (is_trans_port_sync_mode(new_crtc_state) && > > > + is_trans_port_sync_master(new_crtc_state)) > > > continue; > > > > > > + if (intel_crtc_has_type(old_crtc_state, > > > INTEL_OUTPUT_DP_MST)) { > > > + struct intel_encoder *encoder; > > > + > > > + encoder = intel_get_crtc_old_encoder(state, > > > + old_crtc_s > > > tate); > > > + if (intel_dp_mst_master_trans_get(encoder) == > > > + old_crtc_state->cpu_transcoder) > > > + continue; > > > + } > > > + > > > intel_pre_plane_update(old_crtc_state, new_crtc_state); > > > intel_old_crtc_state_disables(state, old_crtc_state, > > > new_crtc_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..23d747cdca64 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > @@ -1052,6 +1052,8 @@ struct intel_crtc_state { > > > /* Pointer to master transcoder in case of tiled displays */ > > > enum transcoder master_transcoder; > > > > > > + bool mst_master_trans_pending; > > > + > > > /* Bitmask to indicate slaves attached */ > > > u8 sync_mode_slaves_mask; > > > }; > > > @@ -1284,6 +1286,7 @@ struct intel_dp { > > > bool can_mst; /* this port supports mst */ > > > bool is_mst; > > > int active_mst_links; > > > + enum transcoder mst_master_transcoder; /* Only used in TGL+ */ > > > > > > /* > > > * DP_TP_* registers may be either on port or transcoder > > > register space. > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > > b/drivers/gpu/drm/i915/display/intel_dp.c > > > index 3123958e2081..ceff6901451a 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > @@ -7424,6 +7424,7 @@ intel_dp_init_connector(struct > > > intel_digital_port *intel_dig_port, > > > intel_dp->reset_link_params = true; > > > intel_dp->pps_pipe = INVALID_PIPE; > > > intel_dp->active_pipe = INVALID_PIPE; > > > + intel_dp->mst_master_transcoder = INVALID_TRANSCODER; > > > > > > /* Preserve the current hw state. */ > > > intel_dp->DP = I915_READ(intel_dp->output_reg); > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > index f8a350359346..9731c3c1d3f2 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > @@ -87,6 +87,47 @@ static int > > > intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > > > return 0; > > > } > > > > > > +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 drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > > + struct intel_crtc_state *new_crtc_state; > > > + struct intel_crtc *crtc; > > > + enum transcoder master; > > > + int i; > > > + > > > + if (INTEL_GEN(dev_priv) < 12) > > > + return 0; > > > + > > > + if (!conn_state->crtc) > > > + return 0; > > > + > > > + master = intel_dp_mst_master_trans_get(encoder); > > > + if (master == INVALID_TRANSCODER) > > > + return 0; > > > + > > > + for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, > > > i) { > > > + /* > > > + * cpu_transcoder is set when computing CRTC state if > > > it will > > > + * be disabled it will not happen, so checking pipe > > > instead > > > + */ > > > + if (crtc->pipe != (enum pipe)master) > > > + continue; > > > + > > > + if (!drm_atomic_crtc_needs_modeset(&new_crtc_state- > > > >uapi) && > > > + new_crtc_state->uapi.enable) > > > + continue; > > > + > > > + pipe_config->mst_master_trans_pending = true; > > > + 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 +195,95 @@ 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 intel_connector *intel_conn = > > > to_intel_connector(connector); > > > + 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 drm_crtc_state *crtc_state; > > > + struct drm_connector *conn_iter; > > > + > > > + if (INTEL_GEN(dev_priv) < 12) > > > + return 0; > > > + > > > + 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 > > > + * - 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_conn->mst_port->mst_master_transcoder != > > > + intel_crtc_state->cpu_transcoder) > > > + return 0; > > > + > > > + /* Is master, mark all other CRTCs as needing a modeset */ > > > + drm_connector_list_iter_begin(state->dev, &conn_list_iter); > > > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) { > > > + struct intel_connector *intel_conn_iter; > > > + struct drm_connector_state *conn_iter_state; > > > + > > > + 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); > > > + } > > > + > > > + intel_crtc_state = to_intel_crtc_state(crtc_state); > > > + crtc_state->mode_changed = true; > > > + } > > > + drm_connector_list_iter_end(&conn_list_iter); > > > + > > > return 0; > > > } > > > > > > @@ -175,6 +305,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; > > > > > > @@ -259,6 +393,7 @@ static void intel_mst_post_disable_dp(struct > > > intel_encoder *encoder, > > > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); > > > intel_dig_port->base.post_disable(&intel_dig_port- > > > >base, > > > old_crtc_state, > > > NULL); > > > + intel_dp->mst_master_transcoder = INVALID_TRANSCODER; > > > } > > > > > > DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links); > > > @@ -314,8 +449,11 @@ static void intel_mst_pre_enable_dp(struct > > > intel_encoder *encoder, > > > > > > DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links); > > > > > > - if (first_mst_stream) > > > + if (first_mst_stream) { > > > + WARN_ON(intel_dp->mst_master_transcoder != > > > INVALID_TRANSCODER); > > > + intel_dp->mst_master_transcoder = pipe_config- > > > >cpu_transcoder; > > > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > > > + } > > > > > > drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector- > > > >port, true); > > > > > > @@ -717,3 +855,12 @@ 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 */ > > > } > > > + > > > +enum transcoder > > > +intel_dp_mst_master_trans_get(struct intel_encoder *encoder) > > > +{ > > > + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder- > > > >base); > > > + struct intel_dp *intel_dp = &intel_mst->primary->dp; > > > + > > > + return intel_dp->mst_master_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..e6f28a517182 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h > > > @@ -7,9 +7,11 @@ > > > #define __INTEL_DP_MST_H__ > > > > > > struct intel_digital_port; > > > +struct intel_encoder; > > > > > > 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); > > > +enum transcoder intel_dp_mst_master_trans_get(struct intel_encoder > > > *encoder); > > > > > > #endif /* __INTEL_DP_MST_H__ */ > > > -- > > > 2.24.0
On Wed, 2019-11-27 at 21:59 +0200, Ville Syrjälä wrote: > On Tue, Nov 26, 2019 at 08:30:31PM +0000, Souza, Jose wrote: > > On Tue, 2019-11-26 at 22:05 +0200, Ville Syrjälä wrote: > > > On Fri, Nov 22, 2019 at 04:54:55PM -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. > > > > > > > > A previous approach was using the lowest pipe/transcoder as > > > > master > > > > transcoder but as the comment in skl_commit_modeset_enables() > > > > states, > > > > that is not always true. > > > > > > > > So here promoting the first pipe/transcoder of the stream as > > > > master. > > > > That caused several other problems as during the commit phase > > > > the > > > > state computed should not be changed. > > > > > > > > So the master transcoder is store into intel_dp and the modeset > > > > in > > > > slave pipes/transcoders is forced using > > > > mst_master_trans_pending. > > > > > > > > v2: > > > > - added missing config compute to trigger fullmodeset in slave > > > > transcoders > > > > > > > > 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 | 10 +- > > > > drivers/gpu/drm/i915/display/intel_display.c | 58 ++++++- > > > > .../drm/i915/display/intel_display_types.h | 3 + > > > > drivers/gpu/drm/i915/display/intel_dp.c | 1 + > > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 149 > > > > +++++++++++++++++- > > > > drivers/gpu/drm/i915/display/intel_dp_mst.h | 2 + > > > > 6 files changed, 216 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > > > index a976606d21c7..d2f0d393d3ee 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > > > @@ -35,6 +35,7 @@ > > > > #include "intel_display_types.h" > > > > #include "intel_dp.h" > > > > #include "intel_dp_link_training.h" > > > > +#include "intel_dp_mst.h" > > > > #include "intel_dpio_phy.h" > > > > #include "intel_dsi.h" > > > > #include "intel_fifo_underrun.h" > > > > @@ -1903,8 +1904,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 = > > > > intel_dp_mst_master_trans_get(encoder); > > > > > > Why isn't that just stored in the crtc state like everything > > > else? > > > > > > I'm thinking we should maybe do it just like port sync and have > > > both > > > master + slave_mask. That way it should be pretty trivial to add > > > all > > > the relevant crtcs to the state when needed. > > > > I guess port sync is not doing the right thing and it could cause > > underruns. > > When it is going to enable the master CRTC of the port sync it > > forcibly > > enables the slave first, what could cause underruns because of > > overlap > > in ddb allocations(that is what I understood from the comment in > > skl_commit_modeset_enables()). > > Not necessarily just underruns but even a system hang. The fix should > be > a trivial "check the slave for ddb overlap as well", but apparently I > failed at convicing people to do that. > > I've actually been pondering about decoupling the plane updates from > the crtc enable stuff entirely. At least theoretically crtc enable > should be able to excute in any order as long we don't enable any > new planes. > > But none of that really matters for the discussion at hand. Though > there are other problems with the port sync stuff that would need > to be handled better. Eg. I think it now adds both crtcs to the state > always which is going to cut the fps in half. Also the place where > it does that stuff is rather suspicious. All that stuff should be > somewhere a bit higher up IMO. > > > So for MST we only know who is the master in the commit phase and > > at > > this point we should not modify the computed state. > > I'm not suggesting modifying anything during commit phase. I think > you are effectiely doing that right now by stuffing that mst master > transcoder into intel_dp. Sorry, I still don't get what approach are you suggesting here. If we can't modify the state in the commit phase, adding mst_master_transcoder in the CRTC state will not be possible while respecting the order imposed by ddb allocations. > > > > > + 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); > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > > index 801b975c7d39..35a59108194e 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" > > > > @@ -5365,6 +5366,36 @@ intel_get_crtc_new_encoder(const struct > > > > intel_atomic_state *state, > > > > return encoder; > > > > } > > > > > > > > +/* > > > > + * Finds the encoder associated with the given CRTC. This can > > > > only > > > > be > > > > + * used when we know that the CRTC isn't feeding multiple > > > > encoders! > > > > + */ > > > > +static struct intel_encoder * > > > > +intel_get_crtc_old_encoder(const struct intel_atomic_state > > > > *state, > > > > + const struct intel_crtc_state > > > > *crtc_state) > > > > +{ > > > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state- > > > > >uapi.crtc); > > > > + const struct drm_connector_state *connector_state; > > > > + const struct drm_connector *connector; > > > > + struct intel_encoder *encoder = NULL; > > > > + int num_encoders = 0; > > > > + int i; > > > > + > > > > + for_each_old_connector_in_state(&state->base, > > > > connector, > > > > + connector_state, i) { > > > > + if (connector_state->crtc != &crtc->base) > > > > + continue; > > > > + > > > > + encoder = to_intel_encoder(connector_state- > > > > > best_encoder); > > > > + num_encoders++; > > > > + } > > > > + > > > > + WARN(num_encoders != 1, "%d encoders for pipe %c\n", > > > > + num_encoders, pipe_name(crtc->pipe)); > > > > + > > > > + return encoder; > > > > +} > > > > > > Argh. I was hoping to kill the other one of these. Got it down to > > > 1 > > > remaining user now I think. > > > > > > > + > > > > /* > > > > * Enable PCH resources required for PCH ports: > > > > * - PCH PLLs > > > > @@ -13365,6 +13396,12 @@ intel_pipe_config_compare(const struct > > > > intel_crtc_state *current_config, > > > > #undef PIPE_CONF_CHECK_COLOR_LUT > > > > #undef PIPE_CONF_QUIRK > > > > > > > > + if (fastset && pipe_config->mst_master_trans_pending) { > > > > + DRM_DEBUG_KMS("[CRTC:%d:%s] fastset mismatch in > > > > mst_master_trans\n", > > > > + crtc->base.base.id, crtc- > > > > >base.name); > > > > + ret = false; > > > > + } > > > > + > > > > return ret; > > > > } > > > > > > > > @@ -14449,22 +14486,35 @@ static void > > > > intel_commit_modeset_disables(struct intel_atomic_state *state) > > > > struct intel_crtc *crtc; > > > > 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) || !crtc- > > > > >active) > > > > continue; > > > > > > > > + if (!is_trans_port_sync_mode(new_crtc_state) && > > > > + !intel_crtc_has_type(old_crtc_state, > > > > INTEL_OUTPUT_DP_MST)) > > > > + continue; > > > > + > > > > /* In case of Transcoder port Sync master slave > > > > CRTCs > > > > can be > > > > * assigned in any order and we need to make > > > > sure that > > > > * slave CRTCs are disabled first and then > > > > master CRTC > > > > since > > > > * Slave vblanks are masked till Master > > > > Vblanks. > > > > */ > > > > - if (!is_trans_port_sync_mode(old_crtc_state)) > > > > - continue; > > > > - if (is_trans_port_sync_master(old_crtc_state)) > > > > + if (is_trans_port_sync_mode(new_crtc_state) && > > > > + is_trans_port_sync_master(new_crtc_state)) > > > > continue; > > > > > > > > + if (intel_crtc_has_type(old_crtc_state, > > > > INTEL_OUTPUT_DP_MST)) { > > > > + struct intel_encoder *encoder; > > > > + > > > > + encoder = > > > > intel_get_crtc_old_encoder(state, > > > > + ol > > > > d_crtc_s > > > > tate); > > > > + if > > > > (intel_dp_mst_master_trans_get(encoder) == > > > > + old_crtc_state->cpu_transcoder) > > > > + continue; > > > > + } > > > > + > > > > intel_pre_plane_update(old_crtc_state, > > > > new_crtc_state); > > > > intel_old_crtc_state_disables(state, > > > > old_crtc_state, > > > > new_crtc_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..23d747cdca64 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > @@ -1052,6 +1052,8 @@ struct intel_crtc_state { > > > > /* Pointer to master transcoder in case of tiled > > > > displays */ > > > > enum transcoder master_transcoder; > > > > > > > > + bool mst_master_trans_pending; > > > > + > > > > /* Bitmask to indicate slaves attached */ > > > > u8 sync_mode_slaves_mask; > > > > }; > > > > @@ -1284,6 +1286,7 @@ struct intel_dp { > > > > bool can_mst; /* this port supports mst */ > > > > bool is_mst; > > > > int active_mst_links; > > > > + enum transcoder mst_master_transcoder; /* Only used in > > > > TGL+ */ > > > > > > > > /* > > > > * DP_TP_* registers may be either on port or > > > > transcoder > > > > register space. > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > > > b/drivers/gpu/drm/i915/display/intel_dp.c > > > > index 3123958e2081..ceff6901451a 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > > @@ -7424,6 +7424,7 @@ intel_dp_init_connector(struct > > > > intel_digital_port *intel_dig_port, > > > > intel_dp->reset_link_params = true; > > > > intel_dp->pps_pipe = INVALID_PIPE; > > > > intel_dp->active_pipe = INVALID_PIPE; > > > > + intel_dp->mst_master_transcoder = INVALID_TRANSCODER; > > > > > > > > /* Preserve the current hw state. */ > > > > intel_dp->DP = I915_READ(intel_dp->output_reg); > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > index f8a350359346..9731c3c1d3f2 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > @@ -87,6 +87,47 @@ static int > > > > intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > > > > return 0; > > > > } > > > > > > > > +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 drm_i915_private *dev_priv = to_i915(encoder- > > > > >base.dev); > > > > + struct intel_crtc_state *new_crtc_state; > > > > + struct intel_crtc *crtc; > > > > + enum transcoder master; > > > > + int i; > > > > + > > > > + if (INTEL_GEN(dev_priv) < 12) > > > > + return 0; > > > > + > > > > + if (!conn_state->crtc) > > > > + return 0; > > > > + > > > > + master = intel_dp_mst_master_trans_get(encoder); > > > > + if (master == INVALID_TRANSCODER) > > > > + return 0; > > > > + > > > > + for_each_new_intel_crtc_in_state(state, crtc, > > > > new_crtc_state, > > > > i) { > > > > + /* > > > > + * cpu_transcoder is set when computing CRTC > > > > state if > > > > it will > > > > + * be disabled it will not happen, so checking > > > > pipe > > > > instead > > > > + */ > > > > + if (crtc->pipe != (enum pipe)master) > > > > + continue; > > > > + > > > > + if > > > > (!drm_atomic_crtc_needs_modeset(&new_crtc_state- > > > > > uapi) && > > > > + new_crtc_state->uapi.enable) > > > > + continue; > > > > + > > > > + pipe_config->mst_master_trans_pending = true; > > > > + 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 +195,95 @@ 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 intel_connector *intel_conn = > > > > to_intel_connector(connector); > > > > + 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 drm_crtc_state *crtc_state; > > > > + struct drm_connector *conn_iter; > > > > + > > > > + if (INTEL_GEN(dev_priv) < 12) > > > > + return 0; > > > > + > > > > + 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 > > > > + * - 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_conn->mst_port->mst_master_transcoder != > > > > + intel_crtc_state->cpu_transcoder) > > > > + return 0; > > > > + > > > > + /* Is master, mark all other CRTCs as needing a modeset > > > > */ > > > > + drm_connector_list_iter_begin(state->dev, > > > > &conn_list_iter); > > > > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) > > > > { > > > > + struct intel_connector *intel_conn_iter; > > > > + struct drm_connector_state *conn_iter_state; > > > > + > > > > + 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); > > > > + } > > > > + > > > > + intel_crtc_state = > > > > to_intel_crtc_state(crtc_state); > > > > + crtc_state->mode_changed = true; > > > > + } > > > > + drm_connector_list_iter_end(&conn_list_iter); > > > > + > > > > return 0; > > > > } > > > > > > > > @@ -175,6 +305,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; > > > > > > > > @@ -259,6 +393,7 @@ static void > > > > intel_mst_post_disable_dp(struct > > > > intel_encoder *encoder, > > > > intel_dp_sink_dpms(intel_dp, > > > > DRM_MODE_DPMS_OFF); > > > > intel_dig_port- > > > > >base.post_disable(&intel_dig_port- > > > > > base, > > > > old_crtc_stat > > > > e, > > > > NULL); > > > > + intel_dp->mst_master_transcoder = > > > > INVALID_TRANSCODER; > > > > } > > > > > > > > DRM_DEBUG_KMS("active links %d\n", intel_dp- > > > > >active_mst_links); > > > > @@ -314,8 +449,11 @@ static void intel_mst_pre_enable_dp(struct > > > > intel_encoder *encoder, > > > > > > > > DRM_DEBUG_KMS("active links %d\n", intel_dp- > > > > >active_mst_links); > > > > > > > > - if (first_mst_stream) > > > > + if (first_mst_stream) { > > > > + WARN_ON(intel_dp->mst_master_transcoder != > > > > INVALID_TRANSCODER); > > > > + intel_dp->mst_master_transcoder = pipe_config- > > > > > cpu_transcoder; > > > > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > > > > + } > > > > > > > > drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, > > > > connector- > > > > > port, true); > > > > > > > > @@ -717,3 +855,12 @@ 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 */ > > > > } > > > > + > > > > +enum transcoder > > > > +intel_dp_mst_master_trans_get(struct intel_encoder *encoder) > > > > +{ > > > > + struct intel_dp_mst_encoder *intel_mst = > > > > enc_to_mst(&encoder- > > > > > base); > > > > + struct intel_dp *intel_dp = &intel_mst->primary->dp; > > > > + > > > > + return intel_dp->mst_master_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..e6f28a517182 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h > > > > @@ -7,9 +7,11 @@ > > > > #define __INTEL_DP_MST_H__ > > > > > > > > struct intel_digital_port; > > > > +struct intel_encoder; > > > > > > > > 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); > > > > +enum transcoder intel_dp_mst_master_trans_get(struct > > > > intel_encoder > > > > *encoder); > > > > > > > > #endif /* __INTEL_DP_MST_H__ */ > > > > -- > > > > 2.24.0
On Thu, Nov 28, 2019 at 01:14:37AM +0000, Souza, Jose wrote: > On Wed, 2019-11-27 at 21:59 +0200, Ville Syrjälä wrote: > > On Tue, Nov 26, 2019 at 08:30:31PM +0000, Souza, Jose wrote: > > > On Tue, 2019-11-26 at 22:05 +0200, Ville Syrjälä wrote: > > > > On Fri, Nov 22, 2019 at 04:54:55PM -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. > > > > > > > > > > A previous approach was using the lowest pipe/transcoder as > > > > > master > > > > > transcoder but as the comment in skl_commit_modeset_enables() > > > > > states, > > > > > that is not always true. > > > > > > > > > > So here promoting the first pipe/transcoder of the stream as > > > > > master. > > > > > That caused several other problems as during the commit phase > > > > > the > > > > > state computed should not be changed. > > > > > > > > > > So the master transcoder is store into intel_dp and the modeset > > > > > in > > > > > slave pipes/transcoders is forced using > > > > > mst_master_trans_pending. > > > > > > > > > > v2: > > > > > - added missing config compute to trigger fullmodeset in slave > > > > > transcoders > > > > > > > > > > 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 | 10 +- > > > > > drivers/gpu/drm/i915/display/intel_display.c | 58 ++++++- > > > > > .../drm/i915/display/intel_display_types.h | 3 + > > > > > drivers/gpu/drm/i915/display/intel_dp.c | 1 + > > > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 149 > > > > > +++++++++++++++++- > > > > > drivers/gpu/drm/i915/display/intel_dp_mst.h | 2 + > > > > > 6 files changed, 216 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > index a976606d21c7..d2f0d393d3ee 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > @@ -35,6 +35,7 @@ > > > > > #include "intel_display_types.h" > > > > > #include "intel_dp.h" > > > > > #include "intel_dp_link_training.h" > > > > > +#include "intel_dp_mst.h" > > > > > #include "intel_dpio_phy.h" > > > > > #include "intel_dsi.h" > > > > > #include "intel_fifo_underrun.h" > > > > > @@ -1903,8 +1904,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 = > > > > > intel_dp_mst_master_trans_get(encoder); > > > > > > > > Why isn't that just stored in the crtc state like everything > > > > else? > > > > > > > > I'm thinking we should maybe do it just like port sync and have > > > > both > > > > master + slave_mask. That way it should be pretty trivial to add > > > > all > > > > the relevant crtcs to the state when needed. > > > > > > I guess port sync is not doing the right thing and it could cause > > > underruns. > > > When it is going to enable the master CRTC of the port sync it > > > forcibly > > > enables the slave first, what could cause underruns because of > > > overlap > > > in ddb allocations(that is what I understood from the comment in > > > skl_commit_modeset_enables()). > > > > Not necessarily just underruns but even a system hang. The fix should > > be > > a trivial "check the slave for ddb overlap as well", but apparently I > > failed at convicing people to do that. > > > > I've actually been pondering about decoupling the plane updates from > > the crtc enable stuff entirely. At least theoretically crtc enable > > should be able to excute in any order as long we don't enable any > > new planes. > > > > But none of that really matters for the discussion at hand. Though > > there are other problems with the port sync stuff that would need > > to be handled better. Eg. I think it now adds both crtcs to the state > > always which is going to cut the fps in half. Also the place where > > it does that stuff is rather suspicious. All that stuff should be > > somewhere a bit higher up IMO. > > > > > So for MST we only know who is the master in the commit phase and > > > at > > > this point we should not modify the computed state. > > > > I'm not suggesting modifying anything during commit phase. I think > > you are effectiely doing that right now by stuffing that mst master > > transcoder into intel_dp. > > Sorry, I still don't get what approach are you suggesting here. > > If we can't modify the state in the commit phase, adding > mst_master_transcoder in the CRTC state will not be possible while > respecting the order imposed by ddb allocations. The ddb allocation ordering only comes into play when there are already active pipes. It should always be possible to not enable the slaves until the master has been shuffled into place in the ddb and enabled. > > > > > > > > + 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); > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > > > index 801b975c7d39..35a59108194e 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" > > > > > @@ -5365,6 +5366,36 @@ intel_get_crtc_new_encoder(const struct > > > > > intel_atomic_state *state, > > > > > return encoder; > > > > > } > > > > > > > > > > +/* > > > > > + * Finds the encoder associated with the given CRTC. This can > > > > > only > > > > > be > > > > > + * used when we know that the CRTC isn't feeding multiple > > > > > encoders! > > > > > + */ > > > > > +static struct intel_encoder * > > > > > +intel_get_crtc_old_encoder(const struct intel_atomic_state > > > > > *state, > > > > > + const struct intel_crtc_state > > > > > *crtc_state) > > > > > +{ > > > > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state- > > > > > >uapi.crtc); > > > > > + const struct drm_connector_state *connector_state; > > > > > + const struct drm_connector *connector; > > > > > + struct intel_encoder *encoder = NULL; > > > > > + int num_encoders = 0; > > > > > + int i; > > > > > + > > > > > + for_each_old_connector_in_state(&state->base, > > > > > connector, > > > > > + connector_state, i) { > > > > > + if (connector_state->crtc != &crtc->base) > > > > > + continue; > > > > > + > > > > > + encoder = to_intel_encoder(connector_state- > > > > > > best_encoder); > > > > > + num_encoders++; > > > > > + } > > > > > + > > > > > + WARN(num_encoders != 1, "%d encoders for pipe %c\n", > > > > > + num_encoders, pipe_name(crtc->pipe)); > > > > > + > > > > > + return encoder; > > > > > +} > > > > > > > > Argh. I was hoping to kill the other one of these. Got it down to > > > > 1 > > > > remaining user now I think. > > > > > > > > > + > > > > > /* > > > > > * Enable PCH resources required for PCH ports: > > > > > * - PCH PLLs > > > > > @@ -13365,6 +13396,12 @@ intel_pipe_config_compare(const struct > > > > > intel_crtc_state *current_config, > > > > > #undef PIPE_CONF_CHECK_COLOR_LUT > > > > > #undef PIPE_CONF_QUIRK > > > > > > > > > > + if (fastset && pipe_config->mst_master_trans_pending) { > > > > > + DRM_DEBUG_KMS("[CRTC:%d:%s] fastset mismatch in > > > > > mst_master_trans\n", > > > > > + crtc->base.base.id, crtc- > > > > > >base.name); > > > > > + ret = false; > > > > > + } > > > > > + > > > > > return ret; > > > > > } > > > > > > > > > > @@ -14449,22 +14486,35 @@ static void > > > > > intel_commit_modeset_disables(struct intel_atomic_state *state) > > > > > struct intel_crtc *crtc; > > > > > 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) || !crtc- > > > > > >active) > > > > > continue; > > > > > > > > > > + if (!is_trans_port_sync_mode(new_crtc_state) && > > > > > + !intel_crtc_has_type(old_crtc_state, > > > > > INTEL_OUTPUT_DP_MST)) > > > > > + continue; > > > > > + > > > > > /* In case of Transcoder port Sync master slave > > > > > CRTCs > > > > > can be > > > > > * assigned in any order and we need to make > > > > > sure that > > > > > * slave CRTCs are disabled first and then > > > > > master CRTC > > > > > since > > > > > * Slave vblanks are masked till Master > > > > > Vblanks. > > > > > */ > > > > > - if (!is_trans_port_sync_mode(old_crtc_state)) > > > > > - continue; > > > > > - if (is_trans_port_sync_master(old_crtc_state)) > > > > > + if (is_trans_port_sync_mode(new_crtc_state) && > > > > > + is_trans_port_sync_master(new_crtc_state)) > > > > > continue; > > > > > > > > > > + if (intel_crtc_has_type(old_crtc_state, > > > > > INTEL_OUTPUT_DP_MST)) { > > > > > + struct intel_encoder *encoder; > > > > > + > > > > > + encoder = > > > > > intel_get_crtc_old_encoder(state, > > > > > + ol > > > > > d_crtc_s > > > > > tate); > > > > > + if > > > > > (intel_dp_mst_master_trans_get(encoder) == > > > > > + old_crtc_state->cpu_transcoder) > > > > > + continue; > > > > > + } > > > > > + > > > > > intel_pre_plane_update(old_crtc_state, > > > > > new_crtc_state); > > > > > intel_old_crtc_state_disables(state, > > > > > old_crtc_state, > > > > > new_crtc_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..23d747cdca64 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > @@ -1052,6 +1052,8 @@ struct intel_crtc_state { > > > > > /* Pointer to master transcoder in case of tiled > > > > > displays */ > > > > > enum transcoder master_transcoder; > > > > > > > > > > + bool mst_master_trans_pending; > > > > > + > > > > > /* Bitmask to indicate slaves attached */ > > > > > u8 sync_mode_slaves_mask; > > > > > }; > > > > > @@ -1284,6 +1286,7 @@ struct intel_dp { > > > > > bool can_mst; /* this port supports mst */ > > > > > bool is_mst; > > > > > int active_mst_links; > > > > > + enum transcoder mst_master_transcoder; /* Only used in > > > > > TGL+ */ > > > > > > > > > > /* > > > > > * DP_TP_* registers may be either on port or > > > > > transcoder > > > > > register space. > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > > > > b/drivers/gpu/drm/i915/display/intel_dp.c > > > > > index 3123958e2081..ceff6901451a 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > > > @@ -7424,6 +7424,7 @@ intel_dp_init_connector(struct > > > > > intel_digital_port *intel_dig_port, > > > > > intel_dp->reset_link_params = true; > > > > > intel_dp->pps_pipe = INVALID_PIPE; > > > > > intel_dp->active_pipe = INVALID_PIPE; > > > > > + intel_dp->mst_master_transcoder = INVALID_TRANSCODER; > > > > > > > > > > /* Preserve the current hw state. */ > > > > > intel_dp->DP = I915_READ(intel_dp->output_reg); > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > > index f8a350359346..9731c3c1d3f2 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > > @@ -87,6 +87,47 @@ static int > > > > > intel_dp_mst_compute_link_config(struct intel_encoder *encoder, > > > > > return 0; > > > > > } > > > > > > > > > > +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 drm_i915_private *dev_priv = to_i915(encoder- > > > > > >base.dev); > > > > > + struct intel_crtc_state *new_crtc_state; > > > > > + struct intel_crtc *crtc; > > > > > + enum transcoder master; > > > > > + int i; > > > > > + > > > > > + if (INTEL_GEN(dev_priv) < 12) > > > > > + return 0; > > > > > + > > > > > + if (!conn_state->crtc) > > > > > + return 0; > > > > > + > > > > > + master = intel_dp_mst_master_trans_get(encoder); > > > > > + if (master == INVALID_TRANSCODER) > > > > > + return 0; > > > > > + > > > > > + for_each_new_intel_crtc_in_state(state, crtc, > > > > > new_crtc_state, > > > > > i) { > > > > > + /* > > > > > + * cpu_transcoder is set when computing CRTC > > > > > state if > > > > > it will > > > > > + * be disabled it will not happen, so checking > > > > > pipe > > > > > instead > > > > > + */ > > > > > + if (crtc->pipe != (enum pipe)master) > > > > > + continue; > > > > > + > > > > > + if > > > > > (!drm_atomic_crtc_needs_modeset(&new_crtc_state- > > > > > > uapi) && > > > > > + new_crtc_state->uapi.enable) > > > > > + continue; > > > > > + > > > > > + pipe_config->mst_master_trans_pending = true; > > > > > + 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 +195,95 @@ 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 intel_connector *intel_conn = > > > > > to_intel_connector(connector); > > > > > + 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 drm_crtc_state *crtc_state; > > > > > + struct drm_connector *conn_iter; > > > > > + > > > > > + if (INTEL_GEN(dev_priv) < 12) > > > > > + return 0; > > > > > + > > > > > + 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 > > > > > + * - 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_conn->mst_port->mst_master_transcoder != > > > > > + intel_crtc_state->cpu_transcoder) > > > > > + return 0; > > > > > + > > > > > + /* Is master, mark all other CRTCs as needing a modeset > > > > > */ > > > > > + drm_connector_list_iter_begin(state->dev, > > > > > &conn_list_iter); > > > > > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) > > > > > { > > > > > + struct intel_connector *intel_conn_iter; > > > > > + struct drm_connector_state *conn_iter_state; > > > > > + > > > > > + 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); > > > > > + } > > > > > + > > > > > + intel_crtc_state = > > > > > to_intel_crtc_state(crtc_state); > > > > > + crtc_state->mode_changed = true; > > > > > + } > > > > > + drm_connector_list_iter_end(&conn_list_iter); > > > > > + > > > > > return 0; > > > > > } > > > > > > > > > > @@ -175,6 +305,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; > > > > > > > > > > @@ -259,6 +393,7 @@ static void > > > > > intel_mst_post_disable_dp(struct > > > > > intel_encoder *encoder, > > > > > intel_dp_sink_dpms(intel_dp, > > > > > DRM_MODE_DPMS_OFF); > > > > > intel_dig_port- > > > > > >base.post_disable(&intel_dig_port- > > > > > > base, > > > > > old_crtc_stat > > > > > e, > > > > > NULL); > > > > > + intel_dp->mst_master_transcoder = > > > > > INVALID_TRANSCODER; > > > > > } > > > > > > > > > > DRM_DEBUG_KMS("active links %d\n", intel_dp- > > > > > >active_mst_links); > > > > > @@ -314,8 +449,11 @@ static void intel_mst_pre_enable_dp(struct > > > > > intel_encoder *encoder, > > > > > > > > > > DRM_DEBUG_KMS("active links %d\n", intel_dp- > > > > > >active_mst_links); > > > > > > > > > > - if (first_mst_stream) > > > > > + if (first_mst_stream) { > > > > > + WARN_ON(intel_dp->mst_master_transcoder != > > > > > INVALID_TRANSCODER); > > > > > + intel_dp->mst_master_transcoder = pipe_config- > > > > > > cpu_transcoder; > > > > > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > > > > > + } > > > > > > > > > > drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, > > > > > connector- > > > > > > port, true); > > > > > > > > > > @@ -717,3 +855,12 @@ 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 */ > > > > > } > > > > > + > > > > > +enum transcoder > > > > > +intel_dp_mst_master_trans_get(struct intel_encoder *encoder) > > > > > +{ > > > > > + struct intel_dp_mst_encoder *intel_mst = > > > > > enc_to_mst(&encoder- > > > > > > base); > > > > > + struct intel_dp *intel_dp = &intel_mst->primary->dp; > > > > > + > > > > > + return intel_dp->mst_master_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..e6f28a517182 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h > > > > > @@ -7,9 +7,11 @@ > > > > > #define __INTEL_DP_MST_H__ > > > > > > > > > > struct intel_digital_port; > > > > > +struct intel_encoder; > > > > > > > > > > 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); > > > > > +enum transcoder intel_dp_mst_master_trans_get(struct > > > > > intel_encoder > > > > > *encoder); > > > > > > > > > > #endif /* __INTEL_DP_MST_H__ */ > > > > > -- > > > > > 2.24.0
On Thu, 2019-11-28 at 14:06 +0200, Ville Syrjälä wrote: > On Thu, Nov 28, 2019 at 01:14:37AM +0000, Souza, Jose wrote: > > On Wed, 2019-11-27 at 21:59 +0200, Ville Syrjälä wrote: > > > On Tue, Nov 26, 2019 at 08:30:31PM +0000, Souza, Jose wrote: > > > > On Tue, 2019-11-26 at 22:05 +0200, Ville Syrjälä wrote: > > > > > On Fri, Nov 22, 2019 at 04:54:55PM -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. > > > > > > > > > > > > A previous approach was using the lowest pipe/transcoder as > > > > > > master > > > > > > transcoder but as the comment in > > > > > > skl_commit_modeset_enables() > > > > > > states, > > > > > > that is not always true. > > > > > > > > > > > > So here promoting the first pipe/transcoder of the stream > > > > > > as > > > > > > master. > > > > > > That caused several other problems as during the commit > > > > > > phase > > > > > > the > > > > > > state computed should not be changed. > > > > > > > > > > > > So the master transcoder is store into intel_dp and the > > > > > > modeset > > > > > > in > > > > > > slave pipes/transcoders is forced using > > > > > > mst_master_trans_pending. > > > > > > > > > > > > v2: > > > > > > - added missing config compute to trigger fullmodeset in > > > > > > slave > > > > > > transcoders > > > > > > > > > > > > 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 | 10 +- > > > > > > drivers/gpu/drm/i915/display/intel_display.c | 58 > > > > > > ++++++- > > > > > > .../drm/i915/display/intel_display_types.h | 3 + > > > > > > drivers/gpu/drm/i915/display/intel_dp.c | 1 + > > > > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 149 > > > > > > +++++++++++++++++- > > > > > > drivers/gpu/drm/i915/display/intel_dp_mst.h | 2 + > > > > > > 6 files changed, 216 insertions(+), 7 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > index a976606d21c7..d2f0d393d3ee 100644 > > > > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > @@ -35,6 +35,7 @@ > > > > > > #include "intel_display_types.h" > > > > > > #include "intel_dp.h" > > > > > > #include "intel_dp_link_training.h" > > > > > > +#include "intel_dp_mst.h" > > > > > > #include "intel_dpio_phy.h" > > > > > > #include "intel_dsi.h" > > > > > > #include "intel_fifo_underrun.h" > > > > > > @@ -1903,8 +1904,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 = > > > > > > intel_dp_mst_master_trans_get(encoder); > > > > > > > > > > Why isn't that just stored in the crtc state like everything > > > > > else? > > > > > > > > > > I'm thinking we should maybe do it just like port sync and > > > > > have > > > > > both > > > > > master + slave_mask. That way it should be pretty trivial to > > > > > add > > > > > all > > > > > the relevant crtcs to the state when needed. > > > > > > > > I guess port sync is not doing the right thing and it could > > > > cause > > > > underruns. > > > > When it is going to enable the master CRTC of the port sync it > > > > forcibly > > > > enables the slave first, what could cause underruns because of > > > > overlap > > > > in ddb allocations(that is what I understood from the comment > > > > in > > > > skl_commit_modeset_enables()). > > > > > > Not necessarily just underruns but even a system hang. The fix > > > should > > > be > > > a trivial "check the slave for ddb overlap as well", but > > > apparently I > > > failed at convicing people to do that. > > > > > > I've actually been pondering about decoupling the plane updates > > > from > > > the crtc enable stuff entirely. At least theoretically crtc > > > enable > > > should be able to excute in any order as long we don't enable any > > > new planes. > > > > > > But none of that really matters for the discussion at hand. > > > Though > > > there are other problems with the port sync stuff that would need > > > to be handled better. Eg. I think it now adds both crtcs to the > > > state > > > always which is going to cut the fps in half. Also the place > > > where > > > it does that stuff is rather suspicious. All that stuff should be > > > somewhere a bit higher up IMO. > > > > > > > So for MST we only know who is the master in the commit phase > > > > and > > > > at > > > > this point we should not modify the computed state. > > > > > > I'm not suggesting modifying anything during commit phase. I > > > think > > > you are effectiely doing that right now by stuffing that mst > > > master > > > transcoder into intel_dp. > > > > Sorry, I still don't get what approach are you suggesting here. > > > > If we can't modify the state in the commit phase, adding > > mst_master_transcoder in the CRTC state will not be possible while > > respecting the order imposed by ddb allocations. > > The ddb allocation ordering only comes into play when there are > already active pipes. It should always be possible to not enable > the slaves until the master has been shuffled into place in the > ddb and enabled. This sounds contradictory to what you answered here: https://lists.freedesktop.org/archives/intel-gfx/2019-November/221608.html Will need to some testing to get the steps but I was able consistent to get to state were doing a full modeset in pipe A(mst master) caused the pipe B(mst slave) to enabled first because of the ddb allocations. So can I or not do something like port sync does? And force the enable of master before the slaves? If possible, the comment in skl_commit_modeset_enables() will need some changes. > > > > > > > + 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); > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > > > > index 801b975c7d39..35a59108194e 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" > > > > > > @@ -5365,6 +5366,36 @@ intel_get_crtc_new_encoder(const > > > > > > struct > > > > > > intel_atomic_state *state, > > > > > > return encoder; > > > > > > } > > > > > > > > > > > > +/* > > > > > > + * Finds the encoder associated with the given CRTC. This > > > > > > can > > > > > > only > > > > > > be > > > > > > + * used when we know that the CRTC isn't feeding multiple > > > > > > encoders! > > > > > > + */ > > > > > > +static struct intel_encoder * > > > > > > +intel_get_crtc_old_encoder(const struct intel_atomic_state > > > > > > *state, > > > > > > + const struct intel_crtc_state > > > > > > *crtc_state) > > > > > > +{ > > > > > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state- > > > > > > > uapi.crtc); > > > > > > + const struct drm_connector_state *connector_state; > > > > > > + const struct drm_connector *connector; > > > > > > + struct intel_encoder *encoder = NULL; > > > > > > + int num_encoders = 0; > > > > > > + int i; > > > > > > + > > > > > > + for_each_old_connector_in_state(&state->base, > > > > > > connector, > > > > > > + connector_state, i) { > > > > > > + if (connector_state->crtc != &crtc->base) > > > > > > + continue; > > > > > > + > > > > > > + encoder = to_intel_encoder(connector_state- > > > > > > > best_encoder); > > > > > > + num_encoders++; > > > > > > + } > > > > > > + > > > > > > + WARN(num_encoders != 1, "%d encoders for pipe %c\n", > > > > > > + num_encoders, pipe_name(crtc->pipe)); > > > > > > + > > > > > > + return encoder; > > > > > > +} > > > > > > > > > > Argh. I was hoping to kill the other one of these. Got it > > > > > down to > > > > > 1 > > > > > remaining user now I think. > > > > > > > > > > > + > > > > > > /* > > > > > > * Enable PCH resources required for PCH ports: > > > > > > * - PCH PLLs > > > > > > @@ -13365,6 +13396,12 @@ intel_pipe_config_compare(const > > > > > > struct > > > > > > intel_crtc_state *current_config, > > > > > > #undef PIPE_CONF_CHECK_COLOR_LUT > > > > > > #undef PIPE_CONF_QUIRK > > > > > > > > > > > > + if (fastset && pipe_config->mst_master_trans_pending) { > > > > > > + DRM_DEBUG_KMS("[CRTC:%d:%s] fastset mismatch in > > > > > > mst_master_trans\n", > > > > > > + crtc->base.base.id, crtc- > > > > > > > base.name); > > > > > > + ret = false; > > > > > > + } > > > > > > + > > > > > > return ret; > > > > > > } > > > > > > > > > > > > @@ -14449,22 +14486,35 @@ static void > > > > > > intel_commit_modeset_disables(struct intel_atomic_state > > > > > > *state) > > > > > > struct intel_crtc *crtc; > > > > > > 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) || !crtc- > > > > > > > active) > > > > > > continue; > > > > > > > > > > > > + if (!is_trans_port_sync_mode(new_crtc_state) && > > > > > > + !intel_crtc_has_type(old_crtc_state, > > > > > > INTEL_OUTPUT_DP_MST)) > > > > > > + continue; > > > > > > + > > > > > > /* In case of Transcoder port Sync master slave > > > > > > CRTCs > > > > > > can be > > > > > > * assigned in any order and we need to make > > > > > > sure that > > > > > > * slave CRTCs are disabled first and then > > > > > > master CRTC > > > > > > since > > > > > > * Slave vblanks are masked till Master > > > > > > Vblanks. > > > > > > */ > > > > > > - if (!is_trans_port_sync_mode(old_crtc_state)) > > > > > > - continue; > > > > > > - if (is_trans_port_sync_master(old_crtc_state)) > > > > > > + if (is_trans_port_sync_mode(new_crtc_state) && > > > > > > + is_trans_port_sync_master(new_crtc_state)) > > > > > > continue; > > > > > > > > > > > > + if (intel_crtc_has_type(old_crtc_state, > > > > > > INTEL_OUTPUT_DP_MST)) { > > > > > > + struct intel_encoder *encoder; > > > > > > + > > > > > > + encoder = > > > > > > intel_get_crtc_old_encoder(state, > > > > > > + ol > > > > > > d_crtc_s > > > > > > tate); > > > > > > + if > > > > > > (intel_dp_mst_master_trans_get(encoder) == > > > > > > + old_crtc_state->cpu_transcoder) > > > > > > + continue; > > > > > > + } > > > > > > + > > > > > > intel_pre_plane_update(old_crtc_state, > > > > > > new_crtc_state); > > > > > > intel_old_crtc_state_disables(state, > > > > > > old_crtc_state, > > > > > > new_crtc_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..23d747cdca64 100644 > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > @@ -1052,6 +1052,8 @@ struct intel_crtc_state { > > > > > > /* Pointer to master transcoder in case of tiled > > > > > > displays */ > > > > > > enum transcoder master_transcoder; > > > > > > > > > > > > + bool mst_master_trans_pending; > > > > > > + > > > > > > /* Bitmask to indicate slaves attached */ > > > > > > u8 sync_mode_slaves_mask; > > > > > > }; > > > > > > @@ -1284,6 +1286,7 @@ struct intel_dp { > > > > > > bool can_mst; /* this port supports mst */ > > > > > > bool is_mst; > > > > > > int active_mst_links; > > > > > > + enum transcoder mst_master_transcoder; /* Only used in > > > > > > TGL+ */ > > > > > > > > > > > > /* > > > > > > * DP_TP_* registers may be either on port or > > > > > > transcoder > > > > > > register space. > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > > > > > b/drivers/gpu/drm/i915/display/intel_dp.c > > > > > > index 3123958e2081..ceff6901451a 100644 > > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > > > > @@ -7424,6 +7424,7 @@ intel_dp_init_connector(struct > > > > > > intel_digital_port *intel_dig_port, > > > > > > intel_dp->reset_link_params = true; > > > > > > intel_dp->pps_pipe = INVALID_PIPE; > > > > > > intel_dp->active_pipe = INVALID_PIPE; > > > > > > + intel_dp->mst_master_transcoder = INVALID_TRANSCODER; > > > > > > > > > > > > /* Preserve the current hw state. */ > > > > > > intel_dp->DP = I915_READ(intel_dp->output_reg); > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > > > index f8a350359346..9731c3c1d3f2 100644 > > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > > > @@ -87,6 +87,47 @@ static int > > > > > > intel_dp_mst_compute_link_config(struct intel_encoder > > > > > > *encoder, > > > > > > return 0; > > > > > > } > > > > > > > > > > > > +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 drm_i915_private *dev_priv = to_i915(encoder- > > > > > > > base.dev); > > > > > > + struct intel_crtc_state *new_crtc_state; > > > > > > + struct intel_crtc *crtc; > > > > > > + enum transcoder master; > > > > > > + int i; > > > > > > + > > > > > > + if (INTEL_GEN(dev_priv) < 12) > > > > > > + return 0; > > > > > > + > > > > > > + if (!conn_state->crtc) > > > > > > + return 0; > > > > > > + > > > > > > + master = intel_dp_mst_master_trans_get(encoder); > > > > > > + if (master == INVALID_TRANSCODER) > > > > > > + return 0; > > > > > > + > > > > > > + for_each_new_intel_crtc_in_state(state, crtc, > > > > > > new_crtc_state, > > > > > > i) { > > > > > > + /* > > > > > > + * cpu_transcoder is set when computing CRTC > > > > > > state if > > > > > > it will > > > > > > + * be disabled it will not happen, so checking > > > > > > pipe > > > > > > instead > > > > > > + */ > > > > > > + if (crtc->pipe != (enum pipe)master) > > > > > > + continue; > > > > > > + > > > > > > + if > > > > > > (!drm_atomic_crtc_needs_modeset(&new_crtc_state- > > > > > > > uapi) && > > > > > > + new_crtc_state->uapi.enable) > > > > > > + continue; > > > > > > + > > > > > > + pipe_config->mst_master_trans_pending = true; > > > > > > + 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 +195,95 @@ 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 intel_connector *intel_conn = > > > > > > to_intel_connector(connector); > > > > > > + 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 drm_crtc_state *crtc_state; > > > > > > + struct drm_connector *conn_iter; > > > > > > + > > > > > > + if (INTEL_GEN(dev_priv) < 12) > > > > > > + return 0; > > > > > > + > > > > > > + 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 > > > > > > + * - 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_conn->mst_port->mst_master_transcoder != > > > > > > + intel_crtc_state->cpu_transcoder) > > > > > > + return 0; > > > > > > + > > > > > > + /* Is master, mark all other CRTCs as needing a modeset > > > > > > */ > > > > > > + drm_connector_list_iter_begin(state->dev, > > > > > > &conn_list_iter); > > > > > > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) > > > > > > { > > > > > > + struct intel_connector *intel_conn_iter; > > > > > > + struct drm_connector_state *conn_iter_state; > > > > > > + > > > > > > + 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); > > > > > > + } > > > > > > + > > > > > > + intel_crtc_state = > > > > > > to_intel_crtc_state(crtc_state); > > > > > > + crtc_state->mode_changed = true; > > > > > > + } > > > > > > + drm_connector_list_iter_end(&conn_list_iter); > > > > > > + > > > > > > return 0; > > > > > > } > > > > > > > > > > > > @@ -175,6 +305,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; > > > > > > > > > > > > @@ -259,6 +393,7 @@ static void > > > > > > intel_mst_post_disable_dp(struct > > > > > > intel_encoder *encoder, > > > > > > intel_dp_sink_dpms(intel_dp, > > > > > > DRM_MODE_DPMS_OFF); > > > > > > intel_dig_port- > > > > > > > base.post_disable(&intel_dig_port- > > > > > > > base, > > > > > > old_crtc_stat > > > > > > e, > > > > > > NULL); > > > > > > + intel_dp->mst_master_transcoder = > > > > > > INVALID_TRANSCODER; > > > > > > } > > > > > > > > > > > > DRM_DEBUG_KMS("active links %d\n", intel_dp- > > > > > > > active_mst_links); > > > > > > @@ -314,8 +449,11 @@ static void > > > > > > intel_mst_pre_enable_dp(struct > > > > > > intel_encoder *encoder, > > > > > > > > > > > > DRM_DEBUG_KMS("active links %d\n", intel_dp- > > > > > > > active_mst_links); > > > > > > > > > > > > - if (first_mst_stream) > > > > > > + if (first_mst_stream) { > > > > > > + WARN_ON(intel_dp->mst_master_transcoder != > > > > > > INVALID_TRANSCODER); > > > > > > + intel_dp->mst_master_transcoder = pipe_config- > > > > > > > cpu_transcoder; > > > > > > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > > > > > > + } > > > > > > > > > > > > drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, > > > > > > connector- > > > > > > > port, true); > > > > > > > > > > > > @@ -717,3 +855,12 @@ 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 */ > > > > > > } > > > > > > + > > > > > > +enum transcoder > > > > > > +intel_dp_mst_master_trans_get(struct intel_encoder > > > > > > *encoder) > > > > > > +{ > > > > > > + struct intel_dp_mst_encoder *intel_mst = > > > > > > enc_to_mst(&encoder- > > > > > > > base); > > > > > > + struct intel_dp *intel_dp = &intel_mst->primary->dp; > > > > > > + > > > > > > + return intel_dp->mst_master_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..e6f28a517182 100644 > > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h > > > > > > @@ -7,9 +7,11 @@ > > > > > > #define __INTEL_DP_MST_H__ > > > > > > > > > > > > struct intel_digital_port; > > > > > > +struct intel_encoder; > > > > > > > > > > > > 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); > > > > > > +enum transcoder intel_dp_mst_master_trans_get(struct > > > > > > intel_encoder > > > > > > *encoder); > > > > > > > > > > > > #endif /* __INTEL_DP_MST_H__ */ > > > > > > -- > > > > > > 2.24.0
On Mon, Dec 02, 2019 at 10:03:38PM +0000, Souza, Jose wrote: > On Thu, 2019-11-28 at 14:06 +0200, Ville Syrjälä wrote: > > On Thu, Nov 28, 2019 at 01:14:37AM +0000, Souza, Jose wrote: > > > On Wed, 2019-11-27 at 21:59 +0200, Ville Syrjälä wrote: > > > > On Tue, Nov 26, 2019 at 08:30:31PM +0000, Souza, Jose wrote: > > > > > On Tue, 2019-11-26 at 22:05 +0200, Ville Syrjälä wrote: > > > > > > On Fri, Nov 22, 2019 at 04:54:55PM -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. > > > > > > > > > > > > > > A previous approach was using the lowest pipe/transcoder as > > > > > > > master > > > > > > > transcoder but as the comment in > > > > > > > skl_commit_modeset_enables() > > > > > > > states, > > > > > > > that is not always true. > > > > > > > > > > > > > > So here promoting the first pipe/transcoder of the stream > > > > > > > as > > > > > > > master. > > > > > > > That caused several other problems as during the commit > > > > > > > phase > > > > > > > the > > > > > > > state computed should not be changed. > > > > > > > > > > > > > > So the master transcoder is store into intel_dp and the > > > > > > > modeset > > > > > > > in > > > > > > > slave pipes/transcoders is forced using > > > > > > > mst_master_trans_pending. > > > > > > > > > > > > > > v2: > > > > > > > - added missing config compute to trigger fullmodeset in > > > > > > > slave > > > > > > > transcoders > > > > > > > > > > > > > > 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 | 10 +- > > > > > > > drivers/gpu/drm/i915/display/intel_display.c | 58 > > > > > > > ++++++- > > > > > > > .../drm/i915/display/intel_display_types.h | 3 + > > > > > > > drivers/gpu/drm/i915/display/intel_dp.c | 1 + > > > > > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 149 > > > > > > > +++++++++++++++++- > > > > > > > drivers/gpu/drm/i915/display/intel_dp_mst.h | 2 + > > > > > > > 6 files changed, 216 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > > index a976606d21c7..d2f0d393d3ee 100644 > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > > @@ -35,6 +35,7 @@ > > > > > > > #include "intel_display_types.h" > > > > > > > #include "intel_dp.h" > > > > > > > #include "intel_dp_link_training.h" > > > > > > > +#include "intel_dp_mst.h" > > > > > > > #include "intel_dpio_phy.h" > > > > > > > #include "intel_dsi.h" > > > > > > > #include "intel_fifo_underrun.h" > > > > > > > @@ -1903,8 +1904,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 = > > > > > > > intel_dp_mst_master_trans_get(encoder); > > > > > > > > > > > > Why isn't that just stored in the crtc state like everything > > > > > > else? > > > > > > > > > > > > I'm thinking we should maybe do it just like port sync and > > > > > > have > > > > > > both > > > > > > master + slave_mask. That way it should be pretty trivial to > > > > > > add > > > > > > all > > > > > > the relevant crtcs to the state when needed. > > > > > > > > > > I guess port sync is not doing the right thing and it could > > > > > cause > > > > > underruns. > > > > > When it is going to enable the master CRTC of the port sync it > > > > > forcibly > > > > > enables the slave first, what could cause underruns because of > > > > > overlap > > > > > in ddb allocations(that is what I understood from the comment > > > > > in > > > > > skl_commit_modeset_enables()). > > > > > > > > Not necessarily just underruns but even a system hang. The fix > > > > should > > > > be > > > > a trivial "check the slave for ddb overlap as well", but > > > > apparently I > > > > failed at convicing people to do that. > > > > > > > > I've actually been pondering about decoupling the plane updates > > > > from > > > > the crtc enable stuff entirely. At least theoretically crtc > > > > enable > > > > should be able to excute in any order as long we don't enable any > > > > new planes. > > > > > > > > But none of that really matters for the discussion at hand. > > > > Though > > > > there are other problems with the port sync stuff that would need > > > > to be handled better. Eg. I think it now adds both crtcs to the > > > > state > > > > always which is going to cut the fps in half. Also the place > > > > where > > > > it does that stuff is rather suspicious. All that stuff should be > > > > somewhere a bit higher up IMO. > > > > > > > > > So for MST we only know who is the master in the commit phase > > > > > and > > > > > at > > > > > this point we should not modify the computed state. > > > > > > > > I'm not suggesting modifying anything during commit phase. I > > > > think > > > > you are effectiely doing that right now by stuffing that mst > > > > master > > > > transcoder into intel_dp. > > > > > > Sorry, I still don't get what approach are you suggesting here. > > > > > > If we can't modify the state in the commit phase, adding > > > mst_master_transcoder in the CRTC state will not be possible while > > > respecting the order imposed by ddb allocations. > > > > The ddb allocation ordering only comes into play when there are > > already active pipes. It should always be possible to not enable > > the slaves until the master has been shuffled into place in the > > ddb and enabled. > > This sounds contradictory to what you answered here: > https://lists.freedesktop.org/archives/intel-gfx/2019-November/221608.html > > Will need to some testing to get the steps but I was able consistent to > get to state were doing a full modeset in pipe A(mst master) caused the > pipe B(mst slave) to enabled first because of the ddb allocations. > > So can I or not do something like port sync does? And force the enable > of master before the slaves? If possible, the comment in > skl_commit_modeset_enables() will need some changes. I suspect for the mst stuff we should do: while_dirty_mst_masters() { if (!ddb_overlap) enable(); } while_dirty_mst_slaves() { if (!ddb_overlap) enable(); }
On Tue, 2019-12-03 at 14:47 +0200, Ville Syrjälä wrote: > On Mon, Dec 02, 2019 at 10:03:38PM +0000, Souza, Jose wrote: > > On Thu, 2019-11-28 at 14:06 +0200, Ville Syrjälä wrote: > > > On Thu, Nov 28, 2019 at 01:14:37AM +0000, Souza, Jose wrote: > > > > On Wed, 2019-11-27 at 21:59 +0200, Ville Syrjälä wrote: > > > > > On Tue, Nov 26, 2019 at 08:30:31PM +0000, Souza, Jose wrote: > > > > > > On Tue, 2019-11-26 at 22:05 +0200, Ville Syrjälä wrote: > > > > > > > On Fri, Nov 22, 2019 at 04:54:55PM -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. > > > > > > > > > > > > > > > > A previous approach was using the lowest > > > > > > > > pipe/transcoder as > > > > > > > > master > > > > > > > > transcoder but as the comment in > > > > > > > > skl_commit_modeset_enables() > > > > > > > > states, > > > > > > > > that is not always true. > > > > > > > > > > > > > > > > So here promoting the first pipe/transcoder of the > > > > > > > > stream > > > > > > > > as > > > > > > > > master. > > > > > > > > That caused several other problems as during the commit > > > > > > > > phase > > > > > > > > the > > > > > > > > state computed should not be changed. > > > > > > > > > > > > > > > > So the master transcoder is store into intel_dp and the > > > > > > > > modeset > > > > > > > > in > > > > > > > > slave pipes/transcoders is forced using > > > > > > > > mst_master_trans_pending. > > > > > > > > > > > > > > > > v2: > > > > > > > > - added missing config compute to trigger fullmodeset > > > > > > > > in > > > > > > > > slave > > > > > > > > transcoders > > > > > > > > > > > > > > > > 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 | 10 +- > > > > > > > > drivers/gpu/drm/i915/display/intel_display.c | 58 > > > > > > > > ++++++- > > > > > > > > .../drm/i915/display/intel_display_types.h | 3 + > > > > > > > > drivers/gpu/drm/i915/display/intel_dp.c | 1 + > > > > > > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 149 > > > > > > > > +++++++++++++++++- > > > > > > > > drivers/gpu/drm/i915/display/intel_dp_mst.h | 2 + > > > > > > > > 6 files changed, 216 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > > > index a976606d21c7..d2f0d393d3ee 100644 > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > > > @@ -35,6 +35,7 @@ > > > > > > > > #include "intel_display_types.h" > > > > > > > > #include "intel_dp.h" > > > > > > > > #include "intel_dp_link_training.h" > > > > > > > > +#include "intel_dp_mst.h" > > > > > > > > #include "intel_dpio_phy.h" > > > > > > > > #include "intel_dsi.h" > > > > > > > > #include "intel_fifo_underrun.h" > > > > > > > > @@ -1903,8 +1904,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 = > > > > > > > > intel_dp_mst_master_trans_get(encoder); > > > > > > > > > > > > > > Why isn't that just stored in the crtc state like > > > > > > > everything > > > > > > > else? > > > > > > > > > > > > > > I'm thinking we should maybe do it just like port sync > > > > > > > and > > > > > > > have > > > > > > > both > > > > > > > master + slave_mask. That way it should be pretty trivial > > > > > > > to > > > > > > > add > > > > > > > all > > > > > > > the relevant crtcs to the state when needed. > > > > > > > > > > > > I guess port sync is not doing the right thing and it could > > > > > > cause > > > > > > underruns. > > > > > > When it is going to enable the master CRTC of the port sync > > > > > > it > > > > > > forcibly > > > > > > enables the slave first, what could cause underruns because > > > > > > of > > > > > > overlap > > > > > > in ddb allocations(that is what I understood from the > > > > > > comment > > > > > > in > > > > > > skl_commit_modeset_enables()). > > > > > > > > > > Not necessarily just underruns but even a system hang. The > > > > > fix > > > > > should > > > > > be > > > > > a trivial "check the slave for ddb overlap as well", but > > > > > apparently I > > > > > failed at convicing people to do that. > > > > > > > > > > I've actually been pondering about decoupling the plane > > > > > updates > > > > > from > > > > > the crtc enable stuff entirely. At least theoretically crtc > > > > > enable > > > > > should be able to excute in any order as long we don't enable > > > > > any > > > > > new planes. > > > > > > > > > > But none of that really matters for the discussion at hand. > > > > > Though > > > > > there are other problems with the port sync stuff that would > > > > > need > > > > > to be handled better. Eg. I think it now adds both crtcs to > > > > > the > > > > > state > > > > > always which is going to cut the fps in half. Also the place > > > > > where > > > > > it does that stuff is rather suspicious. All that stuff > > > > > should be > > > > > somewhere a bit higher up IMO. > > > > > > > > > > > So for MST we only know who is the master in the commit > > > > > > phase > > > > > > and > > > > > > at > > > > > > this point we should not modify the computed state. > > > > > > > > > > I'm not suggesting modifying anything during commit phase. I > > > > > think > > > > > you are effectiely doing that right now by stuffing that mst > > > > > master > > > > > transcoder into intel_dp. > > > > > > > > Sorry, I still don't get what approach are you suggesting here. > > > > > > > > If we can't modify the state in the commit phase, adding > > > > mst_master_transcoder in the CRTC state will not be possible > > > > while > > > > respecting the order imposed by ddb allocations. > > > > > > The ddb allocation ordering only comes into play when there are > > > already active pipes. It should always be possible to not enable > > > the slaves until the master has been shuffled into place in the > > > ddb and enabled. > > > > This sounds contradictory to what you answered here: > > https://lists.freedesktop.org/archives/intel-gfx/2019-November/221608.html > > > > Will need to some testing to get the steps but I was able > > consistent to > > get to state were doing a full modeset in pipe A(mst master) caused > > the > > pipe B(mst slave) to enabled first because of the ddb allocations. > > > > So can I or not do something like port sync does? And force the > > enable > > of master before the slaves? If possible, the comment in > > skl_commit_modeset_enables() will need some changes. > > I suspect for the mst stuff we should do: > > while_dirty_mst_masters() { > if (!ddb_overlap) > enable(); > } > while_dirty_mst_slaves() { > if (!ddb_overlap) > enable(); > } What about this case? Pipe/transcoder A and B in the same MST stream # old state - DDB allocation: AABBB mst master = transcoder A(computed in atomic check phase) entries[0].start = 0 entries[0].end = 1 entries[1].start = 2 entries[1].end = 4 # new state - DDB allocation: AAABBB mst master = transcoder A(computed in atomic check phase) entries[0].start = 0 entries[0].end = 2 entries[1].start = 3 entries[1].end = 5 while_dirty_mst_masters() first iteration: pipe A will overlap with old pipe B DDB allocation second iteration: pipe B is slave of A third iteration: ? >
On Tue, Dec 03, 2019 at 10:12:47PM +0000, Souza, Jose wrote: > On Tue, 2019-12-03 at 14:47 +0200, Ville Syrjälä wrote: > > On Mon, Dec 02, 2019 at 10:03:38PM +0000, Souza, Jose wrote: > > > On Thu, 2019-11-28 at 14:06 +0200, Ville Syrjälä wrote: > > > > On Thu, Nov 28, 2019 at 01:14:37AM +0000, Souza, Jose wrote: > > > > > On Wed, 2019-11-27 at 21:59 +0200, Ville Syrjälä wrote: > > > > > > On Tue, Nov 26, 2019 at 08:30:31PM +0000, Souza, Jose wrote: > > > > > > > On Tue, 2019-11-26 at 22:05 +0200, Ville Syrjälä wrote: > > > > > > > > On Fri, Nov 22, 2019 at 04:54:55PM -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. > > > > > > > > > > > > > > > > > > A previous approach was using the lowest > > > > > > > > > pipe/transcoder as > > > > > > > > > master > > > > > > > > > transcoder but as the comment in > > > > > > > > > skl_commit_modeset_enables() > > > > > > > > > states, > > > > > > > > > that is not always true. > > > > > > > > > > > > > > > > > > So here promoting the first pipe/transcoder of the > > > > > > > > > stream > > > > > > > > > as > > > > > > > > > master. > > > > > > > > > That caused several other problems as during the commit > > > > > > > > > phase > > > > > > > > > the > > > > > > > > > state computed should not be changed. > > > > > > > > > > > > > > > > > > So the master transcoder is store into intel_dp and the > > > > > > > > > modeset > > > > > > > > > in > > > > > > > > > slave pipes/transcoders is forced using > > > > > > > > > mst_master_trans_pending. > > > > > > > > > > > > > > > > > > v2: > > > > > > > > > - added missing config compute to trigger fullmodeset > > > > > > > > > in > > > > > > > > > slave > > > > > > > > > transcoders > > > > > > > > > > > > > > > > > > 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 | 10 +- > > > > > > > > > drivers/gpu/drm/i915/display/intel_display.c | 58 > > > > > > > > > ++++++- > > > > > > > > > .../drm/i915/display/intel_display_types.h | 3 + > > > > > > > > > drivers/gpu/drm/i915/display/intel_dp.c | 1 + > > > > > > > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 149 > > > > > > > > > +++++++++++++++++- > > > > > > > > > drivers/gpu/drm/i915/display/intel_dp_mst.h | 2 + > > > > > > > > > 6 files changed, 216 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > > > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > > > > index a976606d21c7..d2f0d393d3ee 100644 > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > > > > @@ -35,6 +35,7 @@ > > > > > > > > > #include "intel_display_types.h" > > > > > > > > > #include "intel_dp.h" > > > > > > > > > #include "intel_dp_link_training.h" > > > > > > > > > +#include "intel_dp_mst.h" > > > > > > > > > #include "intel_dpio_phy.h" > > > > > > > > > #include "intel_dsi.h" > > > > > > > > > #include "intel_fifo_underrun.h" > > > > > > > > > @@ -1903,8 +1904,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 = > > > > > > > > > intel_dp_mst_master_trans_get(encoder); > > > > > > > > > > > > > > > > Why isn't that just stored in the crtc state like > > > > > > > > everything > > > > > > > > else? > > > > > > > > > > > > > > > > I'm thinking we should maybe do it just like port sync > > > > > > > > and > > > > > > > > have > > > > > > > > both > > > > > > > > master + slave_mask. That way it should be pretty trivial > > > > > > > > to > > > > > > > > add > > > > > > > > all > > > > > > > > the relevant crtcs to the state when needed. > > > > > > > > > > > > > > I guess port sync is not doing the right thing and it could > > > > > > > cause > > > > > > > underruns. > > > > > > > When it is going to enable the master CRTC of the port sync > > > > > > > it > > > > > > > forcibly > > > > > > > enables the slave first, what could cause underruns because > > > > > > > of > > > > > > > overlap > > > > > > > in ddb allocations(that is what I understood from the > > > > > > > comment > > > > > > > in > > > > > > > skl_commit_modeset_enables()). > > > > > > > > > > > > Not necessarily just underruns but even a system hang. The > > > > > > fix > > > > > > should > > > > > > be > > > > > > a trivial "check the slave for ddb overlap as well", but > > > > > > apparently I > > > > > > failed at convicing people to do that. > > > > > > > > > > > > I've actually been pondering about decoupling the plane > > > > > > updates > > > > > > from > > > > > > the crtc enable stuff entirely. At least theoretically crtc > > > > > > enable > > > > > > should be able to excute in any order as long we don't enable > > > > > > any > > > > > > new planes. > > > > > > > > > > > > But none of that really matters for the discussion at hand. > > > > > > Though > > > > > > there are other problems with the port sync stuff that would > > > > > > need > > > > > > to be handled better. Eg. I think it now adds both crtcs to > > > > > > the > > > > > > state > > > > > > always which is going to cut the fps in half. Also the place > > > > > > where > > > > > > it does that stuff is rather suspicious. All that stuff > > > > > > should be > > > > > > somewhere a bit higher up IMO. > > > > > > > > > > > > > So for MST we only know who is the master in the commit > > > > > > > phase > > > > > > > and > > > > > > > at > > > > > > > this point we should not modify the computed state. > > > > > > > > > > > > I'm not suggesting modifying anything during commit phase. I > > > > > > think > > > > > > you are effectiely doing that right now by stuffing that mst > > > > > > master > > > > > > transcoder into intel_dp. > > > > > > > > > > Sorry, I still don't get what approach are you suggesting here. > > > > > > > > > > If we can't modify the state in the commit phase, adding > > > > > mst_master_transcoder in the CRTC state will not be possible > > > > > while > > > > > respecting the order imposed by ddb allocations. > > > > > > > > The ddb allocation ordering only comes into play when there are > > > > already active pipes. It should always be possible to not enable > > > > the slaves until the master has been shuffled into place in the > > > > ddb and enabled. > > > > > > This sounds contradictory to what you answered here: > > > https://lists.freedesktop.org/archives/intel-gfx/2019-November/221608.html > > > > > > Will need to some testing to get the steps but I was able > > > consistent to > > > get to state were doing a full modeset in pipe A(mst master) caused > > > the > > > pipe B(mst slave) to enabled first because of the ddb allocations. > > > > > > So can I or not do something like port sync does? And force the > > > enable > > > of master before the slaves? If possible, the comment in > > > skl_commit_modeset_enables() will need some changes. > > > > I suspect for the mst stuff we should do: > > > > while_dirty_mst_masters() { > > if (!ddb_overlap) > > enable(); > > } > > while_dirty_mst_slaves() { > > if (!ddb_overlap) > > enable(); > > } > > What about this case? > > Pipe/transcoder A and B in the same MST stream > > # old state - DDB allocation: AABBB > mst master = transcoder A(computed in atomic check phase) > entries[0].start = 0 > entries[0].end = 1 > entries[1].start = 2 > entries[1].end = 4 > > # new state - DDB allocation: AAABBB > mst master = transcoder A(computed in atomic check phase) > entries[0].start = 0 > entries[0].end = 2 > entries[1].start = 3 > entries[1].end = 5 > > while_dirty_mst_masters() > first iteration: pipe A will overlap with old pipe B DDB There won't be an old DDB allocation for a pipe if it's going trough a modeset. > allocation > second iteration: pipe B is slave of A > third iteration: ? > > > >
On Wed, 2019-12-04 at 12:55 +0200, Ville Syrjälä wrote: > On Tue, Dec 03, 2019 at 10:12:47PM +0000, Souza, Jose wrote: > > On Tue, 2019-12-03 at 14:47 +0200, Ville Syrjälä wrote: > > > On Mon, Dec 02, 2019 at 10:03:38PM +0000, Souza, Jose wrote: > > > > On Thu, 2019-11-28 at 14:06 +0200, Ville Syrjälä wrote: > > > > > On Thu, Nov 28, 2019 at 01:14:37AM +0000, Souza, Jose wrote: > > > > > > On Wed, 2019-11-27 at 21:59 +0200, Ville Syrjälä wrote: > > > > > > > On Tue, Nov 26, 2019 at 08:30:31PM +0000, Souza, Jose > > > > > > > wrote: > > > > > > > > On Tue, 2019-11-26 at 22:05 +0200, Ville Syrjälä wrote: > > > > > > > > > On Fri, Nov 22, 2019 at 04:54:55PM -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. > > > > > > > > > > > > > > > > > > > > A previous approach was using the lowest > > > > > > > > > > pipe/transcoder as > > > > > > > > > > master > > > > > > > > > > transcoder but as the comment in > > > > > > > > > > skl_commit_modeset_enables() > > > > > > > > > > states, > > > > > > > > > > that is not always true. > > > > > > > > > > > > > > > > > > > > So here promoting the first pipe/transcoder of the > > > > > > > > > > stream > > > > > > > > > > as > > > > > > > > > > master. > > > > > > > > > > That caused several other problems as during the > > > > > > > > > > commit > > > > > > > > > > phase > > > > > > > > > > the > > > > > > > > > > state computed should not be changed. > > > > > > > > > > > > > > > > > > > > So the master transcoder is store into intel_dp and > > > > > > > > > > the > > > > > > > > > > modeset > > > > > > > > > > in > > > > > > > > > > slave pipes/transcoders is forced using > > > > > > > > > > mst_master_trans_pending. > > > > > > > > > > > > > > > > > > > > v2: > > > > > > > > > > - added missing config compute to trigger > > > > > > > > > > fullmodeset > > > > > > > > > > in > > > > > > > > > > slave > > > > > > > > > > transcoders > > > > > > > > > > > > > > > > > > > > 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 | 1 > > > > > > > > > > 0 +- > > > > > > > > > > drivers/gpu/drm/i915/display/intel_display.c | 5 > > > > > > > > > > 8 > > > > > > > > > > ++++++- > > > > > > > > > > .../drm/i915/display/intel_display_types.h | > > > > > > > > > > 3 + > > > > > > > > > > drivers/gpu/drm/i915/display/intel_dp.c | > > > > > > > > > > 1 + > > > > > > > > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | > > > > > > > > > > 149 > > > > > > > > > > +++++++++++++++++- > > > > > > > > > > drivers/gpu/drm/i915/display/intel_dp_mst.h | > > > > > > > > > > 2 + > > > > > > > > > > 6 files changed, 216 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > > > > > > > diff --git > > > > > > > > > > a/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > > > > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > > > > > index a976606d21c7..d2f0d393d3ee 100644 > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > > > > > @@ -35,6 +35,7 @@ > > > > > > > > > > #include "intel_display_types.h" > > > > > > > > > > #include "intel_dp.h" > > > > > > > > > > #include "intel_dp_link_training.h" > > > > > > > > > > +#include "intel_dp_mst.h" > > > > > > > > > > #include "intel_dpio_phy.h" > > > > > > > > > > #include "intel_dsi.h" > > > > > > > > > > #include "intel_fifo_underrun.h" > > > > > > > > > > @@ -1903,8 +1904,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 = > > > > > > > > > > intel_dp_mst_master_trans_get(encoder); > > > > > > > > > > > > > > > > > > Why isn't that just stored in the crtc state like > > > > > > > > > everything > > > > > > > > > else? > > > > > > > > > > > > > > > > > > I'm thinking we should maybe do it just like port > > > > > > > > > sync > > > > > > > > > and > > > > > > > > > have > > > > > > > > > both > > > > > > > > > master + slave_mask. That way it should be pretty > > > > > > > > > trivial > > > > > > > > > to > > > > > > > > > add > > > > > > > > > all > > > > > > > > > the relevant crtcs to the state when needed. > > > > > > > > > > > > > > > > I guess port sync is not doing the right thing and it > > > > > > > > could > > > > > > > > cause > > > > > > > > underruns. > > > > > > > > When it is going to enable the master CRTC of the port > > > > > > > > sync > > > > > > > > it > > > > > > > > forcibly > > > > > > > > enables the slave first, what could cause underruns > > > > > > > > because > > > > > > > > of > > > > > > > > overlap > > > > > > > > in ddb allocations(that is what I understood from the > > > > > > > > comment > > > > > > > > in > > > > > > > > skl_commit_modeset_enables()). > > > > > > > > > > > > > > Not necessarily just underruns but even a system hang. > > > > > > > The > > > > > > > fix > > > > > > > should > > > > > > > be > > > > > > > a trivial "check the slave for ddb overlap as well", but > > > > > > > apparently I > > > > > > > failed at convicing people to do that. > > > > > > > > > > > > > > I've actually been pondering about decoupling the plane > > > > > > > updates > > > > > > > from > > > > > > > the crtc enable stuff entirely. At least theoretically > > > > > > > crtc > > > > > > > enable > > > > > > > should be able to excute in any order as long we don't > > > > > > > enable > > > > > > > any > > > > > > > new planes. > > > > > > > > > > > > > > But none of that really matters for the discussion at > > > > > > > hand. > > > > > > > Though > > > > > > > there are other problems with the port sync stuff that > > > > > > > would > > > > > > > need > > > > > > > to be handled better. Eg. I think it now adds both crtcs > > > > > > > to > > > > > > > the > > > > > > > state > > > > > > > always which is going to cut the fps in half. Also the > > > > > > > place > > > > > > > where > > > > > > > it does that stuff is rather suspicious. All that stuff > > > > > > > should be > > > > > > > somewhere a bit higher up IMO. > > > > > > > > > > > > > > > So for MST we only know who is the master in the commit > > > > > > > > phase > > > > > > > > and > > > > > > > > at > > > > > > > > this point we should not modify the computed state. > > > > > > > > > > > > > > I'm not suggesting modifying anything during commit > > > > > > > phase. I > > > > > > > think > > > > > > > you are effectiely doing that right now by stuffing that > > > > > > > mst > > > > > > > master > > > > > > > transcoder into intel_dp. > > > > > > > > > > > > Sorry, I still don't get what approach are you suggesting > > > > > > here. > > > > > > > > > > > > If we can't modify the state in the commit phase, adding > > > > > > mst_master_transcoder in the CRTC state will not be > > > > > > possible > > > > > > while > > > > > > respecting the order imposed by ddb allocations. > > > > > > > > > > The ddb allocation ordering only comes into play when there > > > > > are > > > > > already active pipes. It should always be possible to not > > > > > enable > > > > > the slaves until the master has been shuffled into place in > > > > > the > > > > > ddb and enabled. > > > > > > > > This sounds contradictory to what you answered here: > > > > https://lists.freedesktop.org/archives/intel-gfx/2019-November/221608.html > > > > > > > > Will need to some testing to get the steps but I was able > > > > consistent to > > > > get to state were doing a full modeset in pipe A(mst master) > > > > caused > > > > the > > > > pipe B(mst slave) to enabled first because of the ddb > > > > allocations. > > > > > > > > So can I or not do something like port sync does? And force the > > > > enable > > > > of master before the slaves? If possible, the comment in > > > > skl_commit_modeset_enables() will need some changes. > > > > > > I suspect for the mst stuff we should do: > > > > > > while_dirty_mst_masters() { > > > if (!ddb_overlap) > > > enable(); > > > } > > > while_dirty_mst_slaves() { > > > if (!ddb_overlap) > > > enable(); > > > } > > > > What about this case? > > > > Pipe/transcoder A and B in the same MST stream > > > > # old state - DDB allocation: AABBB > > mst master = transcoder A(computed in atomic check phase) > > entries[0].start = 0 > > entries[0].end = 1 > > entries[1].start = 2 > > entries[1].end = 4 > > > > # new state - DDB allocation: AAABBB > > mst master = transcoder A(computed in atomic check phase) > > entries[0].start = 0 > > entries[0].end = 2 > > entries[1].start = 3 > > entries[1].end = 5 > > > > while_dirty_mst_masters() > > first iteration: pipe A will overlap with old pipe B DDB > > There won't be an old DDB allocation for a pipe if it's going > trough a modeset. Just added some debug messages to skl_commit_modeset_enables(), changed the resolution of the 2 pipes enabled in the same MST stream and old_crtc_state->wm.skl.ddb is set kept set with the old values. So should we fix that first loop in skl_commit_modeset_enables()? for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) /* ignore allocations for crtc's that have been turned off. */ if (!needs_modeset(new_crtc_state) && new_crtc_state- >hw.active) entries[i] = old_crtc_state->wm.skl.ddb; In this case there is not difference between old_crtc_state->wm.skl.ddb or new_crtc_state->wm.skl.ddb, so maybe also change to new_crtc_state? > > > allocation > > second iteration: pipe B is slave of A > > third iteration: ? > > > >
On Wed, Dec 04, 2019 at 06:48:42PM +0000, Souza, Jose wrote: > On Wed, 2019-12-04 at 12:55 +0200, Ville Syrjälä wrote: > > On Tue, Dec 03, 2019 at 10:12:47PM +0000, Souza, Jose wrote: > > > On Tue, 2019-12-03 at 14:47 +0200, Ville Syrjälä wrote: > > > > On Mon, Dec 02, 2019 at 10:03:38PM +0000, Souza, Jose wrote: > > > > > On Thu, 2019-11-28 at 14:06 +0200, Ville Syrjälä wrote: > > > > > > On Thu, Nov 28, 2019 at 01:14:37AM +0000, Souza, Jose wrote: > > > > > > > On Wed, 2019-11-27 at 21:59 +0200, Ville Syrjälä wrote: > > > > > > > > On Tue, Nov 26, 2019 at 08:30:31PM +0000, Souza, Jose > > > > > > > > wrote: > > > > > > > > > On Tue, 2019-11-26 at 22:05 +0200, Ville Syrjälä wrote: > > > > > > > > > > On Fri, Nov 22, 2019 at 04:54:55PM -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. > > > > > > > > > > > > > > > > > > > > > > A previous approach was using the lowest > > > > > > > > > > > pipe/transcoder as > > > > > > > > > > > master > > > > > > > > > > > transcoder but as the comment in > > > > > > > > > > > skl_commit_modeset_enables() > > > > > > > > > > > states, > > > > > > > > > > > that is not always true. > > > > > > > > > > > > > > > > > > > > > > So here promoting the first pipe/transcoder of the > > > > > > > > > > > stream > > > > > > > > > > > as > > > > > > > > > > > master. > > > > > > > > > > > That caused several other problems as during the > > > > > > > > > > > commit > > > > > > > > > > > phase > > > > > > > > > > > the > > > > > > > > > > > state computed should not be changed. > > > > > > > > > > > > > > > > > > > > > > So the master transcoder is store into intel_dp and > > > > > > > > > > > the > > > > > > > > > > > modeset > > > > > > > > > > > in > > > > > > > > > > > slave pipes/transcoders is forced using > > > > > > > > > > > mst_master_trans_pending. > > > > > > > > > > > > > > > > > > > > > > v2: > > > > > > > > > > > - added missing config compute to trigger > > > > > > > > > > > fullmodeset > > > > > > > > > > > in > > > > > > > > > > > slave > > > > > > > > > > > transcoders > > > > > > > > > > > > > > > > > > > > > > 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 | 1 > > > > > > > > > > > 0 +- > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_display.c | 5 > > > > > > > > > > > 8 > > > > > > > > > > > ++++++- > > > > > > > > > > > .../drm/i915/display/intel_display_types.h | > > > > > > > > > > > 3 + > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_dp.c | > > > > > > > > > > > 1 + > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | > > > > > > > > > > > 149 > > > > > > > > > > > +++++++++++++++++- > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_dp_mst.h | > > > > > > > > > > > 2 + > > > > > > > > > > > 6 files changed, 216 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > > > > > > > > > diff --git > > > > > > > > > > > a/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > > > > > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > > > > > > index a976606d21c7..d2f0d393d3ee 100644 > > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > > > > > > > > > > @@ -35,6 +35,7 @@ > > > > > > > > > > > #include "intel_display_types.h" > > > > > > > > > > > #include "intel_dp.h" > > > > > > > > > > > #include "intel_dp_link_training.h" > > > > > > > > > > > +#include "intel_dp_mst.h" > > > > > > > > > > > #include "intel_dpio_phy.h" > > > > > > > > > > > #include "intel_dsi.h" > > > > > > > > > > > #include "intel_fifo_underrun.h" > > > > > > > > > > > @@ -1903,8 +1904,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 = > > > > > > > > > > > intel_dp_mst_master_trans_get(encoder); > > > > > > > > > > > > > > > > > > > > Why isn't that just stored in the crtc state like > > > > > > > > > > everything > > > > > > > > > > else? > > > > > > > > > > > > > > > > > > > > I'm thinking we should maybe do it just like port > > > > > > > > > > sync > > > > > > > > > > and > > > > > > > > > > have > > > > > > > > > > both > > > > > > > > > > master + slave_mask. That way it should be pretty > > > > > > > > > > trivial > > > > > > > > > > to > > > > > > > > > > add > > > > > > > > > > all > > > > > > > > > > the relevant crtcs to the state when needed. > > > > > > > > > > > > > > > > > > I guess port sync is not doing the right thing and it > > > > > > > > > could > > > > > > > > > cause > > > > > > > > > underruns. > > > > > > > > > When it is going to enable the master CRTC of the port > > > > > > > > > sync > > > > > > > > > it > > > > > > > > > forcibly > > > > > > > > > enables the slave first, what could cause underruns > > > > > > > > > because > > > > > > > > > of > > > > > > > > > overlap > > > > > > > > > in ddb allocations(that is what I understood from the > > > > > > > > > comment > > > > > > > > > in > > > > > > > > > skl_commit_modeset_enables()). > > > > > > > > > > > > > > > > Not necessarily just underruns but even a system hang. > > > > > > > > The > > > > > > > > fix > > > > > > > > should > > > > > > > > be > > > > > > > > a trivial "check the slave for ddb overlap as well", but > > > > > > > > apparently I > > > > > > > > failed at convicing people to do that. > > > > > > > > > > > > > > > > I've actually been pondering about decoupling the plane > > > > > > > > updates > > > > > > > > from > > > > > > > > the crtc enable stuff entirely. At least theoretically > > > > > > > > crtc > > > > > > > > enable > > > > > > > > should be able to excute in any order as long we don't > > > > > > > > enable > > > > > > > > any > > > > > > > > new planes. > > > > > > > > > > > > > > > > But none of that really matters for the discussion at > > > > > > > > hand. > > > > > > > > Though > > > > > > > > there are other problems with the port sync stuff that > > > > > > > > would > > > > > > > > need > > > > > > > > to be handled better. Eg. I think it now adds both crtcs > > > > > > > > to > > > > > > > > the > > > > > > > > state > > > > > > > > always which is going to cut the fps in half. Also the > > > > > > > > place > > > > > > > > where > > > > > > > > it does that stuff is rather suspicious. All that stuff > > > > > > > > should be > > > > > > > > somewhere a bit higher up IMO. > > > > > > > > > > > > > > > > > So for MST we only know who is the master in the commit > > > > > > > > > phase > > > > > > > > > and > > > > > > > > > at > > > > > > > > > this point we should not modify the computed state. > > > > > > > > > > > > > > > > I'm not suggesting modifying anything during commit > > > > > > > > phase. I > > > > > > > > think > > > > > > > > you are effectiely doing that right now by stuffing that > > > > > > > > mst > > > > > > > > master > > > > > > > > transcoder into intel_dp. > > > > > > > > > > > > > > Sorry, I still don't get what approach are you suggesting > > > > > > > here. > > > > > > > > > > > > > > If we can't modify the state in the commit phase, adding > > > > > > > mst_master_transcoder in the CRTC state will not be > > > > > > > possible > > > > > > > while > > > > > > > respecting the order imposed by ddb allocations. > > > > > > > > > > > > The ddb allocation ordering only comes into play when there > > > > > > are > > > > > > already active pipes. It should always be possible to not > > > > > > enable > > > > > > the slaves until the master has been shuffled into place in > > > > > > the > > > > > > ddb and enabled. > > > > > > > > > > This sounds contradictory to what you answered here: > > > > > https://lists.freedesktop.org/archives/intel-gfx/2019-November/221608.html > > > > > > > > > > Will need to some testing to get the steps but I was able > > > > > consistent to > > > > > get to state were doing a full modeset in pipe A(mst master) > > > > > caused > > > > > the > > > > > pipe B(mst slave) to enabled first because of the ddb > > > > > allocations. > > > > > > > > > > So can I or not do something like port sync does? And force the > > > > > enable > > > > > of master before the slaves? If possible, the comment in > > > > > skl_commit_modeset_enables() will need some changes. > > > > > > > > I suspect for the mst stuff we should do: > > > > > > > > while_dirty_mst_masters() { > > > > if (!ddb_overlap) > > > > enable(); > > > > } > > > > while_dirty_mst_slaves() { > > > > if (!ddb_overlap) > > > > enable(); > > > > } > > > > > > What about this case? > > > > > > Pipe/transcoder A and B in the same MST stream > > > > > > # old state - DDB allocation: AABBB > > > mst master = transcoder A(computed in atomic check phase) > > > entries[0].start = 0 > > > entries[0].end = 1 > > > entries[1].start = 2 > > > entries[1].end = 4 > > > > > > # new state - DDB allocation: AAABBB > > > mst master = transcoder A(computed in atomic check phase) > > > entries[0].start = 0 > > > entries[0].end = 2 > > > entries[1].start = 3 > > > entries[1].end = 5 > > > > > > while_dirty_mst_masters() > > > first iteration: pipe A will overlap with old pipe B DDB > > > > There won't be an old DDB allocation for a pipe if it's going > > trough a modeset. > > Just added some debug messages to skl_commit_modeset_enables(), changed > the resolution of the 2 pipes enabled in the same MST stream and > old_crtc_state->wm.skl.ddb is set kept set with the old values. > > So should we fix that first loop in skl_commit_modeset_enables()? > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > new_crtc_state, i) > /* ignore allocations for crtc's that have been turned off. */ > if (!needs_modeset(new_crtc_state) && new_crtc_state- > >hw.active) > entries[i] = old_crtc_state->wm.skl.ddb; I thought this is what it already did, but I guess I was wrong. Yes, I think this is what it should do. > In this case there is not difference between old_crtc_state->wm.skl.ddb > or new_crtc_state->wm.skl.ddb, so maybe also change to new_crtc_state? I think it's better to keep using the old crtc state here. Just in case someone comes up with a good reason to do ddb reallocation across pipes when there isn't a modeset.
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index a976606d21c7..d2f0d393d3ee 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -35,6 +35,7 @@ #include "intel_display_types.h" #include "intel_dp.h" #include "intel_dp_link_training.h" +#include "intel_dp_mst.h" #include "intel_dpio_phy.h" #include "intel_dsi.h" #include "intel_fifo_underrun.h" @@ -1903,8 +1904,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 = intel_dp_mst_master_trans_get(encoder); + 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); diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 801b975c7d39..35a59108194e 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" @@ -5365,6 +5366,36 @@ intel_get_crtc_new_encoder(const struct intel_atomic_state *state, return encoder; } +/* + * Finds the encoder associated with the given CRTC. This can only be + * used when we know that the CRTC isn't feeding multiple encoders! + */ +static struct intel_encoder * +intel_get_crtc_old_encoder(const struct intel_atomic_state *state, + const struct intel_crtc_state *crtc_state) +{ + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); + const struct drm_connector_state *connector_state; + const struct drm_connector *connector; + struct intel_encoder *encoder = NULL; + int num_encoders = 0; + int i; + + for_each_old_connector_in_state(&state->base, connector, + connector_state, i) { + if (connector_state->crtc != &crtc->base) + continue; + + encoder = to_intel_encoder(connector_state->best_encoder); + num_encoders++; + } + + WARN(num_encoders != 1, "%d encoders for pipe %c\n", + num_encoders, pipe_name(crtc->pipe)); + + return encoder; +} + /* * Enable PCH resources required for PCH ports: * - PCH PLLs @@ -13365,6 +13396,12 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config, #undef PIPE_CONF_CHECK_COLOR_LUT #undef PIPE_CONF_QUIRK + if (fastset && pipe_config->mst_master_trans_pending) { + DRM_DEBUG_KMS("[CRTC:%d:%s] fastset mismatch in mst_master_trans\n", + crtc->base.base.id, crtc->base.name); + ret = false; + } + return ret; } @@ -14449,22 +14486,35 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state) struct intel_crtc *crtc; 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) || !crtc->active) continue; + if (!is_trans_port_sync_mode(new_crtc_state) && + !intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST)) + continue; + /* In case of Transcoder port Sync master slave CRTCs can be * assigned in any order and we need to make sure that * slave CRTCs are disabled first and then master CRTC since * Slave vblanks are masked till Master Vblanks. */ - if (!is_trans_port_sync_mode(old_crtc_state)) - continue; - if (is_trans_port_sync_master(old_crtc_state)) + if (is_trans_port_sync_mode(new_crtc_state) && + is_trans_port_sync_master(new_crtc_state)) continue; + if (intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST)) { + struct intel_encoder *encoder; + + encoder = intel_get_crtc_old_encoder(state, + old_crtc_state); + if (intel_dp_mst_master_trans_get(encoder) == + old_crtc_state->cpu_transcoder) + continue; + } + intel_pre_plane_update(old_crtc_state, new_crtc_state); intel_old_crtc_state_disables(state, old_crtc_state, new_crtc_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..23d747cdca64 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -1052,6 +1052,8 @@ struct intel_crtc_state { /* Pointer to master transcoder in case of tiled displays */ enum transcoder master_transcoder; + bool mst_master_trans_pending; + /* Bitmask to indicate slaves attached */ u8 sync_mode_slaves_mask; }; @@ -1284,6 +1286,7 @@ struct intel_dp { bool can_mst; /* this port supports mst */ bool is_mst; int active_mst_links; + enum transcoder mst_master_transcoder; /* Only used in TGL+ */ /* * DP_TP_* registers may be either on port or transcoder register space. diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 3123958e2081..ceff6901451a 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -7424,6 +7424,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, intel_dp->reset_link_params = true; intel_dp->pps_pipe = INVALID_PIPE; intel_dp->active_pipe = INVALID_PIPE; + intel_dp->mst_master_transcoder = INVALID_TRANSCODER; /* Preserve the current hw state. */ intel_dp->DP = I915_READ(intel_dp->output_reg); diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index f8a350359346..9731c3c1d3f2 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -87,6 +87,47 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, return 0; } +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 drm_i915_private *dev_priv = to_i915(encoder->base.dev); + struct intel_crtc_state *new_crtc_state; + struct intel_crtc *crtc; + enum transcoder master; + int i; + + if (INTEL_GEN(dev_priv) < 12) + return 0; + + if (!conn_state->crtc) + return 0; + + master = intel_dp_mst_master_trans_get(encoder); + if (master == INVALID_TRANSCODER) + return 0; + + for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) { + /* + * cpu_transcoder is set when computing CRTC state if it will + * be disabled it will not happen, so checking pipe instead + */ + if (crtc->pipe != (enum pipe)master) + continue; + + if (!drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi) && + new_crtc_state->uapi.enable) + continue; + + pipe_config->mst_master_trans_pending = true; + 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 +195,95 @@ 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 intel_connector *intel_conn = to_intel_connector(connector); + 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 drm_crtc_state *crtc_state; + struct drm_connector *conn_iter; + + if (INTEL_GEN(dev_priv) < 12) + return 0; + + 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 + * - 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_conn->mst_port->mst_master_transcoder != + intel_crtc_state->cpu_transcoder) + return 0; + + /* Is master, mark all other CRTCs as needing a modeset */ + drm_connector_list_iter_begin(state->dev, &conn_list_iter); + drm_for_each_connector_iter(conn_iter, &conn_list_iter) { + struct intel_connector *intel_conn_iter; + struct drm_connector_state *conn_iter_state; + + 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); + } + + intel_crtc_state = to_intel_crtc_state(crtc_state); + crtc_state->mode_changed = true; + } + drm_connector_list_iter_end(&conn_list_iter); + return 0; } @@ -175,6 +305,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; @@ -259,6 +393,7 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder, intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); intel_dig_port->base.post_disable(&intel_dig_port->base, old_crtc_state, NULL); + intel_dp->mst_master_transcoder = INVALID_TRANSCODER; } DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links); @@ -314,8 +449,11 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder, DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links); - if (first_mst_stream) + if (first_mst_stream) { + WARN_ON(intel_dp->mst_master_transcoder != INVALID_TRANSCODER); + intel_dp->mst_master_transcoder = pipe_config->cpu_transcoder; intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); + } drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port, true); @@ -717,3 +855,12 @@ 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 */ } + +enum transcoder +intel_dp_mst_master_trans_get(struct intel_encoder *encoder) +{ + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base); + struct intel_dp *intel_dp = &intel_mst->primary->dp; + + return intel_dp->mst_master_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..e6f28a517182 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h @@ -7,9 +7,11 @@ #define __INTEL_DP_MST_H__ struct intel_digital_port; +struct intel_encoder; 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); +enum transcoder intel_dp_mst_master_trans_get(struct intel_encoder *encoder); #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. A previous approach was using the lowest pipe/transcoder as master transcoder but as the comment in skl_commit_modeset_enables() states, that is not always true. So here promoting the first pipe/transcoder of the stream as master. That caused several other problems as during the commit phase the state computed should not be changed. So the master transcoder is store into intel_dp and the modeset in slave pipes/transcoders is forced using mst_master_trans_pending. v2: - added missing config compute to trigger fullmodeset in slave transcoders 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 | 10 +- drivers/gpu/drm/i915/display/intel_display.c | 58 ++++++- .../drm/i915/display/intel_display_types.h | 3 + drivers/gpu/drm/i915/display/intel_dp.c | 1 + drivers/gpu/drm/i915/display/intel_dp_mst.c | 149 +++++++++++++++++- drivers/gpu/drm/i915/display/intel_dp_mst.h | 2 + 6 files changed, 216 insertions(+), 7 deletions(-)