Re: [PATCH v10 12/15] drm/radeon: add connector luminance support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] drm/radeon: unreachable atomic LUMINANCE property on non-atomic KMS driver
- [High] drm/radeon: legacy LVDS backlight ignored and panel turned off in radeon_legacy_lvds_update()
- [High] drm/radeon: missing luminance callbacks in radeon_dp_connector_helper_funcs
- [Medium] drm/radeon: Kconfig unmet dependency and link error with DRM_LUMINANCE
- [Medium] drm/radeon: ACPI ATIF brightness events ignored when dig->bl_dev is NULL
- [Medium] drm/radeon: memory leak of backlight_priv when LCD encoder links to multiple connectors
--

commit d872f715df68798d7e323e8f154d8558d421e434
Author: Mario Limonciello <mario.limonciello@amd.com>

drm/radeon: add connector luminance support

This commit adds support for exposing Radeon panel brightness through the
connector LUMINANCE property when CONFIG_DRM_LUMINANCE is enabled, reusing
the AtomBIOS and legacy hardware programming paths while keeping the
backlight class interface otherwise.

> diff --git a/drivers/gpu/drm/radeon/Kconfig b/drivers/gpu/drm/radeon/Kconfig
> index 3cd70aef770e1..2ccad1f2f8cee 100644
> --- a/drivers/gpu/drm/radeon/Kconfig
> +++ b/drivers/gpu/drm/radeon/Kconfig
> @@ -17,7 +17,7 @@ config DRM_RADEON
>  	select SND_HDA_COMPONENT if SND_HDA_CORE
>  	select POWER_SUPPLY
>  	select HWMON
> -	select BACKLIGHT_CLASS_DEVICE
> +	select BACKLIGHT_CLASS_DEVICE if !DRM_LUMINANCE
>  	select INTERVAL_TREE
>  	select I2C
>  	select I2C_ALGOBIT

[Severity: Medium]
Can making BACKLIGHT_CLASS_DEVICE conditional on !DRM_LUMINANCE cause an
unmet Kconfig dependency and linker failures when CONFIG_DRM_LUMINANCE=y,
CONFIG_ACPI=y, and CONFIG_BACKLIGHT_CLASS_DEVICE is disabled (or modular
with built-in DRM_RADEON)?

Just below in drivers/gpu/drm/radeon/Kconfig, DRM_RADEON still
unconditionally selects ACPI_VIDEO:

	# radeon depends on ACPI_VIDEO when ACPI is enabled, for select to work
	# ACPI_VIDEO's dependencies must also be selected.
	select INPUT if ACPI
	select ACPI_VIDEO if ACPI

In drivers/acpi/Kconfig, ACPI_VIDEO depends on BACKLIGHT_CLASS_DEVICE:

config ACPI_VIDEO
	tristate "Video"
	depends on BACKLIGHT_CLASS_DEVICE

In addition, radeon_atif_handler() in drivers/gpu/drm/radeon/radeon_acpi.c
unconditionally calls backlight_force_update(), which has no inline stub in
include/linux/backlight.h when CONFIG_BACKLIGHT_CLASS_DEVICE is disabled:

	if (rdev->is_atom_bios) {
		struct radeon_encoder_atom_dig *dig = enc->enc_priv;
		backlight_force_update(dig->bl_dev,
				       BACKLIGHT_UPDATE_HOTKEY);
	} else {
		struct radeon_encoder_lvds *dig = enc->enc_priv;
		backlight_force_update(dig->bl_dev,
				       BACKLIGHT_UPDATE_HOTKEY);
	}

> diff --git a/drivers/gpu/drm/radeon/atombios_encoders.c b/drivers/gpu/drm/radeon/atombios_encoders.c
> index 5b9c72bf40994..e6f762625bf1a 100644
> --- a/drivers/gpu/drm/radeon/atombios_encoders.c
> +++ b/drivers/gpu/drm/radeon/atombios_encoders.c
> @@ -227,6 +227,27 @@ void radeon_atom_backlight_init(struct radeon_encoder *radeon_encoder,
>  	memset(&props, 0, sizeof(props));
>  	props.max_brightness = RADEON_MAX_BL_LEVEL;
>  	props.type = BACKLIGHT_RAW;
> +	pdata->encoder = radeon_encoder;
> +	pdata->atom = true;
> +	radeon_encoder->backlight_priv = pdata;

[Severity: Medium]
Can this leak the struct radeon_backlight_privdata allocation in pdata
when an encoder with ATOM_DEVICE_LCD_SUPPORT is shared across multiple
connectors?

In radeon_add_atom_encoder() and radeon_add_legacy_encoder(), encoders
sharing the same encoder_enum combine their device bitmasks via
radeon_encoder->devices |= supported_device. When
radeon_link_encoder_connector() in drivers/gpu/drm/radeon/radeon_encoders.c
links encoders and connectors, it checks radeon_encoder->devices rather
than radeon_connector->devices:

	if (radeon_encoder->devices & radeon_connector->devices) {
		drm_connector_attach_encoder(connector, encoder);
		if (radeon_encoder->devices & (ATOM_DEVICE_LCD_SUPPORT))
			radeon_encoder_add_backlight(radeon_encoder, connector);
	}

When radeon_encoder_add_backlight() is invoked a second time for the same
radeon_encoder (both here in radeon_atom_backlight_init() and in
radeon_legacy_backlight_init()), radeon_encoder->backlight_priv is
overwritten with the newly allocated pdata. With CONFIG_DRM_LUMINANCE
enabled, drm_connector_attach_luminance_property() succeeds on the second
connector and leaks the first pdata allocation (or if registration fails
and jumps to error:, radeon_encoder->backlight_priv is cleared to NULL
without freeing or preserving the first allocation).

> +
> +	dig = radeon_encoder->enc_priv;
> +	if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
> +		props.brightness = atombios_get_backlight_level(radeon_encoder);
> +		if (!props.brightness)
> +			props.brightness = RADEON_MAX_BL_LEVEL;
> +
> +		if (drm_connector_attach_luminance_property(drm_connector, 0,
> +							    props.max_brightness,
> +							     props.brightness))
> +			goto error;
> +
> +		atombios_set_backlight_level(radeon_encoder, props.brightness);
> +		dig->bl_dev = NULL;
> +		rdev->mode_info.bl_encoder = radeon_encoder;
> +		return;
> +	}

[Severity: High]
How can userspace access or update the LUMINANCE property on radeon when
radeon is a non-atomic KMS driver?

In drivers/gpu/drm/radeon/radeon_drv.c, kms_driver does not advertise
DRIVER_ATOMIC:

	.driver_features =
	    DRIVER_GEM | DRIVER_RENDER | DRIVER_MODESET,

However, drm_connector_attach_luminance_property() creates the LUMINANCE
property with DRM_MODE_PROP_ATOMIC:

	property = drm_property_create_range(connector->dev, DRM_MODE_PROP_ATOMIC,
					     "LUMINANCE", min, max);

Because DRIVER_ATOMIC is not set:
1. drm_setclientcap() rejects DRM_CLIENT_CAP_ATOMIC, so
   drm_mode_object_get_properties() hides DRM_MODE_PROP_ATOMIC properties
   from userspace.
2. Legacy property updates go through drm_mode_obj_set_property_ioctl() ->
   set_property_legacy() -> radeon_lvds_set_property(), which ignores
   everything except scaling_mode_property.
3. radeon_set_luminance() and radeon_disable_luminance() are only invoked
   by drm_atomic_helper_commit_luminance_enables() and
   drm_atomic_helper_commit_luminance_disables(), which radeon never calls.

Since radeon_atom_backlight_init() and radeon_legacy_backlight_init()
return early before backlight_device_register() when CONFIG_DRM_LUMINANCE
is enabled (and set rdev->mode_info.bl_encoder, which also prevents the
acpi_video_register_backlight() fallback in radeon_encoder_add_backlight()),
won't this leave panels with no working brightness control interface?

[Severity: Medium]
Also, when dig->bl_dev is set to NULL here (and in
radeon_legacy_backlight_init()), won't ACPI ATIF panel brightness events
stop working?

In drivers/gpu/drm/radeon/radeon_acpi.c, radeon_acpi_init() locates
atif->encoder_for_bl by checking dig->bl_dev:

	if (rdev->is_atom_bios) {
		struct radeon_encoder_atom_dig *dig = enc->enc_priv;
		if (dig->bl_dev) {
			target = enc;
			break;
		}
	} else {
		struct radeon_encoder_lvds *dig = enc->enc_priv;
		if (dig->bl_dev) {
			target = enc;
			break;
		}
	}

When dig->bl_dev is NULL, atif->encoder_for_bl remains NULL and
radeon_atif_handler() ignores ATIF_PANEL_BRIGHTNESS_CHANGE_REQUEST events.

[ ... ]

> diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
> index 16b048f58c9be..655361407b819 100644
> --- a/drivers/gpu/drm/radeon/radeon_connectors.c
> +++ b/drivers/gpu/drm/radeon/radeon_connectors.c

[ ... ]

> @@ -931,11 +932,44 @@ static int radeon_lvds_set_property(struct drm_connector *connector,
>  	return 0;
>  }
>  
> +static void radeon_set_luminance(const struct drm_connector_state *conn_state)
> +{
> +	struct drm_encoder *encoder = radeon_best_single_encoder(conn_state->connector);
> +	struct radeon_encoder *radeon_encoder;
> +	struct radeon_backlight_privdata *pdata;
> +	u8 level = conn_state->luminance;
> +
> +	if (!encoder)
> +		return;
> +
> +	radeon_encoder = to_radeon_encoder(encoder);
> +	pdata = radeon_encoder->backlight_priv;
> +	if (!pdata)
> +		return;
> +
> +	if (pdata->atom) {
> +		atombios_set_backlight_level(radeon_encoder, level);
> +	} else {
> +		if (pdata->negative)
> +			level = RADEON_MAX_BL_LEVEL - level;
> +		radeon_legacy_set_backlight_level(radeon_encoder, level);
> +	}
> +}
> +
> +static void radeon_disable_luminance(const struct drm_connector_state *conn_state)
> +{
> +	struct drm_connector_state state = *conn_state;
> +
> +	state.luminance = 0;
> +	radeon_set_luminance(&state);
> +}
>  
>  static const struct drm_connector_helper_funcs radeon_lvds_connector_helper_funcs = {
>  	.get_modes = radeon_lvds_get_modes,
>  	.mode_valid = radeon_lvds_mode_valid,
>  	.best_encoder = radeon_best_single_encoder,
> +	.set_luminance = radeon_set_luminance,
> +	.disable_luminance = radeon_disable_luminance,
>  };

