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

[Imre Deak <imre.deak@intel.com>]

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)
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx