Thanks for the review. The first NULL-pointer path does not appear reachable. When a connector is attached to a CRTC, drm_atomic_get_connector_state() also adds that CRTC's state to the atomic commit. amdgpu_dm_atomic_check() additionally calls drm_atomic_get_crtc_state() for every changed connector with a CRTC. Consequently, drm_atomic_get_new_crtc_state() should be non-NULL here. For an unattached connector, acrtc is NULL and the condition short-circuits before accessing the CRTC state.
The stream-NULL case for a DPMS-inactive but still attached CRTC may be a valid pre-existing issue. It is not introduced or worsened by this patch: the existing condition dereferences dm_old_crtc_state->stream before calling amdgpu_dm_get_output_color_space(), which is the function changed here. Broadcast RGB property updates already enter this path for the existing color spaces. I will investigate the inactive-stream case separately and submit an independent fix if needed. Thanks, Satyajit On Sunday, August 30th, 2026 at 12:06 AM, [email protected] <[email protected]> wrote: > 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 <[email protected]> > > 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/[email protected]?part=2 >
