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

[Daniel Dadap <ddadap@nvidia.com>]

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 <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..4dc7fb865083 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
>>
>> 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 <ddadap@nvidia.com>


> 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), &args };
>>> +    acpi_status status;
>>> +
>>> +    status = wmi_evaluate_method(WMI_BRIGHTNESS_GUID, 0,
>>> +                     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)