Re: [PATCH] drm/i915/dip: Write the DIP header last with SDP transmission lines

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Nov 11, 2026 at 04:05:42PM +0530, Chaitanya Kumar Borah wrote:
> On platforms with SDP transmission line registers, the packets anchored
> to them are double buffered in the transcoder (TC) clock domain. The packet
> registers are in the core display (CD) clock domain and the whole packet is
> clock crossed on the write of the header (dword 0), so Bspec requires the
> data to be programmed first and the header last.
> 
> hsw_write_infoframe() writes the header first, so the packet is crossed
> before its payload is in place and the previous payload keeps being sent.
> This hits the AS SDP, and the VSC SDP whenever an AS SDP is enabled, which
> is whenever VRR is possible since commit 6a1712052859 ("drm/i915/dp: Enable
> AS SDP whenever VRR is possible or PR !async"). With CMN_SDP_TL, enabled by
> commit bfa597238073 ("drm/i915/dip: Enable Common SDP Transmission line"),
> it also hits the GMP and VSC SDPs: on NVL, entering HDR on an eDP panel
> leaves the image discoloured and scaled until the headers are rewritten.
> 
> Write the payload, including the ECC zero fill, first and the header last
> on platforms with EMP_AS_SDP_TL. CD domain packets have no ordering
> requirement and Bspec recommends the same order for all packets, so use it
> for all of them. Older platforms keep the current order.
> 
> Bspec: 75640, 71197, 74384
> Fixes: e1123e617e51 ("drm/i915/vrr: Program EMP_AS_SDP_TL for DP AS SDP")
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/17732
> Cc: <stable@vger.kernel.org> # v6.17+
> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Cc: Suraj Kandpal <suraj.kandpal@intel.com>
> Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> ---
> 
> Notes:
>     Fixes tag rationale
>     
>     The Fixes tag points at commit e1123e617e51 ("drm/i915/vrr: Program
>     EMP_AS_SDP_TL for DP AS SDP"). The AS SDP has been written header first
>     since it was added by commit 8845021b7331 ("drm/i915/display: Read/Write
>     Adaptive Sync SDP"), but EMP_AS_SDP_TL, which holds the double buffer
>     point and the transmission line of the AS SDP on BMG and LNL+, was only
>     programmed from that commit on. From then on the driver relies on that
>     register to anchor the AS SDP, and the VSC SDP whenever an AS SDP is
>     enabled, and the programming note of the register asks for the packet
>     data first and the header last. The two later commits cited in the commit
>     message only widened the exposure: the first enabled the AS SDP whenever
>     VRR is possible and the second added CMN_SDP_TL for the GMP and VSC SDPs.
>     
>     Alternatives considered:
>      - 6a1712052859 (v7.2): AS SDP on whenever VRR is possible. The PTL
>        report is fixed by reverting it. Narrower than the spec argument, as
>        it only covers v7.2 and later.
>      - bfa597238073 (7.4): CMN_SDP_TL. The NVL regression, verified on
>        hardware, but covered by the older tag as every kernel with it also
>        has e1123e617e51.
>      - 8845021b7331 (v6.10): first header first AS SDP write, but
>        EMP_AS_SDP_TL was not programmed until e1123e617e51, so exposure
>        before it could not be shown.
>     
>     Not verified:
>      - The failure was observed on NVL with CMN_SDP_TL (rewriting the GMP and
>        VSC headers makes the panel recover). The AS SDP and the VSC SDP with
>        an AS SDP on BMG, LNL and PTL follow from the Bspec requirement and
>        were not reproduced here.
>      - No reproduction on kernels older than v7.2.
>      - Stable backports need the HAS_EMP_AS_SDP_TL gate adapted, as it is
>        defined in intel_dip.h, which is new in 7.4.
> 
>  drivers/gpu/drm/i915/display/intel_hdmi.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 4ec0db8d93fb..3ecf017b3176 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -51,6 +51,7 @@
>  #include "intel_cx0_phy.h"
>  #include "intel_ddi.h"
>  #include "intel_de.h"
> +#include "intel_dip.h"
>  #include "intel_display_driver.h"
>  #include "intel_display_regs.h"
>  #include "intel_display_types.h"
> @@ -524,6 +525,15 @@ void hsw_write_infoframe(struct intel_encoder *encoder,
>  	const u32 *data = frame;
>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>  	intel_reg_t ctl_reg = HSW_TVIDEO_DIP_CTL(display, cpu_transcoder);
> +	/*
> +	 * On platforms with the SDP transmission line registers some of the
> +	 * packets are double buffered in the transcoder clock domain, and the
> +	 * packet is clock crossed on the write of the header (dword 0). Program
> +	 * the payload first and the header last, so that the whole packet gets
> +	 * crossed. Packets double buffered in the core display clock domain
> +	 * have no ordering requirement, so use the same order for all of them.
> +	 */
> +	bool header_last = HAS_EMP_AS_SDP_TL(display);
>  	int data_size;
>  	int i;
>  	u32 val = intel_de_read(display, ctl_reg);
> @@ -535,18 +545,21 @@ void hsw_write_infoframe(struct intel_encoder *encoder,
>  	val &= ~hsw_infoframe_enable(type);
>  	intel_de_write(display, ctl_reg, val);
>  
> -	for (i = 0; i < len; i += 4) {
> +	for (i = header_last ? 4 : 0; i < len; i += 4)

IMO just always use the "write header last" approach. 

>  		intel_de_write(display,
>  			       hsw_dip_data_reg(display, cpu_transcoder, type, i >> 2),
> -			       *data);
> -		data++;
> -	}
> +			       data[i >> 2]);

Making 'i' the dword index rather than the byte index might make this
code a bit nicer. The only slight concern is whether len%4!=0 might
be true. Maybe just have the caller always align it for good measure.
But let's stick to the byte based thing for now to avoid adding more
noise to this patch, and clean it up later.

>  	/* Write every possible data byte to force correct ECC calculation. */
>  	for (; i < data_size; i += 4)
>  		intel_de_write(display,
>  			       hsw_dip_data_reg(display, cpu_transcoder, type, i >> 2),
>  			       0);
>  
> +	if (header_last)
> +		intel_de_write(display,
> +			       hsw_dip_data_reg(display, cpu_transcoder, type, 0),
> +			       data[0]);
> +
>  	/* Wa_14013475917 */
>  	if (!(IS_DISPLAY_VER(display, 13, 14) && crtc_state->has_psr &&
>  	      !crtc_state->has_panel_replay && type == DP_SDP_VSC))
> -- 
> 2.50.1

-- 
Ville Syrjälä
Intel