Re: [PATCH] acpi: fix brightness level is initialized to zero when BIOS does not restore the brightness value to _BQC.

[Alex Hung <alex.hung@canonical.com>]

On 10/08/2012 12:34 PM, Zhang, Rui wrote:
>> -----Original Message-----
>> From: Alex Hung [mailto:alex.hung@canonical.com]
>> Sent: Monday, October 01, 2012 11:27 PM
>> To: Zhang, Rui
>> Cc: joeyli; linux-acpi@vger.kernel.org
>> Subject: Re: [PATCH] acpi: fix brightness level is initialized to zero
>> when BIOS does not restore the brightness value to _BQC.
>> Importance: High
>>
>> Hi Rui,
>>
>> Yes the module parameter is used temporarily. However, we want to
>> eliminate the need of using any parameter since non-technical users
>> won't know them without any help.
>>
>> The question of whether lowest lowest brightness equals black screen is
>> debatable and I heard good / bad stories from both sides and they all
>> makes sense. This, however, does not necessarily play a role here. The
>> patch is to fix a problem - brightness is set to lowest value (not
>> user-friendly) when it was not intended.
>>
>> I also agree that there is nothing wrong with platform setting the
>> brightness to lowest level during boot and that's a side-effect of this
>> patch indeed.. In fact, it took me a few weeks to decide it may be an
>> improvement for the below reasons.
>>
>> 1. In most of cases, lowest level is less desirable or less commonly
>> used. If a BIOS returns uninitialized valube such as a zero in this
>> case, Linux causes confusion and problems to users with such platforms.
>> Comparing to the side-effect, this (may) benefits more users.
>>
>> 2. I also checked ACPI sepc and it says:
>>
>> B.5.4 _BQC (Brightness Query Current level) This method returns the
>> current brightness level of a built-in display output device.
>>
>> The definition makes it hard to argue that it is a BIOS bug not to
>> restore previous brightness to _BQC. I think it is ambiguous and this
>> small fix eliminates very dim screen (whether complete black or not) at
>> boot time.
>>
>> PS. This may not be relevant, but Windows restores the previous
>> brightness without BIOS's _BQC,
>
> Where does windows get the previous brightness?
It seems Windows remember the brightness before shutting down and uses 
that value when it boots up. It was observed in Windows 7.


