On 2/28/2025 2:35 AM, Ville Syrjälä wrote:
On Thu, Feb 27, 2025 at 04:31:28PM +0530, Nautiyal, Ankit K wrote:
On 2/26/2025 8:41 PM, Ville Syrjälä wrote:
On Mon, Feb 24, 2025 at 11:47:15AM +0530, Ankit Nautiyal wrote:
For fixed refresh rate use fixed timings for all platforms that support
VRR. For this add checks to avoid computing and reading VRR for
platforms that do not support VRR.
For platforms that do support VRR, readback vrr timings whether or not
VRR_CTL_FLIP_LINE_EN is set in VRR_CTL or not.
Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
---
drivers/gpu/drm/i915/display/intel_vrr.c | 43 ++++++++++++------------
1 file changed, 22 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
index 551dcc16f0d4..975fed9930c1 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.c
+++ b/drivers/gpu/drm/i915/display/intel_vrr.c
@@ -344,6 +344,9 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
int vmin = 0, vmax = 0;
+ if (!HAS_VRR(display))
+ return;
+
if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
return;
@@ -358,9 +361,6 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
vmin = intel_vrr_compute_vmin(crtc_state);
- if (vmin >= vmax)
- return;
-
crtc_state->vrr.vmin = vmin;
crtc_state->vrr.vmax = vmax;
I think your earlier pathc left vmax==0 here for the !in_range so
this looks a bit wrong. But if you change the earlier patch like I
suggested to set vmax=vmin then this would be fine.
Right, will set the vmax=vmin, as discussed in the earlier patch.
@@ -373,7 +373,7 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
*/
crtc_state->vrr.vmin -= intel_vrr_flipline_offset(display);
- if (crtc_state->uapi.vrr_enabled)
+ if (crtc_state->uapi.vrr_enabled && vmin < vmax)
intel_vrr_compute_vrr_timings(crtc_state);
else if (is_cmrr_frac_required(crtc_state) && is_edp)
intel_vrr_compute_cmrr_timings(crtc_state);
@@ -640,6 +640,9 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
u32 trans_vrr_ctl, trans_vrr_vsync;
+ if (!HAS_VRR(display))
+ return;
I think the caller is already checking that. But I suppose we could
move the checks into the VRR code.
+
trans_vrr_ctl = intel_de_read(display,
TRANS_VRR_CTL(display, cpu_transcoder));
@@ -663,23 +666,21 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
crtc_state->vrr.pipeline_full =
REG_FIELD_GET(VRR_CTL_PIPELINE_FULL_MASK, trans_vrr_ctl);
- if (trans_vrr_ctl & VRR_CTL_FLIP_LINE_EN) {
- crtc_state->vrr.flipline = intel_de_read(display,
- TRANS_VRR_FLIPLINE(display, cpu_transcoder)) + 1;
- crtc_state->vrr.vmax = intel_de_read(display,
- TRANS_VRR_VMAX(display, cpu_transcoder)) + 1;
- crtc_state->vrr.vmin = intel_de_read(display,
- TRANS_VRR_VMIN(display, cpu_transcoder)) + 1;
-
- if (HAS_AS_SDP(display)) {
- trans_vrr_vsync =
- intel_de_read(display,
- TRANS_VRR_VSYNC(display, cpu_transcoder));
- crtc_state->vrr.vsync_start =
- REG_FIELD_GET(VRR_VSYNC_START_MASK, trans_vrr_vsync);
- crtc_state->vrr.vsync_end =
- REG_FIELD_GET(VRR_VSYNC_END_MASK, trans_vrr_vsync);
- }
I think you want to keep the VRR_CTL_FLIP_LINE_EN check around the
TRANS_VRR_FLIPLINE read at least, because we want the state checker
to catch any misprogrammng of VRR_CTL_FLIP_LINE_EN.
Alright, will remove this change.
There is one more thing I wanted your opinion on:
For PTL+, the support for TRANS_VTOTAL.Vtotal bits is going away.
Aren't we still using the legacy timing gnerator currently on ptl?
I think I did manage to run upstream code on a ptl somehwat
succesfully, and I don't remeber any state checker warns or other
real problems (which I would expect to happen if we don't have a
working vtotal programmed in).
For PTL the support for legacy timing generator is still there but its
recommended to switch to VRR Timing generator.
GOP I think has already moved in that direction.
I can
skip writing it based on the intel_vrr_always_use_vrr_tg(), but how
should I fill adjusted_mode->crtc_vtotal during readout?
Can we use vrr.vmin for that? Or should we just remove the state checker
for crtc_vtotal for platforms where this applies?
I think reading it out from VMIN would be the way to go. Otherwise
we'd have to somehow stop using crtc_vtotal everywhere and that sounds
like a lot of work.
Will try to have this read from VRR.Vmin.
Regards,
Ankit
I am intending to include this change as the last patch of the series.
Thanks again for all the reviews and suggestions.
Regards,
Ankit
+ crtc_state->vrr.flipline = intel_de_read(display,
+ TRANS_VRR_FLIPLINE(display, cpu_transcoder)) + 1;
+ crtc_state->vrr.vmax = intel_de_read(display,
+ TRANS_VRR_VMAX(display, cpu_transcoder)) + 1;
+ crtc_state->vrr.vmin = intel_de_read(display,
+ TRANS_VRR_VMIN(display, cpu_transcoder)) + 1;
+
+ if (HAS_AS_SDP(display)) {
+ trans_vrr_vsync =
+ intel_de_read(display,
+ TRANS_VRR_VSYNC(display, cpu_transcoder));
+ crtc_state->vrr.vsync_start =
+ REG_FIELD_GET(VRR_VSYNC_START_MASK, trans_vrr_vsync);
+ crtc_state->vrr.vsync_end =
+ REG_FIELD_GET(VRR_VSYNC_END_MASK, trans_vrr_vsync);
}
crtc_state->vrr.enable = trans_vrr_ctl & VRR_CTL_VRR_ENABLE &&
--
2.45.2
