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

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

[Ville Syrjala]

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;
 		}
 
-- 
2.18.1

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

[Lyude Paul]

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;
>  		}
>  
-- 
Cheers,
	Lyude Paul

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

[Ville Syrjälä]

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

-- 
Ville Syrj�l�
Intel