On Thu, 3 Feb 2011 16:30:56 -0800, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Feb 3, 2011 at 4:06 PM, Dave Airlie <airlied@xxxxxxxxx> 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@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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@xxxxxxxxx
Attachment:
pgpUbZ0iRrcZx.pgp
Description: PGP signature
