Re: [PATCH v3 1/3] ACPI: video: Allow GPU drivers to report no panels
On Thu, Dec 08, 2022 at 10:42:05AM -0600, Mario Limonciello wrote:
> The current logic for the ACPI backlight detection will create
> a backlight device if no native or vendor drivers have created
> 8 seconds after the system has booted if the ACPI tables
> included backlight control methods.
>
> If the GPU drivers have loaded, they may be able to report whether
> any LCD panels were found. Allow using this information to factor
> in whether to enable the fallback logic for making an acpi_video0
> backlight device.
>
> Suggested-by: Hans de Goede
> Signed-off-by: Mario Limonciello
> Reviewed-by: Hans de Goede
> ---
> v2->v3:
> * Add Hans' R-b
> * Add missing declaration for non CONFIG_ACPI_VIDEO case
> v1->v2:
> * Cancel registration for backlight device instead (Hans)
> * drop desktop check (Dan)
> ---
> drivers/acpi/acpi_video.c | 11 +++
> include/acpi/video.h | 2 ++
> 2 files changed, 13 insertions(+)
>
> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> index 32953646caeb..f64fdb029090 100644
> --- a/drivers/acpi/acpi_video.c
> +++ b/drivers/acpi/acpi_video.c
> @@ -2178,6 +2178,17 @@ static bool should_check_lcd_flag(void)
> return false;
> }
>
> +/*
> + * At least one graphics driver has reported that no LCD is connected
> + * via the native interface. cancel the registration for fallback
> acpi_video0.
> + * If another driver still deems this necessary, it can explicitly register
> it.
> + */
> +void acpi_video_report_nolcd(void)
> +{
> + cancel_delayed_work(_bus_register_backlight_work);
> +}
> +EXPORT_SYMBOL(acpi_video_report_nolcd);
> +
Thanks for removing the desktop check.
It's not entirely clear to me what happens if you try to cancel a
delayed work that was never scheduled. I got as far as determining that
del_timer() in kernel/time/timer.c will probably return 0, but I didn't
really feel like walking through the rest of try_to_grab_pending() to
figure out what happens next. You've probably already tested this with
the default disabled timer, so as long as nothing bad happened there,
this seems fine.
This is probably overly complicated, so if you think it's worth doing, I
would definitely add it later, but I wonder if it might make sense to
pass an acpi_handle to a _BC[LM] method or one of its parents, so that
this could be scoped to a particular device. Looking at the ACPI table
dump from a random multi-GPU laptop, it looks like there are two
instances of _BCL, one under _SB.GP.VGA.LCD for the iGPU, and
the other under _SB.PCI.GPP.PEGP.EDP for the dGPU. The
caller would pass in handles for methods/devices that it will handle,
and then the fallback, if it runs at all, would skip any handles that
were registered with it when it crawls for _BC[LM]. Or the equivalent
inverse logic, or something else like that. I think it's probably fine
to keep the current unscoped design and just assert that any other GPU
drivers that want the ACPI video driver to handle panel backlight should
register it explicitly; if for some reason that ends up not working out,
we could revisit scoping it then.
> int acpi_video_register(void)
> {
> int ret = 0;
> diff --git a/include/acpi/video.h b/include/acpi/video.h
> index a275c35e5249..a56c8d45e9f8 100644
> --- a/include/acpi/video.h
> +++ b/include/acpi/video.h
> @@ -53,6 +53,7 @@ enum acpi_backlight_type {
> };
>
> #if IS_ENABLED(CONFIG_ACPI_VIDEO)
> +extern void acpi_video_report_nolcd(void);
> extern int acpi_video_register(void);
> extern void acpi_video_unregister(void);
> extern void acpi_video_register_backlight(void);
> @@ -69,6 +70,7 @@ extern int acpi_video_get_levels(struct acpi_device *device,
>struct acpi_video_device_brightness **dev_br,
>int *pmax_level);
> #else
> +static inline void acpi_video_report_nolcd(void) { return; };
> static inline int acpi_video_register(void) { return -ENODEV; }
> static inline void acpi_video_unregister(void) { return; }
> static inline void acpi_video_register_backlight(void) { return; }
> --
> 2.34.1
>
Re: [PATCH 0/2] Avoid creating acpi_video0 on desktop APUs
On Wed, Dec 07, 2022 at 10:32:05PM +0100, Hans de Goede wrote: > Hi, > > On 12/7/22 22:21, Limonciello, Mario wrote: > > On 12/7/2022 15:04, Hans de Goede wrote: > >> Hi All, > >> > >> Mario, thank you for working on this. > > > > Sure > > > > > >> > >> Note that the problem of the creating a non functional acpi_video0 > >> device happened before the overhaul too. > >> > >> The difference is that now we have the in kernel GPU drivers > >> all call acpi_video_register_backlight() when they detect an > >> internal-panel for which they don't provide native-backlight > >> control themselves (to avoid the acpi_video0 backlight registering > >> before the native backlight gets a chance to register). > >> > >> The timeout is only there in case no drivers ever call > >> acpi_video_register_backlight(). nomodeset is one case, but > >> loosing backlight control in the nomodeset case would be fine > >> IMHO. The bigger worry why we have the timeout is because of > >> the nvidia binary driver, for devices which use that driver + > >> rely on apci_video# for backlight control. > >> > >> Back to the issue at hand of the unwanted (non functional) > >> apci_video# on various AMD APU using desktops. > >> > > > > Thanks for explaining. > > > >> The native drivers now all calling acpi_video_register_backlight() > >> gives us a chance to actually do something about it, so in that > >> sense the 6.1 backlight refactor is relevant. > >> > >>> To avoid this situation from happening add support for video drivers > >>> to notify the ACPI video detection code that no panel was detected. > >>> > >>> To reduce the risk of regressions on multi-GPU systems: > >>> * only use this logic when the system is reported as a desktop enclosure. > >>> * in the amdgpu code only report into this for APUs. > >> > >> I'm afraid that there still is a potential issue for dual > >> GPU machines. The chassistype is not 100% reliable. > > > > Have you ever seen an A+N machine with unreliable chassis type? > > Not specifically. I just know from experience to not > rely on chassis type. > > E.g. I would not be surprised to have some of the desktop-replacement > class laptops from e.g. clevo which sometimes even come with > a desktop CPU for moar power, have their chassis type wrong. > > Granted those are not using AMD APUs (yet), but that might change > with the ryzen 7000 series where every CPU is an APU too... > > > Given Windows HLK certification and knowing that these are to > > be based off reference BIOS of laptops, I would be really surprised > > if this was wrong on an A+N laptop. > > I agree this is unlikely. But I have seen all sort of wrong > chassis-type settings in devices which are not from the > big OEMs. And AFAIK these sometimes also play fasr and loose > with the Windows certification. > > >> Lets say we have a machine with the wrong chassis-type with > >> an AMD APU + nvidia GPU which relies on acpi_video0 for > >> backlight control. > >> > >> Then if the LCD is connected to the nvidia GPU only, the > >> amdgpu code will call the new acpi_video_report_nolcd() > >> function. > > > > + Dan Dadap > > > > Dan - the context is this series: > > https://patchwork.freedesktop.org/series/111745/ > > > > Do you know if this is real or just conceptual? I'm not aware of any specific examples of an A+N system with the incorrect chassis type, but I agree that relying on it to be accurate seems a bit fragile. Besides the "notebook that says it's a desktop" possibility that Hans speculated on, I could imagine e.g. an All-in-One form factor system, whose design is more similar to a notebook, reporting its chassis type as desktop. > >> > >> And then even if the nvidia binary driver is patched > >> by nvidia to call the new acpi_video_register_backlight() > >> when it does see a panel, then acpi_video_should_register_backlight() > >> will still return false. > >> > >> Basically the problem is that we only want to not try > >> and register the acpi_video0 backlight on dual GPU > >> machines if the output detection on *both* GPUs has not > >> found any builtin LCD panel. > >> > >> But this series disables acpi_video0 backlight registration > >> as soon as *one* of the *two* GPUs has not found an internal > >> LCD panel. Yeah, it does seem a little backwards to have the drivers report that they do not see any panels, when we don't know whether there might be a panel on another GPU whose driver hasn't registered its native backlight handler yet. I trust the DRM drivers reporting whether a panel is internal more than I'd trust the DMI chassis type - "positive" reporting when a GPU driver finds an internal panel seems like it would be more reliable than "negative" reporting when a GPU driver does not find any internal panels. IIUC, that's the intent with having the DRM-KMS drivers explicitly call acpi_video_register_backlight() if needed, as described below. > >> As discussed above, after the backlight refactor, > >> GPU(KMS) drivers are
Re: [PATCH v5 17/31] ACPI: video: Add Nvidia WMI EC brightness control detection (v3)
On 8/29/22 06:41, Hans de Goede wrote:
Hi,
On 8/26/22 00:21, Daniel Dadap wrote:
On 8/25/22 9:37 AM, Hans de Goede wrote:
On some new laptop designs a new Nvidia specific WMI interface is present
which gives info about panel brightness control and may allow controlling
the brightness through this interface when the embedded controller is used
for brightness control.
When this WMI interface is present and indicates that the EC is used,
then this interface should be used for brightness control.
Changes in v2:
- Use the new shared nvidia-wmi-ec-backlight.h header for the
WMI firmware API definitions
- ACPI_VIDEO can now be enabled on non X86 too,
adjust the Kconfig changes to match this.
Changes in v3:
- Use WMI_BRIGHTNESS_GUID define
Acked-by: Rafael J. Wysocki
Signed-off-by: Hans de Goede
---
drivers/acpi/Kconfig | 1 +
drivers/acpi/video_detect.c | 37 ++
drivers/gpu/drm/gma500/Kconfig | 2 ++
drivers/gpu/drm/i915/Kconfig | 2 ++
include/acpi/video.h | 1 +
5 files changed, 43 insertions(+)
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 7802d8846a8d..44ad4b6bd234 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -212,6 +212,7 @@ config ACPI_VIDEO
tristate "Video"
depends on BACKLIGHT_CLASS_DEVICE
depends on INPUT
+ depends on ACPI_WMI || !X86
select THERMAL
help
This driver implements the ACPI Extensions For Display Adapters
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index cc9d0d91e268..4dc7fb865083 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -32,6 +32,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -75,6 +76,36 @@ find_video(acpi_handle handle, u32 lvl, void *context, void
**rv)
return AE_OK;
}
+/* This depends on ACPI_WMI which is X86 only */
+#ifdef CONFIG_X86
This could probably also provide the { return false; } stub which you have for
non-x86 if the kernel is built without nvidia-wmi-ec-backight, e.g.:
#if defined(CONFIG_X86) && (defined(CONFIG_NVIDIA_WMI_EC_BACKLIGHT) ||
defined(CONFIG_NVIDIA_WMI_EC_BACKLIGHT_MODULE))
Although I suppose that would break things if somebody has a kernel that
originally had NVIDIA_WMI_EC_BACKLIGHT=n in Kconfig, and then builds the
nvidia-wmi-ec-backlight driver out-of-tree later. I don't know whether that's
intended to be a supported use case, so I guess it is fine either way.
The video-detect code is about detecting what interface should be used.
So far it does this independently of the driver implementing that interface
actually being enabled or not.
If someone has a system which needs the nvidia-wmi-ec-backlight driver,
but it is disabled then they / their distro should enable that driver,
rather then trying to fallback on e.g. acpi_video.
Taking which drivers are enabled into account would both make
the code more complicated and would also explode the test matrix.
All of this is already somewhat fragile, so lets not make it
extra complicated :)
That is fair.
Reviewed-by: Daniel Dadap
Regards,
Hans
+static bool nvidia_wmi_ec_supported(void)
+{
+ struct wmi_brightness_args args = {
+ .mode = WMI_BRIGHTNESS_MODE_GET,
+ .val = 0,
+ .ret = 0,
+ };
+ struct acpi_buffer buf = { (acpi_size)sizeof(args), };
+ acpi_status status;
+
+ status = wmi_evaluate_method(WMI_BRIGHTNESS_GUID, 0,
+ WMI_BRIGHTNESS_METHOD_SOURCE, , );
+ if (ACPI_FAILURE(status))
+ return false;
+
+ /*
+ * If brightness is handled by the EC then nvidia-wmi-ec-backlight
+ * should be used, else the GPU driver(s) should be used.
+ */
+ return args.ret == WMI_BRIGHTNESS_SOURCE_EC;
+}
+#else
+static bool nvidia_wmi_ec_supported(void)
+{
+ return false;
+}
+#endif
+
/* Force to use vendor driver when the ACPI device is known to be
* buggy */
static int video_detect_force_vendor(const struct dmi_system_id *d)
@@ -541,6 +572,7 @@ static const struct dmi_system_id video_detect_dmi_table[]
= {
static enum acpi_backlight_type __acpi_video_get_backlight_type(bool native)
{
static DEFINE_MUTEX(init_mutex);
+ static bool nvidia_wmi_ec_present;
static bool native_available;
static bool init_done;
static long video_caps;
@@ -553,6 +585,7 @@ static enum acpi_backlight_type
__acpi_video_get_backlight_type(bool native)
acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
ACPI_UINT32_MAX, find_video, NULL,
_caps, NULL);
+ nvidia_wmi_ec_present = nvidia_wmi_ec_supported();
init_done = true;
}
if (native)
@@ -570,6 +603,10 @@ static enum acpi_backlight_type
__acpi_video_get_backlight_type(bool native)
if (acpi_backlight_dmi != acpi_backlight_undef)
Re: [PATCH v5 17/31] ACPI: video: Add Nvidia WMI EC brightness control detection (v3)
On 8/25/22 9:37 AM, Hans de Goede wrote:
On some new laptop designs a new Nvidia specific WMI interface is present
which gives info about panel brightness control and may allow controlling
the brightness through this interface when the embedded controller is used
for brightness control.
When this WMI interface is present and indicates that the EC is used,
then this interface should be used for brightness control.
Changes in v2:
- Use the new shared nvidia-wmi-ec-backlight.h header for the
WMI firmware API definitions
- ACPI_VIDEO can now be enabled on non X86 too,
adjust the Kconfig changes to match this.
Changes in v3:
- Use WMI_BRIGHTNESS_GUID define
Acked-by: Rafael J. Wysocki
Signed-off-by: Hans de Goede
---
drivers/acpi/Kconfig | 1 +
drivers/acpi/video_detect.c| 37 ++
drivers/gpu/drm/gma500/Kconfig | 2 ++
drivers/gpu/drm/i915/Kconfig | 2 ++
include/acpi/video.h | 1 +
5 files changed, 43 insertions(+)
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 7802d8846a8d..44ad4b6bd234 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -212,6 +212,7 @@ config ACPI_VIDEO
tristate "Video"
depends on BACKLIGHT_CLASS_DEVICE
depends on INPUT
+ depends on ACPI_WMI || !X86
select THERMAL
help
This driver implements the ACPI Extensions For Display Adapters
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index cc9d0d91e268..4dc7fb865083 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -32,6 +32,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -75,6 +76,36 @@ find_video(acpi_handle handle, u32 lvl, void *context, void
**rv)
return AE_OK;
}
+/* This depends on ACPI_WMI which is X86 only */
+#ifdef CONFIG_X86
This could probably also provide the { return false; } stub which you
have for non-x86 if the kernel is built without nvidia-wmi-ec-backight,
e.g.:
#if defined(CONFIG_X86) && (defined(CONFIG_NVIDIA_WMI_EC_BACKLIGHT) ||
defined(CONFIG_NVIDIA_WMI_EC_BACKLIGHT_MODULE))
Although I suppose that would break things if somebody has a kernel that
originally had NVIDIA_WMI_EC_BACKLIGHT=n in Kconfig, and then builds the
nvidia-wmi-ec-backlight driver out-of-tree later. I don't know whether
that's intended to be a supported use case, so I guess it is fine either
way.
+static bool nvidia_wmi_ec_supported(void)
+{
+ struct wmi_brightness_args args = {
+ .mode = WMI_BRIGHTNESS_MODE_GET,
+ .val = 0,
+ .ret = 0,
+ };
+ struct acpi_buffer buf = { (acpi_size)sizeof(args), };
+ acpi_status status;
+
+ status = wmi_evaluate_method(WMI_BRIGHTNESS_GUID, 0,
+WMI_BRIGHTNESS_METHOD_SOURCE, , );
+ if (ACPI_FAILURE(status))
+ return false;
+
+ /*
+* If brightness is handled by the EC then nvidia-wmi-ec-backlight
+* should be used, else the GPU driver(s) should be used.
+*/
+ return args.ret == WMI_BRIGHTNESS_SOURCE_EC;
+}
+#else
+static bool nvidia_wmi_ec_supported(void)
+{
+ return false;
+}
+#endif
+
/* Force to use vendor driver when the ACPI device is known to be
* buggy */
static int video_detect_force_vendor(const struct dmi_system_id *d)
@@ -541,6 +572,7 @@ static const struct dmi_system_id video_detect_dmi_table[]
= {
static enum acpi_backlight_type __acpi_video_get_backlight_type(bool native)
{
static DEFINE_MUTEX(init_mutex);
+ static bool nvidia_wmi_ec_present;
static bool native_available;
static bool init_done;
static long video_caps;
@@ -553,6 +585,7 @@ static enum acpi_backlight_type
__acpi_video_get_backlight_type(bool native)
acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
ACPI_UINT32_MAX, find_video, NULL,
_caps, NULL);
+ nvidia_wmi_ec_present = nvidia_wmi_ec_supported();
init_done = true;
}
if (native)
@@ -570,6 +603,10 @@ static enum acpi_backlight_type
__acpi_video_get_backlight_type(bool native)
if (acpi_backlight_dmi != acpi_backlight_undef)
return acpi_backlight_dmi;
+ /* Special cases such as nvidia_wmi_ec and apple gmux. */
+ if (nvidia_wmi_ec_present)
+ return acpi_backlight_nvidia_wmi_ec;
+
/* On systems with ACPI video use either native or ACPI video. */
if (video_caps & ACPI_VIDEO_BACKLIGHT) {
/*
diff --git a/drivers/gpu/drm/gma500/Kconfig b/drivers/gpu/drm/gma500/Kconfig
index 0cff20265f97..807b989e3c77 100644
--- a/drivers/gpu/drm/gma500/Kconfig
+++ b/drivers/gpu/drm/gma500/Kconfig
@@ -7,6 +7,8 @@ config DRM_GMA500
select ACPI_VIDEO if ACPI
select
Re: [PATCH v5 15/31] platform/x86: nvidia-wmi-ec-backlight: Move fw interface definitions to a header (v2)
Thanks, Hans.
Reviewed-by: Daniel Dadap
On 8/25/22 9:37 AM, Hans de Goede wrote:
Move the WMI interface definitions to a header, so that the definitions
can be shared with drivers/acpi/video_detect.c .
Changes in v2:
- Add missing Nvidia copyright header
- Move WMI_BRIGHTNESS_GUID to nvidia-wmi-ec-backlight.h as well
Suggested-by: Daniel Dadap
Signed-off-by: Hans de Goede
---
MAINTAINERS | 1 +
.../platform/x86/nvidia-wmi-ec-backlight.c| 68 +
.../x86/nvidia-wmi-ec-backlight.h | 76 +++
3 files changed, 78 insertions(+), 67 deletions(-)
create mode 100644 include/linux/platform_data/x86/nvidia-wmi-ec-backlight.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 9d7f64dc0efe..d6f6b96f51f7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14527,6 +14527,7 @@ M: Daniel Dadap
L:platform-driver-...@vger.kernel.org
S:Supported
F:drivers/platform/x86/nvidia-wmi-ec-backlight.c
+F: include/linux/platform_data/x86/nvidia-wmi-ec-backlight.h
NVM EXPRESS DRIVER
M:Keith Busch
diff --git a/drivers/platform/x86/nvidia-wmi-ec-backlight.c
b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
index 61e37194df70..be803e47eac0 100644
--- a/drivers/platform/x86/nvidia-wmi-ec-backlight.c
+++ b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
@@ -7,74 +7,10 @@
#include
#include
#include
+#include
#include
#include
-/**
- * enum wmi_brightness_method - WMI method IDs
- * @WMI_BRIGHTNESS_METHOD_LEVEL: Get/Set EC brightness level status
- * @WMI_BRIGHTNESS_METHOD_SOURCE: Get/Set EC Brightness Source
- */
-enum wmi_brightness_method {
- WMI_BRIGHTNESS_METHOD_LEVEL = 1,
- WMI_BRIGHTNESS_METHOD_SOURCE = 2,
- WMI_BRIGHTNESS_METHOD_MAX
-};
-
-/**
- * enum wmi_brightness_mode - Operation mode for WMI-wrapped method
- * @WMI_BRIGHTNESS_MODE_GET:Get the current brightness
level/source.
- * @WMI_BRIGHTNESS_MODE_SET:Set the brightness level.
- * @WMI_BRIGHTNESS_MODE_GET_MAX_LEVEL: Get the maximum brightness level. This
- * is only valid when the WMI method is
- * %WMI_BRIGHTNESS_METHOD_LEVEL.
- */
-enum wmi_brightness_mode {
- WMI_BRIGHTNESS_MODE_GET = 0,
- WMI_BRIGHTNESS_MODE_SET = 1,
- WMI_BRIGHTNESS_MODE_GET_MAX_LEVEL = 2,
- WMI_BRIGHTNESS_MODE_MAX
-};
-
-/**
- * enum wmi_brightness_source - Backlight brightness control source selection
- * @WMI_BRIGHTNESS_SOURCE_GPU: Backlight brightness is controlled by the GPU.
- * @WMI_BRIGHTNESS_SOURCE_EC: Backlight brightness is controlled by the
- * system's Embedded Controller (EC).
- * @WMI_BRIGHTNESS_SOURCE_AUX: Backlight brightness is controlled over the
- * DisplayPort AUX channel.
- */
-enum wmi_brightness_source {
- WMI_BRIGHTNESS_SOURCE_GPU = 1,
- WMI_BRIGHTNESS_SOURCE_EC = 2,
- WMI_BRIGHTNESS_SOURCE_AUX = 3,
- WMI_BRIGHTNESS_SOURCE_MAX
-};
-
-/**
- * struct wmi_brightness_args - arguments for the WMI-wrapped ACPI method
- * @mode:Pass in an wmi_brightness_mode value to select between
- * getting or setting a value.
- * @val: In parameter for value to set when using %WMI_BRIGHTNESS_MODE_SET
- * mode. Not used in conjunction with %WMI_BRIGHTNESS_MODE_GET or
- * %WMI_BRIGHTNESS_MODE_GET_MAX_LEVEL mode.
- * @ret: Out parameter returning retrieved value when operating in
- * %WMI_BRIGHTNESS_MODE_GET or %WMI_BRIGHTNESS_MODE_GET_MAX_LEVEL
- * mode. Not used in %WMI_BRIGHTNESS_MODE_SET mode.
- * @ignored: Padding; not used. The ACPI method expects a 24 byte params
struct.
- *
- * This is the parameters structure for the WmiBrightnessNotify ACPI method as
- * wrapped by WMI. The value passed in to @val or returned by @ret will be a
- * brightness value when the WMI method ID is %WMI_BRIGHTNESS_METHOD_LEVEL, or
- * an wmi_brightness_source value with %WMI_BRIGHTNESS_METHOD_SOURCE.
- */
-struct wmi_brightness_args {
- u32 mode;
- u32 val;
- u32 ret;
- u32 ignored[3];
-};
-
/**
* wmi_brightness_notify() - helper function for calling WMI-wrapped ACPI
method
* @w:Pointer to the struct wmi_device identified by %WMI_BRIGHTNESS_GUID
@@ -191,8 +127,6 @@ static int nvidia_wmi_ec_backlight_probe(struct wmi_device
*wdev, const void *ct
return PTR_ERR_OR_ZERO(bdev);
}
-#define WMI_BRIGHTNESS_GUID "603E9613-EF25-4338-A3D0-C46177516DB7"
-
static const struct wmi_device_id nvidia_wmi_ec_backlight_id_table[] = {
{ .guid_string = WMI_BRIGHTNESS_GUID },
{ }
diff --git a/include/linux/platform_data/x86/nvidia-wmi-ec-backlight.h
b/include/linux/platform_data/x86/nvidia-wmi-ec-backlight.h
new file mode 100644
index ..23d60130272c
--- /dev/null
+++ b/include/linux/platform_data/x86/nvidia-wmi-ec-b
Re: [PATCH v3 09/31] ACPI: video: Make backlight class device registration a separate step (v2)
On 8/18/22 1:42 PM, Hans de Goede wrote:
On x86/ACPI boards the acpi_video driver will usually initialize before
the kms driver (except i915). This causes /sys/class/backlight/acpi_video0
to show up and then the kms driver registers its own native backlight
device after which the drivers/acpi/video_detect.c code unregisters
the acpi_video0 device (when acpi_video_get_backlight_type()==native).
This means that userspace briefly sees 2 devices and the disappearing of
acpi_video0 after a brief time confuses the systemd backlight level
save/restore code, see e.g.:
https://bbs.archlinux.org/viewtopic.php?id=269920
To fix this make backlight class device registration a separate step
done by a new acpi_video_register_backlight() function. The intend is for
this to be called by the drm/kms driver *after* it is done setting up its
own native backlight device. So that acpi_video_get_backlight_type() knows
if a native backlight will be available or not at acpi_video backlight
registration time, avoiding the add + remove dance.
Note the new acpi_video_register_backlight() function is also called from
a delayed work to ensure that the acpi_video backlight devices does get
registered if necessary even if there is no drm/kms driver or when it is
disabled.
Changes in v2:
- Make register_backlight_delay a module parameter, mainly so that it can
be disabled by Nvidia binary driver users
Acked-by: Rafael J. Wysocki
Signed-off-by: Hans de Goede
---
drivers/acpi/acpi_video.c | 50 ---
include/acpi/video.h | 2 ++
2 files changed, 49 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 8545bf94866f..09dd86f86cf3 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -73,6 +73,16 @@ module_param(device_id_scheme, bool, 0444);
static int only_lcd = -1;
module_param(only_lcd, int, 0444);
+/*
+ * Display probing is known to take up to 5 seconds, so delay the fallback
+ * backlight registration by 5 seconds + 3 seconds for some extra margin.
+ */
+static int register_backlight_delay = 8;
+module_param(register_backlight_delay, int, 0444);
Would it make sense to make this parameter writable from userspace, e.g.
so that it can be set by a udev rule rather than relying on a riskier
kernel command line edit? Then again, that probably makes things more
complicated, since you'd have to check the parameter again when the
worker fires, and changing the parameter to a non-zero value from either
zero or a different non-zero value would be too weird. And making a
separate writable parameter to allow userspace to turn the worker into a
noop despite it being enabled when the kernel was initially loaded seems
wrong, too.
+MODULE_PARM_DESC(register_backlight_delay,
+ "Delay in seconds before doing fallback (non GPU driver triggered) "
+ "backlight registration, set to 0 to disable.");
+
static bool may_report_brightness_keys;
static int register_count;
static DEFINE_MUTEX(register_count_mutex);
@@ -81,6 +91,9 @@ static LIST_HEAD(video_bus_head);
static int acpi_video_bus_add(struct acpi_device *device);
static int acpi_video_bus_remove(struct acpi_device *device);
static void acpi_video_bus_notify(struct acpi_device *device, u32 event);
+static void acpi_video_bus_register_backlight_work(struct work_struct
*ignored);
+static DECLARE_DELAYED_WORK(video_bus_register_backlight_work,
+ acpi_video_bus_register_backlight_work);
void acpi_video_detect_exit(void);
/*
@@ -1859,8 +1872,6 @@ static int acpi_video_bus_register_backlight(struct
acpi_video_bus *video)
if (video->backlight_registered)
return 0;
- acpi_video_run_bcl_for_osi(video);
-
if (acpi_video_get_backlight_type() != acpi_backlight_video)
return 0;
@@ -2086,7 +2097,11 @@ static int acpi_video_bus_add(struct acpi_device *device)
list_add_tail(>entry, _bus_head);
mutex_unlock(_list_lock);
- acpi_video_bus_register_backlight(video);
+ /*
+* The userspace visible backlight_device gets registered separately
+* from acpi_video_register_backlight().
+*/
+ acpi_video_run_bcl_for_osi(video);
acpi_video_bus_add_notify_handler(video);
return 0;
@@ -2125,6 +2140,11 @@ static int acpi_video_bus_remove(struct acpi_device
*device)
return 0;
}
+static void acpi_video_bus_register_backlight_work(struct work_struct *ignored)
+{
+ acpi_video_register_backlight();
+}
+
static int __init is_i740(struct pci_dev *dev)
{
if (dev->device == 0x00D1)
@@ -2235,6 +2255,18 @@ int acpi_video_register(void)
*/
register_count = 1;
+ /*
+* acpi_video_bus_add() skips registering the userspace visible
+* backlight_device. The intend is for this to be registered by the
+* drm/kms driver calling
Re: [PATCH v3 17/31] ACPI: video: Add Nvidia WMI EC brightness control detection (v2)
On 8/18/22 1:42 PM, Hans de Goede wrote:
On some new laptop designs a new Nvidia specific WMI interface is present
which gives info about panel brightness control and may allow controlling
the brightness through this interface when the embedded controller is used
for brightness control.
When this WMI interface is present and indicates that the EC is used,
then this interface should be used for brightness control.
Changes in v2:
- Use the new shared nvidia-wmi-ec-backlight.h header for the
WMI firmware API definitions
- ACPI_VIDEO can now be enabled on non X86 too,
adjust the Kconfig changes to match this.
Acked-by: Rafael J. Wysocki
Signed-off-by: Hans de Goede
---
drivers/acpi/Kconfig | 1 +
drivers/acpi/video_detect.c| 37 ++
drivers/gpu/drm/gma500/Kconfig | 2 ++
drivers/gpu/drm/i915/Kconfig | 2 ++
include/acpi/video.h | 1 +
5 files changed, 43 insertions(+)
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 7802d8846a8d..44ad4b6bd234 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -212,6 +212,7 @@ config ACPI_VIDEO
tristate "Video"
depends on BACKLIGHT_CLASS_DEVICE
depends on INPUT
+ depends on ACPI_WMI || !X86
select THERMAL
help
This driver implements the ACPI Extensions For Display Adapters
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index cc9d0d91e268..1749e85d6921 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -32,6 +32,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -75,6 +76,36 @@ find_video(acpi_handle handle, u32 lvl, void *context, void
**rv)
return AE_OK;
}
+/* This depends on ACPI_WMI which is X86 only */
+#ifdef CONFIG_X86
+static bool nvidia_wmi_ec_supported(void)
+{
+ struct wmi_brightness_args args = {
+ .mode = WMI_BRIGHTNESS_MODE_GET,
+ .val = 0,
+ .ret = 0,
+ };
+ struct acpi_buffer buf = { (acpi_size)sizeof(args), };
+ acpi_status status;
+
+ status = wmi_evaluate_method("603E9613-EF25-4338-A3D0-C46177516DB7", 0,
I think it would be preferable for the GUID to be in the new shared
header file as well. Apart from that, I think this change looks sane.
+WMI_BRIGHTNESS_METHOD_SOURCE, , );
+ if (ACPI_FAILURE(status))
+ return false;
+
+ /*
+* If brightness is handled by the EC then nvidia-wmi-ec-backlight
+* should be used, else the GPU driver(s) should be used.
+*/
+ return args.ret == WMI_BRIGHTNESS_SOURCE_EC;
+}
+#else
+static bool nvidia_wmi_ec_supported(void)
+{
+ return false;
+}
+#endif
+
/* Force to use vendor driver when the ACPI device is known to be
* buggy */
static int video_detect_force_vendor(const struct dmi_system_id *d)
@@ -541,6 +572,7 @@ static const struct dmi_system_id video_detect_dmi_table[]
= {
static enum acpi_backlight_type __acpi_video_get_backlight_type(bool native)
{
static DEFINE_MUTEX(init_mutex);
+ static bool nvidia_wmi_ec_present;
static bool native_available;
static bool init_done;
static long video_caps;
@@ -553,6 +585,7 @@ static enum acpi_backlight_type
__acpi_video_get_backlight_type(bool native)
acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
ACPI_UINT32_MAX, find_video, NULL,
_caps, NULL);
+ nvidia_wmi_ec_present = nvidia_wmi_ec_supported();
init_done = true;
}
if (native)
@@ -570,6 +603,10 @@ static enum acpi_backlight_type
__acpi_video_get_backlight_type(bool native)
if (acpi_backlight_dmi != acpi_backlight_undef)
return acpi_backlight_dmi;
+ /* Special cases such as nvidia_wmi_ec and apple gmux. */
+ if (nvidia_wmi_ec_present)
+ return acpi_backlight_nvidia_wmi_ec;
+
/* On systems with ACPI video use either native or ACPI video. */
if (video_caps & ACPI_VIDEO_BACKLIGHT) {
/*
diff --git a/drivers/gpu/drm/gma500/Kconfig b/drivers/gpu/drm/gma500/Kconfig
index 0cff20265f97..807b989e3c77 100644
--- a/drivers/gpu/drm/gma500/Kconfig
+++ b/drivers/gpu/drm/gma500/Kconfig
@@ -7,6 +7,8 @@ config DRM_GMA500
select ACPI_VIDEO if ACPI
select BACKLIGHT_CLASS_DEVICE if ACPI
select INPUT if ACPI
+ select X86_PLATFORM_DEVICES if ACPI
+ select ACPI_WMI if ACPI
help
Say yes for an experimental 2D KMS framebuffer driver for the
Intel GMA500 (Poulsbo), Intel GMA600 (Moorestown/Oak Trail) and
diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 7ae3b7d67fcf..3efce05d7b57 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@
Re: [PATCH v3 19/31] platform/x86: nvidia-wmi-ec-backlight: Use acpi_video_get_backlight_type()
On 8/18/22 1:42 PM, Hans de Goede wrote:
Add an acpi_video_get_backlight_type() == acpi_backlight_nvidia_wmi_ec
check. This will make nvidia-wmi-ec-backlight properly honor the user
selecting a different backlight driver through the acpi_backlight=...
kernel commandline option.
Since the auto-detect code check for nvidia-wmi-ec-backlight in
drivers/acpi/video_detect.c already checks that the WMI advertised
brightness-source is the embedded controller, this new check makes it
unnecessary for nvidia_wmi_ec_backlight_probe() to check this itself.
Suggested-by: Daniel Dadap
Signed-off-by: Hans de Goede
---
drivers/platform/x86/Kconfig | 1 +
drivers/platform/x86/nvidia-wmi-ec-backlight.c | 14 +++---
2 files changed, 4 insertions(+), 11 deletions(-)
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index f2f98e942cf2..0cc5ac35fc57 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -93,6 +93,7 @@ config PEAQ_WMI
config NVIDIA_WMI_EC_BACKLIGHT
tristate "EC Backlight Driver for Hybrid Graphics Notebook Systems"
+ depends on ACPI_VIDEO
depends on ACPI_WMI
depends on BACKLIGHT_CLASS_DEVICE
help
diff --git a/drivers/platform/x86/nvidia-wmi-ec-backlight.c
b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
index e84e1d629b14..83d544180264 100644
--- a/drivers/platform/x86/nvidia-wmi-ec-backlight.c
+++ b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
/**
* wmi_brightness_notify() - helper function for calling WMI-wrapped ACPI
method
@@ -87,19 +88,10 @@ static int nvidia_wmi_ec_backlight_probe(struct wmi_device
*wdev, const void *ct
{
struct backlight_properties props = {};
struct backlight_device *bdev;
- u32 source;
int ret;
- ret = wmi_brightness_notify(wdev, WMI_BRIGHTNESS_METHOD_SOURCE,
- WMI_BRIGHTNESS_MODE_GET, );
- if (ret)
- return ret;
-
- /*
-* This driver is only to be used when brightness control is handled
-* by the EC; otherwise, the GPU driver(s) should control brightness.
-*/
- if (source != WMI_BRIGHTNESS_SOURCE_EC)
+ /* drivers/acpi/video_detect.c also checks that SOURCE == EC */
+ if (acpi_video_get_backlight_type() != acpi_backlight_nvidia_wmi_ec)
return -ENODEV;
/*
Reviewed-by: Daniel Dadap
Re: [PATCH v3 15/31] platform/x86: nvidia-wmi-ec-backlight: Move fw interface definitions to a header
On 8/18/22 1:42 PM, Hans de Goede wrote:
Move the WMI interface definitions to a header, so that the definitions
can be shared with drivers/acpi/video_detect.c .
Suggested-by: Daniel Dadap
Signed-off-by: Hans de Goede
---
MAINTAINERS | 1 +
.../platform/x86/nvidia-wmi-ec-backlight.c| 66 +
.../x86/nvidia-wmi-ec-backlight.h | 70 +++
3 files changed, 72 insertions(+), 65 deletions(-)
create mode 100644 include/linux/platform_data/x86/nvidia-wmi-ec-backlight.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 8a5012ba6ff9..8d59c6e9b4db 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14526,6 +14526,7 @@ M: Daniel Dadap
L:platform-driver-...@vger.kernel.org
S:Supported
F:drivers/platform/x86/nvidia-wmi-ec-backlight.c
+F: include/linux/platform_data/x86/nvidia-wmi-ec-backlight.h
NVM EXPRESS DRIVER
M:Keith Busch
diff --git a/drivers/platform/x86/nvidia-wmi-ec-backlight.c
b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
index 61e37194df70..e84e1d629b14 100644
--- a/drivers/platform/x86/nvidia-wmi-ec-backlight.c
+++ b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
@@ -7,74 +7,10 @@
#include
#include
#include
+#include
#include
#include
-/**
- * enum wmi_brightness_method - WMI method IDs
- * @WMI_BRIGHTNESS_METHOD_LEVEL: Get/Set EC brightness level status
- * @WMI_BRIGHTNESS_METHOD_SOURCE: Get/Set EC Brightness Source
- */
-enum wmi_brightness_method {
- WMI_BRIGHTNESS_METHOD_LEVEL = 1,
- WMI_BRIGHTNESS_METHOD_SOURCE = 2,
- WMI_BRIGHTNESS_METHOD_MAX
-};
-
-/**
- * enum wmi_brightness_mode - Operation mode for WMI-wrapped method
- * @WMI_BRIGHTNESS_MODE_GET:Get the current brightness
level/source.
- * @WMI_BRIGHTNESS_MODE_SET:Set the brightness level.
- * @WMI_BRIGHTNESS_MODE_GET_MAX_LEVEL: Get the maximum brightness level. This
- * is only valid when the WMI method is
- * %WMI_BRIGHTNESS_METHOD_LEVEL.
- */
-enum wmi_brightness_mode {
- WMI_BRIGHTNESS_MODE_GET = 0,
- WMI_BRIGHTNESS_MODE_SET = 1,
- WMI_BRIGHTNESS_MODE_GET_MAX_LEVEL = 2,
- WMI_BRIGHTNESS_MODE_MAX
-};
-
-/**
- * enum wmi_brightness_source - Backlight brightness control source selection
- * @WMI_BRIGHTNESS_SOURCE_GPU: Backlight brightness is controlled by the GPU.
- * @WMI_BRIGHTNESS_SOURCE_EC: Backlight brightness is controlled by the
- * system's Embedded Controller (EC).
- * @WMI_BRIGHTNESS_SOURCE_AUX: Backlight brightness is controlled over the
- * DisplayPort AUX channel.
- */
-enum wmi_brightness_source {
- WMI_BRIGHTNESS_SOURCE_GPU = 1,
- WMI_BRIGHTNESS_SOURCE_EC = 2,
- WMI_BRIGHTNESS_SOURCE_AUX = 3,
- WMI_BRIGHTNESS_SOURCE_MAX
-};
-
-/**
- * struct wmi_brightness_args - arguments for the WMI-wrapped ACPI method
- * @mode:Pass in an wmi_brightness_mode value to select between
- * getting or setting a value.
- * @val: In parameter for value to set when using %WMI_BRIGHTNESS_MODE_SET
- * mode. Not used in conjunction with %WMI_BRIGHTNESS_MODE_GET or
- * %WMI_BRIGHTNESS_MODE_GET_MAX_LEVEL mode.
- * @ret: Out parameter returning retrieved value when operating in
- * %WMI_BRIGHTNESS_MODE_GET or %WMI_BRIGHTNESS_MODE_GET_MAX_LEVEL
- * mode. Not used in %WMI_BRIGHTNESS_MODE_SET mode.
- * @ignored: Padding; not used. The ACPI method expects a 24 byte params
struct.
- *
- * This is the parameters structure for the WmiBrightnessNotify ACPI method as
- * wrapped by WMI. The value passed in to @val or returned by @ret will be a
- * brightness value when the WMI method ID is %WMI_BRIGHTNESS_METHOD_LEVEL, or
- * an wmi_brightness_source value with %WMI_BRIGHTNESS_METHOD_SOURCE.
- */
-struct wmi_brightness_args {
- u32 mode;
- u32 val;
- u32 ret;
- u32 ignored[3];
-};
-
/**
* wmi_brightness_notify() - helper function for calling WMI-wrapped ACPI
method
* @w:Pointer to the struct wmi_device identified by %WMI_BRIGHTNESS_GUID
diff --git a/include/linux/platform_data/x86/nvidia-wmi-ec-backlight.h
b/include/linux/platform_data/x86/nvidia-wmi-ec-backlight.h
new file mode 100644
index ..d83104c6c6cb
--- /dev/null
+++ b/include/linux/platform_data/x86/nvidia-wmi-ec-backlight.h
@@ -0,0 +1,70 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
Should the copyright notice from nvidia-wmi-ec-backlight be copied here
as well?
+#ifndef __PLATFORM_DATA_X86_NVIDIA_WMI_EC_BACKLIGHT_H
+#define __PLATFORM_DATA_X86_NVIDIA_WMI_EC_BACKLIGHT_H
+
+/**
+ * enum wmi_brightness_method - WMI method IDs
+ * @WMI_BRIGHTNESS_METHOD_LEVEL: Get/Set EC brightness level status
+ * @WMI_BRIGHTNESS_METHOD_SOURCE: Get/Set EC Brightness Source
+ */
+enum wmi_brightness_method
Re: [PATCH v2 16/29] ACPI: video: Add Nvidia WMI EC brightness control detection
On 8/17/22 7:22 AM, Hans de Goede wrote:
Hi Daniel,
On 7/15/22 13:59, Hans de Goede wrote:
Hi Daniel,
On 7/12/22 22:13, Daniel Dadap wrote:
Thanks, Hans:
On 7/12/22 14:38, Hans de Goede wrote:
On some new laptop designs a new Nvidia specific WMI interface is present
which gives info about panel brightness control and may allow controlling
the brightness through this interface when the embedded controller is used
for brightness control.
When this WMI interface is present and indicates that the EC is used,
then this interface should be used for brightness control.
Signed-off-by: Hans de Goede
---
drivers/acpi/Kconfig | 1 +
drivers/acpi/video_detect.c | 35 ++
drivers/gpu/drm/gma500/Kconfig | 2 ++
drivers/gpu/drm/i915/Kconfig | 2 ++
include/acpi/video.h | 1 +
5 files changed, 41 insertions(+)
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 1e34f846508f..c372385cfc3f 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -212,6 +212,7 @@ config ACPI_VIDEO
tristate "Video"
depends on X86 && BACKLIGHT_CLASS_DEVICE
depends on INPUT
+ depends on ACPI_WMI
select THERMAL
help
This driver implements the ACPI Extensions For Display Adapters
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index 8c2863403040..7b89dc9a04a2 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -75,6 +75,35 @@ find_video(acpi_handle handle, u32 lvl, void *context, void
**rv)
return AE_OK;
}
+#define WMI_BRIGHTNESS_METHOD_SOURCE 2
+#define WMI_BRIGHTNESS_MODE_GET 0
+#define WMI_BRIGHTNESS_SOURCE_EC 2
+
+struct wmi_brightness_args {
+ u32 mode;
+ u32 val;
+ u32 ret;
+ u32 ignored[3];
+};
+
+static bool nvidia_wmi_ec_supported(void)
+{
+ struct wmi_brightness_args args = {
+ .mode = WMI_BRIGHTNESS_MODE_GET,
+ .val = 0,
+ .ret = 0,
+ };
+ struct acpi_buffer buf = { (acpi_size)sizeof(args), };
+ acpi_status status;
+
+ status = wmi_evaluate_method("603E9613-EF25-4338-A3D0-C46177516DB7", 0,
+ WMI_BRIGHTNESS_METHOD_SOURCE, , );
+ if (ACPI_FAILURE(status))
+ return false;
+
+ return args.ret == WMI_BRIGHTNESS_SOURCE_EC;
+}
+
The code duplication here with nvidia-wmi-ec-backlight.c is a little
unfortunate. Can we move the constants, struct definition, and WMI GUID from
that file to a header file that's used both by the EC backlight driver and the
ACPI video driver?
Yes that is a good idea. I suggest using
include/linux/platform_data/x86/nvidia-wmi-ec-backlight.h
to move the shared definitions there.
If you can submit 2 patches on top of this series:
1. Moving the definitions from drivers/platform/x86/nvidia-wmi-ec-backlight.c to
include/linux/platform_data/x86/nvidia-wmi-ec-backlight.h
2. Switching the code from this patch over to using the new
nvidia-wmi-ec-backlight.h
Then for the next version I'll add patch 1. to the series and squash patch 2.
into this one.
Note: I'm preparing a v3 of the series and I've made these changes myself now.
Okay, thanks. Sorry, I had started on this but then got distracted by
other work.
I was thinking it might be nice to add a wrapper around wmi_brightness_notify()
in nvidia-wmi-ec-backlight.c that does this source == WMI_BRIGHTNESS_SOURCE_EC
test, and then export it so that it can be called both here and in the EC
backlight driver's probe routine, but then I guess that would make video.ko
depend on nvidia-wmi-ec-backlight.ko, which seems wrong. It also seems wrong to
implement the WMI plumbing in the ACPI video driver, and export it so that the
EC backlight driver can use it, so I guess I can live with the duplication of
the relatively simple WMI stuff here, it would just be nice to not have to
define all of the API constants, structure, and GUID twice.
Agreed.
/* Force to use vendor driver when the ACPI device is known to be
* buggy */
static int video_detect_force_vendor(const struct dmi_system_id *d)
@@ -518,6 +547,7 @@ static const struct dmi_system_id video_detect_dmi_table[]
= {
static enum acpi_backlight_type __acpi_video_get_backlight_type(bool native)
{
static DEFINE_MUTEX(init_mutex);
+ static bool nvidia_wmi_ec_present;
static bool native_available;
static bool init_done;
static long video_caps;
@@ -530,6 +560,7 @@ static enum acpi_backlight_type
__acpi_video_get_backlight_type(bool native)
acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
ACPI_UINT32_MAX, find_video, NULL,
_caps, NULL);
+ nvidia_wmi_ec_present = nvidia_wmi_ec_supported();
init_done = true;
}
if (native)
@@ -547,6 +578,10 @@ static enum acpi_backlight_type
__acpi_video_ge
Re: [PATCH v2 01/29] ACPI: video: Add acpi_video_backlight_use_native() helper
On 8/17/22 10:05 AM, Hans de Goede wrote:
Hi Daniel,
On 8/2/22 18:49, Daniel Dadap wrote:
On 8/2/22 06:31, Hans de Goede wrote:
Hi Daniel,
On 7/21/22 23:30, Daniel Dadap wrote:
On 7/21/22 16:24, Daniel Dadap wrote:
On 7/12/22 14:38, Hans de Goede wrote:
ATM on x86 laptops where we want userspace to use the acpi_video backlight
device we often register both the GPU's native backlight device and
acpi_video's firmware acpi_video# backlight device. This relies on
userspace preferring firmware type backlight devices over native ones, but
registering 2 backlight devices for a single display really is undesirable.
On x86 laptops where the native GPU backlight device should be used,
the registering of other backlight devices is avoided by their drivers
using acpi_video_get_backlight_type() and only registering their backlight
if the return value matches their type.
acpi_video_get_backlight_type() uses
backlight_device_get_by_type(BACKLIGHT_RAW) to determine if a native
driver is available and will never return native if this returns
false. This means that the GPU's native backlight registering code
cannot just call acpi_video_get_backlight_type() to determine if it
should register its backlight, since acpi_video_get_backlight_type() will
never return native until the native backlight has already registered.
To fix this add a new internal native function parameter to
acpi_video_get_backlight_type(), which when set to true will make
acpi_video_get_backlight_type() behave as if a native backlight has
already been registered.
And add a new acpi_video_backlight_use_native() helper, which sets this
to true, for use in native GPU backlight code.
Changes in v2:
- Replace adding a native parameter to acpi_video_get_backlight_type() with
adding a new acpi_video_backlight_use_native() helper.
Signed-off-by: Hans de Goede
---
drivers/acpi/video_detect.c | 24
include/acpi/video.h | 5 +
2 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index becc198e4c22..4346c990022d 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -17,8 +17,9 @@
* Otherwise vendor specific drivers like thinkpad_acpi, asus-laptop,
* sony_acpi,... can take care about backlight brightness.
*
- * Backlight drivers can use acpi_video_get_backlight_type() to determine
- * which driver should handle the backlight.
+ * Backlight drivers can use acpi_video_get_backlight_type() to determine which
+ * driver should handle the backlight. RAW/GPU-driver backlight drivers must
+ * use the acpi_video_backlight_use_native() helper for this.
*
* 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
@@ -548,9 +549,10 @@ static int acpi_video_backlight_notify(struct
notifier_block *nb,
* Arguably the native on win8 check should be done first, but that would
* be a behavior change, which may causes issues.
*/
-enum acpi_backlight_type acpi_video_get_backlight_type(void)
+static enum acpi_backlight_type __acpi_video_get_backlight_type(bool native)
{
static DEFINE_MUTEX(init_mutex);
+ static bool native_available;
static bool init_done;
static long video_caps;
@@ -570,6 +572,8 @@ enum acpi_backlight_type
acpi_video_get_backlight_type(void)
backlight_notifier_registered = true;
init_done = true;
}
+ if (native)
+ native_available = true;
mutex_unlock(_mutex);
if (acpi_backlight_cmdline != acpi_backlight_undef)
@@ -581,13 +585,25 @@ enum acpi_backlight_type
acpi_video_get_backlight_type(void)
if (!(video_caps & ACPI_VIDEO_BACKLIGHT))
return acpi_backlight_vendor;
- if (acpi_osi_is_win8() && backlight_device_get_by_type(BACKLIGHT_RAW))
+ if (acpi_osi_is_win8() &&
+ (native_available || backlight_device_get_by_type(BACKLIGHT_RAW)))
return acpi_backlight_native;
return acpi_backlight_video;
So I ran into a minor problem when testing the NVIDIA proprietary driver
against this change set, after checking acpi_video_backlight_use_native()
before registering the NVIDIA proprietary driver's backlight handler. Namely,
for the case where a user installs the NVIDIA proprietary driver after the
video.ko has already registered its backlight handler, we end up with both the
firmware and native handlers registered simultaneously, since the ACPI video
driver no longer unregisters its backlight handler. In this state, desktop
environments end up preferring the registered but non-functional firmware
handler from video.ko. (Manually twiddling the sysfs interface for the native
NVIDIA handler works fine.) When rebooting the system after installing the
NVIDIA proprietary driver, it is able to regi
Re: [PATCH v2 01/29] ACPI: video: Add acpi_video_backlight_use_native() helper
On 8/2/22 06:31, Hans de Goede wrote:
Hi Daniel,
On 7/21/22 23:30, Daniel Dadap wrote:
On 7/21/22 16:24, Daniel Dadap wrote:
On 7/12/22 14:38, Hans de Goede wrote:
ATM on x86 laptops where we want userspace to use the acpi_video backlight
device we often register both the GPU's native backlight device and
acpi_video's firmware acpi_video# backlight device. This relies on
userspace preferring firmware type backlight devices over native ones, but
registering 2 backlight devices for a single display really is undesirable.
On x86 laptops where the native GPU backlight device should be used,
the registering of other backlight devices is avoided by their drivers
using acpi_video_get_backlight_type() and only registering their backlight
if the return value matches their type.
acpi_video_get_backlight_type() uses
backlight_device_get_by_type(BACKLIGHT_RAW) to determine if a native
driver is available and will never return native if this returns
false. This means that the GPU's native backlight registering code
cannot just call acpi_video_get_backlight_type() to determine if it
should register its backlight, since acpi_video_get_backlight_type() will
never return native until the native backlight has already registered.
To fix this add a new internal native function parameter to
acpi_video_get_backlight_type(), which when set to true will make
acpi_video_get_backlight_type() behave as if a native backlight has
already been registered.
And add a new acpi_video_backlight_use_native() helper, which sets this
to true, for use in native GPU backlight code.
Changes in v2:
- Replace adding a native parameter to acpi_video_get_backlight_type() with
adding a new acpi_video_backlight_use_native() helper.
Signed-off-by: Hans de Goede
---
drivers/acpi/video_detect.c | 24
include/acpi/video.h | 5 +
2 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index becc198e4c22..4346c990022d 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -17,8 +17,9 @@
* Otherwise vendor specific drivers like thinkpad_acpi, asus-laptop,
* sony_acpi,... can take care about backlight brightness.
*
- * Backlight drivers can use acpi_video_get_backlight_type() to determine
- * which driver should handle the backlight.
+ * Backlight drivers can use acpi_video_get_backlight_type() to determine which
+ * driver should handle the backlight. RAW/GPU-driver backlight drivers must
+ * use the acpi_video_backlight_use_native() helper for this.
*
* 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
@@ -548,9 +549,10 @@ static int acpi_video_backlight_notify(struct
notifier_block *nb,
* Arguably the native on win8 check should be done first, but that would
* be a behavior change, which may causes issues.
*/
-enum acpi_backlight_type acpi_video_get_backlight_type(void)
+static enum acpi_backlight_type __acpi_video_get_backlight_type(bool native)
{
static DEFINE_MUTEX(init_mutex);
+ static bool native_available;
static bool init_done;
static long video_caps;
@@ -570,6 +572,8 @@ enum acpi_backlight_type
acpi_video_get_backlight_type(void)
backlight_notifier_registered = true;
init_done = true;
}
+ if (native)
+ native_available = true;
mutex_unlock(_mutex);
if (acpi_backlight_cmdline != acpi_backlight_undef)
@@ -581,13 +585,25 @@ enum acpi_backlight_type
acpi_video_get_backlight_type(void)
if (!(video_caps & ACPI_VIDEO_BACKLIGHT))
return acpi_backlight_vendor;
- if (acpi_osi_is_win8() && backlight_device_get_by_type(BACKLIGHT_RAW))
+ if (acpi_osi_is_win8() &&
+ (native_available || backlight_device_get_by_type(BACKLIGHT_RAW)))
return acpi_backlight_native;
return acpi_backlight_video;
So I ran into a minor problem when testing the NVIDIA proprietary driver
against this change set, after checking acpi_video_backlight_use_native()
before registering the NVIDIA proprietary driver's backlight handler. Namely,
for the case where a user installs the NVIDIA proprietary driver after the
video.ko has already registered its backlight handler, we end up with both the
firmware and native handlers registered simultaneously, since the ACPI video
driver no longer unregisters its backlight handler. In this state, desktop
environments end up preferring the registered but non-functional firmware
handler from video.ko. (Manually twiddling the sysfs interface for the native
NVIDIA handler works fine.) When rebooting the system after installing the
NVIDIA proprietary driver, it is able to register its native handler before the
delayed work to register the ACPI video backlight handler fires, so we end up
Re: [PATCH v2 01/29] ACPI: video: Add acpi_video_backlight_use_native() helper
On 7/12/22 14:38, Hans de Goede wrote:
ATM on x86 laptops where we want userspace to use the acpi_video backlight
device we often register both the GPU's native backlight device and
acpi_video's firmware acpi_video# backlight device. This relies on
userspace preferring firmware type backlight devices over native ones, but
registering 2 backlight devices for a single display really is undesirable.
On x86 laptops where the native GPU backlight device should be used,
the registering of other backlight devices is avoided by their drivers
using acpi_video_get_backlight_type() and only registering their backlight
if the return value matches their type.
acpi_video_get_backlight_type() uses
backlight_device_get_by_type(BACKLIGHT_RAW) to determine if a native
driver is available and will never return native if this returns
false. This means that the GPU's native backlight registering code
cannot just call acpi_video_get_backlight_type() to determine if it
should register its backlight, since acpi_video_get_backlight_type() will
never return native until the native backlight has already registered.
To fix this add a new internal native function parameter to
acpi_video_get_backlight_type(), which when set to true will make
acpi_video_get_backlight_type() behave as if a native backlight has
already been registered.
And add a new acpi_video_backlight_use_native() helper, which sets this
to true, for use in native GPU backlight code.
Changes in v2:
- Replace adding a native parameter to acpi_video_get_backlight_type() with
adding a new acpi_video_backlight_use_native() helper.
Signed-off-by: Hans de Goede
---
drivers/acpi/video_detect.c | 24
include/acpi/video.h| 5 +
2 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index becc198e4c22..4346c990022d 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -17,8 +17,9 @@
* Otherwise vendor specific drivers like thinkpad_acpi, asus-laptop,
* sony_acpi,... can take care about backlight brightness.
*
- * Backlight drivers can use acpi_video_get_backlight_type() to determine
- * which driver should handle the backlight.
+ * Backlight drivers can use acpi_video_get_backlight_type() to determine which
+ * driver should handle the backlight. RAW/GPU-driver backlight drivers must
+ * use the acpi_video_backlight_use_native() helper for this.
*
* 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
@@ -548,9 +549,10 @@ static int acpi_video_backlight_notify(struct
notifier_block *nb,
* Arguably the native on win8 check should be done first, but that would
* be a behavior change, which may causes issues.
*/
-enum acpi_backlight_type acpi_video_get_backlight_type(void)
+static enum acpi_backlight_type __acpi_video_get_backlight_type(bool native)
{
static DEFINE_MUTEX(init_mutex);
+ static bool native_available;
static bool init_done;
static long video_caps;
@@ -570,6 +572,8 @@ enum acpi_backlight_type acpi_video_get_backlight_type(void)
backlight_notifier_registered = true;
init_done = true;
}
+ if (native)
+ native_available = true;
mutex_unlock(_mutex);
if (acpi_backlight_cmdline != acpi_backlight_undef)
@@ -581,13 +585,25 @@ enum acpi_backlight_type
acpi_video_get_backlight_type(void)
if (!(video_caps & ACPI_VIDEO_BACKLIGHT))
return acpi_backlight_vendor;
- if (acpi_osi_is_win8() && backlight_device_get_by_type(BACKLIGHT_RAW))
+ if (acpi_osi_is_win8() &&
+ (native_available || backlight_device_get_by_type(BACKLIGHT_RAW)))
return acpi_backlight_native;
return acpi_backlight_video;
So I ran into a minor problem when testing the NVIDIA proprietary driver
against this change set, after checking
acpi_video_backlight_use_native() before registering the NVIDIA
proprietary driver's backlight handler. Namely, for the case where a
user installs the NVIDIA proprietary driver after the video.ko has
already registered its backlight handler, we end up with both the
firmware and native handlers registered simultaneously, since the ACPI
video driver no longer unregisters its backlight handler. In this state,
desktop environments end up preferring the registered but non-functional
firmware handler from video.ko. (Manually twiddling the sysfs interface
for the native NVIDIA handler works fine.) When rebooting the system
after installing the NVIDIA proprietary driver, it is able to register
its native handler before the delayed work to register the ACPI video
backlight handler fires, so we end up with only one (native) handler,
and userspace is happy.
Maybe this will be moot later on, when the existing sysfs interface is
Re: [PATCH v2 01/29] ACPI: video: Add acpi_video_backlight_use_native() helper
On 7/21/22 16:24, Daniel Dadap wrote:
On 7/12/22 14:38, Hans de Goede wrote:
ATM on x86 laptops where we want userspace to use the acpi_video
backlight
device we often register both the GPU's native backlight device and
acpi_video's firmware acpi_video# backlight device. This relies on
userspace preferring firmware type backlight devices over native
ones, but
registering 2 backlight devices for a single display really is
undesirable.
On x86 laptops where the native GPU backlight device should be used,
the registering of other backlight devices is avoided by their drivers
using acpi_video_get_backlight_type() and only registering their
backlight
if the return value matches their type.
acpi_video_get_backlight_type() uses
backlight_device_get_by_type(BACKLIGHT_RAW) to determine if a native
driver is available and will never return native if this returns
false. This means that the GPU's native backlight registering code
cannot just call acpi_video_get_backlight_type() to determine if it
should register its backlight, since acpi_video_get_backlight_type()
will
never return native until the native backlight has already registered.
To fix this add a new internal native function parameter to
acpi_video_get_backlight_type(), which when set to true will make
acpi_video_get_backlight_type() behave as if a native backlight has
already been registered.
And add a new acpi_video_backlight_use_native() helper, which sets this
to true, for use in native GPU backlight code.
Changes in v2:
- Replace adding a native parameter to
acpi_video_get_backlight_type() with
adding a new acpi_video_backlight_use_native() helper.
Signed-off-by: Hans de Goede
---
drivers/acpi/video_detect.c | 24
include/acpi/video.h | 5 +
2 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index becc198e4c22..4346c990022d 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -17,8 +17,9 @@
* Otherwise vendor specific drivers like thinkpad_acpi, asus-laptop,
* sony_acpi,... can take care about backlight brightness.
*
- * Backlight drivers can use acpi_video_get_backlight_type() to
determine
- * which driver should handle the backlight.
+ * Backlight drivers can use acpi_video_get_backlight_type() to
determine which
+ * driver should handle the backlight. RAW/GPU-driver backlight
drivers must
+ * use the acpi_video_backlight_use_native() helper for this.
*
* 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
@@ -548,9 +549,10 @@ static int acpi_video_backlight_notify(struct
notifier_block *nb,
* Arguably the native on win8 check should be done first, but that
would
* be a behavior change, which may causes issues.
*/
-enum acpi_backlight_type acpi_video_get_backlight_type(void)
+static enum acpi_backlight_type __acpi_video_get_backlight_type(bool
native)
{
static DEFINE_MUTEX(init_mutex);
+ static bool native_available;
static bool init_done;
static long video_caps;
@@ -570,6 +572,8 @@ enum acpi_backlight_type
acpi_video_get_backlight_type(void)
backlight_notifier_registered = true;
init_done = true;
}
+ if (native)
+ native_available = true;
mutex_unlock(_mutex);
if (acpi_backlight_cmdline != acpi_backlight_undef)
@@ -581,13 +585,25 @@ enum acpi_backlight_type
acpi_video_get_backlight_type(void)
if (!(video_caps & ACPI_VIDEO_BACKLIGHT))
return acpi_backlight_vendor;
- if (acpi_osi_is_win8() &&
backlight_device_get_by_type(BACKLIGHT_RAW))
+ if (acpi_osi_is_win8() &&
+ (native_available ||
backlight_device_get_by_type(BACKLIGHT_RAW)))
return acpi_backlight_native;
return acpi_backlight_video;
So I ran into a minor problem when testing the NVIDIA proprietary
driver against this change set, after checking
acpi_video_backlight_use_native() before registering the NVIDIA
proprietary driver's backlight handler. Namely, for the case where a
user installs the NVIDIA proprietary driver after the video.ko has
already registered its backlight handler, we end up with both the
firmware and native handlers registered simultaneously, since the ACPI
video driver no longer unregisters its backlight handler. In this
state, desktop environments end up preferring the registered but
non-functional firmware handler from video.ko. (Manually twiddling the
sysfs interface for the native NVIDIA handler works fine.) When
rebooting the system after installing the NVIDIA proprietary driver,
it is able to register its native handler before the delayed work to
register the ACPI video backlight handler fires, so we end up with
only one (native) handler, and userspace is happy.
Maybe this will be moot lat
Re: [PATCH v2 16/29] ACPI: video: Add Nvidia WMI EC brightness control detection
> On Jul 15, 2022, at 06:59, Hans de Goede wrote:
>
> Hi Daniel,
>
>> On 7/12/22 22:13, Daniel Dadap wrote:
>> Thanks, Hans:
>>
>>> On 7/12/22 14:38, Hans de Goede wrote:
>>> On some new laptop designs a new Nvidia specific WMI interface is present
>>> which gives info about panel brightness control and may allow controlling
>>> the brightness through this interface when the embedded controller is used
>>> for brightness control.
>>>
>>> When this WMI interface is present and indicates that the EC is used,
>>> then this interface should be used for brightness control.
>>>
>>> Signed-off-by: Hans de Goede
>>> ---
>>> drivers/acpi/Kconfig | 1 +
>>> drivers/acpi/video_detect.c| 35 ++
>>> drivers/gpu/drm/gma500/Kconfig | 2 ++
>>> drivers/gpu/drm/i915/Kconfig | 2 ++
>>> include/acpi/video.h | 1 +
>>> 5 files changed, 41 insertions(+)
>>>
>>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>>> index 1e34f846508f..c372385cfc3f 100644
>>> --- a/drivers/acpi/Kconfig
>>> +++ b/drivers/acpi/Kconfig
>>> @@ -212,6 +212,7 @@ config ACPI_VIDEO
>>> tristate "Video"
>>> depends on X86 && BACKLIGHT_CLASS_DEVICE
>>> depends on INPUT
>>> +depends on ACPI_WMI
>>> select THERMAL
>>> help
>>> This driver implements the ACPI Extensions For Display Adapters
>>> diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
>>> index 8c2863403040..7b89dc9a04a2 100644
>>> --- a/drivers/acpi/video_detect.c
>>> +++ b/drivers/acpi/video_detect.c
>>> @@ -75,6 +75,35 @@ find_video(acpi_handle handle, u32 lvl, void *context,
>>> void **rv)
>>> return AE_OK;
>>> }
>>> +#define WMI_BRIGHTNESS_METHOD_SOURCE2
>>> +#define WMI_BRIGHTNESS_MODE_GET0
>>> +#define WMI_BRIGHTNESS_SOURCE_EC2
>>> +
>>> +struct wmi_brightness_args {
>>> +u32 mode;
>>> +u32 val;
>>> +u32 ret;
>>> +u32 ignored[3];
>>> +};
>>> +
>>> +static bool nvidia_wmi_ec_supported(void)
>>> +{
>>> +struct wmi_brightness_args args = {
>>> +.mode = WMI_BRIGHTNESS_MODE_GET,
>>> +.val = 0,
>>> +.ret = 0,
>>> +};
>>> +struct acpi_buffer buf = { (acpi_size)sizeof(args), };
>>> +acpi_status status;
>>> +
>>> +status = wmi_evaluate_method("603E9613-EF25-4338-A3D0-C46177516DB7", 0,
>>> + WMI_BRIGHTNESS_METHOD_SOURCE, , );
>>> +if (ACPI_FAILURE(status))
>>> +return false;
>>> +
>>> +return args.ret == WMI_BRIGHTNESS_SOURCE_EC;
>>> +}
>>> +
>>
>>
>> The code duplication here with nvidia-wmi-ec-backlight.c is a little
>> unfortunate. Can we move the constants, struct definition, and WMI GUID from
>> that file to a header file that's used both by the EC backlight driver and
>> the ACPI video driver?
>
> Yes that is a good idea. I suggest using
> include/linux/platform_data/x86/nvidia-wmi-ec-backlight.h
> to move the shared definitions there.
>
> If you can submit 2 patches on top of this series:
>
> 1. Moving the definitions from drivers/platform/x86/nvidia-wmi-ec-backlight.c
> to
> include/linux/platform_data/x86/nvidia-wmi-ec-backlight.h
>
> 2. Switching the code from this patch over to using the new
> nvidia-wmi-ec-backlight.h
>
> Then for the next version I'll add patch 1. to the series and squash patch 2.
> into this one.
>
>> I was thinking it might be nice to add a wrapper around
>> wmi_brightness_notify() in nvidia-wmi-ec-backlight.c that does this source
>> == WMI_BRIGHTNESS_SOURCE_EC test, and then export it so that it can be
>> called both here and in the EC backlight driver's probe routine, but then I
>> guess that would make video.ko depend on nvidia-wmi-ec-backlight.ko, which
>> seems wrong. It also seems wrong to implement the WMI plumbing in the ACPI
>> video driver, and export it so that the EC backlight driver can use it, so I
>> guess I can live with the duplication of the relatively simple WMI stuff
>> here, it would just be nice to not have to define all of the API constants,
>> structure, and GUID twice.
>
> Agreed.
>
>>
&
Re: [PATCH v2 20/29] platform/x86: acer-wmi: Move backlight DMI quirks to acpi/video_detect.c
I'll ask around to see if there's some DMI property we can match in
order to detect whether a system is expected to use the EC backlight
driver: if so, maybe we can avoid the WMI interactions in patch 16/29 of
this series. Although I suppose even if there were a DMI property, we'd
still need to call the WMI-wrapped ACPI method to check whether the
system is currently configured to drive the backlight through the EC,
unless the system somehow exports a different DMI table depending on the
current backlight control configuration, which I imagine to be unlikely.
This change looks fine to me, although I suppose somebody who maintains
the acer-wmi driver should comment. The bugzilla links are a nice touch.
On 7/12/22 14:39, Hans de Goede wrote:
Move the backlight DMI quirks to acpi/video_detect.c, so that
the driver no longer needs to call acpi_video_set_dmi_backlight_type().
acpi_video_set_dmi_backlight_type() is troublesome because it may end up
getting called after other backlight drivers have already called
acpi_video_get_backlight_type() resulting in the other drivers
already being registered even though they should not.
Note that even though the DMI quirk table name was video_vendor_dmi_table,
5/6 quirks were actually quirks to use the GPU native backlight.
These 5 quirks also had a callback in their dmi_system_id entry which
disabled the acer-wmi vendor driver; and any DMI match resulted in:
acpi_video_set_dmi_backlight_type(acpi_backlight_vendor);
which disabled the acpi_video driver, so only the native driver was left.
The new entries for these 5/6 devices correctly marks these as needing
the native backlight driver.
Also note that other changes in this series change the native backlight
drivers to no longer unconditionally register their backlight. Instead
these drivers now do this check:
if (acpi_video_get_backlight_type(false) != acpi_backlight_native)
return 0; /* bail */
which without this patch would have broken these 5/6 "special" quirks.
Since I had to look at all the commits adding the quirks anyways, to make
sure that I understood the code correctly, I've also added links to
the various original bugzillas for these quirks to the new entries.
Signed-off-by: Hans de Goede
---
drivers/acpi/video_detect.c | 53 ++
drivers/platform/x86/acer-wmi.c | 66 -
2 files changed, 53 insertions(+), 66 deletions(-)
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index a514adaec14d..cd51cb0d7821 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -147,6 +147,15 @@ static const struct dmi_system_id video_detect_dmi_table[]
= {
DMI_MATCH(DMI_BOARD_NAME, "X360"),
},
},
+ {
+/* https://bugzilla.redhat.com/show_bug.cgi?id=1128309 */
+.callback = video_detect_force_vendor,
+/* Acer KAV80 */
+.matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "KAV80"),
+ },
+ },
{
.callback = video_detect_force_vendor,
/* Asus UL30VT */
@@ -427,6 +436,41 @@ static const struct dmi_system_id video_detect_dmi_table[]
= {
DMI_MATCH(DMI_BOARD_NAME, "JV50"),
},
},
+ {
+/* https://bugzilla.redhat.com/show_bug.cgi?id=1012674 */
+.callback = video_detect_force_native,
+/* Acer Aspire 5741 */
+.matches = {
+ DMI_MATCH(DMI_BOARD_VENDOR, "Acer"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 5741"),
+ },
+ },
+ {
+/* https://bugzilla.kernel.org/show_bug.cgi?id=42993 */
+.callback = video_detect_force_native,
+/* Acer Aspire 5750 */
+.matches = {
+ DMI_MATCH(DMI_BOARD_VENDOR, "Acer"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 5750"),
+ },
+ },
+ {
+/* https://bugzilla.kernel.org/show_bug.cgi?id=42833 */
+.callback = video_detect_force_native,
+/* Acer Extensa 5235 */
+.matches = {
+ DMI_MATCH(DMI_BOARD_VENDOR, "Acer"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Extensa 5235"),
+ },
+ },
+ {
+.callback = video_detect_force_native,
+/* Acer TravelMate 4750 */
+.matches = {
+ DMI_MATCH(DMI_BOARD_VENDOR, "Acer"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "TravelMate 4750"),
+ },
+ },
{
/* https://bugzilla.kernel.org/show_bug.cgi?id=207835 */
.callback = video_detect_force_native,
@@ -437,6 +481,15 @@ static const struct dmi_system_id video_detect_dmi_table[]
= {
DMI_MATCH(DMI_BOARD_NAME, "BA51_MV"),
},
},
+ {
+/* https://bugzilla.kernel.org/show_bug.cgi?id=36322 */
+.callback =
Re: [PATCH v2 16/29] ACPI: video: Add Nvidia WMI EC brightness control detection
Thanks, Hans:
On 7/12/22 14:38, Hans de Goede wrote:
On some new laptop designs a new Nvidia specific WMI interface is present
which gives info about panel brightness control and may allow controlling
the brightness through this interface when the embedded controller is used
for brightness control.
When this WMI interface is present and indicates that the EC is used,
then this interface should be used for brightness control.
Signed-off-by: Hans de Goede
---
drivers/acpi/Kconfig | 1 +
drivers/acpi/video_detect.c| 35 ++
drivers/gpu/drm/gma500/Kconfig | 2 ++
drivers/gpu/drm/i915/Kconfig | 2 ++
include/acpi/video.h | 1 +
5 files changed, 41 insertions(+)
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 1e34f846508f..c372385cfc3f 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -212,6 +212,7 @@ config ACPI_VIDEO
tristate "Video"
depends on X86 && BACKLIGHT_CLASS_DEVICE
depends on INPUT
+ depends on ACPI_WMI
select THERMAL
help
This driver implements the ACPI Extensions For Display Adapters
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index 8c2863403040..7b89dc9a04a2 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -75,6 +75,35 @@ find_video(acpi_handle handle, u32 lvl, void *context, void
**rv)
return AE_OK;
}
+#define WMI_BRIGHTNESS_METHOD_SOURCE 2
+#define WMI_BRIGHTNESS_MODE_GET0
+#define WMI_BRIGHTNESS_SOURCE_EC 2
+
+struct wmi_brightness_args {
+ u32 mode;
+ u32 val;
+ u32 ret;
+ u32 ignored[3];
+};
+
+static bool nvidia_wmi_ec_supported(void)
+{
+ struct wmi_brightness_args args = {
+ .mode = WMI_BRIGHTNESS_MODE_GET,
+ .val = 0,
+ .ret = 0,
+ };
+ struct acpi_buffer buf = { (acpi_size)sizeof(args), };
+ acpi_status status;
+
+ status = wmi_evaluate_method("603E9613-EF25-4338-A3D0-C46177516DB7", 0,
+WMI_BRIGHTNESS_METHOD_SOURCE, , );
+ if (ACPI_FAILURE(status))
+ return false;
+
+ return args.ret == WMI_BRIGHTNESS_SOURCE_EC;
+}
+
The code duplication here with nvidia-wmi-ec-backlight.c is a little
unfortunate. Can we move the constants, struct definition, and WMI GUID
from that file to a header file that's used both by the EC backlight
driver and the ACPI video driver?
I was thinking it might be nice to add a wrapper around
wmi_brightness_notify() in nvidia-wmi-ec-backlight.c that does this
source == WMI_BRIGHTNESS_SOURCE_EC test, and then export it so that it
can be called both here and in the EC backlight driver's probe routine,
but then I guess that would make video.ko depend on
nvidia-wmi-ec-backlight.ko, which seems wrong. It also seems wrong to
implement the WMI plumbing in the ACPI video driver, and export it so
that the EC backlight driver can use it, so I guess I can live with the
duplication of the relatively simple WMI stuff here, it would just be
nice to not have to define all of the API constants, structure, and GUID
twice.
/* Force to use vendor driver when the ACPI device is known to be
* buggy */
static int video_detect_force_vendor(const struct dmi_system_id *d)
@@ -518,6 +547,7 @@ static const struct dmi_system_id video_detect_dmi_table[]
= {
static enum acpi_backlight_type __acpi_video_get_backlight_type(bool native)
{
static DEFINE_MUTEX(init_mutex);
+ static bool nvidia_wmi_ec_present;
static bool native_available;
static bool init_done;
static long video_caps;
@@ -530,6 +560,7 @@ static enum acpi_backlight_type
__acpi_video_get_backlight_type(bool native)
acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
ACPI_UINT32_MAX, find_video, NULL,
_caps, NULL);
+ nvidia_wmi_ec_present = nvidia_wmi_ec_supported();
init_done = true;
}
if (native)
@@ -547,6 +578,10 @@ static enum acpi_backlight_type
__acpi_video_get_backlight_type(bool native)
if (acpi_backlight_dmi != acpi_backlight_undef)
return acpi_backlight_dmi;
+ /* Special cases such as nvidia_wmi_ec and apple gmux. */
+ if (nvidia_wmi_ec_present)
+ return acpi_backlight_nvidia_wmi_ec;
Should there also be a change to the EC backlight driver to call
acpi_video_get_backlight_type() and make sure we get
acpi_backlight_nvidia_wmi_ec? I don't see such a change in this patch
series; I could implement it (and test it) against your patch if there's
some reason you didn't do so with the current patchset.
+
/* On systems with ACPI video use either native or ACPI video. */
if (video_caps & ACPI_VIDEO_BACKLIGHT) {
Re: [PATCH 00/14] drm/kms: Stop registering multiple /sys/class/backlight devs for a single display
On 5/18/22 03:44, Jani Nikula wrote:
On Tue, 17 May 2022, Hans de Goede wrote:
Hi All,
As mentioned in my RFC titled "drm/kms: control display brightness through
drm_connector properties":
https://lore.kernel.org/dri-devel/0d188965-d809-81b5-74ce-7d30c49fe...@redhat.com/
The first step towards this is to deal with some existing technical debt
in backlight handling on x86/ACPI boards, specifically we need to stop
registering multiple /sys/class/backlight devs for a single display.
I guess my question here is, how do you know it's for a *single*
display?
There are already designs out there with two flat panels, with
independent brightness controls. They're still rare and I don't think we
handle them very well. But we've got code to register multiple native
backlight interfaces, see e.g. commit 20f85ef89d94 ("drm/i915/backlight:
use unique backlight device names").
This is one of my concerns as well. There are a small number of (mostly
somewhat old) designs with more than one internal panel. Since the
intent here is to tie the new backlight UAPI to DRM connectors, I think
it should remain possible to address each panel individually (the goal
is to stop having multiple backlight handlers, all of which control the
same display, not to stop having multiple backlight handlers in
general). Where I think it might get tricky is when the same display
might be driven by one GPU or another at different times, for example,
with a switchable mux. Notebook designs which use the
nvidia-wmi-ec-backlight driver will use the same backlight handler for
the display regardless of which GPU is currently driving it, but I
believe there are other designs where the same panel might have
backlight controlled by either the iGPU or the dGPU, depending on the
mux state.
So imagine a design where one of the panels needs backlight control via
ACPI and the other via native backlight control. Granted, I don't know
if one exists, but I think it's very much in the realm of possible
things the OEMs might do. For example, have an EC PWM for primary panel
backlight, and use GPU PWM for secondary. How do you know you actually
do need to register two interfaces?
The "how do you know" question is one I am wondering as well. I need to
check with some of our backlight experts here at NVIDIA to figure out
how exactly reliably we are able to make this determination.
I'm fine with dealing with such cases as they arise to avoid
over-engineering up front, but I also don't want us to completely paint
ourselves in a corner either.
BR,
Jani.
This series implements my RFC describing my plan for these cleanups:
https://lore.kernel.org/dri-devel/98519ba0-7f18-201a-ea34-652f50343...@redhat.com/
Specifically patches 1-6 implement the "Fixing kms driver unconditionally
register their "native" backlight dev" part.
And patches 7-14 implement the "Fixing acpi_video0 getting registered for
a brief time" time.
Note this series does not deal yet with the "Other issues" part, I plan
to do a follow up series for that.
The changes in this series are good to have regardless of the further
"drm/kms: control display brightness through drm_connector properties"
plans. So I plan to push these upstream once they are ready (once
reviewed). Since this crosses various subsystems / touches multiple
kms drivers my plan is to provide an immutable branch based on say
5.19-rc1 and then have that get merged into all the relevant trees.
Please review.
Regards,
Hans
Hans de Goede (14):
ACPI: video: Add a native function parameter to
acpi_video_get_backlight_type()
drm/i915: Don't register backlight when another backlight should be
used
drm/amdgpu: Don't register backlight when another backlight should be
used
drm/radeon: Don't register backlight when another backlight should be
used
drm/nouveau: Don't register backlight when another backlight should be
used
ACPI: video: Drop backlight_device_get_by_type() call from
acpi_video_get_backlight_type()
ACPI: video: Remove acpi_video_bus from list before tearing it down
ACPI: video: Simplify acpi_video_unregister_backlight()
ACPI: video: Make backlight class device registration a separate step
ACPI: video: Remove code to unregister acpi_video backlight when a
native backlight registers
drm/i915: Call acpi_video_register_backlight()
drm/nouveau: Register ACPI video backlight when nv_backlight
registration fails
drm/amdgpu: Register ACPI video backlight when skipping amdgpu
backlight registration
drm/radeon: Register ACPI video backlight when skipping radeon
backlight registration
drivers/acpi/acpi_video.c | 69 ++-
drivers/acpi/video_detect.c | 53 +++---
drivers/gpu/drm/Kconfig | 2 +
.../gpu/drm/amd/amdgpu/atombios_encoders.c| 14 +++-
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 9 +++
Re: [PATCH 09/14] ACPI: video: Make backlight class device registration a separate step
On 5/20/22 16:41, Daniel Dadap wrote: On 5/17/22 10:23, Hans de Goede wrote: On x86/ACPI boards the acpi_video driver will usually initializing before the kms driver (except i915). This causes /sys/class/backlight/acpi_video0 to show up and then the kms driver registers its own native backlight device after which the drivers/acpi/video_detect.c code unregisters the acpi_video0 device (when acpi_video_get_backlight_type()==native). This means that userspace briefly sees 2 devices and the disappearing of acpi_video0 after a brief time confuses the systemd backlight level save/restore code, see e.g.: https://bbs.archlinux.org/viewtopic.php?id=269920 To fix this make backlight class device registration a separate step done by a new acpi_video_register_backlight() function. The intend is for this to be called by the drm/kms driver *after* it is done setting up its own native backlight device. So that acpi_video_get_backlight_type() knows if a native backlight will be available or not at acpi_video backlight registration time, avoiding the add + remove dance. If I'm understanding this correctly, it seems we will want to call acpi_video_register_backlight() from the NVIDIA proprietary driver in a fallback path in case the driver's own GPU-controlled backlight handler either should not be used, or fails to register. That sounds reasonable enough, but I'm not sure what should be done about drivers like nvidia-wmi-ec-backlight, which are independent of the GPU hardware, and wouldn't be part of the acpi_video driver, either. There are a number of other similar vendor-y/platform-y type backlight drivers in drivers/video/backlight and drivers/platform/x86 that I think would be in a similar situation. From a quick skim of the ACPI video driver, it seems that perhaps nvidia-wmi-ec-backlight is missing a call to acpi_video_set_dmi_backlight_type(), perhaps with the acpi_backlight_vendor value? But I'm not familiar enough with this code to be sure that nobody will be checking acpi_video_get_backlight_type() before nvidia-wmi-ec-backlight loads. I'll take a closer look to try to convince myself that it makes sense. Note the new acpi_video_register_backlight() function is also called from a delayed work to ensure that the acpi_video backlight devices does get registered if necessary even if there is no drm/kms driver or when it is disabled. It sounds like maybe everything should be fine as long as nvidia-wmi-ec-backlight (and other vendor-y/platform-y type drivers) gets everything set up before the delayed work which calls acpi_video_register_backlight()? But then is it really necessary to explicitly call acpi_video_register_backlight() from the DRM drivers if it's going to be called later if no GPU driver registered a backlight handler, anyway? Then we'd just need to make sure that the iGPU and dGPU drivers won't attempt to register a backlight handler on systems where a vendor-y/platform-y driver is supposed to handle the backlight instead, which sounds like it has the potential to be quite messy. Ah, I see you addressed this concern in your RFC (sorry for missing that, earlier): The above only takes native vs acpi_video backlight issues into account, there are also a couple of other scenarios which may lead to multiple backlight-devices getting registered: 1) Apple laptops using the apple_gmux driver 2) Nvidia laptops using the (new) nvidia-wmi-ec-backlight driver 3) drivers/platform/x86 drivers calling acpi_video_set_dmi_backlight_type() to override the normal acpi_video_get_backlight_type() heuristics after the native/acpi_video drivers have already loaded The plan for 1) + 2) is to extend the acpi_backlight_type enum with acpi_backlight_gmux and acpi_backlight_nvidia_wmi_ec values and to add detection of these 2 to acpi_video_get_backlight_type(). Is there a reason these shouldn't have the same, generic, type, rather than separate, driver-specific ones? I don't foresee any situation where a system would need to use these two particular drivers simultaneously. Multiple DRM drivers can coexist on the same system, and even though the goal here is to remove the existing "multiple backlight interfaces for the same panel" situation, there shouldn't be any reason why more than one DRM driver couldn't register backlight handlers at the same time, so long as they are driving distinct panels. If we don't need separate backlight types for individual DRM drivers, why should we have them for apple_gmux and nvidia_wmi_ec_backlight? I still think that relying on the GPU drivers to correctly know whether they should register their backlight handlers when a platform-level device is supposed to register one instead might be easier said than done, especially on systems where the same panel could potentially be driven by more than one GPU (but not at the same time). Recall that on at least one system, both amdgpu and the NVIDIA propriet
Re: [PATCH 09/14] ACPI: video: Make backlight class device registration a separate step
On 5/17/22 10:23, Hans de Goede wrote:
On x86/ACPI boards the acpi_video driver will usually initializing before
the kms driver (except i915). This causes /sys/class/backlight/acpi_video0
to show up and then the kms driver registers its own native backlight
device after which the drivers/acpi/video_detect.c code unregisters
the acpi_video0 device (when acpi_video_get_backlight_type()==native).
This means that userspace briefly sees 2 devices and the disappearing of
acpi_video0 after a brief time confuses the systemd backlight level
save/restore code, see e.g.:
https://bbs.archlinux.org/viewtopic.php?id=269920
To fix this make backlight class device registration a separate step
done by a new acpi_video_register_backlight() function. The intend is for
this to be called by the drm/kms driver *after* it is done setting up its
own native backlight device. So that acpi_video_get_backlight_type() knows
if a native backlight will be available or not at acpi_video backlight
registration time, avoiding the add + remove dance.
If I'm understanding this correctly, it seems we will want to call
acpi_video_register_backlight() from the NVIDIA proprietary driver in a
fallback path in case the driver's own GPU-controlled backlight handler
either should not be used, or fails to register. That sounds reasonable
enough, but I'm not sure what should be done about drivers like
nvidia-wmi-ec-backlight, which are independent of the GPU hardware, and
wouldn't be part of the acpi_video driver, either. There are a number of
other similar vendor-y/platform-y type backlight drivers in
drivers/video/backlight and drivers/platform/x86 that I think would be
in a similar situation.
From a quick skim of the ACPI video driver, it seems that perhaps
nvidia-wmi-ec-backlight is missing a call to
acpi_video_set_dmi_backlight_type(), perhaps with the
acpi_backlight_vendor value? But I'm not familiar enough with this code
to be sure that nobody will be checking acpi_video_get_backlight_type()
before nvidia-wmi-ec-backlight loads. I'll take a closer look to try to
convince myself that it makes sense.
Note the new acpi_video_register_backlight() function is also called from
a delayed work to ensure that the acpi_video backlight devices does get
registered if necessary even if there is no drm/kms driver or when it is
disabled.
It sounds like maybe everything should be fine as long as
nvidia-wmi-ec-backlight (and other vendor-y/platform-y type drivers)
gets everything set up before the delayed work which calls
acpi_video_register_backlight()? But then is it really necessary to
explicitly call acpi_video_register_backlight() from the DRM drivers if
it's going to be called later if no GPU driver registered a backlight
handler, anyway? Then we'd just need to make sure that the iGPU and dGPU
drivers won't attempt to register a backlight handler on systems where a
vendor-y/platform-y driver is supposed to handle the backlight instead,
which sounds like it has the potential to be quite messy.
Recall that on at least one system, both amdgpu and the NVIDIA
proprietary driver registered a handler even when it shouldn't:
https://patchwork.kernel.org/project/platform-driver-x86/patch/20220316203325.2242536-1-dda...@nvidia.com/
- I didn't have direct access to this system, but the fact that the
NVIDIA driver registered a handler was almost certainly a bug in either
the driver or the system's firmware (on other systems with the same type
of backlight hardware, NVIDIA does not register a handler), and I
imagine the same is true of the amdgpu driver. In all likelihood nouveau
would have probably tried to register one too; I am not certain whether
the person who reported the issue to me had tested with nouveau. I'm not
convinced that the GPU drivers can reliably determine whether or not
they are supposed to register, but maybe cases where they aren't, such
as the system mentioned above, are supposed to be handled in a quirks
table somewhere.
Signed-off-by: Hans de Goede
---
drivers/acpi/acpi_video.c | 45 ---
include/acpi/video.h | 2 ++
2 files changed, 44 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 95d4868f6a8c..79e75dc86243 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -31,6 +31,12 @@
#define ACPI_VIDEO_BUS_NAME "Video Bus"
#define ACPI_VIDEO_DEVICE_NAME"Video Device"
+/*
+ * Display probing is known to take up to 5 seconds, so delay the fallback
+ * backlight registration by 5 seconds + 3 seconds for some extra margin.
+ */
+#define ACPI_VIDEO_REGISTER_BACKLIGHT_DELAY(8 * HZ)
+
#define MAX_NAME_LEN 20
MODULE_AUTHOR("Bruno Ducrot");
@@ -80,6 +86,9 @@ static LIST_HEAD(video_bus_head);
static int acpi_video_bus_add(struct acpi_device *device);
static int acpi_video_bus_remove(struct acpi_device *device);
static void
Re: [Nouveau] [PATCH 1/4] drm: retrieve EDID via ACPI _DDC method
Thanks, Lukas. I've incorporated your feedback into my local tree, but
will wait for additional feedback from the individual DRM driver
maintainers before sending out a series v2.
On 8/8/20 5:11 PM, Lukas Wunner wrote:
On Mon, Jul 27, 2020 at 03:53:54PM -0500, Daniel Dadap wrote:
+ for (i = 0; i < num_dod_entries; i++) {
+ if (adr == dod_entries[i]) {
+ ret = do_acpi_ddc(child->handle);
+
+ if (ret != NULL)
+ goto done;
I guess ideally we'd want to correlate the display objects with
drm_connectors or at least constrain the search to Display Type
"Internal/Integrated Digital Flat Panel" instead of picking the
first EDID found. Otherwise we might erroneously use the DDC
for an externally attached display.
Yes, we'd definitely need a way to do this if this functionality ever
needs to be extended to systems with more than one _DDC method.
Unfortunately, this will be much easier said than done, since I'm not
aware of any way to reliably do map _DOD entries to connectors in a GPU
driver, especially when we're talking about possibly correlating
connectors on multiple GPUs which mux to the same internal display or
external connector. All systems which I am aware of that implement ACPI
_DDC do so for a single internal panel. I don't believe there's any
reason to ever retrieve an EDID via ACPI _DDC for an external panel, but
a hypothetical design with multiple internal panels, more than one of
which needs to retrieve an EDID via ACPI _DDC, would certainly be
problematic.
On at least the system I'm working with for the various switcheroo and
platform-x86 driver patches I've recently sent off, the dGPU has an ACPI
_DOD table and one _DDC method corresponding to one of the _DOD entries,
but the iGPU has neither a _DOD table nor a _DDC method. Either GPU can
be connected to the internal panel via the dynamically switchable mux,
and the internal panel's EDID is available via _DDC to allow a
disconnected GPU to read the EDID. Since only the DGPU has _DOD and
_DDC, and there's no obvious way to associate connectors on the iGPU
with connectors on the dGPU, I've implemented the ACPI _DDC EDID
retrieval with the "first available" implementation you see here. I'm
open to other ideas if you have them, but didn't see a good way to
search for the "right" _DDC implementation should there be more than one.
As for preventing the ACPI EDID retrieval from being used for external
panels, I've done this in the individual DRM drivers that call into the
new drm_edid_acpi() API since it seemed that each DRM driver had its own
way of distinguishing display connector types. If there's a good way to
filter for internal panels in DRM core, I'd be happy to do that instead.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 4/4] radeon: fall back to ACPI EDID retrieval
On 7/28/20 1:50 AM, Christian König wrote:
Am 27.07.20 um 22:53 schrieb Daniel Dadap:
Fall back to retrieving the EDID via the ACPI _DDC method, when present
for notebook internal panels, when retrieving BIOS-embedded EDIDs.
Signed-off-by: Daniel Dadap
---
drivers/gpu/drm/radeon/radeon_combios.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_combios.c
b/drivers/gpu/drm/radeon/radeon_combios.c
index c3e49c973812..de801d9fca54 100644
--- a/drivers/gpu/drm/radeon/radeon_combios.c
+++ b/drivers/gpu/drm/radeon/radeon_combios.c
@@ -401,9 +401,8 @@ bool radeon_combios_check_hardcoded_edid(struct
radeon_device *rdev)
struct edid *
radeon_bios_get_hardcoded_edid(struct radeon_device *rdev)
{
- struct edid *edid;
-
if (rdev->mode_info.bios_hardcoded_edid) {
+ struct edid *edid;
That's an unrelated an incorrect style change. You need a blank line
after declaration.
Ah, yes, that doesn't really need to be changed. I'll remove it from
this patch. Would a separate patch to change the scope of that
declaration (with a blank line after) be welcome, or should I just leave
it alone?
edid =
kmalloc(rdev->mode_info.bios_hardcoded_edid_size, GFP_KERNEL);
if (edid) {
memcpy((unsigned char *)edid,
@@ -412,7 +411,8 @@ radeon_bios_get_hardcoded_edid(struct
radeon_device *rdev)
return edid;
}
}
- return NULL;
+
+ return drm_get_edid_acpi();
In general a good idea, but I'm wondering if we should really do this so
unconditionally here.
I'm not personally aware of any AMD notebook designs that require the
ACPI _DDC EDID retrieval. I've only seen it on NVIDIA+Intel hybrid
systems and on a small number of NVIDIA discrete-only systems. I just
figured I'd update the radeon DRM-KMS driver while updating i915 and
Nouveau, for completeness, as it could be helpful should such a design
exist. As for whether there should be some condition around this, I
suppose that's reasonable, but I'm not really sure what would make sense
as a condition. As it stands, drm_edid_acpi() only returns a value if at
least one of the VGA or 3D controllers on the system provides an ACPI
_DDC method, and if that ACPI method successfully returns an EDID.
On the caller's end, it's currently part of the path where the radeon
driver is already trying to fall back to a hardcoded EDID provided by
the system. Perhaps instead if we call it within the LVDS || eDP
condition here, instead?
if (rdev->is_atom_bios) {
/* some laptops provide a hardcoded edid in rom for LCDs */
if (((connector->connector_type == DRM_MODE_CONNECTOR_LVDS) ||
(connector->connector_type == DRM_MODE_CONNECTOR_eDP)))
radeon_connector->edid =
radeon_bios_get_hardcoded_edid(rdev);
} else {
/* some servers provide a hardcoded edid in rom for KVMs */
radeon_connector->edid = radeon_bios_get_hardcoded_edid(rdev);
}
That would be more in line with the changes in this patchset for i915
and nouveau.
Regards,
Christian.
}
static struct radeon_i2c_bus_rec combios_setup_i2c_bus(struct
radeon_device *rdev,
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 1/4] drm: retrieve EDID via ACPI _DDC method
Some notebook computer systems expose the EDID for the internal
panel via the ACPI _DDC method. On some systems this is because
the panel does not populate the hardware DDC lines, and on some
systems with dynamic display muxes, _DDC is implemented to allow
the internal panel's EDID to be read at any time, regardless of
how the mux is switched.
The _DDC method can be implemented for each individual display
output, so there could be an arbitrary number of outputs exposing
their EDIDs via _DDC; however, in practice, this has only been
implemented so far on systems with a single panel, so the current
implementation of drm_get_edid_acpi() walks the outputs listed by
each GPU's ACPI _DOD method and returns the first EDID successfully
retrieved by any attached _DDC method. It may be necessary in the
future to allow for the retrieval of distinct EDIDs for different
output devices, but in order to do so, it will first be necessary
to develop a way to correlate individual DRM outputs with their
corresponding entities in ACPI.
Signed-off-by: Daniel Dadap
---
drivers/gpu/drm/drm_edid.c | 161 +
include/drm/drm_edid.h | 1 +
2 files changed, 162 insertions(+)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 116451101426..f66d6bf048c6 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -34,6 +34,7 @@
#include
#include
#include
+#include
#include
#include
@@ -2058,6 +2059,166 @@ struct edid *drm_get_edid_switcheroo(struct
drm_connector *connector,
}
EXPORT_SYMBOL(drm_get_edid_switcheroo);
+#if defined(CONFIG_ACPI) && defined(CONFIG_PCI)
+static u64 *get_dod_entries(acpi_handle handle, int *count)
+{
+ acpi_status status;
+ struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
+ union acpi_object *dod;
+ int i;
+ u64 *ret = NULL;
+
+ *count = 0;
+
+ status = acpi_evaluate_object(handle, "_DOD", NULL, );
+
+ if (ACPI_FAILURE(status))
+ return NULL;
+
+ dod = buf.pointer;
+
+ if (dod == NULL || dod->type != ACPI_TYPE_PACKAGE)
+ goto done;
+
+ ret = kmalloc_array(dod->package.count, sizeof(*ret), GFP_KERNEL);
+ if (ret == NULL)
+ goto done;
+
+ for (i = 0; i < dod->package.count; i++) {
+ if (dod->package.elements[i].type != ACPI_TYPE_INTEGER)
+ continue;
+ ret[*count] = dod->package.elements[i].integer.value;
+ (*count)++;
+ }
+
+done:
+ kfree(buf.pointer);
+ return ret;
+}
+
+static void *do_acpi_ddc(acpi_handle handle)
+{
+ int i;
+ void *ret = NULL;
+
+ /*
+* The _DDC spec defines an integer argument for specifying the size of
+* the EDID to be retrieved. A value of 1 requests a 128-byte EDID, and
+* a value of 2 requests a 256-byte EDID. Attempt the larger read first.
+*/
+ for (i = 2; i >= 1; i--) {
+ struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
+ union acpi_object arg = { ACPI_TYPE_INTEGER };
+ struct acpi_object_list in = { 1, };
+ union acpi_object *edid;
+ acpi_status status;
+
+ arg.integer.value = i;
+ status = acpi_evaluate_object(handle, "_DDC", , );
+ edid = out.pointer;
+
+ if (ACPI_SUCCESS(status))
+ ret = edid->buffer.pointer;
+
+ kfree(edid);
+
+ if (ret)
+ break;
+ }
+
+ return ret;
+}
+
+static struct edid *first_edid_from_acpi_ddc(struct pci_dev *pdev)
+{
+ acpi_handle handle;
+ acpi_status status;
+ struct acpi_device *device = NULL;
+ struct edid *ret = NULL;
+ int num_dod_entries;
+ u64 *dod_entries = NULL;
+ struct list_head *node, *next;
+
+ handle = ACPI_HANDLE(>dev);
+ if (handle == NULL)
+ return NULL;
+
+ dod_entries = get_dod_entries(handle, _dod_entries);
+ if (dod_entries == NULL || num_dod_entries == 0)
+ goto done;
+
+ status = acpi_bus_get_device(handle, );
+ if (ACPI_FAILURE(status) || device == NULL)
+ goto done;
+
+ list_for_each_safe(node, next, >children) {
+ struct acpi_device *child;
+ u64 adr;
+ int i;
+
+ child = list_entry(node, struct acpi_device, node);
+ if (child == NULL)
+ continue;
+
+ status = acpi_evaluate_integer(child->handle, "_ADR", NULL,
+ );
+ if (ACPI_FAILURE(status))
+ continue;
+
+ for (i = 0; i < num_dod_entries; i++) {
+ if (adr == dod_ent
[PATCH 0/4] drm: add support for retrieving EDID via ACPI _DDC
Some notebook systems provide the EDID for the internal panel via the _DDC method in ACPI, instead of or in addition to providing the EDID via DDC on LVDS/eDP. Add a DRM helper to search for an ACP _DDC method under the ACPI namespace for each VGA/3D controller, and return the first EDID successfully retrieved via _DDC. Update the i915, nouveau, and radeon DRM-KMS drivers to fall back to retrieving the EDID via ACPI _DDC on notebook internal display panels after failing to retrieve an EDID via other means. This is useful for retrieving an internal panel's EDID both on hybrid graphics systems with muxed display output, when the display is muxed away, as well as on a small number of non-muxed and/or non-hybrid systems where ACPI _DDC is the only means of accessing the EDID for the internal panel. Daniel Dadap (4): drm: retrieve EDID via ACPI _DDC method i915: fall back to ACPI EDID retrieval nouveau: fall back to ACPI EDID retrieval radeon: fall back to ACPI EDID retrieval drivers/gpu/drm/drm_edid.c | 161 drivers/gpu/drm/i915/display/intel_dp.c | 8 +- drivers/gpu/drm/i915/display/intel_lvds.c | 4 + drivers/gpu/drm/nouveau/nouveau_connector.c | 6 + drivers/gpu/drm/radeon/radeon_combios.c | 6 +- include/drm/drm_edid.h | 1 + 6 files changed, 182 insertions(+), 4 deletions(-) -- 2.18.4 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 2/4] i915: fall back to ACPI EDID retrieval
Fall back to retrieving the EDID via the ACPI _DDC method, when present
for notebook internal panels, when EDID retrieval via the standard EDID
paths is unsuccessful.
Signed-off-by: Daniel Dadap
---
drivers/gpu/drm/i915/display/intel_dp.c | 8 +++-
drivers/gpu/drm/i915/display/intel_lvds.c | 4
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index 804b1d966f66..ff402cef8183 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5657,6 +5657,7 @@ static struct edid *
intel_dp_get_edid(struct intel_dp *intel_dp)
{
struct intel_connector *intel_connector = intel_dp->attached_connector;
+ struct edid *edid;
/* use cached edid if we have one */
if (intel_connector->edid) {
@@ -5666,8 +5667,13 @@ intel_dp_get_edid(struct intel_dp *intel_dp)
return drm_edid_duplicate(intel_connector->edid);
} else
- return drm_get_edid(_connector->base,
+ edid = drm_get_edid(_connector->base,
_dp->aux.ddc);
+
+ if (!edid && intel_dp_is_edp(intel_dp))
+ edid = drm_get_edid_acpi();
+
+ return edid;
}
static void
diff --git a/drivers/gpu/drm/i915/display/intel_lvds.c
b/drivers/gpu/drm/i915/display/intel_lvds.c
index 9a067effcfa0..811eea3f5d9f 100644
--- a/drivers/gpu/drm/i915/display/intel_lvds.c
+++ b/drivers/gpu/drm/i915/display/intel_lvds.c
@@ -946,6 +946,10 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
else
edid = drm_get_edid(connector,
intel_gmbus_get_adapter(dev_priv, pin));
+
+ if (!edid)
+ edid = drm_get_edid_acpi();
+
if (edid) {
if (drm_add_edid_modes(connector, edid)) {
drm_connector_update_edid_property(connector,
--
2.18.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 4/4] radeon: fall back to ACPI EDID retrieval
Fall back to retrieving the EDID via the ACPI _DDC method, when present
for notebook internal panels, when retrieving BIOS-embedded EDIDs.
Signed-off-by: Daniel Dadap
---
drivers/gpu/drm/radeon/radeon_combios.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_combios.c
b/drivers/gpu/drm/radeon/radeon_combios.c
index c3e49c973812..de801d9fca54 100644
--- a/drivers/gpu/drm/radeon/radeon_combios.c
+++ b/drivers/gpu/drm/radeon/radeon_combios.c
@@ -401,9 +401,8 @@ bool radeon_combios_check_hardcoded_edid(struct
radeon_device *rdev)
struct edid *
radeon_bios_get_hardcoded_edid(struct radeon_device *rdev)
{
- struct edid *edid;
-
if (rdev->mode_info.bios_hardcoded_edid) {
+ struct edid *edid;
edid = kmalloc(rdev->mode_info.bios_hardcoded_edid_size,
GFP_KERNEL);
if (edid) {
memcpy((unsigned char *)edid,
@@ -412,7 +411,8 @@ radeon_bios_get_hardcoded_edid(struct radeon_device *rdev)
return edid;
}
}
- return NULL;
+
+ return drm_get_edid_acpi();
}
static struct radeon_i2c_bus_rec combios_setup_i2c_bus(struct radeon_device
*rdev,
--
2.18.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 3/4] nouveau: fall back to ACPI EDID retrieval
Fall back to retrieving the EDID via the ACPI _DDC method, when present
for notebook internal panels, when EDID retrieval via the standard EDID
paths is unsuccessful.
Signed-off-by: Daniel Dadap
---
drivers/gpu/drm/nouveau/nouveau_connector.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 9a9a7f5003d3..95836a02a06b 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -581,6 +581,12 @@ nouveau_connector_detect(struct drm_connector *connector,
bool force)
else
nv_connector->edid = drm_get_edid(connector, i2c);
+ if (!nv_connector->edid &&
+ (nv_connector->type == DCB_CONNECTOR_LVDS ||
+ nv_connector->type == DCB_CONNECTOR_eDP)) {
+ nv_connector->edid = drm_get_edid_acpi();
+ }
+
drm_connector_update_edid_property(connector,
nv_connector->edid);
if (!nv_connector->edid) {
--
2.18.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
