[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
Mon Jan 11 09:51:15 PST 2016
>-----Original Message-----
>From: Ander Conselvan De Oliveira [mailto:conselvan2 at gmail.com]
>Sent: Monday, January 11, 2016 7:40 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 Tue, 2015-12-29 at 18:50 +0000, R, Durgadoss wrote:
>> 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.
>
>One idea would be split the part of intel_get_shared_dpll() that needs atomic
>interfaces to a separate function. Then it would be possible to add a second
>entry point that just takes crtc, encoder and the pll parameters and calls the
>logic that doesn't depend on the atomic state.
>
>Then you could move the calls to intel_get_shared_dpll() from *_ddi_pll_select()
>to haswell_crtc_compute_clock() so they don't get on the way during upfront link
>training, where the new non-atomic entry point is called instead.
Ok, Will try this implementation.
Thanks,
Durga
>
>Ander
>
>>
>> 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