[PATCH 02/10] drm: Add backlight helper
Noralf Trønnes
noralf at tronnes.org
Mon May 4 15:54:23 UTC 2020
Den 04.05.2020 14.06, skrev Daniel Vetter:
> On Wed, Apr 29, 2020 at 02:48:22PM +0200, Noralf Trønnes wrote:
>> This adds a function that creates a backlight device for a connector.
>> It does not deal with the KMS backlight ABI proposition[1] to add a
>> connector property. It only takes the current best practise to standardise
>> the creation of a backlight device for DRM drivers while we wait for the
>> property.
>>
>> The brightness value is set using a connector state variable and an atomic
>> commit.
>>
>> I have looked through some of the backlight users and this is what I've found:
>>
>> GNOME [2]
>> ---------
>>
>> Brightness range: 0-100
>> Scale: Assumes perceptual
>>
>> Avoids setting the sysfs brightness value to zero if max_brightness >= 99.
>> Can connect connector and backlight using the sysfs device.
>>
>> KDE [3]
>> -------
>>
>> Brightness range: 0-100
>> Scale: Assumes perceptual
>>
>> Weston [4]
>> ----------
>>
>> Brightness range: 0-255
>> Scale: Assumes perceptual
>>
>> Chromium OS [5]
>> ---------------
>>
>> Brightness range: 0-100
>> Scale: Depends on the sysfs file 'scale' which is a recent addition (2019)
>>
>> xserver [6]
>> -----------
>>
>> Brightness range: 0-x (driver specific) (1 is minimum, 0 is OFF)
>> Scale: Assumes perceptual
>>
>> The builtin modesetting driver[7] does not support Backlight, Intel[8] does.
>>
>> [1] https://lore.kernel.org/dri-devel/4b17ba08-39f3-57dd-5aad-d37d844b02c6@linux.intel.com/
>> [2] https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/blob/master/plugins/power/gsd-backlight.c
>> [3] https://github.com/KDE/powerdevil/blob/master/daemon/backends/upower/backlighthelper.cpp
>> [4] https://gitlab.freedesktop.org/wayland/weston/-/blob/master/libweston/backend-drm/drm.c
>> [5] https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/power_manager/powerd/system/internal_backlight.cc
>> [6] https://github.com/freedesktop/xorg-randrproto/blob/master/randrproto.txt
>> [7] https://gitlab.freedesktop.org/xorg/xserver/-/blob/master/hw/xfree86/drivers/modesetting/drmmode_display.c
>> [8] https://gitlab.freedesktop.org/xorg/driver/xf86-video-intel/-/blob/master/src/backlight.c
>>
>> Cc: Hans de Goede <hdegoede at redhat.com>
>> Cc: Jani Nikula <jani.nikula at linux.intel.com>
>> Cc: Martin Peres <martin.peres at linux.intel.com>
>> Cc: Daniel Thompson <daniel.thompson at linaro.org>
>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
>> ---
>> Documentation/gpu/drm-kms-helpers.rst | 6 +
>> drivers/gpu/drm/Kconfig | 7 ++
>> drivers/gpu/drm/Makefile | 1 +
>> drivers/gpu/drm/drm_backlight_helper.c | 154 +++++++++++++++++++++++++
>> include/drm/drm_backlight_helper.h | 9 ++
>> include/drm/drm_connector.h | 10 ++
>> 6 files changed, 187 insertions(+)
>> create mode 100644 drivers/gpu/drm/drm_backlight_helper.c
>> create mode 100644 include/drm/drm_backlight_helper.h
>>
>> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
>> index 9668a7fe2408..29a2f4b49fd2 100644
>> --- a/Documentation/gpu/drm-kms-helpers.rst
>> +++ b/Documentation/gpu/drm-kms-helpers.rst
>> @@ -411,3 +411,9 @@ SHMEM GEM Helper Reference
>>
>> .. kernel-doc:: drivers/gpu/drm/drm_gem_shmem_helper.c
>> :export:
>> +
>> +Backlight Helper Reference
>> +==========================
>> +
>> +.. kernel-doc:: drivers/gpu/drm/drm_backlight_helper.c
>> + :export:
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index d0aa6cff2e02..f6e13e18c9ca 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -224,6 +224,13 @@ config DRM_GEM_SHMEM_HELPER
>> help
>> Choose this if you need the GEM shmem helper functions
>>
>> +config DRM_BACKLIGHT_HELPER
>> + bool
>> + depends on DRM
>> + select BACKLIGHT_CLASS_DEVICE
>> + help
>> + Choose this if you need the backlight device helper functions
>> +
>> config DRM_VM
>> bool
>> depends on DRM && MMU
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 6493088a0fdd..0d17662dde0a 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -52,6 +52,7 @@ drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
>> drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
>> drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
>> drm_kms_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o
>> +drm_kms_helper-$(CONFIG_DRM_BACKLIGHT_HELPER) += drm_backlight_helper.o
>>
>> obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
>> obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
>> diff --git a/drivers/gpu/drm/drm_backlight_helper.c b/drivers/gpu/drm/drm_backlight_helper.c
>> new file mode 100644
>> index 000000000000..06e6a75d1d0a
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_backlight_helper.c
>> @@ -0,0 +1,154 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>> +/*
>> + * Copyright 2020 Noralf Trønnes
>> + */
>> +
>> +#include <linux/backlight.h>
>> +
>> +#include <drm/drm_atomic.h>
>> +#include <drm/drm_connector.h>
>> +#include <drm/drm_drv.h>
>> +#include <drm/drm_file.h>
>> +
>> +static int drm_backlight_update_status(struct backlight_device *bd)
>> +{
>> + struct drm_connector *connector = bl_get_data(bd);
>> + struct drm_connector_state *connector_state;
>> + struct drm_device *dev = connector->dev;
>> + struct drm_modeset_acquire_ctx ctx;
>> + struct drm_atomic_state *state;
>> + int ret;
>> +
>> + state = drm_atomic_state_alloc(dev);
>> + if (!state)
>> + return -ENOMEM;
>> +
>> + drm_modeset_acquire_init(&ctx, 0);
>> + state->acquire_ctx = &ctx;
>> +retry:
>> + connector_state = drm_atomic_get_connector_state(state, connector);
>> + if (IS_ERR(connector_state)) {
>> + ret = PTR_ERR(connector_state);
>> + goto out;
>> + }
>> +
>> + connector_state->backlight_brightness = bd->props.brightness;
>> +
>> + ret = drm_atomic_commit(state);
>> +out:
>> + if (ret == -EDEADLK) {
>> + drm_atomic_state_clear(state);
>> + drm_modeset_backoff(&ctx);
>> + goto retry;
>> + }
>> +
>> + drm_atomic_state_put(state);
>> +
>> + drm_modeset_drop_locks(&ctx);
>> + drm_modeset_acquire_fini(&ctx);
>> +
>> + return ret;
>> +}
>
> Looking at this, I'm not sure this is generally useable outside of your
> usb driver. Or stuff that does essentially the same.
>
> For real hw drivers I expect a similar relationship between the kms driver
> and the backlight driver like for i2c, clocks, power domaines, ... i.e.
> the kms driver calls into the backlight driver. This also means that kms
> locks will be nesting outside of the locks of these subordinate
> subsystems, and hence direct access through other userspace interfaces
> (like we have for i2c too) needs to bypass atomic kms. Otherwise we have
> deadlock potential.
>
>
> With your stuff here we can potentially get a backlight locks -> kms locks
> -> backlight locks scenario, and lockdep will be angry about this. So in
> general this doesn't work (I think, locking is hard), and in this specific
> case it only works because your usb driver shovels everything over to
> another machine. Lockdep will still complain if you load this together
> with some other kms driver that uses backlight normally.
>
> Aside: Locking is broken in the backlight code, we take the
> bd->update_lock too late, after bd->props has already been handled
> unsafely. But I guess that's a side effect of the sysfs interface being
> racy by default. Or the backlight_enable/disable helpers should take
> bd->ops_lock too.
>
> So two things:
> - I think the actual update needs to be pushed to a work_struct, with no
> flush_work, to avoid these locking loops completely.
> - I think this is best left in your usb driver for now, until someone
> comes up with maybe an spi forwarder or whatever where we might need
> this again.
>
> Or, and this is probably much simpler, just have a simpler backlight
> driver that's completely orthogonal to the kms side, with some hw lock to
> organize updates. For that case just adding a drm_connector->backlight
> pointer, as prep for Hans' work, would be great.
I don't quite follow you here. This backlight device is for kms drivers
that control the backlight brightness by themselves, by setting a pwm
value or something on their hardware. The kms driver will not act as a
proxy and forward the value to another backlight driver. Nor will
another driver use this backlight device. It's purely for userspace. So
I don't understand how a locking loop can happen.
What this code does for me is pushing brightness changes through the
atomic machinery so I can treat it like any other property change.
So the reason I made it a core helper is that I figured I wasn't the
only one who would want this.
I'll move it inside my driver unless someone chimes in and say they want
it to.
Noralf.
> -Daniel
>
>
>> +
>> +static int drm_backlight_get_brightness(struct backlight_device *bd)
>> +{
>> + struct drm_connector *connector = bl_get_data(bd);
>> + struct drm_device *dev = connector->dev;
>> + int brightness;
>> +
>> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> + brightness = connector->state->backlight_brightness;
>> + drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> +
>> + return brightness;
>> +}
>> +
>> +static const struct backlight_ops drm_backlight_ops = {
>> + .get_brightness = drm_backlight_get_brightness,
>> + .update_status = drm_backlight_update_status,
>> +};
>> +
>> +/* Can be exported for drivers carrying a legacy name */
>> +static int drm_backlight_register_with_name(struct drm_connector *connector, const char *name)
>> +{
>> + struct backlight_device *bd;
>> + const struct backlight_properties props = {
>> + .type = BACKLIGHT_RAW,
>> + .scale = BACKLIGHT_SCALE_NON_LINEAR,
>> + .max_brightness = 100,
>> + };
>> +
>> + if (WARN_ON(!drm_core_check_feature(connector->dev, DRIVER_MODESET) ||
>> + !drm_drv_uses_atomic_modeset(connector->dev) ||
>> + !connector->kdev))
>> + return -EINVAL;
>> +
>> + bd = backlight_device_register(name, connector->kdev, connector,
>> + &drm_backlight_ops, &props);
>> + if (IS_ERR(bd))
>> + return PTR_ERR(bd);
>> +
>> + connector->backlight = bd;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * drm_backlight_register() - Register a backlight device for a connector
>> + * @connector: Connector
>> + *
>> + * This function registers a backlight device for @connector with the following
>> + * characteristics:
>> + *
>> + * - The connector sysfs device is used as a parent device for the backlight device.
>> + * Userspace can use this to connect backlight device and connector.
>> + * - Name will be on the form: **card0-HDMI-A-1-backlight**
>> + * - Type is "raw"
>> + * - Scale is "non-linear" (perceptual)
>> + * - Max brightness is 100 giving a range of 0-100 inclusive
>> + * - Reading sysfs **brightness** returns the backlight device property
>> + * - Reading sysfs **actual_brightness** returns the connector state value
>> + * - Writing sysfs **bl_power** is ignored, the DPMS connector property should
>> + * be used to control power.
>> + * - Backlight device suspend/resume events are ignored.
>> + *
>> + * Note:
>> + *
>> + * Brightness zero should not turn off backlight it should be the minimum
>> + * brightness, DPMS handles power.
>> + *
>> + * This function must be called from &drm_connector_funcs->late_register() since
>> + * it depends on the sysfs device.
>> + *
>> + * Returns:
>> + * Zero on success or negative error code on failure.
>> + */
>> +int drm_backlight_register(struct drm_connector *connector)
>> +{
>> + const char *name = NULL;
>> + int ret;
>> +
>> + name = kasprintf(GFP_KERNEL, "card%d-%s-backlight",
>> + connector->dev->primary->index, connector->name);
>> + if (!name)
>> + return -ENOMEM;
>> +
>> + ret = drm_backlight_register_with_name(connector, name);
>> + kfree(name);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(drm_backlight_register);
>> +
>> +/**
>> + * drm_backlight_unregister() - Unregister backlight device
>> + * @connector: Connector
>> + *
>> + * Unregister a backlight device. This must be called from the
>> + * &drm_connector_funcs->early_unregister() callback.
>> + */
>> +void drm_backlight_unregister(struct drm_connector *connector)
>> +{
>> + backlight_device_unregister(connector->backlight);
>> +}
>> +EXPORT_SYMBOL(drm_backlight_unregister);
>> diff --git a/include/drm/drm_backlight_helper.h b/include/drm/drm_backlight_helper.h
>> new file mode 100644
>> index 000000000000..4151b66eb0b4
>> --- /dev/null
>> +++ b/include/drm/drm_backlight_helper.h
>> @@ -0,0 +1,9 @@
>> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
>> +
>> +#ifndef _LINUX_DRM_BACKLIGHT_HELPER_H
>> +#define _LINUX_DRM_BACKLIGHT_HELPER_H
>> +
>> +int drm_backlight_register(struct drm_connector *connector);
>> +void drm_backlight_unregister(struct drm_connector *connector);
>> +
>> +#endif
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 221910948b37..ce678b694f45 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -32,6 +32,7 @@
>>
>> #include <uapi/drm/drm_mode.h>
>>
>> +struct backlight_device;
>> struct drm_connector_helper_funcs;
>> struct drm_modeset_acquire_ctx;
>> struct drm_device;
>> @@ -656,6 +657,12 @@ struct drm_connector_state {
>> */
>> u8 max_bpc;
>>
>> + /**
>> + * @backlight_brightness: Brightness value of the connector backlight
>> + * device. See drm_backlight_register().
>> + */
>> + u8 backlight_brightness;
>> +
>> /**
>> * @hdr_output_metadata:
>> * DRM blob property for HDR output metadata
>> @@ -1422,6 +1429,9 @@ struct drm_connector {
>>
>> /** @hdr_sink_metadata: HDR Metadata Information read from sink */
>> struct hdr_sink_metadata hdr_sink_metadata;
>> +
>> + /** @backlight: Backlight device. See drm_backlight_register() */
>> + struct backlight_device *backlight;
>> };
>>
>> #define obj_to_connector(x) container_of(x, struct drm_connector, base)
>> --
>> 2.23.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
More information about the dri-devel
mailing list