Re: [PATCH v11 09/12] drm/nouveau: add connector luminance support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/nouveau: unclamped brightness in nv50_backlight_init() fails probe
--

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

drm/nouveau: add connector luminance support

This commit exposes Nouveau panel brightness through the atomic connector
LUMINANCE property when DRM_LUMINANCE is enabled and atomic modesetting is
active, while retaining the backlight class device fallback otherwise.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c
> index da1ee24a2531d..2366901581077 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
> @@ -234,12 +234,15 @@ nv50_backlight_init(struct nouveau_backlight *bl,
>  		    const struct backlight_ops **ops)
>  {
>  	struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev);
> +	int brightness;
> +
> +	brightness = nvif_outp_bl_get(&nv_encoder->outp);
>  
>  	/*
>  	 * Note when this runs the connectors have not been probed yet,
>  	 * so nv_conn->base.status is not set yet.
>  	 */
> -	if (nvif_outp_bl_get(&nv_encoder->outp) < 0 ||
> +	if (brightness < 0 ||
>  	    drm_helper_probe_detect(&nv_conn->base, NULL, false) != connector_status_connected)
>  		return -ENODEV;
>  
> @@ -284,11 +287,25 @@ nv50_backlight_init(struct nouveau_backlight *bl,
>  
>  	*ops = &nv50_bl_ops;
>  	props->max_brightness = 100;
> +	props->brightness = brightness;
>  	return 0;
>  }

[ ... ]

> @@ -349,26 +368,36 @@ 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;
> +	if (nouveau_backlight_use_luminance(connector->dev)) {
> +		ret = drm_connector_attach_luminance_property(connector, 0, props.max_brightness,
> +							      props.brightness);
> +		if (ret)
> +			goto fail_alloc;

[Severity: High]
Can an unclamped initial brightness in nv50_backlight_init() cause
drm_connector_attach_luminance_property() to return -EINVAL and abort the
entire DRM device probe in nouveau_drm_device_init()?

In the nouveau_drm_device_init() -> nouveau_backlight_luminance_init() ->
nouveau_backlight_create() -> nv50_backlight_init() path, non-DPCD SOR
backlights set props->max_brightness = 100 and assign props->brightness =
brightness from nvif_outp_bl_get(&nv_encoder->outp) without clamping.

On NV50/G84/G94/MCP77, nv50_sor_bl_get() reads the 11-bit duty cycle field
and divides by a hardcoded div of 1025 without clamping (unlike
gt215_sor_bl_get()):

drivers/gpu/drm/nouveau/nvkm/engine/disp/nv50.c:nv50_sor_bl_get() {
	u32 div = 1025;
	u32 val = nvkm_rd32(device, 0x61c084 + soff);
	val &= 0x000007ff;
	return ((val * 100) + (div / 2)) / div;
}

When the 11-bit register value is between 1031 and 2047, nv50_sor_bl_get()
returns 101..200. Similarly, for eDP DPCD panels already in
DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD mode, drm_edp_backlight_init() ->
drm_edp_backlight_probe_state() reads the raw 8-bit or 16-bit
DP_EDP_BACKLIGHT_BRIGHTNESS_MSB/LSB registers into current_level without
clamping to bl->edp_info.max.

Previously, nv50_backlight_init() left props->brightness as 0 for non-DPCD
panels, and backlight_device_register() did not reject brightness >
max_brightness. With this change, drm_connector_attach_luminance_property()
checks:

	if (drm_WARN_ON(connector->dev, min > max || value < min || value > max))
		return -EINVAL;

Because nouveau_drm_device_init() treats any non-zero return from
nouveau_backlight_luminance_init() as fatal (jumping to fail_backlight),
should props.brightness be clamped to props.max_brightness before calling
drm_connector_attach_luminance_property()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-superm1-backlight-property-v11-0-00cce7b2ba22@kernel.org?part=9