Re: [PATCH v10 01/15] drm: add connector LUMINANCE property

Re: [PATCH v10 01/15] drm: add connector LUMINANCE property

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/atomic-helper: panel emission disabled on PSR entry in drm_atomic_helper_commit_luminance_disables()
- [Medium] drm/atomic-helper: redundant set_luminance() calls in drm_atomic_helper_commit_luminance_enables()
--

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

drm: add connector LUMINANCE property

This commit adds an atomic per-connector LUMINANCE range property along with
connector helper callbacks to set luminance or disable panel emission during
atomic commits.

> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 9d006f98413a5..1e9b7b8acccfe 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1375,6 +1375,8 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_encoder_bridge_post_disable);
>  static void
>  disable_outputs(struct drm_device *dev, struct drm_atomic_commit *state)
>  {
> +	drm_atomic_helper_commit_luminance_disables(state);
> +
>  	drm_atomic_helper_commit_encoder_bridge_disable(dev, state);
>  
>  	drm_atomic_helper_commit_encoder_bridge_post_disable(dev, state);
> @@ -1382,6 +1384,45 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_commit *state)
>  	drm_atomic_helper_commit_crtc_disable(dev, state);
>  }
>  
> +/**
> + * drm_atomic_helper_commit_luminance_disables - disable panel emission
> + * @state: atomic state object being committed
> + *
> + * Invokes each connector driver's luminance disable callback before a
> + * display pipeline is disabled. The connector property's stored value is left
> + * untouched so it can be restored when the pipeline is enabled again.
> + */
> +void drm_atomic_helper_commit_luminance_disables(struct drm_atomic_commit *state)
> +{
> +	struct drm_connector_state *old_conn_state, *new_conn_state;
> +	struct drm_connector *connector;
> +	int i;
> +
> +	for_each_oldnew_connector_in_state(state, connector, old_conn_state,
> +					   new_conn_state, i) {
> +		const struct drm_connector_helper_funcs *funcs;
> +		struct drm_crtc_state *old_crtc_state, *new_crtc_state = NULL;
> +
> +		if (!connector->luminance_property || !old_conn_state->crtc)
> +			continue;
> +
> +		old_crtc_state =
> +			drm_atomic_get_old_crtc_state(state, old_conn_state->crtc);
> +		if (new_conn_state->crtc)
> +			new_crtc_state =
> +				drm_atomic_get_new_crtc_state(state, new_conn_state->crtc);
> +
> +		if (!crtc_needs_disable(old_crtc_state, new_crtc_state) ||
> +		    !drm_atomic_crtc_needs_modeset(old_conn_state->crtc->state))
> +			continue;
> +
> +		funcs = connector->helper_private;
> +		if (funcs && funcs->disable_luminance)
> +			funcs->disable_luminance(old_conn_state);
> +	}
> +}

[Severity: High]
Will drm_atomic_helper_commit_luminance_disables() turn off panel emission
when a display enters Panel Self-Refresh (PSR)?

When a CRTC enters self-refresh via drm_self_refresh_helper_entry_work() in
drivers/gpu/drm/drm_self_refresh_helper.c, affected connectors are added to
the commit with new_crtc_state->active set to false and
new_crtc_state->self_refresh_active set to true. That transition sets
new_crtc_state->active_changed, so drm_atomic_crtc_needs_modeset() returns
true, and crtc_needs_disable() also returns true because old_state->active
and new_state->self_refresh_active are set:

drivers/gpu/drm/drm_atomic_helper.c:crtc_needs_disable() {
	...
	return old_state->active ||
	       (old_state->self_refresh_active && !new_state->active) ||
	       new_state->self_refresh_active;
}

For comparison, panel_bridge_atomic_disable() in drivers/gpu/drm/drm_panel.c
explicitly skips disabling the panel when entering self-refresh:

drivers/gpu/drm/drm_panel.c:panel_bridge_atomic_disable() {
	...
	new_crtc_state = drm_atomic_get_new_crtc_state(atomic_state, crtc);
	if (new_crtc_state && new_crtc_state->self_refresh_active)
		return;

	drm_panel_disable(panel);
}

Without a similar check for new_crtc_state->self_refresh_active here (and
because drm_atomic_helper_commit_luminance_enables() skips set_luminance()
while new_conn_state->crtc->state->active is false), wouldn't
funcs->disable_luminance(old_conn_state) be called on PSR entry and leave the
panel dark while displaying the static frame?

> +EXPORT_SYMBOL(drm_atomic_helper_commit_luminance_disables);
> +
>  /**
>   * drm_atomic_helper_update_legacy_modeset_state - update legacy modeset state
>   * @dev: DRM device
> @@ -1771,10 +1812,41 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>  
>  	drm_atomic_helper_commit_encoder_bridge_enable(dev, state);
>  
> +	drm_atomic_helper_commit_luminance_enables(state);
> +
>  	drm_atomic_helper_commit_writebacks(dev, state);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
>  
> +/**
> + * drm_atomic_helper_commit_luminance_enables - apply luminance
> + * @state: atomic state object being committed
> + *
> + * Invokes each connector driver's luminance callback after the display
> + * pipeline has been enabled. The callback is also invoked for brightness-only
> + * commits while the connector remains active. Changes committed while the
> + * connector is inactive remain stored in atomic state until a later enable.
> + */
> +void drm_atomic_helper_commit_luminance_enables(struct drm_atomic_commit *state)
> +{
> +	struct drm_connector_state *new_conn_state;
> +	struct drm_connector *connector;
> +	int i;
> +
> +	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> +		const struct drm_connector_helper_funcs *funcs;
> +
> +		if (!connector->luminance_property || !new_conn_state->crtc ||
> +		    !new_conn_state->crtc->state->active)
> +			continue;
> +
> +		funcs = connector->helper_private;
> +		if (funcs && funcs->set_luminance)
> +			funcs->set_luminance(new_conn_state);
> +	}
> +}

[Severity: Medium]
Should drm_atomic_helper_commit_luminance_enables() check whether the CRTC
needs a modeset or old_conn_state->luminance != new_conn_state->luminance
before calling funcs->set_luminance(new_conn_state)?

drm_atomic_helper_commit_modeset_enables() runs on every atomic commit via
drm_atomic_helper_commit_tail() and drm_atomic_helper_commit_tail_rpm(). Any
non-modeset commit on an active CRTC that includes the connector in the
atomic state (such as updates to other connector properties like
hdr_output_metadata or max_bpc, compositors submitting unchanged connector
properties during page flips, or driver atomic_check hooks calling
drm_atomic_add_affected_connectors()) will invoke funcs->set_luminance() and
reprogram the hardware even when the luminance level and power state are
unchanged.

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

Re: [PATCH v10 03/15] drm/amd: add luminance property support

[sashiko-bot]

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 <mario.limonciello@amd.com>

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-b2a6f24131f5@kernel.org?part=3

Re: [PATCH v10 05/15] drm/amd/display: update KUnit luminance fixtures

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/amd/display: wrong connector_type IDA freed in setup_test_connector() user
- [Medium] drm/amd/display: link failure in backlight KUnit test when DRM_LUMINANCE=y
--

commit 66d823079de9e4318719fbb2464a13997a1ae155
Author: Mario Limonciello (AMD) <superm1@kernel.org>

drm/amd/display: update KUnit luminance fixtures

This commit updates the backlight KUnit test fixtures to initialize DRM mode
configuration, connector state, and the display manager lock so tests work
when the atomic LUMINANCE property is enabled.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
> index b21850788f36f..8883010e8af9c 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
> @@ -41,12 +41,21 @@ static void setup_test_connector(struct kunit *test,
>  				 struct dm_backlight_connector_fixture *fixture,
>  				 int bl_idx, enum signal_type signal)
>  {
> -	fixture->adev = kunit_kzalloc(test, sizeof(*fixture->adev), GFP_KERNEL);
> -	fixture->aconnector = kunit_kzalloc(test, sizeof(*fixture->aconnector), GFP_KERNEL);
> +	int ret;
> +
> +	fixture->adev = dm_kunit_alloc_adev(test);
> +	ret = drmm_mode_config_init(&fixture->adev->ddev);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	fixture->aconnector = dm_kunit_alloc_connector(test, fixture->adev, NULL);
>  	fixture->link = kunit_kzalloc(test, sizeof(*fixture->link), GFP_KERNEL);
> -	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fixture->adev);
> -	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fixture->aconnector);
>  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fixture->link);
> +	ret = drmm_connector_init(&fixture->adev->ddev,
> +				  &fixture->aconnector->base,
> +				  &dm_backlight_test_connector_funcs,
> +				  DRM_MODE_CONNECTOR_eDP, NULL);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +	drm_atomic_helper_connector_reset(&fixture->aconnector->base);

[Severity: Medium]
Does hardcoding DRM_MODE_CONNECTOR_eDP in setup_test_connector() corrupt or
leak the connector_type_id IDA when dm_test_should_create_sysfs_non_edp() runs?

drmm_connector_init() -> drm_connector_init_and_add() ->
drm_connector_init_only() allocates connector->connector_type_id from
drm_connector_enum_list[DRM_MODE_CONNECTOR_eDP].ida and registers
drm_connector_cleanup_action() for managed teardown.

However, dm_test_should_create_sysfs_non_edp() mutates connector_type after
calling setup_test_connector():

drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c:dm_test_should_create_sysfs_non_edp() {
	setup_test_connector(test, &fixture, 0, SIGNAL_TYPE_HDMI_TYPE_A);
	fixture.aconnector->base.connector_type = DRM_MODE_CONNECTOR_HDMIA;
	...
}

When the test finishes and drm_connector_cleanup() runs:

drivers/gpu/drm/drm_connector.c:drm_connector_cleanup() {
	...
	ida_free(&drm_connector_enum_list[connector->connector_type].ida,
		 connector->connector_type_id);
	...
}

it frees connector_type_id back to the DRM_MODE_CONNECTOR_HDMIA IDA instead of
DRM_MODE_CONNECTOR_eDP, triggering a warning in ida_free() (or freeing an
active HDMI-A ID) while leaking the eDP connector_type_id.

> @@ -483,12 +492,17 @@ static void dm_test_register_backlight_device_success(struct kunit *test)
>  	struct dc_link *link = dm_kunit_alloc_link(test);
>  	struct drm_minor *primary;
>  	unsigned int max;
> +	int ret;
> +
> +	ret = drmm_mode_config_init(&adev->ddev);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
>  
>  	setup_test_link_service(test, link);
>  	link->dc->link_srv->edp_get_backlight_level = dm_test_get_backlight_level_error;
>  	primary = kunit_kzalloc(test, sizeof(*primary), GFP_KERNEL);
>  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, primary);
>  	adev->ddev.primary = primary;
> +	adev->dm.ddev = &adev->ddev;
>  	adev->dm.backlight_link[0] = link;

[Severity: Medium]
Can this test suite link when CONFIG_DRM_LUMINANCE=y and
CONFIG_BACKLIGHT_CLASS_DEVICE is disabled?

With commit de26f6665aed ("drm/amd: add luminance property support"),
DRM_AMDGPU in drivers/gpu/drm/amd/amdgpu/Kconfig only selects
BACKLIGHT_CLASS_DEVICE when !DRM_LUMINANCE:

	select BACKLIGHT_CLASS_DEVICE if !DRM_LUMINANCE

Neither DRM_AMD_DC_KUNIT_TEST in drivers/gpu/drm/amd/display/Kconfig nor
drivers/gpu/drm/amd/display/amdgpu_dm/tests/.kunitconfig enables
CONFIG_BACKLIGHT_CLASS_DEVICE.

However, dm_test_register_backlight_device_success() and
dm_test_unregister_backlight_device() in amdgpu_dm_backlight_test.c, as well
as amdgpu_dm_register_backlight_device() in amdgpu_dm_backlight.c, still
unconditionally call backlight_device_register() and
backlight_device_unregister():

drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c:dm_test_unregister_backlight_device() {
	backlight_device_unregister(data);
}

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c:amdgpu_dm_register_backlight_device() {
	...
	dm->backlight_dev[aconnector->bl_idx] =
		backlight_device_register(bl_name, aconnector->base.kdev, dm,
					  &amdgpu_dm_backlight_ops, &props);
	...
}

Because include/linux/backlight.h does not provide inline stubs for
backlight_device_register() or backlight_device_unregister() when
CONFIG_BACKLIGHT_CLASS_DEVICE is disabled, won't building the KUnit tests (or
amdgpu) with CONFIG_DRM_LUMINANCE=y and CONFIG_BACKLIGHT_CLASS_DEVICE=n fail
at link time with undefined references to those symbols?

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

Re: [PATCH v10 06/15] drm/i915/display: add luminance property support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/i915/display: missing acpi_video_backlight_use_native() call
- [Medium] drm/i915/display: stale backlight level programmed on modeset enable
- [Low] drm/i915/display: build failure in intel_backlight_set_luminance()
- [Low] drm/i915: unmet BACKLIGHT_CLASS_DEVICE dependency for ACPI_VIDEO
--

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

drm/i915/display: add luminance property support

This commit adds support for the DRM LUMINANCE connector property on eDP,
LVDS, and DSI panels in i915 and Xe when CONFIG_DRM_LUMINANCE is enabled,
while retaining the backlight class device when it is disabled.

> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 71426b9c91f00..720191673a1e7 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -26,7 +26,7 @@ config DRM_I915
>  	select IRQ_WORK
>  	# i915 depends on ACPI_VIDEO when ACPI is enabled
>  	# but for select to work, need to select ACPI_VIDEO's dependencies, ick
> -	select BACKLIGHT_CLASS_DEVICE if ACPI
> +	select BACKLIGHT_CLASS_DEVICE if ACPI && !DRM_LUMINANCE
>  	select INPUT if ACPI
>  	select ACPI_WMI if ACPI
>  	select ACPI_VIDEO if ACPI

[Severity: Low]
Does this cause an unmet direct dependency for ACPI_VIDEO when
CONFIG_DRM_LUMINANCE=y, CONFIG_ACPI=y, and CONFIG_BACKLIGHT_CLASS_DEVICE=n?

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

    config ACPI_VIDEO
        tristate "Video"
        depends on BACKLIGHT_CLASS_DEVICE

Both DRM_I915 here and DRM_XE in drivers/gpu/drm/xe/Kconfig still have:

    select ACPI_VIDEO if ACPI

When DRM_LUMINANCE is enabled and nothing else selects
BACKLIGHT_CLASS_DEVICE, Kconfig warns about an unmet direct dependency on
BACKLIGHT_CLASS_DEVICE and drivers/acpi/acpi_video.c fails to link due to
undefined references to backlight_device_register() and
backlight_device_unregister().

[ ... ]

> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
> index b128896cb1c2d..6d658c7b5d99f 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c

[ ... ]

> @@ -1712,6 +1723,15 @@ int intel_backlight_setup(struct intel_connector *connector, enum pipe pipe)
>  	return 0;
>  }
>  
> +void intel_backlight_set_luminance(const struct drm_connector_state *conn_state)
> +{
> +	struct intel_connector *connector =
> +		to_intel_connector(conn_state->connector);
> +
> +	intel_panel_set_backlight(conn_state, conn_state->luminance,
> +				  connector->panel.backlight.max);
> +}

[Severity: Low]
Will drivers/gpu/drm/i915/display/intel_backlight.c fail to build when
CONFIG_BACKLIGHT_CLASS_DEVICE is disabled?

In drivers/gpu/drm/i915/display/intel_backlight.c, scale_user_to_hw() and
intel_panel_set_backlight() are defined inside an
#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) block:

    #if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
    static u32 intel_panel_get_backlight(struct intel_connector *connector)
    ...
    static void intel_panel_set_backlight(const struct drm_connector_state *conn_state,
                                          u32 user_level, u32 user_max)

Because intel_backlight_set_luminance() is compiled unconditionally outside
that guard, building with CONFIG_BACKLIGHT_CLASS_DEVICE=n (for example when
CONFIG_DRM_LUMINANCE=y) fails with an implicit declaration of
intel_panel_set_backlight().

[ ... ]

> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index d5d4453f283bb..3c81ed630c365 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -7696,6 +7696,8 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>  	display->modeset.funcs->commit_modeset_enables(state);
>  
> +	drm_atomic_helper_commit_luminance_enables(&state->base);
> +
>  	intel_display_power_dc3co_compute(state);

[Severity: Medium]
When a panel's LUMINANCE property is updated while the CRTC is inactive (or
in the same atomic commit that enables the CRTC), doesn't calling
drm_atomic_helper_commit_luminance_enables() after commit_modeset_enables()
initially turn on the panel at the stale brightness level?

While the CRTC is inactive, drm_atomic_helper_commit_luminance_enables()
skips calling .set_luminance(), so panel->backlight.level keeps the old
hardware brightness level while conn_state->luminance holds the new value.

During a subsequent modeset enable in intel_atomic_commit_tail():

    display->modeset.funcs->commit_modeset_enables(state)
      -> intel_edp_backlight_on() / intel_enable_lvds() / gen11_dsi_enable()
        -> intel_backlight_enable()
          -> __intel_backlight_enable()
            -> panel->backlight.funcs->enable(crtc_state, conn_state,
                                              panel->backlight.level);

This powers on and programs the backlight with the old
panel->backlight.level and waits for vblank before
drm_atomic_helper_commit_luminance_enables() calls
intel_backlight_set_luminance() to update panel->backlight.level and
reprogram the hardware a second time.

[ ... ]

> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
> index 81e638d0c7b33..501573a75b89c 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -610,9 +610,11 @@ int intel_panel_register(struct intel_connector *connector)
>  	struct intel_panel *panel = &connector->panel;
>  	int ret;
>  
> -	ret = intel_backlight_device_register(connector);
> -	if (ret)
> -		return ret;
> +	if (!IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
> +		ret = intel_backlight_device_register(connector);
> +		if (ret)
> +			return ret;
> +	}

[Severity: High]
When CONFIG_DRM_LUMINANCE is enabled, how does the driver coordinate with
ACPI video backlight detection and BIOS OpRegion ASLE backlight requests now
that intel_backlight_device_register() is skipped?

Previously, intel_backlight_device_register() called
acpi_video_backlight_use_native(), which notifies drivers/acpi/video_detect.c
that native GPU backlight control is present (setting native_available = true)
and checks whether native backlight control should be used. It also set
panel->backlight.device.

With intel_backlight_device_register() skipped and
intel_backlight_setup() attaching the LUMINANCE property without calling
acpi_video_backlight_use_native():

1. In drivers/gpu/drm/i915/display/intel_acpi.c, intel_acpi_video_register()
checks:

    if (panel->backlight.funcs && !panel->backlight.device) {
        acpi_video_register_backlight();
        break;
    }

Because panel->backlight.device is NULL and acpi_video_backlight_use_native()
was never called, intel_acpi_video_register() will register a conflicting
acpi_video backlight device if CONFIG_BACKLIGHT_CLASS_DEVICE is enabled.

2. In drivers/gpu/drm/i915/display/intel_opregion.c, asle_set_backlight()
checks:

    if (acpi_video_get_backlight_type() == acpi_backlight_native) {
        drm_dbg_kms(display->drm,
                    "opregion backlight request ignored\n");
        return 0;
    }

Because acpi_video_backlight_use_native() was never called,
acpi_video_get_backlight_type() will not return acpi_backlight_native, so
BIOS OpRegion ASLE requests will call intel_backlight_set_acpi() and modify
panel->backlight.level behind the atomic LUMINANCE property's back.

3. In intel_backlight_setup(), the LUMINANCE property is attached even on
systems where acpi_video_backlight_use_native() would return false.

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

Re: [PATCH v10 07/15] drm/bridge: add connector luminance operations

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] drm/bridge: rebase artifact in drm_bridge.h causes redefinition build failure
--

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

drm/bridge: add connector luminance operations

This commit adds a set_luminance callback, DRM_BRIDGE_OP_LUMINANCE flag, and
relative luminance fields to struct drm_bridge so bridges that control panel
brightness can advertise luminance operations.

> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 1981d24a700d2..95f5c2b8656cd 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h

[ ... ]

