No subject

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