On Tue, Mar 05, 2013 at 04:23:59PM +0100, Daniel Vetter wrote: > On Tue, Mar 05, 2013 at 04:59:12PM +0200, Ville Syrj?l? wrote: > > On Tue, Mar 05, 2013 at 03:33:26PM +0100, Patrik Jakobsson wrote: > > > > Accroding to the docs these bits don't exist on PCH platforms. > > > > intel_crt_dpms() already has a check for this, so I suppose > > > > intel_disable_crt() should have one too. > > > > > > > > Also I noticed that we seem to have the hsync and vsync disable > > > > bits reversed. At least that's what the docs are telling me. > > > > > > The PCH check just forces suspend and standby to off and we're only doing dpms > > > off in intel_disable_crt() so no need to check it there. > > > > You're right. I assumed that the check would somehow avoid setting > > these bits too, but it doesn't. So I guess we don't really care > > that they don't exist. > > The dpms state gets clamped to the values support by the hw in > intel_crt_dpms. So I think we should care also in intel_crt_disable. The point was that in intel_crt_dpms() we don't care whether the hsync/vsync disable bits actually exist. We just set them whenever the dpms mode warrants it. So for "off" we always set both bits, and "off" is always supported. And intel_crt_disable() is equal to intel_crt_dpms(DRM_MODE_DPMS_OFF) so the behaviour is consistent across the board. Whether or not there could be side effects from setting those bits on PCH plaforms is another matter. If there are, then the clamping stuff is not enough and we need to add PCH checks to both functions. -- Ville Syrj?l? Intel OTC
