[PATCH] drm/i915/opregion: ignore firmware requests for backlight change

[PATCH] drm/i915/opregion: ignore firmware requests for backlight change

[Aaron Lu]

Some Thinkpad laptops' firmware will initiate a backlight level change
request through operation region on the events of AC plug/unplug, but
since we are not using firmware's interface to do the backlight setting
on these affected laptops, we do not want the firmware to use some
arbitrary value from its ASL variable to set the backlight level on
AC plug/unplug either.

Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=76491
Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=77091
Reported-and-tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
Reported-and-tested-by: Anton Gubarkov <anton.gubarkov@gmail.com>
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/acpi/video.c                  | 3 ++-
 drivers/gpu/drm/i915/intel_opregion.c | 7 +++++++
 include/acpi/video.h                  | 2 ++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index fb9ffe9adc64..cf99d6d2d491 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -241,13 +241,14 @@ static bool acpi_video_use_native_backlight(void)
 		return use_native_backlight_dmi;
 }
 
-static bool acpi_video_verify_backlight_support(void)
+bool acpi_video_verify_backlight_support(void)
 {
 	if (acpi_osi_is_win8() && acpi_video_use_native_backlight() &&
 	    backlight_device_registered(BACKLIGHT_RAW))
 		return false;
 	return acpi_video_backlight_support();
 }
+EXPORT_SYMBOL(acpi_video_verify_backlight_support);
 
 /* backlight device sysfs support */
 static int acpi_video_get_brightness(struct backlight_device *bd)
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 2e2c71fcc9ed..02943d93e88e 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -403,6 +403,13 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
 
 	DRM_DEBUG_DRIVER("bclp = 0x%08x\n", bclp);
 
+	/*
+	 * If the acpi_video interface is not supposed to be used, don't
+	 * bother processing backlight level change requests from firmware.
+	 */
+	if (!acpi_video_verify_backlight_support())
+		return 0;
+
 	if (!(bclp & ASLE_BCLP_VALID))
 		return ASLC_BACKLIGHT_FAILED;
 
diff --git a/include/acpi/video.h b/include/acpi/video.h
index ea4c7bbded4d..92f8c4bffefb 100644
--- a/include/acpi/video.h
+++ b/include/acpi/video.h
@@ -22,6 +22,7 @@ extern void acpi_video_unregister(void);
 extern void acpi_video_unregister_backlight(void);
 extern int acpi_video_get_edid(struct acpi_device *device, int type,
 			       int device_id, void **edid);
+extern bool acpi_video_verify_backlight_support(void);
 #else
 static inline int acpi_video_register(void) { return 0; }
 static inline void acpi_video_unregister(void) { return; }
@@ -31,6 +32,7 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type,
 {
 	return -ENODEV;
 }
+static bool acpi_video_verify_backlight_support() { return false; }
 #endif
 
 #endif
-- 
1.9.3

Re: [PATCH] drm/i915/opregion: ignore firmware requests for backlight change

[Jani Nikula]

On Tue, 24 Jun 2014, Aaron Lu <aaron.lu@intel.com> wrote:
> Some Thinkpad laptops' firmware will initiate a backlight level change
> request through operation region on the events of AC plug/unplug, but
> since we are not using firmware's interface to do the backlight setting
> on these affected laptops, we do not want the firmware to use some
> arbitrary value from its ASL variable to set the backlight level on
> AC plug/unplug either.

I'm curious whether this happens with EFI boot, or only with legacy.

One comment inline, otherwise

Acked-by: Jani Nikula <jani.nikula@intel.com>

for merging through the ACPI tree, as the change is more likely to
conflict there.

> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=76491
> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=77091
> Reported-and-tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
> Reported-and-tested-by: Anton Gubarkov <anton.gubarkov@gmail.com>
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  drivers/acpi/video.c                  | 3 ++-
>  drivers/gpu/drm/i915/intel_opregion.c | 7 +++++++
>  include/acpi/video.h                  | 2 ++
>  3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index fb9ffe9adc64..cf99d6d2d491 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -241,13 +241,14 @@ static bool acpi_video_use_native_backlight(void)
>  		return use_native_backlight_dmi;
>  }
>  
> -static bool acpi_video_verify_backlight_support(void)
> +bool acpi_video_verify_backlight_support(void)
>  {
>  	if (acpi_osi_is_win8() && acpi_video_use_native_backlight() &&
>  	    backlight_device_registered(BACKLIGHT_RAW))
>  		return false;
>  	return acpi_video_backlight_support();
>  }
> +EXPORT_SYMBOL(acpi_video_verify_backlight_support);
>  
>  /* backlight device sysfs support */
>  static int acpi_video_get_brightness(struct backlight_device *bd)
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 2e2c71fcc9ed..02943d93e88e 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -403,6 +403,13 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>  
>  	DRM_DEBUG_DRIVER("bclp = 0x%08x\n", bclp);
>  
> +	/*
> +	 * If the acpi_video interface is not supposed to be used, don't
> +	 * bother processing backlight level change requests from firmware.
> +	 */
> +	if (!acpi_video_verify_backlight_support())
> +		return 0;

I'd appreciate a DRM_DEBUG_KMS here about what happened. We're bound to
wonder about that staring at some dmesg later on!

> +
>  	if (!(bclp & ASLE_BCLP_VALID))
>  		return ASLC_BACKLIGHT_FAILED;
>  
> diff --git a/include/acpi/video.h b/include/acpi/video.h
> index ea4c7bbded4d..92f8c4bffefb 100644
> --- a/include/acpi/video.h
> +++ b/include/acpi/video.h
> @@ -22,6 +22,7 @@ extern void acpi_video_unregister(void);
>  extern void acpi_video_unregister_backlight(void);
>  extern int acpi_video_get_edid(struct acpi_device *device, int type,
>  			       int device_id, void **edid);
> +extern bool acpi_video_verify_backlight_support(void);
>  #else
>  static inline int acpi_video_register(void) { return 0; }
>  static inline void acpi_video_unregister(void) { return; }
> @@ -31,6 +32,7 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type,
>  {
>  	return -ENODEV;
>  }
> +static bool acpi_video_verify_backlight_support() { return false; }
>  #endif
>  
>  #endif
> -- 
> 1.9.3
>

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm/i915/opregion: ignore firmware requests for backlight change

[Aaron Lu]

