Received-SPF: pass (google.com: domain of linux-kernel+bounces-180209-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <04271fb5-cca3-4bca-b98e-3fab8a78c724@redhat.com>
Date: Wed, 15 May 2024 19:29:16 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] ACPI: video: Use vendor backlight on Lenovo X1 Carbon.
To: dengxiang <dengxiang@nfschina.com>
Cc: lenb@kernel.org, linux-acpi@vger.kernel.org, linux-edac@vger.kernel.org,
 linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org,
 prime.zeng@hisilicon.com, rafael@kernel.org, tony.luck@intel.com,
 wanghuiqiang@huawei.com
References: <20240515034501.12772-1-dengxiang@nfschina.com>
Content-Language: en-US, nl
From: Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20240515034501.12772-1-dengxiang@nfschina.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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