On Fri, Dec 20, 2019 at 07:03:16PM +0200, Ville Syrjälä wrote:
> On Fri, Dec 20, 2019 at 04:53:22PM +0000, Souza, Jose wrote:
> > On Fri, 2019-12-20 at 17:47 +0200, Ville Syrjälä wrote:
> > > On Fri, Dec 20, 2019 at 07:29:50AM -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
> > > >
> > > > v5:
> > > > - Improved the parameters of intel_dp_mst_master_trans_compute() to
> > > > simply code
> > > > - Added call drm_atomic_add_affected_planes() in
> > > > intel_dp_mst_atomic_master_trans_check() as helper could not do it
> > > > for us
> > > > - Removed "if (ret)" left over from v3 changes
> > > >
> > > > 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 | 12 +-
> > > > .../drm/i915/display/intel_display_types.h | 3 +
> > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 140
> > > > ++++++++++++++++--
> > > > drivers/gpu/drm/i915/display/intel_dp_mst.h | 5 +
> > > > 7 files changed, 179 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 c9ba7d7f3787..c3ac950e79a8 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);
> > > > @@ -4405,6 +4410,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 fc77829ea958..eb97ad562c96 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"
> > > > @@ -11627,6 +11628,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;
> > > > }
> > > >
> > > > static struct intel_crtc_state *intel_crtc_state_alloc(struct
> > > > intel_crtc *crtc)
> > > > @@ -12484,6 +12486,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;
> > > > @@ -13264,6 +13269,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
> > > > @@ -14348,7 +14355,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))
> > > > @@ -14362,7 +14369,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..a6237da9ac52 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > @@ -87,10 +87,53 @@ static int
> > > > intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
> > > > return 0;
> > > > }
> > > >
> > > > +/*
> > > > + * Iterate over all connectors and return the smallest transcoder
> > > > in the MST
> > > > + * stream
> > > > + */
> > > > +static enum transcoder
> > > > +intel_dp_mst_master_trans_compute(struct intel_atomic_state
> > > > *state,
> > > > + struct intel_dp *mst_port)
> > > > +{
> > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > + struct intel_digital_connector_state *conn_state;
> > > > + struct intel_connector *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, connector,
> > > > conn_state, i) {
> > > > + struct intel_crtc_state *crtc_state;
> > > > + struct intel_crtc *crtc;
> > > > +
> > > > + if (connector->mst_port != mst_port || !conn_state-
> > > > >base.crtc)
> > > > + continue;
> > > > +
> > > > + crtc = to_intel_crtc(conn_state->base.crtc);
> > > > + crtc_state = intel_atomic_get_new_crtc_state(state,
> > > > crtc);
> > > > + if (!crtc_state->uapi.active)
> > > > + continue;
> > > > +
> > > > + /*
> > > > + * Using crtc->pipe because crtc_state->cpu_transcoder
> > > > is
> > > > + * computed, so others CRTCs could have non-computed
> > > > + * cpu_transcoder
> > > > + */
> > > > + if (crtc->pipe < ret)
> > > > + ret = crtc->pipe;
> > > > + }
> > >
> > > Still has the problem that ret==I915_MAX_PIPES if no active crtcs in
> > > the
> > > state.
> > >
> > > I guess a simple
> > > if (ret == MAX_PIPES)
> > > return INVALID;
> > > should do here. Or something similar.
> >
> > It would never happen, if it is computing state for this MST connector
> > it is active, so it can be master. But okay adding it anyways.
>
> active != enabled. Pretty sure it will happen.
BTW don't we have a testcase that just enable everything and then
DPMS off everything? Someone should write one if there isn't one.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