> @@ -1522,6 +1542,60 @@ void drm_bridge_hpd_disable(struct drm_bridge *bridge);
>  void drm_bridge_hpd_notify(struct drm_bridge *bridge,
>  			   enum drm_connector_status status);
>  
> +#ifdef CONFIG_DRM_PANEL_BRIDGE
> +bool drm_bridge_is_panel(const struct drm_bridge *bridge);
> +struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel);
> +struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel,
> +					      u32 connector_type);
> +void drm_panel_bridge_remove(struct drm_bridge *bridge);
> +int drm_panel_bridge_set_orientation(struct drm_connector *connector,
> +				     struct drm_bridge *bridge);
> +struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
> +					     struct drm_panel *panel);
> +struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev,
> +						   struct drm_panel *panel,
> +						   u32 connector_type);
> +struct drm_bridge *drmm_panel_bridge_add(struct drm_device *drm,
> +					     struct drm_panel *panel);
> +struct drm_connector *drm_panel_bridge_connector(struct drm_bridge *bridge);
> +#else
> +static inline bool drm_bridge_is_panel(const struct drm_bridge *bridge)
> +{
> +	return false;
> +}
> +
> +static inline int drm_panel_bridge_set_orientation(struct drm_connector *connector,
> +						   struct drm_bridge *bridge)
> +{
> +	return -EINVAL;
> +}
> +
> +#endif
> +
> +#if defined(CONFIG_OF) && defined(CONFIG_DRM_PANEL_BRIDGE)
> +struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, struct device_node *node,
> +					  u32 port, u32 endpoint);
> +struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm, struct device_node *node,
> +					  u32 port, u32 endpoint);
> +#else
> +static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev,
> +							struct device_node *node,
> +							u32 port,
> +							u32 endpoint)
> +{
> +	return ERR_PTR(-ENODEV);\n> +}
> +
> +static inline struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm,
> +						     struct device_node *node,
> +						     u32 port,
> +						     u32 endpoint)
> +{
> +	return ERR_PTR(-ENODEV);\n> +}
> +#endif
> +
> +void devm_drm_put_bridge(struct device *dev, struct drm_bridge *bridge);

[Severity: Low]
Was this hunk included by mistake during a rebase or patch split?

Prior to this series, commit aa8a8c7e4641 ("drm/bridge: panel: move all code
to drm_panel.c") moved the CONFIG_DRM_PANEL_BRIDGE and CONFIG_OF panel-bridge
declarations and their #else static inline fallback definitions from
include/drm/drm_bridge.h into include/drm/drm_panel.h, and commit 51afe16e33a0
("drm/bridge: remove devm_drm_put_bridge()") removed devm_drm_put_bridge().

Because include/drm/drm_panel.h includes drm/drm_bridge.h and already defines
those same static inline functions in its #else branches:

include/drm/drm_panel.h:
    #ifdef CONFIG_DRM_PANEL_BRIDGE
    bool drm_bridge_is_panel(const struct drm_bridge *bridge);
    ...
    #else
    static inline bool drm_bridge_is_panel(const struct drm_bridge *bridge)
    {
        return false;
    }

    static inline int drm_panel_bridge_set_orientation(struct drm_connector *connector,
                                                       struct drm_bridge *bridge)
    {
        return -EINVAL;
    }
    #endif

    #if defined(CONFIG_OF) && defined(CONFIG_DRM_PANEL_BRIDGE)
    ...
    #else
    static inline struct drm_bridge *devm_drm_of_get_bridge(...)
    ...
    static inline struct drm_bridge *drmm_of_get_bridge(...)
    ...
    #endif

does re-adding this block in include/drm/drm_bridge.h cause redefinition build
failures in translation units that include drm/drm_panel.h (such as
drivers/gpu/drm/display/drm_bridge_connector.c, drivers/gpu/drm/drm_panel.c,
and drivers/gpu/drm/drm_of.c) when CONFIG_OF or CONFIG_DRM_PANEL_BRIDGE is
disabled?

Although a subsequent patch in the series, commit 3eac0878c8a6 ("drm/panel:
add connector luminance forwarding"), removes the #else static inline stubs
and the CONFIG_OF block from include/drm/drm_panel.h, this breaks bisection at
this commit and leaves the misplaced panel-bridge and OF bridge declarations
along with the deleted devm_drm_put_bridge() prototype in
include/drm/drm_bridge.h.

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

Re: [PATCH v10 08/15] drm/panel: add connector luminance forwarding

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] drm/panel: invalid container_of cast to panel_bridge in luminance callbacks
- [High] drm/panel: drm_panel_disable_luminance() and drm_panel_set_luminance() break PSR and blank state
- [Medium] drm/panel: missing drm_connector_cleanup() on error in panel_bridge_attach()
- [Low] drm/panel: unintended removal of panel bridge stubs from drm_panel.h
--

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

drm/panel: add connector luminance forwarding

This commit adds helpers to expose a panel backlight via the DRM connector
luminance property and hooks them into panel bridges that create their own
connector.

> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index c00529bfb706b..a5706edad9735 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -38,6 +38,19 @@ static LIST_HEAD(panel_list);
>   * take look at drm_panel_bridge_add() and devm_drm_panel_bridge_add().
>   */
>  
> +struct panel_bridge {
> +	struct drm_bridge bridge;
> +	struct drm_connector connector;
> +	struct drm_panel *panel;
> +	u32 connector_type;
> +};
> +
> +static inline struct panel_bridge *
> +drm_connector_to_panel_bridge(const struct drm_connector *connector)
> +{
> +	return container_of(connector, struct panel_bridge, connector);
> +}
> +
>  static inline struct drm_panel *
>  drm_bridge_to_panel(const struct drm_bridge *bridge)
>  {
> @@ -50,13 +63,6 @@ drm_connector_to_panel(const struct drm_connector *connector)
>  	return container_of(connector, struct drm_panel, connector);
>  }
>  
> -struct panel_bridge {
> -	struct drm_bridge bridge;
> -	struct drm_connector connector;
> -	struct drm_panel *panel;
> -	u32 connector_type;
> -};
> -
>  static int panel_bridge_connector_get_modes(struct drm_connector *connector)
>  {
>  	struct drm_panel *panel = drm_connector_to_panel(connector);
> @@ -64,9 +70,29 @@ static int panel_bridge_connector_get_modes(struct drm_connector *connector)
>  	return drm_panel_get_modes(panel, connector);
>  }
>  
> +static void
> +panel_bridge_connector_set_luminance(const struct drm_connector_state *conn_state)
> +{
> +	struct panel_bridge *panel_bridge =
> +		drm_connector_to_panel_bridge(conn_state->connector);
> +
> +	drm_panel_set_luminance(panel_bridge->panel, conn_state->luminance);
> +}
> +
> +static void
> +panel_bridge_connector_disable_luminance(const struct drm_connector_state *conn_state)
> +{
> +	struct panel_bridge *panel_bridge =
> +		drm_connector_to_panel_bridge(conn_state->connector);
> +
> +	drm_panel_disable_luminance(panel_bridge->panel);
> +}

[Severity: Critical]
Since commit b7797081ccfe ("drm/panel: embed a drm_bridge into every
drm_panel"), the bridge and connector are embedded directly inside struct
drm_panel, and struct panel_bridge is an unused leftover definition that is
never allocated.

In panel_bridge_attach(), the connector initialized with
panel_bridge_connector_helper_funcs is &panel->connector:

	struct drm_panel *panel = drm_bridge_to_panel(bridge);
	struct drm_connector *connector = &panel->connector;

Calling drm_connector_to_panel_bridge(conn_state->connector) in
panel_bridge_connector_set_luminance() and
panel_bridge_connector_disable_luminance() computes container_of() for struct
panel_bridge on a struct drm_panel instance, so reading panel_bridge->panel
reads the fields following connector in struct drm_panel (connector_type and
list) as a struct drm_panel pointer.

Won't passing that pointer to drm_panel_set_luminance() or
drm_panel_disable_luminance() dereference an invalid panel->backlight address,
and should these callbacks use drm_connector_to_panel(conn_state->connector)
instead?

>  static const struct drm_connector_helper_funcs
>  panel_bridge_connector_helper_funcs = {
>  	.get_modes = panel_bridge_connector_get_modes,
> +	.set_luminance = panel_bridge_connector_set_luminance,
> +	.disable_luminance = panel_bridge_connector_disable_luminance,
>  };
>  
>  static const struct drm_connector_funcs panel_bridge_connector_funcs = {
> @@ -101,6 +127,10 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
>  
>  	drm_panel_bridge_set_orientation(connector, bridge);
>  
> +	ret = drm_panel_attach_luminance_property(panel, connector);
> +	if (ret)
> +		return ret;

[Severity: Medium]
In panel_bridge_attach(), drm_connector_init() has already succeeded and
linked &panel->connector into bridge->dev->mode_config.connector_list before
drm_panel_attach_luminance_property() is called.

When bridge->funcs->attach() fails in drm_bridge_attach(), the error path
resets the bridge without calling bridge->funcs->detach().

Should this error path call drm_connector_cleanup(connector) before returning
ret so that &panel->connector is not left linked on mode_config.connector_list
if drm_panel_attach_luminance_property() fails?

[ ... ]

> @@ -861,6 +891,61 @@ void drm_panel_disable(struct drm_panel *panel)
>  }
>  EXPORT_SYMBOL(drm_panel_disable);
>  
> +/**
> + * drm_panel_attach_luminance_property - expose a panel backlight through DRM
> + * @panel: panel whose backlight is controlled
> + * @connector: connector on which to expose the property
> + *
> + * The backlight class device remains the internal hardware backend. When the
> + * luminance UAPI is enabled, userspace controls that backend through the DRM
> + * connector property instead.
> + *
> + * Return: 0 on success or a negative error code
> + */
> +int drm_panel_attach_luminance_property(struct drm_panel *panel,
> +					struct drm_connector *connector)
> +{
> +	struct backlight_device *backlight;
> +
> +	if (!IS_ENABLED(CONFIG_DRM_LUMINANCE) || !panel)
> +		return 0;
> +
> +	backlight = panel->backlight;
> +	if (!backlight)
> +		return 0;
> +
> +	return drm_connector_attach_luminance_property(connector, 0,
> +					backlight->props.max_brightness,
> +					backlight->props.brightness);
> +}
> +EXPORT_SYMBOL(drm_panel_attach_luminance_property);
> +
> +/**
> + * drm_panel_set_luminance - set a panel backlight's relative luminance
> + * @panel: panel whose backlight is controlled
> + * @luminance: relative luminance level
> + */
> +void drm_panel_set_luminance(struct drm_panel *panel, unsigned int luminance)
> +{
> +	if (!panel || !panel->backlight)
> +		return;
> +
> +	panel->backlight->props.brightness = luminance;
> +	backlight_update_status(panel->backlight);
> +}
> +EXPORT_SYMBOL(drm_panel_set_luminance);
> +
> +/**
> + * drm_panel_disable_luminance - turn off a panel backlight
> + * @panel: panel whose backlight is controlled
> + */
> +void drm_panel_disable_luminance(struct drm_panel *panel)
> +{
> +	if (panel)
> +		backlight_disable(panel->backlight);
> +}
> +EXPORT_SYMBOL(drm_panel_disable_luminance);

[Severity: High]
How do drm_panel_disable_luminance() and drm_panel_set_luminance() interact
with panel self-refresh and the backlight blank state?

In drm_panel_disable_luminance(), backlight_disable() sets:

	bd->props.power = BACKLIGHT_POWER_OFF;
	bd->props.state |= BL_CORE_FBBLANK;

whereas drm_panel_set_luminance() only updates props.brightness and calls
backlight_update_status() without clearing BL_CORE_FBBLANK or restoring
BACKLIGHT_POWER_ON.

In panel_bridge_atomic_disable() and panel_bridge_atomic_enable(), the bridge
checks self_refresh_active and returns early so that drm_panel_disable() and
drm_panel_enable() are skipped during self-refresh:

	new_crtc_state = drm_atomic_get_new_crtc_state(atomic_state, crtc);
	if (new_crtc_state && new_crtc_state->self_refresh_active)
		return;

However, disable_outputs() still runs drm_atomic_helper_commit_luminance_disables()
when entering self-refresh, which calls panel_bridge_connector_disable_luminance()
and turns the backlight off via backlight_disable(). When exiting
self-refresh, panel_bridge_atomic_enable() skips drm_panel_enable() (so
backlight_enable() is never called), and drm_panel_set_luminance() leaves
BACKLIGHT_POWER_OFF and BL_CORE_FBBLANK set, keeping the backlight blanked.

Also, on a normal modeset disable, drm_atomic_helper_commit_luminance_disables()
runs before drm_atomic_helper_commit_encoder_bridge_disable(), which disables
the backlight before panel_disabling followers run in drm_panel_disable(), and
on modeset enable drm_panel_enable() calls backlight_enable() with the old
props.brightness before drm_panel_set_luminance() updates it.

Could drm_panel_set_luminance() unblank the backlight (or use
backlight_enable()), and should panel luminance disable/enable respect
self-refresh and panel sequencing?

> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index 58113ea7d63a0..76d49d4485ff0 100644
> --- a/include/drm/drm_panel.h
> +++ b/include/drm/drm_panel.h

[ ... ]

> @@ -418,40 +423,6 @@ struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev,
>  struct drm_bridge *drmm_panel_bridge_add(struct drm_device *drm,
>  					 struct drm_panel *panel);
>  struct drm_connector *drm_panel_bridge_connector(struct drm_bridge *bridge);
> -#else
> -static inline bool drm_bridge_is_panel(const struct drm_bridge *bridge)
> -{
> -	return false;
> -}
> -
> -static inline int drm_panel_bridge_set_orientation(struct drm_connector *connector,
> -						   struct drm_bridge *bridge)
> -{
> -	return -EINVAL;
> -}
> -#endif
> -
> -#if defined(CONFIG_OF) && defined(CONFIG_DRM_PANEL_BRIDGE)
> -struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, struct device_node *node,
> -					  u32 port, u32 endpoint);
> -struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm, struct device_node *node,
> -				      u32 port, u32 endpoint);
> -#else
> -static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev,
> -							struct device_node *node,
> -							u32 port,
> -							u32 endpoint)
> -{
> -	return ERR_PTR(-ENODEV);
> -}
> -
> -static inline struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm,
> -						    struct device_node *node,
> -						    u32 port,
> -						    u32 endpoint)
> -{
> -	return ERR_PTR(-ENODEV);
> -}
>  #endif

[Severity: Low]
Was this removal of the fallback stubs and the devm_drm_of_get_bridge() /
drmm_of_get_bridge() declarations in include/drm/drm_panel.h an unintended
rebase artifact against commit aa8a8c7e4641 ("drm/bridge: panel: move all code
to drm_panel.c")?

It looks like the preceding patch in the series re-added these blocks to
include/drm/drm_bridge.h, and this hunk removes only the #else half and the
of_get_bridge block from include/drm/drm_panel.h while leaving the #ifdef
CONFIG_DRM_PANEL_BRIDGE declarations duplicated in both headers.

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

Re: [PATCH v10 10/15] drm/nouveau: add connector luminance support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] drm/nouveau: missing luminance commit calls in nv50_disp_atomic_commit_tail()
- [High] drm/nouveau: WARN_ON attaching luminance prop in nouveau_backlight_init()
- [High] drm/nouveau: atomic luminance property unusable on non-atomic NV40 KMS
- [Medium] drm/nouveau: stale brightness in nv50_sor_atomic_enable() on eDP modeset
- [Low] drm/nouveau: unmet BACKLIGHT_CLASS_DEVICE dependency for ACPI_VIDEO
--

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

drm/nouveau: add connector luminance support

This commit exposes Nouveau panel brightness through the connector LUMINANCE
property when CONFIG_DRM_LUMINANCE is enabled, reusing the NV40, NV50, and
DPCD backlight hardware paths while keeping the backlight class device
interface when CONFIG_DRM_LUMINANCE is disabled.

> diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
> index 3b5757aed9c84..2dcb7c5344f48 100644
> --- a/drivers/gpu/drm/nouveau/Kconfig
> +++ b/drivers/gpu/drm/nouveau/Kconfig
> @@ -17,13 +17,13 @@ config DRM_NOUVEAU
>  	select DRM_SCHED
>  	select I2C
>  	select I2C_ALGOBIT
> -	select BACKLIGHT_CLASS_DEVICE if DRM_NOUVEAU_BACKLIGHT
> +	select BACKLIGHT_CLASS_DEVICE if DRM_NOUVEAU_BACKLIGHT && !DRM_LUMINANCE
>  	select X86_PLATFORM_DEVICES if ACPI && X86
>  	select ACPI_WMI if ACPI && X86
>  	select MXM_WMI if ACPI && X86
>  	select POWER_SUPPLY
>  	# Similar to i915, we need to select ACPI_VIDEO and it's dependencies
> -	select BACKLIGHT_CLASS_DEVICE if ACPI && X86
> +	select BACKLIGHT_CLASS_DEVICE if ACPI && X86 && !DRM_LUMINANCE
>  	select INPUT if ACPI && X86
>  	select THERMAL if ACPI && X86
>  	select ACPI_VIDEO if ACPI && X86

[Severity: Low]
Does conditioning BACKLIGHT_CLASS_DEVICE on !DRM_LUMINANCE cause an unmet
Kconfig dependency for ACPI_VIDEO when CONFIG_DRM_LUMINANCE is enabled and
nothing else selects BACKLIGHT_CLASS_DEVICE?

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

config ACPI_VIDEO
	tristate "Video"
	depends on BACKLIGHT_CLASS_DEVICE

Because DRM_NOUVEAU still unconditionally selects ACPI_VIDEO if ACPI && X86
(and nouveau_backlight_init() still falls back to
nouveau_acpi_video_register_backlight() under fail_alloc), enabling
CONFIG_DRM_LUMINANCE with CONFIG_BACKLIGHT_CLASS_DEVICE disabled produces an
unmet direct dependency warning for ACPI_VIDEO and link failures in
drivers/acpi/acpi_video.o for backlight_device_register() and
backlight_device_unregister().

> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index e91130f93db67..79be7900a0794 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -1838,7 +1838,7 @@ nv50_sor_atomic_enable(struct drm_encoder *encoder, struct drm_atomic_commit *st
>  		backlight = nv_connector->backlight;
>  		if (backlight && backlight->uses_dpcd)
>  			drm_edp_backlight_enable(&nv_connector->aux, &backlight->edp_info,
> -						 backlight->dev->props.brightness);
> +						 backlight->brightness);

[Severity: Medium]
When CONFIG_DRM_LUMINANCE is disabled, won't passing backlight->brightness
here in nv50_sor_atomic_enable() reset an eDP panel's brightness to its
initial boot-time value on subsequent modesets, DPMS wakeups, or resume?

backlight->brightness is initialized once in nouveau_backlight_init(),
whereas user brightness updates through /sys/class/backlight call
nv50_edp_set_brightness() in drivers/gpu/drm/nouveau/nouveau_backlight.c:

static int
nv50_edp_set_brightness(struct backlight_device *bd)
{
	...
	if (crtc->state->active)
		ret = drm_edp_backlight_set_level(aux, &nv_bl->edp_info, bd->props.brightness);
	...
}

Because nv50_edp_set_brightness() never updates nv_bl->brightness from
bd->props.brightness, nv50_sor_atomic_enable() will pass the stale initial
backlight->brightness value to drm_edp_backlight_enable().

> diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c
> index da1ee24a2531d..fefcfa8c2a3a9 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c

[ ... ]

