[Intel-gfx] [PATCH 5/5] drm/i915: Enable HPD interrupts with master ctl interrupt
Animesh Manna
animesh.manna at intel.com
Mon Nov 28 13:39:28 UTC 2016
On 11/23/2016 10:31 PM, Imre Deak wrote:
> On Wed, 2016-11-23 at 21:48 +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.
>>
>> 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 | 56 ++++++++++++++++++++++++++++++++++++++---
>> drivers/gpu/drm/i915/i915_reg.h | 12 +++++++++
>> 2 files changed, 65 insertions(+), 3 deletions(-)
>> mode change 100644 => 100755 drivers/gpu/drm/i915/i915_irq.c
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> old mode 100644
>> new mode 100755
>> index cb8a75f..2f9b604
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -110,9 +110,9 @@
>>
>> /* BXT hpd list */
>> static const u32 hpd_bxt[HPD_NUM_PINS] = {
>> - [HPD_PORT_A] = BXT_DE_PORT_HP_DDIA,
>> - [HPD_PORT_B] = BXT_DE_PORT_HP_DDIB,
>> - [HPD_PORT_C] = BXT_DE_PORT_HP_DDIC
>> + [HPD_PORT_A] = (BXT_DE_PORT_HP_DDIA | BXT_PCU_DC9_HP_DDIA),
>> + [HPD_PORT_B] = (BXT_DE_PORT_HP_DDIB | BXT_PCU_DC9_HP_DDIB),
>> + [HPD_PORT_C] = (BXT_DE_PORT_HP_DDIC | BXT_PCU_DC9_HP_DDIC)
> These are bits programmed to GEN8_DE_PORT_*, so adding the PCU bits
> here is bogus.
Thanks Imre for review. I understood the "hpd_bxt" array is to store all
bits which validate hpd interrupt in irq_handler from the respective port.
Previously only from DE_PORT interrupt used to come and after
implementing HPD as wake feature interrupt source will be both DE_PORT
and PCU.
So added pcu related bits in the same array.
Do you want two different array for DE_PORT and PCU. I can do it by
creating a new array named "hpd_bxt_pcu" and change the existing one as
"hpd_bxt_de".
Please let me know your suggestion.
>> };
>>
>> /* IIR can theoretically queue up two events. Be paranoid. */
>> @@ -2463,6 +2463,24 @@ static void bxt_hpd_irq_handler(struct drm_i915_private *dev_priv,
>> DRM_ERROR("The master control interrupt lied (DE PORT)!\n");
>> }
>>
>> + if (master_ctl & GEN8_PCU_IRQ) {
>> + iir = I915_READ(GEN8_PCU_IIR);
>> + if (iir) {
>> + u32 tmp_mask;
>> +
>> + I915_WRITE(GEN8_PCU_IIR, iir);
>> + ret = IRQ_HANDLED;
>> + if (IS_BROXTON(dev_priv)) {
>> + tmp_mask = iir & BXT_PCU_DC9_HOTPLUG_MASK;
>> + if (tmp_mask)
>> + bxt_hpd_irq_handler(dev_priv, tmp_mask,
>> + hpd_bxt);
>> + } else
>> + DRM_ERROR("Unexpected PCU interrupt\n");
>> + } else
>> + DRM_ERROR("The master control interrupt lied (PCU)!\n");
>> + }
>> +
>> for_each_pipe(dev_priv, pipe) {
>> u32 flip_done, fault_errors;
>>
>> @@ -4294,6 +4312,19 @@ 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 = GEN9_DE_PCU_PORTA_HOTPLUG |
>> + GEN9_DE_PCU_PORTB_HOTPLUG |
>> + GEN9_DE_PCU_PORTC_HOTPLUG;
>> +
>> + de_pcu_imr = (I915_READ(GEN8_PCU_IMR) & 0x0);
> Typo.
Will remove "de" tag from all pcu related variables, for anything else
let me know.
GEN8 and GEN9 is using same pcu interrupt registers so used the same
macro "GEN8_PCU_IMR".
>
>> + de_pcu_ier = (I915_READ(GEN8_PCU_IER) | de_pcu_hpd_enable_mask);
>> + GEN5_IRQ_INIT(GEN8_PCU_, de_pcu_imr, de_pcu_ier);
>> +}
>> +
>> /**
>> * intel_runtime_pm_disable_interrupts - runtime interrupt disabling
>> * @dev_priv: i915 device instance
>> @@ -4303,8 +4334,27 @@ void intel_irq_uninstall(struct drm_i915_private *dev_priv)
>> */
>> void intel_runtime_pm_disable_interrupts(struct drm_i915_private *dev_priv)
>> {
>> + unsigned long flags = 0;
>> +
>> dev_priv->drm.driver->irq_uninstall(&dev_priv->drm);
>> dev_priv->pm.irqs_enabled = false;
>> +
>> + if (IS_BROXTON(dev_priv) && dev_priv->vbt.hpd_wakeup_enabled) {
>> +
>> + /* Enable HPD related interrupts during DC9 for HPD wakeup */
>> +
>> + I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
>> + POSTING_READ(GEN8_MASTER_IRQ);
>> +
>> + spin_lock_irqsave(&dev_priv->irq_lock, flags);
>> + if (dev_priv->display.hpd_irq_setup)
>> + dev_priv->display.hpd_irq_setup(dev_priv);
>> + spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
>> +
>> + bxt_enable_pcu_interrupt(dev_priv);
> The lock should be around the whole IRQ programming sequence.
Just to make happy the "assert_spin_locked" which present inside
hpd_irq_setup() taken the irq_lock.
As we are disabling all the inerrupts in initial stage, is it ok to take
lock before and after enabling display port and pcu interrupts.
BTW, intel_runtime_pm_disable_interrupts function don't have any lock
before.
>
>> +
>> + dev_priv->pm.irqs_enabled = true;
>> + }
>> synchronize_irq(dev_priv->drm.irq);
> This should come before the IRQ enabling.
Sure, will do it in next patchset.
>
>> }
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 3361d7f..df89025 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -6030,6 +6030,18 @@ 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 GEN9_DE_PCU_PORTA_HOTPLUG (1 << 31)
>> +#define GEN9_DE_PCU_PORTB_HOTPLUG (1 << 30)
>> +#define GEN9_DE_PCU_PORTC_HOTPLUG (1 << 29)
> Redundant copy of the BXT_ defines?
will remove in the next patchset.
>
>> +
>> #define ILK_DISPLAY_CHICKEN2 _MMIO(0x42004)
>> /* Required on all Ironlake and Sandybridge according to the B-Spec. */
>> #define ILK_ELPIN_409_SELECT (1 << 25)
More information about the Intel-gfx
mailing list