Re: [PATCH] drm/i915: fix i915_frequency_info on BDW

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Aug 18, 2014 at 11:11:50AM -0700, Rodrigo Vivi wrote:
> On Fri, Aug 15, 2014 at 10:12 AM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 
> > 2014-08-15 13:50 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> > >
> > >
> > >
> > > On Fri, Aug 1, 2014 at 2:14 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> > >>
> > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > >>
> > >> The GEN6_PM* registers don't exist on BDW anymore, so when we read
> > >> this file we trigger unclaimed register errors. The equivalent BDW
> > >> register for PMs is GEN8_GT_I*R(2), so use it.
> > >>
> > >> Testcase: igt/pm_rpm/debugfs-read
> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > >> ---
> > >>  drivers/gpu/drm/i915/i915_debugfs.c | 20 +++++++++++++++-----
> > >>  1 file changed, 15 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > >> b/drivers/gpu/drm/i915/i915_debugfs.c
> > >> index 9e737b7..17bd20ff 100644
> > >> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > >> @@ -1024,6 +1024,7 @@ static int i915_frequency_info(struct seq_file *m,
> > >> void *unused)
> > >>                 u32 rpstat, cagf, reqf;
> > >>                 u32 rpupei, rpcurup, rpprevup;
> > >>                 u32 rpdownei, rpcurdown, rpprevdown;
> > >> +               u32 pm_ier, pm_imr, pm_isr, pm_iir, pm_mask;
> > >>                 int max_freq;
> > >>
> > >>                 /* RPSTAT1 is in the GT power well */
> > >> @@ -1061,12 +1062,21 @@ static int i915_frequency_info(struct seq_file
> > *m,
> > >> void *unused)
> > >>                 gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> > >>                 mutex_unlock(&dev->struct_mutex);
> > >>
> > >> +               if (IS_GEN6(dev) || IS_GEN7(dev)) {
> > >> +                       pm_ier = I915_READ(GEN6_PMIER);
> > >> +                       pm_imr = I915_READ(GEN6_PMIMR);
> > >> +                       pm_isr = I915_READ(GEN6_PMISR);
> > >> +                       pm_iir = I915_READ(GEN6_PMIIR);
> > >> +                       pm_mask = I915_READ(GEN6_PMINTRMSK);
> > >> +               } else {
> > >> +                       pm_ier = I915_READ(GEN8_GT_IER(2));
> > >> +                       pm_imr = I915_READ(GEN8_GT_IMR(2));
> > >> +                       pm_isr = I915_READ(GEN8_GT_ISR(2));
> > >> +                       pm_iir = I915_READ(GEN8_GT_IIR(2));
> > >
> > >
> > > Why do we care only about GT(2) interrupt reg?
> > > What about other 0, 1 and 3 regs?
> >
> > Because, as far as I could see, the GEN8_GT(2) register is the one
> > that seems to be equivalent to the old GEN6_PM register in terms of
> > the functionality debugged by this function: it is the one that
> > contains all the RPS stuff. Another thing that influenced me to take
> > this decision was that, for example, snb_update_pm_irq() touches
> > GEN6_PM, while bdw_update_pm_irq() touches only GEN8_GT(2). But I'm
> > not a great user of this code, so maybe we do want more interrupts.
> > OTOH, if we want all, there's always i915_interrupt_info.
> >
> 
> Yeah, makes sense.
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch