[Intel-gfx] [PATCH 05/24] drm/i915/icp: Add Interrupt Support
Lucas De Marchi
lucas.de.marchi at gmail.com
Fri May 25 00:43:24 UTC 2018
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 at 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 at intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa at intel.com>
> > > [Paulo: coding style bikesheds and rebases].
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at 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)
More information about the Intel-gfx
mailing list