On Wed, Nov 09, 2016 at 03:04:05PM +0200, Jani Nikula wrote:
> On Wed, 02 Nov 2016, Manasi Navare <manasi.d.navare@xxxxxxxxx> 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.
> >
> > 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)
> >
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 7 +++
> > drivers/gpu/drm/i915/intel_ddi.c | 22 ++++++++-
> > drivers/gpu/drm/i915/intel_dp.c | 70 ++++++++++++++++++++++++---
> > drivers/gpu/drm/i915/intel_dp_link_training.c | 12 +++--
> > drivers/gpu/drm/i915/intel_drv.h | 6 ++-
> > 5 files changed, 105 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 75ad01d..94b0b3a 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -519,6 +519,13 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
> > connector_state);
> > if (ret)
> > return ret;
> > +
> > + if (connector->state->crtc) {
> > + crtc_state = drm_atomic_get_existing_crtc_state(state,
> > + connector->state->crtc);
> > + if (connector->link_status == DRM_MODE_LINK_STATUS_BAD)
> > + crtc_state->connectors_changed = true;
> > + }
> > }
>
> Could this be a separate patch? Anyway needs to be posted on dri-devel.
>
Yes I agree, I will move this into a separate patch right after the patch
where I declare the connector link status.
> >
> > /*
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 938ac4d..a59f8d2 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,25 @@ 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);
> > + connector->link_status = DRM_MODE_LINK_STATUS_BAD;
>
> Why does this not use intel_dp_set_link_status_property()?
>
I will move this connector->link_status setting inside the set_link_status function.
> > + 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;
> > + connector->link_status = DRM_MODE_LINK_STATUS_GOOD;
> > + intel_dp_set_link_status_property(connector,
> > + DRM_MODE_LINK_STATUS_GOOD);
> > + }
> > +
> > 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 fb4fcdd..659b17f 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -310,9 +310,6 @@ void intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
> > int common_len;
> > int link_rate_index = -1;
> >
> > - if (!intel_dp->link_train_failed)
> > - return;
> > -
>
> You add this in an earlier patch and now remove it?
>
I removed it to address the comment about not using link_train_failed and
link_status to indicate the same thing. But this will be cleaned up when I submit
a clean patch series so that it does not appear as added in previous patch and
removed here.
> > common_len = intel_dp_common_rates(intel_dp, common_rates);
> > link_rate_index = intel_dp_link_rate_index(intel_dp,
> > common_rates,
> > @@ -327,7 +324,6 @@ void intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
> > intel_dp->fallback_lane_count = lane_count >> 1;
> > } else {
> > DRM_ERROR("Link Training Unsuccessful\n");
> > - intel_dp->link_train_failed = false;
> > }
> > }
> >
> > @@ -354,8 +350,14 @@ void 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);
> > @@ -1593,6 +1595,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);
> > @@ -1640,6 +1643,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],
> > @@ -4423,6 +4432,12 @@ 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;
> > + connector->link_status = DRM_MODE_LINK_STATUS_GOOD;
> > + intel_dp_set_link_status_property(connector,
> > + DRM_MODE_LINK_STATUS_GOOD);
>
> Seems like it would be a good idea to have this
> intel_dp_set_link_status_property() helper set connector->link_status
> too.
>
Yup, will do this change.
Regards
Manasi
> >
> > if (intel_dp->is_mst) {
> > DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
> > @@ -4514,6 +4529,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);
> > @@ -5692,6 +5712,40 @@ 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.
> > + */
> > + connector->link_status = DRM_MODE_LINK_STATUS_BAD;
> > + intel_dp_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)
> > @@ -5704,6 +5758,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;
> > }
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index bc25b2b..c0378a9 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 {
> > @@ -900,7 +903,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;
> > @@ -1402,7 +1404,7 @@ void intel_dp_set_link_params(struct intel_dp *intel_dp,
> > bool link_mst);
> > void 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
