Re: [PATCH 00/20] ILK+ interrupt improvements, v2

From: Daniel Vetter <daniel@ffwll.ch>
To: Ben Widawsky <ben@bwidawsk.net>
Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni <paulo.r.zanoni@intel.com>
Subject: Re: [PATCH 00/20] ILK+ interrupt improvements, v2
Date: Wed, 19 Mar 2014 09:36:04 +0100	[thread overview]
Message-ID: <20140319083604.GL30571@phenom.ffwll.local> (raw)
In-Reply-To: <20140318205353.GA29729@bwidawsk.net>

On Tue, Mar 18, 2014 at 01:53:53PM -0700, Ben Widawsky wrote:
> On Fri, Mar 07, 2014 at 08:10:16PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Hi
> > 
> > This is basically a rebase of "[PATCH 00/19] ILK+ interrupt improvements", which
> > was sent to the mailing list on January 22. There are no real differences,
> > except for the last patch, which is new.
> > 
> > Original cover letter:
> > http://lists.freedesktop.org/archives/intel-gfx/2014-January/038679.html
> > 
> > The idea behind this series is that at some point our runtime PM code will just
> > call our irq_preinstall, irq_postinstall and irq_uninstall functions instead of
> > using dev_priv->pc8.regsave, so I decided to audit, cleanup and add a few WARNs
> > to our code before we do that change. We gotta be in shape if we want to be
> > exposed to runtime!
> > 
> > Thanks,
> > Paulo
> > 
> > Paulo Zanoni (20):
> >   drm/i915: add GEN5_IRQ_INIT macro
> >   drm/i915: also use GEN5_IRQ_INIT with south display interrupts
> >   drm/i915: use GEN8_IRQ_INIT on GEN5
> >   drm/i915: add GEN5_IRQ_FINI
> >   drm/i915: don't forget to uninstall the PM IRQs
> >   drm/i915: properly clear IIR at irq_uninstall on Gen5+
> >   drm/i915: add GEN5_IRQ_INIT
> >   drm/i915: check if IIR is still zero at postinstall on Gen5+
> >   drm/i915: fix SERR_INT init/reset code
> >   drm/i915: fix GEN7_ERR_INT init/reset code
> >   drm/i915: fix open coded gen5_gt_irq_preinstall
> >   drm/i915: extract ibx_irq_uninstall
> >   drm/i915: call ibx_irq_uninstall from gen8_irq_uninstall
> >   drm/i915: enable SDEIER later
> >   drm/i915: remove ibx_irq_uninstall
> >   drm/i915: add missing intel_hpd_irq_uninstall
> >   drm/i915: add ironlake_irq_reset
> >   drm/i915: add gen8_irq_reset
> >   drm/i915: only enable HWSTAM interrupts on postinstall on ILK+
> >   drm/i915: add POSTING_READs to the IRQ init/reset macros
> > 
> >  drivers/gpu/drm/i915/i915_irq.c | 270 ++++++++++++++++++----------------------
> >  1 file changed, 121 insertions(+), 149 deletions(-)
> > 
> 
> Okay, here is the summary of my review. At first I was complaining to
> myself about how many patches you used to do a simple thing. But, I must
> admit it made reviewing the thing a lot easier, and when I look back at
> how much stuff you combined, I'm really glad you did it this way. I'm
> sure I've missed something silly though, since every patch looks so
> similar :P
> 
> 1-5: Reviewed-by: Ben Widawsky <ben@bwidawsk.net> (with possible comment
> improvement on #3)
> 
> 7: I don't like. Can we drop? I guess doing this would make a decent
> amount of churn, so if you don't want to drop it, that's fine, and it's
> functionally correct:
>      Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> 8: I'd really like to drop this one.

Comment on this and I think with a pimped commit message this is good to
go imo. I really think the added self-checks are required to start using
this code for runtime pm.

> 9-10: Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> 12-13: I wouldn't mind cpt_irq_* rename, but either way
>        Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> 14: With the requested change in the mail:
>     Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> 16: Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> 20: Should be squashed, but
>     Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> 6, 11, 15, 17, 18, 19: You introduce the term _reset as a verb which
> seems to always mean "disable." I think disable makes the code so much
> clearer, and would really love if you can apply this simple rename. With
> the rename, they're:
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

Paulo's using "reset" functions/macros both in the preinstall hooks and in
the uninstall/disable code. We already use reset for stuff run before
init/enable code to get the hw in a state we expect it to, so I think
Paulo's naming choice is accurate and a plain "disable" more misleading.

I think you raise some good points in your review, and besides the 3 cases
I commented on I lack the detailed knowledge to avoid looking like a fool
;-) So I think I'll wait for Paulo's comments before pulling this all in.

Thanks,
Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch