linux-kernel - Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b85fe6f1-3c1b-4bca-25c5-7b0e6441ed27@redhat.com>
Date:   Wed, 20 Sep 2023 11:32:14 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Julius Zint <julius@...t.sh>
Cc:     Thomas Weißschuh <thomas@...ch.de>,
        Lee Jones <lee@...nel.org>,
        Daniel Thompson <daniel.thompson@...aro.org>,
        Jingoo Han <jingoohan1@...il.com>,
        Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Helge Deller <deller@....de>, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, linux-input@...r.kernel.org,
        linux-fbdev@...r.kernel.org
Subject: Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight
 driver

Hi,

On 9/19/23 19:46, Julius Zint wrote:
> 
> 
> On Wed, 6 Sep 2023, Hans de Goede wrote:
> 
>> Hi Julius,
>>
>> On 9/4/23 21:02, Julius Zint wrote:
>>>
>>>
>>> On Mon, 4 Sep 2023, Thomas Weißschuh wrote:
>>>
>>>> +Cc Hans who ins involved with the backlight subsystem
>>>>
>>>> Hi Julius,
>>>>
>>>> today I stumbled upon a mail from Hans [0], which explains that the
>>>> backlight subsystem is not actually a good fit (yet?) for external
>>>> displays.
>>>>
>>>> It seems a new API is in the works that would better fit, but I'm not
>>>> sure about the state of this API. Maybe Hans can clarify.
>>>>
>>>> This also ties back to my review question how userspace can figure out
>>>> to which display a backlight devices applies. So far it can not.
>>>>
>>>> [0] https://lore.kernel.org/lkml/7f2d88de-60c5-e2ff-9b22-acba35cfdfb6@redhat.com/
>>>>
>>>
>>> Hi Thomas,
>>>
>>> thanks for the hint. I will make sure to give this a proper read and
>>> see, if it fits my use case better then the current backlight subsystem.
>>
>> Note the actual proposal for the new usespace API for display brightness
>> control is here:
>>
>> https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86d2e@redhat.com/
>>
>> I have finished / stabilized the backlight code refactor which I landed
>> in 6.1, which is a prerequisite for the above proposal. But I have not
>> been able to make time to actually implement the above proposal; and
>> I don't know when I will be able to make time for this.
>>
>>> Especially since I wasnt able to properly address your other review
>>> comments for now. You are right that the name should align better with
>>> the kernel module and also, that it is possible for multiple displays to
>>> be attached.
>>>
>>> In its current state, this would mean that you could only control the
>>> backlight for the first HID device (enough for me :-).
>>>
>>> The systemd-backlight@...rvice uses not only the file name, but also the
>>> full bus path for storing/restoring backlights. I did not yet get around
>>> to see how the desktops handle brightness control, but since the
>>> systemd-backlight@...rvice already uses the name, its important to stay
>>> the same over multiple boots.
>>>
>>> I would be able to get a handle on the underlying USB device and use the
>>> serial to uniquely (and persistently) name the backlight. But it does
>>> feel hacky doing it this way.
>>
>> So mutter (gnome-shell compositor library) has a similar issue when saving
>> monitor layouts and I can tell you beforehand that monitor serial numbers
>> by themselves are not unique enough. Some models just report 123456789
>> as serial and if you have a dual-monitor setup with 2 such monitors
>> and name the backlight class device <serial>-vcp-hid or something like that
>> you will still end up with 2 identical names.
>>
>> To avoid this when saving monitor layouts mutter saves both the port
>> to which the monitor is attached (e.g. DP-1 DP-2) and the serialnumber
>> and on startup / monitor hotplug when it checks to see if it has saved
>> layout info for the monitor it checks the port+serialnr combination.
>>
>> So what I think you should do is figure out a way to map which
>> VCP HID device maps to which drm-connector and then use
>> the connector-name + serial-nr to generate the backlight device name.
>>
>> We will need the mapping the a drm-connector object anyway for
>> the new brightness API proposal from above.
>>
>> Note this does NOT solve the fact that registering a new backlight
>> class device for an external monitor on a laptop will hopelessly
>> confuse userspace, see:
>>
>> https://lore.kernel.org/lkml/7f2d88de-60c5-e2ff-9b22-acba35cfdfb6@redhat.com/
>>
>> Regards,
>>
>> Hans
>>
> 
> Thank you for all this additional information. I have watched the talks
> and read up upon the mail threads you`ve linked.

Now I wonder which presentation you have watched, did you watch
the old XDC2014 presentation ?  Note I gave a much more up2date
presentation on this at kernel-recipes last year:

https://kernel-recipes.org/en/2022/talks/new-userspace-api-for-display-panel-brightness-control/

> I will see if I can make the mapping to the DRM connector and plan to
> update this patchset.

Sounds good.

Regards,

Hans