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 <[email protected]> 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-b2a6f2413...@kernel.org?part=12
