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

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

Hello Xiao,

On 11/11/2026 5:46 PM, Xiao Lu wrote:
> Hi Chaitanya,
> 
> On Wed, 11 Nov 2026 16:05:42 +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 is the same change, with the same reasoning (Bspec 75640), as the one
> I posted for Xe2+ two weeks ago:
> 
>    https://patchwork.freedesktop.org/patch/763779/
>    Message-ID: <20261028112423.2445450-1-xiaolu.xie@intel.com>
> 
> Yours looks like the better base for that part, as HAS_EMP_AS_SDP_TL also
> covers BMG and it has the Fixes: and stable tags, so I'm happy to rebase on
> top of it.
> 
> [...]
> 
>> 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.
> 
> Is that PTL report this one?
> 
>    https://gitlab.freedesktop.org/drm/xe/kernel/-/work_items/9693
> 
> Our patch was written for it: an eDP panel dropping the VSC SDP after some
> HDR modesets on PTL, with different VSC ECC registers compared to the
> working case. The reporter tested our patch on the affected machine and
> commented in the issue that the test passes.
> 
> [...]
>

Thanks for the pointer to your earlier patch. Sorry, I missed it. The 
PTL issue I was referring to was a different one:

   https://gitlab.freedesktop.org/drm/xe/kernel/-/work_items/9746

where reverting 6a1712052859 ("drm/i915/dp: Enable AS SDP whenever VRR 
is possible or PR !async") helps, but it looks like the same problem as 
seen in

https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/17732


>> +	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))
> 
> There is one more hunk in our patch that yours does not have. Right below
> this, hsw_write_infoframe() ORs VSC_DIP_HW_DATA_SW_HEA into VIDEO_DIP_CTL
> for the VSC SDP. VSC_SELECT is a two bit field (VSC_SELECT_MASK), so a
> stale value is not replaced but merged with the new one, e.g. 1 | 2 results
> in 3. We clear the field before setting it, and also clear it in
> intel_dp_set_infoframes() when the SDPs get disabled.
> 
> Was leaving that out intentional, e.g. because the field is guaranteed to be
> clear at that point, or is it something that was missed? If it is needed, I
> can send it as a separate patch on top of yours.
> 

I didn't look at VSC_SELECT, as the header write order alone fixed what 
I was seeing. The driver only ever writes the "data only" value there, 
so a stale value would have to come from state the driver didn't write 
(e.g. BIOS) but I reckon clearing it first is harmless. You can post it 
on top of v2 where I plan to drop the HAS_EMP_AS_SDP_TL check, as Ville 
suggested.

==
Chaitanya

> Thanks,
> Xiao