[2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

Commit Message

Aaron Lu Sept. 9, 2013, 8:42 a.m. UTC

According to Matthew Garrett, "Windows 8 leaves backlight control up
to individual graphics drivers rather than making ACPI calls itself.
There's plenty of evidence to suggest that the Intel driver for
Windows [8] doesn't use the ACPI interface, including the fact that
it's broken on a bunch of machines when the OS claims to support
Windows 8.  The simplest thing to do appears to be to disable the
ACPI backlight interface on these systems".

There's a problem with that approach, however, because simply
avoiding to register the ACPI backlight interface if the firmware
calls _OSI for Windows 8 may not work in the following situations:
 (1) The ACPI backlight interface actually works on the given system
     and the i915 driver is not loaded (e.g. another graphics driver
     is used).
 (2) The ACPI backlight interface doesn't work on the given system,
     but there is a vendor platform driver that will register its
     own, equally broken, backlight interface if not prevented from
     doing so by the ACPI subsystem.
Therefore we need to allow the ACPI backlight interface to be
registered until the i915 driver is loaded which then will unregister
it if the firmware has called _OSI for Windows 8 (or will register
the ACPI video driver without backlight support if not already
present).

For this reason, introduce an alternative function for registering
ACPI video, __acpi_video_register(bool), that if ture is passed,
will check whether or not the ACPI video driver has already been
registered and whether or not the backlight Windows 8 quirk has to
be applied. If the quirk has to be applied, it will block the ACPI
backlight support and either unregister the backlight interface if
the ACPI video driver has already been registered, or register the
ACPI video driver without the backlight interface otherwise.  Make
the i915 driver use __acpi_video_register() instead of
acpi_video_register() in i915_driver_load(), and the param passed
there is controlled by the i915 module level parameter
i915_take_over_backlight, which is set to false by default.

This change is evolved from earlier patches of Matthew Garrett,
Chun-Yi Lee and Seth Forshee and is heavily based on two patches
from Rafael:
https://lkml.org/lkml/2013/7/17/720
https://lkml.org/lkml/2013/7/24/806

Signed-off-by: Aaron Lu <[e-mail]>
---
 drivers/acpi/internal.h         |  2 ++
 drivers/acpi/video.c            | 24 ++++++++++++++++--------
 drivers/acpi/video_detect.c     | 15 ++++++++++++++-
 drivers/gpu/drm/i915/i915_dma.c |  2 +-
 drivers/gpu/drm/i915/i915_drv.c |  5 +++++
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 include/acpi/video.h            |  9 +++++++--
 include/linux/acpi.h            |  1 +
 8 files changed, 47 insertions(+), 12 deletions(-)

Comments

Daniel Vetter Sept. 9, 2013, 9:32 a.m. UTC | #1

Hi Aaaron,

Have we grown any clue meanwhile about which Intel boxes need this and for
which we still need to keep the acpi backlight around? I've grown _very_
reluctant to just adding tons of quirks to our driver for the backlight.
Almost all the quirks we have added recently (or that have been proposed
to be added) are for the backlight. Imo that indicates we get something
fundamentally wrong ...

Cheers, Daniel

On Mon, Sep 09, 2013 at 04:42:20PM +0800, Aaron Lu wrote:
> According to Matthew Garrett, "Windows 8 leaves backlight control up
> to individual graphics drivers rather than making ACPI calls itself.
> There's plenty of evidence to suggest that the Intel driver for
> Windows [8] doesn't use the ACPI interface, including the fact that
> it's broken on a bunch of machines when the OS claims to support
> Windows 8.  The simplest thing to do appears to be to disable the
> ACPI backlight interface on these systems".
> 
> There's a problem with that approach, however, because simply
> avoiding to register the ACPI backlight interface if the firmware
> calls _OSI for Windows 8 may not work in the following situations:
>  (1) The ACPI backlight interface actually works on the given system
>      and the i915 driver is not loaded (e.g. another graphics driver
>      is used).
>  (2) The ACPI backlight interface doesn't work on the given system,
>      but there is a vendor platform driver that will register its
>      own, equally broken, backlight interface if not prevented from
>      doing so by the ACPI subsystem.
> Therefore we need to allow the ACPI backlight interface to be
> registered until the i915 driver is loaded which then will unregister
> it if the firmware has called _OSI for Windows 8 (or will register
> the ACPI video driver without backlight support if not already
> present).
> 
> For this reason, introduce an alternative function for registering
> ACPI video, __acpi_video_register(bool), that if ture is passed,
> will check whether or not the ACPI video driver has already been
> registered and whether or not the backlight Windows 8 quirk has to
> be applied. If the quirk has to be applied, it will block the ACPI
> backlight support and either unregister the backlight interface if
> the ACPI video driver has already been registered, or register the
> ACPI video driver without the backlight interface otherwise.  Make
> the i915 driver use __acpi_video_register() instead of
> acpi_video_register() in i915_driver_load(), and the param passed
> there is controlled by the i915 module level parameter
> i915_take_over_backlight, which is set to false by default.
> 
> This change is evolved from earlier patches of Matthew Garrett,
> Chun-Yi Lee and Seth Forshee and is heavily based on two patches
> from Rafael:
> https://lkml.org/lkml/2013/7/17/720
> https://lkml.org/lkml/2013/7/24/806
> 
> Signed-off-by: Aaron Lu <[e-mail]>
> ---
>  drivers/acpi/internal.h         |  2 ++
>  drivers/acpi/video.c            | 24 ++++++++++++++++--------
>  drivers/acpi/video_detect.c     | 15 ++++++++++++++-
>  drivers/gpu/drm/i915/i915_dma.c |  2 +-
>  drivers/gpu/drm/i915/i915_drv.c |  5 +++++
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  include/acpi/video.h            |  9 +++++++--
>  include/linux/acpi.h            |  1 +
>  8 files changed, 47 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index 20f4233..a53832e 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -170,8 +170,10 @@ int acpi_create_platform_device(struct acpi_device *adev,
>    -------------------------------------------------------------------------- */
>  #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
>  bool acpi_video_backlight_quirks(void);
> +bool acpi_video_verify_backlight_support(void);
>  #else
>  static inline bool acpi_video_backlight_quirks(void) { return false; }
> +static inline bool acpi_video_verify_backlight_support(void) { return false; }
>  #endif
>  
>  #endif /* _ACPI_INTERNAL_H_ */
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 5ad5a71..cc709a7 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -1256,8 +1256,8 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event)
>  	unsigned long long level_current, level_next;
>  	int result = -EINVAL;
>  
> -	/* no warning message if acpi_backlight=vendor is used */
> -	if (!acpi_video_backlight_support())
> +	/* no warning message if acpi_backlight=vendor or a quirk is used */
> +	if (!acpi_video_verify_backlight_support())
>  		return 0;
>  
>  	if (!device->brightness)
> @@ -1558,7 +1558,7 @@ acpi_video_bus_match(acpi_handle handle, u32 level, void *context,
>  
>  static void acpi_video_dev_register_backlight(struct acpi_video_device *device)
>  {
> -	if (acpi_video_backlight_support()) {
> +	if (acpi_video_verify_backlight_support()) {
>  		struct backlight_properties props;
>  		struct pci_dev *pdev;
>  		acpi_handle acpi_parent;
> @@ -1933,14 +1933,22 @@ static int __init intel_opregion_present(void)
>  	return opregion;
>  }
>  
> -int acpi_video_register(void)
> +int __acpi_video_register(bool backlight_quirks)
>  {
> -	int result = 0;
> +	bool no_backlight;
> +	int result;
> +
> +	no_backlight = backlight_quirks ? acpi_video_backlight_quirks() : false;
> +
>  	if (register_count) {
>  		/*
> -		 * if the function of acpi_video_register is already called,
> -		 * don't register the acpi_vide_bus again and return no error.
> +		 * If acpi_video_register() has been called already, don't try
> +		 * to register acpi_video_bus, but unregister backlight devices
> +		 * if no backlight support is requested.
>  		 */
> +		if (no_backlight)
> +			acpi_video_unregister_backlight();
> +
>  		return 0;
>  	}
>  
> @@ -1959,7 +1967,7 @@ int acpi_video_register(void)
>  
>  	return 0;
>  }
> -EXPORT_SYMBOL(acpi_video_register);
> +EXPORT_SYMBOL(__acpi_video_register);
>  
>  void acpi_video_unregister(void)
>  {
> diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
> index 940edbf..c7e43ec 100644
> --- a/drivers/acpi/video_detect.c
> +++ b/drivers/acpi/video_detect.c
> @@ -235,7 +235,12 @@ static void acpi_video_caps_check(void)
>  
>  bool acpi_video_backlight_quirks(void)
>  {
> -	return acpi_gbl_osi_data >= ACPI_OSI_WIN_8;
> +	if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8) {
> +		acpi_video_caps_check();
> +		acpi_video_support |= ACPI_VIDEO_SKIP_BACKLIGHT;
> +		return true;
> +	}
> +	return false;
>  }
>  EXPORT_SYMBOL(acpi_video_backlight_quirks);
>  
> @@ -283,6 +288,14 @@ int acpi_video_backlight_support(void)
>  }
>  EXPORT_SYMBOL(acpi_video_backlight_support);
>  
> +/* For the ACPI video driver use only. */
> +bool acpi_video_verify_backlight_support(void)
> +{
> +	return (acpi_video_support & ACPI_VIDEO_SKIP_BACKLIGHT) ?
> +		false : acpi_video_backlight_support();
> +}
> +EXPORT_SYMBOL(acpi_video_verify_backlight_support);
> +
>  /*
>   * Use acpi_backlight=vendor/video to force that backlight switching
>   * is processed by vendor specific acpi drivers or video.ko driver.
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index f466980..75fba17 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	if (INTEL_INFO(dev)->num_pipes) {
>  		/* Must be done after probing outputs */
>  		intel_opregion_init(dev);
> -		acpi_video_register();
> +		__acpi_video_register(i915_take_over_backlight);
>  	}
>  
>  	if (IS_GEN5(dev))
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 45b3c03..b014398 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -132,6 +132,11 @@ int i915_enable_ips __read_mostly = 1;
>  module_param_named(enable_ips, i915_enable_ips, int, 0600);
>  MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
>  
> +bool i915_take_over_backlight = false;
> +module_param_named(take_over_backlight, i915_take_over_backlight, bool, 0644);
> +MODULE_PARM_DESC(take_over_backlight,
> +		"Prevent ACPI backlight from being used (default: false)");
> +
>  static struct drm_driver driver;
>  extern int intel_agp_enabled;
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1929bff..b43c430 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1543,6 +1543,7 @@ extern int i915_enable_ppgtt __read_mostly;
>  extern unsigned int i915_preliminary_hw_support __read_mostly;
>  extern int i915_disable_power_well __read_mostly;
>  extern int i915_enable_ips __read_mostly;
> +extern bool i915_take_over_backlight __read_mostly;
>  
>  extern int i915_suspend(struct drm_device *dev, pm_message_t state);
>  extern int i915_resume(struct drm_device *dev);
> diff --git a/include/acpi/video.h b/include/acpi/video.h
> index 0c665b5..dc5b8ad 100644
> --- a/include/acpi/video.h
> +++ b/include/acpi/video.h
> @@ -17,13 +17,13 @@ struct acpi_device;
>  #define ACPI_VIDEO_DISPLAY_LEGACY_TV      0x0200
>  
>  #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
> -extern int acpi_video_register(void);
> +extern int __acpi_video_register(bool backlight_quirks);
>  extern void acpi_video_unregister(void);
>  extern int acpi_video_unregister_backlight(void);
>  extern int acpi_video_get_edid(struct acpi_device *device, int type,
>  			       int device_id, void **edid);
>  #else
> -static inline int acpi_video_register(void) { return 0; }
> +static inline int __acpi_video_register(bool backlight_quirks) { return 0; }
>  static inline void acpi_video_unregister(void) { return; }
>  static inline int acpi_video_unregister_backlight(void) { return; }
>  static inline int acpi_video_get_edid(struct acpi_device *device, int type,
> @@ -33,4 +33,9 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type,
>  }
>  #endif
>  
> +static inline int acpi_video_register(void)
> +{
> +	return __acpi_video_register(false);
> +}
> +
>  #endif
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index a5db4ae..a0dcce5 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -191,6 +191,7 @@ extern bool wmi_has_guid(const char *guid);
>  #define ACPI_VIDEO_BACKLIGHT_DMI_VIDEO			0x0200
>  #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VENDOR		0x0400
>  #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VIDEO		0x0800
> +#define ACPI_VIDEO_SKIP_BACKLIGHT			0x1000
>  
>  #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
>  
> -- 
> 1.8.4.12.g2ea3df6
>

Igor Gnatenko Sept. 9, 2013, 11:44 a.m. UTC | #2

On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote:
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index f466980..75fba17 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	if (INTEL_INFO(dev)->num_pipes) {
>  		/* Must be done after probing outputs */
>  		intel_opregion_init(dev);
> -		acpi_video_register();
> +		__acpi_video_register(i915_take_over_backlight);
>  	}
>  
>  	if (IS_GEN5(dev))

I can't compile:


DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load':
DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit
declaration of function
'__acpi_video_register' [-Werror=implicit-function-declaration]
DEBUG:    __acpi_video_register(i915_take_over_backlight);
DEBUG:    ^
DEBUG: cc1: some warnings being treated as errors
DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1
DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2
DEBUG: make[2]: *** [drivers/gpu/drm] Error 2
DEBUG: make[1]: *** [drivers/gpu] Error 2
DEBUG: make: *** [drivers] Error 2

Rafael Wysocki Sept. 9, 2013, 12:16 p.m. UTC | #3

Hi,

On Monday, September 09, 2013 11:32:10 AM Daniel Vetter wrote:
> Hi Aaaron,
> 
> Have we grown any clue meanwhile about which Intel boxes need this and for
> which we still need to keep the acpi backlight around?

First of all, there is a bunch of boxes where ACPI backlight works incorrectly
because of the Win8 compatibility issue.  [In short, if we say we are compatible
with Win8, the backlight AML goes into a special code path that is broken on
those machines. Presumably Win8 uses native backlight control on them and that
AML code path is never executed there.]  The collection of machines with this
problem appears to be kind of random (various models from various vendors), but
I *think* they are machines that originally shipped with Win7 with a Win8
"upgrade" option (which in practice requires the BIOS to be updated anyway).

Because of that, last time we tried to switch all of the systems using i915
and telling the BIOS that they are compatible with Win8 over to native backlight
control, but that didn't work for two reasons.  The first reason is that some
user space doesn't know how to use intel_backlight and needs to be taught about
that (so some systems are just not ready for that switch).  The second and more
fundamental reason is that the native backlight control simply doesn't work on
some machines and we don't seem to have any idea why and how to debug this
particular problem (mostly because we don't have enough information and we
don't know what to ask for).

> I've grown _very_ reluctant to just adding tons of quirks to our driver for
> the backlight.
>
> Almost all the quirks we have added recently (or that have been proposed
> to be added) are for the backlight. Imo that indicates we get something
> fundamentally wrong ...

This thing isn't really a quirk.  It rather is an option for the users of
the systems where ACPI backlight doesn't work to switch over to the native
backlight control using a command line switch.  This way they can at least
*see* if the native backlight control works for them and (hopefully) report
problems if that's not the case.  This gives us a chance to get more
information about what the problem is and possibly to make some progress
without making changes for everyone, reverting those changes when they don't
work etc.

An alternative for them is to pass acpi_osi="!Windows 2012" which will probably
make the ACPI backlight work for them again, but this rather is a step back,
because we can't possibly learn anything new from that.

Kind regards,
Rafael


> On Mon, Sep 09, 2013 at 04:42:20PM +0800, Aaron Lu wrote:
> > According to Matthew Garrett, "Windows 8 leaves backlight control up
> > to individual graphics drivers rather than making ACPI calls itself.
> > There's plenty of evidence to suggest that the Intel driver for
> > Windows [8] doesn't use the ACPI interface, including the fact that
> > it's broken on a bunch of machines when the OS claims to support
> > Windows 8.  The simplest thing to do appears to be to disable the
> > ACPI backlight interface on these systems".
> > 
> > There's a problem with that approach, however, because simply
> > avoiding to register the ACPI backlight interface if the firmware
> > calls _OSI for Windows 8 may not work in the following situations:
> >  (1) The ACPI backlight interface actually works on the given system
> >      and the i915 driver is not loaded (e.g. another graphics driver
> >      is used).
> >  (2) The ACPI backlight interface doesn't work on the given system,
> >      but there is a vendor platform driver that will register its
> >      own, equally broken, backlight interface if not prevented from
> >      doing so by the ACPI subsystem.
> > Therefore we need to allow the ACPI backlight interface to be
> > registered until the i915 driver is loaded which then will unregister
> > it if the firmware has called _OSI for Windows 8 (or will register
> > the ACPI video driver without backlight support if not already
> > present).
> > 
> > For this reason, introduce an alternative function for registering
> > ACPI video, __acpi_video_register(bool), that if ture is passed,
> > will check whether or not the ACPI video driver has already been
> > registered and whether or not the backlight Windows 8 quirk has to
> > be applied. If the quirk has to be applied, it will block the ACPI
> > backlight support and either unregister the backlight interface if
> > the ACPI video driver has already been registered, or register the
> > ACPI video driver without the backlight interface otherwise.  Make
> > the i915 driver use __acpi_video_register() instead of
> > acpi_video_register() in i915_driver_load(), and the param passed
> > there is controlled by the i915 module level parameter
> > i915_take_over_backlight, which is set to false by default.
> > 
> > This change is evolved from earlier patches of Matthew Garrett,
> > Chun-Yi Lee and Seth Forshee and is heavily based on two patches
> > from Rafael:
> > https://lkml.org/lkml/2013/7/17/720
> > https://lkml.org/lkml/2013/7/24/806
> > 
> > Signed-off-by: Aaron Lu <[e-mail]>
> > ---
> >  drivers/acpi/internal.h         |  2 ++
> >  drivers/acpi/video.c            | 24 ++++++++++++++++--------
> >  drivers/acpi/video_detect.c     | 15 ++++++++++++++-
> >  drivers/gpu/drm/i915/i915_dma.c |  2 +-
> >  drivers/gpu/drm/i915/i915_drv.c |  5 +++++
> >  drivers/gpu/drm/i915/i915_drv.h |  1 +
> >  include/acpi/video.h            |  9 +++++++--
> >  include/linux/acpi.h            |  1 +
> >  8 files changed, 47 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> > index 20f4233..a53832e 100644
> > --- a/drivers/acpi/internal.h
> > +++ b/drivers/acpi/internal.h
> > @@ -170,8 +170,10 @@ int acpi_create_platform_device(struct acpi_device *adev,
> >    -------------------------------------------------------------------------- */
> >  #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
> >  bool acpi_video_backlight_quirks(void);
> > +bool acpi_video_verify_backlight_support(void);
> >  #else
> >  static inline bool acpi_video_backlight_quirks(void) { return false; }
> > +static inline bool acpi_video_verify_backlight_support(void) { return false; }
> >  #endif
> >  
> >  #endif /* _ACPI_INTERNAL_H_ */
> > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> > index 5ad5a71..cc709a7 100644
> > --- a/drivers/acpi/video.c
> > +++ b/drivers/acpi/video.c
> > @@ -1256,8 +1256,8 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event)
> >  	unsigned long long level_current, level_next;
> >  	int result = -EINVAL;
> >  
> > -	/* no warning message if acpi_backlight=vendor is used */
> > -	if (!acpi_video_backlight_support())
> > +	/* no warning message if acpi_backlight=vendor or a quirk is used */
> > +	if (!acpi_video_verify_backlight_support())
> >  		return 0;
> >  
> >  	if (!device->brightness)
> > @@ -1558,7 +1558,7 @@ acpi_video_bus_match(acpi_handle handle, u32 level, void *context,
> >  
> >  static void acpi_video_dev_register_backlight(struct acpi_video_device *device)
> >  {
> > -	if (acpi_video_backlight_support()) {
> > +	if (acpi_video_verify_backlight_support()) {
> >  		struct backlight_properties props;
> >  		struct pci_dev *pdev;
> >  		acpi_handle acpi_parent;
> > @@ -1933,14 +1933,22 @@ static int __init intel_opregion_present(void)
> >  	return opregion;
> >  }
> >  
> > -int acpi_video_register(void)
> > +int __acpi_video_register(bool backlight_quirks)
> >  {
> > -	int result = 0;
> > +	bool no_backlight;
> > +	int result;
> > +
> > +	no_backlight = backlight_quirks ? acpi_video_backlight_quirks() : false;
> > +
> >  	if (register_count) {
> >  		/*
> > -		 * if the function of acpi_video_register is already called,
> > -		 * don't register the acpi_vide_bus again and return no error.
> > +		 * If acpi_video_register() has been called already, don't try
> > +		 * to register acpi_video_bus, but unregister backlight devices
> > +		 * if no backlight support is requested.
> >  		 */
> > +		if (no_backlight)
> > +			acpi_video_unregister_backlight();
> > +
> >  		return 0;
> >  	}
> >  
> > @@ -1959,7 +1967,7 @@ int acpi_video_register(void)
> >  
> >  	return 0;
> >  }
> > -EXPORT_SYMBOL(acpi_video_register);
> > +EXPORT_SYMBOL(__acpi_video_register);
> >  
> >  void acpi_video_unregister(void)
> >  {
> > diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
> > index 940edbf..c7e43ec 100644
> > --- a/drivers/acpi/video_detect.c
> > +++ b/drivers/acpi/video_detect.c
> > @@ -235,7 +235,12 @@ static void acpi_video_caps_check(void)
> >  
> >  bool acpi_video_backlight_quirks(void)
> >  {
> > -	return acpi_gbl_osi_data >= ACPI_OSI_WIN_8;
> > +	if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8) {
> > +		acpi_video_caps_check();
> > +		acpi_video_support |= ACPI_VIDEO_SKIP_BACKLIGHT;
> > +		return true;
> > +	}
> > +	return false;
> >  }
> >  EXPORT_SYMBOL(acpi_video_backlight_quirks);
> >  
> > @@ -283,6 +288,14 @@ int acpi_video_backlight_support(void)
> >  }
> >  EXPORT_SYMBOL(acpi_video_backlight_support);
> >  
> > +/* For the ACPI video driver use only. */
> > +bool acpi_video_verify_backlight_support(void)
> > +{
> > +	return (acpi_video_support & ACPI_VIDEO_SKIP_BACKLIGHT) ?
> > +		false : acpi_video_backlight_support();
> > +}
> > +EXPORT_SYMBOL(acpi_video_verify_backlight_support);
> > +
> >  /*
> >   * Use acpi_backlight=vendor/video to force that backlight switching
> >   * is processed by vendor specific acpi drivers or video.ko driver.
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index f466980..75fba17 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  	if (INTEL_INFO(dev)->num_pipes) {
> >  		/* Must be done after probing outputs */
> >  		intel_opregion_init(dev);
> > -		acpi_video_register();
> > +		__acpi_video_register(i915_take_over_backlight);
> >  	}
> >  
> >  	if (IS_GEN5(dev))
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 45b3c03..b014398 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -132,6 +132,11 @@ int i915_enable_ips __read_mostly = 1;
> >  module_param_named(enable_ips, i915_enable_ips, int, 0600);
> >  MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
> >  
> > +bool i915_take_over_backlight = false;
> > +module_param_named(take_over_backlight, i915_take_over_backlight, bool, 0644);
> > +MODULE_PARM_DESC(take_over_backlight,
> > +		"Prevent ACPI backlight from being used (default: false)");
> > +
> >  static struct drm_driver driver;
> >  extern int intel_agp_enabled;
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 1929bff..b43c430 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1543,6 +1543,7 @@ extern int i915_enable_ppgtt __read_mostly;
> >  extern unsigned int i915_preliminary_hw_support __read_mostly;
> >  extern int i915_disable_power_well __read_mostly;
> >  extern int i915_enable_ips __read_mostly;
> > +extern bool i915_take_over_backlight __read_mostly;
> >  
> >  extern int i915_suspend(struct drm_device *dev, pm_message_t state);
> >  extern int i915_resume(struct drm_device *dev);
> > diff --git a/include/acpi/video.h b/include/acpi/video.h
> > index 0c665b5..dc5b8ad 100644
> > --- a/include/acpi/video.h
> > +++ b/include/acpi/video.h
> > @@ -17,13 +17,13 @@ struct acpi_device;
> >  #define ACPI_VIDEO_DISPLAY_LEGACY_TV      0x0200
> >  
> >  #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
> > -extern int acpi_video_register(void);
> > +extern int __acpi_video_register(bool backlight_quirks);
> >  extern void acpi_video_unregister(void);
> >  extern int acpi_video_unregister_backlight(void);
> >  extern int acpi_video_get_edid(struct acpi_device *device, int type,
> >  			       int device_id, void **edid);
> >  #else
> > -static inline int acpi_video_register(void) { return 0; }
> > +static inline int __acpi_video_register(bool backlight_quirks) { return 0; }
> >  static inline void acpi_video_unregister(void) { return; }
> >  static inline int acpi_video_unregister_backlight(void) { return; }
> >  static inline int acpi_video_get_edid(struct acpi_device *device, int type,
> > @@ -33,4 +33,9 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type,
> >  }
> >  #endif
> >  
> > +static inline int acpi_video_register(void)
> > +{
> > +	return __acpi_video_register(false);
> > +}
> > +
> >  #endif
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index a5db4ae..a0dcce5 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -191,6 +191,7 @@ extern bool wmi_has_guid(const char *guid);
> >  #define ACPI_VIDEO_BACKLIGHT_DMI_VIDEO			0x0200
> >  #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VENDOR		0x0400
> >  #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VIDEO		0x0800
> > +#define ACPI_VIDEO_SKIP_BACKLIGHT			0x1000
> >  
> >  #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
> >  
> 
>

Daniel Vetter Sept. 9, 2013, 3:21 p.m. UTC | #4

On Mon, Sep 09, 2013 at 02:16:12PM +0200, Rafael J. Wysocki wrote:
> Hi,
> 
> On Monday, September 09, 2013 11:32:10 AM Daniel Vetter wrote:
> > Hi Aaaron,
> > 
> > Have we grown any clue meanwhile about which Intel boxes need this and for
> > which we still need to keep the acpi backlight around?
> 
> First of all, there is a bunch of boxes where ACPI backlight works incorrectly
> because of the Win8 compatibility issue.  [In short, if we say we are compatible
> with Win8, the backlight AML goes into a special code path that is broken on
> those machines. Presumably Win8 uses native backlight control on them and that
> AML code path is never executed there.]  The collection of machines with this
> problem appears to be kind of random (various models from various vendors), but
> I *think* they are machines that originally shipped with Win7 with a Win8
> "upgrade" option (which in practice requires the BIOS to be updated anyway).
> 
> Because of that, last time we tried to switch all of the systems using i915
> and telling the BIOS that they are compatible with Win8 over to native backlight
> control, but that didn't work for two reasons.  The first reason is that some
> user space doesn't know how to use intel_backlight and needs to be taught about
> that (so some systems are just not ready for that switch).  The second and more
> fundamental reason is that the native backlight control simply doesn't work on
> some machines and we don't seem to have any idea why and how to debug this
> particular problem (mostly because we don't have enough information and we
> don't know what to ask for).
> 
> > I've grown _very_ reluctant to just adding tons of quirks to our driver for
> > the backlight.
> >
> > Almost all the quirks we have added recently (or that have been proposed
> > to be added) are for the backlight. Imo that indicates we get something
> > fundamentally wrong ...
> 
> This thing isn't really a quirk.  It rather is an option for the users of
> the systems where ACPI backlight doesn't work to switch over to the native
> backlight control using a command line switch.  This way they can at least
> *see* if the native backlight control works for them and (hopefully) report
> problems if that's not the case.  This gives us a chance to get more
> information about what the problem is and possibly to make some progress
> without making changes for everyone, reverting those changes when they don't
> work etc.
> 
> An alternative for them is to pass acpi_osi="!Windows 2012" which will probably
> make the ACPI backlight work for them again, but this rather is a step back,
> because we can't possibly learn anything new from that.

If Win8 is as broken as we are I'm ok with the module option. It just
sounded to me like right now we don't know of a way to make all machines
somewhat happy, combined with the other pile of random backlight issues
the assumption that we do something (maybe something a bit racy) that
windows doesn't do isn't too far-fetched. So I'm not wary of the machines
where the aml is busted for acpi_os=win8, but for the others where this
broke stuff.

Or do I miss something here?

Cheers, Daniel

Matthew Garrett Sept. 9, 2013, 3:38 p.m. UTC | #5

On Mon, 2013-09-09 at 17:21 +0200, Daniel Vetter wrote:

> If Win8 is as broken as we are I'm ok with the module option. It just
> sounded to me like right now we don't know of a way to make all machines
> somewhat happy, combined with the other pile of random backlight issues
> the assumption that we do something (maybe something a bit racy) that
> windows doesn't do isn't too far-fetched. So I'm not wary of the machines
> where the aml is busted for acpi_os=win8, but for the others where this
> broke stuff.

Windows 8 isn't broken because (as far as we can tell) the Intel drivers
under Windows 8 never use the ACPI backlight set function. Of course, it
would be nice to have that confirmed by Intel.

Rafael Wysocki Sept. 9, 2013, 8:23 p.m. UTC | #6

On Monday, September 09, 2013 05:21:18 PM Daniel Vetter wrote:
> On Mon, Sep 09, 2013 at 02:16:12PM +0200, Rafael J. Wysocki wrote:
> > Hi,
> > 
> > On Monday, September 09, 2013 11:32:10 AM Daniel Vetter wrote:
> > > Hi Aaaron,
> > > 
> > > Have we grown any clue meanwhile about which Intel boxes need this and for
> > > which we still need to keep the acpi backlight around?
> > 
> > First of all, there is a bunch of boxes where ACPI backlight works incorrectly
> > because of the Win8 compatibility issue.  [In short, if we say we are compatible
> > with Win8, the backlight AML goes into a special code path that is broken on
> > those machines. Presumably Win8 uses native backlight control on them and that
> > AML code path is never executed there.]  The collection of machines with this
> > problem appears to be kind of random (various models from various vendors), but
> > I *think* they are machines that originally shipped with Win7 with a Win8
> > "upgrade" option (which in practice requires the BIOS to be updated anyway).
> > 
> > Because of that, last time we tried to switch all of the systems using i915
> > and telling the BIOS that they are compatible with Win8 over to native backlight
> > control, but that didn't work for two reasons.  The first reason is that some
> > user space doesn't know how to use intel_backlight and needs to be taught about
> > that (so some systems are just not ready for that switch).  The second and more
> > fundamental reason is that the native backlight control simply doesn't work on
> > some machines and we don't seem to have any idea why and how to debug this
> > particular problem (mostly because we don't have enough information and we
> > don't know what to ask for).
> > 
> > > I've grown _very_ reluctant to just adding tons of quirks to our driver for
> > > the backlight.
> > >
> > > Almost all the quirks we have added recently (or that have been proposed
> > > to be added) are for the backlight. Imo that indicates we get something
> > > fundamentally wrong ...
> > 
> > This thing isn't really a quirk.  It rather is an option for the users of
> > the systems where ACPI backlight doesn't work to switch over to the native
> > backlight control using a command line switch.  This way they can at least
> > *see* if the native backlight control works for them and (hopefully) report
> > problems if that's not the case.  This gives us a chance to get more
> > information about what the problem is and possibly to make some progress
> > without making changes for everyone, reverting those changes when they don't
> > work etc.
> > 
> > An alternative for them is to pass acpi_osi="!Windows 2012" which will probably
> > make the ACPI backlight work for them again, but this rather is a step back,
> > because we can't possibly learn anything new from that.
> 
> If Win8 is as broken as we are I'm ok with the module option. It just
> sounded to me like right now we don't know of a way to make all machines
> somewhat happy, combined with the other pile of random backlight issues
> the assumption that we do something (maybe something a bit racy) that
> windows doesn't do isn't too far-fetched. So I'm not wary of the machines
> where the aml is busted for acpi_os=win8, but for the others where this
> broke stuff.
> 
> Or do I miss something here?

The ACPI video driver doesn't do anything new now.  It only does things that
did work before we started to tell BIOSes we're compatible with Win8.  The
reason why they don't work on some machines now is not related to whether or
not Win8 is broken, but to what is there in the ACPI tables on those machines.
That *is* pure garbage, but Win8 users don't see that (presumably, because
Win8 does't execute the AML in question).  We don't see that either with
acpi_osi="!Windows 2012" (because then we don't execute that AML either).

Whether or not Win8 is broken doesn't matter here.  What matters is that we
have to deal with broken AML somehow.

One way may be to tell everyone affected by this to pass
acpi_osi="!Windows 2012" in the kernel command line or possibly create a
blacklist of those machines in the kernel (which Felipe has been pushing for
recently) and wash our hands clean of this, but that leaves some exceptionally
bad taste in my mouth.

The alternative is to try to use native backlight in i915, which we did, but
that didn't work on some machines.  I don't think we will know why it didn't
work there before we collect some more information and that's not possible
without user testing.  So, the idea is to make that testing possible without
hacking the kernel and that's why we're introducing the new command line
switch.  And we're going to ask users to try it and report back.

Thanks,
Rafael

Aaron Lu Sept. 10, 2013, 3:27 a.m. UTC | #7

On 09/09/2013 07:44 PM, Igor Gnatenko wrote:
> On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote:
>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>> index f466980..75fba17 100644
>> --- a/drivers/gpu/drm/i915/i915_dma.c
>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>> @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>  	if (INTEL_INFO(dev)->num_pipes) {
>>  		/* Must be done after probing outputs */
>>  		intel_opregion_init(dev);
>> -		acpi_video_register();
>> +		__acpi_video_register(i915_take_over_backlight);
>>  	}
>>  
>>  	if (IS_GEN5(dev))
> 
> I can't compile:
> 
> 
> DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load':
> DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit
> declaration of function
> '__acpi_video_register' [-Werror=implicit-function-declaration]
> DEBUG:    __acpi_video_register(i915_take_over_backlight);
> DEBUG:    ^
> DEBUG: cc1: some warnings being treated as errors
> DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1
> DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2
> DEBUG: make[2]: *** [drivers/gpu/drm] Error 2
> DEBUG: make[1]: *** [drivers/gpu] Error 2
> DEBUG: make: *** [drivers] Error 2
> 

The two patches are based on top of Rafael's linux-next tree. I just
tried it again, no compile problem for me. I also tried on today Linus'
master tree, as there are some updates from i915, two conflicts exist.
I've just resolved them and will update it in next revision.
If you want to try it now, please use:
https://github.com/aaronlu/linux acpi_video_rework

Thanks,
Aaron

Igor Gnatenko Sept. 10, 2013, 5:13 a.m. UTC | #8

On Tue, 2013-09-10 at 11:27 +0800, Aaron Lu wrote:
> On 09/09/2013 07:44 PM, Igor Gnatenko wrote:
> > On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote:
> >> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> >> index f466980..75fba17 100644
> >> --- a/drivers/gpu/drm/i915/i915_dma.c
> >> +++ b/drivers/gpu/drm/i915/i915_dma.c
> >> @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >>  	if (INTEL_INFO(dev)->num_pipes) {
> >>  		/* Must be done after probing outputs */
> >>  		intel_opregion_init(dev);
> >> -		acpi_video_register();
> >> +		__acpi_video_register(i915_take_over_backlight);
> >>  	}
> >>  
> >>  	if (IS_GEN5(dev))
> > 
> > I can't compile:
> > 
> > 
> > DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load':
> > DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit
> > declaration of function
> > '__acpi_video_register' [-Werror=implicit-function-declaration]
> > DEBUG:    __acpi_video_register(i915_take_over_backlight);
> > DEBUG:    ^
> > DEBUG: cc1: some warnings being treated as errors
> > DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1
> > DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2
> > DEBUG: make[2]: *** [drivers/gpu/drm] Error 2
> > DEBUG: make[1]: *** [drivers/gpu] Error 2
> > DEBUG: make: *** [drivers] Error 2
> > 
> 
> The two patches are based on top of Rafael's linux-next tree. I just
> tried it again, no compile problem for me. I also tried on today Linus'
> master tree, as there are some updates from i915, two conflicts exist.
> I've just resolved them and will update it in next revision.
> If you want to try it now, please use:
> https://github.com/aaronlu/linux acpi_video_rework
> 
> Thanks,
> Aaron

Thanks. this patch fixes my problems w/ compilation. I've tested this
two patches and after apply I have:
$ tree /sys/class/backlight/
/sys/class/backlight/
|-- acpi_video0
-> ../../devices/pci0000:00/0000:00:02.0/backlight/acpi_video0
`-- intel_backlight
-> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight

2 directories, 0 files

I think it's didn't unregistered.. I may forget. I need to apply one of
patch from Matthew ?

Some strings from logs:
DMI: LENOVO 23205NG/23205NG, BIOS G2ET92WW (2.52 ) 02/22/2013
thinkpad_acpi: Standard ACPI backlight interface available, not loading
native one

Aaron Lu Sept. 10, 2013, 5:16 a.m. UTC | #9

On 09/10/2013 01:13 PM, Igor Gnatenko wrote:
> On Tue, 2013-09-10 at 11:27 +0800, Aaron Lu wrote:
>> On 09/09/2013 07:44 PM, Igor Gnatenko wrote:
>>> On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote:
>>>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>>>> index f466980..75fba17 100644
>>>> --- a/drivers/gpu/drm/i915/i915_dma.c
>>>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>>>> @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>>>  	if (INTEL_INFO(dev)->num_pipes) {
>>>>  		/* Must be done after probing outputs */
>>>>  		intel_opregion_init(dev);
>>>> -		acpi_video_register();
>>>> +		__acpi_video_register(i915_take_over_backlight);
>>>>  	}
>>>>  
>>>>  	if (IS_GEN5(dev))
>>>
>>> I can't compile:
>>>
>>>
>>> DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load':
>>> DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit
>>> declaration of function
>>> '__acpi_video_register' [-Werror=implicit-function-declaration]
>>> DEBUG:    __acpi_video_register(i915_take_over_backlight);
>>> DEBUG:    ^
>>> DEBUG: cc1: some warnings being treated as errors
>>> DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1
>>> DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2
>>> DEBUG: make[2]: *** [drivers/gpu/drm] Error 2
>>> DEBUG: make[1]: *** [drivers/gpu] Error 2
>>> DEBUG: make: *** [drivers] Error 2
>>>
>>
>> The two patches are based on top of Rafael's linux-next tree. I just
>> tried it again, no compile problem for me. I also tried on today Linus'
>> master tree, as there are some updates from i915, two conflicts exist.
>> I've just resolved them and will update it in next revision.
>> If you want to try it now, please use:
>> https://github.com/aaronlu/linux acpi_video_rework
>>
>> Thanks,
>> Aaron
> 
> Thanks. this patch fixes my problems w/ compilation. I've tested this
> two patches and after apply I have:
> $ tree /sys/class/backlight/
> /sys/class/backlight/
> |-- acpi_video0
> -> ../../devices/pci0000:00/0000:00:02.0/backlight/acpi_video0
> `-- intel_backlight
> -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight
> 
> 2 directories, 0 files
> 
> I think it's didn't unregistered.. I may forget. I need to apply one of
> patch from Matthew ?

You need to specify i915.take_over_backlight=1 in kernel cmdline, that
module option is set to false by default for now.

Thanks for the test.

-Aaron

> 
> Some strings from logs:
> DMI: LENOVO 23205NG/23205NG, BIOS G2ET92WW (2.52 ) 02/22/2013
> thinkpad_acpi: Standard ACPI backlight interface available, not loading
> native one
>

Igor Gnatenko Sept. 10, 2013, 5:22 a.m. UTC | #10

On Tue, 2013-09-10 at 13:16 +0800, Aaron Lu wrote:
> On 09/10/2013 01:13 PM, Igor Gnatenko wrote:
> > On Tue, 2013-09-10 at 11:27 +0800, Aaron Lu wrote:
> >> On 09/09/2013 07:44 PM, Igor Gnatenko wrote:
> >>> On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote:
> >>>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> >>>> index f466980..75fba17 100644
> >>>> --- a/drivers/gpu/drm/i915/i915_dma.c
> >>>> +++ b/drivers/gpu/drm/i915/i915_dma.c
> >>>> @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >>>>  	if (INTEL_INFO(dev)->num_pipes) {
> >>>>  		/* Must be done after probing outputs */
> >>>>  		intel_opregion_init(dev);
> >>>> -		acpi_video_register();
> >>>> +		__acpi_video_register(i915_take_over_backlight);
> >>>>  	}
> >>>>  
> >>>>  	if (IS_GEN5(dev))
> >>>
> >>> I can't compile:
> >>>
> >>>
> >>> DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load':
> >>> DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit
> >>> declaration of function
> >>> '__acpi_video_register' [-Werror=implicit-function-declaration]
> >>> DEBUG:    __acpi_video_register(i915_take_over_backlight);
> >>> DEBUG:    ^
> >>> DEBUG: cc1: some warnings being treated as errors
> >>> DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1
> >>> DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2
> >>> DEBUG: make[2]: *** [drivers/gpu/drm] Error 2
> >>> DEBUG: make[1]: *** [drivers/gpu] Error 2
> >>> DEBUG: make: *** [drivers] Error 2
> >>>
> >>
> >> The two patches are based on top of Rafael's linux-next tree. I just
> >> tried it again, no compile problem for me. I also tried on today Linus'
> >> master tree, as there are some updates from i915, two conflicts exist.
> >> I've just resolved them and will update it in next revision.
> >> If you want to try it now, please use:
> >> https://github.com/aaronlu/linux acpi_video_rework
> >>
> >> Thanks,
> >> Aaron
> > 
> > Thanks. this patch fixes my problems w/ compilation. I've tested this
> > two patches and after apply I have:
> > $ tree /sys/class/backlight/
> > /sys/class/backlight/
> > |-- acpi_video0
> > -> ../../devices/pci0000:00/0000:00:02.0/backlight/acpi_video0
> > `-- intel_backlight
> > -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight
> > 
> > 2 directories, 0 files
> > 
> > I think it's didn't unregistered.. I may forget. I need to apply one of
> > patch from Matthew ?
> 
> You need to specify i915.take_over_backlight=1 in kernel cmdline, that
> module option is set to false by default for now.
> 
> Thanks for the test.
> 
> -Aaron
> 
> > 
> > Some strings from logs:
> > DMI: LENOVO 23205NG/23205NG, BIOS G2ET92WW (2.52 ) 02/22/2013
> > thinkpad_acpi: Standard ACPI backlight interface available, not loading
> > native one
> > 
> 

Thanks for quick answer. Yes. This option do unregister. Thanks. but for
this patch-set I also need "[PATCH 2/3] ACPI / video: Always call
acpi_video_init_brightness() on init" from Matthew (for notifications in
DE).

Igor Gnatenko Sept. 10, 2013, 5:23 a.m. UTC | #11

On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote:
> According to Matthew Garrett, "Windows 8 leaves backlight control up
> to individual graphics drivers rather than making ACPI calls itself.
> There's plenty of evidence to suggest that the Intel driver for
> Windows [8] doesn't use the ACPI interface, including the fact that
> it's broken on a bunch of machines when the OS claims to support
> Windows 8.  The simplest thing to do appears to be to disable the
> ACPI backlight interface on these systems".
> 
> There's a problem with that approach, however, because simply
> avoiding to register the ACPI backlight interface if the firmware
> calls _OSI for Windows 8 may not work in the following situations:
>  (1) The ACPI backlight interface actually works on the given system
>      and the i915 driver is not loaded (e.g. another graphics driver
>      is used).
>  (2) The ACPI backlight interface doesn't work on the given system,
>      but there is a vendor platform driver that will register its
>      own, equally broken, backlight interface if not prevented from
>      doing so by the ACPI subsystem.
> Therefore we need to allow the ACPI backlight interface to be
> registered until the i915 driver is loaded which then will unregister
> it if the firmware has called _OSI for Windows 8 (or will register
> the ACPI video driver without backlight support if not already
> present).
> 
> For this reason, introduce an alternative function for registering
> ACPI video, __acpi_video_register(bool), that if ture is passed,
> will check whether or not the ACPI video driver has already been
> registered and whether or not the backlight Windows 8 quirk has to
> be applied. If the quirk has to be applied, it will block the ACPI
> backlight support and either unregister the backlight interface if
> the ACPI video driver has already been registered, or register the
> ACPI video driver without the backlight interface otherwise.  Make
> the i915 driver use __acpi_video_register() instead of
> acpi_video_register() in i915_driver_load(), and the param passed
> there is controlled by the i915 module level parameter
> i915_take_over_backlight, which is set to false by default.
> 
> This change is evolved from earlier patches of Matthew Garrett,
> Chun-Yi Lee and Seth Forshee and is heavily based on two patches
> from Rafael:
> https://lkml.org/lkml/2013/7/17/720
> https://lkml.org/lkml/2013/7/24/806
> 
> Signed-off-by: Aaron Lu <[e-mail]>
Tested-by: Igor Gnatenko <[e-mail]>
> ---
>  drivers/acpi/internal.h         |  2 ++
>  drivers/acpi/video.c            | 24 ++++++++++++++++--------
>  drivers/acpi/video_detect.c     | 15 ++++++++++++++-
>  drivers/gpu/drm/i915/i915_dma.c |  2 +-
>  drivers/gpu/drm/i915/i915_drv.c |  5 +++++
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  include/acpi/video.h            |  9 +++++++--
>  include/linux/acpi.h            |  1 +
>  8 files changed, 47 insertions(+), 12 deletions(-)

Aaron Lu Sept. 10, 2013, 5:42 a.m. UTC | #12

On 09/10/2013 01:22 PM, Igor Gnatenko wrote:
> On Tue, 2013-09-10 at 13:16 +0800, Aaron Lu wrote:
>> On 09/10/2013 01:13 PM, Igor Gnatenko wrote:
>>> On Tue, 2013-09-10 at 11:27 +0800, Aaron Lu wrote:
>>>> On 09/09/2013 07:44 PM, Igor Gnatenko wrote:
>>>>> On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote:
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>>>>>> index f466980..75fba17 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_dma.c
>>>>>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>>>>>> @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>>>>>  	if (INTEL_INFO(dev)->num_pipes) {
>>>>>>  		/* Must be done after probing outputs */
>>>>>>  		intel_opregion_init(dev);
>>>>>> -		acpi_video_register();
>>>>>> +		__acpi_video_register(i915_take_over_backlight);
>>>>>>  	}
>>>>>>  
>>>>>>  	if (IS_GEN5(dev))
>>>>>
>>>>> I can't compile:
>>>>>
>>>>>
>>>>> DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load':
>>>>> DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit
>>>>> declaration of function
>>>>> '__acpi_video_register' [-Werror=implicit-function-declaration]
>>>>> DEBUG:    __acpi_video_register(i915_take_over_backlight);
>>>>> DEBUG:    ^
>>>>> DEBUG: cc1: some warnings being treated as errors
>>>>> DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1
>>>>> DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2
>>>>> DEBUG: make[2]: *** [drivers/gpu/drm] Error 2
>>>>> DEBUG: make[1]: *** [drivers/gpu] Error 2
>>>>> DEBUG: make: *** [drivers] Error 2
>>>>>
>>>>
>>>> The two patches are based on top of Rafael's linux-next tree. I just
>>>> tried it again, no compile problem for me. I also tried on today Linus'
>>>> master tree, as there are some updates from i915, two conflicts exist.
>>>> I've just resolved them and will update it in next revision.
>>>> If you want to try it now, please use:
>>>> https://github.com/aaronlu/linux acpi_video_rework
>>>>
>>>> Thanks,
>>>> Aaron
>>>
>>> Thanks. this patch fixes my problems w/ compilation. I've tested this
>>> two patches and after apply I have:
>>> $ tree /sys/class/backlight/
>>> /sys/class/backlight/
>>> |-- acpi_video0
>>> -> ../../devices/pci0000:00/0000:00:02.0/backlight/acpi_video0
>>> `-- intel_backlight
>>> -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight
>>>
>>> 2 directories, 0 files
>>>
>>> I think it's didn't unregistered.. I may forget. I need to apply one of
>>> patch from Matthew ?
>>
>> You need to specify i915.take_over_backlight=1 in kernel cmdline, that
>> module option is set to false by default for now.
>>
>> Thanks for the test.
>>
>> -Aaron
>>
>>>
>>> Some strings from logs:
>>> DMI: LENOVO 23205NG/23205NG, BIOS G2ET92WW (2.52 ) 02/22/2013
>>> thinkpad_acpi: Standard ACPI backlight interface available, not loading
>>> native one
>>>
>>
> 
> Thanks for quick answer. Yes. This option do unregister. Thanks. but for
> this patch-set I also need "[PATCH 2/3] ACPI / video: Always call
> acpi_video_init_brightness() on init" from Matthew (for notifications in
> DE).

That patch is reverted as it cause problem for other system:
https://bugs.freedesktop.org/show_bug.cgi?id=68355

OTOH, the thinkpad-acpi module already has a call to _BCL except that
the tpacpi_acpi_handle_locate failed to locate video controller's handle:
https://bugzilla.kernel.org/show_bug.cgi?id=51231#c121
I'll see if I can figure out why.

Thanks,
Aaron

Aaron Lu Sept. 10, 2013, 6:30 a.m. UTC | #13

On Mon, Sep 09, 2013 at 11:32:10AM +0200, Daniel Vetter wrote:
> Hi Aaaron,
> 
> Have we grown any clue meanwhile about which Intel boxes need this and for
> which we still need to keep the acpi backlight around? I've grown _very_

Sorry, no general rule has been found. As Rafael has said, it appears
to be random... And in addition to the "shipped with Win7 with a Win8
upgrade option" case, I also find a Sony laptop that has Win8
pre-installed and a broken ACPI video backlight interface. Its ACPI
video backlight control method is simply a stub, so even the
acpi_osi="!Windows 2012" will not provide a working backlight for this
system.

-Aaron

> reluctant to just adding tons of quirks to our driver for the backlight.
> Almost all the quirks we have added recently (or that have been proposed
> to be added) are for the backlight. Imo that indicates we get something
> fundamentally wrong ...
> 
> Cheers, Daniel
> 
> On Mon, Sep 09, 2013 at 04:42:20PM +0800, Aaron Lu wrote:
> > According to Matthew Garrett, "Windows 8 leaves backlight control up
> > to individual graphics drivers rather than making ACPI calls itself.
> > There's plenty of evidence to suggest that the Intel driver for
> > Windows [8] doesn't use the ACPI interface, including the fact that
> > it's broken on a bunch of machines when the OS claims to support
> > Windows 8.  The simplest thing to do appears to be to disable the
> > ACPI backlight interface on these systems".
> > 
> > There's a problem with that approach, however, because simply
> > avoiding to register the ACPI backlight interface if the firmware
> > calls _OSI for Windows 8 may not work in the following situations:
> >  (1) The ACPI backlight interface actually works on the given system
> >      and the i915 driver is not loaded (e.g. another graphics driver
> >      is used).
> >  (2) The ACPI backlight interface doesn't work on the given system,
> >      but there is a vendor platform driver that will register its
> >      own, equally broken, backlight interface if not prevented from
> >      doing so by the ACPI subsystem.
> > Therefore we need to allow the ACPI backlight interface to be
> > registered until the i915 driver is loaded which then will unregister
> > it if the firmware has called _OSI for Windows 8 (or will register
> > the ACPI video driver without backlight support if not already
> > present).
> > 
> > For this reason, introduce an alternative function for registering
> > ACPI video, __acpi_video_register(bool), that if ture is passed,
> > will check whether or not the ACPI video driver has already been
> > registered and whether or not the backlight Windows 8 quirk has to
> > be applied. If the quirk has to be applied, it will block the ACPI
> > backlight support and either unregister the backlight interface if
> > the ACPI video driver has already been registered, or register the
> > ACPI video driver without the backlight interface otherwise.  Make
> > the i915 driver use __acpi_video_register() instead of
> > acpi_video_register() in i915_driver_load(), and the param passed
> > there is controlled by the i915 module level parameter
> > i915_take_over_backlight, which is set to false by default.
> > 
> > This change is evolved from earlier patches of Matthew Garrett,
> > Chun-Yi Lee and Seth Forshee and is heavily based on two patches
> > from Rafael:
> > https://lkml.org/lkml/2013/7/17/720
> > https://lkml.org/lkml/2013/7/24/806
> > 
> > Signed-off-by: Aaron Lu <[e-mail]>
> > ---
> >  drivers/acpi/internal.h         |  2 ++
> >  drivers/acpi/video.c            | 24 ++++++++++++++++--------
> >  drivers/acpi/video_detect.c     | 15 ++++++++++++++-
> >  drivers/gpu/drm/i915/i915_dma.c |  2 +-
> >  drivers/gpu/drm/i915/i915_drv.c |  5 +++++
> >  drivers/gpu/drm/i915/i915_drv.h |  1 +
> >  include/acpi/video.h            |  9 +++++++--
> >  include/linux/acpi.h            |  1 +
> >  8 files changed, 47 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> > index 20f4233..a53832e 100644
> > --- a/drivers/acpi/internal.h
> > +++ b/drivers/acpi/internal.h
> > @@ -170,8 +170,10 @@ int acpi_create_platform_device(struct acpi_device *adev,
> >    -------------------------------------------------------------------------- */
> >  #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
> >  bool acpi_video_backlight_quirks(void);
> > +bool acpi_video_verify_backlight_support(void);
> >  #else
> >  static inline bool acpi_video_backlight_quirks(void) { return false; }
> > +static inline bool acpi_video_verify_backlight_support(void) { return false; }
> >  #endif
> >  
> >  #endif /* _ACPI_INTERNAL_H_ */
> > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> > index 5ad5a71..cc709a7 100644
> > --- a/drivers/acpi/video.c
> > +++ b/drivers/acpi/video.c
> > @@ -1256,8 +1256,8 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event)
> >  	unsigned long long level_current, level_next;
> >  	int result = -EINVAL;
> >  
> > -	/* no warning message if acpi_backlight=vendor is used */
> > -	if (!acpi_video_backlight_support())
> > +	/* no warning message if acpi_backlight=vendor or a quirk is used */
> > +	if (!acpi_video_verify_backlight_support())
> >  		return 0;
> >  
> >  	if (!device->brightness)
> > @@ -1558,7 +1558,7 @@ acpi_video_bus_match(acpi_handle handle, u32 level, void *context,
> >  
> >  static void acpi_video_dev_register_backlight(struct acpi_video_device *device)
> >  {
> > -	if (acpi_video_backlight_support()) {
> > +	if (acpi_video_verify_backlight_support()) {
> >  		struct backlight_properties props;
> >  		struct pci_dev *pdev;
> >  		acpi_handle acpi_parent;
> > @@ -1933,14 +1933,22 @@ static int __init intel_opregion_present(void)
> >  	return opregion;
> >  }
> >  
> > -int acpi_video_register(void)
> > +int __acpi_video_register(bool backlight_quirks)
> >  {
> > -	int result = 0;
> > +	bool no_backlight;
> > +	int result;
> > +
> > +	no_backlight = backlight_quirks ? acpi_video_backlight_quirks() : false;
> > +
> >  	if (register_count) {
> >  		/*
> > -		 * if the function of acpi_video_register is already called,
> > -		 * don't register the acpi_vide_bus again and return no error.
> > +		 * If acpi_video_register() has been called already, don't try
> > +		 * to register acpi_video_bus, but unregister backlight devices
> > +		 * if no backlight support is requested.
> >  		 */
> > +		if (no_backlight)
> > +			acpi_video_unregister_backlight();
> > +
> >  		return 0;
> >  	}
> >  
> > @@ -1959,7 +1967,7 @@ int acpi_video_register(void)
> >  
> >  	return 0;
> >  }
> > -EXPORT_SYMBOL(acpi_video_register);
> > +EXPORT_SYMBOL(__acpi_video_register);
> >  
> >  void acpi_video_unregister(void)
> >  {
> > diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
> > index 940edbf..c7e43ec 100644
> > --- a/drivers/acpi/video_detect.c
> > +++ b/drivers/acpi/video_detect.c
> > @@ -235,7 +235,12 @@ static void acpi_video_caps_check(void)
> >  
> >  bool acpi_video_backlight_quirks(void)
> >  {
> > -	return acpi_gbl_osi_data >= ACPI_OSI_WIN_8;
> > +	if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8) {
> > +		acpi_video_caps_check();
> > +		acpi_video_support |= ACPI_VIDEO_SKIP_BACKLIGHT;
> > +		return true;
> > +	}
> > +	return false;
> >  }
> >  EXPORT_SYMBOL(acpi_video_backlight_quirks);
> >  
> > @@ -283,6 +288,14 @@ int acpi_video_backlight_support(void)
> >  }
> >  EXPORT_SYMBOL(acpi_video_backlight_support);
> >  
> > +/* For the ACPI video driver use only. */
> > +bool acpi_video_verify_backlight_support(void)
> > +{
> > +	return (acpi_video_support & ACPI_VIDEO_SKIP_BACKLIGHT) ?
> > +		false : acpi_video_backlight_support();
> > +}
> > +EXPORT_SYMBOL(acpi_video_verify_backlight_support);
> > +
> >  /*
> >   * Use acpi_backlight=vendor/video to force that backlight switching
> >   * is processed by vendor specific acpi drivers or video.ko driver.
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index f466980..75fba17 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  	if (INTEL_INFO(dev)->num_pipes) {
> >  		/* Must be done after probing outputs */
> >  		intel_opregion_init(dev);
> > -		acpi_video_register();
> > +		__acpi_video_register(i915_take_over_backlight);
> >  	}
> >  
> >  	if (IS_GEN5(dev))
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 45b3c03..b014398 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -132,6 +132,11 @@ int i915_enable_ips __read_mostly = 1;
> >  module_param_named(enable_ips, i915_enable_ips, int, 0600);
> >  MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
> >  
> > +bool i915_take_over_backlight = false;
> > +module_param_named(take_over_backlight, i915_take_over_backlight, bool, 0644);
> > +MODULE_PARM_DESC(take_over_backlight,
> > +		"Prevent ACPI backlight from being used (default: false)");
> > +
> >  static struct drm_driver driver;
> >  extern int intel_agp_enabled;
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 1929bff..b43c430 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1543,6 +1543,7 @@ extern int i915_enable_ppgtt __read_mostly;
> >  extern unsigned int i915_preliminary_hw_support __read_mostly;
> >  extern int i915_disable_power_well __read_mostly;
> >  extern int i915_enable_ips __read_mostly;
> > +extern bool i915_take_over_backlight __read_mostly;
> >  
> >  extern int i915_suspend(struct drm_device *dev, pm_message_t state);
> >  extern int i915_resume(struct drm_device *dev);
> > diff --git a/include/acpi/video.h b/include/acpi/video.h
> > index 0c665b5..dc5b8ad 100644
> > --- a/include/acpi/video.h
> > +++ b/include/acpi/video.h
> > @@ -17,13 +17,13 @@ struct acpi_device;
> >  #define ACPI_VIDEO_DISPLAY_LEGACY_TV      0x0200
> >  
> >  #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
> > -extern int acpi_video_register(void);
> > +extern int __acpi_video_register(bool backlight_quirks);
> >  extern void acpi_video_unregister(void);
> >  extern int acpi_video_unregister_backlight(void);
> >  extern int acpi_video_get_edid(struct acpi_device *device, int type,
> >  			       int device_id, void **edid);
> >  #else
> > -static inline int acpi_video_register(void) { return 0; }
> > +static inline int __acpi_video_register(bool backlight_quirks) { return 0; }
> >  static inline void acpi_video_unregister(void) { return; }
> >  static inline int acpi_video_unregister_backlight(void) { return; }
> >  static inline int acpi_video_get_edid(struct acpi_device *device, int type,
> > @@ -33,4 +33,9 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type,
> >  }
> >  #endif
> >  
> > +static inline int acpi_video_register(void)
> > +{
> > +	return __acpi_video_register(false);
> > +}
> > +
> >  #endif
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index a5db4ae..a0dcce5 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -191,6 +191,7 @@ extern bool wmi_has_guid(const char *guid);
> >  #define ACPI_VIDEO_BACKLIGHT_DMI_VIDEO			0x0200
> >  #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VENDOR		0x0400
> >  #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VIDEO		0x0800
> > +#define ACPI_VIDEO_SKIP_BACKLIGHT			0x1000
> >  
> >  #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
> >  
> > -- 
> > 1.8.4.12.g2ea3df6
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> [phone] - http://blog.ffwll.ch
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jani Nikula Sept. 10, 2013, 1:53 p.m. UTC | #14

On Mon, 09 Sep 2013, "Rafael J. Wysocki" <[e-mail]> wrote:
> On Monday, September 09, 2013 05:21:18 PM Daniel Vetter wrote:
>> On Mon, Sep 09, 2013 at 02:16:12PM +0200, Rafael J. Wysocki wrote:
>> > Hi,
>> > 
>> > On Monday, September 09, 2013 11:32:10 AM Daniel Vetter wrote:
>> > > Hi Aaaron,
>> > > 
>> > > Have we grown any clue meanwhile about which Intel boxes need this and for
>> > > which we still need to keep the acpi backlight around?
>> > 
>> > First of all, there is a bunch of boxes where ACPI backlight works incorrectly
>> > because of the Win8 compatibility issue.  [In short, if we say we are compatible
>> > with Win8, the backlight AML goes into a special code path that is broken on
>> > those machines. Presumably Win8 uses native backlight control on them and that
>> > AML code path is never executed there.]  The collection of machines with this
>> > problem appears to be kind of random (various models from various vendors), but
>> > I *think* they are machines that originally shipped with Win7 with a Win8
>> > "upgrade" option (which in practice requires the BIOS to be updated anyway).
>> > 
>> > Because of that, last time we tried to switch all of the systems using i915
>> > and telling the BIOS that they are compatible with Win8 over to native backlight
>> > control, but that didn't work for two reasons.  The first reason is that some
>> > user space doesn't know how to use intel_backlight and needs to be taught about
>> > that (so some systems are just not ready for that switch).  The second and more
>> > fundamental reason is that the native backlight control simply doesn't work on
>> > some machines and we don't seem to have any idea why and how to debug this
>> > particular problem (mostly because we don't have enough information and we
>> > don't know what to ask for).
>> > 
>> > > I've grown _very_ reluctant to just adding tons of quirks to our driver for
>> > > the backlight.
>> > >
>> > > Almost all the quirks we have added recently (or that have been proposed
>> > > to be added) are for the backlight. Imo that indicates we get something
>> > > fundamentally wrong ...
>> > 
>> > This thing isn't really a quirk.  It rather is an option for the users of
>> > the systems where ACPI backlight doesn't work to switch over to the native
>> > backlight control using a command line switch.  This way they can at least
>> > *see* if the native backlight control works for them and (hopefully) report
>> > problems if that's not the case.  This gives us a chance to get more
>> > information about what the problem is and possibly to make some progress
>> > without making changes for everyone, reverting those changes when they don't
>> > work etc.
>> > 
>> > An alternative for them is to pass acpi_osi="!Windows 2012" which will probably
>> > make the ACPI backlight work for them again, but this rather is a step back,
>> > because we can't possibly learn anything new from that.
>> 
>> If Win8 is as broken as we are I'm ok with the module option. It just
>> sounded to me like right now we don't know of a way to make all machines
>> somewhat happy, combined with the other pile of random backlight issues
>> the assumption that we do something (maybe something a bit racy) that
>> windows doesn't do isn't too far-fetched. So I'm not wary of the machines
>> where the aml is busted for acpi_os=win8, but for the others where this
>> broke stuff.
>> 
>> Or do I miss something here?
>
> The ACPI video driver doesn't do anything new now.  It only does things that
> did work before we started to tell BIOSes we're compatible with Win8.  The
> reason why they don't work on some machines now is not related to whether or
> not Win8 is broken, but to what is there in the ACPI tables on those machines.
> That *is* pure garbage, but Win8 users don't see that (presumably, because
> Win8 does't execute the AML in question).  We don't see that either with
> acpi_osi="!Windows 2012" (because then we don't execute that AML either).
>
> Whether or not Win8 is broken doesn't matter here.  What matters is that we
> have to deal with broken AML somehow.
>
> One way may be to tell everyone affected by this to pass
> acpi_osi="!Windows 2012" in the kernel command line or possibly create a
> blacklist of those machines in the kernel (which Felipe has been pushing for
> recently) and wash our hands clean of this, but that leaves some exceptionally
> bad taste in my mouth.
>
> The alternative is to try to use native backlight in i915, which we did, but
> that didn't work on some machines.  I don't think we will know why it didn't
> work there before we collect some more information and that's not possible
> without user testing.  So, the idea is to make that testing possible without
> hacking the kernel and that's why we're introducing the new command line
> switch.  And we're going to ask users to try it and report back.

The thing that slightly bugs me with the proposed patches is that
they're adding a module parameter to i915 to tell ACPI video driver
whether to quirk the backlight or not. Before you know, we *will* have
requests to add quirks to i915 to tell ACPI video driver this.

I think the parameter "Does the ACPI backlight interface work or not"
belongs to the ACPI video driver.

Feel free to file this in your bikeshedding folder, but I think i915
should only tell ACPI "I have a native backlight interface". What ACPI
video driver does with that information is its business. The desired
thing to do here would be to check if there's a module parameter or a
quirk to disable the ACPI backlight interface. acpi_backlight=DTRT? ;)

BR,
Jani.

Matthew Garrett Sept. 10, 2013, 1:56 p.m. UTC | #15

On Tue, 2013-09-10 at 16:53 +0300, Jani Nikula wrote:

> I think the parameter "Does the ACPI backlight interface work or not"

> belongs to the ACPI video driver.


That depends on how Windows 8 works. If Windows 8 policy is handled by
the GPU drivers then it belongs in i915. If it's handled by the ACPI
code then it belongs in the ACPI code. But I have no way of determining
that, whereas you work for a company that produces a Windows 8 video
driver…

-- 
Matthew Garrett <[e-mail]>

Matthew Garrett Sept. 10, 2013, 2:21 p.m. UTC | #16

On Tue, 2013-09-10 at 17:21 +0300, Jani Nikula wrote:
> On Tue, 10 Sep 2013, Matthew Garrett <[e-mail]> wrote:
> > On Tue, 2013-09-10 at 16:53 +0300, Jani Nikula wrote:
> >
> >> I think the parameter "Does the ACPI backlight interface work or not"
> >> belongs to the ACPI video driver.
> >
> > That depends on how Windows 8 works. If Windows 8 policy is handled by
> > the GPU drivers then it belongs in i915. If it's handled by the ACPI
> > code then it belongs in the ACPI code.
> 
> I fail to see the logic. Windows 8 policy dictates whether we can use
> the AML code or not. IMHO, ACPI code is in the best position to figure
> this out and quirk as necessary. It's the part that knows about Windows
> 8, not i915.

So if nvidia hardware uses the ACPI interface and Intel doesn't, we
should still quirk it in the ACPI driver?

Jani Nikula Sept. 10, 2013, 2:21 p.m. UTC | #17

On Tue, 10 Sep 2013, Matthew Garrett <[e-mail]> wrote:
> On Tue, 2013-09-10 at 16:53 +0300, Jani Nikula wrote:
>
>> I think the parameter "Does the ACPI backlight interface work or not"
>> belongs to the ACPI video driver.
>
> That depends on how Windows 8 works. If Windows 8 policy is handled by
> the GPU drivers then it belongs in i915. If it's handled by the ACPI
> code then it belongs in the ACPI code.

I fail to see the logic. Windows 8 policy dictates whether we can use
the AML code or not. IMHO, ACPI code is in the best position to figure
this out and quirk as necessary. It's the part that knows about Windows
8, not i915.

> But I have no way of determining that, whereas you work for a company
> that produces a Windows 8 video driver…

Here I do see the logic. However see signature; I'm not in the best of
positions to figure out things about Windows 8 video drivers. ;) But
I'll see what I can do.

BR,
Jani.

Rafael Wysocki Sept. 10, 2013, 7:23 p.m. UTC | #18

On Tuesday, September 10, 2013 04:53:40 PM Jani Nikula wrote:
> On Mon, 09 Sep 2013, "Rafael J. Wysocki" <[e-mail]> wrote:
> > On Monday, September 09, 2013 05:21:18 PM Daniel Vetter wrote:
> >> On Mon, Sep 09, 2013 at 02:16:12PM +0200, Rafael J. Wysocki wrote:
> >> > Hi,
> >> > 
> >> > On Monday, September 09, 2013 11:32:10 AM Daniel Vetter wrote:
> >> > > Hi Aaaron,
> >> > > 
> >> > > Have we grown any clue meanwhile about which Intel boxes need this and for
> >> > > which we still need to keep the acpi backlight around?
> >> > 
> >> > First of all, there is a bunch of boxes where ACPI backlight works incorrectly
> >> > because of the Win8 compatibility issue.  [In short, if we say we are compatible
> >> > with Win8, the backlight AML goes into a special code path that is broken on
> >> > those machines. Presumably Win8 uses native backlight control on them and that
> >> > AML code path is never executed there.]  The collection of machines with this
> >> > problem appears to be kind of random (various models from various vendors), but
> >> > I *think* they are machines that originally shipped with Win7 with a Win8
> >> > "upgrade" option (which in practice requires the BIOS to be updated anyway).
> >> > 
> >> > Because of that, last time we tried to switch all of the systems using i915
> >> > and telling the BIOS that they are compatible with Win8 over to native backlight
> >> > control, but that didn't work for two reasons.  The first reason is that some
> >> > user space doesn't know how to use intel_backlight and needs to be taught about
> >> > that (so some systems are just not ready for that switch).  The second and more
> >> > fundamental reason is that the native backlight control simply doesn't work on
> >> > some machines and we don't seem to have any idea why and how to debug this
> >> > particular problem (mostly because we don't have enough information and we
> >> > don't know what to ask for).
> >> > 
> >> > > I've grown _very_ reluctant to just adding tons of quirks to our driver for
> >> > > the backlight.
> >> > >
> >> > > Almost all the quirks we have added recently (or that have been proposed
> >> > > to be added) are for the backlight. Imo that indicates we get something
> >> > > fundamentally wrong ...
> >> > 
> >> > This thing isn't really a quirk.  It rather is an option for the users of
> >> > the systems where ACPI backlight doesn't work to switch over to the native
> >> > backlight control using a command line switch.  This way they can at least
> >> > *see* if the native backlight control works for them and (hopefully) report
> >> > problems if that's not the case.  This gives us a chance to get more
> >> > information about what the problem is and possibly to make some progress
> >> > without making changes for everyone, reverting those changes when they don't
> >> > work etc.
> >> > 
> >> > An alternative for them is to pass acpi_osi="!Windows 2012" which will probably
> >> > make the ACPI backlight work for them again, but this rather is a step back,
> >> > because we can't possibly learn anything new from that.
> >> 
> >> If Win8 is as broken as we are I'm ok with the module option. It just
> >> sounded to me like right now we don't know of a way to make all machines
> >> somewhat happy, combined with the other pile of random backlight issues
> >> the assumption that we do something (maybe something a bit racy) that
> >> windows doesn't do isn't too far-fetched. So I'm not wary of the machines
> >> where the aml is busted for acpi_os=win8, but for the others where this
> >> broke stuff.
> >> 
> >> Or do I miss something here?
> >
> > The ACPI video driver doesn't do anything new now.  It only does things that
> > did work before we started to tell BIOSes we're compatible with Win8.  The
> > reason why they don't work on some machines now is not related to whether or
> > not Win8 is broken, but to what is there in the ACPI tables on those machines.
> > That *is* pure garbage, but Win8 users don't see that (presumably, because
> > Win8 does't execute the AML in question).  We don't see that either with
> > acpi_osi="!Windows 2012" (because then we don't execute that AML either).
> >
> > Whether or not Win8 is broken doesn't matter here.  What matters is that we
> > have to deal with broken AML somehow.
> >
> > One way may be to tell everyone affected by this to pass
> > acpi_osi="!Windows 2012" in the kernel command line or possibly create a
> > blacklist of those machines in the kernel (which Felipe has been pushing for
> > recently) and wash our hands clean of this, but that leaves some exceptionally
> > bad taste in my mouth.
> >
> > The alternative is to try to use native backlight in i915, which we did, but
> > that didn't work on some machines.  I don't think we will know why it didn't
> > work there before we collect some more information and that's not possible
> > without user testing.  So, the idea is to make that testing possible without
> > hacking the kernel and that's why we're introducing the new command line
> > switch.  And we're going to ask users to try it and report back.
> 
> The thing that slightly bugs me with the proposed patches is that
> they're adding a module parameter to i915 to tell ACPI video driver
> whether to quirk the backlight or not. Before you know, we *will* have
> requests to add quirks to i915 to tell ACPI video driver this.
> 
> I think the parameter "Does the ACPI backlight interface work or not"
> belongs to the ACPI video driver.
>
> Feel free to file this in your bikeshedding folder, but I think i915
> should only tell ACPI "I have a native backlight interface".

It kind of does that already through the intel_opregion_init() thing,
so it would be trivial to test intel_opregion_present() in
acpi_video_register().

So yes, we can add a command line option, say 'use_native_backlight' to the
ACPI video driver that will work like this: "if the Intel opregion is present
and video.use_native_backlight is set, skip registering ACPI backlight".

Is that what you want?

Rafael

Aaron Lu Sept. 11, 2013, 1:32 a.m. UTC | #19

On Tue, Sep 10, 2013 at 09:23:04PM +0200, Rafael J. Wysocki wrote:
> On Tuesday, September 10, 2013 04:53:40 PM Jani Nikula wrote:
> > On Mon, 09 Sep 2013, "Rafael J. Wysocki" <[e-mail]> wrote:
> > > On Monday, September 09, 2013 05:21:18 PM Daniel Vetter wrote:
> > >> On Mon, Sep 09, 2013 at 02:16:12PM +0200, Rafael J. Wysocki wrote:
> > >> > Hi,
> > >> > 
> > >> > On Monday, September 09, 2013 11:32:10 AM Daniel Vetter wrote:
> > >> > > Hi Aaaron,
> > >> > > 
> > >> > > Have we grown any clue meanwhile about which Intel boxes need this and for
> > >> > > which we still need to keep the acpi backlight around?
> > >> > 
> > >> > First of all, there is a bunch of boxes where ACPI backlight works incorrectly
> > >> > because of the Win8 compatibility issue.  [In short, if we say we are compatible
> > >> > with Win8, the backlight AML goes into a special code path that is broken on
> > >> > those machines. Presumably Win8 uses native backlight control on them and that
> > >> > AML code path is never executed there.]  The collection of machines with this
> > >> > problem appears to be kind of random (various models from various vendors), but
> > >> > I *think* they are machines that originally shipped with Win7 with a Win8
> > >> > "upgrade" option (which in practice requires the BIOS to be updated anyway).
> > >> > 
> > >> > Because of that, last time we tried to switch all of the systems using i915
> > >> > and telling the BIOS that they are compatible with Win8 over to native backlight
> > >> > control, but that didn't work for two reasons.  The first reason is that some
> > >> > user space doesn't know how to use intel_backlight and needs to be taught about
> > >> > that (so some systems are just not ready for that switch).  The second and more
> > >> > fundamental reason is that the native backlight control simply doesn't work on
> > >> > some machines and we don't seem to have any idea why and how to debug this
> > >> > particular problem (mostly because we don't have enough information and we
> > >> > don't know what to ask for).
> > >> > 
> > >> > > I've grown _very_ reluctant to just adding tons of quirks to our driver for
> > >> > > the backlight.
> > >> > >
> > >> > > Almost all the quirks we have added recently (or that have been proposed
> > >> > > to be added) are for the backlight. Imo that indicates we get something
> > >> > > fundamentally wrong ...
> > >> > 
> > >> > This thing isn't really a quirk.  It rather is an option for the users of
> > >> > the systems where ACPI backlight doesn't work to switch over to the native
> > >> > backlight control using a command line switch.  This way they can at least
> > >> > *see* if the native backlight control works for them and (hopefully) report
> > >> > problems if that's not the case.  This gives us a chance to get more
> > >> > information about what the problem is and possibly to make some progress
> > >> > without making changes for everyone, reverting those changes when they don't
> > >> > work etc.
> > >> > 
> > >> > An alternative for them is to pass acpi_osi="!Windows 2012" which will probably
> > >> > make the ACPI backlight work for them again, but this rather is a step back,
> > >> > because we can't possibly learn anything new from that.
> > >> 
> > >> If Win8 is as broken as we are I'm ok with the module option. It just
> > >> sounded to me like right now we don't know of a way to make all machines
> > >> somewhat happy, combined with the other pile of random backlight issues
> > >> the assumption that we do something (maybe something a bit racy) that
> > >> windows doesn't do isn't too far-fetched. So I'm not wary of the machines
> > >> where the aml is busted for acpi_os=win8, but for the others where this
> > >> broke stuff.
> > >> 
> > >> Or do I miss something here?
> > >
> > > The ACPI video driver doesn't do anything new now.  It only does things that
> > > did work before we started to tell BIOSes we're compatible with Win8.  The
> > > reason why they don't work on some machines now is not related to whether or
> > > not Win8 is broken, but to what is there in the ACPI tables on those machines.
> > > That *is* pure garbage, but Win8 users don't see that (presumably, because
> > > Win8 does't execute the AML in question).  We don't see that either with
> > > acpi_osi="!Windows 2012" (because then we don't execute that AML either).
> > >
> > > Whether or not Win8 is broken doesn't matter here.  What matters is that we
> > > have to deal with broken AML somehow.
> > >
> > > One way may be to tell everyone affected by this to pass
> > > acpi_osi="!Windows 2012" in the kernel command line or possibly create a
> > > blacklist of those machines in the kernel (which Felipe has been pushing for
> > > recently) and wash our hands clean of this, but that leaves some exceptionally
> > > bad taste in my mouth.
> > >
> > > The alternative is to try to use native backlight in i915, which we did, but
> > > that didn't work on some machines.  I don't think we will know why it didn't
> > > work there before we collect some more information and that's not possible
> > > without user testing.  So, the idea is to make that testing possible without
> > > hacking the kernel and that's why we're introducing the new command line
> > > switch.  And we're going to ask users to try it and report back.
> > 
> > The thing that slightly bugs me with the proposed patches is that
> > they're adding a module parameter to i915 to tell ACPI video driver
> > whether to quirk the backlight or not. Before you know, we *will* have
> > requests to add quirks to i915 to tell ACPI video driver this.
> > 
> > I think the parameter "Does the ACPI backlight interface work or not"
> > belongs to the ACPI video driver.
> >
> > Feel free to file this in your bikeshedding folder, but I think i915
> > should only tell ACPI "I have a native backlight interface".
> 
> It kind of does that already through the intel_opregion_init() thing,
> so it would be trivial to test intel_opregion_present() in
> acpi_video_register().

It is possible the i915 driver decides not to register a backlight
interface for the graphics card for some reason(memory allocation failed
or it knows the native control does not work on this card or whatever),
so I would prefer let i915 tell ACPI video that it has registered a
native backlight control interface as Jani has said.

Then together with the video.use_native_backlight, we can register or
not register ACPI video backlight interface accordingly. Or rather, we
can simply not register ACPI video backlight interface for Win8 systems
as long as i915 indicates that it has native backlight control(if the
native control is broken, i915 should fix it or blacklist it so that
i915 will not indicate it has native backlight control and ACPI video
will continue to register its own).

How does this sound?

-Aaron

> 
> So yes, we can add a command line option, say 'use_native_backlight' to the
> ACPI video driver that will work like this: "if the Intel opregion is present
> and video.use_native_backlight is set, skip registering ACPI backlight".
> 
> Is that what you want?
> 
> Rafael
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jani Nikula Sept. 11, 2013, 8:45 a.m. UTC | #20

On Wed, 11 Sep 2013, Aaron Lu <[e-mail]> wrote:
> It is possible the i915 driver decides not to register a backlight
> interface for the graphics card for some reason(memory allocation failed
> or it knows the native control does not work on this card or whatever),
> so I would prefer let i915 tell ACPI video that it has registered a
> native backlight control interface as Jani has said.
>
> Then together with the video.use_native_backlight, we can register or
> not register ACPI video backlight interface accordingly. Or rather, we
> can simply not register ACPI video backlight interface for Win8 systems
> as long as i915 indicates that it has native backlight control(if the
> native control is broken, i915 should fix it or blacklist it so that
> i915 will not indicate it has native backlight control and ACPI video
> will continue to register its own).
>
> How does this sound?

Sounds good to me.

Before plunging forward, have you observed any difference between the
boot modes? We have reports [1] that the backlight behaviour is
different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the
acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole
story.

Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code
paths, what guarantees do we have of UEFI+CSM or legacy boots working?

BR,
Jani.


[1] https://bugzilla.kernel.org/show_bug.cgi?id=47941#c96

Matthew Garrett Sept. 11, 2013, 8:45 a.m. UTC | #21

On Wed, 2013-09-11 at 11:45 +0300, Jani Nikula wrote:

> Before plunging forward, have you observed any difference between the
> boot modes? We have reports [1] that the backlight behaviour is
> different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the
> acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole
> story.
> 
> Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code
> paths, what guarantees do we have of UEFI+CSM or legacy boots working?

We have no evidence of Windows behaving differently based on the exposed
firmware type.

Yves-Alexis Perez Sept. 11, 2013, 9:09 a.m. UTC | #22

On mer., 2013-09-11 at 08:45 +0000, Matthew Garrett wrote:
> On Wed, 2013-09-11 at 11:45 +0300, Jani Nikula wrote:
> 
> > Before plunging forward, have you observed any difference between the
> > boot modes? We have reports [1] that the backlight behaviour is
> > different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the
> > acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole
> > story.
> > 
> > Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code
> > paths, what guarantees do we have of UEFI+CSM or legacy boots working?
> 
> We have no evidence of Windows behaving differently based on the exposed
> firmware type.
> 
I do have an X230 which boots fine using pure UEFI or UEFI+CSM (and I
guess I can try booting it with a grml for pure legacy), so I can do
tests if needed.

Regards,

Jani Nikula Sept. 11, 2013, 10:29 a.m. UTC | #23

On Wed, 11 Sep 2013, Matthew Garrett <[e-mail]> wrote:
> On Wed, 2013-09-11 at 11:45 +0300, Jani Nikula wrote:
>
>> Before plunging forward, have you observed any difference between the
>> boot modes? We have reports [1] that the backlight behaviour is
>> different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the
>> acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole
>> story.
>> 
>> Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code
>> paths, what guarantees do we have of UEFI+CSM or legacy boots working?
>
> We have no evidence of Windows behaving differently based on the exposed
> firmware type.

By "behaving differently", do you mean internally adapting to the boot
mode, or exhibiting different behaviour to the user?

We have evidence of the firmware behaving differently (VBT, backlight)
based on the boot mode, all else being equal. We don't adapt to that,
and we fail. I don't know if we should adapt, or do things differently
altogether. I don't even know if Windows 8 works on all boot modes on
the machines in question.


BR,
Jani.

Matthew Garrett Sept. 11, 2013, 10:30 a.m. UTC | #24

On Wed, 2013-09-11 at 13:29 +0300, Jani Nikula wrote:
> On Wed, 11 Sep 2013, Matthew Garrett <[e-mail]> wrote:
> > On Wed, 2013-09-11 at 11:45 +0300, Jani Nikula wrote:
> >
> >> Before plunging forward, have you observed any difference between the
> >> boot modes? We have reports [1] that the backlight behaviour is
> >> different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the
> >> acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole
> >> story.
> >> 
> >> Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code
> >> paths, what guarantees do we have of UEFI+CSM or legacy boots working?
> >
> > We have no evidence of Windows behaving differently based on the exposed
> > firmware type.
> 
> By "behaving differently", do you mean internally adapting to the boot
> mode, or exhibiting different behaviour to the user?

As far as backlight control goes, both.

> We have evidence of the firmware behaving differently (VBT, backlight)
> based on the boot mode, all else being equal. We don't adapt to that,
> and we fail. I don't know if we should adapt, or do things differently
> altogether. I don't even know if Windows 8 works on all boot modes on
> the machines in question.

Sure, but Windows knows nothing about VBT or opregion-backed backlight
control. That's up to the Intel driver.

Aaron Lu Sept. 12, 2013, 2:26 a.m. UTC | #25

On Wed, Sep 11, 2013 at 11:45:19AM +0300, Jani Nikula wrote:
> On Wed, 11 Sep 2013, Aaron Lu <[e-mail]> wrote:
> > It is possible the i915 driver decides not to register a backlight
> > interface for the graphics card for some reason(memory allocation failed
> > or it knows the native control does not work on this card or whatever),
> > so I would prefer let i915 tell ACPI video that it has registered a
> > native backlight control interface as Jani has said.
> >
> > Then together with the video.use_native_backlight, we can register or
> > not register ACPI video backlight interface accordingly. Or rather, we
> > can simply not register ACPI video backlight interface for Win8 systems
> > as long as i915 indicates that it has native backlight control(if the
> > native control is broken, i915 should fix it or blacklist it so that
> > i915 will not indicate it has native backlight control and ACPI video
> > will continue to register its own).
> >
> > How does this sound?
> 
> Sounds good to me.
> 
> Before plunging forward, have you observed any difference between the
> boot modes? We have reports [1] that the backlight behaviour is

Not yet from ACPI's point of view.

> different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the
> acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole
> story.

This check in patch 2/2 is a policy: for Win8 system, we think the
native backlight control has a better chance of working than the ACPI
video's, so I think the check is enough in ACPI video.

> 
> Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code
> paths, what guarantees do we have of UEFI+CSM or legacy boots working?

I suppose the 'tested BIOS code paths' means the pure UEFI boot mode? I
don't know what guatantees do we have since I don't know what happened
underneath after the backlight register is set in i915 driver, you or
other i915 driver people should know more than I do :-)

BTW, after the backlight register is set in i915, is it that some
find of firmware code will run in response to the setting of the
register(e.g. the BLC_PWM_CTL/BLC_PWM_CPU_CTL/PCI_LBPC reg)?

Thanks,
Aaron

Patch

diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 20f4233..a53832e 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -170,8 +170,10 @@  int acpi_create_platform_device(struct acpi_device *adev,
   -------------------------------------------------------------------------- */
 #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
 bool acpi_video_backlight_quirks(void);
+bool acpi_video_verify_backlight_support(void);
 #else
 static inline bool acpi_video_backlight_quirks(void) { return false; }
+static inline bool acpi_video_verify_backlight_support(void) { return false; }
 #endif
 
 #endif /* _ACPI_INTERNAL_H_ */
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 5ad5a71..cc709a7 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -1256,8 +1256,8 @@  acpi_video_switch_brightness(struct acpi_video_device *device, int event)
 	unsigned long long level_current, level_next;
 	int result = -EINVAL;
 
-	/* no warning message if acpi_backlight=vendor is used */
-	if (!acpi_video_backlight_support())
+	/* no warning message if acpi_backlight=vendor or a quirk is used */
+	if (!acpi_video_verify_backlight_support())
 		return 0;
 
 	if (!device->brightness)
@@ -1558,7 +1558,7 @@  acpi_video_bus_match(acpi_handle handle, u32 level, void *context,
 
 static void acpi_video_dev_register_backlight(struct acpi_video_device *device)
 {
-	if (acpi_video_backlight_support()) {
+	if (acpi_video_verify_backlight_support()) {
 		struct backlight_properties props;
 		struct pci_dev *pdev;
 		acpi_handle acpi_parent;
@@ -1933,14 +1933,22 @@  static int __init intel_opregion_present(void)
 	return opregion;
 }
 
-int acpi_video_register(void)
+int __acpi_video_register(bool backlight_quirks)
 {
-	int result = 0;
+	bool no_backlight;
+	int result;
+
+	no_backlight = backlight_quirks ? acpi_video_backlight_quirks() : false;
+
 	if (register_count) {
 		/*
-		 * if the function of acpi_video_register is already called,
-		 * don't register the acpi_vide_bus again and return no error.
+		 * If acpi_video_register() has been called already, don't try
+		 * to register acpi_video_bus, but unregister backlight devices
+		 * if no backlight support is requested.
 		 */
+		if (no_backlight)
+			acpi_video_unregister_backlight();
+
 		return 0;
 	}
 
@@ -1959,7 +1967,7 @@  int acpi_video_register(void)
 
 	return 0;
 }
-EXPORT_SYMBOL(acpi_video_register);
+EXPORT_SYMBOL(__acpi_video_register);
 
 void acpi_video_unregister(void)
 {
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index 940edbf..c7e43ec 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -235,7 +235,12 @@  static void acpi_video_caps_check(void)
 
 bool acpi_video_backlight_quirks(void)
 {
-	return acpi_gbl_osi_data >= ACPI_OSI_WIN_8;
+	if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8) {
+		acpi_video_caps_check();
+		acpi_video_support |= ACPI_VIDEO_SKIP_BACKLIGHT;
+		return true;
+	}
+	return false;
 }
 EXPORT_SYMBOL(acpi_video_backlight_quirks);
 
@@ -283,6 +288,14 @@  int acpi_video_backlight_support(void)
 }
 EXPORT_SYMBOL(acpi_video_backlight_support);
 
+/* For the ACPI video driver use only. */
+bool acpi_video_verify_backlight_support(void)
+{
+	return (acpi_video_support & ACPI_VIDEO_SKIP_BACKLIGHT) ?
+		false : acpi_video_backlight_support();
+}
+EXPORT_SYMBOL(acpi_video_verify_backlight_support);
+
 /*
  * Use acpi_backlight=vendor/video to force that backlight switching
  * is processed by vendor specific acpi drivers or video.ko driver.
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index f466980..75fba17 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1650,7 +1650,7 @@  int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	if (INTEL_INFO(dev)->num_pipes) {
 		/* Must be done after probing outputs */
 		intel_opregion_init(dev);
-		acpi_video_register();
+		__acpi_video_register(i915_take_over_backlight);
 	}
 
 	if (IS_GEN5(dev))
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 45b3c03..b014398 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -132,6 +132,11 @@  int i915_enable_ips __read_mostly = 1;
 module_param_named(enable_ips, i915_enable_ips, int, 0600);
 MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
 
+bool i915_take_over_backlight = false;
+module_param_named(take_over_backlight, i915_take_over_backlight, bool, 0644);
+MODULE_PARM_DESC(take_over_backlight,
+		"Prevent ACPI backlight from being used (default: false)");
+
 static struct drm_driver driver;
 extern int intel_agp_enabled;
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1929bff..b43c430 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1543,6 +1543,7 @@  extern int i915_enable_ppgtt __read_mostly;
 extern unsigned int i915_preliminary_hw_support __read_mostly;
 extern int i915_disable_power_well __read_mostly;
 extern int i915_enable_ips __read_mostly;
+extern bool i915_take_over_backlight __read_mostly;
 
 extern int i915_suspend(struct drm_device *dev, pm_message_t state);
 extern int i915_resume(struct drm_device *dev);
diff --git a/include/acpi/video.h b/include/acpi/video.h
index 0c665b5..dc5b8ad 100644
--- a/include/acpi/video.h
+++ b/include/acpi/video.h
@@ -17,13 +17,13 @@  struct acpi_device;
 #define ACPI_VIDEO_DISPLAY_LEGACY_TV      0x0200
 
 #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
-extern int acpi_video_register(void);
+extern int __acpi_video_register(bool backlight_quirks);
 extern void acpi_video_unregister(void);
 extern int acpi_video_unregister_backlight(void);
 extern int acpi_video_get_edid(struct acpi_device *device, int type,
 			       int device_id, void **edid);
 #else
-static inline int acpi_video_register(void) { return 0; }
+static inline int __acpi_video_register(bool backlight_quirks) { return 0; }
 static inline void acpi_video_unregister(void) { return; }
 static inline int acpi_video_unregister_backlight(void) { return; }
 static inline int acpi_video_get_edid(struct acpi_device *device, int type,
@@ -33,4 +33,9 @@  static inline int acpi_video_get_edid(struct acpi_device *device, int type,
 }
 #endif
 
+static inline int acpi_video_register(void)
+{
+	return __acpi_video_register(false);
+}
+
 #endif
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index a5db4ae..a0dcce5 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -191,6 +191,7 @@  extern bool wmi_has_guid(const char *guid);
 #define ACPI_VIDEO_BACKLIGHT_DMI_VIDEO			0x0200
 #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VENDOR		0x0400
 #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VIDEO		0x0800
+#define ACPI_VIDEO_SKIP_BACKLIGHT			0x1000
 
 #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)