| Message ID | 1433941292-21530-8-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
On Wed, Jun 10, 2015 at 03:01:07PM +0200, Hans de Goede wrote: > Currently we have 2 kernel commandline options + dmi-quirks in 3 places all > interacting (in interesting ways) to select which which backlight interface > to use. On the commandline we've acpi_backlight=[video|vendor] and > video.use_native_backlight=[0|1]. DMI quirks we have in > acpi/video-detect.c, acpi/video.c and drivers/platform/x86/*.c . > > This commit is the first step to cleaning this up, replacing the 2 cmdline > options with just acpi_video.backlight=[video|vendor|native|none], and > adding a new API to video_detect.c to reflect this. I like backlight=[firmware|platform|raw|none] better, but that would require to put the selection/quirk logic to backlight core. What do you think? Regards, Aaron > > Follow up commits will also move other related code, like unregistering the > acpi_video backlight interface if it was registered before other drivers > which take priority over it are loaded, to video_detect.c where this > logic really belongs. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/acpi/video_detect.c | 172 +++++++++++++++++++++----------------------- > include/acpi/video.h | 17 +++++ > 2 files changed, 100 insertions(+), 89 deletions(-) > > diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c > index 0bc8b98..01c8c46 100644 > --- a/drivers/acpi/video_detect.c > +++ b/drivers/acpi/video_detect.c > @@ -1,4 +1,6 @@ > /* > + * Copyright (C) 2015 Red Hat Inc. > + * Hans de Goede <hdegoede@redhat.com> > * Copyright (C) 2008 SuSE Linux Products GmbH > * Thomas Renninger <trenn@suse.de> > * > @@ -9,28 +11,24 @@ > * acpi_get_pci_dev() can be called to identify ACPI graphics > * devices for which a real graphics card is plugged in > * > - * Now acpi_video_get_capabilities() can be called to check which > - * capabilities the graphics cards plugged in support. The check for general > - * video capabilities will be triggered by the first caller of > - * acpi_video_get_capabilities(NULL); which will happen when the first > - * backlight switching supporting driver calls: > - * acpi_video_backlight_support(); > - * > * Depending on whether ACPI graphics extensions (cmp. ACPI spec Appendix B) > * are available, video.ko should be used to handle the device. > * > * Otherwise vendor specific drivers like thinkpad_acpi, asus-laptop, > * sony_acpi,... can take care about backlight brightness. > * > - * If CONFIG_ACPI_VIDEO is neither set as "compiled in" (y) nor as a module (m) > - * this file will not be compiled, acpi_video_get_capabilities() and > - * acpi_video_backlight_support() will always return 0 and vendor specific > - * drivers always can handle backlight. > + * Backlight drivers can use acpi_video_get_backlight_type() to determine > + * which driver should handle the backlight. > * > + * If CONFIG_ACPI_VIDEO is neither set as "compiled in" (y) nor as a module (m) > + * this file will not be compiled and acpi_video_get_backlight_type() will > + * always return acpi_backlight_vendor. > */ > > +#include <acpi/video.h> > #include <linux/export.h> > #include <linux/acpi.h> > +#include <linux/backlight.h> > #include <linux/dmi.h> > #include <linux/module.h> > #include <linux/pci.h> > @@ -38,20 +36,24 @@ > ACPI_MODULE_NAME("video"); > #define _COMPONENT ACPI_VIDEO_COMPONENT > > -static long acpi_video_support; > -static bool acpi_video_caps_checked; > +static enum acpi_backlight_type acpi_backlight_cmdline = acpi_backlight_undef; > +static enum acpi_backlight_type acpi_backlight_dmi = acpi_backlight_undef; > > static char acpi_backlight_str[16]; > module_param_string(backlight, acpi_backlight_str, 16, 0444); > MODULE_PARM_DESC(backlight, > - "Select which backlight interface to use [vendor|video]"); > + "Select which backlight interface to use [vendor|video|native]"); > > static void acpi_video_parse_cmdline(void) > { > if (!strcmp("vendor", acpi_backlight_str)) > - acpi_video_support |= ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR; > + acpi_backlight_cmdline = acpi_backlight_vendor; > if (!strcmp("video", acpi_backlight_str)) > - acpi_video_support |= ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO; > + acpi_backlight_cmdline = acpi_backlight_video; > + if (!strcmp("native", acpi_backlight_str)) > + acpi_backlight_cmdline = acpi_backlight_native; > + if (!strcmp("none", acpi_backlight_str)) > + acpi_backlight_cmdline = acpi_backlight_none; > } > > static acpi_status > @@ -82,7 +84,7 @@ find_video(acpi_handle handle, u32 lvl, void *context, void **rv) > * buggy */ > static int video_detect_force_vendor(const struct dmi_system_id *d) > { > - acpi_video_support |= ACPI_VIDEO_BACKLIGHT_DMI_VENDOR; > + acpi_backlight_dmi = acpi_backlight_vendor; > return 0; > } > > @@ -130,99 +132,91 @@ static struct dmi_system_id video_detect_dmi_table[] = { > }; > > /* > - * Returns the video capabilities of a specific ACPI graphics device > + * Determine which type of backlight interface to use on this system, > + * First check cmdline, then dmi quirks, then do autodetect. > + * > + * The autodetect order is: > + * 1) Is the acpi-video backlight interface supported -> > + * no, use a vendor interface > + * 2) Is this a win8 "ready" BIOS and do we have a native interface -> > + * yes, use a native interface > + * 3) Else use the acpi-video interface > * > - * if NULL is passed as argument all ACPI devices are enumerated and > - * all graphics capabilities of physically present devices are > - * summarized and returned. This is cached and done only once. > + * Arguably the native on win8 check should be done first, but that would > + * be a behavior change, which may causes issues. > */ > -static long acpi_video_get_capabilities(acpi_handle graphics_handle) > +enum acpi_backlight_type acpi_video_get_backlight_type(void) > { > - long caps = 0; > - struct acpi_device *tmp_dev; > - acpi_status status; > - > - if (acpi_video_caps_checked && graphics_handle == NULL) > - return acpi_video_support; > - > - if (!graphics_handle) { > - /* Only do the global walk through all graphics devices once */ > - acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, > - ACPI_UINT32_MAX, find_video, NULL, > - &caps, NULL); > - /* There might be boot param flags set already... */ > - acpi_video_support |= caps; > - acpi_video_caps_checked = 1; > - /* Add blacklists here. Be careful to use the right *DMI* bits > - * to still be able to override logic via boot params, e.g.: > - * > - * if (dmi_name_in_vendors("XY")) { > - * acpi_video_support |= > - * ACPI_VIDEO_BACKLIGHT_DMI_VENDOR; > - *} > - */ > + static DEFINE_MUTEX(init_mutex); > + static bool init_done; > + static long video_caps; > > + /* Parse cmdline, dmi and acpi only once */ > + mutex_lock(&init_mutex); > + if (!init_done) { > + acpi_video_parse_cmdline(); > dmi_check_system(video_detect_dmi_table); > - } else { > - status = acpi_bus_get_device(graphics_handle, &tmp_dev); > - if (ACPI_FAILURE(status)) { > - ACPI_EXCEPTION((AE_INFO, status, "Invalid device")); > - return 0; > - } > - acpi_walk_namespace(ACPI_TYPE_DEVICE, graphics_handle, > + acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, > ACPI_UINT32_MAX, find_video, NULL, > - &caps, NULL); > + &video_caps, NULL); > + init_done = true; > } > - ACPI_DEBUG_PRINT((ACPI_DB_INFO, "We have 0x%lX video support %s %s\n", > - graphics_handle ? caps : acpi_video_support, > - graphics_handle ? "on device " : "in general", > - graphics_handle ? acpi_device_bid(tmp_dev) : "")); > - return caps; > + mutex_unlock(&init_mutex); > + > + if (acpi_backlight_cmdline != acpi_backlight_undef) > + return acpi_backlight_cmdline; > + > + if (acpi_backlight_dmi != acpi_backlight_undef) > + return acpi_backlight_dmi; > + > + if (!(video_caps & ACPI_VIDEO_BACKLIGHT)) > + return acpi_backlight_vendor; > + > + if (acpi_osi_is_win8() && backlight_device_registered(BACKLIGHT_RAW)) > + return acpi_backlight_native; > + > + return acpi_backlight_video; > } > +EXPORT_SYMBOL(acpi_video_get_backlight_type); > > -static void acpi_video_caps_check(void) > +/* > + * Set the preferred backlight interface type based on DMI info. > + * This function allows DMI blacklists to be implemented by external > + * platform drivers instead of putting a big blacklist in video_detect.c > + */ > +void acpi_video_set_dmi_backlight_type(enum acpi_backlight_type type) > { > - /* > - * We must check whether the ACPI graphics device is physically plugged > - * in. Therefore this must be called after binding PCI and ACPI devices > - */ > - if (!acpi_video_caps_checked) { > - acpi_video_parse_cmdline(); > - acpi_video_get_capabilities(NULL); > - } > + acpi_backlight_dmi = type; > } > +EXPORT_SYMBOL(acpi_video_set_dmi_backlight_type); > > -/* Promote the vendor interface instead of the generic video module. > - * This function allow DMI blacklists to be implemented by externals > - * platform drivers instead of putting a big blacklist in video_detect.c > +/* > + * Compatiblity function, this is going away as soon as all drivers are > + * converted to acpi_video_set_dmi_backlight_type(). > + * > + * Promote the vendor interface instead of the generic video module. > * After calling this function you will probably want to call > * acpi_video_unregister() to make sure the video module is not loaded > */ > void acpi_video_dmi_promote_vendor(void) > { > - acpi_video_caps_check(); > - acpi_video_support |= ACPI_VIDEO_BACKLIGHT_DMI_VENDOR; > + acpi_video_set_dmi_backlight_type(acpi_backlight_vendor); > } > EXPORT_SYMBOL(acpi_video_dmi_promote_vendor); > > -/* Returns true if video.ko can do backlight switching */ > +/* > + * Compatiblity function, this is going away as soon as all drivers are > + * converted to acpi_video_get_backlight_type(). > + * > + * Returns true if video.ko can do backlight switching. > + */ > int acpi_video_backlight_support(void) > { > - acpi_video_caps_check(); > - > - /* First check for boot param -> highest prio */ > - if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR) > - return 0; > - else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO) > - return 1; > - > - /* Then check for DMI blacklist -> second highest prio */ > - if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VENDOR) > - return 0; > - else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VIDEO) > - return 1; > - > - /* Then go the default way */ > - return acpi_video_support & ACPI_VIDEO_BACKLIGHT; > + /* > + * This is done this way since vendor drivers call this to see > + * if they should load, and we do not want them to load for both > + * the acpi_backlight_video and acpi_backlight_native cases. > + */ > + return acpi_video_get_backlight_type() != acpi_backlight_vendor; > } > EXPORT_SYMBOL(acpi_video_backlight_support); > diff --git a/include/acpi/video.h b/include/acpi/video.h > index 843ef1a..01b5cc7 100644 > --- a/include/acpi/video.h > +++ b/include/acpi/video.h > @@ -16,6 +16,14 @@ struct acpi_device; > #define ACPI_VIDEO_DISPLAY_LEGACY_PANEL 0x0110 > #define ACPI_VIDEO_DISPLAY_LEGACY_TV 0x0200 > > +enum acpi_backlight_type { > + acpi_backlight_undef = -1, > + acpi_backlight_none = 0, > + acpi_backlight_video, > + acpi_backlight_vendor, > + acpi_backlight_native, > +}; > + > #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE) > extern int acpi_video_register(void); > extern void acpi_video_unregister(void); > @@ -23,6 +31,8 @@ extern void acpi_video_unregister_backlight(void); > extern int acpi_video_get_edid(struct acpi_device *device, int type, > int device_id, void **edid); > extern bool acpi_video_verify_backlight_support(void); > +extern enum acpi_backlight_type acpi_video_get_backlight_type(void); > +extern void acpi_video_set_dmi_backlight_type(enum acpi_backlight_type type); > #else > static inline int acpi_video_register(void) { return 0; } > static inline void acpi_video_unregister(void) { return; } > @@ -33,6 +43,13 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type, > return -ENODEV; > } > static inline bool acpi_video_verify_backlight_support(void) { return false; } > +static inline enum acpi_backlight_type acpi_video_get_backlight_type(void) > +{ > + return acpi_backlight_vendor; > +} > +static void acpi_video_set_dmi_backlight_type(enum acpi_backlight_type type) > +{ > +} > #endif > > #endif > -- > 2.4.2 > > -- > 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 -- 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 11-06-15 11:00, Aaron Lu wrote: > On Wed, Jun 10, 2015 at 03:01:07PM +0200, Hans de Goede wrote: >> Currently we have 2 kernel commandline options + dmi-quirks in 3 places all >> interacting (in interesting ways) to select which which backlight interface >> to use. On the commandline we've acpi_backlight=[video|vendor] and >> video.use_native_backlight=[0|1]. DMI quirks we have in >> acpi/video-detect.c, acpi/video.c and drivers/platform/x86/*.c . >> >> This commit is the first step to cleaning this up, replacing the 2 cmdline >> options with just acpi_video.backlight=[video|vendor|native|none], and >> adding a new API to video_detect.c to reflect this. > > I like backlight=[firmware|platform|raw|none] better, but that would > require to put the selection/quirk logic to backlight core. What do you > think? I think you are asking 3 separate questions: 1) Should the backlight selection logic maybe be moved to the backlight class code ? My answer to that one is: no it should not as it is x86/acpi specific. 2) What would be a good kernel cmdline option name for this? Given the answer to one I think it is good to keep acpi in the name space of the cmdline option. Also note that moving the code to the backlight class code does not magically give us a backlight= cmdline option. None module-name prefixed cmdline options are only available to code which is always builtin, and the backlight class code can be build as a module, so putting the cmdline option in the backlight class will give us: backlight.backlight= So all in all I think acpi_video.backlight= is actually pretty ok. 3) What would be a good kernel cmdline option values for this? I agree that the old vendor/video/native value labels are not necessarily the best labels for this, but I would like to keep them as those are what people are used to. Ideally I would like to have just kept acpi_backlight=[video|vendor|native|none] but that is not possible without making all of the acpi_video code always builtin. Regards, Hans >> Follow up commits will also move other related code, like unregistering the >> acpi_video backlight interface if it was registered before other drivers >> which take priority over it are loaded, to video_detect.c where this >> logic really belongs. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/acpi/video_detect.c | 172 +++++++++++++++++++++----------------------- >> include/acpi/video.h | 17 +++++ >> 2 files changed, 100 insertions(+), 89 deletions(-) >> >> diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c >> index 0bc8b98..01c8c46 100644 >> --- a/drivers/acpi/video_detect.c >> +++ b/drivers/acpi/video_detect.c >> @@ -1,4 +1,6 @@ >> /* >> + * Copyright (C) 2015 Red Hat Inc. >> + * Hans de Goede <hdegoede@redhat.com> >> * Copyright (C) 2008 SuSE Linux Products GmbH >> * Thomas Renninger <trenn@suse.de> >> * >> @@ -9,28 +11,24 @@ >> * acpi_get_pci_dev() can be called to identify ACPI graphics >> * devices for which a real graphics card is plugged in >> * >> - * Now acpi_video_get_capabilities() can be called to check which >> - * capabilities the graphics cards plugged in support. The check for general >> - * video capabilities will be triggered by the first caller of >> - * acpi_video_get_capabilities(NULL); which will happen when the first >> - * backlight switching supporting driver calls: >> - * acpi_video_backlight_support(); >> - * >> * Depending on whether ACPI graphics extensions (cmp. ACPI spec Appendix B) >> * are available, video.ko should be used to handle the device. >> * >> * Otherwise vendor specific drivers like thinkpad_acpi, asus-laptop, >> * sony_acpi,... can take care about backlight brightness. >> * >> - * If CONFIG_ACPI_VIDEO is neither set as "compiled in" (y) nor as a module (m) >> - * this file will not be compiled, acpi_video_get_capabilities() and >> - * acpi_video_backlight_support() will always return 0 and vendor specific >> - * drivers always can handle backlight. >> + * Backlight drivers can use acpi_video_get_backlight_type() to determine >> + * which driver should handle the backlight. >> * >> + * If CONFIG_ACPI_VIDEO is neither set as "compiled in" (y) nor as a module (m) >> + * this file will not be compiled and acpi_video_get_backlight_type() will >> + * always return acpi_backlight_vendor. >> */ >> >> +#include <acpi/video.h> >> #include <linux/export.h> >> #include <linux/acpi.h> >> +#include <linux/backlight.h> >> #include <linux/dmi.h> >> #include <linux/module.h> >> #include <linux/pci.h> >> @@ -38,20 +36,24 @@ >> ACPI_MODULE_NAME("video"); >> #define _COMPONENT ACPI_VIDEO_COMPONENT >> >> -static long acpi_video_support; >> -static bool acpi_video_caps_checked; >> +static enum acpi_backlight_type acpi_backlight_cmdline = acpi_backlight_undef; >> +static enum acpi_backlight_type acpi_backlight_dmi = acpi_backlight_undef; >> >> static char acpi_backlight_str[16]; >> module_param_string(backlight, acpi_backlight_str, 16, 0444); >> MODULE_PARM_DESC(backlight, >> - "Select which backlight interface to use [vendor|video]"); >> + "Select which backlight interface to use [vendor|video|native]"); >> >> static void acpi_video_parse_cmdline(void) >> { >> if (!strcmp("vendor", acpi_backlight_str)) >> - acpi_video_support |= ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR; >> + acpi_backlight_cmdline = acpi_backlight_vendor; >> if (!strcmp("video", acpi_backlight_str)) >> - acpi_video_support |= ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO; >> + acpi_backlight_cmdline = acpi_backlight_video; >> + if (!strcmp("native", acpi_backlight_str)) >> + acpi_backlight_cmdline = acpi_backlight_native; >> + if (!strcmp("none", acpi_backlight_str)) >> + acpi_backlight_cmdline = acpi_backlight_none; >> } >> >> static acpi_status >> @@ -82,7 +84,7 @@ find_video(acpi_handle handle, u32 lvl, void *context, void **rv) >> * buggy */ >> static int video_detect_force_vendor(const struct dmi_system_id *d) >> { >> - acpi_video_support |= ACPI_VIDEO_BACKLIGHT_DMI_VENDOR; >> + acpi_backlight_dmi = acpi_backlight_vendor; >> return 0; >> } >> >> @@ -130,99 +132,91 @@ static struct dmi_system_id video_detect_dmi_table[] = { >> }; >> >> /* >> - * Returns the video capabilities of a specific ACPI graphics device >> + * Determine which type of backlight interface to use on this system, >> + * First check cmdline, then dmi quirks, then do autodetect. >> + * >> + * The autodetect order is: >> + * 1) Is the acpi-video backlight interface supported -> >> + * no, use a vendor interface >> + * 2) Is this a win8 "ready" BIOS and do we have a native interface -> >> + * yes, use a native interface >> + * 3) Else use the acpi-video interface >> * >> - * if NULL is passed as argument all ACPI devices are enumerated and >> - * all graphics capabilities of physically present devices are >> - * summarized and returned. This is cached and done only once. >> + * Arguably the native on win8 check should be done first, but that would >> + * be a behavior change, which may causes issues. >> */ >> -static long acpi_video_get_capabilities(acpi_handle graphics_handle) >> +enum acpi_backlight_type acpi_video_get_backlight_type(void) >> { >> - long caps = 0; >> - struct acpi_device *tmp_dev; >> - acpi_status status; >> - >> - if (acpi_video_caps_checked && graphics_handle == NULL) >> - return acpi_video_support; >> - >> - if (!graphics_handle) { >> - /* Only do the global walk through all graphics devices once */ >> - acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, >> - ACPI_UINT32_MAX, find_video, NULL, >> - &caps, NULL); >> - /* There might be boot param flags set already... */ >> - acpi_video_support |= caps; >> - acpi_video_caps_checked = 1; >> - /* Add blacklists here. Be careful to use the right *DMI* bits >> - * to still be able to override logic via boot params, e.g.: >> - * >> - * if (dmi_name_in_vendors("XY")) { >> - * acpi_video_support |= >> - * ACPI_VIDEO_BACKLIGHT_DMI_VENDOR; >> - *} >> - */ >> + static DEFINE_MUTEX(init_mutex); >> + static bool init_done; >> + static long video_caps; >> >> + /* Parse cmdline, dmi and acpi only once */ >> + mutex_lock(&init_mutex); >> + if (!init_done) { >> + acpi_video_parse_cmdline(); >> dmi_check_system(video_detect_dmi_table); >> - } else { >> - status = acpi_bus_get_device(graphics_handle, &tmp_dev); >> - if (ACPI_FAILURE(status)) { >> - ACPI_EXCEPTION((AE_INFO, status, "Invalid device")); >> - return 0; >> - } >> - acpi_walk_namespace(ACPI_TYPE_DEVICE, graphics_handle, >> + acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, >> ACPI_UINT32_MAX, find_video, NULL, >> - &caps, NULL); >> + &video_caps, NULL); >> + init_done = true; >> } >> - ACPI_DEBUG_PRINT((ACPI_DB_INFO, "We have 0x%lX video support %s %s\n", >> - graphics_handle ? caps : acpi_video_support, >> - graphics_handle ? "on device " : "in general", >> - graphics_handle ? acpi_device_bid(tmp_dev) : "")); >> - return caps; >> + mutex_unlock(&init_mutex); >> + >> + if (acpi_backlight_cmdline != acpi_backlight_undef) >> + return acpi_backlight_cmdline; >> + >> + if (acpi_backlight_dmi != acpi_backlight_undef) >> + return acpi_backlight_dmi; >> + >> + if (!(video_caps & ACPI_VIDEO_BACKLIGHT)) >> + return acpi_backlight_vendor; >> + >> + if (acpi_osi_is_win8() && backlight_device_registered(BACKLIGHT_RAW)) >> + return acpi_backlight_native; >> + >> + return acpi_backlight_video; >> } >> +EXPORT_SYMBOL(acpi_video_get_backlight_type); >> >> -static void acpi_video_caps_check(void) >> +/* >> + * Set the preferred backlight interface type based on DMI info. >> + * This function allows DMI blacklists to be implemented by external >> + * platform drivers instead of putting a big blacklist in video_detect.c >> + */ >> +void acpi_video_set_dmi_backlight_type(enum acpi_backlight_type type) >> { >> - /* >> - * We must check whether the ACPI graphics device is physically plugged >> - * in. Therefore this must be called after binding PCI and ACPI devices >> - */ >> - if (!acpi_video_caps_checked) { >> - acpi_video_parse_cmdline(); >> - acpi_video_get_capabilities(NULL); >> - } >> + acpi_backlight_dmi = type; >> } >> +EXPORT_SYMBOL(acpi_video_set_dmi_backlight_type); >> >> -/* Promote the vendor interface instead of the generic video module. >> - * This function allow DMI blacklists to be implemented by externals >> - * platform drivers instead of putting a big blacklist in video_detect.c >> +/* >> + * Compatiblity function, this is going away as soon as all drivers are >> + * converted to acpi_video_set_dmi_backlight_type(). >> + * >> + * Promote the vendor interface instead of the generic video module. >> * After calling this function you will probably want to call >> * acpi_video_unregister() to make sure the video module is not loaded >> */ >> void acpi_video_dmi_promote_vendor(void) >> { >> - acpi_video_caps_check(); >> - acpi_video_support |= ACPI_VIDEO_BACKLIGHT_DMI_VENDOR; >> + acpi_video_set_dmi_backlight_type(acpi_backlight_vendor); >> } >> EXPORT_SYMBOL(acpi_video_dmi_promote_vendor); >> >> -/* Returns true if video.ko can do backlight switching */ >> +/* >> + * Compatiblity function, this is going away as soon as all drivers are >> + * converted to acpi_video_get_backlight_type(). >> + * >> + * Returns true if video.ko can do backlight switching. >> + */ >> int acpi_video_backlight_support(void) >> { >> - acpi_video_caps_check(); >> - >> - /* First check for boot param -> highest prio */ >> - if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR) >> - return 0; >> - else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO) >> - return 1; >> - >> - /* Then check for DMI blacklist -> second highest prio */ >> - if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VENDOR) >> - return 0; >> - else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VIDEO) >> - return 1; >> - >> - /* Then go the default way */ >> - return acpi_video_support & ACPI_VIDEO_BACKLIGHT; >> + /* >> + * This is done this way since vendor drivers call this to see >> + * if they should load, and we do not want them to load for both >> + * the acpi_backlight_video and acpi_backlight_native cases. >> + */ >> + return acpi_video_get_backlight_type() != acpi_backlight_vendor; >> } >> EXPORT_SYMBOL(acpi_video_backlight_support); >> diff --git a/include/acpi/video.h b/include/acpi/video.h >> index 843ef1a..01b5cc7 100644 >> --- a/include/acpi/video.h >> +++ b/include/acpi/video.h >> @@ -16,6 +16,14 @@ struct acpi_device; >> #define ACPI_VIDEO_DISPLAY_LEGACY_PANEL 0x0110 >> #define ACPI_VIDEO_DISPLAY_LEGACY_TV 0x0200 >> >> +enum acpi_backlight_type { >> + acpi_backlight_undef = -1, >> + acpi_backlight_none = 0, >> + acpi_backlight_video, >> + acpi_backlight_vendor, >> + acpi_backlight_native, >> +}; >> + >> #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE) >> extern int acpi_video_register(void); >> extern void acpi_video_unregister(void); >> @@ -23,6 +31,8 @@ extern void acpi_video_unregister_backlight(void); >> extern int acpi_video_get_edid(struct acpi_device *device, int type, >> int device_id, void **edid); >> extern bool acpi_video_verify_backlight_support(void); >> +extern enum acpi_backlight_type acpi_video_get_backlight_type(void); >> +extern void acpi_video_set_dmi_backlight_type(enum acpi_backlight_type type); >> #else >> static inline int acpi_video_register(void) { return 0; } >> static inline void acpi_video_unregister(void) { return; } >> @@ -33,6 +43,13 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type, >> return -ENODEV; >> } >> static inline bool acpi_video_verify_backlight_support(void) { return false; } >> +static inline enum acpi_backlight_type acpi_video_get_backlight_type(void) >> +{ >> + return acpi_backlight_vendor; >> +} >> +static void acpi_video_set_dmi_backlight_type(enum acpi_backlight_type type) >> +{ >> +} >> #endif >> >> #endif >> -- >> 2.4.2 >> >> -- >> 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 -- 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 Wednesday 10 June 2015 15:01:07 Hans de Goede wrote: > Currently we have 2 kernel commandline options + dmi-quirks in 3 places all > interacting (in interesting ways) to select which which backlight interface > to use. On the commandline we've acpi_backlight=[video|vendor] and > video.use_native_backlight=[0|1]. DMI quirks we have in > acpi/video-detect.c, acpi/video.c and drivers/platform/x86/*.c . > > This commit is the first step to cleaning this up, replacing the 2 cmdline > options with just acpi_video.backlight=[video|vendor|native|none], and > adding a new API to video_detect.c to reflect this. > > Follow up commits will also move other related code, like unregistering the > acpi_video backlight interface if it was registered before other drivers > which take priority over it are loaded, to video_detect.c where this > logic really belongs. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Hello, lot of people are using acpi_backlight parameter in kernel cmdline to fix some of problems. I would like to see this parameter still working and to not break existing configuration. E.g acpi_backlight=vendor to use dell-laptop.ko or thinkpad_acpi.ko backlight. It is still nightmare to get laptop panel backlight working on different (broken) laptops and people learnt to try to use acpi_backlight parameter for quit/hot fixing these problems. Upgrading kernel (if you remove acpi_backlight parameter) just break it again.
Hi, On 11-06-15 11:19, Pali Rohár wrote: > On Wednesday 10 June 2015 15:01:07 Hans de Goede wrote: >> Currently we have 2 kernel commandline options + dmi-quirks in 3 places all >> interacting (in interesting ways) to select which which backlight interface >> to use. On the commandline we've acpi_backlight=[video|vendor] and >> video.use_native_backlight=[0|1]. DMI quirks we have in >> acpi/video-detect.c, acpi/video.c and drivers/platform/x86/*.c . >> >> This commit is the first step to cleaning this up, replacing the 2 cmdline >> options with just acpi_video.backlight=[video|vendor|native|none], and >> adding a new API to video_detect.c to reflect this. >> >> Follow up commits will also move other related code, like unregistering the >> acpi_video backlight interface if it was registered before other drivers >> which take priority over it are loaded, to video_detect.c where this >> logic really belongs. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Hello, > > lot of people are using acpi_backlight parameter in kernel cmdline to > fix some of problems. I would like to see this parameter still working > and to not break existing configuration. E.g acpi_backlight=vendor to > use dell-laptop.ko or thinkpad_acpi.ko backlight. I would have like to keep acpi_backlight= for that exact same reason, but that is not possible while keeping acpi_video as a module. Thinking more about this, this is not strictly true, we could make some other (core) part of the acpi code use __setup to get the acpi_backlight= value and have that part export the value for use in the acpi_video module. This is not really pretty, but I think it may be the best way to solve this. > It is still nightmare to get laptop panel backlight working on different > (broken) laptops Well one reason it is a nightmare is because there are too many backlight drivers fighting for control and there is not an easy way to tell the kernel only register this one, this is exactly what this patch-set is trying to address :) People may still need to use a cmdline option but now there is only one option to play with. > and people learnt to try to use acpi_backlight > parameter for quit/hot fixing these problems. People who need to pass a kernel commandline option really should report a bug once they have figured out what option to use. Fedora users are getting pretty good at this as the Fedora kernel maintainers punt all such bug reports to me and I properly deal with them verifying the users solution is sane and then submitting a patch with a dmi based quirk for the users laptop upstream. > Upgrading kernel (if you > remove acpi_backlight parameter) just break it again. I think that is actually (partially) a good thing, as said people who rely on cmdline workarounds should file bugs, so that we can add a quirk. Had the users done so, the quirk would be long in place and the changing of the cmdline option name would not be an issue for them. I realize that this knife cuts both ways, and that this will cause unhappy users, as said if it had been possible to not change the cmdline option name in a clean way I would have done so. If everyone agrees with the solution I've outlined above, we can actually make it so as to not break things for users who's setup relies on acpi_backlight=foo Regards, Hans -- 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 06/11/2015 05:19 PM, Hans de Goede wrote: > Hi, > > On 11-06-15 11:00, Aaron Lu wrote: >> On Wed, Jun 10, 2015 at 03:01:07PM +0200, Hans de Goede wrote: >>> Currently we have 2 kernel commandline options + dmi-quirks in 3 places all >>> interacting (in interesting ways) to select which which backlight interface >>> to use. On the commandline we've acpi_backlight=[video|vendor] and >>> video.use_native_backlight=[0|1]. DMI quirks we have in >>> acpi/video-detect.c, acpi/video.c and drivers/platform/x86/*.c . >>> >>> This commit is the first step to cleaning this up, replacing the 2 cmdline >>> options with just acpi_video.backlight=[video|vendor|native|none], and >>> adding a new API to video_detect.c to reflect this. >> >> I like backlight=[firmware|platform|raw|none] better, but that would >> require to put the selection/quirk logic to backlight core. What do you >> think? > > I think you are asking 3 separate questions: > > 1) Should the backlight selection logic maybe be moved to the backlight > class code ? > > My answer to that one is: no it should not as it is x86/acpi specific. > > 2) What would be a good kernel cmdline option name for this? > > Given the answer to one I think it is good to keep acpi in the name > space of the cmdline option. Also note that moving the code to the > backlight class code does not magically give us a backlight= cmdline > option. None module-name prefixed cmdline options are only available > to code which is always builtin, and the backlight class code can be > build as a module, so putting the cmdline option in the backlight > class will give us: backlight.backlight= > > So all in all I think acpi_video.backlight= is actually pretty ok. > > 3) What would be a good kernel cmdline option values for this? > > I agree that the old vendor/video/native value labels are not > necessarily the best labels for this, but I would like to keep > them as those are what people are used to. Ideally I would like > to have just kept acpi_backlight=[video|vendor|native|none] > but that is not possible without making all of the acpi_video > code always builtin. OK, I don't have other comments, thanks for doing this. Regards, Aaron > > Regards, > > Hans > >>> Follow up commits will also move other related code, like unregistering the >>> acpi_video backlight interface if it was registered before other drivers >>> which take priority over it are loaded, to video_detect.c where this >>> logic really belongs. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> drivers/acpi/video_detect.c | 172 +++++++++++++++++++++----------------------- >>> include/acpi/video.h | 17 +++++ >>> 2 files changed, 100 insertions(+), 89 deletions(-) >>> >>> diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c >>> index 0bc8b98..01c8c46 100644 >>> --- a/drivers/acpi/video_detect.c >>> +++ b/drivers/acpi/video_detect.c >>> @@ -1,4 +1,6 @@ >>> /* >>> + * Copyright (C) 2015 Red Hat Inc. >>> + * Hans de Goede <hdegoede@redhat.com> >>> * Copyright (C) 2008 SuSE Linux Products GmbH >>> * Thomas Renninger <trenn@suse.de> >>> * >>> @@ -9,28 +11,24 @@ >>> * acpi_get_pci_dev() can be called to identify ACPI graphics >>> * devices for which a real graphics card is plugged in >>> * >>> - * Now acpi_video_get_capabilities() can be called to check which >>> - * capabilities the graphics cards plugged in support. The check for general >>> - * video capabilities will be triggered by the first caller of >>> - * acpi_video_get_capabilities(NULL); which will happen when the first >>> - * backlight switching supporting driver calls: >>> - * acpi_video_backlight_support(); >>> - * >>> * Depending on whether ACPI graphics extensions (cmp. ACPI spec Appendix B) >>> * are available, video.ko should be used to handle the device. >>> * >>> * Otherwise vendor specific drivers like thinkpad_acpi, asus-laptop, >>> * sony_acpi,... can take care about backlight brightness. >>> * >>> - * If CONFIG_ACPI_VIDEO is neither set as "compiled in" (y) nor as a module (m) >>> - * this file will not be compiled, acpi_video_get_capabilities() and >>> - * acpi_video_backlight_support() will always return 0 and vendor specific >>> - * drivers always can handle backlight. >>> + * Backlight drivers can use acpi_video_get_backlight_type() to determine >>> + * which driver should handle the backlight. >>> * >>> + * If CONFIG_ACPI_VIDEO is neither set as "compiled in" (y) nor as a module (m) >>> + * this file will not be compiled and acpi_video_get_backlight_type() will >>> + * always return acpi_backlight_vendor. >>> */ >>> >>> +#include <acpi/video.h> >>> #include <linux/export.h> >>> #include <linux/acpi.h> >>> +#include <linux/backlight.h> >>> #include <linux/dmi.h> >>> #include <linux/module.h> >>> #include <linux/pci.h> >>> @@ -38,20 +36,24 @@ >>> ACPI_MODULE_NAME("video"); >>> #define _COMPONENT ACPI_VIDEO_COMPONENT >>> >>> -static long acpi_video_support; >>> -static bool acpi_video_caps_checked; >>> +static enum acpi_backlight_type acpi_backlight_cmdline = acpi_backlight_undef; >>> +static enum acpi_backlight_type acpi_backlight_dmi = acpi_backlight_undef; >>> >>> static char acpi_backlight_str[16]; >>> module_param_string(backlight, acpi_backlight_str, 16, 0444); >>> MODULE_PARM_DESC(backlight, >>> - "Select which backlight interface to use [vendor|video]"); >>> + "Select which backlight interface to use [vendor|video|native]"); >>> >>> static void acpi_video_parse_cmdline(void) >>> { >>> if (!strcmp("vendor", acpi_backlight_str)) >>> - acpi_video_support |= ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR; >>> + acpi_backlight_cmdline = acpi_backlight_vendor; >>> if (!strcmp("video", acpi_backlight_str)) >>> - acpi_video_support |= ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO; >>> + acpi_backlight_cmdline = acpi_backlight_video; >>> + if (!strcmp("native", acpi_backlight_str)) >>> + acpi_backlight_cmdline = acpi_backlight_native; >>> + if (!strcmp("none", acpi_backlight_str)) >>> + acpi_backlight_cmdline = acpi_backlight_none; >>> } >>> >>> static acpi_status >>> @@ -82,7 +84,7 @@ find_video(acpi_handle handle, u32 lvl, void *context, void **rv) >>> * buggy */ >>> static int video_detect_force_vendor(const struct dmi_system_id *d) >>> { >>> - acpi_video_support |= ACPI_VIDEO_BACKLIGHT_DMI_VENDOR; >>> + acpi_backlight_dmi = acpi_backlight_vendor; >>> return 0; >>> } >>> >>> @@ -130,99 +132,91 @@ static struct dmi_system_id video_detect_dmi_table[] = { >>> }; >>> >>> /* >>> - * Returns the video capabilities of a specific ACPI graphics device >>> + * Determine which type of backlight interface to use on this system, >>> + * First check cmdline, then dmi quirks, then do autodetect. >>> + * >>> + * The autodetect order is: >>> + * 1) Is the acpi-video backlight interface supported -> >>> + * no, use a vendor interface >>> + * 2) Is this a win8 "ready" BIOS and do we have a native interface -> >>> + * yes, use a native interface >>> + * 3) Else use the acpi-video interface >>> * >>> - * if NULL is passed as argument all ACPI devices are enumerated and >>> - * all graphics capabilities of physically present devices are >>> - * summarized and returned. This is cached and done only once. >>> + * Arguably the native on win8 check should be done first, but that would >>> + * be a behavior change, which may causes issues. >>> */ >>> -static long acpi_video_get_capabilities(acpi_handle graphics_handle) >>> +enum acpi_backlight_type acpi_video_get_backlight_type(void) >>> { >>> - long caps = 0; >>> - struct acpi_device *tmp_dev; >>> - acpi_status status; >>> - >>> - if (acpi_video_caps_checked && graphics_handle == NULL) >>> - return acpi_video_support; >>> - >>> - if (!graphics_handle) { >>> - /* Only do the global walk through all graphics devices once */ >>> - acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, >>> - ACPI_UINT32_MAX, find_video, NULL, >>> - &caps, NULL); >>> - /* There might be boot param flags set already... */ >>> - acpi_video_support |= caps; >>> - acpi_video_caps_checked = 1; >>> - /* Add blacklists here. Be careful to use the right *DMI* bits >>> - * to still be able to override logic via boot params, e.g.: >>> - * >>> - * if (dmi_name_in_vendors("XY")) { >>> - * acpi_video_support |= >>> - * ACPI_VIDEO_BACKLIGHT_DMI_VENDOR; >>> - *} >>> - */ >>> + static DEFINE_MUTEX(init_mutex); >>> + static bool init_done; >>> + static long video_caps; >>> >>> + /* Parse cmdline, dmi and acpi only once */ >>> + mutex_lock(&init_mutex); >>> + if (!init_done) { >>> + acpi_video_parse_cmdline(); >>> dmi_check_system(video_detect_dmi_table); >>> - } else { >>> - status = acpi_bus_get_device(graphics_handle, &tmp_dev); >>> - if (ACPI_FAILURE(status)) { >>> - ACPI_EXCEPTION((AE_INFO, status, "Invalid device")); >>> - return 0; >>> - } >>> - acpi_walk_namespace(ACPI_TYPE_DEVICE, graphics_handle, >>> + acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, >>> ACPI_UINT32_MAX, find_video, NULL, >>> - &caps, NULL); >>> + &video_caps, NULL); >>> + init_done = true; >>> } >>> - ACPI_DEBUG_PRINT((ACPI_DB_INFO, "We have 0x%lX video support %s %s\n", >>> - graphics_handle ? caps : acpi_video_support, >>> - graphics_handle ? "on device " : "in general", >>> - graphics_handle ? acpi_device_bid(tmp_dev) : "")); >>> - return caps; >>> + mutex_unlock(&init_mutex); >>> + >>> + if (acpi_backlight_cmdline != acpi_backlight_undef) >>> + return acpi_backlight_cmdline; >>> + >>> + if (acpi_backlight_dmi != acpi_backlight_undef) >>> + return acpi_backlight_dmi; >>> + >>> + if (!(video_caps & ACPI_VIDEO_BACKLIGHT)) >>> + return acpi_backlight_vendor; >>> + >>> + if (acpi_osi_is_win8() && backlight_device_registered(BACKLIGHT_RAW)) >>> + return acpi_backlight_native; >>> + >>> + return acpi_backlight_video; >>> } >>> +EXPORT_SYMBOL(acpi_video_get_backlight_type); >>> >>> -static void acpi_video_caps_check(void) >>> +/* >>> + * Set the preferred backlight interface type based on DMI info. >>> + * This function allows DMI blacklists to be implemented by external >>> + * platform drivers instead of putting a big blacklist in video_detect.c >>> + */ >>> +void acpi_video_set_dmi_backlight_type(enum acpi_backlight_type type) >>> { >>> - /* >>> - * We must check whether the ACPI graphics device is physically plugged >>> - * in. Therefore this must be called after binding PCI and ACPI devices >>> - */ >>> - if (!acpi_video_caps_checked) { >>> - acpi_video_parse_cmdline(); >>> - acpi_video_get_capabilities(NULL); >>> - } >>> + acpi_backlight_dmi = type; >>> } >>> +EXPORT_SYMBOL(acpi_video_set_dmi_backlight_type); >>> >>> -/* Promote the vendor interface instead of the generic video module. >>> - * This function allow DMI blacklists to be implemented by externals >>> - * platform drivers instead of putting a big blacklist in video_detect.c >>> +/* >>> + * Compatiblity function, this is going away as soon as all drivers are >>> + * converted to acpi_video_set_dmi_backlight_type(). >>> + * >>> + * Promote the vendor interface instead of the generic video module. >>> * After calling this function you will probably want to call >>> * acpi_video_unregister() to make sure the video module is not loaded >>> */ >>> void acpi_video_dmi_promote_vendor(void) >>> { >>> - acpi_video_caps_check(); >>> - acpi_video_support |= ACPI_VIDEO_BACKLIGHT_DMI_VENDOR; >>> + acpi_video_set_dmi_backlight_type(acpi_backlight_vendor); >>> } >>> EXPORT_SYMBOL(acpi_video_dmi_promote_vendor); >>> >>> -/* Returns true if video.ko can do backlight switching */ >>> +/* >>> + * Compatiblity function, this is going away as soon as all drivers are >>> + * converted to acpi_video_get_backlight_type(). >>> + * >>> + * Returns true if video.ko can do backlight switching. >>> + */ >>> int acpi_video_backlight_support(void) >>> { >>> - acpi_video_caps_check(); >>> - >>> - /* First check for boot param -> highest prio */ >>> - if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR) >>> - return 0; >>> - else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO) >>> - return 1; >>> - >>> - /* Then check for DMI blacklist -> second highest prio */ >>> - if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VENDOR) >>> - return 0; >>> - else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VIDEO) >>> - return 1; >>> - >>> - /* Then go the default way */ >>> - return acpi_video_support & ACPI_VIDEO_BACKLIGHT; >>> + /* >>> + * This is done this way since vendor drivers call this to see >>> + * if they should load, and we do not want them to load for both >>> + * the acpi_backlight_video and acpi_backlight_native cases. >>> + */ >>> + return acpi_video_get_backlight_type() != acpi_backlight_vendor; >>> } >>> EXPORT_SYMBOL(acpi_video_backlight_support); >>> diff --git a/include/acpi/video.h b/include/acpi/video.h >>> index 843ef1a..01b5cc7 100644 >>> --- a/include/acpi/video.h >>> +++ b/include/acpi/video.h >>> @@ -16,6 +16,14 @@ struct acpi_device; >>> #define ACPI_VIDEO_DISPLAY_LEGACY_PANEL 0x0110 >>> #define ACPI_VIDEO_DISPLAY_LEGACY_TV 0x0200 >>> >>> +enum acpi_backlight_type { >>> + acpi_backlight_undef = -1, >>> + acpi_backlight_none = 0, >>> + acpi_backlight_video, >>> + acpi_backlight_vendor, >>> + acpi_backlight_native, >>> +}; >>> + >>> #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE) >>> extern int acpi_video_register(void); >>> extern void acpi_video_unregister(void); >>> @@ -23,6 +31,8 @@ extern void acpi_video_unregister_backlight(void); >>> extern int acpi_video_get_edid(struct acpi_device *device, int type, >>> int device_id, void **edid); >>> extern bool acpi_video_verify_backlight_support(void); >>> +extern enum acpi_backlight_type acpi_video_get_backlight_type(void); >>> +extern void acpi_video_set_dmi_backlight_type(enum acpi_backlight_type type); >>> #else >>> static inline int acpi_video_register(void) { return 0; } >>> static inline void acpi_video_unregister(void) { return; } >>> @@ -33,6 +43,13 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type, >>> return -ENODEV; >>> } >>> static inline bool acpi_video_verify_backlight_support(void) { return false; } >>> +static inline enum acpi_backlight_type acpi_video_get_backlight_type(void) >>> +{ >>> + return acpi_backlight_vendor; >>> +} >>> +static void acpi_video_set_dmi_backlight_type(enum acpi_backlight_type type) >>> +{ >>> +} >>> #endif >>> >>> #endif >>> -- >>> 2.4.2 >>> >>> -- >>> 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 -- 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 Thursday 11 June 2015 11:29:24 Hans de Goede wrote: > Hi, > > On 11-06-15 11:19, Pali Rohár wrote: > >On Wednesday 10 June 2015 15:01:07 Hans de Goede wrote: > >>Currently we have 2 kernel commandline options + dmi-quirks in 3 places all > >>interacting (in interesting ways) to select which which backlight interface > >>to use. On the commandline we've acpi_backlight=[video|vendor] and > >>video.use_native_backlight=[0|1]. DMI quirks we have in > >>acpi/video-detect.c, acpi/video.c and drivers/platform/x86/*.c . > >> > >>This commit is the first step to cleaning this up, replacing the 2 cmdline > >>options with just acpi_video.backlight=[video|vendor|native|none], and > >>adding a new API to video_detect.c to reflect this. > >> > >>Follow up commits will also move other related code, like unregistering the > >>acpi_video backlight interface if it was registered before other drivers > >>which take priority over it are loaded, to video_detect.c where this > >>logic really belongs. > >> > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > >Hello, > > > >lot of people are using acpi_backlight parameter in kernel cmdline to > >fix some of problems. I would like to see this parameter still working > >and to not break existing configuration. E.g acpi_backlight=vendor to > >use dell-laptop.ko or thinkpad_acpi.ko backlight. > > I would have like to keep acpi_backlight= for that exact same reason, > but that is not possible while keeping acpi_video as a module. > > Thinking more about this, this is not strictly true, we could make > some other (core) part of the acpi code use __setup to get the > acpi_backlight= value and have that part export the value for use > in the acpi_video module. This is not really pretty, but I think it > may be the best way to solve this. > This solution should work. It is not nice, but current mess of backlight drivers, acpi, etc, is not nice too. It will help users who already know (or read on internet/forums/discussions) acpi_backlight= parameter... > >It is still nightmare to get laptop panel backlight working on different > >(broken) laptops > > Well one reason it is a nightmare is because there are too many > backlight drivers fighting for control and there is not an easy > way to tell the kernel only register this one, this is exactly > what this patch-set is trying to address :) People may still > need to use a cmdline option but now there is only one option to > play with. > It is not only that. Sometimes there are three possible ways how to control backlight (vendor driver, acpi driver, gpu driver). And all those driver can work, but some of them works "better". E.g in my case on Dell Latitude E6440 all three drivers can register backlight interface and export it to userspace via sysfs. But the best option for *me* is to use one from dell-laptop.ko because it has good granularity of levels for controlling backlight. Intel one has too many levels and does not fit some linear distribution (which I would like to see). ACPI video.ko sometimes does not work correctly with KDE4 setting -- sometimes I need to press brightness key more times to take effect. I do not know if this is problem of KDE or ACPI or whatever. For me for my current desktop & kernel setup is the best solution to use dell-laptop.ko with acpi_backlight=vendor param. I do not have to debug or change anything, just add one param to cmdline. And I think other users (without programming knowledge) who need immediately working laptop will use same option as me. > > and people learnt to try to use acpi_backlight > > parameter for quit/hot fixing these problems. > > People who need to pass a kernel commandline option really should > report a bug once they have figured out what option to use. > This is good assumption, but do not forget that there are people who: 1) do not have time to trying find out where to report bug and how 2) do not know how to report bug 3) even do not know that they can do something (like for MS systems) > Fedora users are getting pretty good at this as the Fedora kernel > maintainers punt all such bug reports to me and I properly deal > with them verifying the users solution is sane and then submitting > a patch with a dmi based quirk for the users laptop upstream. > > > Upgrading kernel (if you > >remove acpi_backlight parameter) just break it again. > > I think that is actually (partially) a good thing, as said people > who rely on cmdline workarounds should file bugs, so that we can > add a quirk. Had the users done so, the quirk would be long in > place and the changing of the cmdline option name would not be > an issue for them. > > I realize that this knife cuts both ways, and that this will > cause unhappy users, as said if it had been possible to not change > the cmdline option name in a clean way I would have done so. > > If everyone agrees with the solution I've outlined above, we > can actually make it so as to not break things for users > who's setup relies on acpi_backlight=foo > > Regards, > > Hans
On Thu, 11 Jun 2015, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 11-06-15 11:19, Pali Rohár wrote: >> On Wednesday 10 June 2015 15:01:07 Hans de Goede wrote: >>> Currently we have 2 kernel commandline options + dmi-quirks in 3 places all >>> interacting (in interesting ways) to select which which backlight interface >>> to use. On the commandline we've acpi_backlight=[video|vendor] and >>> video.use_native_backlight=[0|1]. DMI quirks we have in >>> acpi/video-detect.c, acpi/video.c and drivers/platform/x86/*.c . >>> >>> This commit is the first step to cleaning this up, replacing the 2 cmdline >>> options with just acpi_video.backlight=[video|vendor|native|none], and >>> adding a new API to video_detect.c to reflect this. >>> >>> Follow up commits will also move other related code, like unregistering the >>> acpi_video backlight interface if it was registered before other drivers >>> which take priority over it are loaded, to video_detect.c where this >>> logic really belongs. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> >> Hello, >> >> lot of people are using acpi_backlight parameter in kernel cmdline to >> fix some of problems. I would like to see this parameter still working >> and to not break existing configuration. E.g acpi_backlight=vendor to >> use dell-laptop.ko or thinkpad_acpi.ko backlight. > > I would have like to keep acpi_backlight= for that exact same reason, > but that is not possible while keeping acpi_video as a module. > > Thinking more about this, this is not strictly true, we could make > some other (core) part of the acpi code use __setup to get the > acpi_backlight= value and have that part export the value for use > in the acpi_video module. This is not really pretty, but I think it > may be the best way to solve this. It just might be a good idea to try to not change the backlight related parameters all the time... See [1]. I probably forgot a few. BR, Jani. [1] http://nikula.org/~jani/backlight/#index11h2 > >> It is still nightmare to get laptop panel backlight working on different >> (broken) laptops > > Well one reason it is a nightmare is because there are too many > backlight drivers fighting for control and there is not an easy > way to tell the kernel only register this one, this is exactly > what this patch-set is trying to address :) People may still > need to use a cmdline option but now there is only one option to > play with. > > > and people learnt to try to use acpi_backlight > > parameter for quit/hot fixing these problems. > > People who need to pass a kernel commandline option really should > report a bug once they have figured out what option to use. > > Fedora users are getting pretty good at this as the Fedora kernel > maintainers punt all such bug reports to me and I properly deal > with them verifying the users solution is sane and then submitting > a patch with a dmi based quirk for the users laptop upstream. > > > Upgrading kernel (if you >> remove acpi_backlight parameter) just break it again. > > I think that is actually (partially) a good thing, as said people > who rely on cmdline workarounds should file bugs, so that we can > add a quirk. Had the users done so, the quirk would be long in > place and the changing of the cmdline option name would not be > an issue for them. > > I realize that this knife cuts both ways, and that this will > cause unhappy users, as said if it had been possible to not change > the cmdline option name in a clean way I would have done so. > > If everyone agrees with the solution I've outlined above, we > can actually make it so as to not break things for users > who's setup relies on acpi_backlight=foo > > Regards, > > Hans > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi, On 11-06-15 14:28, Jani Nikula wrote: > On Thu, 11 Jun 2015, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> On 11-06-15 11:19, Pali Rohár wrote: >>> On Wednesday 10 June 2015 15:01:07 Hans de Goede wrote: >>>> Currently we have 2 kernel commandline options + dmi-quirks in 3 places all >>>> interacting (in interesting ways) to select which which backlight interface >>>> to use. On the commandline we've acpi_backlight=[video|vendor] and >>>> video.use_native_backlight=[0|1]. DMI quirks we have in >>>> acpi/video-detect.c, acpi/video.c and drivers/platform/x86/*.c . >>>> >>>> This commit is the first step to cleaning this up, replacing the 2 cmdline >>>> options with just acpi_video.backlight=[video|vendor|native|none], and >>>> adding a new API to video_detect.c to reflect this. >>>> >>>> Follow up commits will also move other related code, like unregistering the >>>> acpi_video backlight interface if it was registered before other drivers >>>> which take priority over it are loaded, to video_detect.c where this >>>> logic really belongs. >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> >>> Hello, >>> >>> lot of people are using acpi_backlight parameter in kernel cmdline to >>> fix some of problems. I would like to see this parameter still working >>> and to not break existing configuration. E.g acpi_backlight=vendor to >>> use dell-laptop.ko or thinkpad_acpi.ko backlight. >> >> I would have like to keep acpi_backlight= for that exact same reason, >> but that is not possible while keeping acpi_video as a module. >> >> Thinking more about this, this is not strictly true, we could make >> some other (core) part of the acpi code use __setup to get the >> acpi_backlight= value and have that part export the value for use >> in the acpi_video module. This is not really pretty, but I think it >> may be the best way to solve this. > > It just might be a good idea to try to not change the backlight related > parameters all the time... See [1]. I probably forgot a few. Right, so as said I'm fine with adding the above work-around to the next version of the patch-set to keep acpi_backlight=[vendor|video] working as before. Regards, Hans -- 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_detect.c b/drivers/acpi/video_detect.c index 0bc8b98..01c8c46 100644 --- a/drivers/acpi/video_detect.c +++ b/drivers/acpi/video_detect.c @@ -1,4 +1,6 @@ /* + * Copyright (C) 2015 Red Hat Inc. + * Hans de Goede <hdegoede@redhat.com> * Copyright (C) 2008 SuSE Linux Products GmbH * Thomas Renninger <trenn@suse.de> * @@ -9,28 +11,24 @@ * acpi_get_pci_dev() can be called to identify ACPI graphics * devices for which a real graphics card is plugged in * - * Now acpi_video_get_capabilities() can be called to check which - * capabilities the graphics cards plugged in support. The check for general - * video capabilities will be triggered by the first caller of - * acpi_video_get_capabilities(NULL); which will happen when the first - * backlight switching supporting driver calls: - * acpi_video_backlight_support(); - * * Depending on whether ACPI graphics extensions (cmp. ACPI spec Appendix B) * are available, video.ko should be used to handle the device. * * Otherwise vendor specific drivers like thinkpad_acpi, asus-laptop, * sony_acpi,... can take care about backlight brightness. * - * If CONFIG_ACPI_VIDEO is neither set as "compiled in" (y) nor as a module (m) - * this file will not be compiled, acpi_video_get_capabilities() and - * acpi_video_backlight_support() will always return 0 and vendor specific - * drivers always can handle backlight. + * Backlight drivers can use acpi_video_get_backlight_type() to determine + * which driver should handle the backlight. * + * If CONFIG_ACPI_VIDEO is neither set as "compiled in" (y) nor as a module (m) + * this file will not be compiled and acpi_video_get_backlight_type() will + * always return acpi_backlight_vendor. */ +#include <acpi/video.h> #include <linux/export.h> #include <linux/acpi.h> +#include <linux/backlight.h> #include <linux/dmi.h> #include <linux/module.h> #include <linux/pci.h> @@ -38,20 +36,24 @@ ACPI_MODULE_NAME("video"); #define _COMPONENT ACPI_VIDEO_COMPONENT -static long acpi_video_support; -static bool acpi_video_caps_checked; +static enum acpi_backlight_type acpi_backlight_cmdline = acpi_backlight_undef; +static enum acpi_backlight_type acpi_backlight_dmi = acpi_backlight_undef; static char acpi_backlight_str[16]; module_param_string(backlight, acpi_backlight_str, 16, 0444); MODULE_PARM_DESC(backlight, - "Select which backlight interface to use [vendor|video]"); + "Select which backlight interface to use [vendor|video|native]"); static void acpi_video_parse_cmdline(void) { if (!strcmp("vendor", acpi_backlight_str)) - acpi_video_support |= ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR; + acpi_backlight_cmdline = acpi_backlight_vendor; if (!strcmp("video", acpi_backlight_str)) - acpi_video_support |= ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO; + acpi_backlight_cmdline = acpi_backlight_video; + if (!strcmp("native", acpi_backlight_str)) + acpi_backlight_cmdline = acpi_backlight_native; + if (!strcmp("none", acpi_backlight_str)) + acpi_backlight_cmdline = acpi_backlight_none; } static acpi_status @@ -82,7 +84,7 @@ find_video(acpi_handle handle, u32 lvl, void *context, void **rv) * buggy */ static int video_detect_force_vendor(const struct dmi_system_id *d) { - acpi_video_support |= ACPI_VIDEO_BACKLIGHT_DMI_VENDOR; + acpi_backlight_dmi = acpi_backlight_vendor; return 0; } @@ -130,99 +132,91 @@ static struct dmi_system_id video_detect_dmi_table[] = { }; /* - * Returns the video capabilities of a specific ACPI graphics device + * Determine which type of backlight interface to use on this system, + * First check cmdline, then dmi quirks, then do autodetect. + * + * The autodetect order is: + * 1) Is the acpi-video backlight interface supported -> + * no, use a vendor interface + * 2) Is this a win8 "ready" BIOS and do we have a native interface -> + * yes, use a native interface + * 3) Else use the acpi-video interface * - * if NULL is passed as argument all ACPI devices are enumerated and - * all graphics capabilities of physically present devices are - * summarized and returned. This is cached and done only once. + * Arguably the native on win8 check should be done first, but that would + * be a behavior change, which may causes issues. */ -static long acpi_video_get_capabilities(acpi_handle graphics_handle) +enum acpi_backlight_type acpi_video_get_backlight_type(void) { - long caps = 0; - struct acpi_device *tmp_dev; - acpi_status status; - - if (acpi_video_caps_checked && graphics_handle == NULL) - return acpi_video_support; - - if (!graphics_handle) { - /* Only do the global walk through all graphics devices once */ - acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, - ACPI_UINT32_MAX, find_video, NULL, - &caps, NULL); - /* There might be boot param flags set already... */ - acpi_video_support |= caps; - acpi_video_caps_checked = 1; - /* Add blacklists here. Be careful to use the right *DMI* bits - * to still be able to override logic via boot params, e.g.: - * - * if (dmi_name_in_vendors("XY")) { - * acpi_video_support |= - * ACPI_VIDEO_BACKLIGHT_DMI_VENDOR; - *} - */ + static DEFINE_MUTEX(init_mutex); + static bool init_done; + static long video_caps; + /* Parse cmdline, dmi and acpi only once */ + mutex_lock(&init_mutex); + if (!init_done) { + acpi_video_parse_cmdline(); dmi_check_system(video_detect_dmi_table); - } else { - status = acpi_bus_get_device(graphics_handle, &tmp_dev); - if (ACPI_FAILURE(status)) { - ACPI_EXCEPTION((AE_INFO, status, "Invalid device")); - return 0; - } - acpi_walk_namespace(ACPI_TYPE_DEVICE, graphics_handle, + acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, ACPI_UINT32_MAX, find_video, NULL, - &caps, NULL); + &video_caps, NULL); + init_done = true; } - ACPI_DEBUG_PRINT((ACPI_DB_INFO, "We have 0x%lX video support %s %s\n", - graphics_handle ? caps : acpi_video_support, - graphics_handle ? "on device " : "in general", - graphics_handle ? acpi_device_bid(tmp_dev) : "")); - return caps; + mutex_unlock(&init_mutex); + + if (acpi_backlight_cmdline != acpi_backlight_undef) + return acpi_backlight_cmdline; + + if (acpi_backlight_dmi != acpi_backlight_undef) + return acpi_backlight_dmi; + + if (!(video_caps & ACPI_VIDEO_BACKLIGHT)) + return acpi_backlight_vendor; + + if (acpi_osi_is_win8() && backlight_device_registered(BACKLIGHT_RAW)) + return acpi_backlight_native; + + return acpi_backlight_video; } +EXPORT_SYMBOL(acpi_video_get_backlight_type); -static void acpi_video_caps_check(void) +/* + * Set the preferred backlight interface type based on DMI info. + * This function allows DMI blacklists to be implemented by external + * platform drivers instead of putting a big blacklist in video_detect.c + */ +void acpi_video_set_dmi_backlight_type(enum acpi_backlight_type type) { - /* - * We must check whether the ACPI graphics device is physically plugged - * in. Therefore this must be called after binding PCI and ACPI devices - */ - if (!acpi_video_caps_checked) { - acpi_video_parse_cmdline(); - acpi_video_get_capabilities(NULL); - } + acpi_backlight_dmi = type; } +EXPORT_SYMBOL(acpi_video_set_dmi_backlight_type); -/* Promote the vendor interface instead of the generic video module. - * This function allow DMI blacklists to be implemented by externals - * platform drivers instead of putting a big blacklist in video_detect.c +/* + * Compatiblity function, this is going away as soon as all drivers are + * converted to acpi_video_set_dmi_backlight_type(). + * + * Promote the vendor interface instead of the generic video module. * After calling this function you will probably want to call * acpi_video_unregister() to make sure the video module is not loaded */ void acpi_video_dmi_promote_vendor(void) { - acpi_video_caps_check(); - acpi_video_support |= ACPI_VIDEO_BACKLIGHT_DMI_VENDOR; + acpi_video_set_dmi_backlight_type(acpi_backlight_vendor); } EXPORT_SYMBOL(acpi_video_dmi_promote_vendor); -/* Returns true if video.ko can do backlight switching */ +/* + * Compatiblity function, this is going away as soon as all drivers are + * converted to acpi_video_get_backlight_type(). + * + * Returns true if video.ko can do backlight switching. + */ int acpi_video_backlight_support(void) { - acpi_video_caps_check(); - - /* First check for boot param -> highest prio */ - if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR) - return 0; - else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO) - return 1; - - /* Then check for DMI blacklist -> second highest prio */ - if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VENDOR) - return 0; - else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VIDEO) - return 1; - - /* Then go the default way */ - return acpi_video_support & ACPI_VIDEO_BACKLIGHT; + /* + * This is done this way since vendor drivers call this to see + * if they should load, and we do not want them to load for both + * the acpi_backlight_video and acpi_backlight_native cases. + */ + return acpi_video_get_backlight_type() != acpi_backlight_vendor; } EXPORT_SYMBOL(acpi_video_backlight_support); diff --git a/include/acpi/video.h b/include/acpi/video.h index 843ef1a..01b5cc7 100644 --- a/include/acpi/video.h +++ b/include/acpi/video.h @@ -16,6 +16,14 @@ struct acpi_device; #define ACPI_VIDEO_DISPLAY_LEGACY_PANEL 0x0110 #define ACPI_VIDEO_DISPLAY_LEGACY_TV 0x0200 +enum acpi_backlight_type { + acpi_backlight_undef = -1, + acpi_backlight_none = 0, + acpi_backlight_video, + acpi_backlight_vendor, + acpi_backlight_native, +}; + #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE) extern int acpi_video_register(void); extern void acpi_video_unregister(void); @@ -23,6 +31,8 @@ extern void acpi_video_unregister_backlight(void); extern int acpi_video_get_edid(struct acpi_device *device, int type, int device_id, void **edid); extern bool acpi_video_verify_backlight_support(void); +extern enum acpi_backlight_type acpi_video_get_backlight_type(void); +extern void acpi_video_set_dmi_backlight_type(enum acpi_backlight_type type); #else static inline int acpi_video_register(void) { return 0; } static inline void acpi_video_unregister(void) { return; } @@ -33,6 +43,13 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type, return -ENODEV; } static inline bool acpi_video_verify_backlight_support(void) { return false; } +static inline enum acpi_backlight_type acpi_video_get_backlight_type(void) +{ + return acpi_backlight_vendor; +} +static void acpi_video_set_dmi_backlight_type(enum acpi_backlight_type type) +{ +} #endif #endif
Currently we have 2 kernel commandline options + dmi-quirks in 3 places all interacting (in interesting ways) to select which which backlight interface to use. On the commandline we've acpi_backlight=[video|vendor] and video.use_native_backlight=[0|1]. DMI quirks we have in acpi/video-detect.c, acpi/video.c and drivers/platform/x86/*.c . This commit is the first step to cleaning this up, replacing the 2 cmdline options with just acpi_video.backlight=[video|vendor|native|none], and adding a new API to video_detect.c to reflect this. Follow up commits will also move other related code, like unregistering the acpi_video backlight interface if it was registered before other drivers which take priority over it are loaded, to video_detect.c where this logic really belongs. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/acpi/video_detect.c | 172 +++++++++++++++++++++----------------------- include/acpi/video.h | 17 +++++ 2 files changed, 100 insertions(+), 89 deletions(-)