lkml.org 
[lkml]   [2023]   [Sep]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver
    From
    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@.service 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@.service 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


    \
     
     \ /
      Last update: 2023-09-06 17:21    [W:5.857 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site