> @@ -317,10 +327,13 @@ nouveau_backlight_init(struct drm_connector *connector)
>  	bl = kzalloc_obj(*bl);
>  	if (!bl)
>  		return -ENOMEM;
> +	bl->id = -1;
> +	bl->encoder = nv_encoder;
>  
>  	switch (device->info.family) {
>  	case NV_DEVICE_INFO_V0_CURIE:
>  		ret = nv40_backlight_init(nv_encoder, &props, &ops);
> +		bl->uses_nv40 = true;
>  		break;

[Severity: High]
Can the LUMINANCE property work on NV40 (NV_DEVICE_INFO_V0_CURIE) or when
Nouveau is booted with atomic modesetting disabled (nouveau.atomic=0)?

NV40 uses the legacy dispnv04 display engine, where nouveau_drm_device_new()
selects driver_legacy_kms without DRIVER_ATOMIC and nv04_display_create() in
drivers/gpu/drm/nouveau/dispnv04/disp.c explicitly clears DRIVER_ATOMIC:

	/* Pre-nv50 doesn't support atomic, so don't expose the ioctls */
	dev->driver_features &= ~DRIVER_ATOMIC;

However, drm_connector_attach_luminance_property() creates the LUMINANCE
property with DRM_MODE_PROP_ATOMIC. On non-atomic drivers, atomic client
capability is rejected, DRM_MODE_PROP_ATOMIC properties are hidden from
drm_mode_object_get_properties(), and legacy property updates via
nouveau_connector_set_property() -> nouveau_conn_atomic_set_property() reject
the property with -EINVAL without calling nouveau_backlight_set_luminance().

Should nouveau_backlight_init() also check
drm_drv_uses_atomic_modeset(connector->dev) before using the LUMINANCE
property instead of registering the backlight class device?

>  	case NV_DEVICE_INFO_V0_TESLA:
>  	case NV_DEVICE_INFO_V0_FERMI:
> @@ -349,26 +362,39 @@ nouveau_backlight_init(struct drm_connector *connector)
>  		goto fail_alloc;
>  	}
>  
> -	if (!nouveau_get_backlight_name(backlight_name, bl)) {
> -		NV_ERROR(drm, "Failed to retrieve a unique name for the backlight interface\n");
> -		goto fail_alloc;
> -	}
> -
>  	props.type = BACKLIGHT_RAW;
> -	bl->dev = backlight_device_register(backlight_name, connector->kdev,
> -					    nv_encoder, ops, &props);
> -	if (IS_ERR(bl->dev)) {
> -		if (bl->id >= 0)
> -			ida_free(&bl_ida, bl->id);
> -		ret = PTR_ERR(bl->dev);
> -		goto fail_alloc;
> +	bl->brightness = props.brightness;
> +	bl->max_brightness = props.max_brightness;
> +	if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
> +		ret = drm_connector_attach_luminance_property(connector, 0, props.max_brightness,
> +							      props.brightness);
> +		if (ret)
> +			goto fail_alloc;

[Severity: High]
Won't calling drm_connector_attach_luminance_property() here in
nouveau_backlight_init() trigger a WARN_ON in __drm_mode_object_add() on
every internal panel registration?

nouveau_backlight_init() is called from nouveau_connector_late_register(),
which runs during drm_dev_register() -> drm_modeset_register_all() ->
drm_connector_register() after drm_dev_register() has already registered the
DRM minor device nodes and set dev->registered = true:

nouveau_connector_late_register()
  -> nouveau_backlight_init()
    -> drm_connector_attach_luminance_property()
      -> drm_property_create_range()
        -> drm_property_create()
          -> drm_mode_object_add()
            -> __drm_mode_object_add()

In drivers/gpu/drm/drm_mode_object.c:__drm_mode_object_add():

	WARN_ON(!dev->driver->load && dev->registered && !obj_free_cb);

Because DRM_MODE_OBJECT_PROPERTY objects have no obj_free_cb and
dev->registered is already true in late_register, this triggers the WARN_ON
and mutates dev->mode_config.property_list, connector->base.properties, and
connector->state->luminance without locking after the DRM device has been
exposed to userspace.

[ ... ]

> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
> index cc3caf8de64b0..56ee579ea7909 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> @@ -1225,6 +1225,8 @@ nouveau_connector_helper_funcs = {
>  	.mode_valid = nouveau_connector_mode_valid,
>  	.best_encoder = nouveau_connector_best_encoder,
>  	.atomic_check = nouveau_connector_atomic_check,
> +	.set_luminance = nouveau_backlight_set_luminance,
> +	.disable_luminance = nouveau_backlight_disable_luminance,
>  };

[Severity: High]
Are nouveau_backlight_set_luminance() and
nouveau_backlight_disable_luminance() ever invoked during atomic commits in
Nouveau?

In drivers/gpu/drm/drm_atomic_helper.c, the .disable_luminance and
.set_luminance connector helper callbacks are called from
drm_atomic_helper_commit_luminance_disables() and
drm_atomic_helper_commit_luminance_enables() (or via
drm_atomic_helper_commit_modeset_disables() and
drm_atomic_helper_commit_modeset_enables()).

However, Nouveau's NV50+ display driver in
drivers/gpu/drm/nouveau/dispnv50/disp.c implements its own atomic commit path
in nv50_disp_atomic_commit() -> nv50_disp_atomic_commit_tail(), which
disables and enables heads, planes, and outputs directly without calling
drm_atomic_helper_commit_modeset_disables(),
drm_atomic_helper_commit_modeset_enables(),
drm_atomic_helper_commit_luminance_disables(), or
drm_atomic_helper_commit_luminance_enables().

Without calling those helpers in nv50_disp_atomic_commit_tail(), won't
updates to the LUMINANCE connector property be silently ignored by the
hardware?

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

Re: [PATCH v10 11/15] drm/gud: add connector luminance support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] drm/gud: use-after-free on disconnect via gud_connector_disable_luminance()
- [High] drm/gud: infinite commit loop in gud_connector_set_luminance()
- [High] drm/gud: brightness clobbered to 0 in gud_connector_disable_luminance()
- [Medium] drm/gud: late property creation in gud_connector_backlight_register()
- [Low] drm/gud: undefined reference to backlight_device_unregister()
--

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

drm/gud: add connector luminance support

This commit exposes GUD display brightness through the connector LUMINANCE
property when CONFIG_DRM_LUMINANCE is enabled, storing brightness state on
struct gud_connector and reusing the asynchronous backlight workqueue path.

> diff --git a/drivers/gpu/drm/gud/Kconfig b/drivers/gpu/drm/gud/Kconfig
> index b4d2136942f0e..6c250d3d5e203 100644
> --- a/drivers/gpu/drm/gud/Kconfig
> +++ b/drivers/gpu/drm/gud/Kconfig
> @@ -7,7 +7,7 @@ config DRM_GUD
>  	select DRM_CLIENT_SELECTION
>  	select DRM_KMS_HELPER
>  	select DRM_GEM_SHMEM_HELPER
> -	select BACKLIGHT_CLASS_DEVICE
> +	select BACKLIGHT_CLASS_DEVICE if !DRM_LUMINANCE
>  	help
>  	  This is a DRM display driver for GUD USB Displays or display
>  	  adapters.
> diff --git a/drivers/gpu/drm/gud/gud_connector.c b/drivers/gpu/drm/gud/gud_connector.c
> index 8141c3a1e30ad..bd7688676e9e1 100644
> --- a/drivers/gpu/drm/gud/gud_connector.c
> +++ b/drivers/gpu/drm/gud/gud_connector.c

[ ... ]

> @@ -84,7 +86,7 @@ static void gud_connector_backlight_update_status_work(struct work_struct *work)
>  	}
>  
>  	/* Reuse tv.brightness to avoid having to subclass */
> -	connector_state->tv.brightness = gconn->backlight->props.brightness;
> +	connector_state->tv.brightness = READ_ONCE(gconn->brightness);
>  
>  	ret = drm_atomic_commit(state);
>  out:

[ ... ]

> @@ -132,6 +136,19 @@ static int gud_connector_backlight_register(struct gud_connector *gconn)
>  		.brightness = gconn->initial_brightness,
>  	};
>  
> +	gconn->brightness = props.brightness;
> +
> +	if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
> +		int ret;
> +
> +		ret = drm_connector_attach_luminance_property(connector, 0, props.max_brightness,
> +							      props.brightness);
> +		if (ret)
> +			return ret;
> +
> +		return 0;
> +	}

[Severity: Medium]
Won't attaching the LUMINANCE property in gud_connector_backlight_register()
trigger a WARN_ON in __drm_mode_object_add() on every probe when
CONFIG_DRM_LUMINANCE is enabled?

During gud_probe(), drm_dev_register() registers the DRM minor nodes and sets
dev->registered = true before calling:

  drm_modeset_register_all()
    -> drm_connector_register_all()
      -> drm_connector_register()
        -> gud_connector_late_register()
          -> gud_connector_backlight_register()
            -> drm_connector_attach_luminance_property()
              -> drm_property_create_range()
                -> drm_property_create()
                  -> drm_mode_object_add()
                    -> __drm_mode_object_add()

In drivers/gpu/drm/drm_mode_object.c:__drm_mode_object_add():

	WARN_ON(!dev->driver->load && dev->registered && !obj_free_cb);

Because gud_drm_driver.load is NULL, dev->registered is already true, and
obj_free_cb is NULL for DRM properties, this warning fires unconditionally and
mutates dev->mode_config.property_list, connector->base.properties, and
connector->state->luminance after userspace already has access to the DRM
device. Should drm_connector_attach_luminance_property() be called during
connector initialization in gud_connector_add_properties() before
drm_mode_config_reset() and drm_dev_register() instead?

> +
>  	name = kasprintf(GFP_KERNEL, "card%d-%s-backlight",
>  			 connector->dev->primary->index, connector->name);
>  	if (!name)
> @@ -314,12 +331,37 @@ static int gud_connector_atomic_check(struct drm_connector *connector,
>  	return 0;
>  }
>  
> +static void gud_connector_set_luminance(const struct drm_connector_state *conn_state);
> +static void gud_connector_disable_luminance(const struct drm_connector_state *conn_state);
> +
>  static const struct drm_connector_helper_funcs gud_connector_helper_funcs = {
>  	.detect_ctx = gud_connector_detect,
>  	.get_modes = gud_connector_get_modes,
>  	.atomic_check = gud_connector_atomic_check,
> +	.set_luminance = gud_connector_set_luminance,
> +	.disable_luminance = gud_connector_disable_luminance,
>  };
>  
> +static void gud_connector_set_luminance(const struct drm_connector_state *conn_state)
> +{
> +	struct drm_connector *connector = conn_state->connector;
> +	struct gud_connector *gconn = to_gud_connector(connector);
> +	unsigned int luminance = conn_state->luminance;
> +
> +	WRITE_ONCE(gconn->brightness, luminance);
> +
> +	/* The workqueue will apply it to the hardware */
> +	queue_work(system_long_wq, &gconn->backlight_work);
> +}

[Severity: High]
Does queueing gconn->backlight_work from gud_connector_set_luminance() cause
an infinite loop of atomic commits on system_long_wq whenever the connector is
active on a CRTC?

When gconn->backlight_work runs gud_connector_backlight_update_status_work(),
the workqueue core clears WORK_STRUCT_PENDING_BIT before invoking the work
callback. gud_connector_backlight_update_status_work() then adds the connector
to a new commit state via drm_atomic_get_connector_state(state, connector) and
calls drm_atomic_commit(state) synchronously:

  gud_connector_backlight_update_status_work()
    -> drm_atomic_commit()
      -> drm_atomic_helper_commit_tail_rpm()
        -> drm_atomic_helper_commit_modeset_enables()
          -> drm_atomic_helper_commit_luminance_enables()
            -> gud_connector_set_luminance()

