Re: [PATCH v4 1/4] drm/amd/display: Add 2.1 FreeSync support for AMD VSDB EDID Block

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The VTEM info packet incorrectly overwrites the AMD FreeSync (SPD) info packet in the local `vrr_infopacket` variable, causing the FreeSync metadata to be lost and the VTEM packet to be programmed into the wrong hardware slot.
- [High] Unsafe dereference of the deprecated `new_stream->sink` pointer without a NULL check inside a spinlock, risking a kernel panic and system deadlock.
--

--- Patch [1]: [PATCH v4 1/4] drm/amd/display: Add 2.1 FreeSync support for AMD VSDB EDID Block ---
commit ac51682620be921a897de496468475a6c9820661
Author: Fangzhi Zuo <Jerry.Zuo@amd.com>
Subject: drm/amd/display: Add 2.1 FreeSync support for AMD VSDB EDID Block

This commit adds HDMI 2.1 FreeSync support by parsing the AMD VSDB for HDMI
FRL signals. It updates the FreeSync capabilities and introduces a function to
build and send the VTEM info packet during the freesync state update.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_freesync.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_freesync.c
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_freesync.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_freesync.c
[ ... ]
> @@ -229,6 +230,9 @@ void amdgpu_dm_update_freesync_state_on_stream(
>  		&vrr_infopacket,
>  		pack_sdp_v1_3);
>  
> +	if (new_stream->sink->sink_signal == SIGNAL_TYPE_HDMI_FRL)

[Severity: High]
Is it possible for new_stream->sink to be NULL here?

Existing KUnit tests like dm_test_update_freesync_state_default_packet()
allocate dummy streams with a NULL sink. Phantom streams or writeback setups
can also have a NULL sink.

Since this check occurs inside the event_lock spinlock, would a NULL sink
cause a kernel panic and a system deadlock? Would it be safer to verify
new_stream->sink is not NULL before dereferencing it, or use the already
available new_stream->signal instead?

> +		mod_build_infopacket_vtem(new_stream, &vrr_params, 0, &vrr_infopacket);

[Severity: High]
Does this overwrite the existing AMD FreeSync metadata stored in
vrr_infopacket?

Looking at the newly added mod_build_infopacket_vtem(), it calls memset()
on the passed infopacket:

drivers/gpu/drm/amd/display/modules/info_packet/info_packet.c:mod_build_infopacket_vtem() {
	memset(infopacket, 0, sizeof(struct dc_info_packet));
	...
}

Since vrr_infopacket was just populated by mod_freesync_build_vrr_infopacket()
immediately before this block, the memset() appears to erase the SPD packet. 

Furthermore, this assigns the VTEM packet to the vrr_infopacket variable,
which is later routed to the SPD hardware slot in set_spd_info_packet().
Should the VTEM packet be stored in new_stream->vtem_infopacket instead to
preserve the FreeSync packet and ensure VTEM is sent in the correct slot?

> +
>  	new_crtc_state->freesync_vrr_info_changed |=
>  		(memcmp(&new_crtc_state->vrr_infopacket,
>  			&vrr_infopacket,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814164344.3098143-1-jerry.zuo@amd.com?part=1