On 06/25/2014 07:08 PM, Jani Nikula wrote:
> On Tue, 24 Jun 2014, Aaron Lu <aaron.lu@intel.com> wrote:
>> Some Thinkpad laptops' firmware will initiate a backlight level change
>> request through operation region on the events of AC plug/unplug, but
>> since we are not using firmware's interface to do the backlight setting
>> on these affected laptops, we do not want the firmware to use some
>> arbitrary value from its ASL variable to set the backlight level on
>> AC plug/unplug either.
> 
> I'm curious whether this happens with EFI boot, or only with legacy.

Igor, Anton,

Are you using legacy boot or UEFI boot?
Possible to test the other case?

> 
> One comment inline, otherwise

Will add that in next revision.

> 
> Acked-by: Jani Nikula <jani.nikula@intel.com>

Thanks for the review!

-Aaron

> 
> for merging through the ACPI tree, as the change is more likely to
> conflict there.
> 
>> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=76491
>> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=77091
>> Reported-and-tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
>> Reported-and-tested-by: Anton Gubarkov <anton.gubarkov@gmail.com>
>> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
>> ---
>>  drivers/acpi/video.c                  | 3 ++-
>>  drivers/gpu/drm/i915/intel_opregion.c | 7 +++++++
>>  include/acpi/video.h                  | 2 ++
>>  3 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
>> index fb9ffe9adc64..cf99d6d2d491 100644
>> --- a/drivers/acpi/video.c
>> +++ b/drivers/acpi/video.c
>> @@ -241,13 +241,14 @@ static bool acpi_video_use_native_backlight(void)
>>  		return use_native_backlight_dmi;
>>  }
>>  
>> -static bool acpi_video_verify_backlight_support(void)
>> +bool acpi_video_verify_backlight_support(void)
>>  {
>>  	if (acpi_osi_is_win8() && acpi_video_use_native_backlight() &&
>>  	    backlight_device_registered(BACKLIGHT_RAW))
>>  		return false;
>>  	return acpi_video_backlight_support();
>>  }
>> +EXPORT_SYMBOL(acpi_video_verify_backlight_support);
>>  
>>  /* backlight device sysfs support */
>>  static int acpi_video_get_brightness(struct backlight_device *bd)
>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> index 2e2c71fcc9ed..02943d93e88e 100644
>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> @@ -403,6 +403,13 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>>  
>>  	DRM_DEBUG_DRIVER("bclp = 0x%08x\n", bclp);
>>  
>> +	/*
>> +	 * If the acpi_video interface is not supposed to be used, don't
>> +	 * bother processing backlight level change requests from firmware.
>> +	 */
>> +	if (!acpi_video_verify_backlight_support())
>> +		return 0;
> 
> I'd appreciate a DRM_DEBUG_KMS here about what happened. We're bound to
> wonder about that staring at some dmesg later on!
> 
>> +
>>  	if (!(bclp & ASLE_BCLP_VALID))
>>  		return ASLC_BACKLIGHT_FAILED;
>>  
>> diff --git a/include/acpi/video.h b/include/acpi/video.h
>> index ea4c7bbded4d..92f8c4bffefb 100644
>> --- a/include/acpi/video.h
>> +++ b/include/acpi/video.h
>> @@ -22,6 +22,7 @@ extern void acpi_video_unregister(void);
>>  extern void acpi_video_unregister_backlight(void);
>>  extern int acpi_video_get_edid(struct acpi_device *device, int type,
>>  			       int device_id, void **edid);
>> +extern bool acpi_video_verify_backlight_support(void);
>>  #else
>>  static inline int acpi_video_register(void) { return 0; }
>>  static inline void acpi_video_unregister(void) { return; }
>> @@ -31,6 +32,7 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type,
>>  {
>>  	return -ENODEV;
>>  }
>> +static bool acpi_video_verify_backlight_support() { return false; }
>>  #endif
>>  
>>  #endif
>> -- 
>> 1.9.3
>>
> 

Re: [PATCH] drm/i915/opregion: ignore firmware requests for backlight change

[Anton Gubar'kov]

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

Hi, I would like to inform that I use UEFI boot. I don't have a spare
machine/disk to test legacy unfortunately.

regards
Anton.


2014-06-27 7:20 GMT+04:00 Aaron Lu <aaron.lu@intel.com>:

> On 06/25/2014 07:08 PM, Jani Nikula wrote:
> > On Tue, 24 Jun 2014, Aaron Lu <aaron.lu@intel.com> wrote:
> >> Some Thinkpad laptops' firmware will initiate a backlight level change
> >> request through operation region on the events of AC plug/unplug, but
> >> since we are not using firmware's interface to do the backlight setting
> >> on these affected laptops, we do not want the firmware to use some
> >> arbitrary value from its ASL variable to set the backlight level on
> >> AC plug/unplug either.
> >
> > I'm curious whether this happens with EFI boot, or only with legacy.
>
> Igor, Anton,
>
> Are you using legacy boot or UEFI boot?
> Possible to test the other case?
>
> >
> > One comment inline, otherwise
>
> Will add that in next revision.
>
> >
> > Acked-by: Jani Nikula <jani.nikula@intel.com>
>
> Thanks for the review!
>
> -Aaron
>
> >
> > for merging through the ACPI tree, as the change is more likely to
> > conflict there.
> >
> >> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=76491
> >> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=77091
> >> Reported-and-tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
> >> Reported-and-tested-by: Anton Gubarkov <anton.gubarkov@gmail.com>
> >> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> >> ---
> >>  drivers/acpi/video.c                  | 3 ++-
> >>  drivers/gpu/drm/i915/intel_opregion.c | 7 +++++++
> >>  include/acpi/video.h                  | 2 ++
> >>  3 files changed, 11 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> >> index fb9ffe9adc64..cf99d6d2d491 100644
> >> --- a/drivers/acpi/video.c
> >> +++ b/drivers/acpi/video.c
> >> @@ -241,13 +241,14 @@ static bool acpi_video_use_native_backlight(void)
> >>              return use_native_backlight_dmi;
> >>  }
> >>
> >> -static bool acpi_video_verify_backlight_support(void)
> >> +bool acpi_video_verify_backlight_support(void)
> >>  {
> >>      if (acpi_osi_is_win8() && acpi_video_use_native_backlight() &&
> >>          backlight_device_registered(BACKLIGHT_RAW))
> >>              return false;
> >>      return acpi_video_backlight_support();
> >>  }
> >> +EXPORT_SYMBOL(acpi_video_verify_backlight_support);
> >>
> >>  /* backlight device sysfs support */
> >>  static int acpi_video_get_brightness(struct backlight_device *bd)
> >> diff --git a/drivers/gpu/drm/i915/intel_opregion.c
> b/drivers/gpu/drm/i915/intel_opregion.c
> >> index 2e2c71fcc9ed..02943d93e88e 100644
> >> --- a/drivers/gpu/drm/i915/intel_opregion.c
> >> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> >> @@ -403,6 +403,13 @@ static u32 asle_set_backlight(struct drm_device
> *dev, u32 bclp)
> >>
> >>      DRM_DEBUG_DRIVER("bclp = 0x%08x\n", bclp);
> >>
> >> +    /*
> >> +     * If the acpi_video interface is not supposed to be used, don't
> >> +     * bother processing backlight level change requests from firmware.
> >> +     */
> >> +    if (!acpi_video_verify_backlight_support())
> >> +            return 0;
> >
> > I'd appreciate a DRM_DEBUG_KMS here about what happened. We're bound to
> > wonder about that staring at some dmesg later on!
> >
> >> +
> >>      if (!(bclp & ASLE_BCLP_VALID))
> >>              return ASLC_BACKLIGHT_FAILED;
> >>
> >> diff --git a/include/acpi/video.h b/include/acpi/video.h
> >> index ea4c7bbded4d..92f8c4bffefb 100644
> >> --- a/include/acpi/video.h
> >> +++ b/include/acpi/video.h
> >> @@ -22,6 +22,7 @@ extern void acpi_video_unregister(void);
> >>  extern void acpi_video_unregister_backlight(void);
> >>  extern int acpi_video_get_edid(struct acpi_device *device, int type,
> >>                             int device_id, void **edid);
> >> +extern bool acpi_video_verify_backlight_support(void);
> >>  #else
> >>  static inline int acpi_video_register(void) { return 0; }
> >>  static inline void acpi_video_unregister(void) { return; }
> >> @@ -31,6 +32,7 @@ static inline int acpi_video_get_edid(struct
> acpi_device *device, int type,
> >>  {
> >>      return -ENODEV;
> >>  }
> >> +static bool acpi_video_verify_backlight_support() { return false; }
> >>  #endif
> >>
> >>  #endif
> >> --
> >> 1.9.3
> >>
> >
>
>

[-- Attachment #1.2: Type: text/html, Size: 6207 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] drm/i915/opregion: ignore firmware requests for backlight change

[Igor Gnatenko]

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

On Fri, 2014-06-27 at 11:20 +0800, Aaron Lu wrote: 
> On 06/25/2014 07:08 PM, Jani Nikula wrote:
> > On Tue, 24 Jun 2014, Aaron Lu <aaron.lu@intel.com> wrote:
> >> Some Thinkpad laptops' firmware will initiate a backlight level change
> >> request through operation region on the events of AC plug/unplug, but
> >> since we are not using firmware's interface to do the backlight setting
> >> on these affected laptops, we do not want the firmware to use some
> >> arbitrary value from its ASL variable to set the backlight level on
> >> AC plug/unplug either.
> > 
> > I'm curious whether this happens with EFI boot, or only with legacy.
> 
> Igor, Anton,
Hi,
> Are you using legacy boot or UEFI boot?
I'm using UEFI. 
> Possible to test the other case?
yes, I'll test it with legacy in 10 mins. 
> 
> > 
> > One comment inline, otherwise
> 
> Will add that in next revision.
> 
> > 
> > Acked-by: Jani Nikula <jani.nikula@intel.com>
> 
> Thanks for the review!
> 
> -Aaron
> 
> > 
> > for merging through the ACPI tree, as the change is more likely to
> > conflict there.
> > 
> >> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=76491
> >> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=77091
> >> Reported-and-tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
> >> Reported-and-tested-by: Anton Gubarkov <anton.gubarkov@gmail.com>
> >> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> >> ---
> >>  drivers/acpi/video.c                  | 3 ++-
> >>  drivers/gpu/drm/i915/intel_opregion.c | 7 +++++++
> >>  include/acpi/video.h                  | 2 ++
> >>  3 files changed, 11 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> >> index fb9ffe9adc64..cf99d6d2d491 100644
> >> --- a/drivers/acpi/video.c
> >> +++ b/drivers/acpi/video.c
> >> @@ -241,13 +241,14 @@ static bool acpi_video_use_native_backlight(void)
> >>  		return use_native_backlight_dmi;
> >>  }
> >>  
> >> -static bool acpi_video_verify_backlight_support(void)
> >> +bool acpi_video_verify_backlight_support(void)
> >>  {
> >>  	if (acpi_osi_is_win8() && acpi_video_use_native_backlight() &&
> >>  	    backlight_device_registered(BACKLIGHT_RAW))
> >>  		return false;
> >>  	return acpi_video_backlight_support();
> >>  }
> >> +EXPORT_SYMBOL(acpi_video_verify_backlight_support);
> >>  
> >>  /* backlight device sysfs support */
> >>  static int acpi_video_get_brightness(struct backlight_device *bd)
> >> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> >> index 2e2c71fcc9ed..02943d93e88e 100644
> >> --- a/drivers/gpu/drm/i915/intel_opregion.c
> >> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> >> @@ -403,6 +403,13 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
> >>  
> >>  	DRM_DEBUG_DRIVER("bclp = 0x%08x\n", bclp);
> >>  
> >> +	/*
> >> +	 * If the acpi_video interface is not supposed to be used, don't
> >> +	 * bother processing backlight level change requests from firmware.
> >> +	 */
> >> +	if (!acpi_video_verify_backlight_support())
> >> +		return 0;
> > 
> > I'd appreciate a DRM_DEBUG_KMS here about what happened. We're bound to
> > wonder about that staring at some dmesg later on!
> > 
> >> +
> >>  	if (!(bclp & ASLE_BCLP_VALID))
> >>  		return ASLC_BACKLIGHT_FAILED;
> >>  
> >> diff --git a/include/acpi/video.h b/include/acpi/video.h
> >> index ea4c7bbded4d..92f8c4bffefb 100644
> >> --- a/include/acpi/video.h
> >> +++ b/include/acpi/video.h
> >> @@ -22,6 +22,7 @@ extern void acpi_video_unregister(void);
> >>  extern void acpi_video_unregister_backlight(void);
> >>  extern int acpi_video_get_edid(struct acpi_device *device, int type,
> >>  			       int device_id, void **edid);
> >> +extern bool acpi_video_verify_backlight_support(void);
> >>  #else
> >>  static inline int acpi_video_register(void) { return 0; }
> >>  static inline void acpi_video_unregister(void) { return; }
> >> @@ -31,6 +32,7 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type,
> >>  {
> >>  	return -ENODEV;
> >>  }
> >> +static bool acpi_video_verify_backlight_support() { return false; }
> >>  #endif
> >>  
> >>  #endif
> >> -- 
> >> 1.9.3
> >>
> > 
> 


-- 
-Igor Gnatenko

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 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] drm/i915/opregion: ignore firmware requests for backlight change

