[PATCH v2 0/3] Add acpi_video_unregister_backlight(), use in acer-wmi

[PATCH v2 0/3] Add acpi_video_unregister_backlight(), use in acer-wmi

[Hans de Goede]

Hi All,

Here is a v2 of my patch series to fix:

https://bugzilla.kernel.org/show_bug.cgi?id=35622
https://bugzilla.redhat.com/show_bug.cgi?id=1012674

I think its best if the entire series goes upstream through the acpi
tree since the platform/x86 patches depend on the acpi changes.

Changes since v1:
-Fix the syntax error in the #ifndef CONFIG_ACPI_VIDEO
 acpi_video_unregister_backlight inline definition.   

Regards,

Hans

[PATCH v2 1/3] acpi-video: Add an acpi_video_unregister_backlight function

[Hans de Goede]

Add an acpi_video_unregister_backlight function, which only unregisters
the backlight device, and leaves the acpi_notifier in place. Some acpi_vendor
driver need this as they don't want the acpi_video# backlight device, but do
need the acpi-video driver for hotkey handling.

Chances are that this new acpi_video_unregister_backlight() is actually
what existing acpi_vendor drivers have wanted all along. Currently acpi_vendor
drivers which want to disable the acpi_video# backlight device, make 2 calls:

acpi_video_dmi_promote_vendor();
acpi_video_unregister();

The intention here is to make things independent of when acpi_video_register()
gets called. As acpi_video_register() will get called on acpi-video load time
on non intel gfx machines, while it gets called on i915 load time on intel
gfx machines.

This leads to the following 2 interesting scenarios:

a) intel gfx:
1) acpi-video module gets loaded (as it is a dependency of acpi_vendor and i915)
2) acpi-video does NOT call acpi_video_register()
3) acpi_vendor loads (lets assume it loads before i915), calls
acpi_video_dmi_promote_vendor(); which sets ACPI_VIDEO_BACKLIGHT_DMI_VENDOR
4) calls acpi_video_unregister -> not registered, nop
5) i915 loads, calls acpi_video_register
6) acpi_video_register registers the acpi_notifier for the hotkeys,
   does NOT register a backlight device because of ACPI_VIDEO_BACKLIGHT_DMI_VENDOR

b) non intel gfx
1) acpi-video module gets loaded (as it is a dependency acpi_vendor)
2) acpi-video calls acpi_video_register()
3) acpi_video_register registers the acpi_notifier for the hotkeys,
   and a backlight device
4) acpi_vendor loads, calls acpi_video_dmi_promote_vendor()
5) calls acpi_video_unregister, this unregisters BOTH the acpi_notifier for
   the hotkeys AND the backlight device

So here we have possibly the same acpi_vendor module, making the same calls,
but with different results, in one cases acpi-video does handle hotkeys,
in the other it does not.

Note that the a) scenario turns into b) if we assume the i915 module loads
before the vendor_acpi module, so we also have different behavior depending
on module loading order!

So as said I believe that quite a few existing acpi_vendor modules really
always want the behavior of a), hence this patch adds a new
acpi_video_unregister_backlight() which gives the behavior of a) independent
of module loading order.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/video.c | 14 ++++++++++++++
 include/acpi/video.h |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 52176ad..ba6e4d7 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -2166,6 +2166,20 @@ void acpi_video_unregister(void)
 }
 EXPORT_SYMBOL(acpi_video_unregister);
 
