[Intel-gfx] [PATCHv2 4/4] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT

[R, Durgadoss]

>-----Original Message-----
>From: Ander Conselvan De Oliveira [mailto:conselvan2 at gmail.com]
>Sent: Friday, February 12, 2016 10:13 PM
>To: R, Durgadoss <durgadoss.r at intel.com>; intel-gfx at lists.freedesktop.org
>Subject: Re: [Intel-gfx] [PATCHv2 4/4] drm/i915/dp: Enable Upfront link training for typeC DP support on
>BXT
>
>On Mon, 2016-02-01 at 19:27 +0530, Durgadoss R wrote:
>> To support USB type C alternate DP mode, the display driver needs to
>> know the number of lanes required by the DP panel as well as number
>> of lanes that can be supported by the type-C cable. Sometimes, the
>> type-C cable may limit the bandwidth even if Panel can support
>> more lanes. To address these scenarios, the display driver will
>> start link training with max lanes, and if that fails, the driver
>> falls back to x2 lanes; and repeats this procedure for all
>> bandwidth/lane configurations.
>>
>> * Since link training is done before modeset only the port
>>   (and not pipe/planes) and its associated PLLs are enabled.
>> * On DP hotplug: Directly start link training on the crtc
>>   associated with the DP encoder.
>> * On Connected boot scenarios: When booted with an LFP and a DP,
>>   typically, BIOS brings up DP. In these cases, we disable the
>>   crtc, do upfront link training and then re-enable it back.
>> * All local changes made for upfront link training are reset
>>   to their previous values once it is done; so that the
>>   subsequent modeset is not aware of these changes.
>>
>
>Please always include a changelog when sending a new version of a patch.

Sure, will add it in the next version.

>
>> Signed-off-by: Durgadoss R <durgadoss.r at intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c     | 102 +++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_display.c |  38 ++++++++++-
>>  drivers/gpu/drm/i915/intel_dp.c      | 122 ++++++++++++++++++++++++++++++++++
>> -
>>  drivers/gpu/drm/i915/intel_drv.h     |  10 +++
>>  4 files changed, 270 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
>> b/drivers/gpu/drm/i915/intel_ddi.c
>> index 3fb9a03..cc5cad5 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -3217,6 +3217,108 @@ intel_ddi_init_hdmi_connector(struct
>> intel_digital_port *intel_dig_port)
>>  	return connector;
>>  }
>>
>> +static void intel_ddi_commit_upfront_pll_config(struct intel_dp *intel_dp,
>> +					struct intel_shared_dpll *pll)
>> +{
>> +	struct intel_shared_dpll_config tmp_config;
>> +
>> +	/*
>> +	 * The shared_dpll_config is computed in intel_get_shared_dpll().
>> +	 * It is committed to 'pll->config' here to be used in
>> +	 * intel_enable/disable_shared_dpll functions. Before committing.
>> +	 * save the existing config, so that once upfront link training is
>> +	 * done, the original value of 'pll->config' can be restored.
>> +	 */
>> +	tmp_config = pll->config;
>> +	pll->config = intel_dp->upfront_pll_config;
>> +	intel_dp->upfront_pll_config = tmp_config;
>> +}
>> +
>> +static struct intel_shared_dpll *intel_ddi_select_upfront_pll_config(
>> +			struct intel_dp *intel_dp, struct intel_crtc *crtc)
>> +{
>> +	if (!intel_ddi_pll_select(crtc, crtc->config))
>> +		return NULL;
>> +
>> +	return intel_crtc_to_shared_dpll(crtc);
>> +}
>> +
>> +static void intel_ddi_clear_upfront_pll_config(struct intel_dp *intel_dp,
>> +					struct intel_shared_dpll *pll)
>> +{
>> +	pll->config = intel_dp->upfront_pll_config;
>> +}
>> +
>
>The shared pll interface is really getting in the way here. And BXT plls aren't
>even shared. We are jumping through hoops to get the pll that matches the given
>encoder and to call the code that calculates the dpll_hw_state based on the DP
>link rate. I'd like to get that interface fixed but I reckon it will be tricky
>to find something that works for all the platforms we support. That's the next
>thing on my todo list.
>
>If we have to live with some intermediary solution, I think it would be better
>to (almost) completely bypass the shared dpll stuff. First we would need to
>split bxt_ddi_pll_sel() so that we would have a function that takes the link
>bandwidth and spits out a dpll_hw_state without looking at crtc_state. Then we
>would just take the right pll based on dig_port->port, make sure it is not being
>used and program it with the hw state we get from that new function.
>
>I wrote a patch to do that split. With it, the PLL enable logic in the upfront
>link train logic would look a bit like below. There is some potential for
>problems with the state checker, but it should be fine as long as the old dpll