[Igor Gnatenko]

I've tested legacy boot. I have this bug.
On Fri, Jun 27, 2014 at 7:20 AM, Aaron Lu <aaron.lu@intel.com> wrote:
> On 06/25/2014 07:08 PM, Jani Nikula wrote:
>> On Tue, 24 Jun 2014, Aaron Lu <aaron.lu@intel.com> wrote:
>>> Some Thinkpad laptops' firmware will initiate a backlight level change
>>> request through operation region on the events of AC plug/unplug, but
>>> since we are not using firmware's interface to do the backlight setting
>>> on these affected laptops, we do not want the firmware to use some
>>> arbitrary value from its ASL variable to set the backlight level on
>>> AC plug/unplug either.
>>
>> I'm curious whether this happens with EFI boot, or only with legacy.
>
> Igor, Anton,
>
> Are you using legacy boot or UEFI boot?
> Possible to test the other case?
>
>>
>> One comment inline, otherwise
>
> Will add that in next revision.
>
>>
>> Acked-by: Jani Nikula <jani.nikula@intel.com>
>
> Thanks for the review!
>
> -Aaron
>
>>
>> for merging through the ACPI tree, as the change is more likely to
>> conflict there.
>>
>>> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=76491
>>> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=77091
>>> Reported-and-tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
>>> Reported-and-tested-by: Anton Gubarkov <anton.gubarkov@gmail.com>
>>> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
>>> ---
>>>  drivers/acpi/video.c                  | 3 ++-
>>>  drivers/gpu/drm/i915/intel_opregion.c | 7 +++++++
>>>  include/acpi/video.h                  | 2 ++
>>>  3 files changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
>>> index fb9ffe9adc64..cf99d6d2d491 100644
>>> --- a/drivers/acpi/video.c
>>> +++ b/drivers/acpi/video.c
>>> @@ -241,13 +241,14 @@ static bool acpi_video_use_native_backlight(void)
>>>              return use_native_backlight_dmi;
>>>  }
>>>
>>> -static bool acpi_video_verify_backlight_support(void)
>>> +bool acpi_video_verify_backlight_support(void)
>>>  {
>>>      if (acpi_osi_is_win8() && acpi_video_use_native_backlight() &&
>>>          backlight_device_registered(BACKLIGHT_RAW))
>>>              return false;
>>>      return acpi_video_backlight_support();
>>>  }
>>> +EXPORT_SYMBOL(acpi_video_verify_backlight_support);
>>>
>>>  /* backlight device sysfs support */
>>>  static int acpi_video_get_brightness(struct backlight_device *bd)
>>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>>> index 2e2c71fcc9ed..02943d93e88e 100644
>>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>>> @@ -403,6 +403,13 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>>>
>>>      DRM_DEBUG_DRIVER("bclp = 0x%08x\n", bclp);
>>>
>>> +    /*
>>> +     * If the acpi_video interface is not supposed to be used, don't
>>> +     * bother processing backlight level change requests from firmware.
>>> +     */
>>> +    if (!acpi_video_verify_backlight_support())
>>> +            return 0;
>>
>> I'd appreciate a DRM_DEBUG_KMS here about what happened. We're bound to
>> wonder about that staring at some dmesg later on!
>>
>>> +
>>>      if (!(bclp & ASLE_BCLP_VALID))
>>>              return ASLC_BACKLIGHT_FAILED;
>>>
>>> diff --git a/include/acpi/video.h b/include/acpi/video.h
>>> index ea4c7bbded4d..92f8c4bffefb 100644
>>> --- a/include/acpi/video.h
>>> +++ b/include/acpi/video.h
>>> @@ -22,6 +22,7 @@ extern void acpi_video_unregister(void);
>>>  extern void acpi_video_unregister_backlight(void);
>>>  extern int acpi_video_get_edid(struct acpi_device *device, int type,
>>>                             int device_id, void **edid);
>>> +extern bool acpi_video_verify_backlight_support(void);
>>>  #else
>>>  static inline int acpi_video_register(void) { return 0; }
>>>  static inline void acpi_video_unregister(void) { return; }
>>> @@ -31,6 +32,7 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type,
>>>  {
>>>      return -ENODEV;
>>>  }
>>> +static bool acpi_video_verify_backlight_support() { return false; }
>>>  #endif
>>>
>>>  #endif
>>> --
>>> 1.9.3
>>>
>>
>



-- 
-Igor Gnatenko

[PATCH v2] drm/i915/opregion: ignore firmware requests for backlight change

[Aaron Lu]

Some Thinkpad laptops' firmware will initiate a backlight level change
request through operation region on the events of AC plug/unplug, but
since we are not using firmware's interface to do the backlight setting
on these affected laptops, we do not want the firmware to use some
arbitrary value from its ASL variable to set the backlight level on
AC plug/unplug either.

Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=76491
Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=77091
Reported-and-tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
Reported-and-tested-by: Anton Gubarkov <anton.gubarkov@gmail.com>
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
---
v2: add a debug message when ignoring opregion request as suggested by
    Jani Nikula.

 drivers/acpi/video.c                  | 3 ++-
 drivers/gpu/drm/i915/intel_opregion.c | 9 +++++++++
 include/acpi/video.h                  | 2 ++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index fb9ffe9adc64..cf99d6d2d491 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -241,13 +241,14 @@ static bool acpi_video_use_native_backlight(void)
 		return use_native_backlight_dmi;
 }
 
-static bool acpi_video_verify_backlight_support(void)
+bool acpi_video_verify_backlight_support(void)
 {
 	if (acpi_osi_is_win8() && acpi_video_use_native_backlight() &&
 	    backlight_device_registered(BACKLIGHT_RAW))
 		return false;
 	return acpi_video_backlight_support();
 }
+EXPORT_SYMBOL(acpi_video_verify_backlight_support);
 
 /* backlight device sysfs support */
 static int acpi_video_get_brightness(struct backlight_device *bd)
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 2e2c71fcc9ed..4f6b53998d79 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -403,6 +403,15 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
 
 	DRM_DEBUG_DRIVER("bclp = 0x%08x\n", bclp);
 
+	/*
+	 * If the acpi_video interface is not supposed to be used, don't
+	 * bother processing backlight level change requests from firmware.
+	 */
+	if (!acpi_video_verify_backlight_support()) {
+		DRM_DEBUG_KMS("opregion backlight request ignored\n");
+		return 0;
+	}
+
 	if (!(bclp & ASLE_BCLP_VALID))
 		return ASLC_BACKLIGHT_FAILED;
 
diff --git a/include/acpi/video.h b/include/acpi/video.h
index ea4c7bbded4d..92f8c4bffefb 100644
--- a/include/acpi/video.h
+++ b/include/acpi/video.h
@@ -22,6 +22,7 @@ extern void acpi_video_unregister(void);
 extern void acpi_video_unregister_backlight(void);
 extern int acpi_video_get_edid(struct acpi_device *device, int type,
 			       int device_id, void **edid);
+extern bool acpi_video_verify_backlight_support(void);
 #else
 static inline int acpi_video_register(void) { return 0; }
 static inline void acpi_video_unregister(void) { return; }
@@ -31,6 +32,7 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type,
 {
 	return -ENODEV;
 }
+static bool acpi_video_verify_backlight_support() { return false; }
 #endif
 
 #endif
-- 
1.9.3

Re: [PATCH v2] drm/i915/opregion: ignore firmware requests for backlight change

[Rafael J. Wysocki]

On Monday, July 07, 2014 03:43:51 PM Aaron Lu wrote:
> Some Thinkpad laptops' firmware will initiate a backlight level change
> request through operation region on the events of AC plug/unplug, but
> since we are not using firmware's interface to do the backlight setting
> on these affected laptops, we do not want the firmware to use some
> arbitrary value from its ASL variable to set the backlight level on
> AC plug/unplug either.
> 
> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=76491
> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=77091
> Reported-and-tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
> Reported-and-tested-by: Anton Gubarkov <anton.gubarkov@gmail.com>
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> Acked-by: Jani Nikula <jani.nikula@intel.com>

I think we need this in 3.16, right?

Is it also necessary in any -stable and if so, which one?

Rafael


> ---
> v2: add a debug message when ignoring opregion request as suggested by
>     Jani Nikula.
> 
>  drivers/acpi/video.c                  | 3 ++-
>  drivers/gpu/drm/i915/intel_opregion.c | 9 +++++++++
>  include/acpi/video.h                  | 2 ++
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index fb9ffe9adc64..cf99d6d2d491 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -241,13 +241,14 @@ static bool acpi_video_use_native_backlight(void)
>  		return use_native_backlight_dmi;
>  }
>  
> -static bool acpi_video_verify_backlight_support(void)
> +bool acpi_video_verify_backlight_support(void)
>  {
>  	if (acpi_osi_is_win8() && acpi_video_use_native_backlight() &&
>  	    backlight_device_registered(BACKLIGHT_RAW))
>  		return false;
>  	return acpi_video_backlight_support();
>  }
> +EXPORT_SYMBOL(acpi_video_verify_backlight_support);
>  
>  /* backlight device sysfs support */
>  static int acpi_video_get_brightness(struct backlight_device *bd)
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 2e2c71fcc9ed..4f6b53998d79 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -403,6 +403,15 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>  
>  	DRM_DEBUG_DRIVER("bclp = 0x%08x\n", bclp);
>  
> +	/*
> +	 * If the acpi_video interface is not supposed to be used, don't
> +	 * bother processing backlight level change requests from firmware.
> +	 */
> +	if (!acpi_video_verify_backlight_support()) {
> +		DRM_DEBUG_KMS("opregion backlight request ignored\n");
> +		return 0;
> +	}
> +
>  	if (!(bclp & ASLE_BCLP_VALID))
>  		return ASLC_BACKLIGHT_FAILED;
>  
> diff --git a/include/acpi/video.h b/include/acpi/video.h
> index ea4c7bbded4d..92f8c4bffefb 100644
> --- a/include/acpi/video.h
> +++ b/include/acpi/video.h
> @@ -22,6 +22,7 @@ extern void acpi_video_unregister(void);
>  extern void acpi_video_unregister_backlight(void);
>  extern int acpi_video_get_edid(struct acpi_device *device, int type,
>  			       int device_id, void **edid);
> +extern bool acpi_video_verify_backlight_support(void);
>  #else
>  static inline int acpi_video_register(void) { return 0; }
>  static inline void acpi_video_unregister(void) { return; }
> @@ -31,6 +32,7 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type,
>  {
>  	return -ENODEV;
>  }
> +static bool acpi_video_verify_backlight_support() { return false; }
>  #endif
>  
>  #endif
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v2] drm/i915/opregion: ignore firmware requests for backlight change

[Rafael J. Wysocki]

On Monday, July 07, 2014 03:43:51 PM Aaron Lu wrote:
> Some Thinkpad laptops' firmware will initiate a backlight level change
> request through operation region on the events of AC plug/unplug, but
> since we are not using firmware's interface to do the backlight setting
> on these affected laptops, we do not want the firmware to use some
> arbitrary value from its ASL variable to set the backlight level on
> AC plug/unplug either.
> 
> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=76491
> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=77091
> Reported-and-tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
> Reported-and-tested-by: Anton Gubarkov <anton.gubarkov@gmail.com>
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> Acked-by: Jani Nikula <jani.nikula@intel.com>
> ---
> v2: add a debug message when ignoring opregion request as suggested by
>     Jani Nikula.
> 
>  drivers/acpi/video.c                  | 3 ++-
>  drivers/gpu/drm/i915/intel_opregion.c | 9 +++++++++
>  include/acpi/video.h                  | 2 ++
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index fb9ffe9adc64..cf99d6d2d491 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -241,13 +241,14 @@ static bool acpi_video_use_native_backlight(void)
>  		return use_native_backlight_dmi;
>  }
>  
> -static bool acpi_video_verify_backlight_support(void)
> +bool acpi_video_verify_backlight_support(void)
>  {
>  	if (acpi_osi_is_win8() && acpi_video_use_native_backlight() &&
>  	    backlight_device_registered(BACKLIGHT_RAW))
>  		return false;
>  	return acpi_video_backlight_support();
>  }
> +EXPORT_SYMBOL(acpi_video_verify_backlight_support);

Is there any reason for this not to be EXPORT_SYMBOL_GPL()?

