[Intel-gfx] [PATCH 7/7] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT
R, Durgadoss
durgadoss.r at intel.com
Tue Dec 29 10:50:46 PST 2015
Hi Ander,
Thanks for looking into this..
>-----Original Message-----
>From: Ander Conselvan De Oliveira [mailto:conselvan2 at gmail.com]
>Sent: Tuesday, December 29, 2015 10:52 PM
>To: R, Durgadoss; intel-gfx at lists.freedesktop.org
>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 at intel.com>
>> ---
>> 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,
More information about the Intel-gfx
mailing list