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

[Lyude Paul]

lgtm

Reviewed-by: Lyude Paul <lyude at redhat.com>

On Thu, 2018-11-08 at 22:04 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala at 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 at vger.kernel.org # 4.19+
> Cc: Lyude Paul <lyude at redhat.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Fixes: b6ca3eee18ba ("drm/i915: Nuke dev_priv->irq_port[]")
> Signed-off-by: Ville Syrjälä <ville.syrjala at 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