On Mon, 2019-12-09 at 21:43 +0200, Ville Syrjälä wrote:
> On Mon, Dec 09, 2019 at 06:45:43PM +0000, Souza, Jose wrote:
> > On Mon, 2019-12-09 at 18:40 +0200, Ville Syrjälä wrote:
> > > On Fri, Dec 06, 2019 at 05:18:29PM -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 first
> > > > 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
> > > > scenario will be handled in the next patch.
> > > >
> > > > 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 | 14 +-
> > > > drivers/gpu/drm/i915/display/intel_display.c | 15 +-
> > > > .../drm/i915/display/intel_display_types.h | 3 +
> > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 163
> > > > ++++++++++++++++++
> > > > drivers/gpu/drm/i915/display/intel_dp_mst.h | 5 +
> > > > 5 files changed, 196 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > index 3cacb1e279c1..be5bc506d4d3 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > @@ -1903,8 +1903,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);
> > >
> > > I'd drop the WARN_ON(). If we keep adding these for every little
> > > thing
> > > we'll drown in them.
> > >
> > > > + temp |=
> > > > TRANS_DDI_MST_TRANSPORT_SELECT(master);
> > > > + }
> > > > } else {
> > > > temp |= TRANS_DDI_MODE_SELECT_DP_SST;
> > > > temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> > > > @@ -4372,6 +4377,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 821ba8053f9d..f89494c849ce 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"
> > > > @@ -12542,6 +12543,11 @@ 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);
> > > >
> > > > + if (INTEL_GEN(dev_priv) >= 12 &&
> > > > + intel_crtc_has_type(pipe_config,
> > > > INTEL_OUTPUT_DP_MST))
> > >
> > > Could probably print this unconditionally to keep the code less
> > > messy.
> >
> > I'm not setting mst_master_transcoder = INVALID_TRANSCODER in the
> > other
> > code paths, so it would print transcoder A for HDMI, DP SST and to
> > DP
> > MST in gen < 12.
> > That is fine? Should I add mst_master_transcoder =
> > INVALID_TRANSCODER
> > to all those code paths? For me the best option is keep this
> > checks.
>
> I think setting to invalid would be nice.
>
> With https://patchwork.freedesktop.org/series/69129/
> we could even do it in a nice central place just the once.
Awesome, just rvb it.
It still apply without conflicts and compile, moving this MST patches
on top.
>
> >
> > > > + DRM_DEBUG_KMS("MST master transcoder: %s\n",
> > > > + transcoder_name(pipe_config-
> > > > > mst_master_transcoder));
> > > > +
> > > > dump_planes:
> > > > if (!state)
> > > > return;
> > > > @@ -13318,6 +13324,10 @@ intel_pipe_config_compare(const struct
> > > > intel_crtc_state *current_config,
> > > > PIPE_CONF_CHECK_I(sync_mode_slaves_mask);
> > > > PIPE_CONF_CHECK_I(master_transcoder);
> > > >
> > > > + if (INTEL_GEN(dev_priv) >= 12 &&
> > > > + intel_crtc_has_type(pipe_config,
> > > > INTEL_OUTPUT_DP_MST))
> > >
> > > These checks are definitely pointless.
> >
> > Similar with the above, it would not cause mismatch because for non
> > gen
> > 12 DP MST it would compare trans A with trans A, will change that
> > depending on your answer to the above.
> >
> >
> > > > + PIPE_CONF_CHECK_I(mst_master_transcoder);
> > > > +
> > > > #undef PIPE_CONF_CHECK_X
> > > > #undef PIPE_CONF_CHECK_I
> > > > #undef PIPE_CONF_CHECK_BOOL
> > > > @@ -14406,7 +14416,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))
> > > > @@ -14420,7 +14430,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 926e49f449a6..49f1518e3ab9 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > @@ -87,6 +87,49 @@ static int
> > > > intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
> > > > return 0;
> > > > }
> > > >
> > > > +/*
> > > > + * Iterate over all the CRTCs and set mst_master_transcoder to
> > > > the
> > > > first active
> > > > + * transcoder that shares the same MST connector.
> > > > + *
> > > > + * TODO: maybe this could be optimized to keep the old master
> > > > transcoder
> > > > + */
> > > > +static int
> > > > +intel_dp_mst_master_trans_compute(struct intel_encoder
> > > > *encoder,
> > > > + struct intel_crtc_state
> > > > *pipe_config,
> > >
> > > s/pipe_config/crtc_state/
> >
> > Okay
> >
> > > > + struct drm_connector_state
> > > > *conn_state)
> > > > +{
> > > > + struct intel_atomic_state *state =
> > > > to_intel_atomic_state(pipe_config->uapi.state);
> > > > + struct intel_connector *conn =
> > > > to_intel_connector(conn_state-
> > > > > connector);
> > >
> > > ocd: s/conn/connector/ all over.
> >
> > Okay
> >
> > > > + struct drm_i915_private *dev_priv = to_i915(encoder-
> > > > >base.dev);
> > > > + struct intel_digital_connector_state *iter_conn_state;
> > > > + struct intel_connector *iter_conn;
> > > > + int i;
> > > > +
> > > > + if (INTEL_GEN(dev_priv) < 12)
> > > > + return 0;
> > > > +
> > > > + for_each_new_intel_connector_in_state(state, iter_conn,
> > > > + iter_conn_state,
> > > > i) {
> > > > + struct intel_crtc_state *iter_crtc_state;
> > > > + struct intel_crtc *iter_crtc;
> > > > +
> > > > + if (conn->mst_port != iter_conn->mst_port ||
> > > > + !iter_conn_state->base.crtc)
> > > > + continue;
> > > > +
> > > > + iter_crtc = to_intel_crtc(iter_conn_state-
> > > > >base.crtc);
> > > > + iter_crtc_state =
> > > > intel_atomic_get_new_crtc_state(state,
> > > > +
> > > > iter_
> > > > crtc);
> > > > + if (!iter_crtc_state->uapi.active)
> > > > + continue;
> > > > +
> > > > + pipe_config->mst_master_transcoder =
> > > > iter_crtc_state-
> > > > > cpu_transcoder;
> > > > + break;
> > >
> > > So we're going to pick this based on the connector order. How
> > > does
> > > that
> > > fit in with the port sync, or did we not have port sync for MST
> > > yet?
> > >
> > > To keep everything consistent and easy my idea was to pick the
> > > first
> > > pipe as the master for both MST and port sync.
> > >
> > > Although I can easily imagine topologies where even that would
> > > land us in some interesting mess. For example:
> > > pipe A -> MST port B -> ... -> whatever : MST master
> > > pipe B -> MST port B -> ... -> tile 0 : port sync master / MST
> > > slave
> > > pipe C -> MST port C -> ... -> tile 1 : port sync slave / MST
> > > master
> > > pipe D -> MST port C -> ... -> whatever : MST slave
> > >
> > > pipe A -> MST port B -> ... -> whatever : MST master
> > > pipe B -> MST port B -> ... -> tile 0 : port sync master / MST
> > > slave
> > > pipe C -> MST port C -> ... -> whatever : MST master
> > > pipe D -> MST port C -> ... -> tile 1 : port sync slave / MST
> > > slave
> > >
> > > No idea if the hw can even do something like that. Needs more
> > > thought
> > > if/when we enable port sync with MST (assuming we're not already
> > > doing
> > > that).
> >
> > According to BSpec is possible to have port sync with MST but it is
> > not
> > implemented. But sure it can be easily fixed, will loop throgh all
> > the
> > connectors updating mst_master_transcoder with the smallest pipe.
>
> I don't think it's quite that easy since the ordering constraints of
> MST vs. port sync might conflict as in my examples above. But let's
> just ignore that particular can of worms for now.
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
