[Intel-gfx] [PATCH 1/2] drm/i915: mask RPS IRQs properly when disabling RPS

[Daniel Vetter]

On Mon, Dec 01, 2014 at 02:47:06PM -0200, Paulo Zanoni wrote:
> 2014-12-01 14:34 GMT-02:00 Chris Wilson <chris at chris-wilson.co.uk>:
> > On Mon, Dec 01, 2014 at 02:29:11PM -0200, Paulo Zanoni wrote:
> >> 2014-11-20 19:01 GMT-02:00 Imre Deak <imre.deak at intel.com>:
> >> > Atm, igt/gem_reset_stats can trigger the recently added WARN on
> >> > left-over PM_IIR bits in gen6_enable_rps_interrupts(). There are two
> >> > reasons for this:
> >> > 1. we call intel_enable_gt_powersave() without a preceeding
> >> >    intel_disable_gt_powersave()
> >> > 2. gen6_disable_rps_interrupts() doesn't mask interrupts in PM_IMR
> >>
> >> We don't do this, but we mask stuff through GEN6_PMINTRMSK. Shouldn't
> >> this be enough to prevent IIR from changing?
> >>
> >> Chris?
> >
> > It should. We should be doing both really, use PM_IMR to treat
> > IMR/IIR/IER consistently with other interrupts, and use the special
> > PMINTRMASK as part of rps power tuning.
> 
> In that case: Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>

Queued for -next, thanks for the patch.
-Daniel

> But one thing makes me wonder: we disable IER on
> gen6_disable_rps_interrupts but never seem to enable it again (except
> for the usual pre/post/uninstall functions)... I know it is not a
> problem introduced by this patch, but shouldn't this be a problem too?

We only need rps interrupts when we are not rpm suspended, so I think this
should be fine. Not fully convinced, so if anyone else has doubts a new
rpm subtest for the pm_rps testcase might be useful to prove this one way
or the other.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch