Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The logic to send a zeroed DRM infoframe fails to enforce the CTA-861-H 2-second duration, risks premature termination, and causes unintended full modesets on subsequent atomic commits. --
commit 1fa58c4612bfbba739489ee34fd7b6bc9eeeb05e Author: Xiao Lu <[email protected]> drm/i915/display: send null DRM infoframe when ending HDR transmission This commit updates the driver to send a null DRM infoframe (EOTF=0) when transitioning from HDR to SDR to comply with CTA-861-H. This prevents DP-to-HDMI converters from latching previous HDR metadata on SDR content. > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > b/drivers/gpu/drm/i915/display/intel_dp.c > index 113d767e62e9c..d5ed96ba7e7ba 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c [ ... ] > @@ -3231,14 +3231,29 @@ intel_dp_in_hdr_mode(const struct drm_connector_state > *conn_state) > static void > intel_dp_compute_hdr_metadata_infoframe_sdp(struct intel_dp *intel_dp, > struct intel_crtc_state *crtc_state, > - const struct drm_connector_state > *conn_state) > + const struct drm_connector_state > *conn_state, > + const struct drm_connector_state > *old_conn_state) > { > struct intel_display *display = to_intel_display(intel_dp); > int ret; > struct hdmi_drm_infoframe *drm_infoframe = > &crtc_state->infoframes.drm.drm; > > - if (!conn_state->hdr_output_metadata) > + if (!conn_state->hdr_output_metadata) { > + /* > + * CTA-861-H requires ending HDR metadata transmission by > + * sending a DRM infoframe with EOTF=0 and all fields zero > + * for at least 2 seconds, rather than abruptly stopping. > + * Abruptly stopping causes DP-to-HDMI converters to latch > + * the previous HDR metadata and forward it to the HDMI sink, > + * resulting in color errors on SDR content. > + */ > + if (old_conn_state && old_conn_state->hdr_output_metadata) { > + hdmi_drm_infoframe_init(drm_infoframe); > + crtc_state->infoframes.enable |= > + > intel_hdmi_infoframe_enable(HDMI_PACKET_TYPE_GAMUT_METADATA); > + } [Severity: High] Does this logic correctly enforce the 2-second transmission requirement? Since this ties the lifecycle of the zeroed DRM infoframe to atomic commit boundaries rather than time, it appears the transmission could be terminated prematurely if userspace performs a subsequent atomic commit (such as a cursor update) within 2 seconds. In that case, both old_conn_state->hdr and conn_state->hdr would be false, so the DRM bit in crtc_state->infoframes.enable would be computed as cleared. Additionally, could this cause an unintended full modeset? When that subsequent commit occurs, intel_pipe_config_compare() will check the active hardware state (which still has the DRM bit set) against the new state (which clears it). The mismatch might force a full screen blanking for a simple SDR cursor update. > return; > + } > > ret = drm_hdmi_infoframe_set_hdr_metadata(drm_infoframe, conn_state); [ ... ] > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c > b/drivers/gpu/drm/i915/display/intel_hdmi.c > index 8a019d3574df2..9fb9be302d35b 100644 > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c [ ... ] > @@ -856,8 +856,25 @@ intel_hdmi_compute_drm_infoframe(struct intel_encoder > *encoder, > if (!crtc_state->has_infoframe) > return true; > > - if (!conn_state->hdr_output_metadata) > + if (!conn_state->hdr_output_metadata) { > + const struct drm_connector_state *old_conn_state = > + drm_atomic_get_old_connector_state(conn_state->state, > + > conn_state->connector); > + /* > + * CTA-861-H requires ending HDR metadata transmission by > + * sending a DRM infoframe with EOTF=0 and all fields zero > + * for at least 2 seconds, rather than abruptly stopping. > + * Abruptly stopping causes DP-to-HDMI converters to latch > + * the previous HDR metadata and forward it to the HDMI sink, > + * resulting in color errors on SDR content. > + */ > + if (old_conn_state && old_conn_state->hdr_output_metadata) { > + hdmi_drm_infoframe_init(frame); > + crtc_state->infoframes.enable |= > + > intel_hdmi_infoframe_enable(HDMI_INFOFRAME_TYPE_DRM); > + } [Severity: High] The native HDMI path in intel_hdmi_compute_drm_infoframe() appears to share the same lifecycle vulnerability as the DP path. Without a timer or workqueue to manage the 2-second timeout, do we risk prematurely aborting the infoframe and triggering unintended modesets on subsequent SDR commits? > return true; > + } > > crtc_state->infoframes.enable |= > intel_hdmi_infoframe_enable(HDMI_INFOFRAME_TYPE_DRM); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
