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

["Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com>]

On 11/11/2026 5:06 PM, Ville Syrjälä wrote:
> 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.
> 

Agreed. I was trying to keep the exposure minimal but this should be 
harmless given the registers are either not double-buffered or double 
buffered without an arming register in the older platforms.

>>   		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.
> 

Noted for 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
>