[Intel-gfx] [PATCH 3/7] drm/i915/tgl: Select master trasconder for MST stream
Ville Syrjälä
ville.syrjala at linux.intel.com
Wed Nov 27 19:59:39 UTC 2019
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 at linux.intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza at 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
--
Ville Syrjälä
Intel
More information about the Intel-gfx
mailing list