Re: [PATCH 1/2] drm/i915: mask RPS IRQs properly when disabling RPS

From: Imre Deak <imre.deak@intel.com>
To: Paulo Zanoni <przanoni@gmail.com>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 1/2] drm/i915: mask RPS IRQs properly when disabling RPS
Date: Mon, 01 Dec 2014 19:13:08 +0200	[thread overview]
Message-ID: <1417453988.11576.14.camel@intelbox> (raw)
In-Reply-To: <CA+gsUGQVa=zZvp+eEKQf=mzbZTFMey0SxAx=-SrA4qKCfygF4Q@mail.gmail.com>

On Mon, 2014-12-01 at 14:47 -0200, Paulo Zanoni wrote:
> 2014-12-01 14:34 GMT-02:00 Chris Wilson <chris@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@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@intel.com>
> 
> 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?

Yes, it is a problem, I haven't noticed this..

It wasn't a problem for suspend/resume, since there during resume we
call pre/postinstall which will reenable all RPS interrupts in IER too.
But after patch 2/2, we'd disable RPS interrupts during reset clearing
IER, but wouldn't re-enable them in IER afterwards in
gen6_enable_rps_interrupts().

The solution imo is to unmask interrupts in IER in
gen6_enable_rps_interrupts(), and leave them disabled in
pre/postinstall. I'll look into this.

Thanks,
Imre

> 
> I also wonder if there's a way to greatly simplify all this RPS
> interrupt handling...
> 
> > -Chris
> >
> > --
> > Chris Wilson, Intel Open Source Technology Centre
> 
> 
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx