Re: [PATCH v3 17/31] ACPI: video: Add Nvidia WMI EC brightness control detection (v2)

From: Daniel Dadap <ddadap@nvidia.com>
To: "Hans de Goede" <hdegoede@redhat.com>,
	"Ben Skeggs" <bskeggs@redhat.com>,
	"Karol Herbst" <kherbst@redhat.com>, Lyude <lyude@redhat.com>,
	"Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
	"Maxime Ripard" <mripard@kernel.org>,
	"Thomas Zimmermann" <tzimmermann@suse.de>,
	"Jani Nikula" <jani.nikula@linux.intel.com>,
	"Joonas Lahtinen" <joonas.lahtinen@linux.intel.com>,
	"Rodrigo Vivi" <rodrigo.vivi@intel.com>,
	"Tvrtko Ursulin" <tvrtko.ursulin@linux.intel.com>,
	"Alex Deucher" <alexander.deucher@amd.com>,
	"Christian König" <christian.koenig@amd.com>,
	Xinhui <Xinhui.Pan@amd.com>,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	"Mika Westerberg" <mika.westerberg@linux.intel.com>,
	"Lukas Wunner" <lukas@wunner.de>,
	"Mark Gross" <markgross@kernel.org>,
	"Andy Shevchenko" <andy@kernel.org>
Cc: nouveau@lists.freedesktop.org, Daniel Vetter <daniel@ffwll.ch>,
	David Airlie <airlied@linux.ie>,
	intel-gfx <intel-gfx@lists.freedesktop.org>,
	dri-devel@lists.freedesktop.org, amd-gfx@lists.freedesktop.org,
	Len Brown <lenb@kernel.org>,
	linux-acpi@vger.kernel.org, platform-driver-x86@vger.kernel.org,
	"Rafael J . Wysocki" <rafael.j.wysocki@intel.com>
Subject: Re: [PATCH v3 17/31] ACPI: video: Add Nvidia WMI EC brightness control detection (v2)
Date: Thu, 18 Aug 2022 14:44:00 -0500	[thread overview]
Message-ID: <12c58f49-835c-c247-f34a-9b2f439b8db8@nvidia.com> (raw)
In-Reply-To: <20220818184302.10051-18-hdegoede@redhat.com>

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 <rafael.j.wysocki@intel.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   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 <linux/dmi.h>
>   #include <linux/module.h>
>   #include <linux/pci.h>
> +#include <linux/platform_data/x86/nvidia-wmi-ec-backlight.h>
>   #include <linux/types.h>
>   #include <linux/workqueue.h>
>   #include <acpi/video.h>
> @@ -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), &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, &buf, &buf);
> +	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,
>   				    &video_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
> @@ -23,6 +23,8 @@ config DRM_I915
>   	# but for select to work, need to select ACPI_VIDEO's dependencies, ick
>   	select BACKLIGHT_CLASS_DEVICE if ACPI
>   	select INPUT if ACPI
> +	select X86_PLATFORM_DEVICES if ACPI
> +	select ACPI_WMI if ACPI
>   	select ACPI_VIDEO if ACPI
>   	select ACPI_BUTTON if ACPI
>   	select SYNC_FILE
> diff --git a/include/acpi/video.h b/include/acpi/video.h
> index 0625806d3bbd..91578e77ac4e 100644
> --- a/include/acpi/video.h
> +++ b/include/acpi/video.h
> @@ -48,6 +48,7 @@ enum acpi_backlight_type {
>   	acpi_backlight_video,
>   	acpi_backlight_vendor,
>   	acpi_backlight_native,
> +	acpi_backlight_nvidia_wmi_ec,
>   };
>   
>   #if IS_ENABLED(CONFIG_ACPI_VIDEO)