diff mbox

[5/5] drm/i915: Enable HPD interrupts with master ctl interrupt

Message ID 1479917907-2468-6-git-send-email-animesh.manna@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Manna, Animesh Nov. 23, 2016, 4:18 p.m. UTC
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@intel.com>
Signed-off-by: A.Sunil Kamath <sunil.kamath@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

Comments

Imre Deak Nov. 23, 2016, 5:01 p.m. UTC | #1
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@intel.com>
> Signed-off-by: A.Sunil Kamath <sunil.kamath@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.

>  };
>  
>  /* 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.

> +	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.

> +
> +		dev_priv->pm.irqs_enabled = true;
> +	}
>  	synchronize_irq(dev_priv->drm.irq);

This should come before the IRQ enabling.

>  }
>  
> 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?

> +
>  #define ILK_DISPLAY_CHICKEN2	_MMIO(0x42004)
>  /* Required on all Ironlake and Sandybridge according to the B-Spec. */
>  #define  ILK_ELPIN_409_SELECT	(1 << 25)
Ville Syrjälä Nov. 23, 2016, 5:10 p.m. UTC | #2
On Wed, Nov 23, 2016 at 09:48:27PM +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@intel.com>
> Signed-off-by: A.Sunil Kamath <sunil.kamath@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)

hpd_bxt_pcu[]

>  };
>  
>  /* 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;
> +

Please add a proper pcu irq ack/handler pair. I actually have such a
thing sitting on a branch:

git://github.com/vsyrjala/linux.git pcode_irq

> +			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);

Umm. Does the PCH_HOTPLUG reg actually work in this "hpd redirected to
pcu" state? As in are we going to detect long vs. short pulses correctly
if you just use bxt_hpd_irq_handler()?

> +			} 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);
> +	de_pcu_ier = (I915_READ(GEN8_PCU_IER) | de_pcu_hpd_enable_mask);

You'll want a bdw_update_pcu_irq() or some such thing.

> +	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);
> +
> +		dev_priv->pm.irqs_enabled = true;
> +	}
>  	synchronize_irq(dev_priv->drm.irq);
>  }
>  
> 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)

Two names for the same bits?

> +
>  #define ILK_DISPLAY_CHICKEN2	_MMIO(0x42004)
>  /* Required on all Ironlake and Sandybridge according to the B-Spec. */
>  #define  ILK_ELPIN_409_SELECT	(1 << 25)
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Manna, Animesh Nov. 28, 2016, 1:39 p.m. UTC | #3
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@intel.com>
>> Signed-off-by: A.Sunil Kamath <sunil.kamath@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)
Imre Deak Nov. 28, 2016, 2:41 p.m. UTC | #4
On ma, 2016-11-28 at 19:09 +0530, Animesh Manna wrote:
> 
> 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@intel.com>
> > > Signed-off-by: A.Sunil Kamath <sunil.kamath@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.

The problem is that - for example - bxt_hpd_irq_setup() will program
now these PCU bits to GEN8_DE_PORT_IMR which is wrong. There's also a
WARN that will trigger because of this.

Yes, using a separate struct would work I think.

