Re: [PATCH 1/2] drm/i915/icp: Add Interrupt Support

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

Em Ter, 2018-06-26 às 11:32 -0700, Srivatsa, Anusha escreveu:
> ________________________________________
> From: Zanoni, Paulo R
> Sent: Monday, June 25, 2018 4:17 PM
> To: Srivatsa, Anusha; intel-gfx@lists.freedesktop.org
> Cc: Pandiyan, Dhinakaran
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/icp: Add Interrupt
> Support
> 
> Em Qua, 2018-06-20 às 14:36 -0700, Anusha Srivatsa escreveu:
> > 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().
> > 
> > v2:
> > - remove redundant register defines.(Lucas)
> > - Change register names to be more consistent with
> > previous platforms (Lucas)
> > 
> > Cc: Lucas De Marchi <lucas.de.marchi@gmail.com>
> > 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 46aaef5..7a7c4a2 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_gen11[HPD_NUM_PINS] = {
> >       [HPD_PORT_F] = GEN11_TC4_HOTPLUG | GEN11_TBT4_HOTPLUG
> >  };
> > 
> > +static const u32 hpd_icp[HPD_NUM_PINS] = {
> > +     [HPD_PORT_A] = SDE_DDIA_HOTPLUG_ICP,
> > +     [HPD_PORT_B] = SDE_DDIB_HOTPLUG_ICP,
> > +     [HPD_PORT_C] = SDE_TC1_HOTPLUG_ICP,
> > +     [HPD_PORT_D] = SDE_TC2_HOTPLUG_ICP,
> > +     [HPD_PORT_E] = SDE_TC3_HOTPLUG_ICP,
> > +     [HPD_PORT_F] = SDE_TC4_HOTPLUG_ICP
> > +};
> > +
> >  /* 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) {
> > @@ -2385,6 +2422,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 & SDE_DDI_MASK_ICP;
> > +     u32 tc_hotplug_trigger = pch_iir & SDE_TC_MASK_ICP;
> > +     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 & SDE_GMBUS_ICP)
> > +             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 &
> > @@ -2804,8 +2878,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);
> > @@ -3584,6 +3661,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(SDE);
> >  }
> > 
> >  void gen8_irq_power_well_post_enable(struct drm_i915_private
> > *dev_priv,
> > @@ -3700,6 +3780,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 = SDE_DDI_MASK_ICP | SDE_TC_MASK_ICP;
> > +     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;
> > @@ -3733,6 +3842,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)
> > @@ -4168,11 +4280,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 = SDE_GMBUS_ICP;
> > +
> > +     WARN_ON(I915_READ(SDEIER) != 0);
> > +     I915_WRITE(SDEIER, 0xffffffff);
> > +     POSTING_READ(SDEIER);
> > +
> > +     gen3_assert_iir_is_zero(dev_priv, SDEIIR);
> > +     I915_WRITE(SDEIMR, ~mask);
> > +
> > +     icp_hpd_detection_setup(dev_priv);
> 
> Having a sentence/paragraph on the commit message explaining why we
> can
> afford to do this instead of going the ibx_irq_pre_postinstall()
> followed by ibx_irq_postinstall() done by the previous platforms
> would
> help.
> 
> My understanding is that this is OK, but a confirmation would always
> be
> fine.
> 
> 
> > +}
> > +
> >  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 4bfd7a9..e347055 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7517,6 +7517,46 @@ enum {
> >  #define  PORTE_HOTPLUG_SHORT_DETECT  (1 << 0)
> >  #define  PORTE_HOTPLUG_LONG_DETECT   (2 << 0)
> > 
> > +/* ICP */
> > +#define   SDE_TC4_HOTPLUG_ICP                (1 << 27)
> > +#define   SDE_TC3_HOTPLUG_ICP                (1 << 26)
> > +#define   SDE_TC2_HOTPLUG_ICP                (1 << 25)
> > +#define   SDE_TC1_HOTPLUG_ICP                (1 << 24)
> > +#define   SDE_GMBUS_ICP                      (1 << 23)
> > +#define   SDE_DDIB_HOTPLUG_ICP               (1 << 17)
> > +#define   SDE_DDIA_HOTPLUG_ICP               (1 << 16)
> > +
> > +#define SDE_DDI_MASK_ICP             (SDE_DDIB_HOTPLUG_ICP |
> > \
> > +                                      SDE_DDIA_HOTPLUG_ICP)
> > +
> > +#define SDE_TC_MASK_ICP                      (SDE_TC4_HOTPLUG_ICP
> > >     \
> > 
> > +                                      SDE_TC3_HOTPLUG_ICP |
> > \
> > +                                      SDE_TC2_HOTPLUG_ICP |
> > \
> > +                                      SDE_TC1_HOTPLUG_ICP)
> 
> Now these definitions are right below PCH_PORT_HOTPLUG2, but these
> bits
> are not for that register. Please move the definitions to the place
> that has the other SDE definitions. Also please change that /* ICP */
> comment to match the style used by the other PCHs: /* south display
> engine interrupt: ICP */ (also possibly fixing the "CPT/PPT" comment
> to
> "CPT - CNP" or something like that.
> Yes. That is good point.
> QQ, the change of "CPT/PPT...." will be a separate patch though,
> right?

Since it's just a single line comment change and it's related to your
current change (update to the bits of the register in question), I
would say there's no need to be in a separate patch IMHO, but of course
we would also accept the change in a separate patch.


> 
> > +
> > +/* This register is a reuse of PCH_PORT_HOTPLUG register. The Spec
> > + * has the name SHOTPLUG_CTL_DDI for ICP. Let us stick to the
> > Spec.
> 
> I think the best way to describe would be to say that what was
> previously done by a single register (PCH_PORT_HOTPLUG) is now done
> by
> two registers, one for DDI and the other for TC, and that's why we
> have
> brand new definitions.
> 
> Thanks paulo for the reviews. Will make the changes.
> 
> Anusha 
> Otherwise, looks good.

Please fix your email client to add ">" when you reply to emails, this
email was super confusing.

> 
> Thanks,
> Paulo
> 
> > + */
> > +
> > +#define SHOTPLUG_CTL_DDI                     _MMIO(0xc4030)
> > +#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)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx