[Intel-gfx] [PATCH 00/20] ILK+ interrupt improvements, v2

Wed Mar 26 21:54:40 CET 2014

On Wed, Mar 26, 2014 at 05:33:20PM -0300, Paulo Zanoni wrote:
> 2014-03-19 14:25 GMT-03:00 Ben Widawsky <ben at bwidawsk.net>:
> > On Wed, Mar 19, 2014 at 09:36:04AM +0100, Daniel Vetter wrote:
> >> 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 at 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 at 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 at 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.
> >>
> >
> > So you don't need my reviewed-by then. I don't like it...
> >
> >> > 9-10: Reviewed-by: Ben Widawsky <ben at bwidawsk.net>
> >> >
> >> > 12-13: I wouldn't mind cpt_irq_* rename, but either way
> >> >        Reviewed-by: Ben Widawsky <ben at bwidawsk.net>
> >> >
> >> > 14: With the requested change in the mail:
> >> >     Reviewed-by: Ben Widawsky <ben at bwidawsk.net>
> >> >
> >> > 16: Reviewed-by: Ben Widawsky <ben at bwidawsk.net>
> >> >
> >> > 20: Should be squashed, but
> >> >     Reviewed-by: Ben Widawsky <ben at 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 at 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 cannot disagree more. Every time I read "reset" it confuses me. But it
> > seems like I am the minority.
> 
> I understand "reset" may not be the best name, I was already expecting
> to see suggestions on the naming here. IMHO "disable" is also usable,
> and I could rename, but Daniel just called it "misleading". So how
> about we rename it to "clear" instead?
> 
> (let's see if I can make Ben and Daniel agree on something!)
> 
> I'll leave discussion of the other topics to the other emails.
> 

"clear" has a distinct definition, and in the case of the mask, you are
not clearing. It's better than "reset"

I still like, "disable"
I can live with "disable_and_mask"

> >
> >> 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
> >
> > Once Paulo responds, I'll make it a top priority to re-review whatever
> > is needed. Sorry for the original delay.
> >
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >
> > --
> > Ben Widawsky, Intel Open Source Technology Center
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center