>  /* backlight device sysfs support */
>  static int acpi_video_get_brightness(struct backlight_device *bd)
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 2e2c71fcc9ed..4f6b53998d79 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -403,6 +403,15 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>  
>  	DRM_DEBUG_DRIVER("bclp = 0x%08x\n", bclp);
>  
> +	/*
> +	 * If the acpi_video interface is not supposed to be used, don't
> +	 * bother processing backlight level change requests from firmware.
> +	 */
> +	if (!acpi_video_verify_backlight_support()) {
> +		DRM_DEBUG_KMS("opregion backlight request ignored\n");
> +		return 0;
> +	}
> +
>  	if (!(bclp & ASLE_BCLP_VALID))
>  		return ASLC_BACKLIGHT_FAILED;
>  
> diff --git a/include/acpi/video.h b/include/acpi/video.h
> index ea4c7bbded4d..92f8c4bffefb 100644
> --- a/include/acpi/video.h
> +++ b/include/acpi/video.h
> @@ -22,6 +22,7 @@ extern void acpi_video_unregister(void);
>  extern void acpi_video_unregister_backlight(void);
>  extern int acpi_video_get_edid(struct acpi_device *device, int type,
>  			       int device_id, void **edid);
> +extern bool acpi_video_verify_backlight_support(void);
>  #else
>  static inline int acpi_video_register(void) { return 0; }
>  static inline void acpi_video_unregister(void) { return; }
> @@ -31,6 +32,7 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type,
>  {
>  	return -ENODEV;
>  }
> +static bool acpi_video_verify_backlight_support() { return false; }

And for this not to be static inline?

>  #endif
>  
>  #endif
> 

Rafael

Re: [PATCH v2] drm/i915/opregion: ignore firmware requests for backlight change

[Aaron Lu]

On 07/07/2014 09:01 PM, Rafael J. Wysocki wrote:
> On Monday, July 07, 2014 03:43:51 PM Aaron Lu wrote:
>> Some Thinkpad laptops' firmware will initiate a backlight level change
>> request through operation region on the events of AC plug/unplug, but
>> since we are not using firmware's interface to do the backlight setting
>> on these affected laptops, we do not want the firmware to use some
>> arbitrary value from its ASL variable to set the backlight level on
>> AC plug/unplug either.
>>
>> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=76491
>> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=77091
>> Reported-and-tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
>> Reported-and-tested-by: Anton Gubarkov <anton.gubarkov@gmail.com>
>> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
>> Acked-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>> v2: add a debug message when ignoring opregion request as suggested by
>>     Jani Nikula.
>>
>>  drivers/acpi/video.c                  | 3 ++-
>>  drivers/gpu/drm/i915/intel_opregion.c | 9 +++++++++
>>  include/acpi/video.h                  | 2 ++
>>  3 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
>> index fb9ffe9adc64..cf99d6d2d491 100644
>> --- a/drivers/acpi/video.c
>> +++ b/drivers/acpi/video.c
>> @@ -241,13 +241,14 @@ static bool acpi_video_use_native_backlight(void)
>>  		return use_native_backlight_dmi;
>>  }
>>  
>> -static bool acpi_video_verify_backlight_support(void)
>> +bool acpi_video_verify_backlight_support(void)
>>  {
>>  	if (acpi_osi_is_win8() && acpi_video_use_native_backlight() &&
>>  	    backlight_device_registered(BACKLIGHT_RAW))
>>  		return false;
>>  	return acpi_video_backlight_support();
>>  }
>> +EXPORT_SYMBOL(acpi_video_verify_backlight_support);
> 
> Is there any reason for this not to be EXPORT_SYMBOL_GPL()?

No particular reason, just follow what is used for other exported
functions in this file. Will change this to EXPORT_SYMBOL_GPL.

> 
>>  /* backlight device sysfs support */
>>  static int acpi_video_get_brightness(struct backlight_device *bd)
>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> index 2e2c71fcc9ed..4f6b53998d79 100644
>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> @@ -403,6 +403,15 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>>  
>>  	DRM_DEBUG_DRIVER("bclp = 0x%08x\n", bclp);
>>  
>> +	/*
>> +	 * If the acpi_video interface is not supposed to be used, don't
>> +	 * bother processing backlight level change requests from firmware.
>> +	 */
>> +	if (!acpi_video_verify_backlight_support()) {
>> +		DRM_DEBUG_KMS("opregion backlight request ignored\n");
>> +		return 0;
>> +	}
>> +
>>  	if (!(bclp & ASLE_BCLP_VALID))
>>  		return ASLC_BACKLIGHT_FAILED;
>>  
>> diff --git a/include/acpi/video.h b/include/acpi/video.h
>> index ea4c7bbded4d..92f8c4bffefb 100644
>> --- a/include/acpi/video.h
>> +++ b/include/acpi/video.h
>> @@ -22,6 +22,7 @@ extern void acpi_video_unregister(void);
>>  extern void acpi_video_unregister_backlight(void);
>>  extern int acpi_video_get_edid(struct acpi_device *device, int type,
>>  			       int device_id, void **edid);
>> +extern bool acpi_video_verify_backlight_support(void);
>>  #else
>>  static inline int acpi_video_register(void) { return 0; }
>>  static inline void acpi_video_unregister(void) { return; }
>> @@ -31,6 +32,7 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type,
>>  {
>>  	return -ENODEV;
>>  }
>> +static bool acpi_video_verify_backlight_support() { return false; }
> 
> And for this not to be static inline?

Will add inline.

Thanks for the review.

-Aaron

> 
>>  #endif
>>  
>>  #endif
>>
> 
> Rafael
> 

Re: [PATCH v2] drm/i915/opregion: ignore firmware requests for backlight change

[Aaron Lu]

On 07/07/2014 08:51 PM, Rafael J. Wysocki wrote:
> On Monday, July 07, 2014 03:43:51 PM Aaron Lu wrote:
>> Some Thinkpad laptops' firmware will initiate a backlight level change
>> request through operation region on the events of AC plug/unplug, but
>> since we are not using firmware's interface to do the backlight setting
>> on these affected laptops, we do not want the firmware to use some
>> arbitrary value from its ASL variable to set the backlight level on
>> AC plug/unplug either.
>>
>> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=76491
>> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=77091
>> Reported-and-tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
>> Reported-and-tested-by: Anton Gubarkov <anton.gubarkov@gmail.com>
>> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
>> Acked-by: Jani Nikula <jani.nikula@intel.com>
> 
> I think we need this in 3.16, right?

Yes, this patch is for v3.16.

> 
> Is it also necessary in any -stable and if so, which one?

For one of the affected system 'ThinkPad X1 Carbon', it makes sense to
back port the patch to v3.14 and v3.15 since it is in video module's
use_native_backlight DMI table. For the other one, since it is not in
the DMI table, it doesn't make a difference. I'll leave this to you to
decide.

Thanks,
Aaron

