Re: [PATCH 04/24] drm/i915/icl: Support for TC North Display interrupts

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Qua, 2018-06-13 às 15:20 -0700, Lucas De Marchi escreveu:
> +Chris
> 
> On Mon, May 21, 2018 at 05:25:38PM -0700, Paulo Zanoni wrote:
> > From: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > 
> > The hotplug interrupts for the ports can be routed to either North
> > Display or South Display depending on the output mode. DP Alternate
> > or
> > DP over TBT outputs will have hotplug interrupts routed to the
> > North
> > Display while interrupts for legacy modes will be routed to the
> > South
> > Display in PCH. This patch adds hotplug interrupt handling support
> > for
> > DP Alternate mode.
> > 
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > [Paulo: coding style changes]
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 95
> > +++++++++++++++++++++++++++++++++++++++--
> >  drivers/gpu/drm/i915/i915_reg.h | 20 +++++++++
> >  2 files changed, 112 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index dde938bbfb0a..9bcec5fdb9d0 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -115,6 +115,13 @@ static const u32 hpd_bxt[HPD_NUM_PINS] = {
> >  	[HPD_PORT_C] = BXT_DE_PORT_HP_DDIC
> >  };
> >  
> > +static const u32 hpd_tc_gen11[HPD_NUM_PINS] = {
> > +	[HPD_PORT_C] = GEN11_TC1_HOTPLUG,
> > +	[HPD_PORT_D] = GEN11_TC2_HOTPLUG,
> > +	[HPD_PORT_E] = GEN11_TC3_HOTPLUG,
> > +	[HPD_PORT_F] = GEN11_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); \
> > @@ -1549,6 +1556,22 @@ static void gen8_gt_irq_handler(struct
> > drm_i915_private *i915,
> >  	}
> >  }
> >  
> > +static bool gen11_port_hotplug_long_detect(enum port port, u32
> > val)
> > +{
> > +	switch (port) {
> > +	case PORT_C:
> > +		return val &
> > GEN11_HOTPLUG_CTL_LONG_DETECT(PORT_TC1);
> > +	case PORT_D:
> > +		return val &
> > GEN11_HOTPLUG_CTL_LONG_DETECT(PORT_TC2);
> > +	case PORT_E:
> > +		return val &
> > GEN11_HOTPLUG_CTL_LONG_DETECT(PORT_TC3);
> > +	case PORT_F:
> > +		return val &
> > GEN11_HOTPLUG_CTL_LONG_DETECT(PORT_TC4);
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> >  static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
> >  {
> >  	switch (port) {
> > @@ -2590,6 +2613,25 @@ static void bxt_hpd_irq_handler(struct
> > drm_i915_private *dev_priv,
> >  	intel_hpd_irq_handler(dev_priv, pin_mask, long_mask);
> >  }
> >  
> > +static void gen11_hpd_irq_handler(struct drm_i915_private
> > *dev_priv, u32 iir)
> > +{
> > +	u32 pin_mask = 0, long_mask = 0;
> > +	u32 trigger_tc, dig_hotplug_reg;
> > +
> > +	trigger_tc = iir & GEN11_DE_TC_HOTPLUG_MASK;
> > +	if (trigger_tc) {
> > +		dig_hotplug_reg = I915_READ(GEN11_TC_HOTPLUG_CTL);
> > +		I915_WRITE(GEN11_TC_HOTPLUG_CTL, dig_hotplug_reg);
> > +
> > +		intel_get_hpd_pins(dev_priv, &pin_mask,
> > &long_mask, trigger_tc,
> > +				   dig_hotplug_reg, hpd_tc_gen11,
> > +				   gen11_port_hotplug_long_detect)
> > ;
> > +		intel_hpd_irq_handler(dev_priv, pin_mask,
> > long_mask);
> > +	} else {
> > +		DRM_ERROR("Unexpected DE HPD interrupt 0x%08x\n",
> > iir);
> > +	}
> > +}
> > +
> >  static irqreturn_t
> >  gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32
> > master_ctl)
> >  {
> > @@ -2626,6 +2668,17 @@ gen8_de_irq_handler(struct drm_i915_private
> > *dev_priv, u32 master_ctl)
> >  			DRM_ERROR("The master control interrupt
> > lied (DE MISC)!\n");
> >  	}
> >  
> > +	if (INTEL_GEN(dev_priv) >= 11 && (master_ctl &
> > GEN11_DE_HPD_IRQ)) {
> > +		iir = I915_READ(GEN11_DE_HPD_IIR);
> > +		if (iir) {
> > +			I915_WRITE(GEN11_DE_HPD_IIR, iir);
> > +			ret = IRQ_HANDLED;
> > +			gen11_hpd_irq_handler(dev_priv, iir);
> 
> I think the same question as for the 2nd patch remains here.
> Shouldn't
> we being re-enabling the master interrupt before doing any further
> processing?
> 
> However all gens are like that. A change here seems to belong on a
> different patch... it's also the case for previous gens. Chris, is
> there
> anything different you are seeing on gen11?

I'd say this type of request should definitely be on a separate patch.

We have some major inconsistencies in the way we handle interrupts:
some places use raw write/read, some places don't, some places do use
the likely/unlikely helpers, some places don't, some places do the post
processing, some places don't. If we want people to use a specific
style we should probably convert the current code to use it first.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> 
> Lucas De Marchi
> 
> 
> > +		} else {
> > +			DRM_ERROR("The master control interrupt
> > lied, (DE HPD)!\n");
> > +		}
> > +	}
> > +
> >  	if (master_ctl & GEN8_DE_PORT_IRQ) {
> >  		iir = I915_READ(GEN8_DE_PORT_IIR);
> >  		if (iir) {
> > @@ -3492,6 +3545,7 @@ static void gen11_irq_reset(struct drm_device
> > *dev)
> >  
> >  	GEN3_IRQ_RESET(GEN8_DE_PORT_);
> >  	GEN3_IRQ_RESET(GEN8_DE_MISC_);
> > +	GEN3_IRQ_RESET(GEN11_DE_HPD_);
> >  	GEN3_IRQ_RESET(GEN11_GU_MISC_);
> >  	GEN3_IRQ_RESET(GEN8_PCU_);
> >  }
> > @@ -3610,6 +3664,34 @@ static void ibx_hpd_irq_setup(struct
> > drm_i915_private *dev_priv)
> >  	ibx_hpd_detection_setup(dev_priv);
> >  }
> >  
> > +static void gen11_hpd_detection_setup(struct drm_i915_private
> > *dev_priv)
> > +{
> > +	u32 hotplug;
> > +
> > +	hotplug = I915_READ(GEN11_TC_HOTPLUG_CTL);
> > +	hotplug |= GEN11_HOTPLUG_CTL_ENABLE(PORT_TC1) |
> > +		   GEN11_HOTPLUG_CTL_ENABLE(PORT_TC2) |
> > +		   GEN11_HOTPLUG_CTL_ENABLE(PORT_TC3) |
> > +		   GEN11_HOTPLUG_CTL_ENABLE(PORT_TC4);
> > +	I915_WRITE(GEN11_TC_HOTPLUG_CTL, hotplug);
> > +}
> > +
> > +static void gen11_hpd_irq_setup(struct drm_i915_private *dev_priv)
> > +{
> > +	u32 hotplug_irqs, enabled_irqs;
> > +	u32 val;
> > +
> > +	enabled_irqs = intel_hpd_enabled_irqs(dev_priv,
> > hpd_tc_gen11);
> > +	hotplug_irqs = GEN11_DE_TC_HOTPLUG_MASK;
> > +
> > +	val = I915_READ(GEN11_DE_HPD_IMR);
> > +	val &= ~hotplug_irqs;
> > +	I915_WRITE(GEN11_DE_HPD_IMR, val);
> > +	POSTING_READ(GEN11_DE_HPD_IMR);
> > +
> > +	gen11_hpd_detection_setup(dev_priv);
> > +}
> > +
> >  static void spt_hpd_detection_setup(struct drm_i915_private
> > *dev_priv)
> >  {
> >  	u32 val, hotplug;
> > @@ -3980,10 +4062,17 @@ static void gen8_de_irq_postinstall(struct
> > drm_i915_private *dev_priv)
> >  	GEN3_IRQ_INIT(GEN8_DE_PORT_, ~de_port_masked,
> > de_port_enables);
> >  	GEN3_IRQ_INIT(GEN8_DE_MISC_, ~de_misc_masked,
> > de_misc_masked);
> >  
> > -	if (IS_GEN9_LP(dev_priv))
> > +	if (INTEL_GEN(dev_priv) >= 11) {
> > +		u32 de_hpd_masked = 0;
> > +		u32 de_hpd_enables = GEN11_DE_TC_HOTPLUG_MASK;
> > +
> > +		GEN3_IRQ_INIT(GEN11_DE_HPD_, ~de_hpd_masked,
> > de_hpd_enables);
> > +		gen11_hpd_detection_setup(dev_priv);
> > +	} else if (IS_GEN9_LP(dev_priv)) {
> >  		bxt_hpd_detection_setup(dev_priv);
> > -	else if (IS_BROADWELL(dev_priv))
> > +	} else if (IS_BROADWELL(dev_priv)) {
> >  		ilk_hpd_detection_setup(dev_priv);
> > +	}
> >  }
> >  
> >  static int gen8_irq_postinstall(struct drm_device *dev)
> > @@ -4505,7 +4594,7 @@ void intel_irq_init(struct drm_i915_private
> > *dev_priv)
> >  		dev->driver->irq_uninstall = gen11_irq_reset;
> >  		dev->driver->enable_vblank = gen8_enable_vblank;
> >  		dev->driver->disable_vblank = gen8_disable_vblank;
> > -		dev_priv->display.hpd_irq_setup =
> > spt_hpd_irq_setup;
> > +		dev_priv->display.hpd_irq_setup =
> > gen11_hpd_irq_setup;
> >  	} else if (INTEL_GEN(dev_priv) >= 8) {
> >  		dev->driver->irq_handler = gen8_irq_handler;
> >  		dev->driver->irq_preinstall = gen8_irq_reset;
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index ca474f6f523c..19600097581f 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7036,11 +7036,31 @@ enum {
> >  #define  GEN11_AUDIO_CODEC_IRQ		(1 << 24)
> >  #define  GEN11_DE_PCH_IRQ		(1 << 23)
> >  #define  GEN11_DE_MISC_IRQ		(1 << 22)
> > +#define  GEN11_DE_HPD_IRQ		(1 << 21)
> >  #define  GEN11_DE_PORT_IRQ		(1 << 20)
> >  #define  GEN11_DE_PIPE_C		(1 << 18)
> >  #define  GEN11_DE_PIPE_B		(1 << 17)
> >  #define  GEN11_DE_PIPE_A		(1 << 16)
> >  
> > +#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)
> > +#define  GEN11_DE_TC_HOTPLUG_MASK		(GEN11_TC4_HOTPLU
> > G | \
> > +						 GEN11_TC3_HOTPLUG
> > | \
> > +						 GEN11_TC2_HOTPLUG
> > | \
> > +						 GEN11_TC1_HOTPLUG
> > )
> > +
> > +#define GEN11_TC_HOTPLUG_CTL				_MMIO(
> > 0x44038)
> > +#define  GEN11_HOTPLUG_CTL_ENABLE(tc_port)		(8 <<
> > (tc_port) * 4)
> > +#define  GEN11_HOTPLUG_CTL_LONG_DETECT(tc_port)		(2
> > << (tc_port) * 4)
> > +#define  GEN11_HOTPLUG_CTL_SHORT_DETECT(tc_port)	(1 <<
> > (tc_port) * 4)
> > +#define  GEN11_HOTPLUG_CTL_NO_DETECT(tc_port)		(0 <<
> > (tc_port) * 4)
> > +
> >  #define GEN11_GT_INTR_DW0		_MMIO(0x190018)
> >  #define  GEN11_CSME			(31)
> >  #define  GEN11_GUNIT			(28)
> > -- 
> > 2.14.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx