Re: [PATCH 10/11] drm/i915: Add port A HPD support for BDW

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

On Thu, Aug 27, 2015 at 04:29:21PM -0300, Paulo Zanoni wrote:
> 2015-08-12 12:44 GMT-03:00  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Wire up the port A HPD for BDW. Compared to earlier platforms the
> > interrupt setup is a bit different, but basically everything else
> > looks the same.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 72 +++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 66 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index de60174..aefa6c4 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -53,6 +53,10 @@ static const u32 hpd_ivb[HPD_NUM_PINS] = {
> >         [HPD_PORT_A] = DE_DP_A_HOTPLUG_IVB,
> >  };
> >
> > +static const u32 hpd_bdw[HPD_NUM_PINS] = {
> > +       [HPD_PORT_A] = GEN8_PORT_DP_A_HOTPLUG,
> > +};
> > +
> >  static const u32 hpd_ibx[HPD_NUM_PINS] = {
> >         [HPD_CRT] = SDE_CRT_HOTPLUG,
> >         [HPD_SDVO_B] = SDE_SDVOB_HOTPLUG,
> > @@ -369,6 +373,36 @@ void gen6_disable_rps_interrupts(struct drm_device *dev)
> >  }
> >
> >  /**
> > +  * bdw_update_port_irq - update DE port interrupt
> > +  * @dev_priv: driver private
> > +  * @interrupt_mask: mask of interrupt bits to update
> > +  * @enabled_irq_mask: mask of interrupt bits to enable
> > +  */
> > +static void bdw_update_port_irq(struct drm_i915_private *dev_priv,
> > +                               uint32_t interrupt_mask,
> > +                               uint32_t enabled_irq_mask)
> > +{
> > +       uint32_t new_val;
> > +       uint32_t old_val;
> > +
> > +       assert_spin_locked(&dev_priv->irq_lock);
> 
> Just like the other similar functions:
> WARN_ON(enabled_irq_mask & ~interrupt_mask);

ack

> 
> 
> Besides this, there's the recurring "enable PORT A hpd on the PCH"
> problem. Don't you have to patch ibx_hpd_irq_setup() to only enable
> PORTA_HOTPLUG_ENABLE based on what you read from FUSE_STRAP3? At least
> that's what's written on the description of 0x44030 on BDW.

Well, as mentioned in the other mail the strap description says it's not
used and should be ignored, and Art said it might not be correct. So not
sure how we're supposed figure it out if we can't use the LP vs. H
approach. I'll guess I could toss in some FIXMEs or something if we
can't figure it all out soon.

> 
> Everything else looks correct.
> 
> > +
> > +       if (WARN_ON(!intel_irqs_enabled(dev_priv)))
> > +               return;
> > +
> > +       old_val = I915_READ(GEN8_DE_PORT_IMR);
> > +
> > +       new_val = old_val;
> > +       new_val &= ~interrupt_mask;
> > +       new_val |= (~enabled_irq_mask & interrupt_mask);
> > +
> > +       if (new_val != old_val) {
> > +               I915_WRITE(GEN8_DE_PORT_IMR, new_val);
> > +               POSTING_READ(GEN8_DE_PORT_IMR);
> > +       }
> > +}
> > +
> > +/**
> >   * ibx_display_interrupt_update - update SDEIMR
> >   * @dev_priv: driver private
> >   * @interrupt_mask: mask of interrupt bits to update
> > @@ -2139,10 +2173,23 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
> >                 tmp = I915_READ(GEN8_DE_PORT_IIR);
> >                 if (tmp) {
> >                         bool found = false;
> > +                       u32 hotplug_trigger = tmp & GEN8_PORT_DP_A_HOTPLUG;
> >
> >                         I915_WRITE(GEN8_DE_PORT_IIR, tmp);
> >                         ret = IRQ_HANDLED;
> >
> > +                       if (hotplug_trigger) {
> > +                               u32 dig_hotplug_reg, pin_mask, long_mask;
> > +
> > +                               dig_hotplug_reg = I915_READ(DIGITAL_PORT_HOTPLUG_CNTRL);
> > +                               I915_WRITE(DIGITAL_PORT_HOTPLUG_CNTRL, dig_hotplug_reg);
> > +
> > +                               intel_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger,
> > +                                                  dig_hotplug_reg, hpd_bdw,
> > +                                                  ilk_port_hotplug_long_detect);
> > +                               intel_hpd_irq_handler(dev, pin_mask, long_mask);
> > +                       }
> > +
> >                         if (tmp & aux_mask) {
> >                                 dp_aux_irq_handler(dev);
> >                                 found = true;
> > @@ -3156,15 +3203,22 @@ static void ilk_hpd_irq_setup(struct drm_device *dev)
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         u32 hotplug_irqs, hotplug, enabled_irqs;
> >
> > -       if (INTEL_INFO(dev)->gen >= 7) {
> > +       if (INTEL_INFO(dev)->gen >= 8) {
> > +               hotplug_irqs = GEN8_PORT_DP_A_HOTPLUG;
> > +               enabled_irqs = intel_hpd_enabled_irqs(dev, hpd_bdw);
> > +
> > +               bdw_update_port_irq(dev_priv, hotplug_irqs, enabled_irqs);
> > +       } else if (INTEL_INFO(dev)->gen >= 7) {
> >                 hotplug_irqs = DE_DP_A_HOTPLUG_IVB;
> >                 enabled_irqs = intel_hpd_enabled_irqs(dev, hpd_ivb);
> > +
> > +               ilk_update_display_irq(dev_priv, hotplug_irqs, enabled_irqs);
> >         } else {
> >                 hotplug_irqs = DE_DP_A_HOTPLUG;
> >                 enabled_irqs = intel_hpd_enabled_irqs(dev, hpd_ilk);
> > -       }
> >
> > -       ilk_update_display_irq(dev_priv, hotplug_irqs, enabled_irqs);
> > +               ilk_update_display_irq(dev_priv, hotplug_irqs, enabled_irqs);
> > +       }
> >
> >         /*
> >          * Enable digital hotplug on the CPU, and configure the DP short pulse
> > @@ -3477,6 +3531,7 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
> >         uint32_t de_pipe_enables;
> >         int pipe;
> >         u32 de_port_en = GEN8_AUX_CHANNEL_A;
> > +       u32 de_port_masked;

I think I made a mess with this name. It's exactly the opposite of the
pipe stuff. The resulting code already confused the hell out of me when
I was looking at it the second time. So I think I need to redo this part
a bit.

BTW I'm already cooking up a few extra patches on top of the series,
mainly to get the BXT code to conform to the same style the rest of
the code uses. Since most patches have needed some adjustment I'll
repost the entire series. But I'll wait until you've gone through this
v1 so that I'll get all your feedback incorporated.

> >
> >         if (IS_GEN9(dev_priv)) {
> >                 de_pipe_masked |= GEN9_PIPE_PLANE1_FLIP_DONE |
> > @@ -3486,9 +3541,14 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
> >
> >                 if (IS_BROXTON(dev_priv))
> >                         de_port_en |= BXT_DE_PORT_GMBUS;
> > -       } else
> > +       } else {
> >                 de_pipe_masked |= GEN8_PIPE_PRIMARY_FLIP_DONE |
> >                                   GEN8_DE_PIPE_IRQ_FAULT_ERRORS;
> > +       }
> > +
> > +       de_port_masked = de_port_en;
> > +       if (IS_BROADWELL(dev_priv))
> > +               de_port_masked |= GEN8_PORT_DP_A_HOTPLUG;
> >
> >         de_pipe_enables = de_pipe_masked | GEN8_PIPE_VBLANK |
> >                                            GEN8_PIPE_FIFO_UNDERRUN;
> > @@ -3504,7 +3564,7 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
> >                                           dev_priv->de_irq_mask[pipe],
> >                                           de_pipe_enables);
> >
> > -       GEN5_IRQ_INIT(GEN8_DE_PORT_, ~de_port_en, de_port_en);
> > +       GEN5_IRQ_INIT(GEN8_DE_PORT_, ~de_port_en, de_port_masked);
> >  }
> >
> >  static int gen8_irq_postinstall(struct drm_device *dev)
> > @@ -4287,7 +4347,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
> >                 else if (HAS_PCH_SPT(dev))
> >                         dev_priv->display.hpd_irq_setup = spt_hpd_irq_setup;
> >                 else
> > -                       dev_priv->display.hpd_irq_setup = ibx_hpd_irq_setup;
> > +                       dev_priv->display.hpd_irq_setup = ilk_hpd_irq_setup;
> >         } else if (HAS_PCH_SPLIT(dev)) {
> >                 dev->driver->irq_handler = ironlake_irq_handler;
> >                 dev->driver->irq_preinstall = ironlake_irq_reset;
> > --
> > 2.4.6
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

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