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.sriva...@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.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 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)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
