On Tue, Mar 18, 2014 at 10:11:38AM -0700, Ben Widawsky wrote: > On Fri, Mar 07, 2014 at 08:10:19PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > > > > And rename is to GEN5_IRQ_INIT. > > > > We have discussed doing equivalent changes on July 2013, and I even > > sent a patch series for this: "[PATCH 00/15] Unify interrupt register > > init/reset". Now that the BDW code was merged, I have one more > > argument in favor of these changes. > > > > Here's what really changes with the Gen 5 IRQ init code: > > - We now clear the IIR registers at preinstall (they are also > > cleared at postinstall, but we will change that later). > > - We have an additional POSTING_READ at the IMR register. > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > > --- > > drivers/gpu/drm/i915/i915_irq.c | 49 +++++++++++++++++++---------------------- > > 1 file changed, 23 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > index 852844d..7be7da1 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -80,12 +80,30 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */ > > [HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS > > }; > > > > +/* > > + * IIR can theoretically queue up two events. Be paranoid. > > + * Also, make sure callers of these macros have something equivalent to a > > + * POSTING_READ on the IIR register. > > + * */ > > I don't understand what you mean in this comment. If you're always going > to sending a posting read after the second IIR write, why not just put > it in the macro? Bspec says if there's a 2nd interrupt queued up IMR/IER don't affect that bit and it will show up /sometimes/ after the first bit has been cleared from IIR. My best guess for that was "on the next read", hence the POSTING_READ to make sure it happens. Or do you mean something else? In any case a bit of Bspec citation for this would be appropriate. -Daniel > > The reason it wasn't in my original macro is because we've done the > posting read on IER, and IMR - so we're not going to get new interrupts. > When the second IIR write lands is irrelevant. The POSTING_READ in > between is to prevent the [probably impossible] case of the writes > getting collapsed into one write. > > > +#define GEN8_IRQ_INIT_NDX(type, which) do { \ > > + I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff); \ > > + POSTING_READ(GEN8_##type##_IMR(which)); \ > > + I915_WRITE(GEN8_##type##_IER(which), 0); \ > > + I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \ > > + POSTING_READ(GEN8_##type##_IIR(which)); \ > > + I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \ > > +} while (0) > > + > > #define GEN5_IRQ_INIT(type) do { \ > > I915_WRITE(type##IMR, 0xffffffff); \ > > + POSTING_READ(type##IMR); \ > > I915_WRITE(type##IER, 0); \ > > - POSTING_READ(type##IER); \ > > + I915_WRITE(type##IIR, 0xffffffff); \ > > + POSTING_READ(type##IIR); \ > > + I915_WRITE(type##IIR, 0xffffffff); \ > > } while (0) > > > > + > > /* For display hotplug interrupt */ > > static void > > ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask) > > @@ -2789,6 +2807,7 @@ static void gen5_gt_irq_preinstall(struct drm_device *dev) > > GEN5_IRQ_INIT(GT); > > if (INTEL_INFO(dev)->gen >= 6) > > GEN5_IRQ_INIT(GEN6_PM); > > + POSTING_READ(GTIIR); > > } > > > > /* drm_dma.h hooks > > @@ -2843,25 +2862,6 @@ static void gen8_irq_preinstall(struct drm_device *dev) > > I915_WRITE(GEN8_MASTER_IRQ, 0); > > POSTING_READ(GEN8_MASTER_IRQ); > > > > - /* IIR can theoretically queue up two events. Be paranoid */ > > -#define GEN8_IRQ_INIT_NDX(type, which) do { \ > > - I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff); \ > > - POSTING_READ(GEN8_##type##_IMR(which)); \ > > - I915_WRITE(GEN8_##type##_IER(which), 0); \ > > - I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \ > > - POSTING_READ(GEN8_##type##_IIR(which)); \ > > - I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \ > > - } while (0) > > - > > -#define GEN8_IRQ_INIT(type) do { \ > > - I915_WRITE(GEN8_##type##_IMR, 0xffffffff); \ > > - POSTING_READ(GEN8_##type##_IMR); \ > > - I915_WRITE(GEN8_##type##_IER, 0); \ > > - I915_WRITE(GEN8_##type##_IIR, 0xffffffff); \ > > - POSTING_READ(GEN8_##type##_IIR); \ > > - I915_WRITE(GEN8_##type##_IIR, 0xffffffff); \ > > - } while (0) > > - > > GEN8_IRQ_INIT_NDX(GT, 0); > > GEN8_IRQ_INIT_NDX(GT, 1); > > GEN8_IRQ_INIT_NDX(GT, 2); > > @@ -2871,12 +2871,9 @@ static void gen8_irq_preinstall(struct drm_device *dev) > > GEN8_IRQ_INIT_NDX(DE_PIPE, pipe); > > } > > > > - GEN8_IRQ_INIT(DE_PORT); > > - GEN8_IRQ_INIT(DE_MISC); > > - GEN8_IRQ_INIT(PCU); > > -#undef GEN8_IRQ_INIT > > -#undef GEN8_IRQ_INIT_NDX > > - > > + GEN5_IRQ_INIT(GEN8_DE_PORT_); > > + GEN5_IRQ_INIT(GEN8_DE_MISC_); > > + GEN5_IRQ_INIT(GEN8_PCU_); > > POSTING_READ(GEN8_PCU_IIR); > > > > ibx_irq_preinstall(dev); > > -- > > 1.8.5.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ben Widawsky, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx