Hi,
On 7/15/22 17:32, Daniel Dadap wrote:
>
>
>> On Jul 15, 2022, at 06:59, Hans de Goede <hdego...@redhat.com> 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.
>>
>>> 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.
>>
>>>
>>>
>>>> +
>>>> /* 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...
>>
>
> Yes, thank you. I don’t have to work with Kconfig much, but that reasoning
> lined up with my best guess. Sorry for not looking at the other patches in
> the series, or at Nouveau’s Kconfig more closely.
>
> I will work on and test the three patches we discussed above. Would you
> prefer if I sent them and set In-reply-to a message in this thread, or as a
> separate message with no In-reply-to?
I have no real preference. Whatever is easiest for you.
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)
>>>
>>