This is similar to what we had in the earlier version except the
ddi_pll_select split.. I moved it this way because we wanted all the
dpll functionality within the dpll interfaces. But yes, as you said,
we could not get the whole thing cleaner.

>hw state is saved and restored when everything is over.
>
>
>/* FIXME: this only works for BXT */
>dpll_id = (enum intel_dpll_id) dig_port->port;
>pll = dev_priv->shared_dplls[dpll_id];
>
>if (WARN_ON(pll->config.crtc_mask) || WARN_ON(pll->active))
>	goto exit;
>
>bxt_ddi_dp_pll_dividers(crtc->config->port_clock, &clk_div);
>if (!bxt_ddi_set_dpll_hw_state(clock, &clk_div,
>			       &pll->config.hw_state)) {
>	DRM_ERROR("Could not setup DPLL\n");
>	goto exit;
>}
>
>
>/* Enable PLL followed by port */
>pll->enable(dev_priv, pll);
>encoder->pre_enable(encoder);
>
>
>This solution would remove the need for patches 1 and 3.
>
>What do you think?

This (and your other patch) looks ok to me. I will test and confirm once.

>
>> +int intel_ddi_upfront_link_train(struct intel_dp *intel_dp,
>> +				struct intel_crtc *crtc)
>> +{
>> +	struct intel_connector *connector = intel_dp->attached_connector;
>> +	struct intel_encoder *encoder = connector->encoder;
>> +	struct intel_shared_dpll *pll = NULL;
>> +	struct drm_crtc *drm_crtc = NULL;
>> +	struct intel_crtc_state *tmp_crtc_config;
>> +	uint8_t link_bw, lane_count;
>> +
>> +	if (!crtc) {
>> +		drm_crtc = intel_get_unused_crtc(&encoder->base);
>> +		if (!drm_crtc) {
>> +			DRM_ERROR("No crtc for upfront link training\n");
>> +			return -EINVAL;
>> +		}
>> +		encoder->base.crtc = drm_crtc;
>> +		crtc = to_intel_crtc(drm_crtc);
>> +	}
>> +
>> +	/* Initialize with Max Link rate & lane count supported by panel */
>> +	link_bw =  intel_dp->dpcd[DP_MAX_LINK_RATE];
>> +	lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
>> +
>> +	/* Save the crtc->config */
>> +	tmp_crtc_config = crtc->config;
>> +
>> +	DRM_DEBUG_KMS("Using pipe %c\n", pipe_name(crtc->pipe));
>> +	do {
>> +		crtc->config->port_clock =
>> drm_dp_bw_code_to_link_rate(link_bw);
>> +		crtc->config->lane_count = lane_count;
>> +
>> +		pll = intel_ddi_select_upfront_pll_config(intel_dp, crtc);
>> +		if (!pll) {
>> +			DRM_ERROR("Could not get shared DPLL\n");
>> +			goto exit;
>> +		}
>> +
>> +		intel_ddi_commit_upfront_pll_config(intel_dp, pll);
>> +
>> +		/* Enable PLL followed by port */
>> +		intel_enable_shared_dpll(crtc);
>> +		encoder->pre_enable(encoder);
>> +
>> +		/* Check if link training passed; if so update DPCD */
>> +		if (intel_dp->train_set_valid)
>> +			intel_dp_update_dpcd_params(intel_dp);
>> +
>> +		/* Disable port followed by PLL for next retry/clean up */
>> +		encoder->post_disable(encoder);
>> +		intel_disable_shared_dpll(crtc);
>> +
>> +		intel_ddi_clear_upfront_pll_config(intel_dp, pll);
>> +
>> +	} while (!intel_dp->train_set_valid &&
>> +		intel_dp_get_link_retry_params(intel_dp, &lane_count,
>> &link_bw));
>> +exit:
>> +	/* Reset local associations made */
>> +	if (drm_crtc)
>> +		encoder->base.crtc = NULL;
>> +
>> +	/* Restore crtc config */
>> +	crtc->config = tmp_crtc_config;
>> +
>> +	DRM_DEBUG_KMS("Upfront link train %s: lanes:%d bw:%d\n",
>> +	intel_dp->train_set_valid ? "Passed" : "Failed", lane_count,
>> link_bw);
>> +
>> +	return intel_dp->train_set_valid ? 0 : -1;
>> +}
>> +
>>  void intel_ddi_init(struct drm_device *dev, enum port port)
>>  {
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index af50e61..6a650aa 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4238,16 +4238,45 @@ static void lpt_pch_enable(struct drm_crtc *crtc)
>>  	lpt_enable_pch_transcoder(dev_priv, cpu_transcoder);
>>  }
>>
>> +static
>> +void intel_get_new_shared_dpll_config(struct drm_i915_private *dev_priv,
>> +				  struct intel_shared_dpll_config
>> *shared_dpll)
>> +{
>> +	enum intel_dpll_id i;
>> +
>> +	/* Create a new shared dpll config by duplicating pll->config */
>> +	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>> +		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
>> +		shared_dpll[i] = pll->config;
>> +		shared_dpll[i].crtc_mask = 0;
>> +	}
>> +}
>> +
>>  struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>>  						struct intel_crtc_state
>> *crtc_state)
>>  {
>>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>>  	struct intel_shared_dpll *pll;
>>  	struct intel_shared_dpll_config *shared_dpll;
>> +	struct intel_shared_dpll_config tmp_config[I915_NUM_PLLS];
>>  	enum intel_dpll_id i;
>>  	int max = dev_priv->num_shared_dpll;
>>
>> -	shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state
>> ->base.state);
>> +	/*
>> +	 * intel_get_shared_dpll needs a shared_dpll[] to store the computed
>> +	 * dpll_config values. For atomic path, it is stored in
>> +	 * intel_atomic_state->shared_dpll[], which is later committed to
>> +	 * dev_priv->shared_dpll[] in atomic commit. For non-atomic path,
>> +	 * (where intel_atomic_state does not exist) the dpll_config values
>> +	 * are stored in 'tmp_config[]' and copied to encoder structures
>> +	 * for later use.
>> +	 */
>> +	if (crtc_state->base.state) {
>> +		shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state
>> ->base.state);
>> +	} else {
>> +		intel_get_new_shared_dpll_config(dev_priv, tmp_config);
>> +		shared_dpll = tmp_config;
>> +	}
>>
>>  	if (HAS_PCH_IBX(dev_priv->dev)) {
>>  		/* Ironlake PCH has a fixed PLL->PCH pipe mapping. */
>> @@ -4277,6 +4306,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct
>> intel_crtc *crtc,
>>  		pll = &dev_priv->shared_dplls[i];
>>  		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
>>  			crtc->base.base.id, pll->name);
>> +
>>  		WARN_ON(shared_dpll[i].crtc_mask);
>>
>>  		goto found;
>> @@ -4325,6 +4355,12 @@ found:
>>
>>  	shared_dpll[i].crtc_mask |= 1 << crtc->pipe;
>>
>> +	/*
>> +	 * For DP, this shared dpll config is saved to intel_dp,
>> +	 * and used by upfront link train logic subsequently.
>> +	 */
>> +	intel_dp_update_shared_dpll_config(crtc_state, shared_dpll[i]);
>> +
>>  	return pll;
>>  }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index e2bea710..cc7b6f3 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1679,6 +1679,55 @@ void intel_dp_set_link_params(struct intel_dp
>> *intel_dp,
>>  	intel_dp->lane_count = pipe_config->lane_count;
>>  }
>>
>> +void intel_dp_update_dpcd_params(struct intel_dp *intel_dp)
>> +{
>> +	intel_dp->dpcd[DP_MAX_LANE_COUNT] &= ~DP_MAX_LANE_COUNT_MASK;
>> +	intel_dp->dpcd[DP_MAX_LANE_COUNT] |=
>> +			intel_dp->lane_count & DP_MAX_LANE_COUNT_MASK;
>> +
>> +	intel_dp->dpcd[DP_MAX_LINK_RATE] =
>> +			drm_dp_link_rate_to_bw_code(intel_dp->link_rate);
>
>Maybe it would be good to have an explicit field for actual max lane count and
>link rate. That way, reading the DPCD from debugfs will give the actual results
>read from the sink instead of an edited value.

Looks like the 'dpcd_show' interface reads the DPCD everytime and prints
It .. And does not seem to look up at intel_dp->dpcd[].
Or, are we talking about two different interfaces ?

>
>> +}
>> +
>> +bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp,
>> +			uint8_t *lane_count, uint8_t *link_bw)
>> +{
>> +	/*
>> +	 * As per DP1.3 Spec, retry all link rates for a particular
>> +	 * lane count value, before reducing number of lanes.
>> +	 */
>> +	if (*link_bw == DP_LINK_BW_5_4) {
>> +		*link_bw = DP_LINK_BW_2_7;
>> +	} else if (*link_bw == DP_LINK_BW_2_7) {
>> +		*link_bw = DP_LINK_BW_1_62;
>> +	} else if (*lane_count == 4) {
>> +		*lane_count = 2;
>> +		*link_bw = intel_dp_max_link_bw(intel_dp);
>> +	} else if (*lane_count == 2) {
>> +		*lane_count = 1;
>> +		*link_bw = intel_dp_max_link_bw(intel_dp);
>> +	} else {
>> +		/* Tried all combinations, so exit */
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +void intel_dp_update_shared_dpll_config(struct intel_crtc_state *pipe_config,
>> +				struct intel_shared_dpll_config config)
>> +{
>> +	struct intel_encoder *encoder;
>> +	struct intel_dp *intel_dp;
>> +
>> +	encoder = intel_ddi_get_crtc_new_encoder(pipe_config);
>> +	if (!encoder || encoder->type != INTEL_OUTPUT_DISPLAYPORT)
>> +		return;
>> +
>> +	intel_dp = enc_to_intel_dp(&encoder->base);
>> +	intel_dp->upfront_pll_config = config;
>> +}
>> +
>>  static void intel_dp_prepare(struct intel_encoder *encoder)
>>  {
>>  	struct drm_device *dev = encoder->base.dev;
>> @@ -4601,6 +4650,66 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
>>  	intel_dp->has_audio = false;
>>  }
>>
>> +static bool intel_dp_upfront_link_train(struct drm_connector *connector)
>> +{
>> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
>> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>> +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
>> +	struct drm_device *dev = intel_encoder->base.dev;
>> +	struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
>> +	struct drm_mode_config *config = &dev->mode_config;
>> +	struct intel_crtc *intel_crtc = crtc ? to_intel_crtc(crtc) : NULL;
>> +	struct drm_modeset_acquire_ctx ctx, *old_ctx = NULL;
>> +	bool need_dpms_on = false;
>> +	int ret;
>> +
>> +	if (!IS_BROXTON(dev))
>> +		return true;
>> +
>> +	/*
>> +	 * On hotplug cases, we call _upfront_link_train directly.
>> +	 * In connected boot scenarios (boot with {MIPI/eDP} + DP),
>> +	 * BIOS typically brings up DP. Hence, we disable the crtc
>> +	 * to do _upfront_link_training and then re-enable it back.
>> +	 */
>> +	if (crtc && crtc->enabled && intel_crtc->active) {
>> +		old_ctx = crtc->acquire_ctx;
>> +		drm_modeset_acquire_init(&ctx, 0);
>> +retry:
>> +		ret = drm_modeset_lock(&config->connection_mutex, &ctx);
>> +		if (ret == -EDEADLK) {
>> +			drm_modeset_backoff(&ctx);
>> +			goto retry;
>> +		}
>
>We need to acquire those locks even if the crtc is disabled, otherwise a modeset
>could sneak past and wreak havoc while we are doing the link training and
>fiddling with the plls.
>
>> +
>> +		crtc->acquire_ctx = &ctx;
>> +		ret = drm_atomic_helper_connector_dpms(connector,
>> DRM_MODE_DPMS_OFF);
>
>The crtc->acquire_ctx override seems wrong. One thing that slipped by me when I
>looked at the previous version of this patch is that we need to acquire some
>locks even if the crtc is already off. So it is probably better to write a

Ok, I think this is what Ville is also mentioning. I did not notice it either ;-(
I am still learning the scheme of ww_* and ctx based locking..
So, yes, will re-work this.

>separate function that takes an acquire context and uses atomic to set
>crtc_state->active to false and calls drm_atomic_commit().
>
>> +		if (ret) {
>> +			DRM_ERROR("DPMS off failed:%d\n", ret);
>> +			goto exit_unlock;
>> +		}
>> +
>> +		need_dpms_on = true;
>> +	}
>> +
>> +	if (HAS_DDI(dev))
>> +		ret = intel_ddi_upfront_link_train(intel_dp, intel_crtc);
>> +		/* Other platforms upfront link train call goes here..*/
>> +
>> +	if (!need_dpms_on)
>> +		return ret;
>> +
>> +	ret = drm_atomic_helper_connector_dpms(connector, DRM_MODE_DPMS_ON);
>> +	if (ret)
>> +		DRM_ERROR("DPMS on failed:%d\n", ret);
>> +
>> +exit_unlock:
>> +	drm_modeset_drop_locks(&ctx);
>> +	drm_modeset_acquire_fini(&ctx);
>> +	crtc->acquire_ctx = old_ctx;
>> +	return ret;
>
>The return type of this function is bool. The conversion here will do the wrong
>thing, since ret potentially has the return value of functions that return non
>-zero on failure.

I wanted to make this as an 'int'. I missed this. Will fix it in next version.

>
>> +}
>> +
>>  static enum drm_connector_status
>>  intel_dp_detect(struct drm_connector *connector, bool force)
>>  {
>> @@ -4610,7 +4719,8 @@ intel_dp_detect(struct drm_connector *connector, bool
>> force)
>>  	struct drm_device *dev = connector->dev;
>>  	enum drm_connector_status status;
>>  	enum intel_display_power_domain power_domain;
>> -	bool ret;
>> +	bool do_upfront_link_train;
>> +	int ret;
>>  	u8 sink_irq_vector;
>>
>>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>> @@ -4684,6 +4794,16 @@ intel_dp_detect(struct drm_connector *connector, bool
>> force)
>>  			DRM_DEBUG_DRIVER("CP or sink specific irq
>> unhandled\n");
>>  	}
>>
>> +	/* Do not do upfront link train, if it is a compliance request */
>> +	do_upfront_link_train =
>> +		intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT &&
>> +		intel_dp->compliance_test_type != DP_TEST_LINK_TRAINING;
>> +
>> +	if (do_upfront_link_train) {
>> +		ret = intel_dp_upfront_link_train(connector);
>> +		if (ret)
>> +			status = connector_status_disconnected;
>> +	}
>
>So if upfront link training fails because there is no unused crtc we'll say that
>the connector is disconnected?

I was not sure on what should be the stand we should take here..

Should we distinguish various causes of upfront failure and then
report status accordingly ? Not just crtc but pll selection, link training
failures, etc..

Please let me know what you think.

Thanks,
Durga

>
>
>Ander
>
>>  out:
>>  	intel_display_power_put(to_i915(dev), power_domain);
>>  	return status;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 9fe7c4b..c5ca4ab 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -812,6 +812,9 @@ struct intel_dp {
>>  	unsigned long compliance_test_type;
>>  	unsigned long compliance_test_data;
>>  	bool compliance_test_active;
>> +
>> +	/* Used to store pll config for upfront link training */
>> +	struct intel_shared_dpll_config upfront_pll_config;
>>  };
>>
>>  struct intel_digital_port {
>> @@ -1031,6 +1034,8 @@ void intel_ddi_clock_get(struct intel_encoder *encoder,
>>  			 struct intel_crtc_state *pipe_config);
>>  void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
>>  uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
>> +int intel_ddi_upfront_link_train(struct intel_dp *intel_dp,
>> +				struct intel_crtc *crtc);
>>
>>  /* intel_frontbuffer.c */
>>  void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
>> @@ -1239,6 +1244,9 @@ bool intel_dp_init_connector(struct intel_digital_port
>> *intel_dig_port,
>>  			     struct intel_connector *intel_connector);
>>  void intel_dp_set_link_params(struct intel_dp *intel_dp,
>>  			      const struct intel_crtc_state *pipe_config);
>> +void intel_dp_update_dpcd_params(struct intel_dp *intel_dp);
>> +bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp,
>> +				uint8_t *lane_count, uint8_t *link_bw);
>>  void intel_dp_start_link_train(struct intel_dp *intel_dp);
>>  void intel_dp_stop_link_train(struct intel_dp *intel_dp);
>>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
>> @@ -1246,6 +1254,8 @@ void intel_dp_encoder_destroy(struct drm_encoder
>> *encoder);
>>  int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
>>  bool intel_dp_compute_config(struct intel_encoder *encoder,
>>  			     struct intel_crtc_state *pipe_config);
>> +void intel_dp_update_shared_dpll_config(struct intel_crtc_state *pipe_config,
>> +				struct intel_shared_dpll_config config);
>>  bool intel_dp_is_edp(struct drm_device *dev, enum port port);
>>  enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port,
>>  				  bool long_hpd);