In drivers/gpu/drm/drm_atomic_helper.c:drm_atomic_helper_commit_luminance_enables():

	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
		const struct drm_connector_helper_funcs *funcs;

		if (!connector->luminance_property || !new_conn_state->crtc ||
		    !new_conn_state->crtc->state->active)
			continue;

		funcs = connector->helper_private;
		if (funcs && funcs->set_luminance)
			funcs->set_luminance(new_conn_state);
	}

Because the connector is in the commit state, connector->luminance_property is
non-NULL, and new_conn_state->crtc->state->active is true,
drm_atomic_helper_commit_luminance_enables() unconditionally invokes
gud_connector_set_luminance() again during the work item's own
drm_atomic_commit() call. That re-queues gconn->backlight_work while the work
function is still executing, so system_long_wq immediately runs
gud_connector_backlight_update_status_work() again in an unbounded loop.

In addition, when old_state->tv.brightness != new_state->tv.brightness,
gud_connector_atomic_check() sets new_crtc_state->connectors_changed = true,
causing drm_atomic_helper_commit_modeset_disables() to also call
drm_atomic_helper_commit_luminance_disables() ->
gud_connector_disable_luminance() before
drm_atomic_helper_commit_luminance_enables() runs.

> +
> +static void gud_connector_disable_luminance(const struct drm_connector_state *conn_state)
> +{
> +	struct gud_connector *gconn = to_gud_connector(conn_state->connector);
> +
> +	WRITE_ONCE(gconn->brightness, 0);
> +	queue_work(system_long_wq, &gconn->backlight_work);
> +}

[Severity: Critical]
Can gud_connector_disable_luminance() re-queue gconn->backlight_work after
gud_connector_early_unregister() has already canceled it, leading to a
use-after-free of struct gud_connector on USB disconnect?

In drivers/gpu/drm/gud/gud_drv.c:gud_disconnect():

	drm_kms_helper_poll_fini(drm);
	drm_dev_unplug(drm);
	drm_atomic_helper_shutdown(drm);

First, drm_dev_unplug() runs:

  drm_dev_unplug()
    -> drm_dev_unregister()
      -> drm_modeset_unregister_all()
        -> drm_connector_unregister()
          -> gud_connector_early_unregister()
            -> cancel_work_sync(&gconn->backlight_work)

Next, drm_atomic_helper_shutdown() disables all active outputs:

  drm_atomic_helper_shutdown()
    -> drm_atomic_helper_disable_all()
      -> drm_atomic_commit()
        -> drm_atomic_helper_commit_modeset_disables()
          -> disable_outputs()
            -> drm_atomic_helper_commit_luminance_disables()
              -> gud_connector_disable_luminance()
                -> queue_work(system_long_wq, &gconn->backlight_work)

When gud_disconnect() finishes and devres releases the drm_device,
drm_mode_config_cleanup() calls gud_connector_destroy(), which cleans up the
connector (zeroing connector->dev) and calls kfree(gconn) without canceling
gconn->backlight_work. If gud_connector_backlight_update_status_work() is
still queued or running on system_long_wq, won't it access freed gconn memory
and dereference a NULL or freed connector->dev in drm_dev_enter()?

[Severity: High]
Also in gud_connector_disable_luminance(), doesn't setting gconn->brightness
to 0 and queueing gconn->backlight_work clobber connector_state->tv.brightness
and send an unpaired GUD_REQ_SET_STATE_CHECK over USB when a CRTC enters DPMS
off?

When a CRTC is set to DPMS off (new_crtc_state->enable == true,
new_crtc_state->active == false), drm_atomic_helper_commit_luminance_disables()
calls gud_connector_disable_luminance(), whereas
drm_atomic_helper_commit_luminance_enables() does not run because
new_crtc_state->active is false.

When gud_connector_backlight_update_status_work() subsequently runs on
system_long_wq, it sets connector_state->tv.brightness = 0 and commits:

1. gud_connector_atomic_check() sees old_state->tv.brightness !=
   new_state->tv.brightness and sets new_crtc_state->connectors_changed = true.
2. Because crtc_state->enable is still true, gud_plane_atomic_check()
   serializes connector_state->tv.brightness = 0 into GUD_REQ_SET_STATE_CHECK
   and sends it to the USB device.
3. Because new_crtc_state->active is false,
   drm_atomic_helper_commit_crtc_enable() skips gud_crtc_atomic_enable(),
   leaving GUD_REQ_SET_STATE_CHECK unpaired with GUD_REQ_SET_STATE_COMMIT.
4. When a later commit re-enables the CRTC (active = true),
   gud_plane_atomic_check() and gud_crtc_atomic_enable() run before
   drm_atomic_helper_commit_luminance_enables(), so the display is first
   enabled with tv.brightness == 0 before gud_connector_set_luminance() queues
   a second full modeset commit to restore conn_state->luminance.

> +
>  static int gud_connector_late_register(struct drm_connector *connector)
>  {
>  	struct gud_connector *gconn = to_gud_connector(connector);
> @@ -334,7 +376,9 @@ static void gud_connector_early_unregister(struct drm_connector *connector)
>  {
>  	struct gud_connector *gconn = to_gud_connector(connector);
>  
> -	backlight_device_unregister(gconn->backlight);
> +	if (gconn->backlight)
> +		backlight_device_unregister(gconn->backlight);
> +
>  	cancel_work_sync(&gconn->backlight_work);
>  }

[Severity: Low]
Will gud_connector_early_unregister() fail to link when CONFIG_DRM_LUMINANCE=y
and CONFIG_BACKLIGHT_CLASS_DEVICE is disabled (or when CONFIG_DRM_GUD=y and
CONFIG_BACKLIGHT_CLASS_DEVICE=m)?

The Kconfig change updates DRM_GUD to:

	select BACKLIGHT_CLASS_DEVICE if !DRM_LUMINANCE

However, include/linux/backlight.h declares backlight_device_unregister() as
an external function without an inline stub when CONFIG_BACKLIGHT_CLASS_DEVICE
is not enabled. Because if (gconn->backlight) is a runtime check on a struct
member rather than !IS_ENABLED(CONFIG_DRM_LUMINANCE), the compiler cannot
eliminate the call to backlight_device_unregister() at compile time.

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

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

[sashiko-bot]

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

Re: [PATCH v10 13/15] drm/gma500: add connector luminance support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/gma500: unreachable atomic luminance support in gma_backlight_init()
- [High] drm/gma500: gma_connector_disable_luminance() permanently disables backlight
- [Medium] drm/gma500: Kconfig unmet dependency and backlight link errors in backlight.c
--

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

drm/gma500: add connector luminance support

This commit exposes panel brightness on gma500 through the connector
luminance property when drm luminance support is enabled, routing updates
through existing platform backlight operations.

> diff --git a/drivers/gpu/drm/gma500/Kconfig b/drivers/gpu/drm/gma500/Kconfig
> index dc4c566388192..08871a8f6fa22 100644
> --- a/drivers/gpu/drm/gma500/Kconfig
> +++ b/drivers/gpu/drm/gma500/Kconfig
> @@ -9,7 +9,7 @@ config DRM_GMA500
>  	select I2C_ALGOBIT
>  	# GMA500 depends on ACPI_VIDEO when ACPI is enabled, just like i915
>  	select ACPI_VIDEO if ACPI
> -	select BACKLIGHT_CLASS_DEVICE if ACPI
> +	select BACKLIGHT_CLASS_DEVICE if ACPI && !DRM_LUMINANCE
>  	select INPUT if ACPI
>  	select ACPI_WMI if ACPI
>  	help
> diff --git a/drivers/gpu/drm/gma500/backlight.c b/drivers/gpu/drm/gma500/backlight.c
> index c8f1716a12d52..4bb39fba04539 100644
> --- a/drivers/gpu/drm/gma500/backlight.c
> +++ b/drivers/gpu/drm/gma500/backlight.c

[ ... ]

> @@ -73,10 +72,9 @@ static const struct backlight_ops gma_backlight_ops __maybe_unused = {
>  	.update_status  = gma_backlight_update_status,
>  };
>  
> -int gma_backlight_init(struct drm_device *dev)
> +int gma_backlight_init(struct drm_device *dev, struct drm_connector *connector)
>  {
>  	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> -	struct backlight_properties props __maybe_unused = {};
>  	int ret;
>  
>  	dev_priv->backlight_enabled = true;
> @@ -92,28 +90,43 @@ int gma_backlight_init(struct drm_device *dev)
>  		return 0;
>  	}
>  
> -#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
> -	props.brightness = dev_priv->backlight_level;
> -	props.max_brightness = PSB_MAX_BRIGHTNESS;
> -	props.type = BACKLIGHT_RAW;
> +	if (IS_ENABLED(CONFIG_DRM_LUMINANCE))
> +		return drm_connector_attach_luminance_property(connector, 0,
> +							 PSB_MAX_BRIGHTNESS,
> +							 dev_priv->backlight_level);

[Severity: High]
Does attaching the luminance property and returning early in
psb_driver_load() -> gma_backlight_init() leave panel brightness completely
uncontrollable when drm luminance is enabled?

In drivers/gpu/drm/gma500/psb_drv.c, the driver does not support atomic
modesetting:

    static const struct drm_driver driver = {
        .driver_features = DRIVER_MODESET | DRIVER_GEM,

However, drm_connector_attach_luminance_property() creates the property with
the atomic flag:

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

Because gma500 does not advertise atomic support, drm_setclientcap() rejects
atomic client capability requests and file_priv->atomic is always false, so
drm_mode_object_get_properties() hides the property from userspace.

In addition, legacy property updates via drm_mode_obj_set_property_ioctl() ->
set_property_legacy() dispatch to psb_intel_lvds_set_property() or
cdv_intel_lvds_set_property(), neither of which handles the luminance
property, and the atomic commit helpers that invoke
gma_connector_set_luminance() and gma_connector_disable_luminance() are never
called by this driver.

Since gma_backlight_init() also skips backlight_device_register() on this
path, is there any way for userspace to adjust panel brightness when drm
luminance is enabled?

> +
> +	{
> +		struct backlight_properties props = {};
>  
> -	dev_priv->backlight_device =
> -		backlight_device_register(dev_priv->ops->backlight_name,
> -					  dev->dev, dev,
> -					  &gma_backlight_ops, &props);
> -	if (IS_ERR(dev_priv->backlight_device))
> -		return PTR_ERR(dev_priv->backlight_device);
> -#endif
> +		props.brightness = dev_priv->backlight_level;
> +		props.max_brightness = PSB_MAX_BRIGHTNESS;
> +		props.type = BACKLIGHT_RAW;
> +
> +		dev_priv->backlight_device =
> +			backlight_device_register(dev_priv->ops->backlight_name,
> +						  dev->dev, dev,
> +						  &gma_backlight_ops, &props);
> +		if (IS_ERR(dev_priv->backlight_device))
> +			return PTR_ERR(dev_priv->backlight_device);
> +	}
>  
>  	return 0;
>  }
>  
> +void gma_connector_set_luminance(const struct drm_connector_state *conn_state)
> +{
> +	gma_backlight_set(conn_state->connector->dev, conn_state->luminance);
> +}
> +
> +void gma_connector_disable_luminance(const struct drm_connector_state *conn_state)
> +{
> +	gma_backlight_disable(conn_state->connector->dev);
> +}

[Severity: High]
Once gma_connector_disable_luminance() has been called, will subsequent calls
to gma_connector_set_luminance() fail to turn the backlight back on?

In drivers/gpu/drm/gma500/backlight.c, gma_connector_disable_luminance()
calls gma_backlight_disable(), which clears dev_priv->backlight_enabled:

    void gma_backlight_disable(struct drm_device *dev)
    {
        struct drm_psb_private *dev_priv = to_drm_psb_private(dev);

        dev_priv->backlight_enabled = false;
        dev_priv->ops->backlight_set(dev, 0);
    }

When gma_connector_set_luminance() is later called to restore or change the
brightness, it calls gma_backlight_set(), which only updates the hardware if
dev_priv->backlight_enabled is true:

    void gma_backlight_set(struct drm_device *dev, int v)
    {
        struct drm_psb_private *dev_priv = to_drm_psb_private(dev);

        dev_priv->backlight_level = v;
        if (dev_priv->backlight_enabled)
            dev_priv->ops->backlight_set(dev, v);
    }

Since nothing in gma_connector_set_luminance() or the encoder enable paths
sets dev_priv->backlight_enabled back to true (or calls
gma_backlight_enable()), won't the panel backlight remain permanently off
after the first disable?

> +
>  void gma_backlight_exit(struct drm_device *dev)
>  {
> -#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
>  	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>  
>  	if (dev_priv->backlight_device)
>  		backlight_device_unregister(dev_priv->backlight_device);
> -#endif
>  }

[Severity: Medium]
Can removing the preprocessor guards in gma_backlight_init() and
gma_backlight_exit() while dropping the backlight class selection in
drivers/gpu/drm/gma500/Kconfig cause unmet dependency warnings and link
failures?

First, in drivers/gpu/drm/gma500/Kconfig:

    select ACPI_VIDEO if ACPI
    select BACKLIGHT_CLASS_DEVICE if ACPI && !DRM_LUMINANCE

Because acpi video in drivers/acpi/Kconfig depends on the backlight class
device, enabling acpi and drm luminance while the backlight class device is
disabled selects acpi video without its direct dependency.

Second, include/linux/backlight.h does not provide inline stubs for
backlight_device_register() or backlight_device_unregister() when the
backlight class device is disabled.

In gma_backlight_exit(), the call to backlight_device_unregister() is only
guarded by a runtime check on dev_priv->backlight_device, so the compiler
still emits an undefined reference to backlight_device_unregister() when the
backlight class device is disabled.

Similarly, if acpi and drm luminance are both disabled (or if gma500 is
built-in while the backlight class device is built as a module),
gma_backlight_init() and gma_backlight_exit() will fail to link against both
backlight_device_register() and backlight_device_unregister().

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

Re: [PATCH v10 14/15] drm/bridge: add PS8622 connector luminance support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/bridge: ps8622: ps8622_send_config() checks bl instead of luminance ops
- [Medium] drm/bridge: ps8622: Kconfig and remove() break link without backlight class
- [Medium] drm/bridge: ps8622: ps8622_set_luminance() writes I2C when bridge disabled
--

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

drm/bridge: add PS8622 connector luminance support

This commit adds bridge luminance support to the Parade PS8622 driver when
CONFIG_DRM_LUMINANCE is enabled, while retaining the backlight class device
registration otherwise.

> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index c7da6b02d7b6b..d27e9b2485fd6 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -278,7 +278,7 @@ config DRM_PARADE_PS8622
>  	depends on OF
>  	select DRM_PANEL_BRIDGE
>  	select DRM_KMS_HELPER
> -	select BACKLIGHT_CLASS_DEVICE
> +	depends on DRM_LUMINANCE || BACKLIGHT_CLASS_DEVICE = y || !BACKLIGHT_CLASS_DEVICE
>  	help
>  	  Parade eDP-LVDS bridge chip driver.

[Severity: Medium]
Can this Kconfig dependency cause linker failures when
CONFIG_BACKLIGHT_CLASS_DEVICE is disabled or built as a module?

When CONFIG_DRM_LUMINANCE=n and CONFIG_BACKLIGHT_CLASS_DEVICE=n, the
!BACKLIGHT_CLASS_DEVICE term evaluates to true, allowing
CONFIG_DRM_PARADE_PS8622 to be enabled. In that configuration,
ps8622_probe() and ps8622_remove() call backlight_device_register() and
backlight_device_unregister(), which do not have inline stubs in
include/linux/backlight.h when CONFIG_BACKLIGHT_CLASS_DEVICE=n.

Furthermore, when CONFIG_DRM_LUMINANCE=y and CONFIG_BACKLIGHT_CLASS_DEVICE=n
(or CONFIG_DRM_PARADE_PS8622=y and CONFIG_BACKLIGHT_CLASS_DEVICE=m),
ps8622_remove() only guards backlight_device_unregister(ps8622->bl) with a
runtime check on ps8622->bl rather than a compile-time
!IS_ENABLED(CONFIG_DRM_LUMINANCE) check:

drivers/gpu/drm/bridge/parade-ps8622.c:ps8622_remove() {
	...
	if (ps8622->bl)
		backlight_device_unregister(ps8622->bl);
	drm_bridge_remove(&ps8622->bridge);
}

Because ps8622->bl is not a compile-time constant, the call to
backlight_device_unregister() is not eliminated and fails to link. Also,
when CONFIG_DRM_LUMINANCE=n and CONFIG_BACKLIGHT_CLASS_DEVICE=m, this
expression evaluates to n and prevents building DRM_PARADE_PS8622=m.

> diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c
> index 3a2ec482037df..229d9d7218c4e 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8622.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8622.c

[ ... ]

> @@ -276,7 +277,7 @@ static int ps8622_send_config(struct ps8622_bridge *ps8622)
>  
>  		/* FFh for 100% brightness, 0h for 0% brightness */
>  		err = ps8622_set(cl, 0x01, 0xa7,
> -				ps8622->bl->props.brightness);
> +				ps8622->brightness);
>  		if (err)
>  			goto error;
>  	} else {

[Severity: High]
When CONFIG_DRM_LUMINANCE is enabled, will ps8622_send_config() ever reach
this branch?

In ps8622_probe(), when use-external-pwm is not set and
CONFIG_DRM_LUMINANCE is enabled, the driver sets DRM_BRIDGE_OP_LUMINANCE on
ps8622->bridge.ops instead of calling backlight_device_register(), leaving
ps8622->bl as NULL.

However, ps8622_pre_enable() -> ps8622_send_config() still checks
ps8622->bl to choose between internal and external PWM:

drivers/gpu/drm/bridge/parade-ps8622.c:ps8622_send_config() {
	...
	if (ps8622->bl) {
		/* DPCD720, internal PWM */
		err = ps8622_set(cl, 0x01, 0xa5, 0xa0);
		if (err)
			goto error;

		/* FFh for 100% brightness, 0h for 0% brightness */
		err = ps8622_set(cl, 0x01, 0xa7,
				ps8622->brightness);
		if (err)
			goto error;
	} else {
		/* DPCD720, external PWM */
		err = ps8622_set(cl, 0x01, 0xa5, 0x80);
		if (err)
			goto error;
	}
	...
}

Because ps8622->bl is NULL, won't ps8622_send_config() always take the else
branch, configure register 0x01:0xa5 for external PWM (0x80) instead of
internal PWM (0xa0), and skip programming the initial brightness into
register 0x01:0xa7?

> @@ -324,6 +325,8 @@ static int ps8622_backlight_update(struct backlight_device *bl)
>  	struct ps8622_bridge *ps8622 = dev_get_drvdata(&bl->dev);
>  	int ret, brightness = backlight_get_brightness(bl);
>  
> +	ps8622->brightness = brightness;
> +
>  	if (!ps8622->enabled)
>  		return -EINVAL;
>  
> @@ -332,6 +335,15 @@ static int ps8622_backlight_update(struct backlight_device *bl)
>  	return ret;
>  }
>  
> +static void ps8622_set_luminance(struct drm_bridge *bridge,
> +				 unsigned int luminance)
> +{
> +	struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge);
> +
> +	ps8622->brightness = luminance;
> +	ps8622_set(ps8622->client, 0x01, 0xa7, luminance);
> +}

[Severity: Medium]
Should ps8622_set_luminance() check ps8622->enabled before calling
ps8622_set(), just like ps8622_backlight_update() does right above it?

If ps8622_send_config() fails during ps8622_pre_enable(), ps8622->enabled
remains false while drm_atomic_helper_commit_modeset_enables() still calls:

  drm_atomic_helper_commit_luminance_enables()
    -> drm_bridge_connector_set_luminance()
      -> ps8622_set_luminance()

Similarly, if a CRTC transitions from self-refresh (where
ps8622_post_disable() has already set ps8622->enabled = false, disabled
v12, and asserted gpio_slp and gpio_rst) to disabled, disable_outputs()
calls:

  drm_atomic_helper_commit_luminance_disables()
    -> drm_bridge_connector_disable_luminance()
      -> ps8622_set_luminance()

Wouldn't calling ps8622_set(ps8622->client, 0x01, 0xa7, luminance) when
!ps8622->enabled attempt an I2C write while the bridge is powered down and
in reset?

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

Re: [PATCH v10 15/15] drm/solomon: add SSD130x connector luminance support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] drm/solomon: vmlinux link failure when BACKLIGHT_CLASS_DEVICE=m
- [High] drm/solomon: contrast state desynchronization in ssd130x_set_luminance_value()
- [High] drm/solomon: unsynchronized command writes in ssd130x_set_luminance_value()
- [Medium] drm/solomon: broken Kconfig dependency for BACKLIGHT_CLASS_DEVICE and DRM_LUMINANCE
- [Low] drm/solomon: duplicated contrast dispatch in ssd130xfb_bl_ops
--

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

