Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight iffirmware expects Windows 8

[Aaron Lu]

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 <aaron.lu@xxxxxxxxx>
> > ---
> >  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
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/