Re: [PATCH] drm/i915: Fix hpd handling for pins with two encoders

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;
  

  }