Re: [PATCH] drm/i915/display: send null DRM infoframe when ending HDR transmission

[sashiko-bot@kernel.org]

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 <xiaolu.xie@intel.com>
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/20260901130649.237080-1-xiaolu.xie@intel.com?part=1