Hi,
On 5/19/22 11:02, Jani Nikula wrote:
On Wed, 18 May 2022, Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 5/18/22 10:55, Jani Nikula wrote:
On Tue, 17 May 2022, Hans de Goede hdegoede@redhat.com wrote:
ATM on x86 laptops where we want userspace to use the acpi_video backlight device we often register both the GPU's native backlight device and acpi_video's firmware acpi_video# backlight device. This relies on userspace preferring firmware type backlight devices over native ones, but registering 2 backlight devices for a single display really is undesirable.
On x86 laptops where the native GPU backlight device should be used, the registering of other backlight devices is avoided by their drivers using acpi_video_get_backlight_type() and only registering their backlight if the return value matches their type.
acpi_video_get_backlight_type() uses backlight_device_get_by_type(BACKLIGHT_RAW) to determine if a native driver is available and will never return native if this returns false. This means that the GPU's native backlight registering code cannot just call acpi_video_get_backlight_type() to determine if it should register its backlight, since acpi_video_get_backlight_type() will never return native until the native backlight has already registered.
To fix this add a native function parameter to acpi_video_get_backlight_type(), which when set to true will make acpi_video_get_backlight_type() behave as if a native backlight has already been registered.
Regarding the question below, this is the part that throws me off.
Note that all current callers are updated to pass false for the new parameter, so this change in itself causes no functional changes.
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c index becc198e4c22..0a06f0edd298 100644 --- a/drivers/acpi/video_detect.c +++ b/drivers/acpi/video_detect.c @@ -17,12 +17,14 @@
- Otherwise vendor specific drivers like thinkpad_acpi, asus-laptop,
- sony_acpi,... can take care about backlight brightness.
- Backlight drivers can use acpi_video_get_backlight_type() to determine
- which driver should handle the backlight.
- Backlight drivers can use acpi_video_get_backlight_type() to determine which
- driver should handle the backlight. RAW/GPU-driver backlight drivers must
- pass true for the native function argument, other drivers must pass false.
- If CONFIG_ACPI_VIDEO is neither set as "compiled in" (y) nor as a module (m)
- this file will not be compiled and acpi_video_get_backlight_type() will
- always return acpi_backlight_vendor.
- return acpi_backlight_native when its native argument is true and
*/
- acpi_backlight_vendor when it is false.
Frankly, I think the boolean native parameter here, and at the call sites, is confusing, and the slightly different explanations in the commit message and comment here aren't helping.
Can you elaborate the "slightly different explanations in the commit message and comment" part a bit (so that I can fix this) ?
I suggest adding a separate function that the native backlight drivers should use. I think it's more obvious all around, and easier to document too.
Code wise I think this would mean renaming the original and then adding 2 wrappers, but that is fine with me. I've no real preference either way and I'm happy with adding a new variant of acpi_video_get_backlight_type() for the native backlight drivers any suggestion for a name ?
Alternatively, do the native backlight drivers have any need for the actual backlight type information from acpi? They only need to be able to ask if they should register themselves, right?
I understand this sounds like bikeshedding, but I'm trying to avoid duplicating the conditions in the drivers where a single predicate function call could be sufficient, and the complexity could be hidden in acpi.
if (!acpi_video_backlight_use_native()) return;
acpi_video_backlight_use_native() sounds good, I like I will change this for v2. This also removes churn in all the other acpi_video_get_backlight_type() callers.
Perhaps all the drivers/platform/x86/* backlight drivers could use:
if (acpi_video_backlight_use_vendor()) ...
Hmm, as part of the ractoring there also will be new apple_gmux and nvidia_wmi_ec types. I'm not sure about adding seperate functions for all of those vs get_type() != foo. I like get_type != foo because it makes clear that there will also be another caller somewhere where get_type == foo and that that one will rbe the one which actually gets to register its backlight.
You can still use the native parameter etc. internally, but just hide the details from everyone else, and, hopefully, make it harder for them to do silly things?
Ack.
Regards,
Hans