On Tue, Mar 5, 2013 at 3:59 PM, Ville Syrj?l? <ville.syrjala at linux.intel.com> 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. > >> I'm looking at the 965/G35 PRM and the "sync disable" are defined correctly but >> used incorrectly in intel_disable_crt(). That's what my patch fixes. I haven't >> checked the other PRMs. Is it different on newer hardware? > > This is what the docs say: > > 11:10 Monitor DPMS: (for CRT port) ... > ... > 00 = ... (will not affect sync pulses) > 01 = ... (HSYNC pulses, VSYNC does not) > 10 = ... (VSYNC pulses, HSYNC does not) > 11 = ... (Neither HSYNC nor VSYNC pulses) > > These are our definintions: > > #define ADPA_VSYNC_CNTL_DISABLE (1<<11) > #define ADPA_HSYNC_CNTL_DISABLE (1<<10) > > As you can see they don't match. Aha, you're right. I thought you meant disable = 0 and enable = 1. So when we're doing suspend we are in fact doing standby and vice versa. -Patrik
