| Message ID | 1371037046-3732-21-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 12, 2013 at 01:37:22PM +0200, Daniel Vetter wrote: > Preinstall disables interrupts, we clear the status register in the > postinstall hook before we actually enable interrupt sources. > > Also add a comment for the curios ring IMR masking, it doesn't > seem to be required on any other platform. > > We seem to have some room for common gt_preinstall/postinstall hooks. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_irq.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 293ee68..b680e1c 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2546,13 +2546,12 @@ static void valleyview_irq_preinstall(struct drm_device *dev) > > /* VLV magic */ > I915_WRITE(VLV_IMR, 0); > + /* Do we really need to clear ring masks for vlv? */ > I915_WRITE(RING_IMR(RENDER_RING_BASE), 0); > I915_WRITE(RING_IMR(GEN6_BSD_RING_BASE), 0); > I915_WRITE(RING_IMR(BLT_RING_BASE), 0); I actually like this for all GENs with rings as a documentation kind of thing. > > /* and GT */ > - I915_WRITE(GTIIR, I915_READ(GTIIR)); > - I915_WRITE(GTIIR, I915_READ(GTIIR)); > I915_WRITE(GTIMR, 0xffffffff); > I915_WRITE(GTIER, 0x0); > POSTING_READ(GTIER); The ordering was wrong here anyway. I think to have the desired effect it should have been 1. mask 2. disable 3. posting read 4. clear 5. posting read 6. clear // potential pending irq But one thing this changes is the double clear, which I feel might be necessary if it is meant to clear the pending interrupt as I would guess. We only seem to do one in postinstall. If this change was intentional, I think we should get Jesse to explain the origin of the original double clear.
On Fri, Jun 28, 2013 at 10:01:12AM -0700, Ben Widawsky wrote: > On Wed, Jun 12, 2013 at 01:37:22PM +0200, Daniel Vetter wrote: > > Preinstall disables interrupts, we clear the status register in the > > postinstall hook before we actually enable interrupt sources. > > > > Also add a comment for the curios ring IMR masking, it doesn't > > seem to be required on any other platform. > > > > We seem to have some room for common gt_preinstall/postinstall hooks. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > drivers/gpu/drm/i915/i915_irq.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > index 293ee68..b680e1c 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -2546,13 +2546,12 @@ static void valleyview_irq_preinstall(struct drm_device *dev) > > > > /* VLV magic */ > > I915_WRITE(VLV_IMR, 0); > > + /* Do we really need to clear ring masks for vlv? */ > > I915_WRITE(RING_IMR(RENDER_RING_BASE), 0); > > I915_WRITE(RING_IMR(GEN6_BSD_RING_BASE), 0); > > I915_WRITE(RING_IMR(BLT_RING_BASE), 0); > > I actually like this for all GENs with rings as a documentation kind of > thing. > > > > > /* and GT */ > > - I915_WRITE(GTIIR, I915_READ(GTIIR)); > > - I915_WRITE(GTIIR, I915_READ(GTIIR)); > > I915_WRITE(GTIMR, 0xffffffff); > > I915_WRITE(GTIER, 0x0); > > POSTING_READ(GTIER); > > The ordering was wrong here anyway. I think to have the desired effect > it should have been > 1. mask > 2. disable > 3. posting read > 4. clear > 5. posting read > 6. clear // potential pending irq > > But one thing this changes is the double clear, which I feel might be > necessary if it is meant to clear the pending interrupt as I would > guess. We only seem to do one in postinstall. If this change was > intentional, I think we should get Jesse to explain the origin of the > original double clear. Yeah, after some more thinking I agree with you. And in general we should do all this in the preinstall hook for all IIR registers, since in the postinstall hook the interupt handler could already be running. And we shouldn't ever steal interrupt source bits from irq handler since doing that too often will result in the interrupt getting disabled. Another mail in this thread in reply to a question from Paulo has some overall idea of how we should set up registers around interrupt enabling. I'd appreciate your comments on that plan. For now I'll drop this patch here and just keep it before calling the generic function. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 293ee68..b680e1c 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2546,13 +2546,12 @@ static void valleyview_irq_preinstall(struct drm_device *dev) /* VLV magic */ I915_WRITE(VLV_IMR, 0); + /* Do we really need to clear ring masks for vlv? */ I915_WRITE(RING_IMR(RENDER_RING_BASE), 0); I915_WRITE(RING_IMR(GEN6_BSD_RING_BASE), 0); I915_WRITE(RING_IMR(BLT_RING_BASE), 0); /* and GT */ - I915_WRITE(GTIIR, I915_READ(GTIIR)); - I915_WRITE(GTIIR, I915_READ(GTIIR)); I915_WRITE(GTIMR, 0xffffffff); I915_WRITE(GTIER, 0x0); POSTING_READ(GTIER);
Preinstall disables interrupts, we clear the status register in the postinstall hook before we actually enable interrupt sources. Also add a comment for the curios ring IMR masking, it doesn't seem to be required on any other platform. We seem to have some room for common gt_preinstall/postinstall hooks. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_irq.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)