drm/i915: Fix DPMS and suspend interaction for intel_panel.c

drm/i915: Fix DPMS and suspend interaction for intel_panel.c

[Indan Zupancic]

drm/i915: Fix DPMS and suspend interaction for intel_panel.c

When suspending intel_panel_disable_backlight() is never called,
but intel_panel_enable_backlight() is called at resume. With the
effect that if the brightness was ever changed after screen
blanking, the wrong brightness gets restored at resume time.

Nothing guarantees that those calls will be balanced, so having
backlight_enabled makes no sense, as the real state can change
without the panel code noticing. So keep things as stateless as
possible.

Signed-off-by: Indan Zupancic <indan@nul.nu>

---

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 456f404..4a3d9ed 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -333,7 +333,6 @@ typedef struct drm_i915_private {

 	/* LVDS info */
 	int backlight_level;  /* restore backlight to this value */
-	bool backlight_enabled;
 	struct drm_display_mode *panel_fixed_mode;
 	struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
 	struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
@@ -1220,10 +1219,6 @@ void i915_debugfs_cleanup(struct drm_minor *minor);
 extern int i915_save_state(struct drm_device *dev);
 extern int i915_restore_state(struct drm_device *dev);

-/* i915_suspend.c */
-extern int i915_save_state(struct drm_device *dev);
-extern int i915_restore_state(struct drm_device *dev);
-
 /* intel_i2c.c */
 extern int intel_setup_gmbus(struct drm_device *dev);
 extern void intel_teardown_gmbus(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 49fb54f..1b5a32d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5924,8 +5924,6 @@ static void intel_setup_outputs(struct drm_device *dev)
 		encoder->base.possible_clones =
 			intel_encoder_clones(dev, encoder->clone_mask);
 	}
-
-	intel_panel_setup_backlight(dev);
 }

 static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2c43104..70e8b82 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -257,7 +257,6 @@ extern void intel_pch_panel_fitting(struct drm_device *dev,
 extern u32 intel_panel_get_max_backlight(struct drm_device *dev);
 extern u32 intel_panel_get_backlight(struct drm_device *dev);
 extern void intel_panel_set_backlight(struct drm_device *dev, u32 level);
-extern void intel_panel_setup_backlight(struct drm_device *dev);
 extern void intel_panel_enable_backlight(struct drm_device *dev);
 extern void intel_panel_disable_backlight(struct drm_device *dev);

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index d860abe..b05631a 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -217,12 +255,11 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
 void intel_panel_disable_backlight(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 level = intel_panel_get_backlight(dev);

-	if (dev_priv->backlight_enabled) {
-		dev_priv->backlight_level = intel_panel_get_backlight(dev);
-		dev_priv->backlight_enabled = false;
-	}
-
+	if (level == 0)
+		return;
+	dev_priv->backlight_level = level;
 	intel_panel_set_backlight(dev, 0);
 }

@@ -230,17 +267,9 @@ void intel_panel_enable_backlight(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;

+	if (intel_panel_get_backlight(dev))
+		return;
 	if (dev_priv->backlight_level == 0)
 		dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
-
 	intel_panel_set_backlight(dev, dev_priv->backlight_level);
-	dev_priv->backlight_enabled = true;
-}
-
-void intel_panel_setup_backlight(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	dev_priv->backlight_level = intel_panel_get_backlight(dev);
-	dev_priv->backlight_enabled = dev_priv->backlight_level != 0;
 }

Re: drm/i915: Fix DPMS and suspend interaction for intel_panel.c

[Takashi Iwai]

[Removed stable-kernel from Cc]

At Fri, 11 Mar 2011 02:35:45 +0100 (CET),
Indan Zupancic wrote:
> 
> drm/i915: Fix DPMS and suspend interaction for intel_panel.c
> 
> When suspending intel_panel_disable_backlight() is never called,
> but intel_panel_enable_backlight() is called at resume. With the
> effect that if the brightness was ever changed after screen
> blanking, the wrong brightness gets restored at resume time.
> 
> Nothing guarantees that those calls will be balanced, so having
> backlight_enabled makes no sense, as the real state can change
> without the panel code noticing. So keep things as stateless as
> possible.
> 
> Signed-off-by: Indan Zupancic <indan@nul.nu>

Indan, when you need a patch to be added to stable kernel, just put
Cc to stable kernel around your sign-off line.  Then Greg will pick it
up automatically once when the patch is merged to Linus tree.

Anyway, the patch looks good to me.  A nice clean-up.

Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

Re: drm/i915: Fix DPMS and suspend interaction for intel_panel.c

[Indan Zupancic]

On Fri, March 11, 2011 08:23, Takashi Iwai wrote:
> [Removed stable-kernel from Cc]
>
> At Fri, 11 Mar 2011 02:35:45 +0100 (CET),
> Indan Zupancic wrote:
>>
>> drm/i915: Fix DPMS and suspend interaction for intel_panel.c
>>
>> When suspending intel_panel_disable_backlight() is never called,
>> but intel_panel_enable_backlight() is called at resume. With the
>> effect that if the brightness was ever changed after screen
>> blanking, the wrong brightness gets restored at resume time.
>>
>> Nothing guarantees that those calls will be balanced, so having
>> backlight_enabled makes no sense, as the real state can change
>> without the panel code noticing. So keep things as stateless as
>> possible.
>>
>> Signed-off-by: Indan Zupancic <indan@nul.nu>
>
> Indan, when you need a patch to be added to stable kernel, just put
> Cc to stable kernel around your sign-off line.  Then Greg will pick it
> up automatically once when the patch is merged to Linus tree.

Ah, okay. I'll do that in the future.

Reading through Documentation/stable_kernel_rules.txt I'm not
sure if this patch applies. It does fix a bug that bothers me
personally, but otherwise it's not very important.

> Anyway, the patch looks good to me.  A nice clean-up.

That was a nice side effect. I like it when fixing code makes
it cleaner. Unfortunately it didn't work for the LBPC thing,
but here it does. My combination removal patch hid this bug
accidentally because PWM was kept at max, it was the main
reason I thought something was horribly wrong with the LBPC
fiddling. But it was just improper state tracking, fixed by
this patch, and my combination removal patch was bogus. Oh
well.

> Reviewed-by: Takashi Iwai <tiwai@suse.de>
>
>
> thanks,
>
> Takashi
>

Greetings,

Indan

Re: drm/i915: Fix DPMS and suspend interaction for intel_panel.c

[Chris Wilson]

On Fri, 11 Mar 2011 02:35:45 +0100 (CET), "Indan Zupancic" <indan@nul.nu> wrote:
> drm/i915: Fix DPMS and suspend interaction for intel_panel.c
> 
> When suspending intel_panel_disable_backlight() is never called,
> but intel_panel_enable_backlight() is called at resume. With the
> effect that if the brightness was ever changed after screen
> blanking, the wrong brightness gets restored at resume time.
> 
> Nothing guarantees that those calls will be balanced, so having
> backlight_enabled makes no sense, as the real state can change
> without the panel code noticing. So keep things as stateless as
> possible.

The problem is wider than described since the BIOS/ACPI may modify the
registers without notifying the driver, and we fail to honour any user
requests whilst the panel is off. Also, there is the greater of problem
that the PWM registers simply have no effect on many machines, and we
need to make an ACPI call to adjust the backlight.  However, this
behaviour has been there since the inception, we can wait until the next
merge cycle.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: drm/i915: Fix DPMS and suspend interaction for intel_panel.c

[Indan Zupancic]

On Fri, March 11, 2011 09:07, Chris Wilson wrote:
>> Nothing guarantees that those calls will be balanced, so having
>> backlight_enabled makes no sense, as the real state can change
>> without the panel code noticing. So keep things as stateless as
>> possible.
>
> The problem is wider than described since the BIOS/ACPI may modify
> the registers without notifying the driver, and we fail to honour
> any user requests whilst the panel is off. Also, there is the greater
> of problem that the PWM registers simply have no effect on many
> machines, and we need to make an ACPI call to adjust the backlight.

Actually, that makes more sense than what ASLE does: There you
have to make an ACPI call which generates the ASLE interrupt and
lets the driver change it, while the BIOS already knows how to
change the backlight, as it does it at boot.

Is the PWM totally ignored, or only when non-zero? Otherwise you
may need to clear bit 31 in BLC_PWM_CLT2 instead. That might be a
better way to enable/disable the backlight for gen >= 4 hardware
anyway, as you don't need to store the current backlight anymore.
But this is 2.6.39 material for sure.

> However, this behaviour has been there since the inception, we can
> wait until the next merge cycle.

I think it was introduced in 2.6.37, at least that when the backlight
problems started for me.

In my opinion this is a patch that fixes a bug by making the code
simpler, so I would merge it this cycle. I am uncertain whether
this bug is important enough to get fixed in stable too though.
Maybe once it has proven itself in 2.6.38 for a while. If you want
to fix it the next cycle then that's fine too, but only delays
things. I think at this point the backlight problems are known
well enough that the fix is obvious and low risk enough, but I've
sent too hasty patches before.

Greetings,

Indan

Re: drm/i915: Fix DPMS and suspend interaction for intel_panel.c

[Jesse Barnes]

On Fri, 11 Mar 2011 02:35:45 +0100 (CET)
"Indan Zupancic" <indan@nul.nu> wrote:

> drm/i915: Fix DPMS and suspend interaction for intel_panel.c
> 
> When suspending intel_panel_disable_backlight() is never called,
> but intel_panel_enable_backlight() is called at resume. With the
> effect that if the brightness was ever changed after screen
> blanking, the wrong brightness gets restored at resume time.
> 
> Nothing guarantees that those calls will be balanced, so having
> backlight_enabled makes no sense, as the real state can change
> without the panel code noticing. So keep things as stateless as
> possible.
> 
> Signed-off-by: Indan Zupancic <indan@nul.nu>

Chris is right that we don't always control the backlight brightness
directly, so we'll want a more complete solution at any rate.

I don't think this one is urgent enough to send upstream now, and it
would be good to make a couple of other fixes as well, while you're
fixing things up. :)  Comments below.

> -/* i915_suspend.c */
> -extern int i915_save_state(struct drm_device *dev);
> -extern int i915_restore_state(struct drm_device *dev);
> -

Looks like a spurious cleanup hunk, should be a separate hunk (possibly
along with other save/restore state cleanups, like removing all the
ILK+ code that probably won't work well :).

> -void intel_panel_setup_backlight(struct drm_device *dev)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	dev_priv->backlight_level = intel_panel_get_backlight(dev);
> -	dev_priv->backlight_enabled = dev_priv->backlight_level != 0;
>  }

This is getting pretty messy.  Your patch is a step in the right
direction, but I think we may as well go further:
  - suspend/resume of backlight state belongs in the backlight class
    driver
  - that driver should call into the registered i915 backlight handler
    if i915 is controlling it natively (PWM native, combo, legacy)
  - we need a backlight driver struct with function pointers for the
    various calls, so we can split out the PCH functions from the rest;
    might be good to separate native, combo, and legacy that way as
    well; even if results in some code duplication it might make each
    easier to read and less likely to break others when we make changes

Any thoughts about that?  I think Chris and Matthew have been talking
about it as well, so I'd be curious what our backlight final solution
ought to be.

Thanks,
Jesse

Re: drm/i915: Fix DPMS and suspend interaction for intel_panel.c

