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 <[email protected]> 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/[email protected]?part=4
