On Thu, Nov 17, 2016 at 02:49:02PM +0200, Jani Nikula wrote: > On Tue, 15 Nov 2016, Manasi Navare <manasi.d.nav...@intel.com> wrote: > > If link training at a link rate optimal for a particular > > mode fails during modeset's atomic commit phase, then we > > let the modeset complete and then retry. We save the link rate > > value at which link training failed, update the link status property > > to "BAD" and use a lower link rate to prune the modes. It will redo > > the modeset on the current mode at lower link rate or if the current > > mode gets pruned due to lower link constraints then, it will send a > > hotplug uevent for userspace to handle it. > > > > This is also required to pass DP CTS tests 4.3.1.3, 4.3.1.4, > > 4.3.1.6. > > > > v5: > > * Move set link status to drm core (Daniel Vetter, Jani Nikula) > > v4: > > * Add fallback support for non DDI platforms too > > * Set connector->link status inside set_link_status function > > (Jani Nikula) > > v3: > > * Set link status property to BAd unconditionally (Jani Nikula) > > * Dont use two separate variables link_train_failed and link_status > > to indicate same thing (Jani Nikula) > > v2: > > * Squashed a few patches (Jani Nikula) > > > > Acked-by: Tony Cheng <tony.ch...@amd.com> > > Acked-by: Harry Wentland <harry.wentl...@amd.com> > > Cc: Jani Nikula <jani.nik...@linux.intel.com> > > Cc: Daniel Vetter <daniel.vet...@intel.com> > > Cc: Ville Syrjala <ville.syrj...@linux.intel.com> > > Signed-off-by: Manasi Navare <manasi.d.nav...@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 21 +++++- > > drivers/gpu/drm/i915/intel_dp.c | 102 > > +++++++++++++++++++++++++- > > drivers/gpu/drm/i915/intel_dp_link_training.c | 12 ++- > > drivers/gpu/drm/i915/intel_drv.h | 6 +- > > 4 files changed, 131 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > b/drivers/gpu/drm/i915/intel_ddi.c > > index 0ad4e16..57b7ee9 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -1684,6 +1684,8 @@ static void intel_ddi_pre_enable_dp(struct > > intel_encoder *encoder, > > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > enum port port = intel_ddi_get_encoder_port(encoder); > > + struct intel_connector *intel_connector = intel_dp->attached_connector; > > + struct drm_connector *connector = &intel_connector->base; > > > > intel_dp_set_link_params(intel_dp, link_rate, lane_count, > > link_mst); > > @@ -1694,7 +1696,24 @@ static void intel_ddi_pre_enable_dp(struct > > intel_encoder *encoder, > > intel_prepare_dp_ddi_buffers(encoder); > > intel_ddi_init_dp_buf_reg(encoder); > > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > > - intel_dp_start_link_train(intel_dp); > > + if (!intel_dp_start_link_train(intel_dp)) { > > + DRM_DEBUG_KMS("Link Training failed at link rate = %d, lane > > count = %d", > > + link_rate, lane_count); > > + if (!intel_dp_get_link_train_fallback_values(intel_dp, > > + link_rate, > > + lane_count)) > > + /* Schedule a Hotplug Uevent to userspace to start > > modeset */ > > + schedule_work(&intel_connector->modeset_retry_work); > > + } else { > > + DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane > > count = %d", > > + link_rate, lane_count); > > + intel_dp->fallback_link_rate_index = -1; > > + intel_dp->fallback_link_rate = 0; > > + intel_dp->fallback_lane_count = 0; > > + drm_mode_connector_set_link_status_property(connector, > > + > > DRM_MODE_LINK_STATUS_GOOD); > > + } > > Code block with intel_dp_start_link_train() failure handling. Okay. > > > + > > if (port != PORT_A || INTEL_GEN(dev_priv) >= 9) > > intel_dp_stop_link_train(intel_dp); > > } > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index e87b451..af2f8b2 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -353,8 +353,14 @@ int intel_dp_get_link_train_fallback_values(struct > > intel_dp *intel_dp, > > target_clock = fixed_mode->clock; > > } > > > > - max_link_clock = intel_dp_max_link_rate(intel_dp); > > - max_lanes = intel_dp_max_lane_count(intel_dp); > > + /* Prune the modes using the fallback link rate/lane count */ > > + if (connector->link_status == DRM_MODE_LINK_STATUS_BAD) { > > + max_link_clock = intel_dp->fallback_link_rate; > > + max_lanes = intel_dp->fallback_lane_count; > > + } else { > > + max_link_clock = intel_dp_max_link_rate(intel_dp); > > + max_lanes = intel_dp_max_lane_count(intel_dp); > > + } > > > > max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes); > > mode_rate = intel_dp_link_required(target_clock, 18); > > @@ -1591,6 +1597,7 @@ static int intel_dp_compute_bpp(struct intel_dp > > *intel_dp, > > enum port port = dp_to_dig_port(intel_dp)->port; > > struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc); > > struct intel_connector *intel_connector = intel_dp->attached_connector; > > + struct drm_connector *connector = &intel_connector->base; > > int lane_count, clock; > > int min_lane_count = 1; > > int max_lane_count = intel_dp_max_lane_count(intel_dp); > > @@ -1638,6 +1645,12 @@ static int intel_dp_compute_bpp(struct intel_dp > > *intel_dp, > > if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) > > return false; > > > > + /* Fall back to lower link rate in case of failure in previous modeset > > */ > > + if (connector->link_status == DRM_MODE_LINK_STATUS_BAD) { > > + min_lane_count = max_lane_count = intel_dp->fallback_lane_count; > > + min_clock = max_clock = intel_dp->fallback_link_rate_index; > > + } > > + > > DRM_DEBUG_KMS("DP link computation with max lane count %i " > > "max bw %d pixel clock %iKHz\n", > > max_lane_count, common_rates[max_clock], > > @@ -2781,6 +2794,8 @@ static void intel_enable_dp(struct intel_encoder > > *encoder, > > struct drm_device *dev = encoder->base.dev; > > struct drm_i915_private *dev_priv = to_i915(dev); > > struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc); > > + struct intel_connector *intel_connector = intel_dp->attached_connector; > > + struct drm_connector *connector = &intel_connector->base; > > uint32_t dp_reg = I915_READ(intel_dp->output_reg); > > enum pipe pipe = crtc->pipe; > > > > @@ -2811,7 +2826,23 @@ static void intel_enable_dp(struct intel_encoder > > *encoder, > > } > > > > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > > - intel_dp_start_link_train(intel_dp); > > + if (!intel_dp_start_link_train(intel_dp)) { > > + DRM_DEBUG_KMS("Link Training failed at link rate = %d, lane > > count = %d", > > + intel_dp->link_rate, intel_dp->lane_count); > > + if (!intel_dp_get_link_train_fallback_values(intel_dp, > > + > > intel_dp->link_rate, > > + > > intel_dp->lane_count)) > > + /* Schedule a Hotplug Uevent to userspace to start > > modeset */ > > + schedule_work(&intel_connector->modeset_retry_work); > > + } else { > > + DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane > > count = %d", > > + intel_dp->link_rate, intel_dp->lane_count); > > + intel_dp->fallback_link_rate_index = -1; > > + intel_dp->fallback_link_rate = 0; > > + intel_dp->fallback_lane_count = 0; > > + drm_mode_connector_set_link_status_property(connector, > > + > > DRM_MODE_LINK_STATUS_GOOD); > > + } > > Second identical copy of intel_dp_start_link_train() failure handling. > > > intel_dp_stop_link_train(intel_dp); > > > > if (pipe_config->has_audio) { > > @@ -4018,6 +4049,8 @@ static void intel_dp_handle_test_request(struct > > intel_dp *intel_dp) > > struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base; > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc); > > + struct intel_connector *intel_connector = intel_dp->attached_connector; > > + struct drm_connector *connector = &intel_connector->base; > > > > /* Suppress underruns caused by re-training */ > > intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false); > > @@ -4025,7 +4058,23 @@ static void intel_dp_handle_test_request(struct > > intel_dp *intel_dp) > > intel_set_pch_fifo_underrun_reporting(dev_priv, > > > > intel_crtc_pch_transcoder(crtc), false); > > > > - intel_dp_start_link_train(intel_dp); > > + if (!intel_dp_start_link_train(intel_dp)) { > > + DRM_DEBUG_KMS("Link Training failed at link rate = %d, lane > > count = %d", > > + intel_dp->link_rate, intel_dp->lane_count); > > + if (!intel_dp_get_link_train_fallback_values(intel_dp, > > + > > intel_dp->link_rate, > > + > > intel_dp->lane_count)) > > + /* Schedule a Hotplug Uevent to userspace to start > > modeset */ > > + schedule_work(&intel_connector->modeset_retry_work); > > + } else { > > + DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane > > count = %d", > > + intel_dp->link_rate, intel_dp->lane_count); > > + intel_dp->fallback_link_rate_index = -1; > > + intel_dp->fallback_link_rate = 0; > > + intel_dp->fallback_lane_count = 0; > > + drm_mode_connector_set_link_status_property(connector, > > + > > DRM_MODE_LINK_STATUS_GOOD); > > + } > > Third identical copy of intel_dp_start_link_train() failure > handling. Please abstract. > > Why don't all intel_dp_start_link_train() calls need this? Could the > above code be *within* intel_dp_start_link_train()? >
Actually the same codeblock got repeated since I expanded this failure handling to all DDI and non DDI platforms and to all cases where link training happens. I think instead of adding to intel_dp_start_link_train, I can create another separate function for link_train_failure handling which can be called at all these places. Having it in a separate function would make it more intuitive that if start_linj_train failed then we are calling this failure handling code block. Let me know what you think. Regards Manasi > > intel_dp_stop_link_train(intel_dp); > > > > /* Keep underrun reporting disabled until things are stable */ > > @@ -4419,6 +4468,11 @@ static bool intel_digital_port_connected(struct > > drm_i915_private *dev_priv, > > intel_dp->compliance_test_active = 0; > > intel_dp->compliance_test_type = 0; > > intel_dp->compliance_test_data = 0; > > + intel_dp->fallback_link_rate_index = -1; > > + intel_dp->fallback_link_rate = 0; > > + intel_dp->fallback_lane_count = 0; > > + drm_mode_connector_set_link_status_property(connector, > > + > > DRM_MODE_LINK_STATUS_GOOD); > > > > if (intel_dp->is_mst) { > > DRM_DEBUG_KMS("MST device may have disappeared %d vs > > %d\n", > > @@ -4510,6 +4564,11 @@ static bool intel_digital_port_connected(struct > > drm_i915_private *dev_priv, > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > > connector->base.id, connector->name); > > > > + /* If this is a retry due to link trianing failure */ > > + if (status == connector_status_connected && > > + connector->link_status == DRM_MODE_LINK_STATUS_BAD) > > + return status; > > + > > /* If full detect is not performed yet, do a full detect */ > > if (!intel_dp->detect_done) > > status = intel_dp_long_pulse(intel_dp->attached_connector); > > @@ -5677,6 +5736,37 @@ static bool intel_edp_init_connector(struct intel_dp > > *intel_dp, > > return false; > > } > > > > +static void intel_dp_modeset_retry_work_fn(struct work_struct *work) > > +{ > > + struct intel_connector *intel_connector; > > + struct drm_connector *connector; > > + struct drm_display_mode *mode; > > + bool verbose_prune = true; > > + > > + intel_connector = container_of(work, typeof(*intel_connector), > > + modeset_retry_work); > > + connector = &intel_connector->base; > > + > > + /* Grab the locks before changing connector property*/ > > + mutex_lock(&connector->dev->mode_config.mutex); > > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, > > + connector->name); > > + list_for_each_entry(mode, &connector->modes, head) { > > + mode->status = intel_dp_mode_valid(connector, > > + mode); > > + } > > + drm_mode_prune_invalid(connector->dev, &connector->modes, > > + verbose_prune); > > + /* Set connector link status to BAD and send a Uevent to notify > > + * userspace to do a modeset. > > + */ > > + drm_mode_connector_set_link_status_property(connector, > > + DRM_MODE_LINK_STATUS_BAD); > > + mutex_unlock(&connector->dev->mode_config.mutex); > > + /* Send Hotplug uevent so userspace can reprobe */ > > + drm_kms_helper_hotplug_event(connector->dev); > > +} > > + > > bool > > intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > > struct intel_connector *intel_connector) > > @@ -5689,6 +5779,10 @@ static bool intel_edp_init_connector(struct intel_dp > > *intel_dp, > > enum port port = intel_dig_port->port; > > int type; > > > > + /* Initialize the work for modeset in case of link train failure */ > > + INIT_WORK(&intel_connector->modeset_retry_work, > > + intel_dp_modeset_retry_work_fn); > > + > > if (WARN(intel_dig_port->max_lanes < 1, > > "Not enough lanes (%d) for DP on port %c\n", > > intel_dig_port->max_lanes, port_name(port))) > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c > > b/drivers/gpu/drm/i915/intel_dp_link_training.c > > index 0048b52..10f81ab 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > > @@ -310,9 +310,15 @@ void intel_dp_stop_link_train(struct intel_dp > > *intel_dp) > > DP_TRAINING_PATTERN_DISABLE); > > } > > > > -void > > +bool > > intel_dp_start_link_train(struct intel_dp *intel_dp) > > { > > - intel_dp_link_training_clock_recovery(intel_dp); > > - intel_dp_link_training_channel_equalization(intel_dp); > > + bool ret; > > + > > + if (intel_dp_link_training_clock_recovery(intel_dp)) { > > + ret = intel_dp_link_training_channel_equalization(intel_dp); > > + if (ret) > > + return true; > > + } > > + return false; > > Usually happy day scenario goes at the top indentation level, and errors > are handled in the nested cases with early returns. Compare with: > > intel_dp_start_link_train(struct intel_dp *intel_dp) > { > if (!intel_dp_link_training_clock_recovery(intel_dp)) > return false; > > if (!intel_dp_link_training_channel_equalization(intel_dp)) > return false; > > return true; > } > Agreed. > > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index 55ceb44..88918f4 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -312,6 +312,9 @@ struct intel_connector { > > void *port; /* store this opaque as its illegal to dereference it */ > > > > struct intel_dp *mst_port; > > + > > + /* Work struct to schedule a uevent on link train failure */ > > + struct work_struct modeset_retry_work; > > }; > > > > struct dpll { > > @@ -890,7 +893,6 @@ struct intel_dp { > > int fallback_link_rate; > > uint8_t fallback_lane_count; > > int fallback_link_rate_index; > > - bool link_train_failed; > > uint8_t sink_count; > > bool link_mst; > > bool has_audio; > > @@ -1389,7 +1391,7 @@ void intel_dp_set_link_params(struct intel_dp > > *intel_dp, > > bool link_mst); > > int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > int link_rate, uint8_t lane_count); > > -void intel_dp_start_link_train(struct intel_dp *intel_dp); > > +bool 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); > > void intel_dp_encoder_reset(struct drm_encoder *encoder); > > -- > Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx