On Thu, Nov 08, 2018 at 06:35:10PM -0500, Lyude Paul wrote:
> lgtm
>
> Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>
Thanks. Pushed to dinq.
>
> 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
--
Ville Syrjälä
Intel
