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 <ch...@chris-wilson.co.uk> wrote:
>
> On Mon, 3 Jan 2011 13:28:56 -0500, Alexander Lam <lambchop...@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
