[Intel-gfx] [PATCH 0/8] sanitize RPS interrupt enabling/disabling

[Daniel Vetter]

On Sat, Nov 08, 2014 at 02:57:58PM +0200, Imre Deak wrote:
> On Fri, 2014-11-07 at 18:42 -0200, Paulo Zanoni wrote:
> > 2014-11-05 16:48 GMT-02:00 Imre Deak <imre.deak at intel.com>:
> > > While fixing [1] I noticed that we can simplify a couple of things in
> > > the RPS enabling/disabling code. So I did that and also fixed one WARN
> > > that we can hit with some of the pm_rpm subtests. Hopefully these
> > > changes also makes it clearer how we avoid the race during RPS interrupt
> > > disabling and makes it less fragile (see the comment in patch 7/8).
> > 
> > For patches 1-4: Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > But for patch 4, can't we try to create a new intel_rps.c and move
> > stuff there, instead of keeping them in i915_irq.c? I kinda like the
> > idea of files containing single features... Anyway, we can do this in
> > yet-another patch.
> 
> intel_rps.c sounds like a good idea. For now I moved the RPS interrupt
> handling to i915_irq.c, since half of it is already there
> (snb_update_pm_irq() which pokes at the same registers) as well as the
> RPS work itself and its scheduling. Ville also had the idea to further
> simplify things since the PM IRQ clearing, masking, enabling is very
> similar to how this is done for other interrupt sources and i915_irq.c
> would be a good staging area for such future work. So I'd also suggest
> refactoring thing into intel_rps.c as a follow-up.

With my recent i915_irq.c cleanup I've left the low-level irq
masking/handling of the interrupt regs in i915_irq.c and only moved the
actual logic for a feature into new files like intel_fifo_underrun.c.

The reason for that is that often the irq bits change on different
platforms than the actual registers, e.g. gmbus has pretty much not
changed since gen2, but the irq bits moved like crazy. And I also like to
keep all the irq masking stuff together - it's tricky code and keeping it
in one feel increases changes that we handle everything the same.
So I think that should generally be a good split.

But intel_rps.c for all the rps code + a bit of kerneldoc sounds like a
really good idea. Especially since rps code is called from lots of places
(e.g. Chris' booster logic).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch