[PATCH] xf86-video-intel: change order of DPMS operations

[PATCH] xf86-video-intel: change order of DPMS operations

[Simon Que]

The operations when setting dpms on should be in the order opposite
of what's done when setting dpms off.

This is because of potentially conflicting effects:
~ drmModeConnectoSetProperty() enables/disables the backlight driver.
Some backlight drivers such as intel_backlight set the backlight to 0
when disabled and to max when enabled.
~ intel_output_dpms_backlight() saves the backlight value when turning
DPMS off and restores it when turning DPMS on.

Here's the current order of operations:

xset dpms force off (backlight is nonzero)
   drmModeConnectoSetProperty(DPMSModeOff)
      kernel: disable backlight, backlight=0
   intel_output_dpms_backlight(DPMSModeOff)
      save backlight value (0) <-- it has been set to 0 by kernel
      set backlight to 0

xset dpms force on
   drmModeConnectoSetProperty(DPMSModeOn)
      kernel: enable backlight, backlight=max
   intel_output_dpms_backlight(DPMSModeOn)
      set backlight to saved value (0)

The correct way to do this would be to reverse the operations during
xset dpms force off:
   intel_output_dpms_backlight(DPMSModeOff)
      save backlight value (nonzero)
      set backlight to 0
   drmModeConnectoSetProperty(DPMSModeOff)
      kernel: enable backlight, backlight=0

This restores the saved nonzero backlight value during the force on.

Signed-off-by: Simon Que <sque@chromium.org>
---
 src/intel_display.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/intel_display.c b/src/intel_display.c
index 2183f4d..2a989d1 100644
--- a/src/intel_display.c
+++ b/src/intel_display.c
@@ -944,13 +944,19 @@ intel_output_dpms(xf86OutputPtr output, int dpms)
 			continue;
 
 		if (!strcmp(props->name, "DPMS")) {
+			/* Make sure to reverse the order between on and off. */
+			if (dpms == DPMSModeOff)
+				intel_output_dpms_backlight(output,
+							    intel_output->dpms_mode,
+							    dpms);
 			drmModeConnectorSetProperty(mode->fd,
 						    intel_output->output_id,
 						    props->prop_id,
 						    dpms);
-			intel_output_dpms_backlight(output,
-						      intel_output->dpms_mode,
-						      dpms);
+			if (dpms != DPMSModeOff)
+				intel_output_dpms_backlight(output,
+							    intel_output->dpms_mode,
+							    dpms);
 			intel_output->dpms_mode = dpms;
 			drmModeFreeProperty(props);
 			return;
-- 
1.7.3.1

Re: [PATCH] xf86-video-intel: change order of DPMS operations

[Chris Wilson]

On Wed,  7 Dec 2011 16:20:41 -0800, Simon Que <sque@chromium.org> wrote:
> The operations when setting dpms on should be in the order opposite
> of what's done when setting dpms off.
> 
> This is because of potentially conflicting effects:
> ~ drmModeConnectoSetProperty() enables/disables the backlight driver.
> Some backlight drivers such as intel_backlight set the backlight to 0
> when disabled and to max when enabled.
> ~ intel_output_dpms_backlight() saves the backlight value when turning
> DPMS off and restores it when turning DPMS on.
> 
> Here's the current order of operations:
> 
> xset dpms force off (backlight is nonzero)
>    drmModeConnectoSetProperty(DPMSModeOff)
>       kernel: disable backlight, backlight=0
>    intel_output_dpms_backlight(DPMSModeOff)
>       save backlight value (0) <-- it has been set to 0 by kernel
>       set backlight to 0
> 
> xset dpms force on
>    drmModeConnectoSetProperty(DPMSModeOn)
>       kernel: enable backlight, backlight=max
>    intel_output_dpms_backlight(DPMSModeOn)
>       set backlight to saved value (0)
> 
> The correct way to do this would be to reverse the operations during
> xset dpms force off:
>    intel_output_dpms_backlight(DPMSModeOff)
>       save backlight value (nonzero)
>       set backlight to 0
>    drmModeConnectoSetProperty(DPMSModeOff)
>       kernel: enable backlight, backlight=0
> 
> This restores the saved nonzero backlight value during the force on.
> 
> Signed-off-by: Simon Que <sque@chromium.org>

I had to remind myself why both the driver and the kernel are both
touching the backlight across DPMS; the answer as I see it is that the
kernel only knows about the raw backlight interface whereas the driver
adjusts it via the preferred interface (which should handle the cases
where the backlight modulation is handled independently of the PWM
registers). So given that there seems to be a need for the driver to
set the backlight, the order it does so is important given the
interaction with the kernel.

Thanks,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] xf86-video-intel: change order of DPMS operations

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 695 bytes --]

On Thu, 08 Dec 2011 14:10:06 +0000, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> I had to remind myself why both the driver and the kernel are both
> touching the backlight across DPMS; the answer as I see it is that the
> kernel only knows about the raw backlight interface whereas the driver
> adjusts it via the preferred interface (which should handle the cases
> where the backlight modulation is handled independently of the PWM
> registers).

Something in the kernel should be managing the backlight or we won't be
doing this right when fbdev is running. User space should not have to
know about the vagaries of the kernel backlight land.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] xf86-video-intel: change order of DPMS operations

[Chris Wilson]

On Thu, 08 Dec 2011 08:35:12 -0800, Keith Packard <keithp@keithp.com> wrote:
> On Thu, 08 Dec 2011 14:10:06 +0000, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > I had to remind myself why both the driver and the kernel are both
> > touching the backlight across DPMS; the answer as I see it is that the
> > kernel only knows about the raw backlight interface whereas the driver
> > adjusts it via the preferred interface (which should handle the cases
> > where the backlight modulation is handled independently of the PWM
> > registers).
> 
> Something in the kernel should be managing the backlight or we won't be
> doing this right when fbdev is running. User space should not have to
> know about the vagaries of the kernel backlight land.

The tricky part is figuring out the right interface to use. Matthew
was working upon such a mechanism to provide userspace with the ability
to actually find the backlight controller for a respective output, but I
don't believe there was ever a kernel API for i915 to call.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre