On Tue, Mar 19, 2013 at 03:35:55PM -0700, Jesse Barnes wrote:
> On Tue, 19 Mar 2013 15:27:36 -0700
> Ben Widawsky <ben at bwidawsk.net> wrote:
>
> > On Fri, Mar 08, 2013 at 10:45:58AM -0800, Jesse Barnes wrote:
> > > From: Ben Widawsky <ben at bwidawsk.net>
> > >
> > > Uses slightly different interfaces than other platforms.
> > >
> > > v2: track actual set freq, not requested (Rohit)
> > > fix debug prints in init code (Jesse)
> > >
> > > Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> >
> > One important question is how is our punit code going to interact with
> > the other potential users? It seems a bunch of power management drivers
> > would also want to touch this uC.
>
> Pretty sure the PUnit has to deal with concurrent access... if not
> we'll have to add common routines for all drivers to use and do proper
> locking.
>
> > > +
> > > + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &pval);
> > > + if ((pval >> 8) != val)
> > > + DRM_DEBUG_DRIVER("punit overrode freq: %d requested, but got %d\n",
> > > + val, pval >> 8);
> > I'm debating whether this is a useful thing to do here... You either get
> > the frequency or you don't. Really it would seem more useful to me to
> > check things are sane when you first enter the function (ie. did the
> > last request do what you want). But I don't care what you end up with.
>
> It's not really a matter of sanity, it's more about what state the
> platform is in. If the Punit decides things are getting too hot for
> example, it may clamp your freq down. That's totally ok and normal
> though, and may change in future calls.
>
> > I suppose if you wanted to make things a bit cleaner you could extract
> > just the frequency setting part, since most of this function is
> > identical to gen6 set rps.
>
> I was afraid more changes would creep in over time, but yeah that's a
> possibility. I have a couple more changes here before I consider that.
>
> > > +
> > > + /* Make sure we continue to get interrupts
> > > + * until we hit the minimum or maximum frequencies.
> > > + */
> > > + I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits);
> > > +
> > > + dev_priv->rps.cur_delay = pval >> 8;
> > > +
> > > + trace_intel_gpu_freq_change(val * 50);
> > Based on our IRC discussion, I believe the value in this trace is wrong.
>
> Ah yeah, correct. I can just trace val here maybe, or I"ll have to put
> this off until I post the real frequencies.
>
> > > +static void valleyview_enable_rps(struct drm_device *dev)
> > > +{
> > > + struct drm_i915_private *dev_priv = dev->dev_private;
> > > + struct intel_ring_buffer *ring;
> > > + u32 gtfifodbg, val;
> > > + int i;
> > > +
> > > + WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> >
> > Should we check FB_GFX_TURBO_EN_FUSE here?
>
> I guess it wouldn't hurt.
>
> > > +
> > > + if ((gtfifodbg = I915_READ(GTFIFODBG))) {
> > > + DRM_ERROR("GT fifo had a previous error %x\n", gtfifodbg);
> > > + I915_WRITE(GTFIFODBG, gtfifodbg);
> > > + }
> > > +
> > > + gen6_gt_force_wake_get(dev_priv);
> > > +
> > > + I915_WRITE(GEN6_RC_SLEEP, 0);
> > You've dropped GEN6_RC1_WAKE_RATE_LIMIT. Intentional?
>
> Uh I should have commented that. I *think* it was intentional since
> RC1 doesn't exist on VLV, but I'll have to check.
>
> > > + I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 0x00280000);
> > > + I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000);
> > > + I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 0x19);
> >
> > Don't use 0x19, use 25 since we use 25 in the gen6 path
>
> Ok.
>
> > > +
> > > + for_each_ring(ring, dev_priv, i)
> > > + I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10);
> > > +
> > > + I915_WRITE(GEN6_RC1e_THRESHOLD, 1000);
> > > + I915_WRITE(GEN6_RC6_THRESHOLD, 0xc350);
> > > +
> > > + I915_WRITE(GEN6_RP_UP_THRESHOLD, 59400);
> > > + I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 245000);
> > > + I915_WRITE(GEN6_RP_UP_EI, 66000);
> > > + I915_WRITE(GEN6_RP_DOWN_EI, 350000);
> > > +
> > > + I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10);
> >
> > I can't find most of the above values, but, I'll assume they're correct.
> > I'd also suggest converging on either all decimal, or all hex, just to
> > make things less confusing.
>
> A comment about units probably wouldn't hurt either. Ok.
>
> > > +
> > > + I915_WRITE(GEN6_RP_CONTROL,
> > > + GEN6_RP_MEDIA_TURBO |
> > > + GEN6_RP_MEDIA_HW_NORMAL_MODE |
> > > + GEN6_RP_MEDIA_IS_GFX |
> > > + GEN6_RP_ENABLE |
> > > + GEN6_RP_UP_BUSY_AVG |
> > > + GEN6_RP_DOWN_IDLE_CONT);
> > > +
> >
> > > + /* allows RC6 residency counter to work */
> > > + I915_WRITE(0x138104, 0xffff00ff);
> > This is writing read only registers. Should be:
> > I915_WRITE(0x138104, 0x800000ff);
> >
> > Also, just for though, we may want to use the upper bits instead of the
> > lower ones...
>
> I think the upper bits are a mask, and all the low 8 bits are
> writeable, so maybe 0x00ff00ff? I'll have to test that.
This was a typo on my part, it should be 0x80ff00ff. Sorry about that. I
haven't read the rest of the email yet. It popped into my head while I
was out.
>
> >
> > > + I915_WRITE(GEN6_RC_CONTROL, GEN6_RC_CTL_HW_ENABLE);
> > > +
> >
> > So the following stuff doesn't seem to jive in the docs I'm reading. Can
> > you double check your docs, and the silicon. I tried to punt this to IRC
> > but you didn't respond. These IOSF registers are all 32 bits, so I'm not
> > sure how this is supposed to work.
> >
> > > + valleyview_punit_read(dev_priv, PUNIT_FUSE_BUS1, &val);
> > > + DRM_DEBUG_DRIVER("max GPU freq: %d\n", val);
> > > + dev_priv->rps.max_delay = val;
> > val >> 16 && 0xff?
> >
> >
> > > +
> > > + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_LFM, &val);
> > > + DRM_DEBUG_DRIVER("min GPU freq: %d\n", val);
> > > + dev_priv->rps.min_delay = val;
> > val & 0xff?
> > > +
> > > + valleyview_punit_read(dev_priv, PUNIT_FUSE_BUS2, &val);
> > > + DRM_DEBUG_DRIVER("max GPLL freq: %d\n", val);
> >
> > One doc suggests this is actually offset 0x87, bits 7:0
>
> Hm I'll check these out; the different docs talk about different bit
> fields, but I think they're offsets into the fuse bus array.
>
> >
> > > +
> > > + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &val);
> > > + DRM_DEBUG_DRIVER("DDR speed: ");
> > > + if (drm_debug & DRM_UT_DRIVER) {
> > > + if (((val >> 6) & 3) == 0) {
> > > + dev_priv->mem_freq = 800;
> > > + printk("800 MHz\n");
> > > + } else if (((val >> 6) & 3) == 1) {
> > > + printk("1066 MHz\n");
> > > + dev_priv->mem_freq = 1066;
> > > + } else if (((val >> 6) & 3) == 2) {
> > > + printk("1333 MHz\n");
> > > + dev_priv->mem_freq = 1333;
> > > + } else if (((val >> 6) & 3) == 3)
> > > + printk("invalid\n");
> > > + }
> >
> > printk?
> > could be one line, but would print 1332:
> > dev_priv->mem_freq = 800 + (266 * (val >> 6) & 3))
>
> Yeah because the DRM_DEBUG above doesn't have a newline. I like your
> calculation better.
>
> Thanks for checking these out; I know wading through these docs is no
> fun.
>
> --
> Jesse Barnes, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ben Widawsky, Intel Open Source Technology Center