drm/solomon: add SSD130x connector luminance support

This commit exposes SSD130x display contrast through the DRM connector
LUMINANCE property when DRM_LUMINANCE is enabled, while guarding the legacy
backlight class registration with BACKLIGHT_CLASS_DEVICE and updating the
Kconfig dependencies.

> diff --git a/drivers/gpu/drm/solomon/Kconfig b/drivers/gpu/drm/solomon/Kconfig
> index 400a6cab3a671..76852246eb9c2 100644
> --- a/drivers/gpu/drm/solomon/Kconfig
> +++ b/drivers/gpu/drm/solomon/Kconfig
> @@ -1,7 +1,7 @@
>  config DRM_SSD130X
>  	tristate "DRM support for Solomon SSD13xx OLED displays"
>  	depends on DRM && MMU
> -	select BACKLIGHT_CLASS_DEVICE
> +	depends on DRM_LUMINANCE || BACKLIGHT_CLASS_DEVICE = y || !BACKLIGHT_CLASS_DEVICE
>  	select DRM_CLIENT_SELECTION
>  	select DRM_GEM_SHMEM_HELPER
>  	select DRM_KMS_HELPER

[Severity: High]
Can this Kconfig dependency cause a vmlinux link failure when
CONFIG_DRM_LUMINANCE=y, CONFIG_BACKLIGHT_CLASS_DEVICE=m, and
CONFIG_DRM_SSD130X=y?

Because DRM_LUMINANCE is a bool symbol and BACKLIGHT_CLASS_DEVICE is a
tristate symbol, when DRM_LUMINANCE=y and BACKLIGHT_CLASS_DEVICE=m, the
disjunction evaluates to max(y, n, m) = y, which allows DRM_SSD130X to be
built-in (y).

However, ssd130x_probe() in drivers/gpu/drm/solomon/ssd130x.c guards the call
to devm_backlight_device_register() with #if
IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE), which evaluates to 1 when
CONFIG_BACKLIGHT_CLASS_DEVICE=m. Built-in ssd130x.o will then call
devm_backlight_device_register() from the loadable module backlight.ko (which
has no inline stub in include/linux/backlight.h), failing to link vmlinux.

[Severity: Medium]
Does replacing select BACKLIGHT_CLASS_DEVICE with this dependency also cause a
regression when both CONFIG_DRM_LUMINANCE=n and
CONFIG_BACKLIGHT_CLASS_DEVICE=n?

When both options are disabled, !BACKLIGHT_CLASS_DEVICE evaluates to y,
allowing DRM_SSD130X to build with neither /sys/class/backlight nor the
connector LUMINANCE property enabled, leaving the display with no contrast
control interface at all.

In addition, on SSD132X_FAMILY panels, ssd132x_init() hardcodes
SSD13XX_CONTRAST to 0x80 and previously relied on backlight_enable() in
ssd132x_encoder_atomic_enable() to program ssd130x->contrast (127). When both
options are disabled, neither backlight_enable() nor
drm_atomic_helper_commit_luminance_enables() runs to apply ssd130x->contrast.

> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index ee43cf0da8792..aaf2b194273ba 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c

[ ... ]

> @@ -1980,8 +1985,44 @@ static int ssd130x_connector_get_modes(struct drm_connector *connector)
>  	return drm_connector_helper_get_modes_fixed(connector, &ssd130x->mode);
>  }
>  
> +static int ssd130x_set_luminance_value(struct ssd130x_device *ssd130x,
> +				       unsigned int luminance)
> +{
> +	int ret;
> +
> +	ssd130x->contrast = luminance;
> +
> +	switch (ssd130x->device_info->family_id) {
> +	case SSD133X_FAMILY:
> +		return ssd133x_set_contrast(ssd130x, luminance);
> +	case SSD135X_FAMILY:
> +		return ssd135x_set_contrast(ssd130x, luminance);
> +	default:
> +		ret = ssd130x_write_cmd(ssd130x, 1, SSD13XX_CONTRAST);
> +		if (ret < 0)
> +			return ret;
> +		return ssd130x_write_cmd(ssd130x, 1, luminance);
> +	}
> +}
> +
> +static void ssd130x_connector_set_luminance(const struct drm_connector_state *conn_state)
> +{
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(conn_state->connector->dev);
> +
> +	ssd130x_set_luminance_value(ssd130x, conn_state->luminance);
> +}
> +
> +static void ssd130x_connector_disable_luminance(const struct drm_connector_state *conn_state)
> +{
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(conn_state->connector->dev);
> +
> +	ssd130x_set_luminance_value(ssd130x, 0);
> +}

[Severity: High]
Can concurrent DRM atomic commits and sysfs backlight writes interleave the
multi-byte command writes in ssd130x_set_luminance_value()?

When both CONFIG_DRM_LUMINANCE and CONFIG_BACKLIGHT_CLASS_DEVICE are enabled,
sysfs backlight updates invoke ssd130x_update_bl(), ssd133x_update_bl(), or
ssd135x_update_bl() under bdev->update_lock, whereas DRM atomic commits invoke
ssd130x_connector_set_luminance() and ssd130x_connector_disable_luminance()
from drm_atomic_helper_commit_tail() without holding bdev->update_lock or any
shared driver lock.

In ssd130x_set_luminance_value(), ssd130x->contrast is updated without locking,
and on SSD130X_FAMILY and SSD132X_FAMILY the contrast update is sent as two
separate ssd130x_write_cmd() calls (and ssd133x_set_contrast() similarly issues
multiple separate regmap writes):

	ret = ssd130x_write_cmd(ssd130x, 1, SSD13XX_CONTRAST);
	if (ret < 0)
		return ret;
	return ssd130x_write_cmd(ssd130x, 1, luminance);

If a sysfs backlight write and a DRM atomic luminance update race between
those two regmap_write() calls, the second thread will send SSD13XX_CONTRAST
(0x81) before the first thread sends its parameter byte. Wouldn't the OLED
controller consume 0x81 as the contrast parameter for the first command, and
then interpret both threads' subsequent luminance parameter bytes as arbitrary
SSD13XX_COMMAND opcodes (such as 0xae SSD13XX_DISPLAY_OFF, 0xa0/0xa1 segment
remap, or 0xc0/0xc8 COM scan direction)?

[Severity: High]
Does exposing both ssd130x->bl_dev and the connector LUMINANCE property at the
same time desynchronize contrast state across atomic commits and display
disable/enable cycles?

In ssd130x_probe() and ssd130x_init_modeset(), the driver registers
ssd130x->bl_dev whenever IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) is true and
also attaches the LUMINANCE property whenever IS_ENABLED(CONFIG_DRM_LUMINANCE)
is true, unlike drivers that only register a legacy backlight device when
DRM_LUMINANCE is not used:

1. ssd130x_connector_set_luminance() updates ssd130x->contrast via
ssd130x_set_luminance_value() without updating
ssd130x->bl_dev->props.brightness, while ssd130x_update_bl(),
ssd133x_update_bl(), and ssd135x_update_bl() update ssd130x->contrast from
bl_dev->props.brightness without updating connector->state->luminance. If
brightness is changed via /sys/class/backlight, any subsequent atomic commit
including the active connector runs
drm_atomic_helper_commit_luminance_enables() and overwrites the hardware
contrast back to conn_state->luminance.

2. When disabling the display, drm_atomic_helper_commit_luminance_disables()
calls ssd130x_connector_disable_luminance() ->
ssd130x_set_luminance_value(ssd130x, 0), which clobbers ssd130x->contrast to
0, and ssd130x_encoder_atomic_disable() then calls
backlight_disable(ssd130x->bl_dev), writing contrast 0 a second time.

3. When re-enabling the display, ssd130x_encoder_atomic_enable() (and its
ssd132x, ssd133x, and ssd135x counterparts) runs before
drm_atomic_helper_commit_luminance_enables(): ssd130x_init(), ssd133x_init(),
and ssd135x_init() first initialize the panel with the clobbered
ssd130x->contrast (0) prior to SSD13XX_DISPLAY_ON; next,
backlight_enable(ssd130x->bl_dev) programs the stale bl_dev->props.brightness
while the display is on; and finally
drm_atomic_helper_commit_luminance_enables() calls
ssd130x_connector_set_luminance() to program conn_state->luminance.

4. If ssd130x_power_on() or ssd130x_init() fails in
ssd130x_encoder_atomic_enable() (or its ssd132x, ssd133x, or ssd135x variants)
and powers off vcc_reg, drm_atomic_helper_commit_modeset_enables() still calls
drm_atomic_helper_commit_luminance_enables() ->
ssd130x_connector_set_luminance(), issuing contrast commands over I2C/SPI to
an unpowered controller.

[ ... ]

> @@ -2015,23 +2056,13 @@ static const struct drm_driver ssd130x_drm_driver = {
>  	.fops			= &ssd130x_fops,
>  };
>  
> +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>  static int ssd130x_update_bl(struct backlight_device *bdev)
>  {
>  	struct ssd130x_device *ssd130x = bl_get_data(bdev);
> -	int brightness = backlight_get_brightness(bdev);
> -	int ret;
> -
> -	ssd130x->contrast = brightness;
> -
> -	ret = ssd130x_write_cmd(ssd130x, 1, SSD13XX_CONTRAST);
> -	if (ret < 0)
> -		return ret;
>  
> -	ret = ssd130x_write_cmd(ssd130x, 1, ssd130x->contrast);
> -	if (ret < 0)
> -		return ret;
> -
> -	return 0;
> +	return ssd130x_set_luminance_value(ssd130x,
> +					  backlight_get_brightness(bdev));
>  }
>  
>  static int ssd133x_update_bl(struct backlight_device *bdev)
> @@ -2066,6 +2097,7 @@ static const struct backlight_ops ssd130xfb_bl_ops[] = {
>  		.update_status	= ssd135x_update_bl,
>  	},
>  };
> +#endif

[Severity: Low]
This isn't a bug, but since ssd130x_update_bl() now delegates to
ssd130x_set_luminance_value(), which already switches on
ssd130x->device_info->family_id and dispatches to ssd133x_set_contrast() and
ssd135x_set_contrast(), could ssd133x_update_bl(), ssd135x_update_bl(), and the
per-family ssd130xfb_bl_ops[] array be dropped in favor of a single
backlight_ops struct using ssd130x_update_bl()?

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