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