>
>> and therefore it does not suffer from
>> very dim / black screen.
>>
>> Cheers,
>> Alex Hung
>>
>>
>> On 10/01/2012 09:36 PM, Zhang, Rui wrote:
>>> I think this is probably what you're looking for,
>>>
>>> /*
>>>    * Some BIOSes claim they use minimum backlight at boot,
>>>    * and this may bring dimming screen after boot
>>>    */
>>> static bool use_bios_initial_backlight = 1;
>>> module_param(use_bios_initial_backlight, bool, 0644);
>>>
>>>
>>> Lowest brightness level does not equal black screen.
>>> It is not a bug if a platform wants to set its brightness to lowest
>> brightness level during boot.
>>> If it IS in your case, you can use this module parameter to ignore
>> the initial _BQC value and use max_level instead.
>>>
>>> Thanks,
>>> rui
>>>
>>>> -----Original Message-----
>>>> From: joeyli [mailto:jlee@suse.com]
>>>> Sent: Monday, October 01, 2012 5:19 PM
>>>> To: Alex Hung
>>>> Cc: Zhang, Rui; linux-acpi@vger.kernel.org
>>>> Subject: Re: [PATCH] acpi: fix brightness level is initialized to
>>>> zero when BIOS does not restore the brightness value to _BQC.
>>>> Importance: High
>>>>
>>>> 於 一，2012-10-01 於 17:11 +0800，Alex Hung 提到：
>>>>> On 10/01/2012 04:34 PM, joeyli wrote:
>>>>>> 於 一，2012-10-01 於 15:17 +0800，joeyli 提到：
>>>>>>> 於 一，2012-10-01 於 15:03 +0800，Alex Hung 提到：
>>>>>>>> On 10/01/2012 02:47 PM, joeyli wrote:
>>>>>>>>> Hi Alex,
>>>>>>>>>
>>>>>>>>> 於 一，2012-10-01 於 13:39 +0800，Alex Hung 提到：
>>>>>>>>>> Signed-off-by: Alex Hung <alex.hung@canonical.com>
>>>>>>>>>> ---
>>>>>>>>>>      drivers/acpi/video.c |    4 ++++
>>>>>>>>>>      1 files changed, 4 insertions(+), 0 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index
>>>>>>>>>> 42b226e..eaa9573 100644
>>>>>>>>>> --- a/drivers/acpi/video.c
>>>>>>>>>> +++ b/drivers/acpi/video.c
>>>>>>>>>> @@ -724,6 +724,10 @@ acpi_video_init_brightness(struct
>>>> acpi_video_device *device)
>>>>>>>>>>      				if (level_old == br->levels[i])
>>>>>>>>>>      					level = level_old;
>>>>>>>>>>      		}
>>>>>>>>>> +
>>>>>>>>>> +		if (level == 0)
>>>>>>>>>> +			level = br->levels[(br->count) / 2 + 1];
>>>>>>>>>
>>>>>>>>> Looks here used the 50% brightness level.
>>>>>>>>>
>>>>>>>>> Per comment in video.c, we want set the backlight to max_level
>>>>>>>>> when level_old is invalid:
>>>>>>>>>
>>>>>>>>>             if (!br->flags._BQC_use_index) {
>>>>>>>>>                     /*
>>>>>>>>>                      * Set the backlight to the initial state.
>>>>>>>>>                      * On some buggy laptops, _BQC returns an
>>>> uninitialized value
>>>>>>>>>                      * when invoked for the first time, i.e.
>>>> level_old is invalid.
>>>>>>>>>                      * set the backlight to max_level in this
>> case
>>>>>>>>>                      */
>>>>>>>>>
>>>>>>>>> I think here used max_level to fulfill it, e.g.
>>>>>>>>>
>>>>>>>>> +		if (level == 0)
>>>>>>>>> +			level = max_level;
>>>>>>>>>
>>>>>>>>> How do you think?
>>>>>>>> Hi Joey,
>>>>>>>>
>>>>>>>> I was debating with myself which level to be set, ex. 50%, ~75%
>>>> or
>>>>>>>> 100%, and I think 50% *might* be closer to normal use-case (just
>>>> a
>>>>>>>> personal guess).
>>>>>>>>
>>>>>>>> However, "max_level" seems to fit best if we treat the initial
>>>>>>>> zero brightness in invalid. I can modify it according it that's
>>>> preferred.
>>>>>>>>
>>>>>>>> Thanks for the feedback.
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> Alex Hung
>>>>>>>>
>>>>>>>
>>>>>>> hm.... I have a question for what's the BIOS's problem that
>> causes
>>>>>>> 'level == 0'?
>>>>>>> That implied the issue machine's max_level is 0?
>>>>>>>
>>>>>>> 		/*
>>>>>>> 		 * Set the level to maximum and check if _BQC uses
>> indexed
>>>> value
>>>>>>> 		 */
>>>>>>> 		result = acpi_video_device_lcd_set_level(device,
>> max_level);
>>>> 		/* write max_level purposely, then read level back, compare
>> them */
>>>>>>> 		...
>>>>>>> 		result =
>> acpi_video_device_lcd_get_level_current(device,
>>>> &level, 0);
>>>>>>> 		...
>>>>>>> 		br->flags._BQC_use_index = (level == max_level ? 0 :
>> 1);
>>>>>>> 		if (!br->flags._BQC_use_index) {
>> 	/*
>>>> _BQC_use_index is 0 will run into if, means level == max_level */
>>>>>>>
>>>>>>> So, looks the 'level == max_level == 0' when level_old is invalid.
>>>>>>>
>>>>>>> Just wonder what's defect of BIOS (in _BCL?) causes problem.
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Sorry for my misunderstood!
>>>>>>
>>>>>> I think that's possible the level_old is 0 and there have a 0
>> value
>>>>>> in the return package from _BCL.
>>>>>>
>>>>>
>>>>> Yes, there is nothing wrong with _BCL and _BQC except that _BQC
>>>>> returns a zero initially.
>>>>>
>>>>>> Could you please share the _BCL in DSDT from issue machine? Does
>>>>>> there have 0 value in _BCL?
>>>>>
>>>>> _BCL returns below data and there is a zero in the list.
>>>>>
>>>>> [  744.572289] Brightness[0] = 100
>>>>> [  744.572293] Brightness[1] = 50
>>>>> [  744.572295] Brightness[2] = 0
>>>>> [  744.572297] Brightness[3] = 10
>>>>> [  744.572299] Brightness[4] = 20
>>>>> [  744.572301] Brightness[5] = 30
>>>>> [  744.572303] Brightness[6] = 40
>>>>> [  744.572305] Brightness[7] = 50
>>>>> [  744.572306] Brightness[8] = 60
>>>>> [  744.572308] Brightness[9] = 70
>>>>> [  744.572310] Brightness[10] = 80
>>>>> [  744.572312] Brightness[11] = 90
>>>>> [  744.572314] Brightness[12] = 100
>>>>>
>>>>> The below is the complete _BCL for references
>>>>>
>>>>>                    Method (_BCL, 0, Serialized)
>>>>>                    {
>>>>>                        Name (_T_0, Zero)
>>>>>                        If (_OSI ("NOT_WINP_KEY"))
>>>>>                        {
>>>>>                            While (One)
>>>>>                            {
>>>>>                                Store (LCDD, _T_0)
>>>>>                                If (LEqual (_T_0, 0x303CAF06))
>>>>>                                {
>>>>>                                    Return (AUOL)
>>>>>                                }
>>>>>                                Else
>>>>>                                {
>>>>>                                    If (LEqual (_T_0, 0x1475AE0D))
>>>>>                                    {
>>>>>                                        Return (CMIL)
>>>>>                                    }
>>>>>                                    Else
>>>>>                                    {
>>>>>                                        If (LEqual (_T_0, 0x033FE430))
>>>>>                                        {
>>>>>                                            Return (LGDL)
>>>>>                                        }
>>>>>                                        Else
>>>>>                                        {
>>>>>                                            If (LEqual (_T_0,
>>>> 0x3942A34C))
>>>>>                                            {
>>>>>                                                Return (SAML)
>>>>>                                            }
>>>>>                                            Else
>>>>>                                            {
>>>>>                                                Return (DEFL)
>>>>>                                            }
>>>>>                                        }
>>>>>                                    }
>>>>>                                }
>>>>>
>>>>>                                Break
>>>>>                            }
>>>>>                        }
>>>>>                        Else
>>>>>                        {
>>>>>                            Return (Package (0x0D)
>>>>>                            {
>>>>>                                0x64,
>>>>>                                0x32,
>>>>>                                Zero,
>>>>
>>>> Yes, have Zero value in _BCL return package.
>>>>
>>>>>                                0x0A,
>>>>>                                0x14,
>>>>>                                0x1E,
>>>>>                                0x28,
>>>>>                                0x32,
>>>>>                                0x3C,
>>>>>                                0x46,
>>>>>                                0x50,
>>>>>                                0x5A,
>>>>>                                0x64
>>>>>                            })
>>>>>                        }
>>>>>                    }
>>>>>
>>>>>
>>>>
>>>> According to the above information, it make sense now!
>>>>
>>>>
>>>> Thank a lot!
>>>> Joey Lee
>>>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html