Re: [PATCH 5/8] drm/i915/display: Check if final vblank is sufficient for PSR features

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

On 10/13/2025 4:27 PM, Hogander, Jouni wrote:
> On Fri, 2025-10-10 at 19:12 +0530, Nautiyal, Ankit K wrote:
>> On 10/10/2025 12:23 PM, Hogander, Jouni wrote:
>>> On Thu, 2025-10-09 at 14:30 +0530, Ankit Nautiyal wrote:
>>>> Currently, wake line latency checks rely on the vblank length,
>>>> which does not account for either the extra vblank delay for
>>>> icl/tgl
>>>> or for
>>>> the optimized guardband which will come into picture later at
>>>> some
>>>> point.
>>>>
>>>> Introduce intel_dp_compute_config_late() to handle late-stage
>>>> configuration checks for DP/eDP features. For now, it validates
>>>> whether the
>>>> final vblank (with extra vblank delay) or guardband is sufficient
>>>> to
>>>> support wake line latencies required by Panel Replay and PSR2
>>>> selective
>>>> update.
>>>>
>>>> Check if vblank is sufficient for PSR features, and disable them
>>>> if
>>>> their
>>>> wake requirements cannot be accomodated.
>>> Now as we are adding this: Can't we just drop checks made earlier
>>> and
>>> rely on psr_compute_config_late checking the vblank?
>>
>> You're right to raise this question. The key point is that there are
>> dependencies between the PSR configuration, the VRR guardband, and
>> SCL
>> that influence the sequence of checks.
>>
>> Here’s how the flow works:
>>
>>
>> 1. psr_compute_config()
>> This is called first to determine if PSR is possible.
>> At this stage:
>>
>> -> We check if the vblank is long enough to accommodate wake lines.
>> -> However, we don’t yet know the actual guardband or whether SCL
>> lines
>> need to be accounted for.
>> -> So, we can only establish whether the vblank length is sufficient
>> in
>> a general sense.
>> -> On platforms like ICL/TGL (with extra vblank delay) or with
>> optimized
>> guardband, the actual lines may be fewer than the full vblank length.
> Please add a comment into psr_compute_config that it is roughly
> checking if PSR is possible with current understanding of vblank
> length. It will be checked later in psr_compute_config_late against
> optimized vblank length.

Makes sense. Will add the appropriate comment.


>>
>> 2. compute_scl()
>>
>> -> This computes the SCL.
>> -> If PSR was not enabled earlier, SCL will be 0 at this point.
>> -> The vblank_start is adjusted to accommodate the SCL lines.
>>
>>
>> 3. vrr_compute_guardband()
>>
>> -> This sets the guardband.
>> -> With optimized guardband, we consider max PSR requirements and
>> other
>> prefill latencies.
>> -> On platforms where VRR TG is always active, the guardband cannot
>> be
>> changed dynamically and any change in guardband triggers a full
>> modeset.
>> -> So, the goal is to set a guardband during modeset that works
>> across
>> most scenarios.
>>
>>
>> 4. psr_compute_config_late()
>>
>> -> This is where we re-check if the guardband is sufficient for PSR
>> wake
>> time latencies.
>> -> If not, we disable PSR features that can’t be supported with the
>> current timing.
> Add comment into psr_compute_config_late about SCL being left untouched
> and containing intel_psr_set_context_latency if PSR was possible after
> intel_psr_compute_config.

Hmm sure can add rationale for not re-setting SCL.

Thanks & Regards,

Ankit

>
>>
>> As mentioned in the earlier comment, more details are available in
>> the
>> following references:
>> [1] https://lore.kernel.org/all/aOVOJp2zeN1eCp7O@intel.com/
>> [2]
>> https://patchwork.freedesktop.org/patch/678520/?series=151245&rev=13
>>
>> So to answer your question: We can't entirely drop the early checks
>> in
>> psr_compute_config(), as it helps to filter PSR early based on vblank
>> length, and also helps to get the SCL adjustments. By the time we
>> reach
>> psr_compute_config_late() we have more accurate picture to take a
>> call
>> to disable specific PSR features.
>>
>>
>> That said, do you see any issues if we disable these later?
>> Also, are there other parts or logic that depend on
>> crtc_state->has_panel_replay and crtc_state->has_sel_update that you
>> think could be moved to psr_compute_config_late()?
> I don't see other need for psr_compute_config_late ATM.
>
> BR,
>
> Jouni Högander
>> Regards,
>>
>> Ankit
>>
>>> BR,
>>>
>>> Jouni Högander
>>>
>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>>> Cc: Animesh Manna <animesh.manna@intel.com>
>>>> Cc: Jouni Högander <jouni.hogander@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/display/intel_ddi.c |  3 ++
>>>>    drivers/gpu/drm/i915/display/intel_dp.c  |  9 +++++
>>>>    drivers/gpu/drm/i915/display/intel_dp.h  |  3 ++
>>>>    drivers/gpu/drm/i915/display/intel_psr.c | 51
>>>> ++++++++++++++++++++--
>>>> --
>>>>    drivers/gpu/drm/i915/display/intel_psr.h |  2 +
>>>>    5 files changed, 60 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
>>>> b/drivers/gpu/drm/i915/display/intel_ddi.c
>>>> index c09aa759f4d4..94c593bbedf4 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
>>>> @@ -4560,6 +4560,9 @@ static int
>>>> intel_ddi_compute_config_late(struct
>>>> intel_encoder *encoder,
>>>>    	struct drm_connector *connector = conn_state->connector;
>>>>    	u8 port_sync_transcoders = 0;
>>>>    
>>>> +	if (intel_crtc_has_dp_encoder(crtc_state))
>>>> +		intel_dp_compute_config_late(encoder,
>>>> crtc_state,
>>>> conn_state);
>>>> +
>>>>    	drm_dbg_kms(display->drm, "[ENCODER:%d:%s]
>>>> [CRTC:%d:%s]\n",
>>>>    		    encoder->base.base.id, encoder->base.name,
>>>>    		    crtc_state->uapi.crtc->base.id, crtc_state-
>>>>> uapi.crtc->name);
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>>>> b/drivers/gpu/drm/i915/display/intel_dp.c
>>>> index a723e846321f..e481ff4c4959 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>>>> @@ -6979,3 +6979,12 @@ void intel_dp_mst_resume(struct
>>>> intel_display
>>>> *display)
>>>>    		}
>>>>    	}
>>>>    }
>>>> +
>>>> +void intel_dp_compute_config_late(struct intel_encoder *encoder,
>>>> +				  struct intel_crtc_state
>>>> *crtc_state,
>>>> +				  struct drm_connector_state
>>>> *conn_state)
>>>> +{
>>>> +	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>>>> +
>>>> +	intel_psr_compute_config_late(intel_dp, crtc_state);
>>>> +}
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h
>>>> b/drivers/gpu/drm/i915/display/intel_dp.h
>>>> index b379443e0211..0d9573ca44cb 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dp.h
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
>>>> @@ -218,5 +218,8 @@ int intel_dp_compute_min_hblank(struct
>>>> intel_crtc_state *crtc_state,
>>>>    int intel_dp_dsc_bpp_step_x16(const struct intel_connector
>>>> *connector);
>>>>    void intel_dp_dpcd_set_probe(struct intel_dp *intel_dp, bool
>>>> force_on_external);
>>>>    bool intel_dp_in_hdr_mode(const struct drm_connector_state
>>>> *conn_state);
>>>> +void intel_dp_compute_config_late(struct intel_encoder *encoder,
>>>> +				  struct intel_crtc_state
>>>> *crtc_state,
>>>> +				  struct drm_connector_state
>>>> *conn_state);
>>>>    
>>>>    #endif /* __INTEL_DP_H__ */
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
>>>> b/drivers/gpu/drm/i915/display/intel_psr.c
>>>> index 212bd244beed..dcab4127b399 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_psr.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>>>> @@ -1405,6 +1405,20 @@ int
>>>> _intel_psr_min_set_context_latency(const
>>>> struct intel_crtc_state *crtc_state
>>>>    		return 1;
>>>>    }
>>>>    
>>>> +static bool _wake_lines_fit_into_vblank(const struct
>>>> intel_crtc_state *crtc_state,
>>>> +					int vblank,
>>>> +					int wake_lines)
>>>> +{
>>>> +	if (crtc_state->req_psr2_sdp_prior_scanline)
>>>> +		vblank -= 1;
>>>> +
>>>> +	/* Vblank >= PSR2_CTL Block Count Number maximum line
>>>> count
>>>> */
>>>> +	if (vblank < wake_lines)
>>>> +		return false;
>>>> +
>>>> +	return true;
>>>> +}
>>>> +
>>>>    static bool wake_lines_fit_into_vblank(struct intel_dp
>>>> *intel_dp,
>>>>    				       const struct
>>>> intel_crtc_state
>>>> *crtc_state,
>>>>    				       bool aux_less,
>>>> @@ -1428,14 +1442,7 @@ static bool
>>>> wake_lines_fit_into_vblank(struct
>>>> intel_dp *intel_dp,
>>>>    					       crtc_state-
>>>>> alpm_state.fast_wake_lines) :
>>>>    			crtc_state->alpm_state.io_wake_lines;
>>>>    
>>>> -	if (crtc_state->req_psr2_sdp_prior_scanline)
>>>> -		vblank -= 1;
>>>> -
>>>> -	/* Vblank >= PSR2_CTL Block Count Number maximum line
>>>> count
>>>> */
>>>> -	if (vblank < wake_lines)
>>>> -		return false;
>>>> -
>>>> -	return true;
>>>> +	return _wake_lines_fit_into_vblank(crtc_state, vblank,
>>>> wake_lines);
>>>>    }
>>>>    
>>>>    static bool alpm_config_valid(struct intel_dp *intel_dp,
>>>> @@ -4346,3 +4353,31 @@ bool intel_psr_needs_alpm_aux_less(struct
>>>> intel_dp *intel_dp,
>>>>    {
>>>>    	return intel_dp_is_edp(intel_dp) && crtc_state-
>>>>> has_panel_replay;
>>>>    }
>>>> +
>>>> +void intel_psr_compute_config_late(struct intel_dp *intel_dp,
>>>> +				   struct intel_crtc_state
>>>> *crtc_state)
>>>> +{
>>>> +	struct intel_display *display =
>>>> to_intel_display(intel_dp);
>>>> +	int vblank = intel_crtc_vblank_length(crtc_state);
>>>> +	int aux_less_wake_lines = crtc_state-
>>>>> alpm_state.aux_less_wake_lines;
>>>> +	int wake_lines = DISPLAY_VER(display) < 20 ?
>>>> +			 psr2_block_count_lines(crtc_state-
>>>>> alpm_state.io_wake_lines,
>>>> +						crtc_state-
>>>>> alpm_state.fast_wake_lines) :
>>>> +			 crtc_state->alpm_state.io_wake_lines;
>>>> +
>>>> +	if (intel_psr_needs_alpm_aux_less(intel_dp, crtc_state)
>>>> &&
>>>> +	    !_wake_lines_fit_into_vblank(crtc_state, vblank,
>>>> aux_less_wake_lines)) {
>>>> +		drm_dbg_kms(display->drm,
>>>> +			    "Disabling Panel replay: vblank
>>>> insufficient for wakelines = %d\n",
>>>> +			    aux_less_wake_lines);
>>>> +		crtc_state->has_panel_replay = false;
>>>> +	}
>>>> +
>>>> +	if (crtc_state->has_sel_update &&
>>>> +	    !_wake_lines_fit_into_vblank(crtc_state, vblank,
>>>> wake_lines)) {
>>>> +		drm_dbg_kms(display->drm,
>>>> +			    "Disabling Selective Update: vblank
>>>> insufficient for wakelines = %d\n",
>>>> +			    wake_lines);
>>>> +		crtc_state->has_sel_update = false;
>>>> +	}
>>>> +}
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
>>>> b/drivers/gpu/drm/i915/display/intel_psr.h
>>>> index 9147996d6c9e..b17ce312dc37 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_psr.h
>>>> +++ b/drivers/gpu/drm/i915/display/intel_psr.h
>>>> @@ -83,5 +83,7 @@ void intel_psr_debugfs_register(struct
>>>> intel_display *display);
>>>>    bool intel_psr_needs_alpm(struct intel_dp *intel_dp, const
>>>> struct
>>>> intel_crtc_state *crtc_state);
>>>>    bool intel_psr_needs_alpm_aux_less(struct intel_dp *intel_dp,
>>>>    				   const struct intel_crtc_state
>>>> *crtc_state);
>>>> +void intel_psr_compute_config_late(struct intel_dp *intel_dp,
>>>> +				   struct intel_crtc_state
>>>> *crtc_state);
>>>>    
>>>>    #endif /* __INTEL_PSR_H__ */