Mario Limonciello <[email protected]> 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(). -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