> > >   };
> > >   
> > >   /* 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".

I meant the '& 0x0' part, looks like '& ~de_pcu_hpd_enable_mask' is
what you meant.

> > 
> > > +	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.

The problem is that with the above sequence once you enable interrupts
the handler may be called with irqs_enabled = false and ignoring the
interrupt. But looking at it again, taking the lock around the whole
sequence won't actually solve that, you need to set
GEN8_MASTER_IRQ_CONTROL as the last step (and move synchronize_irq
before the enabling).

> > 
> > > +
> > > +		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)
>
Manna, Animesh Nov. 28, 2016, 3:49 p.m. UTC | #5
On 11/23/2016 10:40 PM, Ville Syrjälä wrote:
> On Wed, Nov 23, 2016 at 09:48:27PM +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@intel.com>
>> Signed-off-by: A.Sunil Kamath <sunil.kamath@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)
> hpd_bxt_pcu[]
Thanks Ville for review.
yes, will add a separate struct for pcu.
>
>>   };
>>   
>>   /* 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;
>> +
> Please add a proper pcu irq ack/handler pair. I actually have such a
> thing sitting on a branch:
>
> git://github.com/vsyrjala/linux.git pcode_irq
Downloaded the code, can you please help with the commit you are referring.
I can see some old commit as last commit (will try to sync with you over 
irc):
amanna@DispDev:~/PROJECT_CODEBASE/drm-intel-ville/pcode_irq$ git log
commit 20624d17963c737bbd9f242402bf3136cb664d10
Merge: 9a08da1 f4274e2
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Wed Apr 8 15:12:25 2015 -0700

     Merge branch 'drm-fixes' of git://people.freedesktop.org/~airlied/linux


>
>> +			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);
> Umm. Does the PCH_HOTPLUG reg actually work in this "hpd redirected to
> pcu" state? As in are we going to detect long vs. short pulses correctly
> if you just use bxt_hpd_irq_handler()?
After programming the PMC_HPD_CTL register we can get PCU interrupt only 
during S0ix for HPD.
AFAIK for both DP and HDMI we will be getting long pulse for hotplug.
And In non-S0ix scenario HPD interrupt will always come from DE_PORT. So 
do we need to handle short pulse interrupt?
>
>> +			} 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);
>> +	de_pcu_ier = (I915_READ(GEN8_PCU_IER) | de_pcu_hpd_enable_mask);
> You'll want a bdw_update_pcu_irq() or some such thing.
Yes, better to use bdw tag, will update in next patchset.
>
>> +	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);
>> +
>> +		dev_priv->pm.irqs_enabled = true;
>> +	}
>>   	synchronize_irq(dev_priv->drm.irq);
>>   }
>>   
>> 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)
> Two names for the same bits?
will correct it.

- Animesh
>
>> +
>>   #define ILK_DISPLAY_CHICKEN2	_MMIO(0x42004)
>>   /* Required on all Ironlake and Sandybridge according to the B-Spec. */
>>   #define  ILK_ELPIN_409_SELECT	(1 << 25)
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Nov. 28, 2016, 4:02 p.m. UTC | #6
On Mon, Nov 28, 2016 at 09:19:36PM +0530, Animesh Manna wrote:
> 
> 
> On 11/23/2016 10:40 PM, Ville Syrjälä wrote:
> > On Wed, Nov 23, 2016 at 09:48:27PM +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@intel.com>
> >> Signed-off-by: A.Sunil Kamath <sunil.kamath@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)
> > hpd_bxt_pcu[]
> Thanks Ville for review.
> yes, will add a separate struct for pcu.
> >
> >>   };
> >>   
> >>   /* 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;
> >> +
> > Please add a proper pcu irq ack/handler pair. I actually have such a
> > thing sitting on a branch:
> >
> > git://github.com/vsyrjala/linux.git pcode_irq
> Downloaded the code, can you please help with the commit you are referring.
> I can see some old commit as last commit (will try to sync with you over 
> irc):
> amanna@DispDev:~/PROJECT_CODEBASE/drm-intel-ville/pcode_irq$ git log
> commit 20624d17963c737bbd9f242402bf3136cb664d10
> Merge: 9a08da1 f4274e2
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Wed Apr 8 15:12:25 2015 -0700
> 
>      Merge branch 'drm-fixes' of git://people.freedesktop.org/~airlied/linux

git checkout pcode_irq

> 
> 
> >
> >> +			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);
> > Umm. Does the PCH_HOTPLUG reg actually work in this "hpd redirected to
> > pcu" state? As in are we going to detect long vs. short pulses correctly
> > if you just use bxt_hpd_irq_handler()?
> After programming the PMC_HPD_CTL register we can get PCU interrupt only 
> during S0ix for HPD.
> AFAIK for both DP and HDMI we will be getting long pulse for hotplug.
> And In non-S0ix scenario HPD interrupt will always come from DE_PORT. So 
> do we need to handle short pulse interrupt?

Short pulse probably isn't needed during s0ix since I presume we can't
have any active displays anyway. The only slight concern might be DP
branch devices which would signal downstream port HPDs via a short
pulse. I have a vague recollection that there might be a way to aske
them for a long HPD instead via some DPCD register.

> >
> >> +			} 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);
> >> +	de_pcu_ier = (I915_READ(GEN8_PCU_IER) | de_pcu_hpd_enable_mask);
> > You'll want a bdw_update_pcu_irq() or some such thing.
> Yes, better to use bdw tag, will update in next patchset.
> >
> >> +	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);
> >> +
> >> +		dev_priv->pm.irqs_enabled = true;
> >> +	}
> >>   	synchronize_irq(dev_priv->drm.irq);
> >>   }
> >>   
> >> 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)
> > Two names for the same bits?
> will correct it.
> 
> - Animesh
> >
> >> +
> >>   #define ILK_DISPLAY_CHICKEN2	_MMIO(0x42004)
> >>   /* Required on all Ironlake and Sandybridge according to the B-Spec. */
> >>   #define  ILK_ELPIN_409_SELECT	(1 << 25)
> >> -- 
> >> 1.9.1
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

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)
 };
 
 /* 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);
+	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);
+
+		dev_priv->pm.irqs_enabled = true;
+	}
 	synchronize_irq(dev_priv->drm.irq);
 }
 
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)
+
 #define ILK_DISPLAY_CHICKEN2	_MMIO(0x42004)
 /* Required on all Ironlake and Sandybridge according to the B-Spec. */
 #define  ILK_ELPIN_409_SELECT	(1 << 25)