lgtm
Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>
On Thu, 2018-11-08 at 22:04 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> In my haste to remove irq_port[] I accidentally changed the
> way we deal with hpd pins that are shared by multiple encoders
> (DP and HDMI for pre-DDI platforms). Previously we would only
> handle such pins via ->hpd_pulse(), but now we queue up the
> hotplug work for the HDMI encoder directly. Worse yet, we now
> count each hpd twice and this increment the hpd storm count
> twice as fast. This can lead to spurious storms being detected.
>
> Go back to the old way of doing things, ie. delegate to
> ->hpd_pulse() for any pin which has an encoder with that hook
> implemented. I don't really like the idea of adding irq_port[]
> back so let's loop through the encoders first to check if we
> have an encoder with ->hpd_pulse() for the pin, and then go
> through all the pins and decided on the correct course of action
> based on the earlier findings.
>
> I have occasionally toyed with the idea of unifying the pre-DDI
> HDMI and DP encoders into a single encoder as well. Besides the
> hotplug processing it would have the other benefit of preventing
> userspace from trying to enable both encoders at the same time.
> That is simply illegal as they share the same clock/data pins.
> We have some testcases that will attempt that and thus fail on
> many older machines. But for now let's stick to fixing just the
> hotplug code.
>
> Cc: stable@xxxxxxxxxxxxxxx # 4.19+
> Cc: Lyude Paul <lyude@xxxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Fixes: b6ca3eee18ba ("drm/i915: Nuke dev_priv->irq_port[]")
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_hotplug.c | 55 +++++++++++++++++++++-------
> 1 file changed, 42 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c
> b/drivers/gpu/drm/i915/intel_hotplug.c
> index 42e61e10f517..e24174d08fed 100644
> --- a/drivers/gpu/drm/i915/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> @@ -414,33 +414,54 @@ void intel_hpd_irq_handler(struct drm_i915_private
> *dev_priv,
> struct intel_encoder *encoder;
> bool storm_detected = false;
> bool queue_dig = false, queue_hp = false;
> + u32 long_hpd_pulse_mask = 0;
> + u32 short_hpd_pulse_mask = 0;
> + enum hpd_pin pin;
>
> if (!pin_mask)
> return;
>
> spin_lock(&dev_priv->irq_lock);
> +
> + /*
> + * Determine whether ->hpd_pulse() exists for each pin, and
> + * whether we have a short or a long pulse. This is needed
> + * as each pin may have up to two encoders (HDMI and DP) and
> + * only the one of them (DP) will have ->hpd_pulse().
> + */
> for_each_intel_encoder(&dev_priv->drm, encoder) {
> - enum hpd_pin pin = encoder->hpd_pin;
> bool has_hpd_pulse = intel_encoder_has_hpd_pulse(encoder);
> - bool long_hpd = true;
> + enum port port = encoder->port;
> + bool long_hpd;
>
> + pin = encoder->hpd_pin;
> if (!(BIT(pin) & pin_mask))
> continue;
>
> - if (has_hpd_pulse) {
> - enum port port = encoder->port;
> + if (!has_hpd_pulse)
> + continue;
>
> - long_hpd = long_mask & BIT(pin);
> + long_hpd = long_mask & BIT(pin);
>
> - DRM_DEBUG_DRIVER("digital hpd port %c - %s\n",
> port_name(port),
> - long_hpd ? "long" : "short");
> - queue_dig = true;
> - if (long_hpd)
> - dev_priv->hotplug.long_port_mask |= (1 <<
> port);
> - else
> - dev_priv->hotplug.short_port_mask |= (1 <<
> port);
> + DRM_DEBUG_DRIVER("digital hpd port %c - %s\n",
> port_name(port),
> + long_hpd ? "long" : "short");
> + queue_dig = true;
>
> + if (long_hpd) {
> + long_hpd_pulse_mask |= BIT(pin);
> + dev_priv->hotplug.long_port_mask |= BIT(port);
> + } else {
> + short_hpd_pulse_mask |= BIT(pin);
> + dev_priv->hotplug.short_port_mask |= BIT(port);
> }
> + }
> +
> + /* Now process each pin just once */
> + for_each_hpd_pin(pin) {
> + bool long_hpd;
> +
> + if (!(BIT(pin) & pin_mask))
> + continue;
>
> if (dev_priv->hotplug.stats[pin].state == HPD_DISABLED) {
> /*
> @@ -457,8 +478,16 @@ void intel_hpd_irq_handler(struct drm_i915_private
> *dev_priv,
> if (dev_priv->hotplug.stats[pin].state != HPD_ENABLED)
> continue;
>
> - if (!has_hpd_pulse) {
> + /*
> + * Delegate to ->hpd_pulse() if one of the encoders for this
> + * pin has it, otherwise let the hotplug_work deal with this
> + * pin directly.
> + */
> + if (((short_hpd_pulse_mask | long_hpd_pulse_mask) & BIT(pin)))
> {
> + long_hpd = long_hpd_pulse_mask & BIT(pin);
> + } else {
> dev_priv->hotplug.event_bits |= BIT(pin);
> + long_hpd = true;
> queue_hp = true;
> }
>
--
Cheers,
Lyude Paul
