Message ID | 20200605094801.17709-1-imre.deak@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [RESEND,v3,1/3] drm/i915/dp_mst: Fix disabling MST on a port | expand |
On Fri, 2020-06-05 at 12:48 +0300, Imre Deak wrote: > Currently MST on a port can get enabled/disabled from the hotplug work > and get disabled from the short pulse work in a racy way. Fix this by > relying on the MST state checking in the hotplug work and just schedule > a hotplug work from the short pulse handler if some problem happened > during the MST interrupt handling. > > This removes the explicit MST disabling in case of an AUX failure, but > if AUX fails, then probably the detection will also fail during the > scheduled hotplug work and it's not guaranteed that we'll see > intermittent errors anyway. > > While at it also simplify the error checking of the MST interrupt > handler. > > v2: > - Convert intel_dp_check_mst_status() to return bool. (Ville) > - Change the intel_dp->is_mst check to an assert, since after this patch > the condition can't change after we checked it previously. > - Document the return value from intel_dp_check_mst_status(). > v3: > - Remove the intel_dp->is_mst check from intel_dp_check_mst_status(). > There is no point in checking the same condition twice, even though > there is a chance that the hotplug work running concurrently changes > it. > > Cc: José Roberto de Souza <jose.souza@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Reviewed-by: José Roberto de Souza <jose.souza@intel.com> (v1) > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 66 ++++++++++--------------- > 1 file changed, 26 insertions(+), 40 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 55fda074c0ad..42589cae766d 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -5556,35 +5556,46 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) > "Could not write test response to sink\n"); > } > > -static int > +/** > + * intel_dp_check_mst_status - service any pending MST interrupts, check link status > + * @intel_dp: Intel DP struct > + * > + * Read any pending MST interrupts, call MST core to handle these and ack the > + * interrupts. Check if the main and AUX link state is ok. > + * > + * Returns: > + * - %true if pending interrupts were serviced (or no interrupts were > + * pending) w/o detecting an error condition. > + * - %false if an error condition - like AUX failure or a loss of link - is > + * detected, which needs servicing from the hotplug work. > + */ > +static bool > intel_dp_check_mst_status(struct intel_dp *intel_dp) > { > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > - bool need_retrain = false; > - > - if (!intel_dp->is_mst) > - return -EINVAL; > + bool link_ok = true; > > drm_WARN_ON_ONCE(&i915->drm, intel_dp->active_mst_links < 0); > > for (;;) { > u8 esi[DP_DPRX_ESI_LEN] = {}; > - bool bret, handled; > + bool handled; > int retry; > > - bret = intel_dp_get_sink_irq_esi(intel_dp, esi); > - if (!bret) { > + if (!intel_dp_get_sink_irq_esi(intel_dp, esi)) { > drm_dbg_kms(&i915->drm, > "failed to get ESI - device may have failed\n"); > - return -EINVAL; > + link_ok = false; > + > + break; > } > > /* check link status - esi[10] = 0x200c */ > - if (intel_dp->active_mst_links > 0 && !need_retrain && > + if (intel_dp->active_mst_links > 0 && link_ok && > !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) { > drm_dbg_kms(&i915->drm, > "channel EQ not ok, retraining\n"); > - need_retrain = true; > + link_ok = false; > } > > drm_dbg_kms(&i915->drm, "got esi %3ph\n", esi); > @@ -5604,7 +5615,7 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp) > } > } > > - return need_retrain; > + return link_ok; > } > > static bool > @@ -7255,35 +7266,10 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) > } > > if (intel_dp->is_mst) { > - switch (intel_dp_check_mst_status(intel_dp)) { > - case -EINVAL: > - /* > - * If we were in MST mode, and device is not > - * there, get out of MST mode > - */ > - drm_dbg_kms(&i915->drm, > - "MST device may have disappeared %d vs %d\n", > - intel_dp->is_mst, > - intel_dp->mst_mgr.mst_state); > - intel_dp->is_mst = false; > - drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, > - intel_dp->is_mst); > - > - return IRQ_NONE; > - case 1: > - return IRQ_NONE; > - default: > - break; > - } > - } > - > - if (!intel_dp->is_mst) { > - bool handled; > - > - handled = intel_dp_short_pulse(intel_dp); > - > - if (!handled) > + if (!intel_dp_check_mst_status(intel_dp)) > return IRQ_NONE; > + } else if (!intel_dp_short_pulse(intel_dp)) { > + return IRQ_NONE; > } > Now it don't need the braces but this is minor. Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > return IRQ_HANDLED;
On Mon, Jun 08, 2020 at 01:11:44AM +0300, Souza, Jose wrote: > On Fri, 2020-06-05 at 12:48 +0300, Imre Deak wrote: > > Currently MST on a port can get enabled/disabled from the hotplug work > > and get disabled from the short pulse work in a racy way. Fix this by > > relying on the MST state checking in the hotplug work and just schedule > > a hotplug work from the short pulse handler if some problem happened > > during the MST interrupt handling. > > > > This removes the explicit MST disabling in case of an AUX failure, but > > if AUX fails, then probably the detection will also fail during the > > scheduled hotplug work and it's not guaranteed that we'll see > > intermittent errors anyway. > > > > While at it also simplify the error checking of the MST interrupt > > handler. > > > > v2: > > - Convert intel_dp_check_mst_status() to return bool. (Ville) > > - Change the intel_dp->is_mst check to an assert, since after this patch > > the condition can't change after we checked it previously. > > - Document the return value from intel_dp_check_mst_status(). > > v3: > > - Remove the intel_dp->is_mst check from intel_dp_check_mst_status(). > > There is no point in checking the same condition twice, even though > > there is a chance that the hotplug work running concurrently changes > > it. > > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Reviewed-by: José Roberto de Souza <jose.souza@intel.com> (v1) > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dp.c | 66 ++++++++++--------------- > > 1 file changed, 26 insertions(+), 40 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > index 55fda074c0ad..42589cae766d 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -5556,35 +5556,46 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) > > "Could not write test response to sink\n"); > > } > > > > -static int > > +/** > > + * intel_dp_check_mst_status - service any pending MST interrupts, check link status > > + * @intel_dp: Intel DP struct > > + * > > + * Read any pending MST interrupts, call MST core to handle these and ack the > > + * interrupts. Check if the main and AUX link state is ok. > > + * > > + * Returns: > > + * - %true if pending interrupts were serviced (or no interrupts were > > + * pending) w/o detecting an error condition. > > + * - %false if an error condition - like AUX failure or a loss of link - is > > + * detected, which needs servicing from the hotplug work. > > + */ > > +static bool > > intel_dp_check_mst_status(struct intel_dp *intel_dp) > > { > > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > - bool need_retrain = false; > > - > > - if (!intel_dp->is_mst) > > - return -EINVAL; > > + bool link_ok = true; > > > > drm_WARN_ON_ONCE(&i915->drm, intel_dp->active_mst_links < 0); > > > > for (;;) { > > u8 esi[DP_DPRX_ESI_LEN] = {}; > > - bool bret, handled; > > + bool handled; > > int retry; > > > > - bret = intel_dp_get_sink_irq_esi(intel_dp, esi); > > - if (!bret) { > > + if (!intel_dp_get_sink_irq_esi(intel_dp, esi)) { > > drm_dbg_kms(&i915->drm, > > "failed to get ESI - device may have failed\n"); > > - return -EINVAL; > > + link_ok = false; > > + > > + break; > > } > > > > /* check link status - esi[10] = 0x200c */ > > - if (intel_dp->active_mst_links > 0 && !need_retrain && > > + if (intel_dp->active_mst_links > 0 && link_ok && > > !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) { > > drm_dbg_kms(&i915->drm, > > "channel EQ not ok, retraining\n"); > > - need_retrain = true; > > + link_ok = false; > > } > > > > drm_dbg_kms(&i915->drm, "got esi %3ph\n", esi); > > @@ -5604,7 +5615,7 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp) > > } > > } > > > > - return need_retrain; > > + return link_ok; > > } > > > > static bool > > @@ -7255,35 +7266,10 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) > > } > > > > if (intel_dp->is_mst) { > > - switch (intel_dp_check_mst_status(intel_dp)) { > > - case -EINVAL: > > - /* > > - * If we were in MST mode, and device is not > > - * there, get out of MST mode > > - */ > > - drm_dbg_kms(&i915->drm, > > - "MST device may have disappeared %d vs %d\n", > > - intel_dp->is_mst, > > - intel_dp->mst_mgr.mst_state); > > - intel_dp->is_mst = false; > > - drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, > > - intel_dp->is_mst); > > - > > - return IRQ_NONE; > > - case 1: > > - return IRQ_NONE; > > - default: > > - break; > > - } > > - } > > - > > - if (!intel_dp->is_mst) { > > - bool handled; > > - > > - handled = intel_dp_short_pulse(intel_dp); > > - > > - if (!handled) > > + if (!intel_dp_check_mst_status(intel_dp)) > > return IRQ_NONE; > > + } else if (!intel_dp_short_pulse(intel_dp)) { > > + return IRQ_NONE; > > } > > > > Now it don't need the braces but this is minor. Without the braces it wouldn't be correct. > > Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > > > return IRQ_HANDLED;
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 55fda074c0ad..42589cae766d 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -5556,35 +5556,46 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) "Could not write test response to sink\n"); } -static int +/** + * intel_dp_check_mst_status - service any pending MST interrupts, check link status + * @intel_dp: Intel DP struct + * + * Read any pending MST interrupts, call MST core to handle these and ack the + * interrupts. Check if the main and AUX link state is ok. + * + * Returns: + * - %true if pending interrupts were serviced (or no interrupts were + * pending) w/o detecting an error condition. + * - %false if an error condition - like AUX failure or a loss of link - is + * detected, which needs servicing from the hotplug work. + */ +static bool intel_dp_check_mst_status(struct intel_dp *intel_dp) { struct drm_i915_private *i915 = dp_to_i915(intel_dp); - bool need_retrain = false; - - if (!intel_dp->is_mst) - return -EINVAL; + bool link_ok = true; drm_WARN_ON_ONCE(&i915->drm, intel_dp->active_mst_links < 0); for (;;) { u8 esi[DP_DPRX_ESI_LEN] = {}; - bool bret, handled; + bool handled; int retry; - bret = intel_dp_get_sink_irq_esi(intel_dp, esi); - if (!bret) { + if (!intel_dp_get_sink_irq_esi(intel_dp, esi)) { drm_dbg_kms(&i915->drm, "failed to get ESI - device may have failed\n"); - return -EINVAL; + link_ok = false; + + break; } /* check link status - esi[10] = 0x200c */ - if (intel_dp->active_mst_links > 0 && !need_retrain && + if (intel_dp->active_mst_links > 0 && link_ok && !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) { drm_dbg_kms(&i915->drm, "channel EQ not ok, retraining\n"); - need_retrain = true; + link_ok = false; } drm_dbg_kms(&i915->drm, "got esi %3ph\n", esi); @@ -5604,7 +5615,7 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp) } } - return need_retrain; + return link_ok; } static bool @@ -7255,35 +7266,10 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) } if (intel_dp->is_mst) { - switch (intel_dp_check_mst_status(intel_dp)) { - case -EINVAL: - /* - * If we were in MST mode, and device is not - * there, get out of MST mode - */ - drm_dbg_kms(&i915->drm, - "MST device may have disappeared %d vs %d\n", - intel_dp->is_mst, - intel_dp->mst_mgr.mst_state); - intel_dp->is_mst = false; - drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, - intel_dp->is_mst); - - return IRQ_NONE; - case 1: - return IRQ_NONE; - default: - break; - } - } - - if (!intel_dp->is_mst) { - bool handled; - - handled = intel_dp_short_pulse(intel_dp); - - if (!handled) + if (!intel_dp_check_mst_status(intel_dp)) return IRQ_NONE; + } else if (!intel_dp_short_pulse(intel_dp)) { + return IRQ_NONE; } return IRQ_HANDLED;