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

[sashiko-bot@kernel.org]

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