[PATCH] drm/i915/crt: Only support DPMS On/Off with the PCH registers

[PATCH] drm/i915/crt: Only support DPMS On/Off with the PCH registers

[Chris Wilson]

As part of the PCH split, the ability to control CRT standby/suspend
states was defeatured; the bits are now marked reserved and apparently
have no effect.

Reported-by: Ouping Zhang <ouping.zhang@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48491
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_crt.c |   24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 70b0f1a..039f101 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -55,18 +55,30 @@ static struct intel_crt *intel_attached_crt(struct drm_connector *connector)
 			    struct intel_crt, base);
 }
 
+static void intel_pch_crt_dpms(struct drm_encoder *encoder, int mode)
+{
+	struct drm_device *dev = encoder->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 val;
+
+	val = I915_READ(PCH_ADPA);
+	if (mode == DRM_MODE_DPMS_ON)
+		val |= ADPA_DAC_ENABLE;
+	else
+		val &= ~ADPA_DAC_ENABLE;
+	I915_WRITE(PCH_ADPA, val);
+}
+
 static void intel_crt_dpms(struct drm_encoder *encoder, int mode)
 {
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 temp, reg;
+	u32 temp;
 
 	if (HAS_PCH_SPLIT(dev))
-		reg = PCH_ADPA;
-	else
-		reg = ADPA;
+		return intel_pch_crt_dpms(encoder, mode);
 
-	temp = I915_READ(reg);
+	temp = I915_READ(ADPA);
 	temp &= ~(ADPA_HSYNC_CNTL_DISABLE | ADPA_VSYNC_CNTL_DISABLE);
 	temp &= ~ADPA_DAC_ENABLE;
 
@@ -85,7 +97,7 @@ static void intel_crt_dpms(struct drm_encoder *encoder, int mode)
 		break;
 	}
 
-	I915_WRITE(reg, temp);
+	I915_WRITE(ADPA, temp);
 }
 
 static int intel_crt_mode_valid(struct drm_connector *connector,
-- 
1.7.9.5

Re: [PATCH] drm/i915/crt: Only support DPMS On/Off with the PCH registers

[Daniel Vetter]

On Tue, Apr 10, 2012 at 09:35:30AM +0100, Chris Wilson wrote:
> As part of the PCH split, the ability to control CRT standby/suspend
> states was defeatured; the bits are now marked reserved and apparently
> have no effect.
> 
> Reported-by: Ouping Zhang <ouping.zhang@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48491
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

We've discussed a bit whether we should split up helper_funcs vtable to
avoid the is(PCH_SPLIT) check. We concluded it's not yet ugly enough ;-)

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915/crt: Only support DPMS On/Off with the PCH registers

[Adam Jackson]

On 4/10/12 4:35 AM, Chris Wilson wrote:
> As part of the PCH split, the ability to control CRT standby/suspend
> states was defeatured; the bits are now marked reserved and apparently
> have no effect.

Are the intermediate states even tested?  I have vague memories of them 
not working.  I'd be just as happy to see them aliased to Off.

- ajax

Re: [PATCH] drm/i915/crt: Only support DPMS On/Off with the PCH registers

[Chris Wilson]

On Tue, 10 Apr 2012 09:20:38 -0400, Adam Jackson <ajax@redhat.com> wrote:
> On 4/10/12 4:35 AM, Chris Wilson wrote:
> > As part of the PCH split, the ability to control CRT standby/suspend
> > states was defeatured; the bits are now marked reserved and apparently
> > have no effect.
> 
> Are the intermediate states even tested?  I have vague memories of them 
> not working.  I'd be just as happy to see them aliased to Off.

All I can say is that QA reported that ILK+ suspend/standby did not work
with an external VGA monitor. However, that doesn't say whether or not
they ever tested earlier generations if it took a few years to spot the
behavioural regression.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915/crt: Only support DPMS On/Off with the PCH registers

[Keith Packard]

<#part sign=pgpmime>
On Tue, 10 Apr 2012 09:20:38 -0400, Adam Jackson <ajax@redhat.com> wrote:
> On 4/10/12 4:35 AM, Chris Wilson wrote:
> > As part of the PCH split, the ability to control CRT standby/suspend
> > states was defeatured; the bits are now marked reserved and apparently
> > have no effect.
> 
> Are the intermediate states even tested?  I have vague memories of them 
> not working.  I'd be just as happy to see them aliased to Off.

It's worse than that -- iirc, there were patent issues with the
intermediate states which caused them to be removed from some newer
display signaling standards...

As far as I know, only CRTs could usefully use them anyways.

-- 
keith.packard@intel.com