[Intel-gfx] [PATCH 6/7] drm/i915: abstract away platform specific parts from hpd handling
Paulo Zanoni
przanoni at gmail.com
Thu May 28 12:08:42 PDT 2015
2015-05-28 9:43 GMT-03:00 Jani Nikula <jani.nikula at intel.com>:
> Split intel_hpd_irq_handler into platforms specific and platform
> agnostic parts. The platform specific parts decode the registers into
> information about which hpd pins triggered, and if they were long
> pulses. The platform agnostic parts do further processing, such as
> interrupt storm mitigation and scheduling bottom halves.
>
> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> ---
> drivers/gpu/drm/i915/i915_irq.c | 147 +++++++++++++++++++++++++++-------------
> 1 file changed, 101 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 6fffbfd3121a..d401c863aeee 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)
We could try to think on something better than just "val". IMHO all
the variable names for the whole hotplug code on this file feel weird.
> {
> 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;
How about if we at least DRM_DEBUG_KMS() in case the short bit is not
1 for these "valid" ports? But I would prefer DRM_ERROR. This
can/should be a separate patch.
> + default:
> + return false;
> }
> }
>
> -static int i915_port_to_hotplug_shift(enum port port)
> +static bool i9xx_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;
> }
> }
>
> @@ -1421,43 +1417,96 @@ static enum port get_port_from_pin(enum hpd_pin pin)
> }
> }
>
> +/* Get a bit mask of pins that have triggered, and which ones may be long. */
> +static void pch_get_hpd_pins(u32 *pin_mask, u32 *long_mask,
> + u32 hotplug_trigger, u32 dig_hotplug_reg, const u32 hpd[HPD_NUM_PINS])
<insert even louder complaints about going past 80 columns>
> +{
> + int i;
> +
> + *pin_mask = 0;
> + *long_mask = 0;
> +
> + if (!hotplug_trigger)
> + return;
> +
> + for_each_hpd_pin(i) {
> + if (hpd[i] & hotplug_trigger) {
> + *pin_mask |= BIT(i);
> +
> + if (pch_port_hotplug_long_detect(get_port_from_pin(i), dig_hotplug_reg))
> + *long_mask |= BIT(i);
> + }
> + }
> +
> + DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x, dig 0x%08x, pins 0x%08x\n",
> + hotplug_trigger, dig_hotplug_reg, *pin_mask);
> +
> +}
> +
> +/* Get a bit mask of pins that have triggered, and which ones may be long. */
> +static void i9xx_get_hpd_pins(u32 *pin_mask, u32 *long_mask,
> + u32 hotplug_trigger, const u32 hpd[HPD_NUM_PINS])
> +{
> + int i;
> +
> + *pin_mask = 0;
> + *long_mask = 0;
> +
> + if (!hotplug_trigger)
> + return;
> +
> + for_each_hpd_pin(i) {
> + if (hpd[i] & hotplug_trigger) {
> + *pin_mask |= BIT(i);
> +
> + if (i9xx_port_hotplug_long_detect(get_port_from_pin(i), hotplug_trigger))
> + *long_mask |= BIT(i);
> + }
> + }
> +
> + DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x, pins 0x%08x\n",
> + hotplug_trigger, *pin_mask);
The fact that this is mostly duplicated code feels a little bit weird.
I won't be surprised if some random person writes a patch to
deduplicate this in the future. I could even give it a R-B.
> +}
> +
> +/**
> + * intel_hpd_irq_handler - main hotplug irq handler
> + * @dev: drm device
> + * @pin_mask: a mask of hpd pins that have triggered the irq
> + * @long_mask: a mask of hpd pins that may be long hpd pulses
> + *
> + * This is the main hotplug irq handler for all platforms. The platform specific
> + * irq handlers call the platform specific hotplug irq handlers, which read and
> + * decode the appropriate registers into bitmasks about hpd pins that have
> + * triggered (@pin_mask), and which of those pins may be long pulses
> + * (@long_mask). The @long_mask is ignored if the port corresponding to the pin
> + * is not a digital port.
Nice!
> + *
> + * Here, we do hotplug irq storm detection and mitigation, and pass further
> + * processing to appropriate bottom halves.
Now you can extract the code that does the storm detection and put it
on its own function :)
intel_hpd_storm_pin_update() or something else...
Besides all the bikeshedding and suggestions for "part III", the code
looks correct, and more organized, so with or without changes:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> + */
> static void intel_hpd_irq_handler(struct drm_device *dev,
> - u32 hotplug_trigger,
> - u32 dig_hotplug_reg,
> - const u32 hpd[HPD_NUM_PINS])
> + u32 pin_mask, u32 long_mask)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> int i;
> enum port port;
> bool storm_detected = false;
> bool queue_dig = false, queue_hp = false;
> - u32 dig_shift;
> bool is_dig_port;
>
> - if (!hotplug_trigger)
> + if (!pin_mask)
> return;
>
> - DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x, dig 0x%08x\n",
> - hotplug_trigger, dig_hotplug_reg);
> -
> spin_lock(&dev_priv->irq_lock);
> for_each_hpd_pin(i) {
> - if (!(hpd[i] & hotplug_trigger))
> + if (!(BIT(i) & pin_mask))
> continue;
>
> port = get_port_from_pin(i);
> is_dig_port = port && dev_priv->hotplug.irq_port[port];
>
> if (is_dig_port) {
> - bool long_hpd;
> -
> - 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;
> - }
> + bool long_hpd = long_mask & BIT(i);
>
> DRM_DEBUG_DRIVER("digital hpd port %c - %s\n", port_name(port),
> long_hpd ? "long" : "short");
> @@ -1483,9 +1532,7 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
> * interrupts on saner platforms.
> */
> WARN_ONCE(INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev),
> - "Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n",
> - hotplug_trigger, i, hpd[i]);
> -
> + "Received HPD interrupt on pin %d although disabled\n", i);
> continue;
> }
>
> @@ -1493,7 +1540,7 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
> continue;
>
> if (!is_dig_port) {
> - dev_priv->hotplug.event_bits |= (1 << i);
> + dev_priv->hotplug.event_bits |= BIT(i);
> queue_hp = true;
> }
>
> @@ -1505,7 +1552,7 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
> DRM_DEBUG_KMS("Received HPD interrupt on PIN %d - cnt: 0\n", i);
> } else if (dev_priv->hotplug.stats[i].count > HPD_STORM_THRESHOLD) {
> dev_priv->hotplug.stats[i].state = HPD_MARK_DISABLED;
> - dev_priv->hotplug.event_bits &= ~(1 << i);
> + dev_priv->hotplug.event_bits &= ~BIT(i);
> DRM_DEBUG_KMS("HPD interrupt storm detected on PIN %d\n", i);
> storm_detected = true;
> } else {
> @@ -1753,6 +1800,7 @@ static void i9xx_hpd_irq_handler(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> u32 hotplug_status = I915_READ(PORT_HOTPLUG_STAT);
> + u32 pin_mask, long_mask;
>
> if (!hotplug_status)
> return;
> @@ -1767,14 +1815,16 @@ static void i9xx_hpd_irq_handler(struct drm_device *dev)
> if (IS_G4X(dev) || IS_VALLEYVIEW(dev)) {
> u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_G4X;
>
> - intel_hpd_irq_handler(dev, hotplug_trigger, 0, hpd_status_g4x);
> + i9xx_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, hpd_status_g4x);
> + intel_hpd_irq_handler(dev, pin_mask, long_mask);
>
> if (hotplug_status & DP_AUX_CHANNEL_MASK_INT_STATUS_G4X)
> dp_aux_irq_handler(dev);
> } else {
> u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
>
> - intel_hpd_irq_handler(dev, hotplug_trigger, 0, hpd_status_i915);
> + i9xx_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, hpd_status_i915);
> + intel_hpd_irq_handler(dev, pin_mask, long_mask);
> }
> }
>
> @@ -1874,11 +1924,13 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
> int pipe;
> u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK;
> u32 dig_hotplug_reg;
> + u32 pin_mask, long_mask;
>
> dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG);
> I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg);
>
> - intel_hpd_irq_handler(dev, hotplug_trigger, dig_hotplug_reg, hpd_ibx);
> + pch_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, dig_hotplug_reg, hpd_ibx);
> + intel_hpd_irq_handler(dev, pin_mask, long_mask);
>
> if (pch_iir & SDE_AUDIO_POWER_MASK) {
> int port = ffs((pch_iir & SDE_AUDIO_POWER_MASK) >>
> @@ -1971,11 +2023,13 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
> int pipe;
> u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_CPT;
> u32 dig_hotplug_reg;
> + u32 pin_mask, long_mask;
>
> dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG);
> I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg);
>
> - intel_hpd_irq_handler(dev, hotplug_trigger, dig_hotplug_reg, hpd_cpt);
> + pch_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, dig_hotplug_reg, hpd_cpt);
> + intel_hpd_irq_handler(dev, pin_mask, long_mask);
>
> if (pch_iir & SDE_AUDIO_POWER_MASK_CPT) {
> int port = ffs((pch_iir & SDE_AUDIO_POWER_MASK_CPT) >>
> @@ -2174,8 +2228,8 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
> static void bxt_hpd_handler(struct drm_device *dev, uint32_t iir_status)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - uint32_t hp_control;
> - uint32_t hp_trigger;
> + u32 hp_control, hp_trigger;
> + u32 pin_mask, long_mask;
>
> /* Get the status */
> hp_trigger = iir_status & BXT_DE_PORT_HOTPLUG_MASK;
> @@ -2191,7 +2245,8 @@ static void bxt_hpd_handler(struct drm_device *dev, uint32_t iir_status)
> hp_control & BXT_HOTPLUG_CTL_MASK);
>
> /* Check for HPD storm and schedule bottom half */
> - intel_hpd_irq_handler(dev, hp_trigger, hp_control, hpd_bxt);
> + pch_get_hpd_pins(&pin_mask, &long_mask, hp_trigger, hp_control, hpd_bxt);
> + intel_hpd_irq_handler(dev, pin_mask, long_mask);
>
> /*
> * FIXME: Save the hot plug status for bottom half before
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
More information about the Intel-gfx
mailing list