[RFC 0/3] Add acpi_video_unregister_backlight and use it in acer-wmi

[RFC 0/3] Add acpi_video_unregister_backlight and use it in acer-wmi

[Hans de Goede]

Hi Aaron and Lee,

While working on figuring out howto fix this Acer backlight bug properly:
https://bugzilla.redhat.com/show_bug.cgi?id=1012674

TL;DR: needs acpi-video to not register the backlight device, but keep it
processing hotkeys, like video.use_native_backlight=1 does, but this machine
does not have a win8 ready BIOS.

This eventually lead me to https://bugzilla.kernel.org/show_bug.cgi?id=35622
and the whole discussion between you there, as well as to the patches
attached there. I believe that Lee's patches there to add a function to
acpi-video to only unregister the backlight device are the correct way
forward. I've explained why in the commit messages.

Note that for the fix for bko35622 I've actually opted for a bigger hammer,
by simply always calling acpi_video_unregister_backlight instead of
acpi_video_unregister. Again rationale is in the commit message, it boils
down to acpi_video_unregister_backlight() leading to consistent behavior,
where as acpi_video_unregister() leads to different behavior depending on
module load ordering. So even if this causes issues somewhere (which would be
2 keypresses reported for each press, which is not too bad), it still is
better, because it at least will give us consistent wrong or wright behavior
and we can then fix the double key presses from a stable base.

One could go as far as to say that acpi_video_unregister() should simply go
away completely.

The 3th patch fixes the bug I started with by simply adding a dmi table
entry for the model in the rhbz1012674

This series is RFC for now because some of these changes may be somewhat
controversial, but if you agree with the proposed changes, then I would like
your acks and then get this merged.

Regards,

Hans

[RFC 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), which calling acpi_video_unregister_backlight()
instead of acpi_video_unregister() will give them.

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

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 6a2099d..80a6759 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -1843,7 +1843,7 @@ static void acpi_video_dev_register_backlight(struct acpi_video_device *device)
 	}
 }
 
-static int acpi_video_bus_register_backlight(struct acpi_video_bus *video)
+static void acpi_video_bus_register_backlight(struct acpi_video_bus *video)
 {
 	struct acpi_video_device *dev;
 
@@ -1851,10 +1851,6 @@ static int acpi_video_bus_register_backlight(struct acpi_video_bus *video)
 	list_for_each_entry(dev, &video->video_device_list, entry)
 		acpi_video_dev_register_backlight(dev);
 	mutex_unlock(&video->device_list_lock);
-
-	video->pm_nb.notifier_call = acpi_video_resume;
-	video->pm_nb.priority = 0;
-	return register_pm_notifier(&video->pm_nb);
 }
 
 static void acpi_video_dev_unregister_backlight(struct acpi_video_device *device)
@@ -1876,17 +1872,14 @@ static void acpi_video_dev_unregister_backlight(struct acpi_video_device *device
 	}
 }
 
-static int acpi_video_bus_unregister_backlight(struct acpi_video_bus *video)
+static void acpi_video_bus_unregister_backlight(struct acpi_video_bus *video)
 {
 	struct acpi_video_device *dev;
-	int error = unregister_pm_notifier(&video->pm_nb);
 
 	mutex_lock(&video->device_list_lock);
 	list_for_each_entry(dev, &video->video_device_list, entry)
 		acpi_video_dev_unregister_backlight(dev);
 	mutex_unlock(&video->device_list_lock);
-
-	return error;
 }
 
 static void acpi_video_dev_add_notify_handler(struct acpi_video_device *device)
@@ -2059,6 +2052,11 @@ static int acpi_video_bus_add(struct acpi_device *device)
 	mutex_unlock(&video_list_lock);
 
 	acpi_video_bus_register_backlight(video);
+
+	video->pm_nb.notifier_call = acpi_video_resume;
+	video->pm_nb.priority = 0;
+	register_pm_notifier(&video->pm_nb);
+
 	acpi_video_bus_add_notify_handler(video);
 
 	return 0;
@@ -2084,6 +2082,7 @@ static int acpi_video_bus_remove(struct acpi_device *device)
 	video = acpi_driver_data(device);
 
 	acpi_video_bus_remove_notify_handler(video);
+	unregister_pm_notifier(&video->pm_nb);
 	acpi_video_bus_unregister_backlight(video);
 	acpi_video_bus_put_devices(video);
 
@@ -2173,6 +2172,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..4722c06 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; }
+extern 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

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

[Aaron Lu]

On 05/13/2014 02:03 AM, 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), which calling acpi_video_unregister_backlight()
> instead of acpi_video_unregister() will give them.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/acpi/video.c | 31 ++++++++++++++++++++++---------
>  include/acpi/video.h |  2 ++
>  2 files changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 6a2099d..80a6759 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -1843,7 +1843,7 @@ static void acpi_video_dev_register_backlight(struct acpi_video_device *device)
>  	}
>  }
>  
> -static int acpi_video_bus_register_backlight(struct acpi_video_bus *video)
> +static void acpi_video_bus_register_backlight(struct acpi_video_bus *video)
>  {
>  	struct acpi_video_device *dev;
>  
> @@ -1851,10 +1851,6 @@ static int acpi_video_bus_register_backlight(struct acpi_video_bus *video)
>  	list_for_each_entry(dev, &video->video_device_list, entry)
>  		acpi_video_dev_register_backlight(dev);
>  	mutex_unlock(&video->device_list_lock);
> -
> -	video->pm_nb.notifier_call = acpi_video_resume;
> -	video->pm_nb.priority = 0;
> -	return register_pm_notifier(&video->pm_nb);

Hmm, I don't understand this. The pm notifier is used to restore
backlight level on system resume, so should be there when we have
created the backlight interface and gone when we unregistered the
backlight interface. It doesn't have anything to do with hotkey
processing.

Thanks,
Aaron

>  }
>  
>  static void acpi_video_dev_unregister_backlight(struct acpi_video_device *device)
> @@ -1876,17 +1872,14 @@ static void acpi_video_dev_unregister_backlight(struct acpi_video_device *device
>  	}
>  }
>  
> -static int acpi_video_bus_unregister_backlight(struct acpi_video_bus *video)
> +static void acpi_video_bus_unregister_backlight(struct acpi_video_bus *video)
>  {
>  	struct acpi_video_device *dev;
> -	int error = unregister_pm_notifier(&video->pm_nb);
>  
>  	mutex_lock(&video->device_list_lock);
>  	list_for_each_entry(dev, &video->video_device_list, entry)
>  		acpi_video_dev_unregister_backlight(dev);
>  	mutex_unlock(&video->device_list_lock);
> -
> -	return error;
>  }
>  
>  static void acpi_video_dev_add_notify_handler(struct acpi_video_device *device)
> @@ -2059,6 +2052,11 @@ static int acpi_video_bus_add(struct acpi_device *device)
>  	mutex_unlock(&video_list_lock);
>  
>  	acpi_video_bus_register_backlight(video);
> +
> +	video->pm_nb.notifier_call = acpi_video_resume;
> +	video->pm_nb.priority = 0;
> +	register_pm_notifier(&video->pm_nb);
> +
>  	acpi_video_bus_add_notify_handler(video);
>  
>  	return 0;
> @@ -2084,6 +2082,7 @@ static int acpi_video_bus_remove(struct acpi_device *device)
>  	video = acpi_driver_data(device);
>  
>  	acpi_video_bus_remove_notify_handler(video);
> +	unregister_pm_notifier(&video->pm_nb);
>  	acpi_video_bus_unregister_backlight(video);
>  	acpi_video_bus_put_devices(video);
>  
> @@ -2173,6 +2172,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..4722c06 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; }
> +extern 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)
>  {
> 

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

[Hans de Goede]

Hi,

On 05/13/2014 05:11 PM, Aaron Lu wrote:
> On 05/13/2014 02:03 AM, 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), which calling acpi_video_unregister_backlight()
>> instead of acpi_video_unregister() will give them.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/acpi/video.c | 31 ++++++++++++++++++++++---------
>>  include/acpi/video.h |  2 ++
>>  2 files changed, 24 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
>> index 6a2099d..80a6759 100644
>> --- a/drivers/acpi/video.c
>> +++ b/drivers/acpi/video.c
>> @@ -1843,7 +1843,7 @@ static void acpi_video_dev_register_backlight(struct acpi_video_device *device)
>>  	}
>>  }
>>  
>> -static int acpi_video_bus_register_backlight(struct acpi_video_bus *video)
>> +static void acpi_video_bus_register_backlight(struct acpi_video_bus *video)
>>  {
>>  	struct acpi_video_device *dev;
>>  
>> @@ -1851,10 +1851,6 @@ static int acpi_video_bus_register_backlight(struct acpi_video_bus *video)
>>  	list_for_each_entry(dev, &video->video_device_list, entry)
>>  		acpi_video_dev_register_backlight(dev);
>>  	mutex_unlock(&video->device_list_lock);
>> -
>> -	video->pm_nb.notifier_call = acpi_video_resume;
>> -	video->pm_nb.priority = 0;
>> -	return register_pm_notifier(&video->pm_nb);
> 
> Hmm, I don't understand this. The pm notifier is used to restore
> backlight level on system resume, so should be there when we have
> created the backlight interface and gone when we unregistered the
> backlight interface. It doesn't have anything to do with hotkey
> processing.

A valid point, I did things this way because I wanted the new
acpi_video_unregister_backlight to result in the same behavior as
having set the acpi_video=vendor / called acpi_video_dmi_promote_vendor()
before acpi_video_register runs.

So this means undoing what-ever acpi_video_register() does, which it
would not have done if acpi_video_dmi_promote_vendor() came first.

If you look at the current code (so without this patch) then
the pm notifier gets installed unconditionally by
acpi_video_bus_register_backlight() which gets called unconditionally
by acpi_video_bus_add(). So even with acpi_video=vendor it still gets
installed.

acpi_video=vendor / acpi_video_dmi_promote_vendor() influences
acpi_video_backlight_support() which only gets called by
acpi_video_verify_backlight_support() which only gets called by
acpi_video_dev_register_backlight(). So acpi_video=vendor only causes
the backlight devices to not register, the pm notifier will still be
installed. My intend was to behave the same independent of module
loading / init order hence I moved the pm notifier install / uninstall
to keep the pm notifier installed when calling the new
acpi_video_unregister_backlight().

Looking at the code you're right that this is not really sensible, since
the acpi_video_resume call done by the notifier will be a nop when there
are no backlight devices registered.

So I can split this patch into 2 patches, 1 to not install the pm notifier
if we're not going to register backlight devices anyways, and a 2nd patch
adding the new acpi_video_unregister_backlight().

Does that sound like a plan ?

Regards,

Hans




> 
> Thanks,
> Aaron
> 
>>  }
>>  
>>  static void acpi_video_dev_unregister_backlight(struct acpi_video_device *device)
>> @@ -1876,17 +1872,14 @@ static void acpi_video_dev_unregister_backlight(struct acpi_video_device *device
>>  	}
>>  }
>>  
>> -static int acpi_video_bus_unregister_backlight(struct acpi_video_bus *video)
>> +static void acpi_video_bus_unregister_backlight(struct acpi_video_bus *video)
>>  {
>>  	struct acpi_video_device *dev;
>> -	int error = unregister_pm_notifier(&video->pm_nb);
>>  
>>  	mutex_lock(&video->device_list_lock);
>>  	list_for_each_entry(dev, &video->video_device_list, entry)
>>  		acpi_video_dev_unregister_backlight(dev);
>>  	mutex_unlock(&video->device_list_lock);
>> -
>> -	return error;
>>  }
>>  
>>  static void acpi_video_dev_add_notify_handler(struct acpi_video_device *device)
>> @@ -2059,6 +2052,11 @@ static int acpi_video_bus_add(struct acpi_device *device)
>>  	mutex_unlock(&video_list_lock);
>>  
>>  	acpi_video_bus_register_backlight(video);
>> +
>> +	video->pm_nb.notifier_call = acpi_video_resume;
>> +	video->pm_nb.priority = 0;
>> +	register_pm_notifier(&video->pm_nb);
>> +
>>  	acpi_video_bus_add_notify_handler(video);
>>  
>>  	return 0;
>> @@ -2084,6 +2082,7 @@ static int acpi_video_bus_remove(struct acpi_device *device)
>>  	video = acpi_driver_data(device);
>>  
>>  	acpi_video_bus_remove_notify_handler(video);
>> +	unregister_pm_notifier(&video->pm_nb);
>>  	acpi_video_bus_unregister_backlight(video);
>>  	acpi_video_bus_put_devices(video);
>>  
>> @@ -2173,6 +2172,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..4722c06 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; }
>> +extern 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)
>>  {
>>
> 

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

[Aaron Lu]

On 05/14/2014 05:08 PM, Hans de Goede wrote:
> Hi,
> 
> On 05/13/2014 05:11 PM, Aaron Lu wrote:
>> On 05/13/2014 02:03 AM, Hans de Goede wrote:
>>> -static int acpi_video_bus_register_backlight(struct acpi_video_bus *video)
>>> +static void acpi_video_bus_register_backlight(struct acpi_video_bus *video)
>>>  {
>>>  	struct acpi_video_device *dev;
>>>  
>>> @@ -1851,10 +1851,6 @@ static int acpi_video_bus_register_backlight(struct acpi_video_bus *video)
>>>  	list_for_each_entry(dev, &video->video_device_list, entry)
>>>  		acpi_video_dev_register_backlight(dev);
>>>  	mutex_unlock(&video->device_list_lock);
>>> -
>>> -	video->pm_nb.notifier_call = acpi_video_resume;
>>> -	video->pm_nb.priority = 0;
>>> -	return register_pm_notifier(&video->pm_nb);
>>
>> Hmm, I don't understand this. The pm notifier is used to restore
>> backlight level on system resume, so should be there when we have
>> created the backlight interface and gone when we unregistered the
>> backlight interface. It doesn't have anything to do with hotkey
>> processing.
> 
> A valid point, I did things this way because I wanted the new
> acpi_video_unregister_backlight to result in the same behavior as
> having set the acpi_video=vendor / called acpi_video_dmi_promote_vendor()
> before acpi_video_register runs.
> 
> So this means undoing what-ever acpi_video_register() does, which it
> would not have done if acpi_video_dmi_promote_vendor() came first.
> 
> If you look at the current code (so without this patch) then
> the pm notifier gets installed unconditionally by
> acpi_video_bus_register_backlight() which gets called unconditionally
> by acpi_video_bus_add(). So even with acpi_video=vendor it still gets
> installed.
> 
> acpi_video=vendor / acpi_video_dmi_promote_vendor() influences
> acpi_video_backlight_support() which only gets called by
> acpi_video_verify_backlight_support() which only gets called by
> acpi_video_dev_register_backlight(). So acpi_video=vendor only causes
> the backlight devices to not register, the pm notifier will still be
> installed. My intend was to behave the same independent of module
> loading / init order hence I moved the pm notifier install / uninstall
> to keep the pm notifier installed when calling the new
> acpi_video_unregister_backlight().
> 
> Looking at the code you're right that this is not really sensible, since
> the acpi_video_resume call done by the notifier will be a nop when there
> are no backlight devices registered.
> 
> So I can split this patch into 2 patches, 1 to not install the pm notifier
> if we're not going to register backlight devices anyways, and a 2nd patch
> adding the new acpi_video_unregister_backlight().
> 
> Does that sound like a plan ?

Yes, sounds great, thanks!

-Aaron

[RFC 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

[RFC 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