On Wed, 2019-12-18 at 21:24 +0200, Ville Syrjälä wrote:
> On Wed, Dec 18, 2019 at 10:59:06AM -0800, José Roberto de Souza
> wrote:
> > On TGL the blending of all the streams have moved from DDI to
> > transcoder, so now every transcoder working over the same MST port
> > must
> > send its stream to a master transcoder and master will send to DDI
> > respecting the time slots.
> >
> > So here adding all the CRTCs that shares the same MST stream if
> > needed and computing their state again, it will pick the lowest
> > pipe/transcoder among the ones in the same stream to be master.
> >
> > Most of the time skl_commit_modeset_enables() enables pipes in a
> > crescent order but due DDB overlapping it might not happen, this
> > scenarios will be handled in the next patch.
> >
> > v2:
> > - Using recently added intel_crtc_state_reset() to set
> > mst_master_transcoder to invalid transcoder for all non gen12 & MST
> > code paths
> > - Setting lowest pipe/transcoder as master, previously it was the
> > first one but setting a predictable one will help in future MST e
> > port sync integration
> > - Moving to intel type as much as we can
> >
> > v3:
> > - Now intel_dp_mst_master_trans_compute() returns the MST master
> > transcoder
> > - Replaced stdbool.h by linux/types.h
> > - Skip the connector being checked in
> > intel_dp_mst_atomic_master_trans_check()
> > - Using pipe instead of transcoder to compute MST master
> >
> > v4:
> > - renamed connector_state to conn_state
> >
> > 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_atomic.c | 14 ++
> > drivers/gpu/drm/i915/display/intel_atomic.h | 4 +
> > drivers/gpu/drm/i915/display/intel_ddi.c | 14 +-
> > drivers/gpu/drm/i915/display/intel_display.c | 13 +-
> > .../drm/i915/display/intel_display_types.h | 3 +
> > drivers/gpu/drm/i915/display/intel_dp_mst.c | 143
> > ++++++++++++++++--
> > drivers/gpu/drm/i915/display/intel_dp_mst.h | 5 +
> > 7 files changed, 183 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > index b7dda18b6f29..0eb973f65977 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > @@ -192,6 +192,20 @@ intel_connector_needs_modeset(struct
> > intel_atomic_state *state,
> >
> > new_conn_state->crtc)));
> > }
> >
> > +struct intel_digital_connector_state *
> > +intel_atomic_get_digital_connector_state(struct intel_atomic_state
> > *state,
> > + struct intel_connector
> > *connector)
> > +{
> > + struct drm_connector_state *conn_state;
> > +
> > + conn_state = drm_atomic_get_connector_state(&state->base,
> > + &connector->base);
> > + if (IS_ERR(conn_state))
> > + return ERR_CAST(conn_state);
> > +
> > + return to_intel_digital_connector_state(conn_state);
> > +}
> > +
> > /**
> > * intel_crtc_duplicate_state - duplicate crtc state
> > * @crtc: drm crtc
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h
> > b/drivers/gpu/drm/i915/display/intel_atomic.h
> > index a7d1a8576c48..74c749dbfb4f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> > @@ -17,6 +17,7 @@ struct drm_device;
> > struct drm_i915_private;
> > struct drm_property;
> > struct intel_atomic_state;
> > +struct intel_connector;
> > struct intel_crtc;
> > struct intel_crtc_state;
> >
> > @@ -34,6 +35,9 @@ struct drm_connector_state *
> > intel_digital_connector_duplicate_state(struct drm_connector
> > *connector);
> > bool intel_connector_needs_modeset(struct intel_atomic_state
> > *state,
> > struct drm_connector *connector);
> > +struct intel_digital_connector_state *
> > +intel_atomic_get_digital_connector_state(struct intel_atomic_state
> > *state,
> > + struct intel_connector
> > *connector);
> >
> > struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc
> > *crtc);
> > void intel_crtc_destroy_state(struct drm_crtc *crtc,
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index fa40ba7cbcad..9d99ec82d072 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -1899,8 +1899,13 @@ intel_ddi_transcoder_func_reg_val_get(const
> > struct intel_crtc_state *crtc_state)
> > temp |= TRANS_DDI_MODE_SELECT_DP_MST;
> > temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> >
> > - if (INTEL_GEN(dev_priv) >= 12)
> > - temp |=
> > TRANS_DDI_MST_TRANSPORT_SELECT(crtc_state->cpu_transcoder);
> > + if (INTEL_GEN(dev_priv) >= 12) {
> > + enum transcoder master;
> > +
> > + master = crtc_state->mst_master_transcoder;
> > + WARN_ON(master == INVALID_TRANSCODER);
> > + temp |= TRANS_DDI_MST_TRANSPORT_SELECT(master);
> > + }
> > } else {
> > temp |= TRANS_DDI_MODE_SELECT_DP_SST;
> > temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> > @@ -4400,6 +4405,11 @@ void intel_ddi_get_config(struct
> > intel_encoder *encoder,
> > pipe_config->output_types |= BIT(INTEL_OUTPUT_DP_MST);
> > pipe_config->lane_count =
> > ((temp & DDI_PORT_WIDTH_MASK) >>
> > DDI_PORT_WIDTH_SHIFT) + 1;
> > +
> > + if (INTEL_GEN(dev_priv) >= 12)
> > + pipe_config->mst_master_transcoder =
> > + REG_FIELD_GET(TRANS_DDI_MST_TRA
> > NSPORT_SELECT_MASK, temp);
> > +
> > intel_dp_get_m_n(intel_crtc, pipe_config);
> > break;
> > default:
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 25bf084427bf..59b3bfe8b721 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"
> > @@ -11630,6 +11631,7 @@ static void intel_crtc_state_reset(struct
> > intel_crtc_state *crtc_state,
> > crtc_state->hsw_workaround_pipe = INVALID_PIPE;
> > crtc_state->output_format = INTEL_OUTPUT_FORMAT_INVALID;
> > crtc_state->scaler_state.scaler_id = -1;
> > + crtc_state->mst_master_transcoder = INVALID_TRANSCODER;
> > }
> >
> > /* Returns the currently programmed mode of the given encoder. */
> > @@ -12477,6 +12479,9 @@ static void intel_dump_pipe_config(const
> > struct intel_crtc_state *pipe_config,
> > pipe_config->csc_mode, pipe_config-
> > >gamma_mode,
> > pipe_config->gamma_enable, pipe_config-
> > >csc_enable);
> >
> > + DRM_DEBUG_KMS("MST master transcoder: %s\n",
> > + transcoder_name(pipe_config-
> > >mst_master_transcoder));
> > +
> > dump_planes:
> > if (!state)
> > return;
> > @@ -12618,6 +12623,7 @@ intel_crtc_prepare_cleared_state(struct
> > intel_crtc_state *crtc_state)
> > memcpy(saved_state->icl_port_dplls, crtc_state->icl_port_dplls,
> > sizeof(saved_state->icl_port_dplls));
> > saved_state->crc_enabled = crtc_state->crc_enabled;
> > + saved_state->mst_master_transcoder = INVALID_TRANSCODER;
>
> That's redundant now?
No, we don't call intel_crtc_state_reset() when computing state.
Did not changed that because we are copying a bunch of stuff from the
saved_state.
>
> > if (IS_G4X(dev_priv) ||
> > IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > saved_state->wm = crtc_state->wm;
> > @@ -13257,6 +13263,8 @@ intel_pipe_config_compare(const struct
> > intel_crtc_state *current_config,
> > PIPE_CONF_CHECK_I(dsc.dsc_split);
> > PIPE_CONF_CHECK_I(dsc.compressed_bpp);
> >
> > + PIPE_CONF_CHECK_I(mst_master_transcoder);
> > +
> > #undef PIPE_CONF_CHECK_X
> > #undef PIPE_CONF_CHECK_I
> > #undef PIPE_CONF_CHECK_BOOL
> > @@ -14341,7 +14349,7 @@ static void
> > intel_commit_modeset_disables(struct intel_atomic_state *state)
> > u32 handled = 0;
> > int i;
> >
> > - /* Only disable port sync slaves */
> > + /* Only disable port sync and MST slaves */
> > for_each_oldnew_intel_crtc_in_state(state, crtc,
> > old_crtc_state,
> > new_crtc_state, i) {
> > if (!needs_modeset(new_crtc_state))
> > @@ -14355,7 +14363,8 @@ static void
> > intel_commit_modeset_disables(struct intel_atomic_state *state)
> > * slave CRTCs are disabled first and then master CRTC
> > since
> > * Slave vblanks are masked till Master Vblanks.
> > */
> > - if (!is_trans_port_sync_slave(old_crtc_state))
> > + if (!is_trans_port_sync_slave(old_crtc_state) &&
> > + !intel_dp_mst_is_slave_trans(old_crtc_state))
> > continue;
> >
> > intel_pre_plane_update(state, crtc);
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 83ea04149b77..630a94892b7b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1054,6 +1054,9 @@ struct intel_crtc_state {
> >
> > /* Bitmask to indicate slaves attached */
> > u8 sync_mode_slaves_mask;
> > +
> > + /* Only valid on TGL+ */
> > + enum transcoder mst_master_transcoder;
> > };
> >
> > struct intel_crtc {
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index 7aa0975c33b7..710137984c71 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -87,6 +87,54 @@ static int
> > intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
> > return 0;
> > }
> >
> > +/*
> > + * Iterate over all connectors and set mst_master_transcoder to
> > the smallest
> > + * transcoder id that shares the same MST connector.
> > + */
> > +static enum transcoder
> > +intel_dp_mst_master_trans_compute(struct intel_encoder *encoder,
> > + struct intel_crtc_state *crtc_state,
> > + struct drm_connector_state
> > *connector_state)
> > +{
> > + struct intel_atomic_state *state =
> > to_intel_atomic_state(crtc_state->uapi.state);
>
> Could grab that from the connector state and not pass in crtc_state
> at
> all?
>
> Hmm. Might be even better to just pass inin
> intel_atomic_state+mst_port
> so we wouldn't have the clash with the iterators and could use the
> normal names for those. Shrug.
Okay
>
>
> > + struct intel_connector *connector =
> > to_intel_connector(connector_state->connector);
> > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > + struct intel_digital_connector_state *iter_connector_state;
>
> Could sprinkle some consts on the crtc/connector states now that
> we're pure.
>
> > + struct intel_connector *iter_connector;
> > + enum pipe ret = I915_MAX_PIPES;
> > + int i;
> > +
> > + if (INTEL_GEN(dev_priv) < 12)
> > + return INVALID_TRANSCODER;
> > +
> > + for_each_new_intel_connector_in_state(state, iter_connector,
> > + iter_connector_state, i)
> > {
> > + struct intel_crtc_state *iter_crtc_state;
> > + struct intel_crtc *iter_crtc;
> > +
> > + if (connector->mst_port != iter_connector->mst_port ||
> > + !iter_connector_state->base.crtc)
> > + continue;
> > +
> > + iter_crtc = to_intel_crtc(iter_connector_state-
> > >base.crtc);
> > + iter_crtc_state =
> > intel_atomic_get_new_crtc_state(state,
> > + iter_
> > crtc);
> > + if (!iter_crtc_state->uapi.active)
> > + continue;
> > +
> > + /*
> > + * Using crtc->pipe because crtc_state->cpu_transcoder
> > is
> > + * computed, so others pipe cpu_transcoder could have
> > being
> > + * computed yet
> > + */
> > + if (iter_crtc->pipe < ret)
> > + ret = iter_crtc->pipe;
> > + }
>
> What if we have no active pipes? Aren't we going to come here with
> ret==MAX_PIPES?
>
> I guess we can't really change the !active check to !enabled
> because we surely can't use an inactive transcoder as the
> master. So !active does seem like the right thing to check.
>
> > +
> > + /* Simple cast works because TGL don't have a eDP transcoder */
> > + return (enum transcoder)ret;
>
> I'd have probably extracted the thing to compute the transcoder.
> But doesn't matter for MST since it can't be going over the EDP
> transcoder. Port sync will be a different story. We can cross that
> bridge when we come to it.
>
> > +}
> > +
> > static int intel_dp_mst_compute_config(struct intel_encoder
> > *encoder,
> > struct intel_crtc_state
> > *pipe_config,
> > struct drm_connector_state
> > *conn_state)
> > @@ -154,24 +202,89 @@ static int intel_dp_mst_compute_config(struct
> > intel_encoder *encoder,
> >
> > intel_ddi_compute_min_voltage_level(dev_priv, pipe_config);
> >
> > + pipe_config->mst_master_transcoder =
> > intel_dp_mst_master_trans_compute(encoder,
> > +
> > pipe_config,
> > +
> > conn_state);
> > + if (ret)
> > + return ret;
>
> ret?
My bad, it is left over from the change to return the master
transcoder.
>
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * If one of the connectors in a MST stream needs a modeset, mark
> > all CRTCs
> > + * that have a connector in the same MST stream as mode changed,
> > + * intel_modeset_pipe_config()+intel_crtc_check_fastset() will
> > take to do a
> > + * fastset when possible.
> > + */
> > +static int
> > +intel_dp_mst_atomic_master_trans_check(struct intel_connector
> > *connector,
> > + struct intel_atomic_state
> > *state)
> > +{
> > + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > + struct drm_connector_list_iter connector_list_iter;
> > + struct intel_connector *connector_iter;
> > +
> > + if (INTEL_GEN(dev_priv) < 12)
> > + return 0;
> > +
> > + if (!intel_connector_needs_modeset(state, &connector->base))
> > + return 0;
> > +
> > + drm_connector_list_iter_begin(&dev_priv->drm,
> > &connector_list_iter);
> > + for_each_intel_connector_iter(connector_iter,
> > &connector_list_iter) {
> > + struct intel_digital_connector_state *conn_iter_state;
> > + struct intel_crtc_state *crtc_state;
> > + struct intel_crtc *crtc;
> > +
> > + if (connector_iter->mst_port != connector->mst_port ||
> > + connector_iter == connector)
> > + continue;
> > +
> > + conn_iter_state =
> > intel_atomic_get_digital_connector_state(state,
> > +
> > connector_iter);
> > + if (IS_ERR(conn_iter_state)) {
> > + drm_connector_list_iter_end(&connector_list_ite
> > r);
> > + return PTR_ERR(conn_iter_state);
> > + }
> > +
> > + if (!conn_iter_state->base.crtc)
> > + continue;
> > +
> > + crtc = to_intel_crtc(conn_iter_state->base.crtc);
> > + crtc_state = intel_atomic_get_crtc_state(&state->base,
> > crtc);
> > + if (IS_ERR(crtc_state)) {
> > + drm_connector_list_iter_end(&connector_list_ite
> > r);
> > + return PTR_ERR(crtc_state);
> > + }
> > +
> > + crtc_state->uapi.mode_changed = true;
>
> We might need drm_atomic_add_affected_planes() here. The helper won't
> do
> that for us if the connector->atomic_check() was called from the
> second
> loop.
Done
>
> The rest lgtm.
>
> > + }
> > + drm_connector_list_iter_end(&connector_list_iter);
> > +
> > return 0;
> > }
> >
> > static int
> > intel_dp_mst_atomic_check(struct drm_connector *connector,
> > - struct drm_atomic_state *state)
> > + struct drm_atomic_state *_state)
> > {
> > + struct intel_atomic_state *state =
> > to_intel_atomic_state(_state);
> > struct drm_connector_state *new_conn_state =
> > - drm_atomic_get_new_connector_state(state, connector);
> > + drm_atomic_get_new_connector_state(&state->base,
> > connector);
> > struct drm_connector_state *old_conn_state =
> > - drm_atomic_get_old_connector_state(state, connector);
> > + drm_atomic_get_old_connector_state(&state->base,
> > connector);
> > struct intel_connector *intel_connector =
> > to_intel_connector(connector);
> > struct drm_crtc *new_crtc = new_conn_state->crtc;
> > struct drm_dp_mst_topology_mgr *mgr;
> > int ret;
> >
> > - ret = intel_digital_connector_atomic_check(connector, state);
> > + ret = intel_digital_connector_atomic_check(connector, &state-
> > >base);
> > + if (ret)
> > + return ret;
> > +
> > + ret = intel_dp_mst_atomic_master_trans_check(intel_connector,
> > state);
> > if (ret)
> > return ret;
> >
> > @@ -182,12 +295,9 @@ intel_dp_mst_atomic_check(struct drm_connector
> > *connector,
> > * connector
> > */
> > if (new_crtc) {
> > - struct intel_atomic_state *intel_state =
> > - to_intel_atomic_state(state);
> > struct intel_crtc *intel_crtc =
> > to_intel_crtc(new_crtc);
> > struct intel_crtc_state *crtc_state =
> > - intel_atomic_get_new_crtc_state(intel_state,
> > - intel_crtc);
> > + intel_atomic_get_new_crtc_state(state,
> > intel_crtc);
> >
> > if (!crtc_state ||
> > !drm_atomic_crtc_needs_modeset(&crtc_state->uapi)
> > ||
> > @@ -196,7 +306,7 @@ intel_dp_mst_atomic_check(struct drm_connector
> > *connector,
> > }
> >
> > mgr = &enc_to_mst(old_conn_state->best_encoder)->primary-
> > >dp.mst_mgr;
> > - ret = drm_dp_atomic_release_vcpi_slots(state, mgr,
> > + ret = drm_dp_atomic_release_vcpi_slots(&state->base, mgr,
> > intel_connector->port);
> >
> > return ret;
> > @@ -240,6 +350,8 @@ static void intel_mst_post_disable_dp(struct
> > intel_encoder *encoder,
> >
> > intel_dp->active_mst_links--;
> > last_mst_stream = intel_dp->active_mst_links == 0;
> > + WARN_ON(INTEL_GEN(dev_priv) >= 12 && last_mst_stream &&
> > + !intel_dp_mst_is_master_trans(old_crtc_state));
> >
> > intel_crtc_vblank_off(old_crtc_state);
> >
> > @@ -317,6 +429,8 @@ static void intel_mst_pre_enable_dp(struct
> > intel_encoder *encoder,
> > connector->encoder = encoder;
> > intel_mst->connector = connector;
> > first_mst_stream = intel_dp->active_mst_links == 0;
> > + WARN_ON(INTEL_GEN(dev_priv) >= 12 && first_mst_stream &&
> > + !intel_dp_mst_is_master_trans(pipe_config));
> >
> > DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
> >
> > @@ -722,3 +836,14 @@ intel_dp_mst_encoder_cleanup(struct
> > intel_digital_port *intel_dig_port)
> > drm_dp_mst_topology_mgr_destroy(&intel_dp->mst_mgr);
> > /* encoders will get killed by normal cleanup */
> > }
> > +
> > +bool intel_dp_mst_is_master_trans(const struct intel_crtc_state
> > *crtc_state)
> > +{
> > + return crtc_state->mst_master_transcoder == crtc_state-
> > >cpu_transcoder;
> > +}
> > +
> > +bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state
> > *crtc_state)
> > +{
> > + return crtc_state->mst_master_transcoder != INVALID_TRANSCODER
> > &&
> > + crtc_state->mst_master_transcoder != crtc_state-
> > >cpu_transcoder;
> > +}
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > index f660ad80db04..854724f68f09 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> > @@ -6,10 +6,15 @@
> > #ifndef __INTEL_DP_MST_H__
> > #define __INTEL_DP_MST_H__
> >
> > +#include <linux/types.h>
> > +
> > struct intel_digital_port;
> > +struct intel_crtc_state;
> >
> > int intel_dp_mst_encoder_init(struct intel_digital_port
> > *intel_dig_port, int conn_id);
> > void intel_dp_mst_encoder_cleanup(struct intel_digital_port
> > *intel_dig_port);
> > int intel_dp_mst_encoder_active_links(struct intel_digital_port
> > *intel_dig_port);
> > +bool intel_dp_mst_is_master_trans(const struct intel_crtc_state
> > *crtc_state);
> > +bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state
> > *crtc_state);
> >
> > #endif /* __INTEL_DP_MST_H__ */
> > --
> > 2.24.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
