[Intel-gfx] [PATCH v3 5/5] drm/i915: Enable HPD interrupts with master ctl interrupt
Imre Deak
imre.deak at intel.com
Fri Feb 10 17:37:25 UTC 2017
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 at intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: Animesh Manna <animesh.manna at intel.com>
> Signed-off-by: A.Sunil Kamath <sunil.kamath at intel.com>
> ---
> 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?
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
>
More information about the Intel-gfx
mailing list