Re: [PATCH] allow 945 to control self refresh automatically

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Alexander Lam <lambchop468@gmail.com>lambchop468@gmail.com,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] allow 945 to control self refresh automatically
Date: Mon, 03 Jan 2011 19:33:05 +0000	[thread overview]
Message-ID: <849307$b0cd6l@azsmga001.ch.intel.com> (raw)
In-Reply-To: <1294079336-2744-1-git-send-email-lambchop468@gmail.com>

On Mon,  3 Jan 2011 13:28:56 -0500, Alexander Lam <lambchop468@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?

> 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.

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?
-Chrs

-- 
Chris Wilson, Intel Open Source Technology Centre