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@xxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Reviewed-by: José Roberto de Souza <jose.souza@xxxxxxxxx> (v1)
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>
> return IRQ_HANDLED;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
