From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 6F8A5C433F5 for ; Fri, 8 Apr 2022 10:09:49 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id A543510F13F; Fri, 8 Apr 2022 10:09:45 +0000 (UTC) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by gabe.freedesktop.org (Postfix) with ESMTPS id 14BBA10F136 for ; Fri, 8 Apr 2022 10:09:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1649412583; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=EiVIxvljQFV5G39uBsrGYp4sBEpxW/A4qxbBuQByXAY=; b=SWhMKzS8pZPxnFgL+ksZdYMI2rUQPOVAEIcjfjMnkE/Y4qU8yfoM9jdZQpzDllAhMgQ/MG 1Eb8VOdbh8PsjUJ3JQEya6hv+SQVJR5Hq/rAqyY06Mvg+umA4I7xTBp5GB1Xdi59Qh9cph agKWm9yfeW0TG/X5nblAcnHVc1EHxSY= Received: from mail-ej1-f71.google.com (mail-ej1-f71.google.com [209.85.218.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-28-BccGCD2zM1eCxk7uKtuAcQ-1; Fri, 08 Apr 2022 06:09:42 -0400 X-MC-Unique: BccGCD2zM1eCxk7uKtuAcQ-1 Received: by mail-ej1-f71.google.com with SMTP id nd34-20020a17090762a200b006e0ef16745cso4626682ejc.20 for ; Fri, 08 Apr 2022 03:09:41 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:from:to:cc:references:in-reply-to :content-transfer-encoding; bh=EiVIxvljQFV5G39uBsrGYp4sBEpxW/A4qxbBuQByXAY=; b=xuUtRdzh92GII6DRt3l/ytXR5EjnR1/vEmbKFppjUCgtBRK3exh/5pQWCUaYsup4DP cFkxmp173KuX9etBxHHHdHkriJKEWWT2AmgtzS0ZCxd22XB/wtuqTaQjHG/j+m8Z33dw +gpXd3rqLKoHtphSwyUdBuFZAjSE3tqqwqJghijVbrYP2B941TfTWJw2b9CVXo68cHth w/UxGZjg0YyCjAzZQJ9ndDWAus22yP22jJBrpQAuvbJ8dSuGCzIZtHDpDoUo4Pyuy+Vu UC/XZohIj/g+wj5yu6JN3uBRcv5pHL/VdiO41aY7DgX95Vy6CYeQEK7OH8R2UCFT8/MT xHug== X-Gm-Message-State: AOAM533gbLPn7c868SjntHFS+3GTYmosxd+spQAX06Ym1wDMsjkEDuJQ zS+LoiV8PEg9yjvfqjFzMzgtFrSpg8DtkNjjHIZblaAaHgQJGcT+Elc94ubQHlvSrw7gPHJxr3h O6faCU0xDaZzvwDO6ki7HYy/+iW5e X-Received: by 2002:a17:907:eaa:b0:6e8:2f3:45f9 with SMTP id ho42-20020a1709070eaa00b006e802f345f9mr18126222ejc.323.1649412580675; Fri, 08 Apr 2022 03:09:40 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyYqz9Govwh6K0rCRA8ZO9jX5Vwo5yI19Wlo8YLhE1hopx0UBwVmROY0VxnCRonKEy2aUS2yg== X-Received: by 2002:a17:907:eaa:b0:6e8:2f3:45f9 with SMTP id ho42-20020a1709070eaa00b006e802f345f9mr18126194ejc.323.1649412580276; Fri, 08 Apr 2022 03:09:40 -0700 (PDT) Received: from ?IPV6:2001:1c00:c1e:bf00:1db8:22d3:1bc9:8ca1? (2001-1c00-0c1e-bf00-1db8-22d3-1bc9-8ca1.cable.dynamic.v6.ziggo.nl. [2001:1c00:c1e:bf00:1db8:22d3:1bc9:8ca1]) by smtp.gmail.com with ESMTPSA id dm11-20020a170907948b00b006cf488e72e3sm8572563ejc.25.2022.04.08.03.09.38 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 08 Apr 2022 03:09:39 -0700 (PDT) Message-ID: Date: Fri, 8 Apr 2022 12:09:38 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [RFC] drm/kms: control display brightness through drm_connector properties From: Hans de Goede To: Daniel Vetter , Alex Deucher References: <0d188965-d809-81b5-74ce-7d30c49fee2d@redhat.com> <0e1cffc1-e8b6-dc58-56ff-53f911f33e60@redhat.com> In-Reply-To: Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=hdegoede@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Sebastian Wick , Martin Roukala , Christoph Grenz , wayland , "dri-devel@lists.freedesktop.org" , Yusuf Khan Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Hi, On 4/8/22 11:58, Hans de Goede wrote: > Hi Daniel, > > On 4/8/22 10:07, Daniel Vetter wrote: >> On Thu, Apr 07, 2022 at 05:05:52PM -0400, Alex Deucher wrote: >>> On Thu, Apr 7, 2022 at 1:43 PM Hans de Goede wrote: >>>> >>>> Hi Simon, >>>> >>>> On 4/7/22 18:51, Simon Ser wrote: >>>>> Very nice plan! Big +1 for the overall approach. >>>> >>>> Thanks. >>>> >>>>> On Thursday, April 7th, 2022 at 17:38, Hans de Goede wrote: >>>>> >>>>>> The drm_connector brightness properties >>>>>> ======================================= >>>>>> >>>>>> bl_brightness: rw 0-int32_max property controlling the brightness setting >>>>>> of the connected display. The actual maximum of this will be less then >>>>>> int32_max and is given in bl_brightness_max. >>>>> >>>>> Do we need to split this up into two props for sw/hw state? The privacy screen >>>>> stuff needed this, but you're pretty familiar with that. :) >>>> >>>> Luckily that won't be necessary, since the privacy-screen is a security >>>> feature the firmware/embedded-controller may refuse our requests >>>> (may temporarily lock-out changes) and/or may make changes without >>>> us requesting them itself. Neither is really the case with the >>>> brightness setting of displays. >>>> >>>>>> bl_brightness_max: ro 0-int32_max property giving the actual maximum >>>>>> of the display's brightness setting. This will report 0 when brightness >>>>>> control is not available (yet). >>>>> >>>>> I don't think we actually need that one. Integer KMS props all have a >>>>> range which can be fetched via drmModeGetProperty. The max can be >>>>> exposed via this range. Example with the existing alpha prop: >>>>> >>>>> "alpha": range [0, UINT16_MAX] = 65535 >>>> >>>> Right, I already knew that, which is why I explicitly added a range >>>> to the props already. The problem is that the range must be set >>>> before registering the connector and when the backlight driver >>>> only shows up (much) later during boot then we don't know the >>>> range when registering the connector. I guess we could "patch-up" >>>> the range later. But AFAIK that would be a bit of abuse of the >>>> property API as the range is intended to never change, not >>>> even after hotplug uevents. At least atm there is no infra >>>> in the kernel to change the range later. >>>> >>>> Which is why I added an explicit bl_brightness_max property >>>> of which the value gives the actual effective maximum of the >>>> brightness. >> >> Uh ... I'm not a huge fan tbh. The thing is, if we allow hotplugging >> brightness control later on then we just perpetuate the nonsense we have >> right now, forever. >> >> Imo we should support two kinds of drivers: >> >> - drivers which are non-crap, and make sure their backlight driver is >> loaded before they register the drm_device (or at least the >> drm_connector). For those we want the drm_connector->backlight pointer >> to bit static over the lifetime of the connector, and then we can also >> set up the brightness range correctly. > > The only problem is that outside of device-tree platforms where > we can have a backlight link in a devicetree display-connector node, > there are no non crap devices and thus no non crap drivers. > >> - funny drivers which implement the glorious fallback dance which >> libbacklight implements currently in userspace. Imo for these drivers we >> should have a libbacklight_heuristics_backlight, which normalizes or >> whatever, and is also ways there. And then internally handles the >> fallback mess to the "right" backlight driver. > > So this will be pretty much all of them including i915 and nouveau. > > My first thoughts where the same as yours and we can mostly guarantee > that the drm_connector->backlight pointer is static over lifetime of > the connector. But the problem is with the backlight device-s provided > by things like the dell-laptop, thinkpad_acpi, etc. drivers which are > still necessary / used for backlight control on core2duo era laptops > which are still being active used by people. > > Basically atm the kernel code to determine which backlight-device > to use (which assumes a single internal LCD panel) goes like this (1): > > 1. Check cmdline-override, DMI quirks (and return their value if set) > 2. If ACPI video extensions are not supported then expect a backlight > device of the dell-laptop, thinkpad_acpi, etc. type, and use that. > 3. If the ACPI tables have been written for Windows8 or later and > the GPU driver offers a GPU native backlight device use that. > 4. Use the ACPI video extensions backlight device > >> We might have some gaps on acpi systems to make sure the drm driver can >> wait for the backlight driver to show up, but that's about it. > > The problem here is 2. or IOW devices which don't support the > ACPI video extensions, these typically (always?) also don't offer > a GPU native backlight device, instead relying on > the embedded-controller for backlight control using some vendor > specific firmware API to talk to the EC. > > For the other cases there are indeed some gaps which I plan to close > so that we can make sure that the backlight device will be in place > when we register the connector. > > But the old devices without ACPI video extensions case is a big > problem and more then just some gaps" and that is a path which all > major x86 drivers may hit. > > In some cases I even expect the backlight_device to simply never > show up when hitting 2. Either because the necessary driver is > not enabled in the kernel or because no-one ever added support for > the specific fw interface used on the laptop in question. But I > do expect this to be quite rare. > > For the privacy-screen case where we had a similar issue this > was solved by in essence duplicating the detection part of the > privacy-screen drivers inside the drm_privacy code and use > -EPROBE_DEFER to wait for the privacy-screen driver to load. > > But in this case that is not really feasible IMHO because: > > [hans@shalem linux]$ ack -l backlight_device_register drivers/platform/x86 > drivers/platform/x86/toshiba_acpi.c > drivers/platform/x86/intel/oaktrail.c > drivers/platform/x86/dell/dell-laptop.c > drivers/platform/x86/msi-laptop.c > drivers/platform/x86/panasonic-laptop.c > drivers/platform/x86/ideapad-laptop.c > drivers/platform/x86/sony-laptop.c > drivers/platform/x86/thinkpad_acpi.c > drivers/platform/x86/acer-wmi.c > drivers/platform/x86/samsung-q10.c > drivers/platform/x86/asus-wmi.c > drivers/platform/x86/apple-gmux.c > drivers/platform/x86/nvidia-wmi-ec-backlight.c > drivers/platform/x86/msi-wmi.c > drivers/platform/x86/asus-laptop.c > drivers/platform/x86/classmate-laptop.c > drivers/platform/x86/eeepc-laptop.c > drivers/platform/x86/fujitsu-laptop.c > drivers/platform/x86/samsung-laptop.c > drivers/platform/x86/compal-laptop.c > [hans@shalem linux]$ ack -l backlight_device_register drivers/platform/x86 | wc -l > 20 > > Duplicating 20 wildly different ACPI/WMI backlight detection > routines is a bit much; and also something which I cannot test > easily and doing EPROBE_DEFER like behavior will require all > of these to also be available in the initrd. > > So IMHO at least for devices relying on these it is best to allow > having the bl_brightness* properties be presend on the internal > LCD connector at registration time with a hint that they are > not functional and then send an uevent when they become functional. > > I really see no other way to deal with these (old) devices. Oh and one important thing which I forgot to add, it is these old vendor specific firmware APIs for setting the backlight which have the issue of having only say 8 levels, so scaling those to 0-65535 leads to the: "E.g GNOME decides on a step size for the hotkeys by doing min(brightness_max/20, 1). Some of the vendor specific backlight fw APIs (e.g. dell-laptop) have only 8 steps. When giving userspace the actual max_brightness value, then this will all work just fine. When hardcode brightness_max to 65535 OTOH then in this case GNOME will still give the user 20 steps where only 1 in every 2-3 steps actually changes the brightness which IMHO is an unacceptably bad user experience." problem from my original email starting the thread. One thing which I did consider is to always scale to 0-65535 and then add a "bl_brightness_step_size" property which would then be set to 65535/8 = 8192 in this case. But there are 2 disadvantages to this: 1. We still need a uevent for when the step-size changes once the backlight-device finally shows up on impacted old devices 2. Scaling between the backlight device and the property value sooner or later may lead to drift due to rounding issues. So I don't really see this as better, TBH the whole scaling + reporting step-size thing feels significantly worse then just updating brightness_max. And then we would need to report step-size = 0 to report no backlight device is available yet, which also feels worse then using brightness_max=0 to indicate lack of brightness control. Regards, Hans > 1) For now I, intend to extend this with detection of Apple GMUX and > NVIDIA_WMI_EC_BACKLIGHT support >