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 <hdego...@redhat.com>
>>> ---
>>> 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), &args };
>>> + acpi_status status;
>>> +
>>> + status = wmi_evaluate_method("603E9613-EF25-4338-A3D0-C46177516DB7", 0,
>>> + WMI_BRIGHTNESS_METHOD_SOURCE, &buf, &buf);
>>> + 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.
>> 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,
>>> &video_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.
>
> I was thinking about this myself too and I decided it was not necessary since
> acpi_video_get_backlight_type() will always return
> acpi_backlight_nvidia_wmi_ec.
>
> But thinking more about this, that is not true, a user might try to force
> using a different backlight firmware interface by e.g. adding:
> acpi_backlight=video on the kernel commandline.
>
> So yes a patch adding something like this:
>
> if (acpi_video_get_backlight_type() != acpi_backlight_nvidia_wmi_ec)
> return -ENODEV;
>
> to the EC backlight driver would be very welcome.
I will also add a patch for this to v3 of the series myself.
Regards,
Hans
>
>>
>>
>>> +
>>> /* 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
>>
>>
>> I'm not sure I understand why the Intel DRM drivers pick up the additional
>> platform/x86 and WMI dependencies here. ACPI_VIDEO already depends on these,
>> doesn't it?
>
> It does.
>
>> If Kconfig doesn't otherwise automatically pull in an option's dependencies
>> when selecting that option
>
> Right that is the reason why this is done, for select the Kconfig block must
> also select all deps
>
>> then shouldn't Nouveau's Kconfig get updated as well?
>> It selects ACPI_VIDEO in some configuration cases.
>
> nouveau's Kconfig block already selects ACPI_WMI:
>
> config DRM_NOUVEAU
> tristate "Nouveau (NVIDIA) cards"
> ...
> select X86_PLATFORM_DEVICES if ACPI && X86
> select ACPI_WMI if ACPI && X86
> ...
> select ACPI_VIDEO if ACPI && X86
>
> That is why this patch does not add this.
>
>> (It looks like amdgpu doesn't currently select ACPI_VIDEO, maybe because it
>> doesn't depend on it the way the Intel drivers do: there are several
>> AMD+NVIDIA iGPU/dGPU designs out there which use this backlight interface.)
>
> Correct, but with this series amdgpu/radeon also start using ACPI_VIDEO
> functions so these patches:
>
> https://patchwork.freedesktop.org/patch/493650/
> https://patchwork.freedesktop.org/patch/493653/
>
> Add the necessary selects and I cheated a bit and also made
> them select ACPI_WMI already even though that is only
> necessary after this patch (which comes later in the series).
>
> I hope this answers al your questions...
>
> Regards,
>
> Hans
>
>
>
>>
>>
>>> 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)
>>
