Re: [PATCH 07/32] acpi-video-detect: Rewrite backlight interface selection logic

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
>>> ---
>>>   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@xxxxxxxxxx>
>>>    *  Copyright (C) 2008       SuSE Linux Products GmbH
>>>    *                           Thomas Renninger <trenn@xxxxxxx>
>>>    *
>>> @@ -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@xxxxxxxxxxxxxxx
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html


------------------------------------------------------------------------------
_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel



[Index of Archives]     [Linux ACPI]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Photo]     [Yosemite Photos]     [Yosemite Advice]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]

  Powered by Linux