Re: [PATCH 20/24] drm/i915: kill bogus GTIIR clearing in vlv_preinstall hook

[Daniel Vetter <daniel@ffwll.ch>]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch