Re: [PATCH] drm/i915/vrr: Fix transmission line for Adaptive Sync SDP

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 10/24/2025 6:09 PM, Ville Syrjälä wrote:
> On Thu, Oct 23, 2025 at 06:08:47PM +0300, Ville Syrjälä wrote:
>> On Thu, Oct 23, 2025 at 05:40:09PM +0300, Ville Syrjälä wrote:
>>> On Thu, Oct 23, 2025 at 05:57:02PM +0530, Nautiyal, Ankit K wrote:
>>>> On 10/23/2025 5:34 PM, Ville Syrjälä wrote:
>>>>> On Thu, Oct 23, 2025 at 01:46:14PM +0530, Ankit Nautiyal wrote:
>>>>>> Currently the EMP_AS_SDP_TL is set to vrr.vsync_start which is
>>>>>> incorrect.
>>>>>>
>>>>>> As per Bspec:71197 the transmission line must be within the SCL +
>>>>>> guardband region. Before guardband optimization, guradband was same as
>>>>>> vblank length so EMP_AS_SDP_TL set with vrr.sync_start was falling in
>>>>>> this region and it was not giving an issue.
>>>>>>
>>>>>> Now with optimized guardband, this is falling outside the SCL +
>>>>>> guardband region and since the same transmission line is used by VSC SDP
>>>>>> also, this results in PSR timeout issues.
>>>>>>
>>>>>> Further restrictions on the position of the transmission line:
>>>>>> For DP/eDP, if there is a set context latency (SCL) window, then it
>>>>>> cannot be the first line of SCL
>>>>>> For DP/eDP, if there is no SCL window, then it cannot be the first line of
>>>>>> the Delayed V. Blank
>>>>>>
>>>>>> Fix the EMP_AS_SDP_TL to VTOTAL - (delayed vblank_start - SCL + 1)
>>>>>> Internally the HW computes the value as VTOTAL - EMP_AS_SDP_TL.
>>>>>>
>>>>>> Fixes: e1123e617e51 ("drm/i915/vrr: Program EMP_AS_SDP_TL for DP AS SDP")
>>>>>> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>>>>> Cc: Jouni Högander <jouni.hogander@intel.com>
>>>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>>>>> ---
>>>>>>    drivers/gpu/drm/i915/display/intel_vrr.c | 12 +++++++++---
>>>>>>    1 file changed, 9 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
>>>>>> index 92fb72b56f16..dd81d2133aba 100644
>>>>>> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
>>>>>> @@ -655,18 +655,24 @@ void intel_vrr_set_db_point_and_transmission_line(const struct intel_crtc_state
>>>>>>    {
>>>>>>    	struct intel_display *display = to_intel_display(crtc_state);
>>>>>>    	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>>>>>> +	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
>>>>>> +	int transmission_line;
>>>>>>    
>>>>>>    	/*
>>>>>>    	 * For BMG and LNL+ onwards the EMP_AS_SDP_TL is used for programming
>>>>>>    	 * double buffering point and transmission line for VRR packets for
>>>>>>    	 * HDMI2.1/DP/eDP/DP->HDMI2.1 PCON.
>>>>>>    	 * Since currently we support VRR only for DP/eDP, so this is programmed
>>>>>> -	 * to for Adaptive Sync SDP to Vsync start.
>>>>>> +	 * for Adaptive Sync SDP.
>>>>>>    	 */
>>>>>> -	if (DISPLAY_VERx100(display) == 1401 || DISPLAY_VER(display) >= 20)
>>>>>> +	if (DISPLAY_VERx100(display) == 1401 || DISPLAY_VER(display) >= 20) {
>>>>>> +		transmission_line = adjusted_mode->crtc_vtotal - (adjusted_mode->crtc_vblank_start -
>>>>>> +								  crtc_state->set_context_latency +
>>>>>> +								  1);
>>>>>>    		intel_de_write(display,
>>>>>>    			       EMP_AS_SDP_TL(display, cpu_transcoder),
>>>>>> -			       EMP_AS_SDP_DB_TL(crtc_state->vrr.vsync_start));
>>>>>> +			       EMP_AS_SDP_DB_TL(transmission_line));
>>>>>> +	}
>>>>> Pretty sure we are expected to send it at vsync_start.
>>>> Hmm.. then do we need to move vsync_start too similar to vblank_start
>>>> for optimized guardband?
>>> The vsync pulse location is dictated by the timings.
>>>
>>>> If we do not move vsync_start, and set the transmission line to
>>>> vsync_start, it will never fall in the region SCL + guardband with a
>>>> reduced guardband.
>>> Only if the vsync pulse is early in the vblank. That's up to the
>>> display.
>> Oh and I think we should get rid of that 'assume_all_enabled' stuff
>> for the AS SDP, and account for it only when actually needed.
>> Which I *think* means PCON or panel replay with AUX-less ALPM.
>>
>> There's also that t1 vs. t2 setup time thing for the panel replay,
>> which seems to be telling me that we could sometimes transmit the
>> AS SDP later. But if I'm reading that right we have to switch to the
>> t1 (vsync) transmission line whenever we switch to live frame mode,
>> which I presume can happen basically at any time. So maybe we can't
>> actually use that t2 transmission line.
> Ah, apparently we have to always put t1 into EMP_AS_SDP_TL, and
> there's some other logic for panel replay to use t2 when possible.
> But since it's all dynamic we have to use t1 when calculating the
> guardband/SCL.

Yeah t2 will be taken up by HW based on PR_ALPM_CTL adaptive sync SDP 
position (to have either t1/t2 or t1 always or t2 always) and also what 
we send in AS SDP payload wrt to Fixed Vtotal, Fixed AVG VTotal mode.

I agree with your suggestion above. For PCON, VRR support still needs 
work so perhaps we can skip that. Makes sense to always enable AS  SDP 
when Panel replay with Auxless-ALPM is supported.

With that we can use Jouni's patch to increase the guardband such that 
Vsync falls in the required region [1], effectively 
`crtc_state->vrr.vsync_start + 1`.

I was trying with the above changes suggested, sent also to trybot [2].

But now I see that we have a problem with LRR on such panels.

When switching to lower mode with LRR, the `vsync_start` changes, which 
results in guardband being changed.

So seamless switch to the required lower mode cannot take place. :-(


[1] https://patchwork.freedesktop.org/patch/682984/?series=156341&rev=2

[2] https://patchwork.freedesktop.org/series/156569/


Regards,

Ankit

>