Re: [PATCH 06/11] drm/i915: Introduce spt_irq_handler()

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

On Thu, Aug 27, 2015 at 07:13:49PM +0300, Ville Syrjälä wrote:
> On Thu, Aug 27, 2015 at 10:38:25AM +0300, Jani Nikula wrote:
> > On Thu, 27 Aug 2015, Paulo Zanoni <przanoni@gmail.com> wrote:
> > > 2015-08-12 12:44 GMT-03:00  <ville.syrjala@linux.intel.com>:
> > >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >>
> > >> Starting from SPT the only interrupts living in the south are GMBUS and
> > >> HPD. What's worse some of the SPT specific new bits conflict with some
> > >> other bits on earlier PCH generations. So better not use the
> > >> cpt_irq_handler() for SPT+ anymore.
> > >>
> > >> Also kill the hand rolled port E handling with something more
> > >> standardish. This also avoids accidentally confusing port B and port E
> > >> long pulses since the bits occupy the same positions, just in different
> > >> registers.
> > >>
> > >> Also add a comment noting that the short pulse duration bits are
> > >> reserved on LPT+. The 2ms value we program is 0, so no issue wrt. the
> > >> MBZ in the spec.
> > >>
> > >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >> ---
> > >>  drivers/gpu/drm/i915/i915_irq.c | 123 +++++++++++++++++++++++++++-------------
> > >>  1 file changed, 83 insertions(+), 40 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > >> index d12106c..e2485bd 100644
> > >> --- a/drivers/gpu/drm/i915/i915_irq.c
> > >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> > >> @@ -1260,6 +1260,16 @@ static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
> > >>         }
> > >>  }
> > >>
> > >> +static bool spt_port_hotplug2_long_detect(enum port port, u32 val)
> > >> +{
> > >> +       switch (port) {
> > >> +       case PORT_E:
> > >> +               return val & PORTE_HOTPLUG_LONG_DETECT;
> > >> +       default:
> > >> +               return false;
> > >> +       }
> > >> +}
> > >> +
> > >>  static bool pch_port_hotplug_long_detect(enum port port, u32 val)
> > >>  {
> > >>         switch (port) {
> > >> @@ -1269,8 +1279,6 @@ static bool pch_port_hotplug_long_detect(enum port port, u32 val)
> > >>                 return val & PORTC_HOTPLUG_LONG_DETECT;
> > >>         case PORT_D:
> > >>                 return val & PORTD_HOTPLUG_LONG_DETECT;
> > >> -       case PORT_E:
> > >> -               return val & PORTE_HOTPLUG_LONG_DETECT;
> > >>         default:
> > >>                 return false;
> > >>         }
> > >> @@ -1771,12 +1779,7 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
> > >>  {
> > >>         struct drm_i915_private *dev_priv = dev->dev_private;
> > >>         int pipe;
> > >> -       u32 hotplug_trigger;
> > >> -
> > >> -       if (HAS_PCH_SPT(dev))
> > >> -               hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_SPT;
> > >> -       else
> > >> -               hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_CPT;
> > >> +       u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_CPT;
> > >>
> > >>         if (hotplug_trigger) {
> > >>                 u32 dig_hotplug_reg, pin_mask, long_mask;
> > >> @@ -1784,22 +1787,10 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
> > >>                 dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG);
> > >>                 I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg);
> > >>
> > >> -               if (HAS_PCH_SPT(dev)) {
> > >> -                       intel_get_hpd_pins(&pin_mask, &long_mask,
> > >> -                                          hotplug_trigger,
> > >> -                                          dig_hotplug_reg, hpd_spt,
> > >> -                                          pch_port_hotplug_long_detect);
> > >> -
> > >> -                       /* detect PORTE HP event */
> > >> -                       dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG2);
> > >> -                       if (pch_port_hotplug_long_detect(PORT_E,
> > >> -                                                        dig_hotplug_reg))
> > >> -                               long_mask |= 1 << HPD_PORT_E;
> > >> -               } else
> > >> -                       intel_get_hpd_pins(&pin_mask, &long_mask,
> > >> -                                          hotplug_trigger,
> > >> -                                          dig_hotplug_reg, hpd_cpt,
> > >> -                                          pch_port_hotplug_long_detect);
> > >> +               intel_get_hpd_pins(&pin_mask, &long_mask,
> > >> +                                  hotplug_trigger,
> > >> +                                  dig_hotplug_reg, hpd_cpt,
> > >> +                                  pch_port_hotplug_long_detect);
> > >>
> > >>                 intel_hpd_irq_handler(dev, pin_mask, long_mask);
> > >>         }
> > >> @@ -1833,6 +1824,42 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
> > >>                 cpt_serr_int_handler(dev);
> > >>  }
> > >>
> > >> +static void spt_irq_handler(struct drm_device *dev, u32 pch_iir)
> > >> +{
> > >> +       struct drm_i915_private *dev_priv = dev->dev_private;
> > >> +       u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_SPT &
> > >> +               ~SDE_PORTE_HOTPLUG_SPT;
> > >> +       u32 hotplug2_trigger = pch_iir & SDE_PORTE_HOTPLUG_SPT;
> > >> +
> > >> +       if (hotplug_trigger) {
> > >> +               u32 dig_hotplug_reg, pin_mask, long_mask;
> > >> +
> > >> +               dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG);
> > >> +               I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg);
> > >> +
> > >> +               intel_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger,
> > >> +                                  dig_hotplug_reg, hpd_spt,
> > >> +                                  pch_port_hotplug_long_detect);
> > >> +               intel_hpd_irq_handler(dev, pin_mask, long_mask);
> > >> +       }
> > >> +
> > >> +       if (hotplug2_trigger) {
> > >> +               u32 dig_hotplug_reg, pin_mask, long_mask;
> > >> +
> > >> +               dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG2);
> > >> +               I915_WRITE(PCH_PORT_HOTPLUG2, dig_hotplug_reg);
> > >> +
> > >> +               intel_get_hpd_pins(&pin_mask, &long_mask, hotplug2_trigger,
> > >> +                                  dig_hotplug_reg, hpd_spt,
> > >> +                                  spt_port_hotplug2_long_detect);
> > >> +
> > >> +               intel_hpd_irq_handler(dev, pin_mask, long_mask);
> > >> +       }
> > >
> > > Instead of calling intel_hpd_irq_handler() twice, can't you just do
> > > something like:
> > > intel_hpd_irq_hanlder(dev, pin_mask1 | pin_mask2, long_mask1 | long_mask2) ?
> > 
> > 
> > And if you reverted
> > 
> > commit fd63e2a972c670887e5e8a08440111d3812c0996
> > Author: Imre Deak <imre.deak@intel.com>
> > Date:   Tue Jul 21 15:32:44 2015 -0700
> > 
> >     drm/i915: combine i9xx_get_hpd_pins and pch_get_hpd_pins
> > 
> > and added another version for spt, this would all be pretty and each of
> > the functions would do exactly what is required and you could have
> > meaningful, platform specific debug logging in each of the functions.
> > 
> > By focusing on deduplicating a few lines in get_hpd_pins, the callers
> > (which are platform specific code to begin with) get more complicated.
> 
> OK, let me have a look at this mess in a bit more detail and see what can
> be done. Paulo's idea at least is simple enough to do, so I'll do at
> least that.

I think what I'll do is leave intel_get_hpd_pins() mostly intact, except
I'll move the pin_mask/long_mask 0 initialization out into the callers.
That way anyone can call it multiple times and accumulate all the bits to
the same two masks, and then call intel_hpd_irq_handler just once.

Going back to platform specific variants of the function didn't seem too
nice as I'd probably then need to add more variants for port A HPD etc.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx