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

Message ID	20180522002558.29262-6-paulo.r.zanoni@intel.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <intel-gfx-bounces@lists.freedesktop.org> From: Paulo Zanoni <paulo.r.zanoni@intel.com> To: intel-gfx@lists.freedesktop.org Date: Mon, 21 May 2018 17:25:39 -0700 Message-Id: <20180522002558.29262-6-paulo.r.zanoni@intel.com> In-Reply-To: <20180522002558.29262-1-paulo.r.zanoni@intel.com> References: <20180522002558.29262-1-paulo.r.zanoni@intel.com> Subject: [Intel-gfx] [PATCH 05/24] drm/i915/icp: Add Interrupt Support Precedence: list Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>

Zanoni, Paulo R May 22, 2018, 12:25 a.m. UTC

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(-)

Lucas De Marchi May 24, 2018, 11:53 p.m. UTC | #1

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_detect);
> +	}
> +
> +	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.

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

Lucas De Marchi May 25, 2018, 12:43 a.m. UTC | #2

On Thu, May 24, 2018 at 05:45:43PM -0700, Dhinakaran Pandiyan wrote:
> 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.

Still the register is the same as SDEISR and there are places in which we
read it expecting to be the same number.

Only the bits are different, so name the bits differently as it is for
other platforms.  I think of the symmetry here just and accident of
life expecting to be different if north and south engines don't have the
same ports.

Lucas De Marchi

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

Dhinakaran Pandiyan May 25, 2018, 12:45 a.m. UTC | #3

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)

Lucas De Marchi May 30, 2018, 12:04 a.m. UTC | #4

On Thu, May 24, 2018 at 05:43:24PM -0700, Lucas De Marchi wrote:
> On Thu, May 24, 2018 at 05:45:43PM -0700, Dhinakaran Pandiyan wrote:
> > 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);

to be clear on what I was saying... See the context just above: we read
and write SDEIIR to get/clear the interrupt bits. Yet you are defining a
ICP_SDE_IIR that has to be the same value.  To avoid any confusion, I
think it's better to stay with SDEIIR and just change the bit
definition.


> > > > +			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.
> 
> Still the register is the same as SDEISR and there are places in which we
> read it expecting to be the same number.
> 
> Only the bits are different, so name the bits differently as it is for
> other platforms.  I think of the symmetry here just and accident of
> life expecting to be different if north and south engines don't have the
> same ports.

This is on gen8_de_irq_handler() which is still used for gen11.

Lucas De Marchi

> 
> Lucas De Marchi
> 
> > 
> > 
> > > > 
> > > > +
> > > > +#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)

Lucas De Marchi June 13, 2018, 10:23 p.m. UTC | #5

On Tue, May 29, 2018 at 05:04:58PM -0700, Lucas De Marchi wrote:
> On Thu, May 24, 2018 at 05:43:24PM -0700, Lucas De Marchi wrote:
> > On Thu, May 24, 2018 at 05:45:43PM -0700, Dhinakaran Pandiyan wrote:
> > > 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);
> 
> to be clear on what I was saying... See the context just above: we read
> and write SDEIIR to get/clear the interrupt bits. Yet you are defining a
> ICP_SDE_IIR that has to be the same value.  To avoid any confusion, I
> think it's better to stay with SDEIIR and just change the bit
> definition.

Dk, any news on this?

Lucas De Marchi

> 
> 
> > > > > +			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.
> > 
> > Still the register is the same as SDEISR and there are places in which we
> > read it expecting to be the same number.
> > 
> > Only the bits are different, so name the bits differently as it is for
> > other platforms.  I think of the symmetry here just and accident of
> > life expecting to be different if north and south engines don't have the
> > same ports.
> 
> This is on gen8_de_irq_handler() which is still used for gen11.
> 
> Lucas De Marchi
> 
> > 
> > Lucas De Marchi
> > 
> > > 
> > > 
> > > > > 
> > > > > +
> > > > > +#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)

Zanoni, Paulo R June 14, 2018, 12:04 a.m. UTC | #6

Em Qua, 2018-06-13 às 15:23 -0700, Lucas De Marchi escreveu:
> On Tue, May 29, 2018 at 05:04:58PM -0700, Lucas De Marchi wrote:
> > On Thu, May 24, 2018 at 05:43:24PM -0700, Lucas De Marchi wrote:
> > > On Thu, May 24, 2018 at 05:45:43PM -0700, Dhinakaran Pandiyan
> > > wrote:
> > > > 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_lo
> > > > > > ng_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_lon
> > > > > > g_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);
> > 
> > to be clear on what I was saying... See the context just above: we
> > read
> > and write SDEIIR to get/clear the interrupt bits. Yet you are
> > defining a
> > ICP_SDE_IIR that has to be the same value.  To avoid any confusion,
> > I
> > think it's better to stay with SDEIIR and just change the bit
> > definition.
> 
> Dk, any news on this?
> 
> Lucas De Marchi
> 
> > 
> > 
> > > > > > +			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.

I agree, there's no need to have an alias and this can indeed make the
code harder to read. I'd vote to just remove it, but if we indeed to
chose to do it, we can do something like:

#define ICP_SDE_ISR SDE_ISR

But I think it's worth keeping the bit definitions separate since
they're very different. Perhaps we should keep the comment to explain
that the ICP registers are below:

#define registers_from_before (x < y)
/* ICP: */
#define registers_from_icp (x < y)

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

I don't have strong opinions here, I'd go with whatever. Just a notice
that ICP is an SDE, so we have some ugly redundancy here. But yeah, our
 standard is weird: registers new to a platform have the platform name
as a prefix, bits new to a platform have the platform name as a
suffix...


> > > > 
> > > > 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.
> > > 
> > > Still the register is the same as SDEISR and there are places in
> > > which we
> > > read it expecting to be the same number.
> > > 
> > > Only the bits are different, so name the bits differently as it
> > > is for
> > > other platforms.  I think of the symmetry here just and accident
> > > of
> > > life expecting to be different if north and south engines don't
> > > have the
> > > same ports.
> > 
> > This is on gen8_de_irq_handler() which is still used for gen11.
> > 
> > Lucas De Marchi
> > 
> > > 
> > > Lucas De Marchi
> > > 
> > > > 
> > > > 
> > > > > > 
> > > > > > +
> > > > > > +#define ICP_SDE_DDI_MASK		(ICP_DDIB_HOTPLUG
> > > > > > |	
> > > > > > \
> > > > > > +					 ICP_DDIA_HOTPLUG)
> > > > > > +
> > > > > > +#define ICP_SDE_TC_MASK			(ICP_TC4_HO
> > > > > > TPLUG |	
> > > > > > \
> > > > > > +					 ICP_TC3_HOTPLUG |
> > > > > > 	
> > > > > > \
> > > > > > +					 ICP_TC2_HOTPLUG |
> > > > > > 	
> > > > > > \
> > > > > > +					 ICP_TC1_HOTPLUG)
> > > > > > +
> > > > > > +#define SHOTPLUG_CTL_DDI			_MMIO(0xc4
> > > > > > 030)	
> > > > > > /* 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 thing to keep in mind here is that ICP split this register in two:
SHOTPLUG_CTL_DDI and SHOTPLUG_CTL_TC, so I'd be in favor of never using
just SHOTPLUG_CTL.



> > > >  
> > > > 
> > > > > 
> > > > > 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				_MM
> > > > > > IO(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)

Dhinakaran Pandiyan June 14, 2018, 2:21 a.m. UTC | #7

On Wed, 2018-06-13 at 15:23 -0700, Lucas De Marchi wrote:
> On Tue, May 29, 2018 at 05:04:58PM -0700, Lucas De Marchi wrote:
> > 
> > On Thu, May 24, 2018 at 05:43:24PM -0700, Lucas De Marchi wrote:
> > > 
> > > On Thu, May 24, 2018 at 05:45:43PM -0700, Dhinakaran Pandiyan
> > > wrote:
> > > > 
> > > > 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_lo
> > > > > > ng_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_lon
> > > > > > g_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);
> > to be clear on what I was saying... See the context just above: we
> > read
> > and write SDEIIR to get/clear the interrupt bits. Yet you are
> > defining a
> > ICP_SDE_IIR that has to be the same value.  To avoid any confusion,
> > I
> > think it's better to stay with SDEIIR and just change the bit
> > definition.
> Dk, any news on this?

SDEIIR is fine by me. The patch author has to make the required
changes, I just reviewed the patch :)

> 
> Lucas De Marchi
> 
> > 
> > 
> > 
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > +			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.
> > > Still the register is the same as SDEISR and there are places in
> > > which we
> > > read it expecting to be the same number.
> > > 
> > > Only the bits are different, so name the bits differently as it
> > > is for
> > > other platforms.  I think of the symmetry here just and accident
> > > of
> > > life expecting to be different if north and south engines don't
> > > have the
> > > same ports.
> > This is on gen8_de_irq_handler() which is still used for gen11.
> > 
> > Lucas De Marchi
> > 
> > > 
> > > 
> > > Lucas De Marchi
> > > 
> > > > 
> > > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > +
> > > > > > +#define ICP_SDE_DDI_MASK		(ICP_DDIB_HOTPLUG
> > > > > > |	
> > > > > > \
> > > > > > +					 ICP_DDIA_HOTPLUG)
> > > > > > +
> > > > > > +#define ICP_SDE_TC_MASK			(ICP_TC4_HO
> > > > > > TPLUG |	
> > > > > > \
> > > > > > +					 ICP_TC3_HOTPLUG |
> > > > > > 	
> > > > > > \
> > > > > > +					 ICP_TC2_HOTPLUG |
> > > > > > 	
> > > > > > \
> > > > > > +					 ICP_TC1_HOTPLUG)
> > > > > > +
> > > > > > +#define SHOTPLUG_CTL_DDI			_MMIO(0xc4
> > > > > > 030)	
> > > > > > /* 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.
> > > >  
If you want to retain the original definition PCH_HOTPLUG, appending to
the existing comment that the register is called SHOTPLUG_CTL_DDI on
ICP would be nice.

Srivatsa, Anusha June 18, 2018, 7:10 p.m. UTC | #8

On Wed, Jun 13, 2018 at 07:21:54PM -0700, Dhinakaran Pandiyan wrote:
> On Wed, 2018-06-13 at 15:23 -0700, Lucas De Marchi wrote:
> > On Tue, May 29, 2018 at 05:04:58PM -0700, Lucas De Marchi wrote:
> > > 
> > > On Thu, May 24, 2018 at 05:43:24PM -0700, Lucas De Marchi wrote:
> > > > 
> > > > On Thu, May 24, 2018 at 05:45:43PM -0700, Dhinakaran Pandiyan
> > > > wrote:
> > > > > 
> > > > > 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_lo
> > > > > > > ng_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_lon
> > > > > > > g_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);
> > > to be clear on what I was saying... See the context just above: we
> > > read
> > > and write SDEIIR to get/clear the interrupt bits. Yet you are
> > > defining a
> > > ICP_SDE_IIR that has to be the same value.  To avoid any confusion,
> > > I
> > > think it's better to stay with SDEIIR and just change the bit
> > > definition.
> > Dk, any news on this?

Lucas, Dk, thanks for the review.
Sorry for the late response.

Yes, i see that the registers are exactly the same. better to reuse the
original SDE{} and have only new bit definitions in place.

I shall remove the ICP_SDE{} register defines.

> SDEIIR is fine by me. The patch author has to make the required
> changes, I just reviewed the patch :)
> 
> > 
> > Lucas De Marchi
> > 
> > > 
> > > 
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > +			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)

I see that changing ICP_TC4_HOTPLUG to SDE_TC4_HOTPLUG_ICP will align it with rest
of platform. looking at BSpec though the bitfield is named "Hotplug TypeC Port n".
Following that maybe SDE_HOTPLUG_TC4_ICP  will be a good option.
This will break the symmetry with the registers in north display though....


> > > > > With interrupts getting routed to north or south engines for
> > > > > the same
> > > > > port, this naming scheme makes the duality clearer IMO.
> > > > Still the register is the same as SDEISR and there are places in
> > > > which we
> > > > read it expecting to be the same number.
> > > > 
> > > > Only the bits are different, so name the bits differently as it
> > > > is for
> > > > other platforms.  I think of the symmetry here just and accident
> > > > of
> > > > life expecting to be different if north and south engines don't
> > > > have the
> > > > same ports.
> > > This is on gen8_de_irq_handler() which is still used for gen11.

> > > Lucas De Marchi
> > > 
> > > > 
> > > > 
> > > > Lucas De Marchi
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > +
> > > > > > > +#define ICP_SDE_DDI_MASK		(ICP_DDIB_HOTPLUG
> > > > > > > |	
> > > > > > > \
> > > > > > > +					 ICP_DDIA_HOTPLUG)
> > > > > > > +
> > > > > > > +#define ICP_SDE_TC_MASK			(ICP_TC4_HO
> > > > > > > TPLUG |	
> > > > > > > \
> > > > > > > +					 ICP_TC3_HOTPLUG |
> > > > > > > 	
> > > > > > > \
> > > > > > > +					 ICP_TC2_HOTPLUG |
> > > > > > > 	
> > > > > > > \
> > > > > > > +					 ICP_TC1_HOTPLUG)
> > > > > > > +
> > > > > > > +#define SHOTPLUG_CTL_DDI			_MMIO(0xc4
> > > > > > > 030)	
> > > > > > > /* 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.
That is correct.
 
> > > > > I don't believe we should attempt to make these definitions
> > > > > consistent
> > > > > with previous platforms over making them consistent with each
> > > > > other.
> > > > >  
> If you want to retain the original definition PCH_HOTPLUG, appending to
> the existing comment that the register is called SHOTPLUG_CTL_DDI on
> ICP would be nice.

So have the PCH_HOTPLUG define and the new SHOTPLUG_CTL_DDI with comment
addressing that they are the same registers but keeping the name consistent with BSpec
should suffice?

Thanks,
Anusha 
 
__ _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Commit Message

Comments

Patch