[Indan Zupancic]

On Fri, March 11, 2011 18:27, Jesse Barnes wrote:
> On Fri, 11 Mar 2011 02:35:45 +0100 (CET)
> "Indan Zupancic" <indan@nul.nu> wrote:
>
>> drm/i915: Fix DPMS and suspend interaction for intel_panel.c
>>
>> When suspending intel_panel_disable_backlight() is never called,
>> but intel_panel_enable_backlight() is called at resume. With the
>> effect that if the brightness was ever changed after screen
>> blanking, the wrong brightness gets restored at resume time.
>>
>> Nothing guarantees that those calls will be balanced, so having
>> backlight_enabled makes no sense, as the real state can change
>> without the panel code noticing. So keep things as stateless as
>> possible.
>>
>> Signed-off-by: Indan Zupancic <indan@nul.nu>
>
> Chris is right that we don't always control the backlight brightness
> directly, so we'll want a more complete solution at any rate.

Well, not having control basically means not caching any register
values, or as little as possible. For gen >=4 there's the top bit
of BLC_PWM_CTL2, so the brightness handling can be cleanly separated
from the panel disabling/enabling. For older gens we need to save
the brightness and then set it to zero, as happens now.

For brightness control we only need to set the new brightness on
systems with ASLE.

> I don't think this one is urgent enough to send upstream now, and it
> would be good to make a couple of other fixes as well, while you're
> fixing things up. :)  Comments below.

It's not urgent, it's just a bugfix. I did my best to resist the
urge to do other cleanups as well, so close to the release cycle.

>From my point of view it's sad that 2.6.37 was released with buggy
backlight handling, and it would be sad if it wasn't fixed in 2.6.38.
But I think this is only because I have a gen 2 and the check for
combination mode for gen 2 was added in 2.6.37, everyone else already
had the buggy backlight behaviour for ages. Without the check gen 2
works correctly because the PWM value never changes, only LBPC, and
the driver didn't touch LBPC.

>> -/* i915_suspend.c */
>> -extern int i915_save_state(struct drm_device *dev);
>> -extern int i915_restore_state(struct drm_device *dev);
>> -
>
> Looks like a spurious cleanup hunk, should be a separate hunk (possibly
> along with other save/restore state cleanups, like removing all the
> ILK+ code that probably won't work well :).

Wild guess: ILK+ means Ironlake+?

But indeed, though as the duplicate declaration is in the diff context
it seemed safe enough at the time. ;-)

