Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752597AbbG3Oo4 (ORCPT ); Thu, 30 Jul 2015 10:44:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39745 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751062AbbG3Ooz (ORCPT ); Thu, 30 Jul 2015 10:44:55 -0400 Date: Thu, 30 Jul 2015 10:44:52 -0400 From: Benjamin Tissoires To: Sivakumar Thulasimani Cc: Daniel Vetter , intel-gfx@lists.freedesktop.org, Todd Broch , linux-kernel@vger.kernel.org Subject: Re: [Intel-gfx] [PATCH 3/3] drm/i915: Support DDI lane reversal for DP Message-ID: <20150730144452.GA12778@mail.corp.redhat.com> References: <1438099409-25456-1-git-send-email-benjamin.tissoires@redhat.com> <1438099409-25456-4-git-send-email-benjamin.tissoires@redhat.com> <55B88E25.6000006@intel.com> <20150729152240.GD2743@mail.corp.redhat.com> <55B9A471.2070107@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <55B9A471.2070107@intel.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12012 Lines: 317 On Jul 30 2015 or thereabouts, Sivakumar Thulasimani wrote: > > > On 7/29/2015 8:52 PM, Benjamin Tissoires wrote: > >On Jul 29 2015 or thereabouts, Sivakumar Thulasimani wrote: > >>why not detect reverse in intel_dp_detect/intel_hpd_pulse ? that way you can > >>identify both lane count and reversal state without touching anything in the > >>link training code. i am yet to upstream my changes for CHT that i can share > >>if required that does the same in intel_dp_detect without touching any line > >>in link training path. > >With my current limited knowledge of the dp hotplug (and i915 driver) I > >am not sure we could detect the reversed state without trying to train 1 > >lane only. I'd be glad to look at your changes and test them on my > >system if you think that could help having a cleaner solution. > > > >Cheers, > >Benjamin > No, what i recommended was to do link training but in intel_dp_detect. Since > USB Type C cable > also has its own lane count restriction (it can have different lane count > than the one supported > by panel) you might have to figure that out as well. so both reversal and > lane count detection > can be done outside the modeset path and keep the code free of type C > changes outside > detection path. Thanks for the detailed explanations. Now I see what you mean and definitively understand why this is better. > > Please find below the code to do the same. Do not waste time trying to apply > this directly on > nightly since this is based on a local tree and because this is pre- atomic > changes code, so you > might have to modify chv_upfront_link_train to work on top of the latest > nightly code. we > are supposed to upstream this and is in my todo list. Thanks for this patch. I'll try to adapt it to test on the Broadwell laptop I have and get the reversed state detected here. Cheers, Benjamin > > --------------------------------------------------------------------------------------------------------------------------- > > Author: Durgadoss R > Date: Fri May 22 14:30:07 2015 +0530 > > drm/i915: Enable Upfront link training for type-C DP support > > To support USB type C alternate DP mode, the display driver needs to > know the > number of lanes required by 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 the > link > training fails, the driver then falls back to x2 lanes. > > * Since link training is done before modeset, planes are not enabled. > Only > encoder and the its associated PLLs are enabled. > * Once link training is done, the encoder and its PLLs are disabled; so > that > the subsequent modeset is not aware of these changes. > * As of now, this is tested only on CHV. > > Signed-off-by: Durgadoss R > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 0c8ae2a..c72dcaa 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -14793,3 +14793,121 @@ intel_display_print_error_state(struct > drm_i915_error_state_buf *m, > err_printf(m, " VSYNC: %08x\n", error->transcoder[i].vsync); > } > } > + > +bool chv_upfront_link_train(struct drm_device *dev, > + struct intel_dp *intel_dp, struct intel_crtc *crtc) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_connector *connector = intel_dp->attached_connector; > + struct intel_encoder *encoder = connector->encoder; > + bool found = false; > + bool valid_crtc = false; > + > + if (!connector || !encoder) { > + DRM_DEBUG_KMS("dp connector/encoder is NULL\n"); > + return false; > + } > + > + /* If we already have a crtc, start link training directly */ > + if (crtc) { > + valid_crtc = true; > + goto start_link_train; > + } > + > + /* Find an unused crtc and use it for link training */ > + for_each_intel_crtc(dev, crtc) { > + if (intel_crtc_active(&crtc->base)) > + continue; > + > + connector->new_encoder = encoder; > + encoder->new_crtc = crtc; > + encoder->base.crtc = &crtc->base; > + > + /* Make sure the new crtc will work with the encoder */ > + if (drm_encoder_crtc_ok(&encoder->base, > + &crtc->base)) { > + found = true; > + break; > + } > + } > + > + if (!found) { > + DRM_ERROR("Could not find crtc for upfront link training\n"); > + return false; > + } > + > +start_link_train: > + > + DRM_DEBUG_KMS("upfront link training on pipe:%c\n", > + pipe_name(crtc->pipe)); > + found = false; > + > + /* Initialize with Max Link rate & lane count supported by panel */ > + intel_dp->link_bw = intel_dp->dpcd[DP_MAX_LINK_RATE]; > + intel_dp->lane_count = intel_dp->dpcd[DP_MAX_LANE_COUNT] & > + DP_MAX_LANE_COUNT_MASK; > + > + do { > + /* Find port clock from link_bw */ > + crtc->config.port_clock = > + drm_dp_bw_code_to_link_rate(intel_dp->link_bw); > + > + /* Enable PLL followed by port */ > + intel_dp_set_clock(encoder, &crtc->config, intel_dp->link_bw); > + chv_update_pll(crtc); > + encoder->pre_pll_enable(encoder); > + chv_enable_pll(crtc); > + encoder->pre_enable(encoder); > + > + /* Check if link training passed; if so update lane count */ > + if (intel_dp->train_set_valid) { > + 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; > + > + found = true; > + } > + > + /* Reset encoder for next retry or for clean up */ > + encoder->disable(encoder); > + encoder->post_disable(encoder); > + chv_disable_pll(dev_priv, crtc->pipe); > + > + if (found) > + goto exit; > + > + DRM_DEBUG_KMS("upfront link training failed. lanes:%d bw:%d\n", > + intel_dp->lane_count, intel_dp->link_bw); > + > + /* Go down to the next level and retry link training */ > + if (intel_dp->lane_count == 4) { > + intel_dp->lane_count = 2; > + } else if (intel_dp->lane_count == 2) { > + intel_dp->lane_count = 1; > + } else if (intel_dp->link_bw == DP_LINK_BW_5_4) { > + intel_dp->link_bw = DP_LINK_BW_2_7; > + intel_dp->lane_count = 4; > + } else if (intel_dp->link_bw == DP_LINK_BW_2_7) { > + intel_dp->link_bw = DP_LINK_BW_1_62; > + intel_dp->lane_count = 4; > + } else { > + /* Tried all combinations, so exit */ > + break; > + } > + > + } while (1); > + > +exit: > + /* Clear local associations made */ > + if (!valid_crtc) { > + connector->new_encoder = NULL; > + encoder->new_crtc = NULL; > + encoder->base.crtc = NULL; > + } > + > + if (found) > + DRM_DEBUG_KMS("upfront link training passed. lanes:%d bw:%d\n", > + intel_dp->lane_count, intel_dp->link_bw); > + return found; > +} > diff --git a/drivers/gpu/drm/i915/intel_dp.c > b/drivers/gpu/drm/i915/intel_dp.c > index 97f03bf..394a7a5 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -878,7 +878,7 @@ intel_dp_connector_unregister(struct intel_connector > *intel_connector) > intel_connector_unregister(intel_connector); > } > > -static void > +void > intel_dp_set_clock(struct intel_encoder *encoder, > struct intel_crtc_config *pipe_config, int link_bw) > { > @@ -1010,7 +1010,6 @@ intel_dp_compute_config(struct intel_encoder *encoder, > link_clock = drm_dp_bw_code_to_link_rate(bws[clock]); > link_avail = intel_dp_max_data_rate(link_clock, > lane_count); > - > if (mode_rate <= link_avail) { > goto found; > } > @@ -4522,6 +4521,11 @@ g4x_dp_detect(struct intel_dp *intel_dp) > if ((I915_READ(PORT_HOTPLUG_STAT) & bit) == 0) > return connector_status_disconnected; > > + /* Avoid DPCD opertations if status is same */ > + if (intel_dp->attached_connector->base.status == > + connector_status_connected) > + return connector_status_connected; > + > return intel_dp_detect_dpcd(intel_dp); > } > > @@ -4566,11 +4570,13 @@ intel_dp_detect(struct drm_connector *connector, > bool force) > 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_crtc *crtc = intel_dig_port->base.base.crtc; > struct drm_device *dev = connector->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > enum drm_connector_status status; > enum intel_display_power_domain power_domain; > struct edid *edid = NULL; > + struct intel_crtc *intel_crtc = NULL; > > intel_runtime_pm_get(dev_priv); > > @@ -4590,6 +4596,11 @@ intel_dp_detect(struct drm_connector *connector, bool > force) > if (status != connector_status_connected) > goto out; > > + if (connector->status == connector_status_connected) { > + DRM_DEBUG_KMS("Connector status is already connected\n"); > + goto out; > + } > + > intel_dp_probe_oui(intel_dp); > > if (intel_dp->force_audio != HDMI_AUDIO_AUTO) { > @@ -4604,6 +4615,22 @@ intel_dp_detect(struct drm_connector *connector, bool > force) > > if (intel_encoder->type != INTEL_OUTPUT_EDP) > intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; > + > + if (IS_CHERRYVIEW(dev) && > + intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) { > + /* Handle connected boot scenario where panel is enabled > + * by GOP/VBIOS. > + */ > + if (intel_encoder->connectors_active && > + crtc && crtc->enabled) { > + intel_crtc = to_intel_crtc(crtc); > + DRM_DEBUG_KMS("Disabling crtc %c for upfront link training\n", > + pipe_name(intel_crtc->pipe)); > + intel_crtc_control(crtc, false); > + } > + chv_upfront_link_train(dev, intel_dp, intel_crtc); > + } > + > status = connector_status_connected; > > out: > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index fc266da..15eef94d 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -991,6 +991,10 @@ void intel_dp_start_link_train(struct intel_dp > *intel_dp); > void intel_dp_complete_link_train(struct intel_dp *intel_dp); > void intel_dp_stop_link_train(struct intel_dp *intel_dp); > bool intel_dp_fast_link_train(struct intel_dp *intel_dp); > +bool chv_upfront_link_train(struct drm_device *dev, > + struct intel_dp *intel_dp, struct intel_crtc *crtc); > +void intel_dp_set_clock(struct intel_encoder *encoder, > + struct intel_crtc_config *pipe_config, int link_bw); > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode); > void intel_dp_set_m2_n2(struct intel_crtc *crtc, struct intel_link_m_n > *m_n); > void intel_dp_encoder_destroy(struct drm_encoder *encoder); > > -- > regards, > Sivakumar > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/