| Message ID | AANLkTi=7EOih=fY5FCvhQbewSc=3a49cYVaKJRWM+-3d@mail.gmail.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Feb 3, 2011 at 2:10 PM, Linus Torvalds <torvalds@linux-foundation.org> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 4, 2011 at 8:10 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Thu, Feb 3, 2011 at 1:56 PM, Carlos Mafra <crmafra2@gmail.com> wrote: >>> >>> I added https://bugzilla.kernel.org/show_bug.cgi?id=24982 to the list of >>> post-2.6.36 regressions for further tracking. >> >> 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 netdev" 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 <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? Linus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 4, 2011 at 10:30 AM, 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. 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" 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 5:05 PM, Keith Packard <keithp@keithp.com> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 3 Feb 2011 17:11:14 -0800, Linus Torvalds <torvalds@linux-foundation.org> 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.
On Fri, Feb 4, 2011 at 11:11 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Thu, Feb 3, 2011 at 5:05 PM, Keith Packard <keithp@keithp.com> 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? 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 3.Feb'11 at 17:11:14 -0800, Linus Torvalds wrote: > On Thu, Feb 3, 2011 at 5:05 PM, Keith Packard <keithp@keithp.com> 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? Yes! (tested on top of 2.6.38-rc3+). Thanks to everyone involved! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
At Thu, 3 Feb 2011 17:11:14 -0800, Linus Torvalds wrote: > > On Thu, Feb 3, 2011 at 5:05 PM, Keith Packard <keithp@keithp.com> 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? 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
drivers/gpu/drm/drm_crtc_helper.c | 6 +++--- include/drm/drm_mode.h | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 952b3d4..7f585ed 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -681,11 +681,11 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set) goto fail; } } - DRM_DEBUG_KMS("Setting connector DPMS state to on\n"); + DRM_DEBUG_KMS("Setting connector DPMS state to 'unknown'\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_DEBUG_KMS("\t[CONNECTOR:%d:%s] set DPMS 'unknown'\n", set->connectors[i]->base.id, drm_get_connector_name(set->connectors[i])); - set->connectors[i]->dpms = DRM_MODE_DPMS_ON; + set->connectors[i]->dpms = DRM_MODE_DPMS_UNKNOWN; } kfree(save_connectors); diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h index 0fc7397..4b5144c 100644 --- a/include/drm/drm_mode.h +++ b/include/drm/drm_mode.h @@ -59,6 +59,7 @@ /* DPMS flags */ /* bit compatible with the xorg definitions. */ +#define DRM_MODE_DPMS_UNKNOWN (-1) #define DRM_MODE_DPMS_ON 0 #define DRM_MODE_DPMS_STANDBY 1 #define DRM_MODE_DPMS_SUSPEND 2