Re: [PATCHv4 3/3] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Mon, 2016-04-18 at 19:01 +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 and then do upfront link training; and let the subsequent
>   modeset re-enable the crtc.
> * 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.
> 
> Changes since v3:
> * Fixed a return value on BXT check
> * Reworked on top of bxt_ddi_pll_select split from Ander
> * Renamed from ddi_upfront to bxt_upfront since the
>   upfront logic includes BXT specific functions for now.
> Changes since v2:
> * Rebased on top of latest dpll_mgr.c code and
>   latest HPD related clean ups.
> * Corrected return values from upfront (Ander)
> * Corrected atomic locking for upfront in intel_dp.c (Ville)
> Changes since v1:
> *  all pll related functions inside ddi.c
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c |  73 ++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c  | 180
> ++++++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h |   5 ++
>  3 files changed, 256 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 96234c5..f7fa3db 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2268,6 +2268,79 @@ intel_ddi_init_hdmi_connector(struct intel_digital_port
> *intel_dig_port)
>  	return connector;
>  }
>  
> +int intel_bxt_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 drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_shared_dpll *pll;
> +	struct intel_shared_dpll_config tmp_pll_config;
> +	struct bxt_clk_div clk_div = {0};
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +	enum intel_dpll_id dpll_id = (enum intel_dpll_id)dig_port->port;
> +	uint8_t link_bw, lane_count;
> +
> +	if (WARN_ON(!crtc))
> +		return -EINVAL;
> +
> +	/* 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);

What if the panel supports HBR3? We should use the maximum common rate between
source and sink. And it would be nice if those details where in intel_dp.c.
Ideally, the platform specific part would take link rate and lane count as
arguments and just do the pll/encoder enabling/disabling.

> +
> +	mutex_lock(&dev_priv->dpll_lock);
> +	/*
> +	 * FIXME: Works only for BXT.
> +	 * Select the required PLL. This works for platforms where
> +	 * there is no shared DPLL.
> +	 */
> +	pll = &dev_priv->shared_dplls[dpll_id];
> +	if (WARN_ON(pll->active_mask)) {
> +		DRM_ERROR("Shared DPLL already in use. active_mask:%x\n",
> pll->active_mask);
> +		mutex_unlock(&dev_priv->dpll_lock);
> +		return -EINVAL;
> +	}
> +
> +	tmp_pll_config = pll->config;
> +
> +	DRM_DEBUG_KMS("Using pipe %c with pll %s\n",
> +			pipe_name(crtc->pipe), pll->name);
> +	do {
> +		crtc->config->port_clock =
> drm_dp_bw_code_to_link_rate(link_bw);
> +		crtc->config->lane_count = lane_count;
> +
> +		bxt_ddi_dp_pll_dividers(crtc->config->port_clock, &clk_div);
> +		if (!bxt_ddi_set_dpll_hw_state(crtc->config->port_clock,
> +					&clk_div, &pll->config.hw_state)) {
> +			DRM_ERROR("Could not setup DPLL\n");
> +			goto exit_pll;
> +		}
> +
> +		/* Enable PLL followed by port */
> +		pll->funcs.enable(dev_priv, pll);
> +		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);
> +		pll->funcs.disable(dev_priv, pll);
> +
> +	} while (!intel_dp->train_set_valid &&
> +		intel_dp_get_link_retry_params(intel_dp, &lane_count,
> &link_bw));
> +
> +	DRM_DEBUG_KMS("Upfront link train %s: lanes:%d bw:%d\n",
> +	intel_dp->train_set_valid ? "Passed" : "Failed", lane_count,
> link_bw);
> +
> +exit_pll:
> +	pll->config = tmp_pll_config;
> +	mutex_unlock(&dev_priv->dpll_lock);
> +
> +	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_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 61ee226..57d7159 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1595,6 +1595,41 @@ 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);

I think I mentioned this before. I don't think we should update the dpcd fields
directly. Instead, there should be a field for the max rate and lane count
achieved through upfront link training in struct intel_dp and
intel_dp_max_link_bw() and intel_dp_max_lane_count() should take that into
account.

