Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] drm/amdgpu: broken atomic LUMINANCE support on non-atomic legacy display - [High] drm/amd/display: skipped backlight restore and setup when DRM_LUMINANCE=y - [Medium] drm/amd/display: unconditional backlight programming blocks PSR/Replay - [Low] drm/amd: Kconfig and link breakage when DRM_LUMINANCE=y without backlight --
commit de26f6665aed29ccc412fe4be3f0737b06b98759 Author: Mario Limonciello <[email protected]> drm/amd: add luminance property support This commit adds support for the DRM LUMINANCE connector property on internal panels when CONFIG_DRM_LUMINANCE is enabled, bypassing sysfs backlight device registration and wiring connector luminance callbacks for Display Core and legacy display paths. > diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig > b/drivers/gpu/drm/amd/amdgpu/Kconfig > index 50305630f5335..7a6a049aa6e90 100644 > --- a/drivers/gpu/drm/amd/amdgpu/Kconfig > +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig > @@ -20,7 +20,7 @@ config DRM_AMDGPU > select I2C > select I2C_ALGOBIT > select CRC16 > - select BACKLIGHT_CLASS_DEVICE > + select BACKLIGHT_CLASS_DEVICE if !DRM_LUMINANCE > select INTERVAL_TREE > select DRM_BUDDY > select DRM_SUBALLOC_HELPER [Severity: Low] Does dropping select BACKLIGHT_CLASS_DEVICE when CONFIG_DRM_LUMINANCE is enabled cause Kconfig dependency warnings and link errors when CONFIG_BACKLIGHT_CLASS_DEVICE is disabled (or built as a module with CONFIG_DRM_AMDGPU=y)? In drivers/gpu/drm/amd/amdgpu/Kconfig, config DRM_AMDGPU still unconditionally selects ACPI_VIDEO when ACPI is enabled: # decidable, for the moment, to just select all of ACPI_VIDEO's # dependencies, which is fewer than you might think. select ACPI_VIDEO if ACPI Because config ACPI_VIDEO in drivers/acpi/Kconfig depends on BACKLIGHT_CLASS_DEVICE, enabling CONFIG_DRM_LUMINANCE=y with CONFIG_ACPI=y and CONFIG_BACKLIGHT_CLASS_DEVICE=n triggers an unmet direct dependency warning for ACPI_VIDEO. In addition, include/linux/backlight.h declares backlight_device_register(), backlight_device_unregister(), and backlight_device_set_brightness() without inline stubs when CONFIG_BACKLIGHT_CLASS_DEVICE is disabled, while amdgpu still compiles direct references to those symbols: - amdgpu_dm_register_backlight_device() in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c is a non-static function compiled unconditionally that calls backlight_device_register(). - amdgpu_dm_connector_destroy() in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c and amdgpu_atombios_encoder_fini_backlight() in drivers/gpu/drm/amd/amdgpu/atombios_encoders.c call backlight_device_unregister() behind runtime pointer checks. - amdgpu_atif_handler() in drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c calls backlight_device_set_brightness(atif->bd, req.backlight_level) behind a runtime check on atif->bd. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > index 92c98e999efe9..209a7a6c246db 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > @@ -798,11 +798,31 @@ static int amdgpu_connector_set_lcd_property(struct > drm_connector *connector, > return 0; > } > > +static void amdgpu_connector_set_luminance(const struct drm_connector_state > *conn_state) > +{ > + struct drm_encoder *encoder = > + amdgpu_connector_best_single_encoder(conn_state->connector); > + > + if (encoder) > + > amdgpu_atombios_encoder_set_backlight_level(to_amdgpu_encoder(encoder), > + > conn_state->luminance); > +} > + > +static void amdgpu_connector_disable_luminance(const struct > drm_connector_state *conn_state) > +{ > + struct drm_encoder *encoder = > + amdgpu_connector_best_single_encoder(conn_state->connector); > + > + if (encoder) > + > amdgpu_atombios_encoder_set_backlight_level(to_amdgpu_encoder(encoder), 0); > +} > > static const struct drm_connector_helper_funcs > amdgpu_connector_lvds_helper_funcs = { > .get_modes = amdgpu_connector_lvds_get_modes, > .mode_valid = amdgpu_connector_lvds_mode_valid, > .best_encoder = amdgpu_connector_best_single_encoder, > + .set_luminance = amdgpu_connector_set_luminance, > + .disable_luminance = amdgpu_connector_disable_luminance, > }; > > static const struct drm_connector_funcs amdgpu_connector_lvds_funcs = { > diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c > b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c > index a1d05df0ac99b..dbff532ac1bcd 100644 > --- a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c > +++ b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c > @@ -191,6 +191,21 @@ void amdgpu_atombios_encoder_init_backlight(struct > amdgpu_encoder *amdgpu_encode > goto register_acpi_backlight; > } > > + dig = amdgpu_encoder->enc_priv; > + if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) { > + unsigned int brightness; > + > + brightness = > amdgpu_atombios_encoder_get_backlight_level(amdgpu_encoder); > + if (drm_connector_attach_luminance_property(drm_connector, 0, > + AMDGPU_MAX_BL_LEVEL, > + brightness)) > + return; > + > + dig->backlight_level = brightness; > + dig->bl_dev = NULL; > + return; > + } [Severity: High] Can atomic LUMINANCE work on the legacy non-DC display path in drivers/gpu/drm/amd/amdgpu/atombios_encoders.c and amdgpu_connectors.c? Those files are used only when Display Core is not active, where amdgpu_pci_probe() in drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c clears DRIVER_ATOMIC: if (!supports_atomic) ddev->driver_features &= ~DRIVER_ATOMIC; On this legacy path, connectors do not implement atomic state callbacks (.reset, .atomic_duplicate_state, .atomic_destroy_state), so drm_connector->state is NULL, and modesets use legacy CRTC helpers (drm_crtc_helper_set_config() and drm_helper_connector_dpms()). Because drm_connector_attach_luminance_property() creates LUMINANCE with DRM_MODE_PROP_ATOMIC, non-atomic drivers hide LUMINANCE from userspace, legacy property updates go through amdgpu_connector_set_lcd_property() (which ignores everything except scaling_mode_property), and the legacy helpers never invoke drm_atomic_helper_commit_luminance_enables() or drm_atomic_helper_commit_luminance_disables(). Skipping backlight_device_register() in amdgpu_atombios_encoder_init_backlight() therefore leaves legacy panels with no working brightness control when CONFIG_DRM_LUMINANCE=y. Even if atomic callbacks were reachable here, there are two additional issues in drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c: 1. .set_luminance and .disable_luminance are only added to amdgpu_connector_lvds_helper_funcs and are missing from amdgpu_connector_dp_helper_funcs, which is used by DRM_MODE_CONNECTOR_eDP and DP-bridge LVDS connectors that also call amdgpu_atombios_encoder_init_backlight(). 2. amdgpu_connector_disable_luminance() calls amdgpu_atombios_encoder_set_backlight_level(to_amdgpu_encoder(encoder), 0), which overwrites dig->backlight_level = 0 and mmBIOS_SCRATCH_2 = 0, clobbering the saved brightness level used by amdgpu_atombios_encoder_setup_dig() on enable and by dce_v*_suspend(). [ ... ] > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index 0ea45ab12119c..53ff9a55c3a8e 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c [ ... ] > @@ -5397,11 +5398,16 @@ static void amdgpu_dm_atomic_commit_tail(struct > drm_atomic_commit *state) > /* Update audio instances for each connector. */ > amdgpu_dm_commit_audio(dev, state); > > - /* restore the backlight level */ > - for (i = 0; i < dm->num_of_edps; i++) { > - if (dm->backlight_dev[i] && > - (dm->actual_brightness[i] != dm->brightness[i])) > - amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]); > + if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) { > + /* Apply brightness after the display pipeline is enabled. */ > + drm_atomic_helper_commit_luminance_enables(state); > + } else { > + /* Restore the legacy backlight level. */ > + for (i = 0; i < dm->num_of_edps; i++) { > + if (dm->backlight_dev[i] && > + dm->actual_brightness[i] != dm->brightness[i]) > + amdgpu_dm_backlight_set_level(dm, i, > dm->brightness[i]); > + } > } [Severity: Medium] Unlike the !CONFIG_DRM_LUMINANCE branch, which checks dm->actual_brightness[i] != dm->brightness[i], doesn't calling drm_atomic_helper_commit_luminance_enables(state) -> amdgpu_dm_set_luminance() -> amdgpu_dm_backlight_set_level() here reprogram the backlight on every atomic commit that includes an active eDP connector in state even when state->luminance has not changed? In amdgpu_dm_atomic_commit_tail(), drm_atomic_helper_commit_luminance_enables() runs after amdgpu_dm_commit_planes(). Inside amdgpu_dm_backlight_set_level() in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c, the driver exits IPS, reprograms the backlight via mod_power_set_backlight_nits() or mod_power_set_backlight_percent(), and disables PSR and Panel Replay until the next plane flip: amdgpu_dm_psr_set_event(dm, stream, true, psr_event_hw_programming, true); amdgpu_dm_replay_set_event(dm, stream, true, replay_event_hw_programming, true); Because amdgpu_dm_commit_planes() -> amdgpu_dm_enable_self_refresh() has already run earlier in amdgpu_dm_atomic_commit_tail(), any commit that pulls the active eDP connector into state without changing state->luminance (such as CRTC color management or VRR updates via drm_atomic_add_affected_connectors(), ABM/HDR/scaling connector property updates, or compositor commits that include connector properties) will immediately re-assert psr_event_hw_programming and replay_event_hw_programming and block PSR/Replay entry. > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c > index 0f664c658a127..0857354e7327e 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c [ ... ] > @@ -657,6 +662,10 @@ void amdgpu_dm_setup_backlight_device(struct > amdgpu_display_manager *dm, > { > struct dc_link *link = aconnector->dc_link; > int bl_idx = dm->num_of_edps; > + struct backlight_properties props = { 0 }; > + struct amdgpu_dm_backlight_caps *caps; > + struct drm_connector *connector = &aconnector->base; > + int ret; > > if (!(link->connector_signal & (SIGNAL_TYPE_EDP | SIGNAL_TYPE_LVDS)) || > link->type == dc_connection_none) > @@ -675,6 +684,19 @@ void amdgpu_dm_setup_backlight_device(struct > amdgpu_display_manager *dm, > > amdgpu_dm_update_connector_ext_caps(aconnector); > > + caps = &dm->backlight_caps[bl_idx]; > + amdgpu_dm_backlight_fill_props(caps, power_supply_is_system_supplied() > > 0, > + !(amdgpu_dc_debug_mask & > + DC_DISABLE_CUSTOM_BRIGHTNESS_CURVE), > + &props); > + if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) { > + unsigned int max = props.max_brightness; > + unsigned int value = props.brightness; > + > + ret = drm_connector_attach_luminance_property(connector, 0, > max, value); > + drm_WARN_ON(adev_to_drm(dm->adev), ret); > + } > + > /* Offer ABM property when user didn't turn off by module parameter. > * OLED panels are included to support CACP (Content Adaptive > * Contrast and Power) feature via set_abm_level. [ ... ] > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c > index b498fc5fbb140..f2c2a2e730d3e 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c [ ... ] > @@ -2038,7 +2039,8 @@ amdgpu_dm_connector_late_register(struct drm_connector > *connector) > return r; > } > > - amdgpu_dm_register_backlight_device(amdgpu_dm_connector); > + if (!IS_ENABLED(CONFIG_DRM_LUMINANCE)) > + amdgpu_dm_register_backlight_device(amdgpu_dm_connector); [Severity: High] When CONFIG_DRM_LUMINANCE is enabled, skipping amdgpu_dm_register_backlight_device() leaves dm->backlight_dev[i] as NULL and leaves dm->brightness[bl_idx] uninitialized (0) in amdgpu_dm_setup_backlight_device(). Doesn't this break several backlight restoration and initialization paths in Display Core? Specifically: 1. In dm_resume() in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c, the GPU reset recovery branch (if (amdgpu_in_reset(adev))) does not go through drm_atomic_helper_resume() or amdgpu_dm_atomic_commit_tail(), and instead restores panel brightness via: /* set the backlight after a reset */ for (i = 0; i < dm->num_of_edps; i++) { if (dm->backlight_dev[i]) amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]); } Because dm->backlight_dev[i] is NULL (and dm->brightness[i] is 0 until a luminance commit occurs), panel brightness will not be restored after a GPU reset. 2. In amdgpu_dm_commit_streams() in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c, after dc_commit_streams() is called, backlight restoration is likewise gated on dm->backlight_dev[i]: if (set_backlight_level) { for (i = 0; i < dm->num_of_edps; i++) { if (dm->backlight_dev[i]) amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]); } } If a modeset happens on an external display on another CRTC while the eDP connector is not part of the atomic commit state, neither amdgpu_dm_commit_streams() nor drm_atomic_helper_commit_luminance_enables(state) will restore the eDP brightness. 3. Unlike amdgpu_dm_register_backlight_device(), amdgpu_dm_setup_backlight_device() does not check acpi_video_backlight_use_native() before attaching the LUMINANCE property, and skipping amdgpu_dm_register_backlight_device() also skips the acpi_video_register_backlight() fallback when native backlight control should not be used. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20261102-superm1-backlight-property-v10-0-b2a6f2413...@kernel.org?part=3
