On Fri, Aug 23, 2019 at 03:25:18PM +0300, Ville Syrjälä wrote:
> On Fri, Aug 23, 2019 at 01:20:46AM -0700, Lucas De Marchi wrote:
> > Gen 12 onwards moves the DP_TP_* registers to be transcoder-based rather
> > than port-based. This add the new register address and changes the
> > functions that are used with DDI on gen 12 to use the new registers. On
> > MST the master transcoder is the one to be used.
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_ddi.c | 42 ++++++++----
> > .../drm/i915/display/intel_display_types.h | 1 +
> > drivers/gpu/drm/i915/display/intel_dp.c | 66 +++++++++++++++++--
> > drivers/gpu/drm/i915/display/intel_dp.h | 9 +++
> > drivers/gpu/drm/i915/display/intel_dp_mst.c | 20 ++++--
> > drivers/gpu/drm/i915/i915_reg.h | 4 ++
> > 6 files changed, 119 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index f4cb6bd74421..3eb73dbaf9fd 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -3136,17 +3136,22 @@ static void intel_ddi_enable_fec(struct intel_encoder *encoder,
> > const struct intel_crtc_state *crtc_state)
> > {
> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > + enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > enum port port = encoder->port;
> > + i915_reg_t ctl, status;
> > u32 val;
> >
> > if (!crtc_state->fec_enable)
> > return;
> >
> > - val = I915_READ(DP_TP_CTL(port));
> > + ctl = intel_dp_tp_ctl_reg(dev_priv, cpu_transcoder, port);
> > + status = intel_dp_tp_status_reg(dev_priv, cpu_transcoder, port);
> > +
> > + val = I915_READ(ctl);
> > val |= DP_TP_CTL_FEC_ENABLE;
> > - I915_WRITE(DP_TP_CTL(port), val);
> > + I915_WRITE(ctl, val);
> >
> > - if (intel_de_wait_for_set(dev_priv, DP_TP_STATUS(port),
> > + if (intel_de_wait_for_set(dev_priv, status,
> > DP_TP_STATUS_FEC_ENABLE_LIVE, 1))
> > DRM_ERROR("Timed out waiting for FEC Enable Status\n");
> > }
> > @@ -3155,16 +3160,19 @@ static void intel_ddi_disable_fec_state(struct intel_encoder *encoder,
> > const struct intel_crtc_state *crtc_state)
> > {
> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > + enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > enum port port = encoder->port;
> > + i915_reg_t ctl;
> > u32 val;
> >
> > if (!crtc_state->fec_enable)
> > return;
> >
> > - val = I915_READ(DP_TP_CTL(port));
> > + ctl = intel_dp_tp_ctl_reg(dev_priv, cpu_transcoder, port);
> > + val = I915_READ(ctl);
> > val &= ~DP_TP_CTL_FEC_ENABLE;
> > - I915_WRITE(DP_TP_CTL(port), val);
> > - POSTING_READ(DP_TP_CTL(port));
> > + I915_WRITE(ctl, val);
> > + POSTING_READ(ctl);
> > }
> >
> > static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> > @@ -3326,7 +3334,9 @@ static void intel_disable_ddi_buf(struct intel_encoder *encoder,
> > const struct intel_crtc_state *crtc_state)
> > {
> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > + enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > enum port port = encoder->port;
> > + i915_reg_t ctl;
> > bool wait = false;
> > u32 val;
> >
> > @@ -3337,10 +3347,11 @@ static void intel_disable_ddi_buf(struct intel_encoder *encoder,
> > wait = true;
> > }
> >
> > - val = I915_READ(DP_TP_CTL(port));
> > + ctl = intel_dp_tp_ctl_reg(dev_priv, cpu_transcoder, port);
> > + val = I915_READ(ctl);
> > val &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
> > val |= DP_TP_CTL_LINK_TRAIN_PAT1;
> > - I915_WRITE(DP_TP_CTL(port), val);
> > + I915_WRITE(ctl, val);
> >
> > /* Disable FEC in DP Sink */
> > intel_ddi_disable_fec_state(encoder, crtc_state);
> > @@ -3765,10 +3776,13 @@ static void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
> > struct drm_i915_private *dev_priv =
> > to_i915(intel_dig_port->base.base.dev);
> > enum port port = intel_dig_port->base.port;
> > + i915_reg_t ctl;
> > u32 val;
> > bool wait = false;
> > + enum transcoder cpu_transcoder = intel_dp_get_transcoder(intel_dp);
> >
> > - if (I915_READ(DP_TP_CTL(port)) & DP_TP_CTL_ENABLE) {
> > + ctl = intel_dp_tp_ctl_reg(dev_priv, cpu_transcoder, port);
> > + if (I915_READ(ctl) & DP_TP_CTL_ENABLE) {
> > val = I915_READ(DDI_BUF_CTL(port));
> > if (val & DDI_BUF_CTL_ENABLE) {
> > val &= ~DDI_BUF_CTL_ENABLE;
> > @@ -3776,11 +3790,11 @@ static void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
> > wait = true;
> > }
> >
> > - val = I915_READ(DP_TP_CTL(port));
> > + val = I915_READ(ctl);
> > val &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
> > val |= DP_TP_CTL_LINK_TRAIN_PAT1;
> > - I915_WRITE(DP_TP_CTL(port), val);
> > - POSTING_READ(DP_TP_CTL(port));
> > + I915_WRITE(ctl, val);
> > + POSTING_READ(ctl);
> >
> > if (wait)
> > intel_wait_ddi_buf_idle(dev_priv, port);
> > @@ -3795,8 +3809,8 @@ static void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
> > if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
> > val |= DP_TP_CTL_ENHANCED_FRAME_ENABLE;
> > }
> > - I915_WRITE(DP_TP_CTL(port), val);
> > - POSTING_READ(DP_TP_CTL(port));
> > + I915_WRITE(ctl, val);
> > + POSTING_READ(ctl);
> >
> > intel_dp->DP |= DDI_BUF_CTL_ENABLE;
> > I915_WRITE(DDI_BUF_CTL(port), intel_dp->DP);
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index d84a66459daf..a87fa06994ad 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1214,6 +1214,7 @@ struct intel_dp {
> > bool can_mst; /* this port supports mst */
> > bool is_mst;
> > int active_mst_links;
> > + enum transcoder mst_master_trans; /* Only valid on TGL+ */
Hmm. So maybe just s/mst_master_trans/dp_tp_transcoder/ etc. and set
this unconditionally in eg. intel_dp_set_link_params().
> > /* connector directly attached - won't be use for modeset in mst world */
> > struct intel_connector *attached_connector;
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 23908da1cd5d..943392faaea2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -3209,6 +3209,51 @@ static void chv_post_disable_dp(struct intel_encoder *encoder,
> > vlv_dpio_put(dev_priv);
> > }
> >
> > +i915_reg_t intel_dp_tp_ctl_reg(const struct drm_i915_private *dev_priv,
> > + enum transcoder cpu_transcoder,
> > + enum port port)
> > +{
> > + if (INTEL_GEN(dev_priv) >= 12) {
> > + WARN_ON(cpu_transcoder == TRANSCODER_INVALID);
> > + return TGL_DP_TP_CTL(cpu_transcoder);
> > + } else {
> > + return DP_TP_CTL(port);
> > + }
> > +}
> > +
> > +i915_reg_t intel_dp_tp_status_reg(const struct drm_i915_private *dev_priv,
> > + enum transcoder cpu_transcoder,
> > + enum port port)
> > +{
> > + if (INTEL_GEN(dev_priv) >= 12) {
> > + WARN_ON(cpu_transcoder == TRANSCODER_INVALID);
> > + return TGL_DP_TP_STATUS(cpu_transcoder);
> > + } else {
> > + return DP_TP_STATUS(port);
> > + }
> > +}
> > +
> > +/*
> > + * Return the transcoder that this intel_dp port is driven.
> > + * When in MST mode it will return the master transcoder of the MST so do not
> > + * use it when reading or writing registers in the slave transcoders.
> > + */
> > +enum transcoder intel_dp_get_transcoder(struct intel_dp *intel_dp)
> > +{
> > + struct intel_connector *connector;
> > + struct drm_connector_state *conn_state;
> > + struct intel_crtc_state *crtc_state;
> > +
> > + if (intel_dp->is_mst)
> > + return intel_dp->mst_master_trans;
> > +
> > + connector = intel_dp->attached_connector;
> > + conn_state = connector->base.state;
> > + crtc_state = to_intel_crtc_state(conn_state->crtc->state);
>
> That's illegal. Ideally we'd plumb the crtc state into the link training
> stuff, but IIRC last time I tried that we still had at least one case
> where that wasn't trivial. I can't remember right now where that case
> was though.
>
> I guess the alternative is to continue duplicating things in intel_dp.
>
> > +
> > + return crtc_state->cpu_transcoder;
> > +}
> > +
> > static void
> > _intel_dp_set_link_train(struct intel_dp *intel_dp,
> > u32 *DP,
> > @@ -3224,8 +3269,13 @@ _intel_dp_set_link_train(struct intel_dp *intel_dp,
> > dp_train_pat & train_pat_mask);
> >
> > if (HAS_DDI(dev_priv)) {
> > - u32 temp = I915_READ(DP_TP_CTL(port));
> > + enum transcoder cpu_transcoder;
> > + i915_reg_t ctl;
> > + u32 temp;
> >
> > + cpu_transcoder = intel_dp_get_transcoder(intel_dp);
> > + ctl = intel_dp_tp_ctl_reg(dev_priv, cpu_transcoder, port);
> > + temp = I915_READ(ctl);
> > if (dp_train_pat & DP_LINK_SCRAMBLING_DISABLE)
> > temp |= DP_TP_CTL_SCRAMBLE_DISABLE;
> > else
> > @@ -3250,7 +3300,7 @@ _intel_dp_set_link_train(struct intel_dp *intel_dp,
> > temp |= DP_TP_CTL_LINK_TRAIN_PAT4;
> > break;
> > }
> > - I915_WRITE(DP_TP_CTL(port), temp);
> > + I915_WRITE(ctl, temp);
> >
> > } else if ((IS_IVYBRIDGE(dev_priv) && port == PORT_A) ||
> > (HAS_PCH_CPT(dev_priv) && port != PORT_A)) {
> > @@ -3943,15 +3993,21 @@ void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
> > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > enum port port = intel_dig_port->base.port;
> > + enum transcoder cpu_transcoder;
> > + i915_reg_t ctl, status;
> > u32 val;
> >
> > if (!HAS_DDI(dev_priv))
> > return;
> >
> > - val = I915_READ(DP_TP_CTL(port));
> > + cpu_transcoder = intel_dp_get_transcoder(intel_dp);
> > + ctl = intel_dp_tp_ctl_reg(dev_priv, cpu_transcoder, port);
> > + status = intel_dp_tp_status_reg(dev_priv, cpu_transcoder, port);
> > +
> > + val = I915_READ(ctl);
> > val &= ~DP_TP_CTL_LINK_TRAIN_MASK;
> > val |= DP_TP_CTL_LINK_TRAIN_IDLE;
> > - I915_WRITE(DP_TP_CTL(port), val);
> > + I915_WRITE(ctl, val);
> >
> > /*
> > * On PORT_A we can have only eDP in SST mode. There the only reason
> > @@ -3963,7 +4019,7 @@ void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
> > if (port == PORT_A)
> > return;
> >
> > - if (intel_de_wait_for_set(dev_priv, DP_TP_STATUS(port),
> > + if (intel_de_wait_for_set(dev_priv, status,
> > DP_TP_STATUS_IDLE_DONE, 1))
> > DRM_ERROR("Timed out waiting for DP idle patterns\n");
> > }
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> > index 657bbb1f5ed0..107129b5d9a3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> > @@ -11,6 +11,7 @@
> > #include <drm/i915_drm.h>
> >
> > #include "i915_reg.h"
> > +#include "intel_display.h"
> >
> > enum pipe;
> > struct drm_connector_state;
> > @@ -113,6 +114,14 @@ int intel_dp_link_required(int pixel_clock, int bpp);
> > int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
> > bool intel_digital_port_connected(struct intel_encoder *encoder);
> >
> > +i915_reg_t intel_dp_tp_ctl_reg(const struct drm_i915_private *dev_priv,
> > + enum transcoder cpu_transcoder,
> > + enum port port);
> > +i915_reg_t intel_dp_tp_status_reg(const struct drm_i915_private *dev_priv,
> > + enum transcoder cpu_transcoder,
> > + enum port port);
> > +enum transcoder intel_dp_get_transcoder(struct intel_dp *intel_dp);
> > +
> > static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
> > {
> > return ~((1 << lane_count) - 1) & 0xf;
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index 8002dca9b734..630db2fadaca 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -354,9 +354,11 @@ static void intel_mst_pre_pll_enable_dp(struct intel_encoder *encoder,
> > struct intel_digital_port *intel_dig_port = intel_mst->primary;
> > struct intel_dp *intel_dp = &intel_dig_port->dp;
> >
> > - if (intel_dp->active_mst_links == 0)
> > + if (intel_dp->active_mst_links == 0) {
> > + intel_dp->mst_master_trans = pipe_config->mst_master_trans;
The current transcoder should be the master here, no?
Maybe sprinkle some
assert_is_master_transcoder(crtc_state)
{
WARN(crtc_has_type(MST) &&
crtc_state->cpu_trans != crtc_state->master_trans);
}
stuff into the ddi code?
> > intel_dig_port->base.pre_pll_enable(&intel_dig_port->base,
> > pipe_config, NULL);
> > + }
> > }
> >
> > static void intel_mst_post_pll_disable_dp(struct intel_encoder *encoder,
> > @@ -384,6 +386,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
> > enum port port = intel_dig_port->base.port;
> > struct intel_connector *connector =
> > to_intel_connector(conn_state->connector);
> > + i915_reg_t status;
> > int ret;
> > u32 temp;
> >
> > @@ -412,8 +415,12 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
> > DRM_ERROR("failed to allocate vcpi\n");
> >
> > intel_dp->active_mst_links++;
> > - temp = I915_READ(DP_TP_STATUS(port));
> > - I915_WRITE(DP_TP_STATUS(port), temp);
> > +
> > + status = intel_dp_tp_status_reg(dev_priv,
> > + pipe_config->mst_master_trans,
> > + port);
> > + temp = I915_READ(status);
> > + I915_WRITE(status, temp);
> >
> > ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr);
> >
> > @@ -429,10 +436,15 @@ static void intel_mst_enable_dp(struct intel_encoder *encoder,
> > struct intel_dp *intel_dp = &intel_dig_port->dp;
> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > enum port port = intel_dig_port->base.port;
> > + i915_reg_t status;
> >
> > DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
> >
> > - if (intel_de_wait_for_set(dev_priv, DP_TP_STATUS(port),
> > + status = intel_dp_tp_status_reg(dev_priv,
> > + pipe_config->mst_master_trans,
> > + port);
> > +
> > + if (intel_de_wait_for_set(dev_priv, status,
> > DP_TP_STATUS_ACT_SENT, 1))
> > DRM_ERROR("Timed out waiting for ACT sent\n");
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index bff9ee191832..fffaed0ca3cd 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -9454,7 +9454,9 @@ enum skl_power_gate {
> > /* DisplayPort Transport Control */
> > #define _DP_TP_CTL_A 0x64040
> > #define _DP_TP_CTL_B 0x64140
> > +#define _TGL_DP_TP_CTL_A 0x60540
> > #define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A, _DP_TP_CTL_B)
> > +#define TGL_DP_TP_CTL(tran) _MMIO_TRANS2((tran), _TGL_DP_TP_CTL_A)
> > #define DP_TP_CTL_ENABLE (1 << 31)
> > #define DP_TP_CTL_FEC_ENABLE (1 << 30)
> > #define DP_TP_CTL_MODE_SST (0 << 27)
> > @@ -9474,7 +9476,9 @@ enum skl_power_gate {
> > /* DisplayPort Transport Status */
> > #define _DP_TP_STATUS_A 0x64044
> > #define _DP_TP_STATUS_B 0x64144
> > +#define _TGL_DP_TP_STATUS_A 0x60544
> > #define DP_TP_STATUS(port) _MMIO_PORT(port, _DP_TP_STATUS_A, _DP_TP_STATUS_B)
> > +#define TGL_DP_TP_STATUS(tran) _MMIO_TRANS2((tran), _TGL_DP_TP_STATUS_A)
> > #define DP_TP_STATUS_FEC_ENABLE_LIVE (1 << 28)
> > #define DP_TP_STATUS_IDLE_DONE (1 << 25)
> > #define DP_TP_STATUS_ACT_SENT (1 << 24)
> > --
> > 2.23.0
>
> --
> Ville Syrjälä
> Intel
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
