[Intel-gfx] [PATCH] allow 945 to control self refresh automatically

[Alexander Lam]

Hi,

I probably should mention that the patch is in reply to: (I got Li's ack here)
http://lists.freedesktop.org/archives/intel-gfx/2010-October/008302.html

School kept me busy since then and I haven't been able to find any free
time until this winter break to respin. (College unexpectedly took more time
than I imagined)

On Mon, Jan 3, 2011 at 2:33 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
>
> On Mon,  3 Jan 2011 13:28:56 -0500, Alexander Lam <lambchop468 at gmail.com> wrote:
> > I changed 945's self refresh to work without the need for the driver to
> > enable/disable self refresh manually based on the idle state of the gpu.
> > This is much better than enabling/disabling self refresh for various
> > reasons, including staying in a lower power state for more time and
> > avoiding the need for cpu cycles.
> >
> > Something must have been fixed in the driver between the initial
> > implementation of 945 self refresh and now.
> > (maybe 944001201ca0196bcdb088129e5866a9f379d08c: drm/i915: enable low
> > power render writes on GEN3 hardware?)
>
> Considering that there is no rationale in the git history as why it was
> done manually, maybe you could explain the reason why it could not have
> been automatically before? Was it simply causing hangs? Do you have any
> measurements that show power/performance impacts of the switch?

It is explained in commit ee980b80
(same as Jesse says)

I don't have any measurements (although the absolute idle power savings
is the same before and after). The problem is that I don't really have a way
to reliably reproduce a workload situation for this and measure average
power consumption (I guess I could throw together a test, but I don't think
I have time for that). Anyway, this would totally solve the problem fixed
by 060e645a.

> > This patch probably doesn't cover all cases concerning if SR should
> > be enabled/disabled, not to mention doing things in the wrong order or
> > in the wrong place.
>
> Does this patch introduce bugs? Certainly sounds like it does, but that may
> have just been badly phrased.

What that really is is a disclaimer that I might be programming the
hardware's bits
in the wrong order (i.e. I don't know if we are allowed to write FW_BLC_SELF_EN
before writing the actual watermarks) because I don't have the hardware docs.

> Reading the patch did raise one concern:
> >               /* Turn off self refresh if both pipes are enabled */
> >               if (IS_I945G(dev) || IS_I945GM(dev)) {
> > +                     DRM_DEBUG_KMS("disable memory self refresh on 945\n");
> > +                     sr_enabled = 0;
> >                       I915_WRITE(FW_BLC_SELF, I915_READ(FW_BLC_SELF)
> >                                  & ~FW_BLC_SELF_EN);
>
> This looks wrong, as elsewhere to disable self-refresh we do:
>
> I915_WRITE(FW_BLC_SELF, (I915_READ(FW_BLC_SELF) | FW_BLC_SELF_EN_MASK) & ~FB_BLC_SELF_EN);
>
> This looks to be a bug in the original code, 33c5fd12, but does it mean
> that upon applying your patch that we never disable SR, even when it is
> not supported by the hardware configuration?

ee980b80 uses both methods, so I'm not sure. I figured going with the
original code would be best, but I'm not entirely sure without
docs. I didn't consider that the original code was incorrect, so it
may be because
the manual enable/disable could have masked the bug. I am not sure why this
issue didn't show up in testing....should I respin?


Also, is it possible for me to move
I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN_MASK | FW_BLC_SELF_EN);
to before the watermark is written (to avoid needing sr_enabled; I
used sr_enabled
to keep the ordering of writing these bits the same compared to prior
to this patch)

> -Chrs
>
> --
> Chris Wilson, Intel Open Source Technology Centre

Thanks,

--
Alexander Lam