| Message ID | 20240513033834.9174-1-dengxiang@nfschina.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | ACPI: video: Use vendor backlight on Lenovo X1 Carbon. | expand |
Hi dengxiang, On 5/13/24 5:38 AM, dengxiang wrote: > Lenovo X1 Carbon advertises both native and vendor backlight > control interfaces.But Linux defaults to picking the native ACPI > video backlight interface, which will make that the vendor > zx_backlight interface to not work. A couple of remarks / questions: 1. Looking at the strings you match on this is not for a Lenovo X1 Carbon, but rather for a Lenovo Kaitan model ? So it seems that the commit message and the comment for the quirk need some work. 2. I have never heard of a zx_backlight interface before and there certainly is no upstream driver providing this. I believe you need to explain what is going on in a bit more detail here and then we can see if this really is the best way to fix this. It seems that these Lenovo Kaitan laptops are using Zhaoxin Kaixian x86 processors with integrate graphics. I would expect the zx_backlight interface to be provided by the driver for the Zhaoxin Kaixian integrated graphics in this case. And if that is the case then the integrated graphics driver should use BACKLIGHT_RAW (aka native) for the backlight type and with that change this quirk should not be necessary . 3. Vendor specific backlight interfaces are normally only found on really old laptops. Since Windows XP laptops typically use the ACPI backlight interface and since Windows 8 they typically use the GPU's native backlight driver. So adding a quirk to use a vendor interface in 2024 is weird. Again can you explain in a lot more detail what is going on here, but I guess the backlight class device is provided by the driver for the integrated graphics and in that case the fix is to simply change the type of the backlight device registered by the igfx driver to BACKLIGHT_RAW. 4. You posted the same patch twice ? Regards, Hans > > Add a DMI quirk to force use of the vendor interface. > > Signed-off-by: dengxiang <dengxiang@nfschina.com> > --- > drivers/acpi/video_detect.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c > index 2cc3821b2b16..e647186b4e83 100644 > --- a/drivers/acpi/video_detect.c > +++ b/drivers/acpi/video_detect.c > @@ -875,6 +875,14 @@ static const struct dmi_system_id video_detect_dmi_table[] = { > DMI_MATCH(DMI_PRODUCT_NAME, "Mipad2"), > }, > }, > + { > + .callback = video_detect_force_vendor, > + /* Lenovo X1 Carbon */ > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "KaiTian"), > + DMI_MATCH(DMI_PRODUCT_NAME, "KaiTian X1 G1d"), > + }, > + }, > { }, > }; >
Hi Hans, > A couple of remarks / questions: > 1. Looking at the strings you match on this is not for a Lenovo X1 Carbon, > but rather for a Lenovo Kaitan model ? So it seems that the commit message > and the comment for the quirk need some work. ok, I will add DMI_PRODUCT_VERSION & DMI_BOARD_NAME to make a distinction between X1 Carbon and other kaitian models. > 2. I have never heard of a zx_backlight interface before and there certainly > is no upstream driver providing this. I believe you need to explain what > is going on in a bit more detail here and then we can see if this really is > the best way to fix this. It seems that these Lenovo Kaitan laptops are > using Zhaoxin Kaixian x86 processors with integrate graphics. I would expect > the zx_backlight interface to be provided by the driver for the Zhaoxin Kaixian > integrated graphics in this case. And if that is the case then the integrated > graphics driver should use BACKLIGHT_RAW (aka native) for the backlight type > and with that change this quirk should not be necessary . Yes, zx_backlight interface has been provided by the driver for the Zhaoxin Kaixian integrated graphics. Also use backlight_device_register("zx_backlight",...). Strangely enough, X1 Carbon laptops will generate two native acpi_video as belows: lrwxrwxrwx 1 root root 0 5月 14 16:20 acpi_video0 -> ../../devices/pci0000:00/0000:00:01.0/backlight/acpi_video0 lrwxrwxrwx 1 root root 0 5月 14 16:20 acpi_video1 -> ../../devices/pci0000:00/0000:00:01.0/backlight/acpi_video1 As you see, it will conflict with the same pci bus, then zx_blacklight interface can't be shown on the path /sys/class/backlight/. That is to say, zhaoxin driver contain key code as belows: #if DRM_VERSION_CODE >= KERNEL_VERSION(4, 2, 0) if(acpi_video_get_backlight_type() != acpi_backlight_vendor) { return ret; } #endif If i remove the key code, this laptops will generate two native acpi_video and zx_backlight on the sys backlight patch. Once add acpi_backlight=vendor parameter into kernel cmdline, just zx_backlight interface has been left on the sys path, which mean that both acpi_video0 and acpi_video1 interface can not be found. > 3. Vendor specific backlight interfaces are normally only found on really > old laptops. Since Windows XP laptops typically use the ACPI backlight > interface and since Windows 8 they typically use the GPU's native > backlight driver. So adding a quirk to use a vendor interface in 2024 is > weird. Again can you explain in a lot more detail what is going on here, > but I guess the backlight class device is provided by the driver for the > integrated graphics and in that case the fix is to simply change the type > of the backlight device registered by the igfx driver to BACKLIGHT_RAW. As mentioned in 2 questions above zhaoxin drivers had used backlight_device_register("zx_backlight"...) as BACKLIGHT_RAW. > 4. You posted the same patch twice ? Sorry, i was wrong to think that before patch would be missed by you. also i forgot about the time zone difference. I am sorry for any inconvenience that I have brought to you. Best Regards, dengxiang
Hi dengxian, On 5/15/24 5:45 AM, dengxiang wrote: > Hi Hans, > >> A couple of remarks / questions: > >> 1. Looking at the strings you match on this is not for a Lenovo X1 Carbon, >> but rather for a Lenovo Kaitan model ? So it seems that the commit message >> and the comment for the quirk need some work. > > ok, I will add DMI_PRODUCT_VERSION & DMI_BOARD_NAME to make a distinction between X1 Carbon and other kaitian models. > >> 2. I have never heard of a zx_backlight interface before and there certainly >> is no upstream driver providing this. I believe you need to explain what >> is going on in a bit more detail here and then we can see if this really is >> the best way to fix this. It seems that these Lenovo Kaitan laptops are >> using Zhaoxin Kaixian x86 processors with integrate graphics. I would expect >> the zx_backlight interface to be provided by the driver for the Zhaoxin Kaixian >> integrated graphics in this case. And if that is the case then the integrated >> graphics driver should use BACKLIGHT_RAW (aka native) for the backlight type >> and with that change this quirk should not be necessary . > > Yes, zx_backlight interface has been provided by the driver for the Zhaoxin Kaixian integrated graphics. Also use backlight_device_register("zx_backlight",...). > Strangely enough, X1 Carbon laptops will generate two native acpi_video as belows: > > lrwxrwxrwx 1 root root 0 5月 14 16:20 acpi_video0 -> ../../devices/pci0000:00/0000:00:01.0/backlight/acpi_video0 > lrwxrwxrwx 1 root root 0 5月 14 16:20 acpi_video1 -> ../../devices/pci0000:00/0000:00:01.0/backlight/acpi_video1 > > As you see, it will conflict with the same pci bus, then zx_blacklight interface can't be shown on the path /sys/class/backlight/. > That is to say, zhaoxin driver contain key code as belows: > #if DRM_VERSION_CODE >= KERNEL_VERSION(4, 2, 0) > if(acpi_video_get_backlight_type() != acpi_backlight_vendor) > { > return ret; > } > #endif > > If i remove the key code, this laptops will generate two native acpi_video and zx_backlight on the sys backlight patch. Once add acpi_backlight=vendor parameter into kernel cmdline, > just zx_backlight interface has been left on the sys path, which mean that both acpi_video0 and acpi_video1 interface can not be found. Ok, so the way this is supposed to work is as follows, there is a single acpi_video_get_backlight_type() function which all backlight drivers are supposed to use (and all in tree drivers do use). This looks like this (simplified a bit, see drivers/acpi/video_detect.c): enum acpi_backlight_type __acpi_video_get_backlight_type(bool native, bool *auto_detect) { ... /* Use ACPI video if available, except when native should be preferred. */ if ((video_caps & ACPI_VIDEO_BACKLIGHT) && !(native_available && prefer_native_over_acpi_video())) return acpi_backlight_video; /* Use native if available */ if (native_available) return acpi_backlight_native; /* ... long comment explaining this ... */ if (acpi_osi_is_win8()) return acpi_backlight_none; /* No ACPI video/native (old hw), use vendor specific fw methods. */ return acpi_backlight_vendor; } as you can see here acpi_backlight_video is only returned if available (which it is in this case) *and* there is no native GPU backlight driver or prefer_native_over_acpi_video() returns false. Since there is no way for this function to know a native GPU driver is supported it uses the native parameter passed to it for this, so native backlight drivers, like the Zhaoxin Kaixian integrated graphics driver must call a special helper, which internally calls the above function with native=true. I think not calling that special helper is why you see the acpi_video backlight devices, assuming you are using a recent mainline kernel. So that: #if DRM_VERSION_CODE >= KERNEL_VERSION(4, 2, 0) if (acpi_video_get_backlight_type() != acpi_backlight_vendor) { return ret; } #endif block you quoted should look like this when using recent upstream kernels: if (!acpi_video_backlight_use_native()) { return ret; } Although that return ret looks weird, maybe hardcode 0 for success (not registering is on purpose, so success ?) Or to keep things compatible with multiple kernel versions use: #if LINUX_VERSION_CODE >= KERNEL_VERSION(6, 1, 0) if (!acpi_video_backlight_use_native()) { return ret; } #elif DRM_VERSION_CODE >= KERNEL_VERSION(4, 2, 0) if (acpi_video_get_backlight_type() != acpi_backlight_vendor) { return ret; } #endif Please give this a try, I believe you will not need a quirk when the Zhaoxin Kaixian integrated graphics driver does the right thing. Regards, Hans
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c index 2cc3821b2b16..e647186b4e83 100644 --- a/drivers/acpi/video_detect.c +++ b/drivers/acpi/video_detect.c @@ -875,6 +875,14 @@ static const struct dmi_system_id video_detect_dmi_table[] = { DMI_MATCH(DMI_PRODUCT_NAME, "Mipad2"), }, }, + { + .callback = video_detect_force_vendor, + /* Lenovo X1 Carbon */ + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "KaiTian"), + DMI_MATCH(DMI_PRODUCT_NAME, "KaiTian X1 G1d"), + }, + }, { }, };
Lenovo X1 Carbon advertises both native and vendor backlight control interfaces.But Linux defaults to picking the native ACPI video backlight interface, which will make that the vendor zx_backlight interface to not work. Add a DMI quirk to force use of the vendor interface. Signed-off-by: dengxiang <dengxiang@nfschina.com> --- drivers/acpi/video_detect.c | 8 ++++++++ 1 file changed, 8 insertions(+)