Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The implementation fails to enforce the CTA-861-H 2-second minimum duration for zero-filled DRM infoframes and leaves them enabled indefinitely during normal SDR operation. --
commit 36a317ecec2b669b256a0c4c50d72b83f1882ac8 Author: Xiao Lu <[email protected]> Subject: drm/i915/display: send null DRM infoframe when ending HDR transmission This commit aims to fix color errors on DP-to-HDMI converters during the HDR-to-SDR transition. It implements the CTA-861-H requirement to send a null DRM infoframe for at least 2 seconds before stopping the transmission entirely. > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > b/drivers/gpu/drm/i915/display/intel_dp.c > index 6b60e0a1c8eef..7680594ae6702 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: Medium] How is the 2-second minimum duration enforced here? The infoframe transmission is tied to the transient atomic state transition where old_conn_state->hdr_output_metadata is true. Since the crtc_state persists until the next commit, wouldn't the null infoframe transmit indefinitely during normal SDR operation, rather than stopping after 2 seconds? Additionally, if a subsequent userspace commit (like a page flip or resolution change) occurs within 2 seconds, old_conn_state->hdr_output_metadata will be false for that new commit. Doesn't this mean the infoframe will abruptly stop, violating the 2-second minimum requirement? > 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 9b637e38a1a51..bdacad8ea30a3 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: Medium] This appears to have the same issue for the native HDMI path. Tying the transmission requirement to an event-based atomic commit means the 2-second minimum duration isn't guaranteed by a timer or delayed worker. Could this result in the null infoframe either being disabled too early (if another commit happens quickly) or remaining enabled forever? > 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
