Re: [PATCH 12/14] drm/i915: unify GT/PM irq postinstall code

From: Daniel Vetter <daniel.vetter@ffwll.ch>
To: Paulo Zanoni <przanoni@gmail.com>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 12/14] drm/i915: unify GT/PM irq postinstall code
Date: Thu, 11 Jul 2013 08:13:34 +0200	[thread overview]
Message-ID: <CAKMK7uGiNT2KxLear3Gk63_AQMF+QF6EWRiSHA0eTSonhtg+FA@mail.gmail.com> (raw)
In-Reply-To: <CA+gsUGSnJpHWaDOaha3S4LwthAhJE5tFHXtLbyvd81iB124TWQ@mail.gmail.com>

On Wed, Jul 10, 2013 at 10:48 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> Which means we're now initializing GEN6_PM* code on SNB, which seems
> good. You might want to dedicate a paragraph for this on the commit
> message.
>
> On the IRQ patches I wrote (but did not sent yet) I unified the
> ILK/SNB irq_handler vfuncs with IVB/HSW ones. I guess bugs like the
> one you've just fixed here and in the previous patch are a good way to
> justify my patches :)

I think we have a language communication fail going on here in these
few patches. With "unify" I don't mean "extract identical code, simple
refactoring with no functional change but "make them work the same way
since currently they don't".

Note that with the exception of the gt_irq_mask initialization on vlv
there's no bugfix in here: PM interrupte setup simply works
differently on snb/vlv than on ivb/hsw after Ben's VECS enabling
patches. What these few patches here try to do is unify the sequences
again so all platforms set up the PM registers the same way.

Note that the PMIER update in gen6_enable_rps isn't a bugfix either:
The interrupt handler doesn't touch this register. The only other
place is the hsw vecs interrupt setup in the ivybridge irq vfuncs, but
those two codepaths can never run in parrallel due to init/teardown
sequence ordering (even though that gen6_enable_rps runs from a work
item).

I've read through the commit message again and for me it seems to be
clear what's going on (but I'm obviously biased). So again, do you
have ideas for improvements? Wrt patch splitting I'm not sure whether
it's worth it, since the current code is simply a bit a confusing mess
imo.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch