Re: [PATCH 05/24] drm/i915/icp: Add Interrupt Support

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

On Thu, 2018-05-24 at 16:53 -0700, Lucas De Marchi wrote:
> On Mon, May 21, 2018 at 05:25:39PM -0700, Paulo Zanoni wrote:
> > 
> > From: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > 
> > This patch addresses Interrupts from south display engine (SDE).
> > 
> > ICP has two registers - SHOTPLUG_CTL_DDI and SHOTPLUG_CTL_TC.
> > Introduce these registers and their intended values.
> > 
> > Introduce icp_irq_handler().
> > 
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > [Paulo: coding style bikesheds and rebases].
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 134
> > +++++++++++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/i915_reg.h |  40 ++++++++++++
> >  2 files changed, 172 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index 9bcec5fdb9d0..6b109991786f 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -122,6 +122,15 @@ static const u32 hpd_tc_gen11[HPD_NUM_PINS] =
> > {
> >  	[HPD_PORT_F] = GEN11_TC4_HOTPLUG
> >  };
> >  
> > +static const u32 hpd_icp[HPD_NUM_PINS] = {
> > +	[HPD_PORT_A] = ICP_DDIA_HOTPLUG,
> > +	[HPD_PORT_B] = ICP_DDIB_HOTPLUG,
> > +	[HPD_PORT_C] = ICP_TC1_HOTPLUG,
> > +	[HPD_PORT_D] = ICP_TC2_HOTPLUG,
> > +	[HPD_PORT_E] = ICP_TC3_HOTPLUG,
> > +	[HPD_PORT_F] = ICP_TC4_HOTPLUG
> > +};
> > +
> >  /* IIR can theoretically queue up two events. Be paranoid. */
> >  #define GEN8_IRQ_RESET_NDX(type, which) do { \
> >  	I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff); \
> > @@ -1586,6 +1595,34 @@ static bool
> > bxt_port_hotplug_long_detect(enum port port, u32 val)
> >  	}
> >  }
> >  
> > +static bool icp_ddi_port_hotplug_long_detect(enum port port, u32
> > val)
> > +{
> > +	switch (port) {
> > +	case PORT_A:
> > +		return val & ICP_DDIA_HPD_LONG_DETECT;
> > +	case PORT_B:
> > +		return val & ICP_DDIB_HPD_LONG_DETECT;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> > +static bool icp_tc_port_hotplug_long_detect(enum port port, u32
> > val)
> > +{
> > +	switch (port) {
> > +	case PORT_C:
> > +		return val & ICP_TC_HPD_LONG_DETECT(PORT_TC1);
> > +	case PORT_D:
> > +		return val & ICP_TC_HPD_LONG_DETECT(PORT_TC2);
> > +	case PORT_E:
> > +		return val & ICP_TC_HPD_LONG_DETECT(PORT_TC3);
> > +	case PORT_F:
> > +		return val & ICP_TC_HPD_LONG_DETECT(PORT_TC4);
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> >  static bool spt_port_hotplug2_long_detect(enum port port, u32 val)
> >  {
> >  	switch (port) {
> > @@ -2377,6 +2414,43 @@ static void cpt_irq_handler(struct
> > drm_i915_private *dev_priv, u32 pch_iir)
> >  		cpt_serr_int_handler(dev_priv);
> >  }
> >  
> > +static void icp_irq_handler(struct drm_i915_private *dev_priv, u32
> > pch_iir)
> > +{
> > +	u32 ddi_hotplug_trigger = pch_iir & ICP_SDE_DDI_MASK;
> > +	u32 tc_hotplug_trigger = pch_iir & ICP_SDE_TC_MASK;
> > +	u32 pin_mask = 0, long_mask = 0;
> > +
> > +	if (ddi_hotplug_trigger) {
> > +		u32 dig_hotplug_reg;
> > +
> > +		dig_hotplug_reg = I915_READ(SHOTPLUG_CTL_DDI);
> > +		I915_WRITE(SHOTPLUG_CTL_DDI, dig_hotplug_reg);
> > +
> > +		intel_get_hpd_pins(dev_priv, &pin_mask,
> > &long_mask,
> > +				   ddi_hotplug_trigger,
> > +				   dig_hotplug_reg, hpd_icp,
> > +				   icp_ddi_port_hotplug_long_detec
> > t);
> > +	}
> > +
> > +	if (tc_hotplug_trigger) {
> > +		u32 dig_hotplug_reg;
> > +
> > +		dig_hotplug_reg = I915_READ(SHOTPLUG_CTL_TC);
> > +		I915_WRITE(SHOTPLUG_CTL_TC, dig_hotplug_reg);
> > +
> > +		intel_get_hpd_pins(dev_priv, &pin_mask,
> > &long_mask,
> > +				   tc_hotplug_trigger,
> > +				   dig_hotplug_reg, hpd_icp,
> > +				   icp_tc_port_hotplug_long_detect
> > );
> > +	}
> > +
> > +	if (pin_mask)
> > +		intel_hpd_irq_handler(dev_priv, pin_mask,
> > long_mask);
> > +
> > +	if (pch_iir & ICP_GMBUS)
> > +		gmbus_irq_handler(dev_priv);
> > +}
> > +
> >  static void spt_irq_handler(struct drm_i915_private *dev_priv, u32
> > pch_iir)
> >  {
> >  	u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_SPT &
> > @@ -2779,8 +2853,11 @@ gen8_de_irq_handler(struct drm_i915_private
> > *dev_priv, u32 master_ctl)
> >  			I915_WRITE(SDEIIR, iir);
> >  			ret = IRQ_HANDLED;
> >  
> > -			if (HAS_PCH_SPT(dev_priv) ||
> > HAS_PCH_KBP(dev_priv) ||
> > -			    HAS_PCH_CNP(dev_priv))
> > +			if (HAS_PCH_ICP(dev_priv))
> > +				icp_irq_handler(dev_priv, iir);
> > +			else if (HAS_PCH_SPT(dev_priv) ||
> > +				 HAS_PCH_KBP(dev_priv) ||
> > +				 HAS_PCH_CNP(dev_priv))
> >  				spt_irq_handler(dev_priv, iir);
> >  			else
> >  				cpt_irq_handler(dev_priv, iir);
> > @@ -3548,6 +3625,9 @@ static void gen11_irq_reset(struct drm_device
> > *dev)
> >  	GEN3_IRQ_RESET(GEN11_DE_HPD_);
> >  	GEN3_IRQ_RESET(GEN11_GU_MISC_);
> >  	GEN3_IRQ_RESET(GEN8_PCU_);
> > +
> > +	if (HAS_PCH_ICP(dev_priv))
> > +		GEN3_IRQ_RESET(ICP_SDE_);
> >  }
> >  
> >  void gen8_irq_power_well_post_enable(struct drm_i915_private
> > *dev_priv,
> > @@ -3664,6 +3744,35 @@ static void ibx_hpd_irq_setup(struct
> > drm_i915_private *dev_priv)
> >  	ibx_hpd_detection_setup(dev_priv);
> >  }
> >  
> > +static void icp_hpd_detection_setup(struct drm_i915_private
> > *dev_priv)
> > +{
> > +	u32 hotplug;
> > +
> > +	hotplug = I915_READ(SHOTPLUG_CTL_DDI);
> > +	hotplug |= ICP_DDIA_HPD_ENABLE |
> > +		   ICP_DDIB_HPD_ENABLE;
> > +	I915_WRITE(SHOTPLUG_CTL_DDI, hotplug);
> > +
> > +	hotplug = I915_READ(SHOTPLUG_CTL_TC);
> > +	hotplug |= ICP_TC_HPD_ENABLE(PORT_TC1) |
> > +		   ICP_TC_HPD_ENABLE(PORT_TC2) |
> > +		   ICP_TC_HPD_ENABLE(PORT_TC3) |
> > +		   ICP_TC_HPD_ENABLE(PORT_TC4);
> > +	I915_WRITE(SHOTPLUG_CTL_TC, hotplug);
> > +}
> > +
> > +static void icp_hpd_irq_setup(struct drm_i915_private *dev_priv)
> > +{
> > +	u32 hotplug_irqs, enabled_irqs;
> > +
> > +	hotplug_irqs = ICP_SDE_DDI_MASK | ICP_SDE_TC_MASK;
> > +	enabled_irqs = intel_hpd_enabled_irqs(dev_priv, hpd_icp);
> > +
> > +	ibx_display_interrupt_update(dev_priv, hotplug_irqs,
> > enabled_irqs);
> > +
> > +	icp_hpd_detection_setup(dev_priv);
> > +}
> > +
> >  static void gen11_hpd_detection_setup(struct drm_i915_private
> > *dev_priv)
> >  {
> >  	u32 hotplug;
> > @@ -3690,6 +3799,9 @@ static void gen11_hpd_irq_setup(struct
> > drm_i915_private *dev_priv)
> >  	POSTING_READ(GEN11_DE_HPD_IMR);
> >  
> >  	gen11_hpd_detection_setup(dev_priv);
> > +
> > +	if (HAS_PCH_ICP(dev_priv))
> > +		icp_hpd_irq_setup(dev_priv);
> >  }
> >  
> >  static void spt_hpd_detection_setup(struct drm_i915_private
> > *dev_priv)
> > @@ -4121,11 +4233,29 @@ static void gen11_gt_irq_postinstall(struct
> > drm_i915_private *dev_priv)
> >  	I915_WRITE(GEN11_GPM_WGBOXPERF_INTR_MASK,  ~0);
> >  }
> >  
> > +static void icp_irq_postinstall(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	u32 mask = ICP_GMBUS;
> > +
> > +	WARN_ON(I915_READ(ICP_SDE_IER) != 0);
> > +	I915_WRITE(ICP_SDE_IER, 0xffffffff);
> > +	POSTING_READ(ICP_SDE_IER);
> > +
> > +	gen3_assert_iir_is_zero(dev_priv, ICP_SDE_IIR);
> > +	I915_WRITE(ICP_SDE_IMR, ~mask);
> > +
> > +	icp_hpd_detection_setup(dev_priv);
> > +}
> > +
> >  static int gen11_irq_postinstall(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	u32 gu_misc_masked = GEN11_GU_MISC_GSE;
> >  
> > +	if (HAS_PCH_ICP(dev_priv))
> > +		icp_irq_postinstall(dev);
> > +
> >  	gen11_gt_irq_postinstall(dev_priv);
> >  	gen8_de_irq_postinstall(dev_priv);
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 19600097581f..28ce96ce0484 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7460,6 +7460,46 @@ enum {
> >  #define  PORTE_HOTPLUG_SHORT_DETECT	(1 << 0)
> >  #define  PORTE_HOTPLUG_LONG_DETECT	(2 << 0)
> >  
> > +/* ICP */
> > +#define ICP_SDE_ISR			_MMIO(0xc4000)
> > +#define ICP_SDE_IMR			_MMIO(0xc4004)
> > +#define ICP_SDE_IIR			_MMIO(0xc4008)
> > +#define ICP_SDE_IER			_MMIO(0xc400c)
> These are exactly the same registers as SDE{ISR,IMR,IIR,IER}. For all
> the other platforms what we do is rather postfix the platform name.
> 
> I think we should follow what they do here.
> 
> 
> > 
> > +#define   ICP_TC4_HOTPLUG		(1 << 27)
> > +#define   ICP_TC3_HOTPLUG		(1 << 26)
> > +#define   ICP_TC2_HOTPLUG		(1 << 25)
> > +#define   ICP_TC1_HOTPLUG		(1 << 24)
> > +#define   ICP_GMBUS			(1 << 23)
> > +#define   ICP_DDIB_HOTPLUG		(1 << 17)
> > +#define   ICP_DDIA_HOTPLUG		(1 << 16)
> so these would become SDE_TC4_HOTPLUG_ICP and so on.
> 

The reason I preferred this naming for gen-11 is it is symmetric to the
corresponding definitions in the north engine.

For example,
+#define GEN11_DE_HPD_ISR               _MMIO(0x44470)
+#define GEN11_DE_HPD_IMR               _MMIO(0x44474)
+#define GEN11_DE_HPD_IIR               _MMIO(0x44478)
+#define GEN11_DE_HPD_IER               _MMIO(0x4447c)
+#define  GEN11_TC4_HOTPLUG                     (1 << 19)
+#define  GEN11_TC3_HOTPLUG                     (1 << 18)
+#define  GEN11_TC2_HOTPLUG                     (1 << 17)
+#define  GEN11_TC1_HOTPLUG                     (1 << 16)

With interrupts getting routed to north or south engines for the same
port, this naming scheme makes the duality clearer IMO.


> > 
> > +
> > +#define ICP_SDE_DDI_MASK		(ICP_DDIB_HOTPLUG |	
> > \
> > +					 ICP_DDIA_HOTPLUG)
> > +
> > +#define ICP_SDE_TC_MASK			(ICP_TC4_HOTPLUG |	
> > \
> > +					 ICP_TC3_HOTPLUG |	
> > \
> > +					 ICP_TC2_HOTPLUG |	
> > \
> > +					 ICP_TC1_HOTPLUG)
> > +
> > +#define SHOTPLUG_CTL_DDI			_MMIO(0xc4030)	
> > /* SHOTPLUG_CTL */
> This also seems to reuse what we have defined as PCH_PORT_HOTPLUG
> with a
> comment to SHOTPLUG_CTL there, although here I tend to be in favor of
> using the current real name of the register (SHOTPLUG_CTL).

The real name I see is SHOTPLUG_CTL_DDI for ICP.

I don't believe we should attempt to make these definitions consistent
with previous platforms over making them consistent with each other.
 

> 
> The rest looks good to me.
> 
> Lucas De Marchi
> 
> > 
> > +#define   ICP_DDIB_HPD_ENABLE			(1 << 7)
> > +#define   ICP_DDIB_HPD_STATUS_MASK		(3 << 4)
> > +#define   ICP_DDIB_HPD_NO_DETECT		(0 << 4)
> > +#define   ICP_DDIB_HPD_SHORT_DETECT		(1 << 4)
> > +#define   ICP_DDIB_HPD_LONG_DETECT		(2 << 4)
> > +#define   ICP_DDIB_HPD_SHORT_LONG_DETECT	(3 << 4)
> > +#define   ICP_DDIA_HPD_ENABLE			(1 << 3)
> > +#define   ICP_DDIA_HPD_STATUS_MASK		(3 << 0)
> > +#define   ICP_DDIA_HPD_NO_DETECT		(0 << 0)
> > +#define   ICP_DDIA_HPD_SHORT_DETECT		(1 << 0)
> > +#define   ICP_DDIA_HPD_LONG_DETECT		(2 << 0)
> > +#define   ICP_DDIA_HPD_SHORT_LONG_DETECT	(3 << 0)
> > +
> > +#define SHOTPLUG_CTL_TC				_MMIO(0xc40
> > 34)
> > +#define   ICP_TC_HPD_ENABLE(tc_port)		(8 <<
> > (tc_port) * 4)
> > +#define   ICP_TC_HPD_LONG_DETECT(tc_port)	(2 << (tc_port) *
> > 4)
> > +#define   ICP_TC_HPD_SHORT_DETECT(tc_port)	(1 << (tc_port)
> > * 4)
> > +
> >  #define PCH_GPIOA               _MMIO(0xc5010)
> >  #define PCH_GPIOB               _MMIO(0xc5014)
> >  #define PCH_GPIOC               _MMIO(0xc5018)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx