From: Carlos R. Mafra <hidden> Date: 2011-02-03 11:20:47
On Thu 3.Feb'11 at 1:03:41 +0100, Rafael J. Wysocki wrote:
If you know of any other unresolved post-2.6.36 regressions, please let us know
either and we'll add them to the list. Also, please let us know if any
of the entries below are invalid.
I'm sorry if I'm overlooking something, but as far as I can see the regression
reported here:
https://lkml.org/lkml/2011/1/24/457
is not in the list (update on that report: reverting that commit on top of
2.6.37 fixes the issue).
After some time, I also ended up finding an earlier report in the kernel bugzilla
which I think is the same regression (it was bisected to the same commit):
https://bugzilla.kernel.org/show_bug.cgi?id=24982
but I do not see it in the list either, even though it's marked as a
regression in the bugzilla.
The issue was also present in 2.6.38-rc2 last time I tested.
On Thu, Feb 3, 2011 at 3:23 AM, Carlos R. Mafra [off-list ref] wrote:
On Thu 3.Feb'11 at 1:03:41 +0100, Rafael J. Wysocki wrote:
quoted
If you know of any other unresolved post-2.6.36 regressions, please let us know
either and we'll add them to the list. Also, please let us know if any
of the entries below are invalid.
I'm sorry if I'm overlooking something, but as far as I can see the regression
reported here:
https://lkml.org/lkml/2011/1/24/457
is not in the list (update on that report: reverting that commit on top of
2.6.37 fixes the issue).
Ok, added Keith and Dave to the cc, since they are the signers of that commit.
After some time, I also ended up finding an earlier report in the kernel bugzilla
which I think is the same regression (it was bisected to the same commit):
https://bugzilla.kernel.org/show_bug.cgi?id=24982
but I do not see it in the list either, even though it's marked as a
regression in the bugzilla.
The issue was also present in 2.6.38-rc2 last time I tested.
Just to confirm, can you also check -rc3? I'm pretty sure nothing has
changed, but there were a few drm patches after -rc2, so it's alsways
good to double-check.
Keithp?
Linus
At Thu, 3 Feb 2011 07:42:05 -0800,
Linus Torvalds wrote:
On Thu, Feb 3, 2011 at 3:23 AM, Carlos R. Mafra [off-list ref] wrote:
quoted
On Thu 3.Feb'11 at 1:03:41 +0100, Rafael J. Wysocki wrote:
quoted
If you know of any other unresolved post-2.6.36 regressions, please let us know
either and we'll add them to the list. Also, please let us know if any
of the entries below are invalid.
I'm sorry if I'm overlooking something, but as far as I can see the regression
reported here:
https://lkml.org/lkml/2011/1/24/457
is not in the list (update on that report: reverting that commit on top of
2.6.37 fixes the issue).
Ok, added Keith and Dave to the cc, since they are the signers of that commit.
quoted
After some time, I also ended up finding an earlier report in the kernel bugzilla
which I think is the same regression (it was bisected to the same commit):
https://bugzilla.kernel.org/show_bug.cgi?id=24982
but I do not see it in the list either, even though it's marked as a
regression in the bugzilla.
The issue was also present in 2.6.38-rc2 last time I tested.
Just to confirm, can you also check -rc3? I'm pretty sure nothing has
changed, but there were a few drm patches after -rc2, so it's alsways
good to double-check.
The problem I reported in the bugzilla above is still present in
2.6.38-rc3. I'm pretty sure that's the same issue as Carlos' case.
thanks,
Takashi
From: Rafael J. Wysocki <hidden> Date: 2011-02-03 19:09:32
On Thursday, February 03, 2011, Takashi Iwai wrote:
At Thu, 3 Feb 2011 07:42:05 -0800,
Linus Torvalds wrote:
quoted
On Thu, Feb 3, 2011 at 3:23 AM, Carlos R. Mafra [off-list ref] wrote:
quoted
On Thu 3.Feb'11 at 1:03:41 +0100, Rafael J. Wysocki wrote:
quoted
If you know of any other unresolved post-2.6.36 regressions, please let us know
either and we'll add them to the list. Also, please let us know if any
of the entries below are invalid.
I'm sorry if I'm overlooking something, but as far as I can see the regression
reported here:
https://lkml.org/lkml/2011/1/24/457
is not in the list (update on that report: reverting that commit on top of
2.6.37 fixes the issue).
Ok, added Keith and Dave to the cc, since they are the signers of that commit.
quoted
After some time, I also ended up finding an earlier report in the kernel bugzilla
which I think is the same regression (it was bisected to the same commit):
https://bugzilla.kernel.org/show_bug.cgi?id=24982
but I do not see it in the list either, even though it's marked as a
regression in the bugzilla.
The issue was also present in 2.6.38-rc2 last time I tested.
Just to confirm, can you also check -rc3? I'm pretty sure nothing has
changed, but there were a few drm patches after -rc2, so it's alsways
good to double-check.
The problem I reported in the bugzilla above is still present in
2.6.38-rc3. I'm pretty sure that's the same issue as Carlos' case.
From: Carlos Mafra <hidden> Date: 2011-02-03 21:56:33
On Thu, Feb 3, 2011 at 8:09 PM, Rafael J. Wysocki [off-list ref] wrote:
On Thursday, February 03, 2011, Takashi Iwai wrote:
quoted
At Thu, 3 Feb 2011 07:42:05 -0800,
Linus Torvalds wrote:
quoted
On Thu, Feb 3, 2011 at 3:23 AM, Carlos R. Mafra [off-list ref] wrote:
quoted
On Thu 3.Feb'11 at 1:03:41 +0100, Rafael J. Wysocki wrote:
quoted
quoted
Just to confirm, can you also check -rc3? I'm pretty sure nothing has
changed, but there were a few drm patches after -rc2, so it's alsways
good to double-check.
The problem I reported in the bugzilla above is still present in
2.6.38-rc3. I'm pretty sure that's the same issue as Carlos' case.
I also tested on 2.6.38-rc3+ now and the issue is not solved,
just like Takashi expected.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
I also tested on 2.6.38-rc3+ now and the issue is not solved,
just like Takashi expected.
Hmm. That commit (bf9dc102e284) still reverts cleanly.
Keith, Dave, should we just revert it? It's definitely a regression,
and we do _not_ allow "fixes" to one thing that just causes a
regression to another.
Quite frankly, I think it's totally wrong to just blindly set DPMS
status to ON like that. It's as wrong as it was to leave it off, and
the regressions reported are basically mirror images of the exact same
bug that that commit tried to fix.
IOW, the commit message says:
When setting a new crtc configuration, force the DPMS state of all
connectors to ON. Otherwise, they'll be left at OFF and a future mode set
that disables the specified connector will not turn the connector off.
but setting it to ON doesn't actually _fix_ anything, because you just
get the exact same issue in reverse, ie you just get
.. and a future mode set that ENables the specified connector will
not turn the connector ON.
instead. Which is exactly what Carlos and Takashi are reporting.
Maybe the right thing to do is to set it to 'unknown', something like this.
TOTALLY UNTESTED!
Linus
On Thu, Feb 3, 2011 at 2:10 PM, Linus Torvalds
[off-list ref] wrote:
Maybe the right thing to do is to set it to 'unknown', something like this.
TOTALLY UNTESTED!
Doing some grepping and "git blame", I found this: commit 032d2a0d068
("drm/i915: Prevent double dpms on") which took a very similar
approach, except it just uses "-1" directly instead of introducing
that DRM_MODE_DPMS_UNKNOWN concept.
So I suspect that my patch is going in the right direction, and
judging by the comments in that commit, we probably should do this
correctly at the dri level rather than have drivers see those stupid
"let's set dpms to the state that it was already in". But that very
much does require that kind of "UNKNOWN" state option.
So maybe we can get that patch tested (and acked if it works)? Carlos, Takashi?
Linus
I also tested on 2.6.38-rc3+ now and the issue is not solved,
just like Takashi expected.
Hmm. That commit (bf9dc102e284) still reverts cleanly.
Keith, Dave, should we just revert it? It's definitely a regression,
and we do _not_ allow "fixes" to one thing that just causes a
regression to another.
Quite frankly, I think it's totally wrong to just blindly set DPMS
status to ON like that. It's as wrong as it was to leave it off, and
the regressions reported are basically mirror images of the exact same
bug that that commit tried to fix.
IOW, the commit message says:
When setting a new crtc configuration, force the DPMS state of all
connectors to ON. Otherwise, they'll be left at OFF and a future mode set
that disables the specified connector will not turn the connector off.
but setting it to ON doesn't actually _fix_ anything, because you just
get the exact same issue in reverse, ie you just get
.. and a future mode set that ENables the specified connector will
not turn the connector ON.
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.
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.
Dave.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 3, 2011 at 4:06 PM, Dave Airlie [off-list ref] 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?
Linus
From: Dave Airlie <airlied@gmail.com> Date: 2011-02-04 00:45:23
On Fri, Feb 4, 2011 at 10:30 AM, 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.
Good point, I'm just trying to get -rc3 onto a machine where I can
reproduce this now, unfortunately that looks like the machine with the
1.8" disk, so this could take a little while.
hopefully Keith will decloak and tell us more.
Dave.
On Thu, Feb 3, 2011 at 5:05 PM, Keith Packard [off-list ref] wrote:
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.
Ok, patch looks sane, but it does leave me with the "what about the
'fb_changed' case?" question. Is that case basically guaranteed to not
change any existing dpms state?
(note, this patch compiles, but is otherwise only lightly tested).
Carlos? Takashi? Ignore my crazy patch, try this one instead. Does it
fix things for you?
Linus
From: Keith Packard <keithp@keithp.com> Date: 2011-02-04 01:14:28
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(-)
@@ -665,6 +665,12 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)ret=-EINVAL;gotofail;}+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);}elseif(fb_changed){
@@ -681,12 +687,6 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)gotofail;}}-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);
From: Keith Packard <keithp@keithp.com> Date: 2011-02-04 01:41:55
On Thu, 3 Feb 2011 17:11:14 -0800, Linus Torvalds [off-list ref] wrote:
Ok, patch looks sane, but it does leave me with the "what about the
'fb_changed' case?" question. Is that case basically guaranteed to not
change any existing dpms state?
None of the existing drivers turn anything on or off in the
mode_set_base code; the fix I intended was purely for the mode_set case,
which always turns on all of the connected outputs. I just screwed up
and stuck it in the wrong place.
--
keith.packard@intel.com
From: Dave Airlie <airlied@gmail.com> Date: 2011-02-04 01:42:37
On Fri, Feb 4, 2011 at 11:11 AM, Linus Torvalds
[off-list ref] wrote:
On Thu, Feb 3, 2011 at 5:05 PM, Keith Packard [off-list ref] wrote:
quoted
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.
Ok, patch looks sane, but it does leave me with the "what about the
'fb_changed' case?" question. Is that case basically guaranteed to not
change any existing dpms state?
Yes its inconsistent behaviour but nothing in the fb_changed case will
affect the DPMS
state. I expect we should probably do that so all paths via that
function turn DPMS on,
and it'll be consistent, might be something for 39.
Dave.
From: Carlos R. Mafra <hidden> Date: 2011-02-04 07:03:25
On Thu 3.Feb'11 at 17:11:14 -0800, Linus Torvalds wrote:
On Thu, Feb 3, 2011 at 5:05 PM, Keith Packard [off-list ref] wrote:
quoted
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.
Ok, patch looks sane, but it does leave me with the "what about the
'fb_changed' case?" question. Is that case basically guaranteed to not
change any existing dpms state?
quoted
(note, this patch compiles, but is otherwise only lightly tested).
Carlos? Takashi? Ignore my crazy patch, try this one instead. Does it
fix things for you?
Yes! (tested on top of 2.6.38-rc3+).
Thanks to everyone involved!
At Thu, 3 Feb 2011 17:11:14 -0800,
Linus Torvalds wrote:
On Thu, Feb 3, 2011 at 5:05 PM, Keith Packard [off-list ref] wrote:
quoted
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.
Ok, patch looks sane, but it does leave me with the "what about the
'fb_changed' case?" question. Is that case basically guaranteed to not
change any existing dpms state?
quoted
(note, this patch compiles, but is otherwise only lightly tested).
Carlos? Takashi? Ignore my crazy patch, try this one instead. Does it
fix things for you?
Yes, the patch fixes the issue with xrandr off and on.
However, another issue I reported in that bugzilla still remains:
namely, DPMS value returned via ioctl or obtained via sysfs is
inconsistent with the actually applied value. The reason is that
there are two places keeping the current DPMS values, in connector and
in crtc device properties. A similar fix like my patch in the
bugzilla would be still needed, I guess.
thanks,
Takashi