Hi Ander, Thanks for looking at it. Few queries below.. > -----Original Message----- > From: Ander Conselvan De Oliveira [mailto:conselvan2@xxxxxxxxx] > Sent: Friday, May 6, 2016 6:39 PM > To: R, Durgadoss <durgadoss.r@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCHv4 3/3] drm/i915/dp: Enable Upfront link > training for typeC DP support on BXT > > 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@xxxxxxxxx> > > --- > > 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 Ok, today that will add one more iteration of Link training before going down to a supported 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. May be with the new {link_bw/lane_count}_upfront variables in intel_dp structure, we could do this easily. Will try this implementation and see how it comes up. > > > + > > + 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. I thought we agreed for having that change as a later implementation. Anyway, it's not a big change; So I will do as part of next revision, probably as a separate patch. > > > +} > > + > > +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. This function needs to be called from two places. One in the commit() below, and the other in the upfront_link_train() itself, when the crtc is not enabled already. > > > + > > + 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 Sure, better name makes sense.. > 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() ? Still this would need to call the above get_crtc_state(..) function. So, I will re-name and if it all looks Ok, will move it to atomic.c Otherwise, will keep it here.. > > > > > +{ > > + 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. Will fix this.. > > > + } > > + 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 Ok, will change. > > > + 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; > Ok, makes sense. Will re-order this way. > > + > > + 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. Ok, will save this earlier. But since we don’t re-enable crtc here after upfront, (we let it get re-enabled as part of subsequent modeset) the restoring with false does not happen today.. > But that would cause intel_crtc->config to be freed, so you need to > use intel_crtc_duplicate_state(). Since we alloc a new state and then only call get_crtc_state(), I thought that automatically boils down to duplicate_state() inside drm_atomic_get_crtc_state(). Please correct if my understanding is wrong. > > > + 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. We let it get re-enabled as part of mode set that happens subsequently. Please let me know your opinion on this. Thanks, Durga > > > > + > > +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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