> +}
> +
> +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;
> +}
> +
>  static void intel_dp_prepare(struct intel_encoder *encoder)
>  {
>  	struct drm_device *dev = encoder->base.dev;
> @@ -4548,6 +4583,135 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
>  	intel_dp->has_audio = false;
>  }
>  
> +static struct intel_crtc_state *intel_dp_upfront_get_crtc_state(
> +				struct intel_crtc *crtc,
> +				struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_atomic_state *state;
> +	struct intel_crtc_state *crtc_state;
> +
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state)
> +		return ERR_PTR(-ENOMEM);
> +
> +	state->acquire_ctx = ctx;
> +
> +	crtc_state = intel_atomic_get_crtc_state(state, crtc);
> +	if (IS_ERR(crtc_state)) {
> +		drm_atomic_state_free(state);
> +		state = NULL;
> +	}

All this function does is call intel_atomic_get_crtc_state(). Since the caller
also have to deal with error handling, I don't see what it adds on top of that.
The code is actually simpler if merged with the function below.

> +
> +	return crtc_state;
> +}
> +
> +static int intel_dp_upfront_commit(struct intel_crtc *crtc,
> +				struct drm_modeset_acquire_ctx *ctx)

This function could use a better name. Something that suggests the crtc is being
disabled. Maybe disable_crtc_for_upfront() ? It doens't really depends on
anything DP specific, so maybe it would make sense to move it to intel_display.c
or intel_atomic.c. Then we could call it intel_crtc_atomic_disable_locked(). The
"locked" part since this assumes the ctx already have crtc->mutex locked so that
there's no risk of -EDEADLK. Although it can just pass the -EDEADLK error to the
caller, so just intel_crtc_atomic_disable() ? 