+void acpi_video_unregister_backlight(void)
+{
+	struct acpi_video_bus *video;
+
+	if (!register_count)
+		return;
+
+	mutex_lock(&video_list_lock);
+	list_for_each_entry(video, &video_bus_head, entry)
+		acpi_video_bus_unregister_backlight(video);
+	mutex_unlock(&video_list_lock);
+}
+EXPORT_SYMBOL(acpi_video_unregister_backlight);
+
 /*
  * This is kind of nasty. Hardware using Intel chipsets may require
  * the video opregion code to be run first in order to initialise
diff --git a/include/acpi/video.h b/include/acpi/video.h
index 61109f2..ea4c7bb 100644
--- a/include/acpi/video.h
+++ b/include/acpi/video.h
@@ -19,11 +19,13 @@ struct acpi_device;
 #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
 extern int acpi_video_register(void);
 extern void acpi_video_unregister(void);
+extern void acpi_video_unregister_backlight(void);
 extern int acpi_video_get_edid(struct acpi_device *device, int type,
 			       int device_id, void **edid);
 #else
 static inline int acpi_video_register(void) { return 0; }
 static inline void acpi_video_unregister(void) { return; }
+static inline void acpi_video_unregister_backlight(void) { return; }
 static inline int acpi_video_get_edid(struct acpi_device *device, int type,
 				      int device_id, void **edid)
 {
-- 
1.9.0

[PATCH v2 2/3] acer-wmi: Switch to acpi_video_unregister_backlight

[Hans de Goede]

Switch from acpi_video_unregister(), to acpi_video_unregister_backlight(),
so that the hotkeys handler registered by acpi-video stays in place.

Since there are no mappings for the atkbd raw codes for the brightness
keys used by newer Acer models in /lib/udev/hwdb.d/60-keyboard.hwdb, and
since we map the wmi events with a code of KE_IGNORE, we rely on acpi-video
to do the hotkey handling for us.

For laptops such as the Acer Aspire 5750 which uses intel gfx this works
despite us calling acpi_video_unregister() because the following happens:

1) acpi-video module gets loaded (as it is a dependency of acer-wmi and i915)
2) acpi-video does NOT call acpi_video_register()
3) acer-wmi loads (assume it loads before i915), calls
acpi_video_dmi_promote_vendor(); which sets ACPI_VIDEO_BACKLIGHT_DMI_VENDOR
4) calls acpi_video_unregister -> not registered, nop
5) i915 loads, calls acpi_video_register
6) acpi_video_register registers the acpi_notifier for the hotkeys,
   does NOT register a backlight device because of ACPI_VIDEO_BACKLIGHT_DMI_VENDOR

But on the Acer Aspire 5750G, which uses nvidia graphics the following happens:
1) acpi-video module gets loaded (as it is a dependency of acer-wmi)
2) acpi-video calls acpi_video_register()
3) acpi_video_register registers the acpi_notifier for the hotkeys,
   and a backlight device
4) acer-wmi loads, calls acpi_video_dmi_promote_vendor()
5) calls acpi_video_unregister, this unregisters BOTH the acpi_notifier for
   the hotkeys AND the backlight device

And we end up without any handler for the brightness hotkeys. This patch fixes
this by switching over to acpi_video_unregister_backlight() which keeps the
hotkey handler in place.

https://bugzilla.kernel.org/show_bug.cgi?id=35622

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/acer-wmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index c91f69b3..3a74699 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -2228,7 +2228,7 @@ static int __init acer_wmi_init(void)
 		pr_info("Brightness must be controlled by acpi video driver\n");
 	} else {
 		pr_info("Disabling ACPI video driver\n");
-		acpi_video_unregister();
+		acpi_video_unregister_backlight();
 	}
 
 	if (wmi_has_guid(WMID_GUID3)) {
-- 
1.9.0

[PATCH v2 3/3] acer-wmi: Add Aspire 5741 to video_vendor_dmi_table

[Hans de Goede]

The Aspire 5741 has broken acpi-video backlight control, so add it to the
quirk table.

https://bugzilla.redhat.com/show_bug.cgi?id=1012674

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/acer-wmi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index 3a74699..bbf78b2 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -570,6 +570,14 @@ static const struct dmi_system_id video_vendor_dmi_table[] = {
 			DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 5750"),
 		},
 	},
+	{
+		.callback = video_set_backlight_video_vendor,
+		.ident = "Acer Aspire 5741",
+		.matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "Acer"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 5741"),
+		},
+	},
 	{}
 };
 
-- 
1.9.0

Re: [PATCH v2 2/3] acer-wmi: Switch to acpi_video_unregister_backlight

[joeyli]

On Sat, May 17, 2014 at 10:48:02AM +0200, Hans de Goede wrote:
> Switch from acpi_video_unregister(), to acpi_video_unregister_backlight(),
> so that the hotkeys handler registered by acpi-video stays in place.
> 
> Since there are no mappings for the atkbd raw codes for the brightness
> keys used by newer Acer models in /lib/udev/hwdb.d/60-keyboard.hwdb, and
> since we map the wmi events with a code of KE_IGNORE, we rely on acpi-video
> to do the hotkey handling for us.
> 
> For laptops such as the Acer Aspire 5750 which uses intel gfx this works
> despite us calling acpi_video_unregister() because the following happens:
> 
> 1) acpi-video module gets loaded (as it is a dependency of acer-wmi and i915)
> 2) acpi-video does NOT call acpi_video_register()
> 3) acer-wmi loads (assume it loads before i915), calls
> acpi_video_dmi_promote_vendor(); which sets ACPI_VIDEO_BACKLIGHT_DMI_VENDOR
> 4) calls acpi_video_unregister -> not registered, nop
> 5) i915 loads, calls acpi_video_register
> 6) acpi_video_register registers the acpi_notifier for the hotkeys,
>    does NOT register a backlight device because of ACPI_VIDEO_BACKLIGHT_DMI_VENDOR
> 
> But on the Acer Aspire 5750G, which uses nvidia graphics the following happens:
> 1) acpi-video module gets loaded (as it is a dependency of acer-wmi)
> 2) acpi-video calls acpi_video_register()
> 3) acpi_video_register registers the acpi_notifier for the hotkeys,
>    and a backlight device
> 4) acer-wmi loads, calls acpi_video_dmi_promote_vendor()
> 5) calls acpi_video_unregister, this unregisters BOTH the acpi_notifier for
>    the hotkeys AND the backlight device
> 
> And we end up without any handler for the brightness hotkeys. This patch fixes
> this by switching over to acpi_video_unregister_backlight() which keeps the
> hotkey handler in place.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=35622
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

This patch is good to me.

Reviewd-by: Lee, Chun-Yi <jlee@suse.com>


Thanks for your patch.

> ---
>  drivers/platform/x86/acer-wmi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index c91f69b3..3a74699 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -2228,7 +2228,7 @@ static int __init acer_wmi_init(void)
>  		pr_info("Brightness must be controlled by acpi video driver\n");
>  	} else {
>  		pr_info("Disabling ACPI video driver\n");
> -		acpi_video_unregister();
> +		acpi_video_unregister_backlight();
>  	}
>  
>  	if (wmi_has_guid(WMID_GUID3)) {
> -- 
> 1.9.0
> 

Joey Lee

Re: [PATCH v2 3/3] acer-wmi: Add Aspire 5741 to video_vendor_dmi_table

[joeyli]

On Sat, May 17, 2014 at 10:48:03AM +0200, Hans de Goede wrote:
> The Aspire 5741 has broken acpi-video backlight control, so add it to the
> quirk table.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1012674
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Lee, Chun-Yi <jlee@suse.com>

Thanks for your patch!

> ---
>  drivers/platform/x86/acer-wmi.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index 3a74699..bbf78b2 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -570,6 +570,14 @@ static const struct dmi_system_id video_vendor_dmi_table[] = {
>  			DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 5750"),
>  		},
>  	},
> +	{
> +		.callback = video_set_backlight_video_vendor,
> +		.ident = "Acer Aspire 5741",
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_VENDOR, "Acer"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 5741"),
> +		},
> +	},
>  	{}
>  };
>  
> -- 
> 1.9.0
> 
Joey Lee

Re: [PATCH v2 1/3] acpi-video: Add an acpi_video_unregister_backlight function

[Aaron Lu]

On 05/17/2014 04:48 PM, Hans de Goede wrote:
> Add an acpi_video_unregister_backlight function, which only unregisters
> the backlight device, and leaves the acpi_notifier in place. Some acpi_vendor
> driver need this as they don't want the acpi_video# backlight device, but do
> need the acpi-video driver for hotkey handling.
> 
> Chances are that this new acpi_video_unregister_backlight() is actually
> what existing acpi_vendor drivers have wanted all along. Currently acpi_vendor
> drivers which want to disable the acpi_video# backlight device, make 2 calls:
> 
> acpi_video_dmi_promote_vendor();
> acpi_video_unregister();
> 
> The intention here is to make things independent of when acpi_video_register()
> gets called. As acpi_video_register() will get called on acpi-video load time
> on non intel gfx machines, while it gets called on i915 load time on intel
> gfx machines.
> 
> This leads to the following 2 interesting scenarios:
> 
> a) intel gfx:
> 1) acpi-video module gets loaded (as it is a dependency of acpi_vendor and i915)
> 2) acpi-video does NOT call acpi_video_register()
> 3) acpi_vendor loads (lets assume it loads before i915), calls
> acpi_video_dmi_promote_vendor(); which sets ACPI_VIDEO_BACKLIGHT_DMI_VENDOR
> 4) calls acpi_video_unregister -> not registered, nop
> 5) i915 loads, calls acpi_video_register
> 6) acpi_video_register registers the acpi_notifier for the hotkeys,
>    does NOT register a backlight device because of ACPI_VIDEO_BACKLIGHT_DMI_VENDOR
> 
> b) non intel gfx
> 1) acpi-video module gets loaded (as it is a dependency acpi_vendor)
> 2) acpi-video calls acpi_video_register()
> 3) acpi_video_register registers the acpi_notifier for the hotkeys,
>    and a backlight device
> 4) acpi_vendor loads, calls acpi_video_dmi_promote_vendor()
> 5) calls acpi_video_unregister, this unregisters BOTH the acpi_notifier for
>    the hotkeys AND the backlight device
> 
> So here we have possibly the same acpi_vendor module, making the same calls,
> but with different results, in one cases acpi-video does handle hotkeys,
> in the other it does not.
> 
> Note that the a) scenario turns into b) if we assume the i915 module loads
> before the vendor_acpi module, so we also have different behavior depending
> on module loading order!
> 
> So as said I believe that quite a few existing acpi_vendor modules really
> always want the behavior of a), hence this patch adds a new
> acpi_video_unregister_backlight() which gives the behavior of a) independent
> of module loading order.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Aaron Lu <aaron.lu@intel.com>

Thanks,
Aaron

> ---
>  drivers/acpi/video.c | 14 ++++++++++++++
>  include/acpi/video.h |  2 ++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 52176ad..ba6e4d7 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -2166,6 +2166,20 @@ void acpi_video_unregister(void)
>  }
>  EXPORT_SYMBOL(acpi_video_unregister);
>  
> +void acpi_video_unregister_backlight(void)
> +{
> +	struct acpi_video_bus *video;
> +
> +	if (!register_count)
> +		return;
> +
> +	mutex_lock(&video_list_lock);
> +	list_for_each_entry(video, &video_bus_head, entry)
> +		acpi_video_bus_unregister_backlight(video);
> +	mutex_unlock(&video_list_lock);
> +}
> +EXPORT_SYMBOL(acpi_video_unregister_backlight);
> +
>  /*
>   * This is kind of nasty. Hardware using Intel chipsets may require
>   * the video opregion code to be run first in order to initialise
> diff --git a/include/acpi/video.h b/include/acpi/video.h
> index 61109f2..ea4c7bb 100644
> --- a/include/acpi/video.h
> +++ b/include/acpi/video.h
> @@ -19,11 +19,13 @@ struct acpi_device;
>  #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
>  extern int acpi_video_register(void);
>  extern void acpi_video_unregister(void);
> +extern void acpi_video_unregister_backlight(void);
>  extern int acpi_video_get_edid(struct acpi_device *device, int type,
>  			       int device_id, void **edid);
>  #else
>  static inline int acpi_video_register(void) { return 0; }
>  static inline void acpi_video_unregister(void) { return; }
> +static inline void acpi_video_unregister_backlight(void) { return; }
>  static inline int acpi_video_get_edid(struct acpi_device *device, int type,
>  				      int device_id, void **edid)
>  {
>