On 10/7/2025 11:00 PM, Ville Syrjälä wrote:
On Tue, Oct 07, 2025 at 06:16:57PM +0300, Ville Syrjälä wrote:
On Tue, Oct 07, 2025 at 11:22:44AM +0530, Nautiyal, Ankit K wrote:
On 10/7/2025 1:26 AM, Ville Syrjälä wrote:
On Mon, Oct 06, 2025 at 09:58:47AM +0530, Ankit Nautiyal wrote:
Currently crtc_vblank_start is assumed to be the vblank_start for the fixed
refresh rate case. That value can be different from the variable refresh
rate case whenever always_use_vrr_tg()==false. On icl/tgl it's always
different due to the extra vblank delay, and also on adl+ it could be
different if we were to use an optimized guardband.
So places where crtc_vblank_start is used to compute vblank length needs
change so as to account for cases where vrr is enabled. Specifically
with vrr.enable the effective vblank length is actually guardband.
Add a helper to get the correct vblank length for both vrr and fixed
refresh rate cases. Use this helper where vblank_start is used to
compute the vblank length.
Signed-off-by: Ankit Nautiyal<[email protected]>
---
drivers/gpu/drm/i915/display/intel_pfit.c | 11 +++++++----
drivers/gpu/drm/i915/display/intel_psr.c | 3 +--
drivers/gpu/drm/i915/display/intel_vblank.c | 10 ++++++++++
drivers/gpu/drm/i915/display/intel_vblank.h | 2 ++
drivers/gpu/drm/i915/display/skl_watermark.c | 3 ++-
5 files changed, 22 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_pfit.c
b/drivers/gpu/drm/i915/display/intel_pfit.c
index 68539e7c2a24..ebbaa1d419ba 100644
--- a/drivers/gpu/drm/i915/display/intel_pfit.c
+++ b/drivers/gpu/drm/i915/display/intel_pfit.c
@@ -14,6 +14,7 @@
#include "intel_lvds_regs.h"
#include "intel_pfit.h"
#include "intel_pfit_regs.h"
+#include "intel_vblank.h"
#include "skl_scaler.h"
static int intel_pch_pfit_check_dst_window(const struct intel_crtc_state *crtc_state)
@@ -306,14 +307,15 @@ centre_horizontally(struct drm_display_mode
*adjusted_mode,
}
static void
-centre_vertically(struct drm_display_mode *adjusted_mode,
+centre_vertically(struct intel_crtc_state *crtc_state,
+ struct drm_display_mode *adjusted_mode,
int height)
{
u32 border, sync_pos, blank_width, sync_width;
/* keep the vsync and vblank widths constant */
sync_width = adjusted_mode->crtc_vsync_end -
adjusted_mode->crtc_vsync_start;
- blank_width = adjusted_mode->crtc_vblank_end -
adjusted_mode->crtc_vblank_start;
+ blank_width = intel_crtc_vblank_length(crtc_state);
This pfit stuff is computed way before the guardband, and also only
relevant for ancient gen2-4 hardware. So no point in touching this
stuff IMO.
Alright can skip this stuff.
sync_pos = (blank_width - sync_width + 1) / 2;
border = (adjusted_mode->crtc_vdisplay - height + 1) / 2;
@@ -392,7 +394,8 @@ static void i9xx_scale_aspect(struct intel_crtc_state
*crtc_state,
PFIT_HORIZ_INTERP_BILINEAR);
}
} else if (scaled_width < scaled_height) { /* letter */
- centre_vertically(adjusted_mode,
+ centre_vertically(crtc_state,
+ adjusted_mode,
scaled_width / pipe_src_w);
*border = LVDS_BORDER_ENABLE;
@@ -489,7 +492,7 @@ static int gmch_panel_fitting(struct intel_crtc_state
*crtc_state,
* heights and modify the values programmed into the CRTC.
*/
centre_horizontally(adjusted_mode, pipe_src_w);
- centre_vertically(adjusted_mode, pipe_src_h);
+ centre_vertically(crtc_state, adjusted_mode, pipe_src_h);
border = LVDS_BORDER_ENABLE;
break;
case DRM_MODE_SCALE_ASPECT:
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
b/drivers/gpu/drm/i915/display/intel_psr.c
index f7115969b4c5..ae6b94a5d450 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -1365,8 +1365,7 @@ static bool wake_lines_fit_into_vblank(struct intel_dp
*intel_dp,
bool aux_less)
{
struct intel_display *display = to_intel_display(intel_dp);
- int vblank = crtc_state->hw.adjusted_mode.crtc_vblank_end -
- crtc_state->hw.adjusted_mode.crtc_vblank_start;
+ int vblank = intel_crtc_vblank_length(crtc_state);
I *think* this also gets computed during .compute_config() which is
before the guardband calculation. So if this stuff actually depends on
the guardband then we have a real problem here. And if it doesn't (as
in it really interested in the undelayed vblank length) them maybe it
should just compute it as crtc_vtotal-crtc_vactive.
As far as I understand it depends on guardband for VRR case.
For non vrr case : crtc_vtotal - crtc_vactive - scl lines
For vrr case: guardband length.
Currently since guardband is equal to vblank length this can be
crtc_vtotal - crtc_vactive - scl lines.
Perhaps with the optimized guardband, we need to set the guardband
during intel_vrr_compute_config().
Later intel_psr_compute_config gets called and then we can check the
guardband.
Originally we moved the vblank delay calculation to happen later
because we needed to know about PSR for it to be done correctly.
I think someone will need to try to actually write down all the
requirements from both PSR and VRR side and sides and come up
with a way to get it all done in the right order, without any
more chicken vs. egg problems.
I haven't actually checked any of PSR details here, but I'm thinking
if my assumptions hold that there is a dependency both ways, we migth
need soemthing like this:
1. .compute_config()
Check if PSR is generallty possible/desired, and verify that a maximum
guardband would suffice for PSR (this check could also take PSR specific
SCL requirements into consideration)
psr_compute_config currently checks for wake_lines_fit_into_vblank().
So here we continue to check against maximum guardband (vblank_length)
So only thing to add here is the SCL considerations.
2. compute_scl()
Bump SCL if PSR (or anything else) needs it
This is already there, so we are good.
3. vrr_compute_guardband()
Try to accomodate PSR requirements, but don't worry if we can't satisy
that
Hmm here currently we are not checking anything.
With optimized guardband we must check with max psr requirements and not
the current psr requirements.
(With VRR psr will be off so we don't want to change guardband here)
.compute_config_late()
Check whether the actual guardband is sufficient for PSR, and
calculate any other state that depends on the guardband. If not,
disable PSR (hopefully we can still do that at this point...)
Can try disabling psr here if the guardband is not sufficient with
actual psr requirements.
We can check here for other latencies also e.g. SDP.
But scaler related latencies we cannot check here.
Coming back to this patch to use the vblank_start adjustment, I guess we
need to re-evaluate.
Regards,
Ankit
I think that might generally work (if the assumption about being
able to revert the early PSR decision in .compute_config_late()
is valid).
The only corner case I see is if something else requires
bumping SCL and that reduces the guardband below what PSR needs.
But perhaps we should not worry about such issues, unless perhaps
that other SCL bumping requirement can be trivially accomodated
in the PSR .compute_config() as well.
Or I suppose we might try to see if we could compute SCL (considering
only the non-PSR requirements) even earlier (as in before PSR
.compute_config()), and then have the PSR code itself bump SCL if
required during .compute_config(). But this sort of approach we could
look into later, doesn't have to be done now IMO.