[Severity: High]
Are .set_luminance and .disable_luminance also needed in
radeon_dp_connector_helper_funcs?

In radeon_add_atom_connector(), native DRM_MODE_CONNECTOR_eDP connectors
and DP-bridge (TRAVIS/NUTMEG) DRM_MODE_CONNECTOR_LVDS/eDP connectors use
radeon_dp_connector_helper_funcs:

	drm_connector_init_with_ddc(dev, &radeon_connector->base,
				    &radeon_edp_connector_funcs,
				    connector_type,
				    ddc);
	drm_connector_helper_add(&radeon_connector->base, &radeon_dp_connector_helper_funcs);

Their encoders have ATOM_DEVICE_LCD_SUPPORT set, so
radeon_link_encoder_connector() -> radeon_encoder_add_backlight() ->
radeon_atom_backlight_init() attaches the LUMINANCE property to those
connectors, but radeon_dp_connector_helper_funcs does not wire up
.set_luminance or .disable_luminance (and on DP-bridge connectors,
radeon_dvi_encoder() rather than radeon_best_single_encoder() selects the
encoder with enc_priv).

> diff --git a/drivers/gpu/drm/radeon/radeon_legacy_encoders.c b/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
> index 1f82e2b57f5d5..96b0dfd8e3521 100644
> --- a/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
> +++ b/drivers/gpu/drm/radeon/radeon_legacy_encoders.c

[ ... ]

> @@ -441,6 +434,36 @@ void radeon_legacy_backlight_init(struct radeon_encoder *radeon_encoder,
>  #endif
>  	}
>  
> +	if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
> +		props.brightness = pdata->negative ?
> +			RADEON_MAX_BL_LEVEL - backlight_level : backlight_level;
> +		if (drm_connector_attach_luminance_property(drm_connector, 0,
> +							    props.max_brightness,
> +							     props.brightness))
> +			goto error;
> +
> +		if (rdev->is_atom_bios) {
> +			struct radeon_encoder_atom_dig *lvds = radeon_encoder->enc_priv;
> +
> +			lvds->bl_dev = NULL;
> +		} else {
> +			struct radeon_encoder_lvds *lvds = radeon_encoder->enc_priv;
> +
> +			lvds->bl_dev = NULL;
> +		}
> +		rdev->mode_info.bl_encoder = radeon_encoder;
> +		return;
> +	}

[Severity: High]
When lvds->bl_dev is set to NULL here without initializing
lvds->backlight_level, won't radeon_legacy_lvds_update() ignore brightness
updates and radeon_legacy_set_backlight_level() power off the panel?

In radeon_legacy_lvds_update(), lvds->backlight_level is only used if
lvds->bl_dev is non-NULL:

	if (radeon_encoder->enc_priv) {
		if (rdev->is_atom_bios) {
			struct radeon_encoder_atom_dig *lvds = radeon_encoder->enc_priv;
			panel_pwr_delay = lvds->panel_pwr_delay;
			if (lvds->bl_dev)
				backlight_level = lvds->backlight_level;
		} else {
			struct radeon_encoder_lvds *lvds = radeon_encoder->enc_priv;
			panel_pwr_delay = lvds->panel_pwr_delay;
			if (lvds->bl_dev)
				backlight_level = lvds->backlight_level;
		}
	}

Furthermore, radeon_legacy_set_backlight_level() checks the previous value
of lvds->backlight_level before updating it:

	if (radeon_encoder->enc_priv) {
		if (rdev->is_atom_bios) {
			struct radeon_encoder_atom_dig *lvds = radeon_encoder->enc_priv;
			if (lvds->backlight_level > 0)
				dpms_mode = lvds->dpms_mode;
			else
				dpms_mode = DRM_MODE_DPMS_OFF;
			lvds->backlight_level = level;
		} else {
			struct radeon_encoder_lvds *lvds = radeon_encoder->enc_priv;
			if (lvds->backlight_level > 0)
				dpms_mode = lvds->dpms_mode;
			else
				dpms_mode = DRM_MODE_DPMS_OFF;
			lvds->backlight_level = level;
		}
	}

	radeon_legacy_lvds_update(&radeon_encoder->base, dpms_mode);

Because lvds->backlight_level starts at 0 (or becomes 0 when maximum
brightness is set on a pdata->negative panel where
level = RADEON_MAX_BL_LEVEL - level), radeon_legacy_set_backlight_level()
will pass DRM_MODE_DPMS_OFF to radeon_legacy_lvds_update().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-superm1-backlight-property-v10-0-b2a6f24131f5@kernel.org?part=12