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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Ander,

Thanks for looking into this..

>-----Original Message-----
>From: Ander Conselvan De Oliveira [mailto:conselvan2@xxxxxxxxx]
>Sent: Tuesday, December 29, 2015 10:52 PM
>To: R, Durgadoss; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>Subject: Re: [PATCH 7/7] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT
>
>On Fri, 2015-12-11 at 15:09 +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.
>>
>> Signed-off-by: Durgadoss R <durgadoss.r@xxxxxxxxx>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c | 81
>> ++++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_dp.c  | 77 +++++++++++++++++++++++++++++++++++++-
>>  drivers/gpu/drm/i915/intel_drv.h |  2 +
>>  3 files changed, 159 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
>> b/drivers/gpu/drm/i915/intel_ddi.c
>> index 632044a..43efc26 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -3274,6 +3274,87 @@ intel_ddi_init_hdmi_connector(struct intel_digital_port
>> *intel_dig_port)
>>  	return connector;
>>  }
>>
>> +bool intel_ddi_upfront_link_train(struct intel_dp *intel_dp,
>> +				struct intel_crtc *crtc)
>> +{
>> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> +	struct intel_connector *connector = intel_dp->attached_connector;
>> +	struct intel_encoder *encoder = connector->encoder;
>> +	struct drm_device *dev = encoder->base.dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct intel_shared_dpll *pll;
>> +	struct drm_crtc *drm_crtc = NULL;
>> +	struct intel_crtc_state *tmp_crtc_config;
>> +	struct intel_dpll_hw_state tmp_dpll_hw_state;
>> +	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 false;
>> +		}
>> +		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;
>
>> +	tmp_dpll_hw_state = crtc->config->dpll_hw_state;
>> +
>> +	/* Select the shared DPLL to use for this port */
>> +	intel_get_ddi_pll(dev_priv, dig_port->port, crtc->config)
>
>This reads the current programmed DPLL from the hardware. Is there any reason we
>can't trust the value that is in crtc->config already? I don't think this code
>would run before reading out and sanitizing the hardware state.

I was not sure of what will be the DPLL attached to crtc->config when the
crtc is found by 'get_unused_crtc' logic. So, we select the DPLL based
on port again..

>
>> ;
>> +	pll = intel_crtc_to_shared_dpll(crtc);
>> +	if (!pll) {
>> +		DRM_ERROR("Could not get shared DPLL\n");
>> +		goto exit;
>> +	}
>> +	DRM_DEBUG_KMS("Using %s for pipe %c\n", pll->name, pipe_name(crtc
>> ->pipe));
>> +
>> +	do {
>> +		crtc->config->port_clock =
>> drm_dp_bw_code_to_link_rate(link_bw);
>> +		crtc->config->lane_count = lane_count;
>> +		if (!intel_ddi_pll_select(crtc, crtc->config, encoder,
>> false))
>
>
>> +			goto exit;
>> +
>> +		pll->config.crtc_mask |= (1 << crtc->pipe);
>> +		pll->config.hw_state = crtc->config->dpll_hw_state;
>> +
>> +		/* Enable PLL followed by port */
>> +		intel_enable_shared_dpll(crtc);
>> +		encoder->pre_enable(encoder);
>
>The pll handling here seems dodgy. Instead of using intel_get_shared_dpll(),

I initially tried this, but intel_get_shared_dpll() uses crtc_state which is not
set (in outside modeset contexts) thus creating crashes. Specifically, 
the call to intel_ddi_get_crtc_new_encoder() for broxton code path.

That’s why I had to create some initial code to not call get_shared_dpll
If we have valid encoder attached. (patches 1/7 and 2/7 of this series)

So, If you have suggestions on how to fix this in a neat way,
I would be happy to try them out.

Thanks,
Durga

>this fiddles with pll internals itself. I think this will work for broxton,
>since it doesn't actually have shared DPLLs (their chosen based on the encoder).
>It might just work for haswell too since the plls used by DP are not shared.
>
>But to do this cleanly we need the DPLL interface to just give us the right pll.
>
>Ander
>
>
>> +
>> +		/* 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);
>> +
>> +	} while (!intel_dp->train_set_valid &&
>> +		!intel_dp_get_link_retry_params(&lane_count, &link_bw));
>> +
>> +	/* Reset pll state as before */
>> +	pll->config.crtc_mask &= ~(1 << crtc->pipe);
>> +	pll->config.hw_state = tmp_dpll_hw_state;
>> +
>> +exit:
>> +	/* Reset local associations made */
>> +	if (drm_crtc)
>> +		encoder->base.crtc = NULL;
>> +	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;
>> +}
>> +
>>  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 d8f7830..47b6266 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4622,6 +4622,71 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
>>  	intel_dp->has_audio = false;
>>  }
>>
>> +static void intel_dp_upfront_dpms_off(struct drm_connector *connector,
>> +				struct drm_modeset_acquire_ctx *ctx)
>> +{
>> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
>> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>> +	struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
>> +	struct intel_load_detect_pipe tmp;
>> +
>> +	if (intel_get_load_detect_pipe(connector, NULL, &tmp, ctx)) {
>> +		crtc->acquire_ctx = ctx;
>> +		tmp.dpms_mode = DRM_MODE_DPMS_OFF;
>> +		intel_release_load_detect_pipe(connector, &tmp, ctx);
>> +	}
>> +}
>> +
>> +static void intel_dp_upfront_dpms_on(struct drm_connector *connector,
>> +				struct drm_modeset_acquire_ctx *ctx)
>> +{
>> +	struct intel_load_detect_pipe tmp;
>> +
>> +	intel_get_load_detect_pipe(connector, NULL, &tmp, ctx);
>> +
>> +	drm_modeset_drop_locks(ctx);
>> +	drm_modeset_acquire_fini(ctx);
>> +}
>> +
>> +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 intel_crtc *intel_crtc = crtc ? to_intel_crtc(crtc) : NULL;
>> +	struct drm_modeset_acquire_ctx ctx, *old_ctx = NULL;
>> +	bool ret = true, need_dpms_on = false;
>> +
>> +	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) {
>> +		DRM_DEBUG_KMS("Disabling pipe %c\n", pipe_name(intel_crtc
>> ->pipe));
>> +		old_ctx = crtc->acquire_ctx;
>> +		drm_modeset_acquire_init(&ctx, 0);
>> +		intel_dp_upfront_dpms_off(connector, &ctx);
>> +		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) {
>> +		intel_dp_upfront_dpms_on(connector, &ctx);
>> +		crtc->acquire_ctx = old_ctx;
>> +	}
>> +	return ret;
>> +}
>> +
>> +
>>  static enum drm_connector_status
>>  intel_dp_detect(struct drm_connector *connector, bool force)
>>  {
>> @@ -4631,7 +4696,7 @@ 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 ret, do_upfront_link_train;
>>  	u8 sink_irq_vector;
>>
>>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>> @@ -4705,6 +4770,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;
>> +	}
>>  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 5912955..3cfab20 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1025,6 +1025,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);
>> +bool 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,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux