________________________________________
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.mar...@gmail.com>
> Cc: Paulo Zanoni <paulo.r.zan...@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
> Cc: Ville Syrjala <ville.syrj...@linux.intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.sriva...@intel.com>
> [Paulo: coding style bikesheds and rebases].
> Signed-off-by: Paulo Zanoni <paulo.r.zan...@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?
> +
> +/* 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.
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
