| Message ID | 1399917824-10341-2-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
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) > { > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) >> { >> > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) {
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(-)