On Thu, 3 Feb 2011 16:30:56 -0800, Linus Torvalds torvalds@linux-foundation.org wrote:
On Thu, Feb 3, 2011 at 4:06 PM, Dave Airlie airlied@gmail.com wrote:
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)
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);