Re: [PATCH v4 4/4] drm/amd/display: Enable HDMI ALLM for Gaming-VRR

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Overloading `vsp_infopacket` with the HDMI Forum VSIF (HF-VSIF) overwrites the standard HDMI 1.4 VSIF, silently dropping the HDMI VIC required for certain modes.
--

--- Patch [4]: [PATCH v4 4/4] drm/amd/display: Enable HDMI ALLM for Gaming-VRR ---
commit d9e1d834d57e6ab1f6bb6abbfa9007cc7a9851fc
Author: Fangzhi Zuo <jerry.zuo@amd.com>

drm/amd/display: Enable HDMI ALLM for Gaming-VRR

This commit enables Auto Low-Latency Mode (ALLM) by setting ALLM_Mode=1
in the HF-VSIF when Gaming-VRR is active. It transmits the updated HF-VSIF
as a stream update when the sink advertises ALLM support.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4029,9 +4029,12 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_commit *state,
>  		}
>  
>  		if (acrtc_state->stream) {
> -			if (acrtc_state->freesync_vrr_info_changed)
> +			if (acrtc_state->freesync_vrr_info_changed) {
>  				bundle->stream_update.vrr_infopacket =
>  					&acrtc_state->stream->vrr_infopacket;
> +				bundle->stream_update.vsp_infopacket =
> +					&acrtc_state->stream->vsp_infopacket;

[Severity: High]
Does updating the stream using vsp_infopacket drop the standard HDMI Vendor
Specific InfoFrame (VSIF)?

The dc_stream_state structure has a separate hfvsif_infopacket member that
might be intended for the HF-VSIF payload instead of overloading the
vsp_infopacket.

> +			}
>  		}
>  	}
>  
> 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

[ ... ]

> +	if (new_stream->signal == SIGNAL_TYPE_HDMI_TYPE_A ||
> +	    new_stream->signal == SIGNAL_TYPE_HDMI_FRL) {
> +		struct dc_info_packet vsp_infopacket = {0};
> +		bool sink_allm = aconn && aconn->base.display_info.hdmi.allm;
> +		bool allm = sink_allm &&
> +			(vrr_params.state == VRR_STATE_ACTIVE_VARIABLE ||
> +			 vrr_params.state == VRR_STATE_ACTIVE_FIXED);
> +		bool allm_changed;
> +
> +		mod_build_hf_vsif_infopacket(new_stream, &vsp_infopacket, allm, allm);
> +
> +		allm_changed = memcmp(&new_stream->vsp_infopacket, &vsp_infopacket,
> +				      sizeof(vsp_infopacket)) != 0;
> +		new_crtc_state->freesync_vrr_info_changed |= allm_changed;
> +		new_stream->vsp_infopacket = vsp_infopacket;

[Severity: High]
Does storing the HF-VSIF inside the vsp_infopacket slot overwrite the
standard HDMI 1.4 VSIF?

When ALLM is active, this assignment appears to silently overwrite the VSIF
designated for HDMI 1.4, which can result in the display losing format
information like the hdmi_vic required for certain modes.

Looking at resource_build_info_frame() in dc_resource.c, the hardware
explicitly supports transmitting both the standard VSIF and the HF-VSIF
simultaneously via separate slots:

dc/core/dc_resource.c:resource_build_info_frame() {
    ...
    set_vendor_info_packet(&info->vendor, pipe_ctx->stream);
    set_hfvs_info_packet(&info->hfvsif, pipe_ctx->stream);
    ...
}

Should this code instead populate the hfvsif_infopacket member of new_stream?

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