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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx