Re: [PATCH v8 04/14] drm: add connector backlight (LUMINANCE) infrastructure

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 22.09.26 um 13:42 schrieb Javier Martinez Canillas:
> Mario Limonciello <mario.limonciello@amd.com> writes:
>
>> Backlight brightness is a property of a display, and thus of a DRM
>> connector, yet it has historically only been controllable through the
>> separate backlight sysfs interface. Add a generic, backend-agnostic
>> per-connector LUMINANCE range property so brightness can be driven
>> through the atomic modeset path like any other connector state.
>>
>> A struct drm_backlight is embedded in every connector and initialized by
>> the core; drivers do not allocate it. A driver links a backend (today a
>> backlight_device, in the future DDC/CI, MIPI-DCS, ...) with
>> drm_backlight_link(), which creates the connector's LUMINANCE property
> The infrastructure is meant to be backend-agnostic but drm_backlight_link()
> which creates the connector's LUMINANCE property is backend specific. Maybe
> would be better to not call drm_backlight to the DRM core helpers since it
> conflates the backlight subsystem specific helpers, and the ones that are
> supposed to be backend-agnostic.
>
> Maybe drm_brightness or drm_luminance would be more suitable? That way would
> be clear what is generic infra to handle the LUMINANCE property and what is
> specific to drivers using a backlight device.
>
> In fact, I think that would be better if you split this patch in the DRM core
> helpers and another patch that adds the backlight specific helpers on top. It
> would also be useful to have an example of a "native" backend, for drivers that
> have a trivial brightness management.
>
> For example, many small I2C or SPI display panels just send a few commands over
> the bus to change the backlight brightness. Those may have their own get/set
> luminance callbacks and register their own struct drm_backlight_funcs.
>
> Because right now seems to me that this drm_backlight infrastructure is mixing
> the two layers and we have a leaky abstraction.
>
>> with the backend's range. The property value is staged into the atomic
>> connector state and only pushed to the hardware from the commit/enable
>> path, via a workqueue so that slow backends never stall a commit. DPMS
>> off drives the backlight to 0 and DPMS on restores the committed value.
>>
>> The property range is per-connector (created from the backend's
>> max_brightness), so multiple panels no longer share and corrupt a single
>> device-wide range. drm_backlight_link() also carries the legacy-sysfs
>> takeover accounting used by the client capability added in a later patch.
>>
>> The whole feature is guarded by CONFIG_DRM_BACKLIGHT (which depends on,
>> rather than selects, BACKLIGHT_CLASS_DEVICE) so DRM does not pull the
>> backlight subsystem into the kernel when it is not wanted.
>>
> If we had a good separation between the drm_backlight core helpers and the
> backlight subsystem then this dependency wouldn't be needed. Drivers would
> use depend on CONFIG_DRM_BACKLIGHT to use the drm_backlight helpers and use
> BACKLIGHT_CLASS_DEVICE dependent helpers, to be used as set/get callbacks.
>
> Since DRM drivers that register a backlight device already depend on this
> Kconfig symbol, no additional dependencies will be needed. And drivers that
> only use the CONFIG_DRM_BACKLIGHT (with a different backend), won't need to
> depend on CONFIG_DRM_BACKLIGHT.
>
> In general, I think is preferable to have good helper functions that could
> be reused by drivers, instead of having mid-layers such as drm_backlight_link().

I want to second what Javier says here. I raise the same points in my 
rant about DRM design.

Best regards
Thomas


>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)