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.
> + 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
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
