| Message ID | 16ca48b1e74c618929245e9a085b9e3483c3a16d.1491485983.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jani, This patch has the necessary ACKs and R-bs, anything else blocking this from getting merged? Regards Manasi On Thu, Apr 06, 2017 at 04:44:19PM +0300, Jani Nikula wrote: > From: Manasi Navare <manasi.d.navare@intel.com> > > 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. > > This patch is a resend of the original commit id (233ce881dd91fb > "drm/i915: Implement Link Rate fallback on Link training failure") > which got reverted in this commit id (afc1ebf4562a14 Revert > "drm/i915: Implement Link Rate fallback on Link training failure") > due to CI failures. > > After investigating the CI failures it was found that these > were essentially the failures which were always there but hidden because > they used to be DRM_DEBUG_KMS messages for link failures so never got > caught by CI. But now this patch actually throws DRM_ERROR if the link > training fails at RBR and 1 lane. So it caught these link train failures. > > There were two failures: > 1. On SKL 6700k this was because the machine in CI lab is a SKL desktop > without eDP on Port A. But our VBT initialization code in the driver writes > VBT defaults in a way that it always sets DP flag on Port A and this does > not get cleared after parsing the VBT outputs. This has been fixed in > commit id (bb1d132935c2f8 "drm/i915/vbt: split out defaults that are set > when there is no VBT) and (665788572c6410b "drm/i915/vbt: don't propagate > errors from intel_bios_init()) > > 2. On ILK-650 desktop - This was happening because of a bad monitor desktop > combination. I switched the monitor in the CI lab and that helped get rid > of the link failures on ILK system. > > v10: > * Rebase on drm-tip and resend after revert > v9: > * Use the trimmed max values of link rate/lane count based on > link train fallback (Daniel Vetter) > v8: > * Set link_status to BAD first and then call mode_valid (Jani Nikula) > v7: > Remove the redundant variable in previous patch itself > v6: > * Obtain link rate index from fallback_link_rate using > the helper intel_dp_link_rate_index (Jani Nikula) > * Include fallback within intel_dp_start_link_train (Jani Nikula) > 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.cheng@amd.com> > Acked-by: Harry Wentland <Harry.wentland@amd.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > Reviewed-by: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 27 +++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_dp_link_training.c | 22 ++++++++++++++++++++-- > drivers/gpu/drm/i915/intel_drv.h | 3 +++ > 3 files changed, 50 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 6915d89a238c..f2d792a1b18e 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -5977,6 +5977,29 @@ intel_dp_init_connector_port_info(struct intel_digital_port *intel_dig_port) > } > } > > +static void intel_dp_modeset_retry_work_fn(struct work_struct *work) > +{ > + struct intel_connector *intel_connector; > + struct drm_connector *connector; > + > + intel_connector = container_of(work, typeof(*intel_connector), > + modeset_retry_work); > + connector = &intel_connector->base; > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, > + connector->name); > + > + /* Grab the locks before changing connector property*/ > + mutex_lock(&connector->dev->mode_config.mutex); > + /* 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) > @@ -5989,6 +6012,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > 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 694ad0ffb523..b79c1c0e404c 100644 > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > @@ -314,6 +314,24 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp) > void > 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); > + struct intel_connector *intel_connector = intel_dp->attached_connector; > + > + if (!intel_dp_link_training_clock_recovery(intel_dp)) > + goto failure_handling; > + if (!intel_dp_link_training_channel_equalization(intel_dp)) > + goto failure_handling; > + > + DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane count = %d", > + intel_dp->link_rate, intel_dp->lane_count); > + return; > + > + failure_handling: > + 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); > + return; > } > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 5f6e1aad909d..d36026eff7cd 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -321,6 +321,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 { > -- > 2.1.4 >
On Thu, 13 Apr 2017, Manasi Navare <manasi.d.navare@intel.com> wrote: > Hi Jani, > > This patch has the necessary ACKs and R-bs, anything else > blocking this from getting merged? Pushed, finally, sorry for the delay. BR, Jani. > > Regards > Manasi > > On Thu, Apr 06, 2017 at 04:44:19PM +0300, Jani Nikula wrote: >> From: Manasi Navare <manasi.d.navare@intel.com> >> >> 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. >> >> This patch is a resend of the original commit id (233ce881dd91fb >> "drm/i915: Implement Link Rate fallback on Link training failure") >> which got reverted in this commit id (afc1ebf4562a14 Revert >> "drm/i915: Implement Link Rate fallback on Link training failure") >> due to CI failures. >> >> After investigating the CI failures it was found that these >> were essentially the failures which were always there but hidden because >> they used to be DRM_DEBUG_KMS messages for link failures so never got >> caught by CI. But now this patch actually throws DRM_ERROR if the link >> training fails at RBR and 1 lane. So it caught these link train failures. >> >> There were two failures: >> 1. On SKL 6700k this was because the machine in CI lab is a SKL desktop >> without eDP on Port A. But our VBT initialization code in the driver writes >> VBT defaults in a way that it always sets DP flag on Port A and this does >> not get cleared after parsing the VBT outputs. This has been fixed in >> commit id (bb1d132935c2f8 "drm/i915/vbt: split out defaults that are set >> when there is no VBT) and (665788572c6410b "drm/i915/vbt: don't propagate >> errors from intel_bios_init()) >> >> 2. On ILK-650 desktop - This was happening because of a bad monitor desktop >> combination. I switched the monitor in the CI lab and that helped get rid >> of the link failures on ILK system. >> >> v10: >> * Rebase on drm-tip and resend after revert >> v9: >> * Use the trimmed max values of link rate/lane count based on >> link train fallback (Daniel Vetter) >> v8: >> * Set link_status to BAD first and then call mode_valid (Jani Nikula) >> v7: >> Remove the redundant variable in previous patch itself >> v6: >> * Obtain link rate index from fallback_link_rate using >> the helper intel_dp_link_rate_index (Jani Nikula) >> * Include fallback within intel_dp_start_link_train (Jani Nikula) >> 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.cheng@amd.com> >> Acked-by: Harry Wentland <Harry.wentland@amd.com> >> Cc: Jani Nikula <jani.nikula@linux.intel.com> >> Cc: Daniel Vetter <daniel.vetter@intel.com> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> >> Reviewed-by: Jani Nikula <jani.nikula@intel.com> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 27 +++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_dp_link_training.c | 22 ++++++++++++++++++++-- >> drivers/gpu/drm/i915/intel_drv.h | 3 +++ >> 3 files changed, 50 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 6915d89a238c..f2d792a1b18e 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -5977,6 +5977,29 @@ intel_dp_init_connector_port_info(struct intel_digital_port *intel_dig_port) >> } >> } >> >> +static void intel_dp_modeset_retry_work_fn(struct work_struct *work) >> +{ >> + struct intel_connector *intel_connector; >> + struct drm_connector *connector; >> + >> + intel_connector = container_of(work, typeof(*intel_connector), >> + modeset_retry_work); >> + connector = &intel_connector->base; >> + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, >> + connector->name); >> + >> + /* Grab the locks before changing connector property*/ >> + mutex_lock(&connector->dev->mode_config.mutex); >> + /* 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) >> @@ -5989,6 +6012,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, >> 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 694ad0ffb523..b79c1c0e404c 100644 >> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c >> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c >> @@ -314,6 +314,24 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp) >> void >> 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); >> + struct intel_connector *intel_connector = intel_dp->attached_connector; >> + >> + if (!intel_dp_link_training_clock_recovery(intel_dp)) >> + goto failure_handling; >> + if (!intel_dp_link_training_channel_equalization(intel_dp)) >> + goto failure_handling; >> + >> + DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane count = %d", >> + intel_dp->link_rate, intel_dp->lane_count); >> + return; >> + >> + failure_handling: >> + 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); >> + return; >> } >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 5f6e1aad909d..d36026eff7cd 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -321,6 +321,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 { >> -- >> 2.1.4 >>
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 6915d89a238c..f2d792a1b18e 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -5977,6 +5977,29 @@ intel_dp_init_connector_port_info(struct intel_digital_port *intel_dig_port) } } +static void intel_dp_modeset_retry_work_fn(struct work_struct *work) +{ + struct intel_connector *intel_connector; + struct drm_connector *connector; + + intel_connector = container_of(work, typeof(*intel_connector), + modeset_retry_work); + connector = &intel_connector->base; + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, + connector->name); + + /* Grab the locks before changing connector property*/ + mutex_lock(&connector->dev->mode_config.mutex); + /* 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) @@ -5989,6 +6012,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, 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 694ad0ffb523..b79c1c0e404c 100644 --- a/drivers/gpu/drm/i915/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c @@ -314,6 +314,24 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp) void 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); + struct intel_connector *intel_connector = intel_dp->attached_connector; + + if (!intel_dp_link_training_clock_recovery(intel_dp)) + goto failure_handling; + if (!intel_dp_link_training_channel_equalization(intel_dp)) + goto failure_handling; + + DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane count = %d", + intel_dp->link_rate, intel_dp->lane_count); + return; + + failure_handling: + 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); + return; } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 5f6e1aad909d..d36026eff7cd 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -321,6 +321,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 {