Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The logic intended to send a null DRM infoframe for 2 seconds fails to enforce the CTA-861-H requirement, either transmitting the infoframe indefinitely during SDR operation or stopping prematurely on subsequent modesets, and forces an unnecessary full modeset. --
commit 76ba1f2a6961be364a75dece13a26afe2ad6b08f Author: Xiao Lu <[email protected]> Subject: drm/i915/display: send null DRM infoframe when ending HDR transmission This commit modifies the DP and HDMI display paths to send a zeroed DRM infoframe when transitioning from HDR to SDR. This intends to fulfill the CTA-861-H requirement to send this null infoframe for at least 2 seconds rather than abruptly stopping HDR metadata transmission. > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > b/drivers/gpu/drm/i915/display/intel_dp.c > index 0cd5e6b5034cf..f71e2a021663a 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 fulfill the 2-second CTA-861-H requirement mentioned in the comment? The zeroed infoframe is enabled here when transitioning from HDR to SDR, but it doesn't appear to be disabled by a timer after 2 seconds. Can this lead to indefinite transmission of the null infoframe during SDR operation if no subsequent commits occur? Furthermore, because this logic strictly relies on old_conn_state->hdr_output_metadata, won't the very next atomic commit involving the connector evaluate this as false? That would drop the DRM infoframe enable bit, causing intel_pipe_config_compare() to flag a mismatch in infoframes.enable. Will this force an unexpected full modeset on the subsequent commit? > return; > + } [ ... ] > 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: High] Similarly for the HDMI path in intel_hdmi_compute_drm_infoframe(), does this suffer from the same indefinite transmission and fastset mismatch issues on subsequent atomic commits as the DP path? > return true; > + } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