> +{
> +	int ret;
> +	struct intel_crtc_state *crtc_state;
> +	enum pipe pipe = crtc->pipe;
> +
> +	crtc_state = intel_dp_upfront_get_crtc_state(crtc, ctx);
> +	if (IS_ERR(crtc_state))
> +		return PTR_ERR(crtc_state);
> +
> +	DRM_DEBUG_KMS("Disabling crtc %c for upfront link train\n",
> pipe_name(pipe));
> +
> +	crtc_state->base.active = false;
> +	ret = drm_atomic_commit(crtc_state->base.state);
> +	if (ret) {
> +		drm_atomic_state_free(crtc_state->base.state);
> +		crtc_state->base.state = NULL;

The function drm_atomic_state_free() frees the crtc_state, so this is a use
after free.

> +	}
> +	return ret;
> +}
> +
> +static int intel_dp_upfront_link_train(struct drm_connector *connector)
> +{
> +	struct intel_dp *intel_dp = intel_attached_dp(connector);

You could just pass intel_dp directly. The only use of connector is when
connector->state->crtc is inspected. But that should have the same value as
intel_dp->base.base.crtc

> +	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_mode_config *config = &dev->mode_config;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct intel_crtc_state *crtc_state, *tmp_crtc_config;
> +	struct intel_crtc *intel_crtc;
> +	struct drm_crtc *crtc = NULL;
> +	bool crtc_enabled = false;
> +	int ret, status = 0;
> +
> +	if (!IS_BROXTON(dev))
> +		return 0;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +retry:
> +	ret = drm_modeset_lock(&config->connection_mutex, &ctx);
> +	if (ret)
> +		goto exit_fail;
> +
> +	if (connector->state->crtc) {
> +		crtc = connector->state->crtc;
> +
> +		ret = drm_modeset_lock(&crtc->mutex, &ctx);
> +		if (ret)
> +			goto exit_fail;
> +
> +		crtc_enabled = true;
> +	} else {
> +		crtc = intel_get_unused_crtc(&intel_encoder->base, &ctx);
> +		if (IS_ERR_OR_NULL(crtc)) {
> +			ret = PTR_ERR_OR_ZERO(crtc);
> +			DRM_DEBUG_KMS("No crtc available for upfront link
> train:%d\n", ret);
> +			goto exit_fail;
> +		}
> +		intel_encoder->base.crtc = crtc;
> +	}

IMO, the following would be a bit more readable.

	if (intel_encoder->base.crtc)
		crtc = intel_encoder->base.crtc;
	else
		crtc = intel_get_unused_crtc(&intel_encoder->base, &ctx);

	if (IS_ERR(crtc)) {
		...
	}

	ret = drm_modeset_lock(&crtc->mutex, &ctx);
	if (ret)
		goto exit_fail;

	crtc_enabled = crtc->config->active;

> +
> +	intel_crtc = to_intel_crtc(crtc);
> +	DRM_DEBUG_KMS("Using pipe %c for upfront link training\n",
> +					pipe_name(intel_crtc->pipe));
> +
> +	ret = drm_modeset_lock(&crtc->primary->mutex, &ctx);
> +	if (ret)
> +		goto exit_fail;
> +
> +	if (crtc_enabled) {
> +		ret = intel_dp_upfront_commit(intel_crtc, &ctx);
> +		if (ret)
> +			goto exit_fail;
> +	}
> +
> +	crtc_state = intel_dp_upfront_get_crtc_state(intel_crtc, &ctx);
> +	if (IS_ERR(crtc_state)) {
> +		ret = PTR_ERR(crtc_state);
> +		goto exit_fail;
> +	}
> +
> +	/* Save the existing config */
> +	tmp_crtc_config = intel_crtc->config;

You need to save this earlier. Otherwise if intel_dp_upfront_commit() is called,
the state will be restored with active == false when it should actually be true.
But that would cause intel_crtc->config to be freed, so you need to
use intel_crtc_duplicate_state().

> +	intel_crtc->config = crtc_state;
> +
> +	if (IS_BROXTON(dev))
> +		status = intel_bxt_upfront_link_train(intel_dp, intel_crtc);
> +		/* Other platforms upfront link train call goes here..*/
> +
> +	/* Restore the saved config */
> +	intel_crtc->config = tmp_crtc_config;
> +	intel_encoder->base.crtc = crtc_enabled ? crtc : NULL;
> +	drm_atomic_state_free(crtc_state->base.state);

This doesn't actually re-enables the crtc if if was disabled in
intel_dp_upfront_commit(). This will require another atomic operation.
Maybe that intel_crtc_atomic_disable() should just take a bool specifying if it
should on of off.


> +
> +exit_fail:
> +	if (ret == -EDEADLK) {
> +		drm_modeset_backoff(&ctx);
> +		goto retry;
> +	}
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +	return status;
> +}
> +
>  static void
>  intel_dp_long_pulse(struct intel_connector *intel_connector)
>  {
> @@ -4558,7 +4722,7 @@ intel_dp_long_pulse(struct intel_connector
> *intel_connector)
>  	struct drm_device *dev = connector->dev;
>  	enum drm_connector_status status;
>  	enum intel_display_power_domain power_domain;
> -	bool ret;
> +	bool ret, do_upfront_link_train;
>  	u8 sink_irq_vector;
>  
>  	power_domain = intel_display_port_aux_power_domain(intel_encoder);
> @@ -4604,7 +4768,11 @@ intel_dp_long_pulse(struct intel_connector
> *intel_connector)
>  		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  		intel_dp_check_link_status(intel_dp);
>  		drm_modeset_unlock(&dev->mode_config.connection_mutex);
> -		goto out;
> +		/*
> +		 * If we are here, we have (re)read DPCD above and hence
> +		 * need to do upfront link train again.
> +		 */
> +		goto upfront;

We can avoid this by not overwriting intel_dp->dpcd as I mentioned above.


Ander

>  	}
>  
>  	/*
> @@ -4634,6 +4802,14 @@ intel_dp_long_pulse(struct intel_connector
> *intel_connector)
>  			DRM_DEBUG_DRIVER("CP or sink specific irq
> unhandled\n");
>  	}
>  
> +upfront:
> +	/* 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 && intel_dp_upfront_link_train(connector))
> +		status = connector_status_disconnected;
>  out:
>  	if (status != connector_status_connected) {
>  		intel_dp_unset_edid(intel_dp);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 27b5dcd..bf98473 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1082,6 +1082,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_bxt_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,
> @@ -1284,6 +1286,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);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx