Netdev List

* Re: 2.6.38-rc3-git1: Reported regressions 2.6.36 -> 2.6.37
From: Keith Packard @ 2011-02-04  1:05 UTC (permalink / raw)
  To: Linus Torvalds, Dave Airlie
  Cc: Linux SCSI List, Linux ACPI, Takashi Iwai, Carlos Mafra,
	Linux Wireless List, Linux Kernel Mailing List, DRI,
	Rafael J. Wysocki, Florian Mickler, Network Development,
	Dave Airlie, Andrew Morton, Kernel Testers List, Linux PM List,
	Maciej Rutecki
In-Reply-To: <AANLkTimQPPDyBwkN0HWM2+bPcbzVd8YHZvn2iR8MVzfL@mail.gmail.com>


[-- Attachment #1.1: Type: text/plain, Size: 4011 bytes --]

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);
-- 
1.7.2.3

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related