>> -void intel_panel_setup_backlight(struct drm_device *dev)
>> -{
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>> -
>> -	dev_priv->backlight_level = intel_panel_get_backlight(dev);
>> -	dev_priv->backlight_enabled = dev_priv->backlight_level != 0;
>>  }
>
> This is getting pretty messy.

That functions was added in the 2.6.38 cycle I think, in an attempt
to fix the backlight problems.

>  Your patch is a step in the right
> direction, but I think we may as well go further:
>   - suspend/resume of backlight state belongs in the backlight class
>     driver

There is no backlight suspend/resume handling, the panel just gets
enabled at resume. The registers save/restore is done i915_suspend.c,
where it belongs. The current bugginess is not only for suspend, but
for any two DPMS on calls without a disable in between. Try

xset dpms force off
xset dpms force on
..change brightness..
xset dpms force on

I think you'll get the old brightness.

>   - that driver should call into the registered i915 backlight handler
>     if i915 is controlling it natively (PWM native, combo, legacy)

Brightness setting is only needed for ASLE, otherwise it's never set
by the driver. So in the end most complexity is only there because of
one combination: ASLE + legacy combination mode.

>   - we need a backlight driver struct with function pointers for the
>     various calls, so we can split out the PCH functions from the rest;
>     might be good to separate native, combo, and legacy that way as
>     well; even if results in some code duplication it might make each
>     easier to read and less likely to break others when we make changes

Problem is, are we sure that systems don't change from legacy combination
mode to BLC_PWM_CTL only? Otherwise you have to check every time.

I must admit I'm not sure what the backlight hierarchy is, but I don't
think the intel_panel.c code has much to do with it: It has nothing to
do with backlight key events and doesn't handle them. All it does is
enabling or disabling the panel for DPMS. The only thing that needs to
set the backlight is intel_opregion.c when ASLE is used, and that bit
should indeed go somewhere else.

So if I'd clean up the code, I think this is what I would try first:

Rip out all brightness control out of intel_panel.c and replace it
with a generic, minimal register saving for disabling the panel, with
all system specific info (which registers to use, masks, etc.) stored
in struct intel_device_info.

All the extra complexity comes from ASLE. As you wrote the Intel ASLE
documentation, I hope you can give some more information about it:

1) First, are there any gen 2 or gen 3 systems with ASLE? If not, there
is no need to handle legacy combination mode for those gens. I think
ASLE came with gen 4, but can you confirm that? Either the gen 2 check
for legacy mode is not needed, or a gen 3 check needs to be added.

2) What happens if the driver doesn't support ASLE? If the BIOS changes
the brightness directly, then we can rip out the whole ASLE thing, as
it's useless. This is probably not possible, so we have to keep the
ASLE madness.

If ASLE needs to set the brightness then we need a way to do that.
But I'd change the interface to a percentage or 0-255 scale, that
fits better with the ASLE thing and the max brightness is hidden.

(I dislike ASLE because it's clear the BIOS knows how to do what it
wants, but bothers the driver with it, which has to do it the same
way as the BIOS, or things stop working.)

And instead of all those almost duplicate functions I'd have one
generic one that works for all, with system specific stuff put in
struct intel_device_info.

i915_read_blc_pwm_ctl() can be greatly simplified by calling
i915_save_state() early and often enough. I don't think you want crash
recovery scattered around the code like that, but done centrally so it's
easier to recover from a hardware crash. I guess saving state just after
driver initialisation and after every modeset is close to enough, but I
have no idea about 3D. My hope is that after a GPU hang, the system can
be restored by resetting the GPU and restore the state. I haven't looked
into this yet, are there any reasons why this won't work?

The intel_panel_get_max_backlight() code can be simplified by always
ignoring the first bit, so that the Pineview and gen < 4 handling can
be generic.

I think abstracting more specific system behaviour into struct
intel_device_info instead of the gen checks everywhere would often
simplify and reduce the code.

> Any thoughts about that?  I think Chris and Matthew have been
> talking about it as well, so I'd be curious what our backlight
> final solution ought to be.

Didn't Matthew factor out the intel opregion code? That should get the
brightness setting hooked up into the right spot too.

After the above cleanups it's only a matter where to put the set_backlight()
code.

But it seems all improvements can be done incrementally, and as they change
a lot of code, it's good to start from a working base. So that would be a
reason to apply my patch this cycle instead of later. Either that, or apply
it in rc1 and do everything else in rc2 or later, but that seems worse. But
it's up to you guys.

Greetings,

Indan