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

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Dereferencing deprecated `new_stream->sink` pointer risks a NULL pointer dereference.
- [High] VTEM packet overwrites the FreeSync SPD infopacket and is misrouted to the hardware's SPD slot.
- [Medium] Unconditional VTEM transmission to all HDMI FRL displays, including non-VRR sinks.
- [Medium] 32-bit integer overflow when calculating `fieldRateInHz` for VTEM packets in ultra-high bandwidth modes.
- [Low] Unused parameter `signal` in `build_infopacket_header_vtem()`.
--

--- Patch [1]: [PATCH v2 1/4] drm/amd/display: Add 2.1 FreeSync support for AMD VSDB EDID Block ---
commit b0ebd1a49b36a5e82edcd0396a4c4d22fe23f9d3
Author: Fangzhi Zuo <[email protected]>

drm/amd/display: Add 2.1 FreeSync support for AMD VSDB EDID Block

This commit adds 2.1 FreeSync support for AMD VSDB EDID block over HDMI FRL. 
It updates the capability parsing to accept HDMI FRL signals and builds the 
required VTEM info packet. The VTEM data set length is dynamically set to 0 
when no VTEM feature is enabled to comply with HDMI GCTS.

> 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
> index 7e484adb4120f..1fa078cdba527 100644
> --- 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]
Can this dereference a NULL pointer? The sink pointer inside
dc_stream_state is officially deprecated and could be NULL in headless or
writeback scenarios. Would it be safer to use new_stream->signal here?

[Severity: Medium]
Will this transmit VTEM packets even to sinks that do not support VRR or FVA?
There doesn't appear to be a check for vrr_params.send_info_frame, which
could result in unconditional VTEM transmission to all HDMI FRL displays,
potentially violating the HDMI 2.1 specifications.

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

[Severity: High]
Does this overwrite the previously built AMD Vendor Specific InfoFrame
(VSIF)? By passing &vrr_infopacket, the newly created VTEM packet is stored
in the same local variable.

Additionally, since dc_resource.c writes stream->vrr_infopacket exclusively
to the hardware's SPD slot, will this cause the VTEM packet to be incorrectly
transmitted through the SPD slot instead of the VTEM slot?

> +
>  	new_crtc_state->freesync_vrr_info_changed |=
>  		(memcmp(&new_crtc_state->vrr_infopacket,
>  			&vrr_infopacket,
> diff --git a/drivers/gpu/drm/amd/display/modules/info_packet/info_packet.c b/drivers/gpu/drm/amd/display/modules/info_packet/info_packet.c
> index f5ac4bf32a784..e956c707ac507 100644
> --- a/drivers/gpu/drm/amd/display/modules/info_packet/info_packet.c
> +++ b/drivers/gpu/drm/amd/display/modules/info_packet/info_packet.c

[ ... ]

> @@ -644,6 +659,100 @@ void mod_build_hf_vsif_infopacket(const struct dc_stream_state *stream,
>  		info_packet->valid = true;
>  }
>  
> +static void build_vtem_infopacket_data(const struct dc_stream_state *stream,
> +		const struct mod_vrr_params *vrr, int fva_factor,
> +		struct dc_info_packet *infopacket)
> +{
> +	unsigned int fieldRateInHz;
> +
> +	/* FVA Factor setting */
> +	setFieldWithMask(&infopacket->sb[VTEM_MD0], MASK_VTEM_MD0__FVA_FACTOR_M1,
> +			(fva_factor > 0)?(fva_factor-1):0);

[ ... ]

> +		fieldRateInHz = stream->timing.pix_clk_100hz * 100;
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Can this multiplication overflow for ultra-high bandwidth modes? For displays
like 8K@120Hz, stream->timing.pix_clk_100hz * 100 could exceed the 32-bit
unsigned integer limit before the subsequent division. Should this be cast
to a 64-bit integer?

> +		fieldRateInHz /= stream->timing.h_total;
> +		fieldRateInHz = (fieldRateInHz + stream->timing.v_total / 2)
> +						/ stream->timing.v_total;
> +
> +	}
> +

[ ... ]

> +static void build_infopacket_header_vtem(enum signal_type signal,
                                            ^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Low]
Is the signal parameter needed here? It doesn't appear to be evaluated or
used anywhere within the body of this function.

> +		struct dc_info_packet *infopacket)
> +{
> +	// HEADER
> +
> +	// HB0, HB1, HB2 indicates PacketType VTEMPacket
> +	infopacket->hb0 = 0x7F;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1