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@xxxxxxxxxxxxxxx> > > > > Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx> > > > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx> > > > > --- > > > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx