Re: [PATCH 5/7] drm/i915: abstract port hotplug long pulse detection more

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, May 27, 2015 at 02:41:29PM +0200, Daniel Vetter wrote:
> On Wed, May 27, 2015 at 03:03:43PM +0300, Jani Nikula wrote:
> > Add platform specific functions to decipher the register contents
> > instead of just returning the shift value. Use macros instead of magic
> > numbers to look at the register bits.
> > 
> > As a side effect, if an unsupported port is passed, consistently return
> > false (indicating short hotplug) instead of returning -1 which was used
> > for shifting.
> > 
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> Not convinced. Currently we have a strange mix of passing register+decode
> tables to intel_hpd_irq_handler for hpds and decoding the long vs. short
> stuff internally. That already doesn't work with things like port A which
> tends to be (at least on some platforms) in a totally different register.
> And it splits up the definitions for the hpd bits from the long vs. short
> bits unecessarily. I even suspect we might have some bugs with correctly
> clearing those additional status registers sometimes.
> 
> Imo if we clean this up we should pull it all together into one place,
> i.e. platform code does all the register reading&decoding and passes
> decode bitmasks down to generic handlers. Or something along those lines
> at least.

Forgot to add: Merged all the other patches to dinq, thanks.
-Daniel

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 40 ++++++++++++++++------------------------
> >  1 file changed, 16 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 91cb0b6ec47b..90cc248691d7 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1375,35 +1375,31 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
> >  #define HPD_STORM_DETECT_PERIOD 1000
> >  #define HPD_STORM_THRESHOLD 5
> >  
> > -static int pch_port_to_hotplug_shift(enum port port)
> > +static bool pch_port_hotplug_long_detect(enum port port, u32 val)
> >  {
> >  	switch (port) {
> > -	case PORT_A:
> > -	case PORT_E:
> > -	default:
> > -		return -1;
> >  	case PORT_B:
> > -		return 0;
> > +		return val & PORTB_HOTPLUG_LONG_DETECT;
> >  	case PORT_C:
> > -		return 8;
> > +		return val & PORTC_HOTPLUG_LONG_DETECT;
> >  	case PORT_D:
> > -		return 16;
> > +		return val & PORTD_HOTPLUG_LONG_DETECT;
> > +	default:
> > +		return false;
> >  	}
> >  }
> >  
> > -static int i915_port_to_hotplug_shift(enum port port)
> > +static bool i915_port_hotplug_long_detect(enum port port, u32 val)
> >  {
> >  	switch (port) {
> > -	case PORT_A:
> > -	case PORT_E:
> > -	default:
> > -		return -1;
> >  	case PORT_B:
> > -		return 17;
> > +		return val & PORTB_HOTPLUG_INT_LONG_PULSE;
> >  	case PORT_C:
> > -		return 19;
> > +		return val & PORTC_HOTPLUG_INT_LONG_PULSE;
> >  	case PORT_D:
> > -		return 21;
> > +		return val & PORTD_HOTPLUG_INT_LONG_PULSE;
> > +	default:
> > +		return false;
> >  	}
> >  }
> >  
> > @@ -1431,7 +1427,6 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
> >  	enum port port;
> >  	bool storm_detected = false;
> >  	bool queue_dig = false, queue_hp = false;
> > -	u32 dig_shift;
> >  	u32 dig_port_mask = 0;
> >  
> >  	if (!hotplug_trigger)
> > @@ -1451,13 +1446,10 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
> >  		if (!port || !dev_priv->hotplug.irq_port[port])
> >  			continue;
> >  
> > -		if (!HAS_GMCH_DISPLAY(dev_priv)) {
> > -			dig_shift = pch_port_to_hotplug_shift(port);
> > -			long_hpd = (dig_hotplug_reg >> dig_shift) & PORTB_HOTPLUG_LONG_DETECT;
> > -		} else {
> > -			dig_shift = i915_port_to_hotplug_shift(port);
> > -			long_hpd = (hotplug_trigger >> dig_shift) & PORTB_HOTPLUG_LONG_DETECT;
> > -		}
> > +		if (HAS_GMCH_DISPLAY(dev_priv))
> > +			long_hpd = i915_port_hotplug_long_detect(port, hotplug_trigger);
> > +		else
> > +			long_hpd = pch_port_hotplug_long_detect(port, dig_hotplug_reg);
> >  
> >  		DRM_DEBUG_DRIVER("digital hpd port %c - %s\n", port_name(port),
> >  				 long_hpd ? "long" : "short");
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx