On Thu, 3 Feb 2011 16:30:56 -0800, Linus Torvalds [off-list ref] wrote:

On Thu, Feb 3, 2011 at 4:06 PM, Dave Airlie [off-list ref] wrote:

quoted

If we are setting a mode on a connector it automatically will end up
in a DPMS on state,
so this seemed correct from what I can see.

The more I look at that function, the more I disagree with you and
with that patch.

The code is just crazy.

First off, it isn't even necessarily setting a mode to begin with,
because as far as I can tell. If the mode doesn't change, neither
mode_changed nor fb_changed will be true, afaik. There seems to be a
fair amount of code there explicitly to avoid changing modes if not
necessary.

But even _if_ we are setting a mode, if I read the code correctly, the
mode may be set to NULL - which seems to mean "turn it off". In which
case it looks to me that drm_helper_disable_unused_functions() will
actually do a

   (*crtc_funcs->dpms)(crtc, DRM_MODE_DPMS_OFF);

call on the crtc in question. So then blindly just saying "it's mode
DRM_MODE_DPMS_ON" afterwards looks rather bogus to me.

_Maybe_ it would work if it was done before that whole
"disable_unused" logic. Or maybe it should just be done in
drm_crtc_helper_set_mode(), which is what actually sets the mode (but
there's the 'fb_changed' case too)

quoted

A future mode set shouldn't ever not turn the connector on, since
modesetting is an implicit
DPMS,

It sounds like something more subtle than that, though I'm happy to
revert this for now, and let Keith
think about it a bit more.

So I haven't heard anything from Keith. Keith? Just revert it? Or do
you have a patch for people to try?

The goal is to make it so that when you *do* set a mode, DPMS gets set
to ON (as the monitor will actually be "on" at that point). Here's a
patch which does the DPMS_ON precisely when setting a mode.

Dave thinks we should instead force dpms to match crtc->enabled, I'd
rather have dpms get set when we know the hardware has been changed.

(note, this patch compiles, but is otherwise only lightly tested).

From 38507bb3a67441425e11085d17d727f3b230f927 Mon Sep 17 00:00:00 2001
From: Keith Packard <keithp@keithp.com>
Date: Thu, 3 Feb 2011 16:57:28 -0800
Subject: [PATCH] drm: Only set DPMS ON when actually configuring a mode

In drm_crtc_helper_set_config, instead of always forcing all outputs
to DRM_MODE_DPMS_ON, only set them if the CRTC is actually getting a
mode set, as any mode set will turn all outputs on.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/drm_crtc_helper.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 952b3d4..17459ee 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c

@@ -665,6 +665,12 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 				ret = -EINVAL;
 				goto fail;
 			}
+			DRM_DEBUG_KMS("Setting connector DPMS state to on\n");
+			for (i = 0; i < set->num_connectors; i++) {
+				DRM_DEBUG_KMS("\t[CONNECTOR:%d:%s] set DPMS on\n", set->connectors[i]->base.id,
+					      drm_get_connector_name(set->connectors[i]));
+				set->connectors[i]->dpms = DRM_MODE_DPMS_ON;
+			}
 		}
 		drm_helper_disable_unused_functions(dev);
 	} else if (fb_changed) {

@@ -681,12 +687,6 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 			goto fail;
 		}
 	}
-	DRM_DEBUG_KMS("Setting connector DPMS state to on\n");
-	for (i = 0; i < set->num_connectors; i++) {
-		DRM_DEBUG_KMS("\t[CONNECTOR:%d:%s] set DPMS on\n", set->connectors[i]->base.id,
-			      drm_get_connector_name(set->connectors[i]));
-		set->connectors[i]->dpms = DRM_MODE_DPMS_ON;
-	}
 
 	kfree(save_connectors);
 	kfree(save_encoders);

-- 
1.7.2.3

-- 
keith.packard@intel.com