| Message ID | 20181108200424.28371-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix hpd handling for pins with two encoders | expand |
lgtm Reviewed-by: Lyude Paul <lyude@redhat.com> On Thu, 2018-11-08 at 22:04 +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > 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@vger.kernel.org # 4.19+ > Cc: Lyude Paul <lyude@redhat.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Fixes: b6ca3eee18ba ("drm/i915: Nuke dev_priv->irq_port[]") > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > 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; > } >
On Thu, Nov 08, 2018 at 06:35:10PM -0500, Lyude Paul wrote: > lgtm > > Reviewed-by: Lyude Paul <lyude@redhat.com> Thanks. Pushed to dinq. > > On Thu, 2018-11-08 at 22:04 +0200, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > 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@vger.kernel.org # 4.19+ > > Cc: Lyude Paul <lyude@redhat.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Fixes: b6ca3eee18ba ("drm/i915: Nuke dev_priv->irq_port[]") > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > 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
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; }