On Fri, Aug 23, 2019 at 01:20:45AM -0700, Lucas De Marchi wrote:
> From: José Roberto de Souza <jose.so...@intel.com>
>
> On TGL the blending of all the streams have moved from DDI to
> transcoder, so now every transcoder working over the same MST port must
> send its stream to a master transcoder and master will send to DDI
> respecting the time slots.
>
> So here it is picking the lowest pipe/transcoder as it will be
> enabled first and disabled last.
> BSpec: 50493
> BSpec: 49190
>
> v2: Missed set mst_master_trans to TRANSCODER_INVALID when computing HSW
> encoder config. HSW CRT hw state readout calls
> hsw_crt_get_config()->intel_ddi_get_config() that will set
> mst_master_trans to TRANSCODER_INVALID causing the mismatch when
> verifying CRTC state after a modeset. (José)
>
> v3: Add WARN_ON() requested by Jani.
> Add FIXME. From Jani: double check PIPE_CONF_CHECK_I(mst_master_trans) -
> it's
> now checking for all platforms and MST and non-MST alike.
> Perhaps in general I'd like the approach of only doing the readout when
> it's relevant, and only checking the value when it's relevant.
>
> Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> Cc: Manasi Navare <manasi.d.nav...@intel.com>
> Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
> Signed-off-by: José Roberto de Souza <jose.so...@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demar...@intel.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_crt.c | 2 +
> drivers/gpu/drm/i915/display/intel_ddi.c | 18 ++
> drivers/gpu/drm/i915/display/intel_display.c | 15 ++
> drivers/gpu/drm/i915/display/intel_display.h | 3 +
> .../drm/i915/display/intel_display_types.h | 3 +
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 159 +++++++++++++++++-
> drivers/gpu/drm/i915/display/intel_dp_mst.h | 2 +
> drivers/gpu/drm/i915/i915_reg.h | 3 +
> 8 files changed, 202 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_crt.c
> b/drivers/gpu/drm/i915/display/intel_crt.c
> index e6e8d4a82044..503135200cb3 100644
> --- a/drivers/gpu/drm/i915/display/intel_crt.c
> +++ b/drivers/gpu/drm/i915/display/intel_crt.c
> @@ -416,6 +416,8 @@ static int hsw_crt_compute_config(struct intel_encoder
> *encoder,
> /* FDI must always be 2.7 GHz */
> pipe_config->port_clock = 135000 * 2;
>
> + pipe_config->mst_master_trans = TRANSCODER_INVALID;
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 8eb2b3ec01ed..f4cb6bd74421 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -1840,6 +1840,13 @@ void intel_ddi_enable_transcoder_func(const struct
> intel_crtc_state *crtc_state)
> } else if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST)) {
> temp |= TRANS_DDI_MODE_SELECT_DP_MST;
> temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> +
> + if (INTEL_GEN(dev_priv) >= 12) {
> + enum transcoder master = crtc_state->mst_master_trans;
> +
> + WARN_ON(master == TRANSCODER_INVALID);
> + temp |= TRANS_DDI_MST_TRANSPORT_SELECT_DPTP(master);
> + }
> } else {
> temp |= TRANS_DDI_MODE_SELECT_DP_SST;
> temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> @@ -3861,6 +3868,8 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
> break;
> }
>
> + pipe_config->mst_master_trans = TRANSCODER_INVALID;
> +
> switch (temp & TRANS_DDI_MODE_SELECT_MASK) {
> case TRANS_DDI_MODE_SELECT_HDMI:
> pipe_config->has_hdmi_sink = true;
> @@ -3896,6 +3905,13 @@ void intel_ddi_get_config(struct intel_encoder
> *encoder,
> pipe_config->output_types |= BIT(INTEL_OUTPUT_DP_MST);
> pipe_config->lane_count =
> ((temp & DDI_PORT_WIDTH_MASK) >> DDI_PORT_WIDTH_SHIFT)
> + 1;
> +
> + if (INTEL_GEN(dev_priv) >= 12) {
> + temp = temp & TRANS_DDI_MST_TRANSPORT_SELECT_MASK;
> + temp = temp >> TRANS_DDI_MST_TRANSPORT_SELECT_SHIFT;
> + pipe_config->mst_master_trans = temp;
> + }
> +
> intel_dp_get_m_n(intel_crtc, pipe_config);
> break;
> default:
> @@ -3998,6 +4014,8 @@ static int intel_ddi_compute_config(struct
> intel_encoder *encoder,
>
> intel_ddi_compute_min_voltage_level(dev_priv, pipe_config);
>
> + pipe_config->mst_master_trans = TRANSCODER_INVALID;
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index ddb8436e2208..109d4fd961c6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -47,6 +47,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"
> @@ -12138,6 +12139,14 @@ static void intel_dump_pipe_config(const struct
> intel_crtc_state *pipe_config,
>
> intel_dpll_dump_hw_state(dev_priv, &pipe_config->dpll_hw_state);
>
> + if (INTEL_GEN(dev_priv) >= 12 &&
> + intel_crtc_has_type(pipe_config, INTEL_OUTPUT_DP_MST)) {
> + enum transcoder master = pipe_config->mst_master_trans;
> +
> + DRM_DEBUG_KMS("master mst cpu_transcoder: %s\n",
> + transcoder_name(master));
> + }
> +
> dump_planes:
> if (!state)
> return;
> @@ -12821,6 +12830,8 @@ intel_pipe_config_compare(const struct
> intel_crtc_state *current_config,
> PIPE_CONF_CHECK_INFOFRAME(hdmi);
> PIPE_CONF_CHECK_INFOFRAME(drm);
>
> + PIPE_CONF_CHECK_I(mst_master_trans);
> +
> #undef PIPE_CONF_CHECK_X
> #undef PIPE_CONF_CHECK_I
> #undef PIPE_CONF_CHECK_BOOL
> @@ -13597,6 +13608,10 @@ static int intel_atomic_check(struct drm_device *dev,
> int ret, i;
> bool any_ms = state->cdclk.force_min_cdclk_changed;
>
> + ret = intel_dp_mst_atomic_add_affected_crtcs(state);
> + if (ret)
> + return ret;
This feels too early. I think it should be after
drm_atomic_helper_check_modeset() at least.
> +
> /* Catch I915_MODE_FLAG_INHERITED */
> for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i) {
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h
> b/drivers/gpu/drm/i915/display/intel_display.h
> index b63fb7a4599e..282e8d8f1cc1 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -91,6 +91,8 @@ enum pipe {
> #define pipe_name(p) ((p) + 'A')
>
> enum transcoder {
> + TRANSCODER_INVALID = -1,
> +
> /*
> * The following transcoders have a 1:1 transcoder -> pipe mapping,
> * keep their values fixed: the code assumes that TRANSCODER_A=0, the
> @@ -132,6 +134,7 @@ static inline const char *transcoder_name(enum transcoder
> transcoder)
> return "DSI A";
> case TRANSCODER_DSI_C:
> return "DSI C";
> + case TRANSCODER_INVALID:
> default:
> return "<invalid>";
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 449abaea619f..d84a66459daf 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -984,6 +984,9 @@ struct intel_crtc_state {
>
> /* Forward Error correction State */
> bool fec_enable;
> +
> + /* Master transcoder for all streams, only used on TGL+ */
> + enum transcoder mst_master_trans;
> };
>
> struct intel_crtc {
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 2c5ac3dd647f..8002dca9b734 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -87,6 +87,50 @@ static int intel_dp_mst_compute_link_config(struct
> intel_encoder *encoder,
> return 0;
> }
>
> +/*
> + * Iterate over all the CRTCs and return the transcoder of the lowest CRTC
> that
> + * share the same MST connector.
> + */
> +static enum transcoder
> +mst_compute_master_trans(struct drm_atomic_state *state,
> + struct drm_connector *mst_conn)
> +{
> + struct intel_connector *intel_mst_conn = to_intel_connector(mst_conn);
> + struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> + struct drm_i915_private *dev_priv = to_i915(state->dev);
> + struct intel_crtc_state *intel_crtc_state;
> + struct intel_crtc *intel_crtc;
Please switch to intel_ types fully and drop the intel_ prefix from the
variables.
> + int i;
> +
> + if (INTEL_GEN(dev_priv) < 12)
> + return TRANSCODER_INVALID;
> +
> + /* Iterate from the lowest to the highest pipe */
> + for_each_new_intel_crtc_in_state(intel_state, intel_crtc,
> intel_crtc_state, i) {
> + struct intel_digital_connector_state *intel_conn_state;
> + struct intel_connector *intel_conn;
> + int j;
> +
> + if (!intel_crtc_state->base.active)
> + continue;
> +
> + for_each_new_intel_connector_in_state(intel_state, intel_conn,
> + intel_conn_state, j) {
> + /* Only care about connectors of this CRTC */
> + if (intel_conn_state->base.crtc !=
> + intel_crtc_state->base.crtc)
> + continue;
> +
> + if (intel_conn->mst_port != intel_mst_conn->mst_port)
> + continue;
> +
> + return intel_crtc_state->cpu_transcoder;
> + }
> + }
> +
> + return TRANSCODER_INVALID;
> +}
> +
> static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
> struct intel_crtc_state *pipe_config,
> struct drm_connector_state *conn_state)
> @@ -94,14 +138,15 @@ static int intel_dp_mst_compute_config(struct
> intel_encoder *encoder,
> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
> struct intel_dp *intel_dp = &intel_mst->primary->dp;
> - struct intel_connector *connector =
> - to_intel_connector(conn_state->connector);
> + struct drm_connector *connector = conn_state->connector;
> + struct intel_connector *intel_connector = to_intel_connector(connector);
> struct intel_digital_connector_state *intel_conn_state =
> to_intel_digital_connector_state(conn_state);
> const struct drm_display_mode *adjusted_mode =
> &pipe_config->base.adjusted_mode;
> - void *port = connector->port;
> + void *port = intel_connector->port;
> struct link_config_limits limits;
> + enum transcoder master;
> int ret;
>
> if (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)
> @@ -146,6 +191,51 @@ static int intel_dp_mst_compute_config(struct
> intel_encoder *encoder,
>
> intel_ddi_compute_min_voltage_level(dev_priv, pipe_config);
>
> + master = mst_compute_master_trans(conn_state->state, connector);
> + pipe_config->mst_master_trans = master;
> +
> + return 0;
> +}
> +
> +static int
> +intel_dp_mst_master_trans_check(struct drm_connector *conn,
> + struct drm_connector_state *new_conn_state)
> +{
> + struct drm_i915_private *dev_priv = to_i915(conn->dev);
> + struct drm_atomic_state *state = new_conn_state->state;
> + struct drm_connector_state *old_conn_state =
> + drm_atomic_get_old_connector_state(state, conn);
> + struct drm_crtc *new_crtc = new_conn_state->crtc;
> + struct drm_crtc *old_crtc = old_conn_state->crtc;
> + enum transcoder old_master_trans = TRANSCODER_INVALID;
> + enum transcoder new_master_trans = TRANSCODER_INVALID;
> + struct drm_crtc_state *new_crtc_state, *old_crtc_state;
> +
> + if (INTEL_GEN(dev_priv) < 12)
> + return 0;
> +
> + if (old_crtc) {
> + struct intel_crtc_state *intel_crtc_state;
Same story with types and variable naming.
> +
> + old_crtc_state = drm_atomic_get_new_crtc_state(state, old_crtc);
> + intel_crtc_state = to_intel_crtc_state(old_crtc_state);
> + old_master_trans = intel_crtc_state->mst_master_trans;
> + }
> +
> + if (new_crtc) {
> + struct intel_crtc_state *intel_crtc_state;
> +
> + new_crtc_state = drm_atomic_get_new_crtc_state(state, new_crtc);
> + intel_crtc_state = to_intel_crtc_state(new_crtc_state);
> + new_master_trans = mst_compute_master_trans(state, conn);
> + }
> +
> + if (old_crtc && old_master_trans != new_master_trans)
> + old_crtc_state->mode_changed = true;
> +
> + if (new_crtc && old_master_trans != new_master_trans)
> + new_crtc_state->mode_changed = true;
> +
> return 0;
> }
>
> @@ -168,6 +258,10 @@ intel_dp_mst_atomic_check(struct drm_connector
> *connector,
> if (ret)
> return ret;
>
> + ret = intel_dp_mst_master_trans_check(connector, new_conn_state);
> + if (ret)
> + return ret;
> +
> if (!old_conn_state->crtc)
> return 0;
>
> @@ -674,3 +768,62 @@ 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 */
> }
> +
> +/**
> + * intel_dp_mst_atomic_add_affected_crtcs - Add all CRTCs that share the MST
> + * stream with the CRTCs in the current atomic state.
> + * @state: state to add CRTCs
> + *
> + * It is needed add the CRTCs trigger a call to atomic_check() to
> + * every connector attached to the CRTC in case a new master transcoder will
> + * be needed.
> + */
> +int intel_dp_mst_atomic_add_affected_crtcs(struct intel_atomic_state *state)
> +{
> + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> + struct intel_digital_connector_state *intel_conn_state;
> + struct drm_device *dev = state->base.dev;
> + struct intel_connector *intel_conn;
> + int i;
> +
> + if (INTEL_GEN(dev_priv) < 12)
> + return 0;
> +
> + for_each_new_intel_connector_in_state(state, intel_conn,
> intel_conn_state, i) {
> + struct drm_connector_list_iter conn_list_iter;
> + struct drm_connector *conn_iter;
> +
> + if (!intel_conn->mst_port)
> + continue;
I think we should be able to skip this unless a modeset is required.
> +
> + drm_connector_list_iter_begin(dev, &conn_list_iter);
> + drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> + struct drm_connector_state *conn_iter_state;
> + struct intel_connector *intel_conn_iter;
> + struct drm_crtc_state *crtc_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->base, 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->base,
> +
> conn_iter_state->crtc);
> + if (IS_ERR(crtc_state)) {
> + drm_connector_list_iter_end(&conn_list_iter);
> + return PTR_ERR(crtc_state);
> + }
> + }
> + drm_connector_list_iter_end(&conn_list_iter);
> + }
> +
> + return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> index f660ad80db04..173598aa81d2 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> @@ -6,10 +6,12 @@
> #ifndef __INTEL_DP_MST_H__
> #define __INTEL_DP_MST_H__
>
> +struct intel_atomic_state;
> struct intel_digital_port;
>
> 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_atomic_add_affected_crtcs(struct intel_atomic_state *state);
> int intel_dp_mst_encoder_active_links(struct intel_digital_port
> *intel_dig_port);
>
> #endif /* __INTEL_DP_MST_H__ */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 3de02683d856..bff9ee191832 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9424,6 +9424,9 @@ enum skl_power_gate {
> #define TRANS_DDI_EDP_INPUT_A_ONOFF (4 << 12)
> #define TRANS_DDI_EDP_INPUT_B_ONOFF (5 << 12)
> #define TRANS_DDI_EDP_INPUT_C_ONOFF (6 << 12)
> +#define TRANS_DDI_MST_TRANSPORT_SELECT_SHIFT 10 /* TGL+ */
> +#define TRANS_DDI_MST_TRANSPORT_SELECT_MASK (0x3 << 10)
> +#define TRANS_DDI_MST_TRANSPORT_SELECT_DPTP(trans) ((trans) << 10)
> #define TRANS_DDI_HDCP_SIGNALLING (1 << 9)
> #define TRANS_DDI_DP_VC_PAYLOAD_ALLOC (1 << 8)
> #define TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE (1 << 7)
> --
> 2.23.0
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