> 
> Rafael
> 
> 
>> ---
>> v2: add a debug message when ignoring opregion request as suggested by
>>     Jani Nikula.
>>
>>  drivers/acpi/video.c                  | 3 ++-
>>  drivers/gpu/drm/i915/intel_opregion.c | 9 +++++++++
>>  include/acpi/video.h                  | 2 ++
>>  3 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
>> index fb9ffe9adc64..cf99d6d2d491 100644
>> --- a/drivers/acpi/video.c
>> +++ b/drivers/acpi/video.c
>> @@ -241,13 +241,14 @@ static bool acpi_video_use_native_backlight(void)
>>  		return use_native_backlight_dmi;
>>  }
>>  
>> -static bool acpi_video_verify_backlight_support(void)
>> +bool acpi_video_verify_backlight_support(void)
>>  {
>>  	if (acpi_osi_is_win8() && acpi_video_use_native_backlight() &&
>>  	    backlight_device_registered(BACKLIGHT_RAW))
>>  		return false;
>>  	return acpi_video_backlight_support();
>>  }
>> +EXPORT_SYMBOL(acpi_video_verify_backlight_support);
>>  
>>  /* backlight device sysfs support */
>>  static int acpi_video_get_brightness(struct backlight_device *bd)
>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> index 2e2c71fcc9ed..4f6b53998d79 100644
>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> @@ -403,6 +403,15 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>>  
>>  	DRM_DEBUG_DRIVER("bclp = 0x%08x\n", bclp);
>>  
>> +	/*
>> +	 * If the acpi_video interface is not supposed to be used, don't
>> +	 * bother processing backlight level change requests from firmware.
>> +	 */
>> +	if (!acpi_video_verify_backlight_support()) {
>> +		DRM_DEBUG_KMS("opregion backlight request ignored\n");
>> +		return 0;
>> +	}
>> +
>>  	if (!(bclp & ASLE_BCLP_VALID))
>>  		return ASLC_BACKLIGHT_FAILED;
>>  
>> diff --git a/include/acpi/video.h b/include/acpi/video.h
>> index ea4c7bbded4d..92f8c4bffefb 100644
>> --- a/include/acpi/video.h
>> +++ b/include/acpi/video.h
>> @@ -22,6 +22,7 @@ extern void acpi_video_unregister(void);
>>  extern void acpi_video_unregister_backlight(void);
>>  extern int acpi_video_get_edid(struct acpi_device *device, int type,
>>  			       int device_id, void **edid);
>> +extern bool acpi_video_verify_backlight_support(void);
>>  #else
>>  static inline int acpi_video_register(void) { return 0; }
>>  static inline void acpi_video_unregister(void) { return; }
>> @@ -31,6 +32,7 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type,
>>  {
>>  	return -ENODEV;
>>  }
>> +static bool acpi_video_verify_backlight_support() { return false; }
>>  #endif
>>  
>>  #endif
>>
> 

[PATCH v3] drm/i915/opregion: ignore firmware requests for backlight change

[Aaron Lu]

Some Thinkpad laptops' firmware will initiate a backlight level change
request through operation region on the events of AC plug/unplug, but
since we are not using firmware's interface to do the backlight setting
on these affected laptops, we do not want the firmware to use some
arbitrary value from its ASL variable to set the backlight level on
AC plug/unplug either.

Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=76491
Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=77091
Reported-and-tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
Reported-and-tested-by: Anton Gubarkov <anton.gubarkov@gmail.com>
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
---
v3:
  - use EXPORT_SYMBOL_GPL instead of EXPORT_SYMBOL;
  - add inline to stub
  as suggested by Rafael.

 drivers/acpi/video.c                  | 3 ++-
 drivers/gpu/drm/i915/intel_opregion.c | 9 +++++++++
 include/acpi/video.h                  | 2 ++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index fb9ffe9adc64..5bb1278e9324 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -241,13 +241,14 @@ static bool acpi_video_use_native_backlight(void)
 		return use_native_backlight_dmi;
 }
 
-static bool acpi_video_verify_backlight_support(void)
+bool acpi_video_verify_backlight_support(void)
 {
 	if (acpi_osi_is_win8() && acpi_video_use_native_backlight() &&
 	    backlight_device_registered(BACKLIGHT_RAW))
 		return false;
 	return acpi_video_backlight_support();
 }
+EXPORT_SYMBOL_GPL(acpi_video_verify_backlight_support);
 
 /* backlight device sysfs support */
 static int acpi_video_get_brightness(struct backlight_device *bd)
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 2e2c71fcc9ed..4f6b53998d79 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -403,6 +403,15 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
 
 	DRM_DEBUG_DRIVER("bclp = 0x%08x\n", bclp);
 
+	/*
+	 * If the acpi_video interface is not supposed to be used, don't
+	 * bother processing backlight level change requests from firmware.
+	 */
+	if (!acpi_video_verify_backlight_support()) {
+		DRM_DEBUG_KMS("opregion backlight request ignored\n");
+		return 0;
+	}
+
 	if (!(bclp & ASLE_BCLP_VALID))
 		return ASLC_BACKLIGHT_FAILED;
 
diff --git a/include/acpi/video.h b/include/acpi/video.h
index ea4c7bbded4d..3d0bf9cd88c9 100644
--- a/include/acpi/video.h
+++ b/include/acpi/video.h
@@ -22,6 +22,7 @@ extern void acpi_video_unregister(void);
 extern void acpi_video_unregister_backlight(void);
 extern int acpi_video_get_edid(struct acpi_device *device, int type,
 			       int device_id, void **edid);
+extern bool acpi_video_verify_backlight_support(void);
 #else
 static inline int acpi_video_register(void) { return 0; }
 static inline void acpi_video_unregister(void) { return; }
@@ -31,6 +32,7 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type,
 {
 	return -ENODEV;
 }
+static inline bool acpi_video_verify_backlight_support() { return false; }
 #endif
 
 #endif
-- 
1.9.3

Re: [PATCH v3] drm/i915/opregion: ignore firmware requests for backlight change

[Rafael J. Wysocki]

On Tuesday, July 08, 2014 04:09:25 PM Aaron Lu wrote:
> Some Thinkpad laptops' firmware will initiate a backlight level change
> request through operation region on the events of AC plug/unplug, but
> since we are not using firmware's interface to do the backlight setting
> on these affected laptops, we do not want the firmware to use some
> arbitrary value from its ASL variable to set the backlight level on
> AC plug/unplug either.
> 
> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=76491
> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=77091
> Reported-and-tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
> Reported-and-tested-by: Anton Gubarkov <anton.gubarkov@gmail.com>
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> Acked-by: Jani Nikula <jani.nikula@intel.com>
> ---
> v3:
>   - use EXPORT_SYMBOL_GPL instead of EXPORT_SYMBOL;
>   - add inline to stub
>   as suggested by Rafael.

I've made these two changes already in the commit in my tree.

>  drivers/acpi/video.c                  | 3 ++-
>  drivers/gpu/drm/i915/intel_opregion.c | 9 +++++++++
>  include/acpi/video.h                  | 2 ++
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index fb9ffe9adc64..5bb1278e9324 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -241,13 +241,14 @@ static bool acpi_video_use_native_backlight(void)
>  		return use_native_backlight_dmi;
>  }
>  
> -static bool acpi_video_verify_backlight_support(void)
> +bool acpi_video_verify_backlight_support(void)
>  {
>  	if (acpi_osi_is_win8() && acpi_video_use_native_backlight() &&
>  	    backlight_device_registered(BACKLIGHT_RAW))
>  		return false;
>  	return acpi_video_backlight_support();
>  }
> +EXPORT_SYMBOL_GPL(acpi_video_verify_backlight_support);
>  
>  /* backlight device sysfs support */
>  static int acpi_video_get_brightness(struct backlight_device *bd)
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 2e2c71fcc9ed..4f6b53998d79 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -403,6 +403,15 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>  
>  	DRM_DEBUG_DRIVER("bclp = 0x%08x\n", bclp);
>  
> +	/*
> +	 * If the acpi_video interface is not supposed to be used, don't
> +	 * bother processing backlight level change requests from firmware.
> +	 */
> +	if (!acpi_video_verify_backlight_support()) {
> +		DRM_DEBUG_KMS("opregion backlight request ignored\n");
> +		return 0;
> +	}
> +
>  	if (!(bclp & ASLE_BCLP_VALID))
>  		return ASLC_BACKLIGHT_FAILED;
>  
> diff --git a/include/acpi/video.h b/include/acpi/video.h
> index ea4c7bbded4d..3d0bf9cd88c9 100644
> --- a/include/acpi/video.h
> +++ b/include/acpi/video.h
> @@ -22,6 +22,7 @@ extern void acpi_video_unregister(void);
>  extern void acpi_video_unregister_backlight(void);
>  extern int acpi_video_get_edid(struct acpi_device *device, int type,
>  			       int device_id, void **edid);
> +extern bool acpi_video_verify_backlight_support(void);
>  #else
>  static inline int acpi_video_register(void) { return 0; }
>  static inline void acpi_video_unregister(void) { return; }
> @@ -31,6 +32,7 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type,
>  {
>  	return -ENODEV;
>  }
> +static inline bool acpi_video_verify_backlight_support() { return false; }

And here you need acpi_video_verify_backlight_support(void) (fixed up too).

>  #endif
>  
>  #endif
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3] drm/i915/opregion: ignore firmware requests for backlight change

[Aaron Lu]

On 07/08/2014 08:48 PM, Rafael J. Wysocki wrote:
> On Tuesday, July 08, 2014 04:09:25 PM Aaron Lu wrote:
>> Some Thinkpad laptops' firmware will initiate a backlight level change
>> request through operation region on the events of AC plug/unplug, but
>> since we are not using firmware's interface to do the backlight setting
>> on these affected laptops, we do not want the firmware to use some
>> arbitrary value from its ASL variable to set the backlight level on
>> AC plug/unplug either.
>>
>> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=76491
>> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=77091
>> Reported-and-tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
>> Reported-and-tested-by: Anton Gubarkov <anton.gubarkov@gmail.com>
>> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
>> Acked-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>> v3:
>>   - use EXPORT_SYMBOL_GPL instead of EXPORT_SYMBOL;
>>   - add inline to stub
>>   as suggested by Rafael.
> 
> I've made these two changes already in the commit in my tree.

Good, thanks!

-Aaron

> 
>>  drivers/acpi/video.c                  | 3 ++-
>>  drivers/gpu/drm/i915/intel_opregion.c | 9 +++++++++
>>  include/acpi/video.h                  | 2 ++
>>  3 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
>> index fb9ffe9adc64..5bb1278e9324 100644
>> --- a/drivers/acpi/video.c
>> +++ b/drivers/acpi/video.c
>> @@ -241,13 +241,14 @@ static bool acpi_video_use_native_backlight(void)
>>  		return use_native_backlight_dmi;
>>  }
>>  
>> -static bool acpi_video_verify_backlight_support(void)
>> +bool acpi_video_verify_backlight_support(void)
>>  {
>>  	if (acpi_osi_is_win8() && acpi_video_use_native_backlight() &&
>>  	    backlight_device_registered(BACKLIGHT_RAW))
>>  		return false;
>>  	return acpi_video_backlight_support();
>>  }
>> +EXPORT_SYMBOL_GPL(acpi_video_verify_backlight_support);
>>  
>>  /* backlight device sysfs support */
>>  static int acpi_video_get_brightness(struct backlight_device *bd)
>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> index 2e2c71fcc9ed..4f6b53998d79 100644
>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> @@ -403,6 +403,15 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>>  
>>  	DRM_DEBUG_DRIVER("bclp = 0x%08x\n", bclp);
>>  
>> +	/*
>> +	 * If the acpi_video interface is not supposed to be used, don't
>> +	 * bother processing backlight level change requests from firmware.
>> +	 */
>> +	if (!acpi_video_verify_backlight_support()) {
>> +		DRM_DEBUG_KMS("opregion backlight request ignored\n");
>> +		return 0;
>> +	}
>> +
>>  	if (!(bclp & ASLE_BCLP_VALID))
>>  		return ASLC_BACKLIGHT_FAILED;
>>  
>> diff --git a/include/acpi/video.h b/include/acpi/video.h
>> index ea4c7bbded4d..3d0bf9cd88c9 100644
>> --- a/include/acpi/video.h
>> +++ b/include/acpi/video.h
>> @@ -22,6 +22,7 @@ extern void acpi_video_unregister(void);
>>  extern void acpi_video_unregister_backlight(void);
>>  extern int acpi_video_get_edid(struct acpi_device *device, int type,
>>  			       int device_id, void **edid);
>> +extern bool acpi_video_verify_backlight_support(void);
>>  #else
>>  static inline int acpi_video_register(void) { return 0; }
>>  static inline void acpi_video_unregister(void) { return; }
>> @@ -31,6 +32,7 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type,
>>  {
>>  	return -ENODEV;
>>  }
>> +static inline bool acpi_video_verify_backlight_support() { return false; }
> 
> And here you need acpi_video_verify_backlight_support(void) (fixed up too).
> 
>>  #endif
>>  
>>  #endif
>>
>