Re: [PATCH 02/10] drm: Add backlight helper

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxxxxxxx/
>> [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@xxxxxxxxxx>
>> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
>> Cc: Martin Peres <martin.peres@xxxxxxxxxxxxxxx>
>> Cc: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
>> Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
>> ---
>>  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@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux