Re: [PATCH 6/7] drm/i915: abstract away platform specific parts from hpd handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 28 May 2015, Paulo Zanoni <przanoni@xxxxxxxxx> wrote:
> 2015-05-28 9:43 GMT-03:00 Jani Nikula <jani.nikula@xxxxxxxxx>:
>> 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@xxxxxxxxx>
>> ---
>>  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.

I know, I was divided here myself. In the end I didn't want to make this
superficially platform independent with ifs inside, when the callers are
already platform specific. Also, the i9xx one does not have
dig_hotplug_reg, so not passing that in or logging adds clarity. But I
could have gone either way.

>
>
>> +}
>> +
>> +/**
>> + * 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...

Yup, that's the plan, but things seem to move forward better with
smaller steps!

>
> 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@xxxxxxxxx>

Thanks for the review and suggestions!

BR,
Jani.


>
>
>> + */
>>  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@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> -- 
> Paulo Zanoni

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux