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

[Daniel Vetter <daniel@ffwll.ch>]

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