On Fri, Feb 10, 2017 at 07:37:25PM +0200, Imre Deak wrote:
> On Tue, Dec 27, 2016 at 06:59:56PM +0530, Animesh Manna wrote:
> > While suspending the device hpd related interrupts are enabled
> > to get the interrupt when device is in suspend state.
> >
> > Though display is in DC9 but system can be in S0 or S0i3 state.
> > Hot plug during S0 state will generate de_port_interrupt but if
> > system is in S0i3 state then display driver will get hotplug
> > interrupt as pcu_hpd_interrupt which will come via pmc. So
> > added the interrupt handling for pcu hpd interrupt.
> >
> > v1: Initial version as RFC.
> >
> > v2: Based on review comments from Imre, Ville,
> > - separate hpd_bxt_pcu structure added.
> > - pcu_irq_ack/pcu_irq_handler pair added to handle pcu interrupt.
> > - sequence for enabling pcu interrupt is modified.
> > - Removed the definition of pcu interrupt mask bit as it is matching
> > with enable bit.
> >
> > Cc: Imre Deak <imre.deak@xxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Signed-off-by: Animesh Manna <animesh.manna@xxxxxxxxx>
> > Signed-off-by: A.Sunil Kamath <sunil.kamath@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_irq.c | 73 +++++++++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/i915/i915_reg.h | 8 +++++
> > 2 files changed, 81 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index a0e70f5..e5de22e 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -115,6 +115,12 @@ static const u32 hpd_bxt[HPD_NUM_PINS] = {
> > [HPD_PORT_C] = BXT_DE_PORT_HP_DDIC
> > };
> >
> > +static const u32 hpd_bxt_pcu[HPD_NUM_PINS] = {
> > + [HPD_PORT_A] = BXT_PCU_DC9_HP_DDIA,
> > + [HPD_PORT_B] = BXT_PCU_DC9_HP_DDIB,
> > + [HPD_PORT_C] = BXT_PCU_DC9_HP_DDIC
> > +};
> > +
> > /* IIR can theoretically queue up two events. Be paranoid. */
> > #define GEN8_IRQ_RESET_NDX(type, which) do { \
> > I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff); \
> > @@ -2537,12 +2543,49 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
> > return ret;
> > }
> >
> > +static irqreturn_t
> > +gen8_pcu_irq_ack(struct drm_i915_private *dev_priv,
> > + u32 master_ctl, u32 *pcu_iir)
> > +{
> > + irqreturn_t ret = IRQ_NONE;
> > +
> > + if (master_ctl & GEN8_PCU_IRQ) {
> > + *pcu_iir = I915_READ(GEN8_PCU_IIR);
> > +
> > + if (*pcu_iir) {
> > + I915_WRITE(GEN8_PCU_IIR, *pcu_iir);
> > + ret = IRQ_HANDLED;
> > + } else {
> > + DRM_ERROR("The master control interrupt lied (PCU)!\n");
> > + }
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static void
> > +gen8_pcu_irq_handler(struct drm_i915_private *dev_priv, u32 pcu_iir)
> > +{
> > + bool found = false;
> > +
> > + if (IS_BROXTON(dev_priv) && (pcu_iir & BXT_PCU_DC9_HOTPLUG_MASK)) {
> > + u32 tmp_mask = pcu_iir & BXT_PCU_DC9_HOTPLUG_MASK;
> > +
> > + bxt_hpd_irq_handler(dev_priv, tmp_mask, hpd_bxt_pcu);
>
> As Ville already pointed out this won't be able to distinguish between
> long and short pulses. From BSpec:
> """
> During DC9 hardware will dynamically transition between display engine
> powered on and off, so hotplug may be detected by PMC or display engine,
> and hardware will not reliably differentiate between short and long
> pulse hotplug events.
> """
>
> So it needs its own handler that assumes a long HPD happened without
> relying on PCH_PORT_HOTPLUG.
>
> > + found = true;
> > + }
> > +
> > + if (!found)
> > + DRM_ERROR("Unexpected PCU interrupt\n");
> > +}
> > +
> > static irqreturn_t gen8_irq_handler(int irq, void *arg)
> > {
> > struct drm_device *dev = arg;
> > struct drm_i915_private *dev_priv = to_i915(dev);
> > u32 master_ctl;
> > u32 gt_iir[4] = {};
> > + u32 pcu_iir = 0;
> > irqreturn_t ret;
> >
> > if (!intel_irqs_enabled(dev_priv))
> > @@ -2560,7 +2603,9 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
> >
> > /* Find, clear, then process each source of interrupt */
> > ret = gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
> > + ret |= gen8_pcu_irq_ack(dev_priv, master_ctl, &pcu_iir);
> > gen8_gt_irq_handler(dev_priv, gt_iir);
> > + gen8_pcu_irq_handler(dev_priv, pcu_iir);
> > ret |= gen8_de_irq_handler(dev_priv, master_ctl);
> >
> > I915_WRITE_FW(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
> > @@ -4290,6 +4335,17 @@ void intel_irq_uninstall(struct drm_i915_private *dev_priv)
> > dev_priv->pm.irqs_enabled = false;
> > }
> >
> > +static void bxt_enable_pcu_interrupt(struct drm_i915_private *dev_priv)
> > +{
> > + u32 de_pcu_hpd_enable_mask, de_pcu_imr, de_pcu_ier;
> > +
> > + de_pcu_hpd_enable_mask = BXT_PCU_DC9_HOTPLUG_MASK;
> > +
> > + de_pcu_imr = (I915_READ(GEN8_PCU_IMR) & ~de_pcu_hpd_enable_mask);
> > + de_pcu_ier = (I915_READ(GEN8_PCU_IER) | de_pcu_hpd_enable_mask);
>
> No need for the outer parentheses.
>
> > + GEN5_IRQ_INIT(GEN8_PCU_, de_pcu_imr, de_pcu_ier);
> > +}
> > +
> > /**
> > * intel_runtime_pm_disable_interrupts - runtime interrupt disabling
> > * @dev_priv: i915 device instance
> > @@ -4302,6 +4358,23 @@ void intel_runtime_pm_disable_interrupts(struct drm_i915_private *dev_priv)
> > dev_priv->drm.driver->irq_uninstall(&dev_priv->drm);
> > dev_priv->pm.irqs_enabled = false;
> > synchronize_irq(dev_priv->drm.irq);
> > +
> > + if (IS_BROXTON(dev_priv) && dev_priv->vbt.hpd_wakeup_enabled) {
> > + unsigned long flags = 0;
> > +
> > + /* Enable HPD related interrupts during DC9 for HPD wakeup */
> > + spin_lock_irqsave(&dev_priv->irq_lock, flags);
> > + if (dev_priv->display.hpd_irq_setup)
> > + dev_priv->display.hpd_irq_setup(dev_priv);
>
> This will complain now that interrupts have been disabled already.
>
> > + spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
> > +
> > + bxt_enable_pcu_interrupt(dev_priv);
> > +
> > + dev_priv->pm.irqs_enabled = true;
>
> This will cause a WARN in assert_can_enable_dc9() about interrupts being
> enabled. You can just remove that, since its assumption doesn't hold any
> more.
>
> > + I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
> > + POSTING_READ(GEN8_MASTER_IRQ);
> > +
> > + }
> > }
>
> What about the corresponding disabling of the HPD PCU interrupt during
> resume?
Ah, intel_runtime_pm_enable_interrupts() will take care of the above, so
that's fine.
>
> intel_runtime_pm_disable_interrupts() is called during S3/S4 too, I
> don't think we want to enable HPD for those states too.
>
> After this both HPD IRQs and HPD polling will be enabled, we only want
> HPD IRQs.
>
> >
> > /**
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 8e47b59..19717c0 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6402,6 +6402,14 @@ enum {
> > #define GEN8_PCU_IIR _MMIO(0x444e8)
> > #define GEN8_PCU_IER _MMIO(0x444ec)
> >
> > +/* BXT PCU DC9 hotplug control */
> > +#define BXT_PCU_DC9_HP_DDIA (1<<31)
> > +#define BXT_PCU_DC9_HP_DDIB (1<<30)
> > +#define BXT_PCU_DC9_HP_DDIC (1<<29)
> > +#define BXT_PCU_DC9_HOTPLUG_MASK (BXT_PCU_DC9_HP_DDIA | \
> > + BXT_PCU_DC9_HP_DDIB | \
> > + BXT_PCU_DC9_HP_DDIC)
> > +
> > #define ILK_DISPLAY_CHICKEN2 _MMIO(0x42004)
> > /* Required on all Ironlake and Sandybridge according to the B-Spec. */
> > #define ILK_ELPIN_409_SELECT (1 << 25)
> > --
> > 2.7.4
> >
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
