Re: [PATCH 2/3] drm/amd/display: Honor Broadcast RGB for BT.2020 RGB output

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] NULL pointer dereference in `amdgpu_dm_atomic_commit_tail` when changing the `Broadcast RGB` connector property.
--

commit 25a82016e5e929302d7c5fe7edf379f4e6cec7a0
Author: Satyajit Roy <sroy14@alum.utk.edu>

drm/amd/display: Honor Broadcast RGB for BT.2020 RGB output

This patch updates amdgpu_dm_get_output_color_space() to apply the Broadcast
RGB connector property for BT.2020 RGB output. It explicitly selects limited
range when requested, fixing an issue where it always defaulted to full range.

> 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 8a35e4df13cbf..f2f6a2a0db99e 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
> @@ -758,10 +758,14 @@ amdgpu_dm_get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
>  		break;
>  	case DRM_MODE_COLORIMETRY_BT2020_RGB:
>  	case DRM_MODE_COLORIMETRY_BT2020_YCC:
> -		if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB)
> -			color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
> -		else
> +		if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB) {
> +			if (connector_state->hdmi.broadcast_rgb == DRM_HDMI_BROADCAST_RGB_LIMITED)
> +				color_space = COLOR_SPACE_2020_RGB_LIMITEDRANGE;
> +			else
> +				color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
> +		} else {
>  			color_space = COLOR_SPACE_2020_YCBCR_LIMITED;
> +		}
>  		break;
>  	case DRM_MODE_COLORIMETRY_DEFAULT: /* ITU601 */

[Severity: Critical]
This is a pre-existing issue, but does changing the Broadcast RGB property
crash the kernel via a NULL pointer dereference?

When userspace changes the Broadcast RGB property on a connector, DRM core
creates an atomic commit containing only the updated connector state. The AMD
driver's amdgpu_dm_atomic_check() does not add the associated CRTC to the
atomic state for these broadcast_rgb changes.

This leads to a crash in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
within amdgpu_dm_atomic_commit_tail(), where drm_atomic_get_new_crtc_state()
returns NULL:

	if (acrtc) {
		new_crtc_state = drm_atomic_get_new_crtc_state(state, &acrtc->base);
		old_crtc_state = drm_atomic_get_old_crtc_state(state, &acrtc->base);
	}

	/* Skip any modesets/resets */
	if (!acrtc || drm_atomic_crtc_needs_modeset(new_crtc_state))
		continue;

Since new_crtc_state is NULL, drm_atomic_crtc_needs_modeset() unconditionally
dereferences it and crashes.

Even if this did not crash (for example on a disabled CRTC where modeset
might evaluate to false), execution proceeds to the fast-update path in
the same function:

	if ((new_con_state->hdmi.broadcast_rgb != old_con_state->hdmi.broadcast_rgb) &&
		(dm_old_crtc_state->stream->output_color_space !=
			amdgpu_dm_get_output_color_space(&dm_new_crtc_state->stream->timing, new_con_state)))

For a disabled CRTC, stream is NULL, which causes dm_new_crtc_state->stream
and dm_old_crtc_state->stream to trigger a second unavoidable NULL pointer
dereference.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830035120.937992-1-edu042sjroy@proton.me?part=2