On Wed, 2016-09-07 at 09:40 -0700, Jim Bride wrote:
> On Wed, Sep 07, 2016 at 01:53:31PM +0300, Mika Kahola wrote:
> >
> > On Tue, 2016-09-06 at 17:13 -0700, Manasi Navare wrote:
> > >
> > > From: Jim Bride <jim.bride@xxxxxxxxxxxxxxx>
> > >
> > > Add upfront link training to intel_dp_mst_mode_valid() so that we
> > > know
> > > topology constraints before we validate the legality of modes to
> > > be
> > > checked.
> > > Call the function that loops through the link rates and lane
> > > counts
> > > starting from highest supported link rate and lane count for
> > > training
> > > the link in compliance with DP spec
> > >
> > > v2:
> > > * Rebased on new revision of link training patch (Manasi Navare)
> > >
> > > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > > Signed-off-by: Jim Bride <jim.bride@xxxxxxxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/intel_dp.c | 9 ++---
> > > drivers/gpu/drm/i915/intel_dp_mst.c | 74
> > > +++++++++++++++++++++++++++----------
> > > drivers/gpu/drm/i915/intel_drv.h | 3 ++
> > > 3 files changed, 61 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 7794180..0c7674f 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -131,7 +131,7 @@ static void vlv_steal_power_sequencer(struct
> > > drm_device *dev,
> > > enum pipe pipe);
> > > static void intel_dp_unset_edid(struct intel_dp *intel_dp);
> > >
> > > -static int
> > > +int
> > > intel_dp_max_link_bw(struct intel_dp *intel_dp)
> > > {
> > > int max_link_bw = intel_dp->dpcd[DP_MAX_LINK_RATE];
> > > @@ -150,7 +150,7 @@ intel_dp_max_link_bw(struct
> > > intel_dp *intel_dp)
> > > return max_link_bw;
> > > }
> > >
> > > -static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp)
> > > +u8 intel_dp_max_lane_count(struct intel_dp *intel_dp)
> > > {
> > > struct intel_digital_port *intel_dig_port =
> > > dp_to_dig_port(intel_dp);
> > > u8 temp, source_max, sink_max;
> > > @@ -312,8 +312,7 @@ static int intersect_rates(const int
> > > *source_rates, int source_len,
> > > return k;
> > > }
> > >
> > > -static int intel_dp_common_rates(struct intel_dp *intel_dp,
> > > - int *common_rates)
> > > +int intel_dp_common_rates(struct intel_dp *intel_dp, int
> > > *common_rates)
> > > {
> > > const int *source_rates, *sink_rates;
> > > int source_len, sink_len;
> > > @@ -336,7 +335,7 @@ static int intel_dp_common_rates(struct
> > > intel_dp
> > > *intel_dp,
> > > common_rates);
> > > }
> > >
> > > -static bool intel_dp_upfront_link_train(struct intel_dp
> > > *intel_dp)
> > > +bool intel_dp_upfront_link_train(struct intel_dp *intel_dp)
> > > {
> > > struct intel_digital_port *intel_dig_port =
> > > dp_to_dig_port(intel_dp);
> > > struct intel_encoder *intel_encoder = &intel_dig_port-
> > > >base;
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > index 54a9d76..98d45a4 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > @@ -41,21 +41,30 @@ static bool
> > > intel_dp_mst_compute_config(struct
> > > intel_encoder *encoder,
> > > int bpp;
> > > int lane_count, slots;
> > > const struct drm_display_mode *adjusted_mode =
> > > &pipe_config-
> > > >
> > > > base.adjusted_mode;
> > > - int mst_pbn;
> > > + int mst_pbn, common_len;
> > > + int common_rates[DP_MAX_SUPPORTED_RATES] = {};
> > >
> > > pipe_config->dp_encoder_is_mst = true;
> > > pipe_config->has_pch_encoder = false;
> > > - bpp = 24;
> > > +
> > > /*
> > > - * for MST we always configure max link bw - the spec
> > > doesn't
> > > - * seem to suggest we should do otherwise.
> > > + * For MST we always configure for the maximum trainable
> > > link bw -
> > > + * the spec doesn't seem to suggest we should do
> > > otherwise. The
> > > + * calls to intel_dp_max_lane_count() and
> > > intel_dp_common_rates()
> > > + * both take successful upfront link training into
> > > account,
> > > and
> > > + * return the DisplayPort max supported values in the
> > > event
> > > that
> > > + * upfront link training was not done.
> > > */
> > > - lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
> > > + lane_count = intel_dp_max_lane_count(intel_dp);
> > >
> > > pipe_config->lane_count = lane_count;
> > >
> > > - pipe_config->pipe_bpp = 24;
> > > - pipe_config->port_clock =
> > > intel_dp_max_link_rate(intel_dp);
> > > + pipe_config->pipe_bpp = bpp = 24;
> > > + common_len = intel_dp_common_rates(intel_dp,
> > > common_rates);
> > > + pipe_config->port_clock = common_rates[common_len - 1];
> > > +
> > > + DRM_DEBUG_KMS("DP MST link configured for %d lanes @
> > > %d.\n",
> > > + pipe_config->lane_count, pipe_config-
> > > >
> > > > port_clock);
> > >
> > > state = pipe_config->base.state;
> > >
> > > @@ -137,6 +146,8 @@ static void intel_mst_pre_enable_dp(struct
> > > intel_encoder *encoder,
> > > enum port port = intel_dig_port->port;
> > > struct intel_connector *connector =
> > > to_intel_connector(conn_state->connector);
> > > + struct intel_shared_dpll *pll = pipe_config-
> > > >shared_dpll;
> > > + struct intel_shared_dpll_config tmp_pll_config;
> > > int ret;
> > > uint32_t temp;
> > > int slots;
> > > @@ -150,21 +161,23 @@ static void intel_mst_pre_enable_dp(struct
> > > intel_encoder *encoder,
> > > DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> > >
> > > if (intel_dp->active_mst_links == 0) {
> > > - intel_ddi_clk_select(&intel_dig_port->base,
> > > - pipe_config->shared_dpll);
> > > -
> > > - intel_prepare_dp_ddi_buffers(&intel_dig_port-
> > > >base);
> > > - intel_dp_set_link_params(intel_dp,
> > > - pipe_config-
> > > >port_clock,
> > > - pipe_config-
> > > >lane_count,
> > > - true);
> > > -
> > > - intel_ddi_init_dp_buf_reg(&intel_dig_port-
> > > >base);
> > >
> > > - intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > > + /* Disable the PLL since we need to acquire the
> > > PLL
> > > + * based on the link rate in the link training
> > > sequence
> > > + */
> > > + tmp_pll_config = pll->config;
> > > + pll->funcs.disable(dev_priv, pll);
> > > + pll->config.crtc_mask = 0;
> > > +
> > > + /* If Link Training fails, send a uevent to
> > > generate
> > > a
> > > + *hotplug
> > > + */
> > > + if (!(intel_ddi_link_train(intel_dp,
> > > pipe_config-
> > > >
> > > > port_clock,
> > > + pipe_config-
> > > >lane_count,
> > > true,
> > > + false)))
> > > + drm_kms_helper_hotplug_event(encoder-
> > > >
> > > > base.dev);
> > > + pll->config = tmp_pll_config;
> > >
> > > - intel_dp_start_link_train(intel_dp);
> > > - intel_dp_stop_link_train(intel_dp);
> > > }
> > >
> > > ret = drm_dp_mst_allocate_vcpi(&intel_dp->mst_mgr,
> > > @@ -336,6 +349,27 @@ intel_dp_mst_mode_valid(struct drm_connector
> > > *connector,
> > > struct drm_display_mode *mode)
> > > {
> > > int max_dotclk = to_i915(connector->dev)-
> > > >max_dotclk_freq;
> > > + struct intel_connector *intel_connector =
> > > to_intel_connector(connector);
> > > + struct intel_dp *intel_dp = intel_connector->mst_port;
> > > +
> > > + if (intel_dp->upfront_link_train && !intel_dp-
> > > >upfront_done)
> > > {
> > > + bool do_upfront_link_train;
> > > +
> > > + do_upfront_link_train = intel_dp-
> > > >
> > > > compliance_test_type !=
> > > + DP_TEST_LINK_TRAINING;
> > > + if (do_upfront_link_train) {
> > > + intel_dp->upfront_done =
> > > + intel_dp_upfront_link_train(inte
> > > l_dp
> > > );
> > > + if (intel_dp->upfront_done) {
> > > + DRM_DEBUG_KMS("MST upfront
> > > trained
> > > at "
> > > + "%d lanes @ %d.",
> > > + intel_dp-
> > > >
> > > > max_lanes_upfront,
> > > + intel_dp-
> > > >
> > > > max_link_rate_upfront);
> > > + } else
> > > + DRM_DEBUG_KMS("MST upfront link
> > > training "
> > > + "failed.");
> > Link training has failed and we have a blank screen. Should we
> > throw an
> > error here? Maybe
> >
> > return MODE_ERROR;
> This is only for upfront link training, which is helpful but not
> absolutely
> required. If this were a link training associated with a mode set
> I'd
> agree. As it is, I think a debug message is fine for this case.
>
> Jim
Ok, so at this point we are not associated with any mode so I think
debug message is just fine here.
>
>
> >
> >
> > >
> > > + }
> > > + }
> > >
> > > /* TODO - validate mode against available PBN for link
> > > */
> > > if (mode->clock < 10000)
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index a2bbf68..34af3e8 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1416,6 +1416,7 @@ void intel_edp_panel_off(struct intel_dp
> > > *intel_dp);
> > > void intel_dp_add_properties(struct intel_dp *intel_dp, struct
> > > drm_connector *connector);
> > > void intel_dp_mst_suspend(struct drm_device *dev);
> > > void intel_dp_mst_resume(struct drm_device *dev);
> > > +u8 intel_dp_max_lane_count(struct intel_dp *intel_dp);
> > > int intel_dp_max_link_rate(struct intel_dp *intel_dp);
> > > int intel_dp_link_rate_index(struct intel_dp *intel_dp, int
> > > *common_rates,
> > > int link_rate);
> > > @@ -1448,6 +1449,8 @@ intel_dp_pre_emphasis_max(struct intel_dp
> > > *intel_dp, uint8_t voltage_swing);
> > > void intel_dp_compute_rate(struct intel_dp *intel_dp, int
> > > port_clock,
> > > uint8_t *link_bw, uint8_t
> > > *rate_select);
> > > bool intel_dp_source_supports_hbr2(struct intel_dp *intel_dp);
> > > +int intel_dp_common_rates(struct intel_dp *intel_dp, int
> > > *common_rates);
> > > +bool intel_dp_upfront_link_train(struct intel_dp *intel_dp);
> > > bool
> > > intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t
> > > link_status[DP_LINK_STATUS_SIZE]);
> > >
--
Mika Kahola - Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
