[Intel-gfx] [PATCH 1/4] drm/i915: Update vblank timestamping stuff on seamless M/N change

[Intel-gfx] [PATCH 1/4] drm/i915: Update vblank timestamping stuff on seamless M/N change

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When we change the M/N values seamlessly during a fastset we should
also update the vblank timestamping stuff to make sure the vblank
timestamp corrections/guesstimations come out exact.

Note that only crtc_clock and framedur_ns can actually end up
changing here during fastsets. Everything else we touch can
only change during full modesets.

Technically we should try to do this exactly at the start of
vblank, but that would require some kind of double buffering
scheme. Let's skip that for now and just update things right
after the commit has been submitted to the hardware. This
means the information will be properly up to date when the
vblank irq handler goes to work. Only if someone ends up
querying some vblanky stuff in between the commit and start
of vblank may we see a slight discrepancy.

Also this same problem really exists for the DRRS downclocking
stuff. But as that is supposed to be more or less transparent
to the user, and it only drops to low gear after a long delay
(1 sec currently) we probably don't have to worry about it.
Any time something is actively submitting updates DRRS will
remain in high gear and so the timestamping constants will
match the hardware state.

Fixes: e6f29923c048 ("drm/i915: Allow M/N change during fastset on bdw+")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_crtc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
index b79a8834559f..41d381bbb57a 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc.c
@@ -686,6 +686,14 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
 	 */
 	intel_vrr_send_push(new_crtc_state);
 
+	/*
+	 * Seamless M/N update may need to update frame timings.
+	 *
+	 * FIXME Should be synchronized with the start of vblank somehow...
+	 */
+	if (new_crtc_state->seamless_m_n && intel_crtc_needs_fastset(new_crtc_state))
+		intel_crtc_update_active_timings(new_crtc_state);
+
 	local_irq_enable();
 
 	if (intel_vgpu_active(dev_priv))
-- 
2.39.2

[Intel-gfx] [PATCH 2/4] drm/i915: Add belts and suspenders locking for seamless M/N changes

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add some (probably overkill) locking to protect the vblank
timestamping constants updates during seamless M/N fastsets.

As everything should be naturally aligned I think the individual
pieces should probably end up updating atomically enough. So this
is only really meant to guarantee everyone sees a consistent whole.

All the drm_vblank.c usage is covered by vblank_time_lock,
and uncore.lock will take care of __intel_get_crtc_scanline()
that can also be called from outside the core vblank functionality.

Currently only crtc_clock and framedur_ns can change, but in
the future might fastset also across eg. vtotal/vblank_end
changes, so let's just grab the locks across the whole thing.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 24 +++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index a1fbdf32bd21..020320468967 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -5908,6 +5908,8 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	struct drm_display_mode adjusted_mode;
+	int vmax_vblank_start = 0;
+	unsigned long irqflags;
 
 	drm_mode_init(&adjusted_mode, &crtc_state->hw.adjusted_mode);
 
@@ -5915,11 +5917,28 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
 		adjusted_mode.crtc_vtotal = crtc_state->vrr.vmax;
 		adjusted_mode.crtc_vblank_end = crtc_state->vrr.vmax;
 		adjusted_mode.crtc_vblank_start = intel_vrr_vmin_vblank_start(crtc_state);
-		crtc->vmax_vblank_start = intel_vrr_vmax_vblank_start(crtc_state);
+		vmax_vblank_start = intel_vrr_vmax_vblank_start(crtc_state);
 	}
 
+	/*
+	 * Belts and suspenders locking to guarantee everyone sees 100%
+	 * consistent state during fastset seamless refresh rate changes.
+	 *
+	 * vblank_time_lock takes care of all drm_vblank.c stuff, and
+	 * uncore.lock takes care of __intel_get_crtc_scanline() which
+	 * may get called elsewhere as well.
+	 *
+	 * TODO maybe just protect everything (including
+	 * __intel_get_crtc_scanline()) with vblank_time_lock?
+	 * Need to audit everything to make sure it's safe.
+	 */
+	spin_lock_irqsave(&dev_priv->drm.vblank_time_lock, irqflags);
+	spin_lock(&dev_priv->uncore.lock);
+
 	drm_calc_timestamping_constants(&crtc->base, &adjusted_mode);
 
+	crtc->vmax_vblank_start = vmax_vblank_start;
+
 	crtc->mode_flags = crtc_state->mode_flags;
 
 	/*
@@ -5963,6 +5982,9 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
 	} else {
 		crtc->scanline_offset = 1;
 	}
+
+	spin_unlock(&dev_priv->uncore.lock);
+	spin_unlock_irqrestore(&dev_priv->drm.vblank_time_lock, irqflags);
 }
 
 /*
-- 
2.39.2

[Intel-gfx] [PATCH 3/4] drm/i915: Relocate intel_crtc_update_active_timings()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Move intel_crtc_update_active_timings() into intel_vblank.c
where it more properly belongs.

Also do the s/dev_priv/i915/ modernization rename while at it.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c  | 84 ------------------
 drivers/gpu/drm/i915/display/intel_display.h  |  1 -
 .../drm/i915/display/intel_modeset_setup.c    |  1 +
 drivers/gpu/drm/i915/display/intel_vblank.c   | 85 +++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_vblank.h   |  2 +
 5 files changed, 88 insertions(+), 85 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 020320468967..add3bed3d2e1 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -5903,90 +5903,6 @@ int intel_modeset_all_pipes(struct intel_atomic_state *state,
 	return 0;
 }
 
-void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
-{
-	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	struct drm_display_mode adjusted_mode;
-	int vmax_vblank_start = 0;
-	unsigned long irqflags;
-
-	drm_mode_init(&adjusted_mode, &crtc_state->hw.adjusted_mode);
-
-	if (crtc_state->vrr.enable) {
-		adjusted_mode.crtc_vtotal = crtc_state->vrr.vmax;
-		adjusted_mode.crtc_vblank_end = crtc_state->vrr.vmax;
-		adjusted_mode.crtc_vblank_start = intel_vrr_vmin_vblank_start(crtc_state);
-		vmax_vblank_start = intel_vrr_vmax_vblank_start(crtc_state);
-	}
-
-	/*
-	 * Belts and suspenders locking to guarantee everyone sees 100%
-	 * consistent state during fastset seamless refresh rate changes.
-	 *
-	 * vblank_time_lock takes care of all drm_vblank.c stuff, and
-	 * uncore.lock takes care of __intel_get_crtc_scanline() which
-	 * may get called elsewhere as well.
-	 *
-	 * TODO maybe just protect everything (including
-	 * __intel_get_crtc_scanline()) with vblank_time_lock?
-	 * Need to audit everything to make sure it's safe.
-	 */
-	spin_lock_irqsave(&dev_priv->drm.vblank_time_lock, irqflags);
-	spin_lock(&dev_priv->uncore.lock);
-
-	drm_calc_timestamping_constants(&crtc->base, &adjusted_mode);
-
-	crtc->vmax_vblank_start = vmax_vblank_start;
-
-	crtc->mode_flags = crtc_state->mode_flags;
-
-	/*
-	 * The scanline counter increments at the leading edge of hsync.
-	 *
-	 * On most platforms it starts counting from vtotal-1 on the
-	 * first active line. That means the scanline counter value is
-	 * always one less than what we would expect. Ie. just after
-	 * start of vblank, which also occurs at start of hsync (on the
-	 * last active line), the scanline counter will read vblank_start-1.
-	 *
-	 * On gen2 the scanline counter starts counting from 1 instead
-	 * of vtotal-1, so we have to subtract one (or rather add vtotal-1
-	 * to keep the value positive), instead of adding one.
-	 *
-	 * On HSW+ the behaviour of the scanline counter depends on the output
-	 * type. For DP ports it behaves like most other platforms, but on HDMI
-	 * there's an extra 1 line difference. So we need to add two instead of
-	 * one to the value.
-	 *
-	 * On VLV/CHV DSI the scanline counter would appear to increment
-	 * approx. 1/3 of a scanline before start of vblank. Unfortunately
-	 * that means we can't tell whether we're in vblank or not while
-	 * we're on that particular line. We must still set scanline_offset
-	 * to 1 so that the vblank timestamps come out correct when we query
-	 * the scanline counter from within the vblank interrupt handler.
-	 * However if queried just before the start of vblank we'll get an
-	 * answer that's slightly in the future.
-	 */
-	if (DISPLAY_VER(dev_priv) == 2) {
-		int vtotal;
-
-		vtotal = adjusted_mode.crtc_vtotal;
-		if (adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
-			vtotal /= 2;
-
-		crtc->scanline_offset = vtotal - 1;
-	} else if (HAS_DDI(dev_priv) &&
-		   intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
-		crtc->scanline_offset = 2;
-	} else {
-		crtc->scanline_offset = 1;
-	}
-
-	spin_unlock(&dev_priv->uncore.lock);
-	spin_unlock_irqrestore(&dev_priv->drm.vblank_time_lock, irqflags);
-}
-
 /*
  * This implements the workaround described in the "notes" section of the mode
  * set sequence documentation. When going from no pipes or single pipe to
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index 50285fb4fcf5..ce97cb72f6d3 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -422,7 +422,6 @@ bool intel_crtc_get_pipe_config(struct intel_crtc_state *crtc_state);
 bool intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 			       const struct intel_crtc_state *pipe_config,
 			       bool fastset);
-void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state);
 
 void intel_plane_destroy(struct drm_plane *plane);
 void i9xx_set_pipeconf(const struct intel_crtc_state *crtc_state);
diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
index 60f71e6f0491..c51209a3d36a 100644
--- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
+++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
@@ -26,6 +26,7 @@
 #include "intel_modeset_setup.h"
 #include "intel_pch_display.h"
 #include "intel_pm.h"
+#include "intel_vblank.h"
 #include "intel_wm.h"
 #include "skl_watermark.h"
 
diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
index 571f5dda1e66..48bf3923af11 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.c
+++ b/drivers/gpu/drm/i915/display/intel_vblank.c
@@ -8,6 +8,7 @@
 #include "intel_de.h"
 #include "intel_display_types.h"
 #include "intel_vblank.h"
+#include "intel_vrr.h"
 
 /*
  * This timing diagram depicts the video signal in and
@@ -439,3 +440,87 @@ void intel_wait_for_pipe_scanline_moving(struct intel_crtc *crtc)
 {
 	wait_for_pipe_scanline_moving(crtc, true);
 }
+
+void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
+	struct drm_display_mode adjusted_mode;
+	int vmax_vblank_start = 0;
+	unsigned long irqflags;
+
+	drm_mode_init(&adjusted_mode, &crtc_state->hw.adjusted_mode);
+
+	if (crtc_state->vrr.enable) {
+		adjusted_mode.crtc_vtotal = crtc_state->vrr.vmax;
+		adjusted_mode.crtc_vblank_end = crtc_state->vrr.vmax;
+		adjusted_mode.crtc_vblank_start = intel_vrr_vmin_vblank_start(crtc_state);
+		vmax_vblank_start = intel_vrr_vmax_vblank_start(crtc_state);
+	}
+
+	/*
+	 * Belts and suspenders locking to guarantee everyone sees 100%
+	 * consistent state during fastset seamless refresh rate changes.
+	 *
+	 * vblank_time_lock takes care of all drm_vblank.c stuff, and
+	 * uncore.lock takes care of __intel_get_crtc_scanline() which
+	 * may get called elsewhere as well.
+	 *
+	 * TODO maybe just protect everything (including
+	 * __intel_get_crtc_scanline()) with vblank_time_lock?
+	 * Need to audit everything to make sure it's safe.
+	 */
+	spin_lock_irqsave(&i915->drm.vblank_time_lock, irqflags);
+	spin_lock(&i915->uncore.lock);
+
+	drm_calc_timestamping_constants(&crtc->base, &adjusted_mode);
+
+	crtc->vmax_vblank_start = vmax_vblank_start;
+
+	crtc->mode_flags = crtc_state->mode_flags;
+
+	/*
+	 * The scanline counter increments at the leading edge of hsync.
+	 *
+	 * On most platforms it starts counting from vtotal-1 on the
+	 * first active line. That means the scanline counter value is
+	 * always one less than what we would expect. Ie. just after
+	 * start of vblank, which also occurs at start of hsync (on the
+	 * last active line), the scanline counter will read vblank_start-1.
+	 *
+	 * On gen2 the scanline counter starts counting from 1 instead
+	 * of vtotal-1, so we have to subtract one (or rather add vtotal-1
+	 * to keep the value positive), instead of adding one.
+	 *
+	 * On HSW+ the behaviour of the scanline counter depends on the output
+	 * type. For DP ports it behaves like most other platforms, but on HDMI
+	 * there's an extra 1 line difference. So we need to add two instead of
+	 * one to the value.
+	 *
+	 * On VLV/CHV DSI the scanline counter would appear to increment
+	 * approx. 1/3 of a scanline before start of vblank. Unfortunately
+	 * that means we can't tell whether we're in vblank or not while
+	 * we're on that particular line. We must still set scanline_offset
+	 * to 1 so that the vblank timestamps come out correct when we query
+	 * the scanline counter from within the vblank interrupt handler.
+	 * However if queried just before the start of vblank we'll get an
+	 * answer that's slightly in the future.
+	 */
+	if (DISPLAY_VER(i915) == 2) {
+		int vtotal;
+
+		vtotal = adjusted_mode.crtc_vtotal;
+		if (adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
+			vtotal /= 2;
+
+		crtc->scanline_offset = vtotal - 1;
+	} else if (HAS_DDI(i915) &&
+		   intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
+		crtc->scanline_offset = 2;
+	} else {
+		crtc->scanline_offset = 1;
+	}
+
+	spin_unlock(&i915->uncore.lock);
+	spin_unlock_irqrestore(&i915->drm.vblank_time_lock, irqflags);
+}
diff --git a/drivers/gpu/drm/i915/display/intel_vblank.h b/drivers/gpu/drm/i915/display/intel_vblank.h
index c9fea2c2a990..0884db7e76ae 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.h
+++ b/drivers/gpu/drm/i915/display/intel_vblank.h
@@ -11,6 +11,7 @@
 
 struct drm_crtc;
 struct intel_crtc;
+struct intel_crtc_state;
 
 u32 i915_get_vblank_counter(struct drm_crtc *crtc);
 u32 g4x_get_vblank_counter(struct drm_crtc *crtc);
@@ -19,5 +20,6 @@ bool intel_crtc_get_vblank_timestamp(struct drm_crtc *crtc, int *max_error,
 int intel_get_crtc_scanline(struct intel_crtc *crtc);
 void intel_wait_for_pipe_scanline_stopped(struct intel_crtc *crtc);
 void intel_wait_for_pipe_scanline_moving(struct intel_crtc *crtc);
+void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state);
 
 #endif /* __INTEL_VBLANK_H__ */
-- 
2.39.2

[Intel-gfx] [PATCH 4/4] drm/i915: Extract intel_crtc_scanline_offset()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pull the scanline_offset calculation into its own function. Might
have further use for this later with DSB scanline waits.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_vblank.c | 89 +++++++++++----------
 1 file changed, 48 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
index 48bf3923af11..f8bf9810527d 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.c
+++ b/drivers/gpu/drm/i915/display/intel_vblank.c
@@ -441,6 +441,53 @@ void intel_wait_for_pipe_scanline_moving(struct intel_crtc *crtc)
 	wait_for_pipe_scanline_moving(crtc, true);
 }
 
+static int intel_crtc_scanline_offset(const struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
+	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
+
+	/*
+	 * The scanline counter increments at the leading edge of hsync.
+	 *
+	 * On most platforms it starts counting from vtotal-1 on the
+	 * first active line. That means the scanline counter value is
+	 * always one less than what we would expect. Ie. just after
+	 * start of vblank, which also occurs at start of hsync (on the
+	 * last active line), the scanline counter will read vblank_start-1.
+	 *
+	 * On gen2 the scanline counter starts counting from 1 instead
+	 * of vtotal-1, so we have to subtract one (or rather add vtotal-1
+	 * to keep the value positive), instead of adding one.
+	 *
+	 * On HSW+ the behaviour of the scanline counter depends on the output
+	 * type. For DP ports it behaves like most other platforms, but on HDMI
+	 * there's an extra 1 line difference. So we need to add two instead of
+	 * one to the value.
+	 *
+	 * On VLV/CHV DSI the scanline counter would appear to increment
+	 * approx. 1/3 of a scanline before start of vblank. Unfortunately
+	 * that means we can't tell whether we're in vblank or not while
+	 * we're on that particular line. We must still set scanline_offset
+	 * to 1 so that the vblank timestamps come out correct when we query
+	 * the scanline counter from within the vblank interrupt handler.
+	 * However if queried just before the start of vblank we'll get an
+	 * answer that's slightly in the future.
+	 */
+	if (DISPLAY_VER(i915) == 2) {
+		int vtotal;
+
+		vtotal = adjusted_mode->crtc_vtotal;
+		if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
+			vtotal /= 2;
+
+		return vtotal - 1;
+	} else if (HAS_DDI(i915) && intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
+		return 2;
+	} else {
+		return 1;
+	}
+}
+
 void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
@@ -479,47 +526,7 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
 
 	crtc->mode_flags = crtc_state->mode_flags;
 
-	/*
-	 * The scanline counter increments at the leading edge of hsync.
-	 *
-	 * On most platforms it starts counting from vtotal-1 on the
-	 * first active line. That means the scanline counter value is
-	 * always one less than what we would expect. Ie. just after
-	 * start of vblank, which also occurs at start of hsync (on the
-	 * last active line), the scanline counter will read vblank_start-1.
-	 *
-	 * On gen2 the scanline counter starts counting from 1 instead
-	 * of vtotal-1, so we have to subtract one (or rather add vtotal-1
-	 * to keep the value positive), instead of adding one.
-	 *
-	 * On HSW+ the behaviour of the scanline counter depends on the output
-	 * type. For DP ports it behaves like most other platforms, but on HDMI
-	 * there's an extra 1 line difference. So we need to add two instead of
-	 * one to the value.
-	 *
-	 * On VLV/CHV DSI the scanline counter would appear to increment
-	 * approx. 1/3 of a scanline before start of vblank. Unfortunately
-	 * that means we can't tell whether we're in vblank or not while
-	 * we're on that particular line. We must still set scanline_offset
-	 * to 1 so that the vblank timestamps come out correct when we query
-	 * the scanline counter from within the vblank interrupt handler.
-	 * However if queried just before the start of vblank we'll get an
-	 * answer that's slightly in the future.
-	 */
-	if (DISPLAY_VER(i915) == 2) {
-		int vtotal;
-
-		vtotal = adjusted_mode.crtc_vtotal;
-		if (adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
-			vtotal /= 2;
-
-		crtc->scanline_offset = vtotal - 1;
-	} else if (HAS_DDI(i915) &&
-		   intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
-		crtc->scanline_offset = 2;
-	} else {
-		crtc->scanline_offset = 1;
-	}
+	crtc->scanline_offset = intel_crtc_scanline_offset(crtc_state);
 
 	spin_unlock(&i915->uncore.lock);
 	spin_unlock_irqrestore(&i915->drm.vblank_time_lock, irqflags);
-- 
2.39.2

Re: [Intel-gfx] [PATCH 1/4] drm/i915: Update vblank timestamping stuff on seamless M/N change

[Jani Nikula]

On Mon, 06 Mar 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> When we change the M/N values seamlessly during a fastset we should
> also update the vblank timestamping stuff to make sure the vblank
> timestamp corrections/guesstimations come out exact.
>
> Note that only crtc_clock and framedur_ns can actually end up
> changing here during fastsets. Everything else we touch can
> only change during full modesets.
>
> Technically we should try to do this exactly at the start of
> vblank, but that would require some kind of double buffering
> scheme. Let's skip that for now and just update things right
> after the commit has been submitted to the hardware. This
> means the information will be properly up to date when the
> vblank irq handler goes to work. Only if someone ends up
> querying some vblanky stuff in between the commit and start
> of vblank may we see a slight discrepancy.
>
> Also this same problem really exists for the DRRS downclocking
> stuff. But as that is supposed to be more or less transparent
> to the user, and it only drops to low gear after a long delay
> (1 sec currently) we probably don't have to worry about it.
> Any time something is actively submitting updates DRRS will
> remain in high gear and so the timestamping constants will
> match the hardware state.
>
> Fixes: e6f29923c048 ("drm/i915: Allow M/N change during fastset on bdw+")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_crtc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> index b79a8834559f..41d381bbb57a 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> @@ -686,6 +686,14 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
>  	 */
>  	intel_vrr_send_push(new_crtc_state);
>  
> +	/*
> +	 * Seamless M/N update may need to update frame timings.
> +	 *
> +	 * FIXME Should be synchronized with the start of vblank somehow...
> +	 */
> +	if (new_crtc_state->seamless_m_n && intel_crtc_needs_fastset(new_crtc_state))
> +		intel_crtc_update_active_timings(new_crtc_state);

Side note, do we ensure somewhere that intel_crtc_needs_modeset() &&
intel_crtc_needs_fastset() is never true?

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> +
>  	local_irq_enable();
>  
>  	if (intel_vgpu_active(dev_priv))

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH 2/4] drm/i915: Add belts and suspenders locking for seamless M/N changes

[Jani Nikula]

On Mon, 06 Mar 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Add some (probably overkill) locking to protect the vblank
> timestamping constants updates during seamless M/N fastsets.
>
> As everything should be naturally aligned I think the individual
> pieces should probably end up updating atomically enough. So this
> is only really meant to guarantee everyone sees a consistent whole.
>
> All the drm_vblank.c usage is covered by vblank_time_lock,
> and uncore.lock will take care of __intel_get_crtc_scanline()
> that can also be called from outside the core vblank functionality.

The patch seems to do what it says on the box, but I increasingly
dislike the use of uncore.lock for anything other than the nuts and
bolts of uncore.

BR,
Jani.

>
> Currently only crtc_clock and framedur_ns can change, but in
> the future might fastset also across eg. vtotal/vblank_end
> changes, so let's just grab the locks across the whole thing.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 24 +++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index a1fbdf32bd21..020320468967 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -5908,6 +5908,8 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	struct drm_display_mode adjusted_mode;
> +	int vmax_vblank_start = 0;
> +	unsigned long irqflags;
>  
>  	drm_mode_init(&adjusted_mode, &crtc_state->hw.adjusted_mode);
>  
> @@ -5915,11 +5917,28 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
>  		adjusted_mode.crtc_vtotal = crtc_state->vrr.vmax;
>  		adjusted_mode.crtc_vblank_end = crtc_state->vrr.vmax;
>  		adjusted_mode.crtc_vblank_start = intel_vrr_vmin_vblank_start(crtc_state);
> -		crtc->vmax_vblank_start = intel_vrr_vmax_vblank_start(crtc_state);
> +		vmax_vblank_start = intel_vrr_vmax_vblank_start(crtc_state);
>  	}
>  
> +	/*
> +	 * Belts and suspenders locking to guarantee everyone sees 100%
> +	 * consistent state during fastset seamless refresh rate changes.
> +	 *
> +	 * vblank_time_lock takes care of all drm_vblank.c stuff, and
> +	 * uncore.lock takes care of __intel_get_crtc_scanline() which
> +	 * may get called elsewhere as well.
> +	 *
> +	 * TODO maybe just protect everything (including
> +	 * __intel_get_crtc_scanline()) with vblank_time_lock?
> +	 * Need to audit everything to make sure it's safe.
> +	 */
> +	spin_lock_irqsave(&dev_priv->drm.vblank_time_lock, irqflags);
> +	spin_lock(&dev_priv->uncore.lock);
> +
>  	drm_calc_timestamping_constants(&crtc->base, &adjusted_mode);
>  
> +	crtc->vmax_vblank_start = vmax_vblank_start;
> +
>  	crtc->mode_flags = crtc_state->mode_flags;
>  
>  	/*
> @@ -5963,6 +5982,9 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
>  	} else {
>  		crtc->scanline_offset = 1;
>  	}
> +
> +	spin_unlock(&dev_priv->uncore.lock);
> +	spin_unlock_irqrestore(&dev_priv->drm.vblank_time_lock, irqflags);
>  }
>  
>  /*

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH 3/4] drm/i915: Relocate intel_crtc_update_active_timings()

[Jani Nikula]

On Mon, 06 Mar 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Move intel_crtc_update_active_timings() into intel_vblank.c
> where it more properly belongs.

Wish the function naming could reflect the file name, but hey, if it
declutters intel_display.c it's good!

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>
> Also do the s/dev_priv/i915/ modernization rename while at it.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c  | 84 ------------------
>  drivers/gpu/drm/i915/display/intel_display.h  |  1 -
>  .../drm/i915/display/intel_modeset_setup.c    |  1 +
>  drivers/gpu/drm/i915/display/intel_vblank.c   | 85 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_vblank.h   |  2 +
>  5 files changed, 88 insertions(+), 85 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 020320468967..add3bed3d2e1 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -5903,90 +5903,6 @@ int intel_modeset_all_pipes(struct intel_atomic_state *state,
>  	return 0;
>  }
>  
> -void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
> -{
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	struct drm_display_mode adjusted_mode;
> -	int vmax_vblank_start = 0;
> -	unsigned long irqflags;
> -
> -	drm_mode_init(&adjusted_mode, &crtc_state->hw.adjusted_mode);
> -
> -	if (crtc_state->vrr.enable) {
> -		adjusted_mode.crtc_vtotal = crtc_state->vrr.vmax;
> -		adjusted_mode.crtc_vblank_end = crtc_state->vrr.vmax;
> -		adjusted_mode.crtc_vblank_start = intel_vrr_vmin_vblank_start(crtc_state);
> -		vmax_vblank_start = intel_vrr_vmax_vblank_start(crtc_state);
> -	}
> -
> -	/*
> -	 * Belts and suspenders locking to guarantee everyone sees 100%
> -	 * consistent state during fastset seamless refresh rate changes.
> -	 *
> -	 * vblank_time_lock takes care of all drm_vblank.c stuff, and
> -	 * uncore.lock takes care of __intel_get_crtc_scanline() which
> -	 * may get called elsewhere as well.
> -	 *
> -	 * TODO maybe just protect everything (including
> -	 * __intel_get_crtc_scanline()) with vblank_time_lock?
> -	 * Need to audit everything to make sure it's safe.
> -	 */
> -	spin_lock_irqsave(&dev_priv->drm.vblank_time_lock, irqflags);
> -	spin_lock(&dev_priv->uncore.lock);
> -
> -	drm_calc_timestamping_constants(&crtc->base, &adjusted_mode);
> -
> -	crtc->vmax_vblank_start = vmax_vblank_start;
> -
> -	crtc->mode_flags = crtc_state->mode_flags;
> -
> -	/*
> -	 * The scanline counter increments at the leading edge of hsync.
> -	 *
> -	 * On most platforms it starts counting from vtotal-1 on the
> -	 * first active line. That means the scanline counter value is
> -	 * always one less than what we would expect. Ie. just after
> -	 * start of vblank, which also occurs at start of hsync (on the
> -	 * last active line), the scanline counter will read vblank_start-1.
> -	 *
> -	 * On gen2 the scanline counter starts counting from 1 instead
> -	 * of vtotal-1, so we have to subtract one (or rather add vtotal-1
> -	 * to keep the value positive), instead of adding one.
> -	 *
> -	 * On HSW+ the behaviour of the scanline counter depends on the output
> -	 * type. For DP ports it behaves like most other platforms, but on HDMI
> -	 * there's an extra 1 line difference. So we need to add two instead of
> -	 * one to the value.
> -	 *
> -	 * On VLV/CHV DSI the scanline counter would appear to increment
> -	 * approx. 1/3 of a scanline before start of vblank. Unfortunately
> -	 * that means we can't tell whether we're in vblank or not while
> -	 * we're on that particular line. We must still set scanline_offset
> -	 * to 1 so that the vblank timestamps come out correct when we query
> -	 * the scanline counter from within the vblank interrupt handler.
> -	 * However if queried just before the start of vblank we'll get an
> -	 * answer that's slightly in the future.
> -	 */
> -	if (DISPLAY_VER(dev_priv) == 2) {
> -		int vtotal;
> -
> -		vtotal = adjusted_mode.crtc_vtotal;
> -		if (adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> -			vtotal /= 2;
> -
> -		crtc->scanline_offset = vtotal - 1;
> -	} else if (HAS_DDI(dev_priv) &&
> -		   intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
> -		crtc->scanline_offset = 2;
> -	} else {
> -		crtc->scanline_offset = 1;
> -	}
> -
> -	spin_unlock(&dev_priv->uncore.lock);
> -	spin_unlock_irqrestore(&dev_priv->drm.vblank_time_lock, irqflags);
> -}
> -
>  /*
>   * This implements the workaround described in the "notes" section of the mode
>   * set sequence documentation. When going from no pipes or single pipe to
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> index 50285fb4fcf5..ce97cb72f6d3 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -422,7 +422,6 @@ bool intel_crtc_get_pipe_config(struct intel_crtc_state *crtc_state);
>  bool intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  			       const struct intel_crtc_state *pipe_config,
>  			       bool fastset);
> -void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state);
>  
>  void intel_plane_destroy(struct drm_plane *plane);
>  void i9xx_set_pipeconf(const struct intel_crtc_state *crtc_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> index 60f71e6f0491..c51209a3d36a 100644
> --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> @@ -26,6 +26,7 @@
>  #include "intel_modeset_setup.h"
>  #include "intel_pch_display.h"
>  #include "intel_pm.h"
> +#include "intel_vblank.h"
>  #include "intel_wm.h"
>  #include "skl_watermark.h"
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> index 571f5dda1e66..48bf3923af11 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -8,6 +8,7 @@
>  #include "intel_de.h"
>  #include "intel_display_types.h"
>  #include "intel_vblank.h"
> +#include "intel_vrr.h"
>  
>  /*
>   * This timing diagram depicts the video signal in and
> @@ -439,3 +440,87 @@ void intel_wait_for_pipe_scanline_moving(struct intel_crtc *crtc)
>  {
>  	wait_for_pipe_scanline_moving(crtc, true);
>  }
> +
> +void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> +	struct drm_display_mode adjusted_mode;
> +	int vmax_vblank_start = 0;
> +	unsigned long irqflags;
> +
> +	drm_mode_init(&adjusted_mode, &crtc_state->hw.adjusted_mode);
> +
> +	if (crtc_state->vrr.enable) {
> +		adjusted_mode.crtc_vtotal = crtc_state->vrr.vmax;
> +		adjusted_mode.crtc_vblank_end = crtc_state->vrr.vmax;
> +		adjusted_mode.crtc_vblank_start = intel_vrr_vmin_vblank_start(crtc_state);
> +		vmax_vblank_start = intel_vrr_vmax_vblank_start(crtc_state);
> +	}
> +
> +	/*
> +	 * Belts and suspenders locking to guarantee everyone sees 100%
> +	 * consistent state during fastset seamless refresh rate changes.
> +	 *
> +	 * vblank_time_lock takes care of all drm_vblank.c stuff, and
> +	 * uncore.lock takes care of __intel_get_crtc_scanline() which
> +	 * may get called elsewhere as well.
> +	 *
> +	 * TODO maybe just protect everything (including
> +	 * __intel_get_crtc_scanline()) with vblank_time_lock?
> +	 * Need to audit everything to make sure it's safe.
> +	 */
> +	spin_lock_irqsave(&i915->drm.vblank_time_lock, irqflags);
> +	spin_lock(&i915->uncore.lock);
> +
> +	drm_calc_timestamping_constants(&crtc->base, &adjusted_mode);
> +
> +	crtc->vmax_vblank_start = vmax_vblank_start;
> +
> +	crtc->mode_flags = crtc_state->mode_flags;
> +
> +	/*
> +	 * The scanline counter increments at the leading edge of hsync.
> +	 *
> +	 * On most platforms it starts counting from vtotal-1 on the
> +	 * first active line. That means the scanline counter value is
> +	 * always one less than what we would expect. Ie. just after
> +	 * start of vblank, which also occurs at start of hsync (on the
> +	 * last active line), the scanline counter will read vblank_start-1.
> +	 *
> +	 * On gen2 the scanline counter starts counting from 1 instead
> +	 * of vtotal-1, so we have to subtract one (or rather add vtotal-1
> +	 * to keep the value positive), instead of adding one.
> +	 *
> +	 * On HSW+ the behaviour of the scanline counter depends on the output
> +	 * type. For DP ports it behaves like most other platforms, but on HDMI
> +	 * there's an extra 1 line difference. So we need to add two instead of
> +	 * one to the value.
> +	 *
> +	 * On VLV/CHV DSI the scanline counter would appear to increment
> +	 * approx. 1/3 of a scanline before start of vblank. Unfortunately
> +	 * that means we can't tell whether we're in vblank or not while
> +	 * we're on that particular line. We must still set scanline_offset
> +	 * to 1 so that the vblank timestamps come out correct when we query
> +	 * the scanline counter from within the vblank interrupt handler.
> +	 * However if queried just before the start of vblank we'll get an
> +	 * answer that's slightly in the future.
> +	 */
> +	if (DISPLAY_VER(i915) == 2) {
> +		int vtotal;
> +
> +		vtotal = adjusted_mode.crtc_vtotal;
> +		if (adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> +			vtotal /= 2;
> +
> +		crtc->scanline_offset = vtotal - 1;
> +	} else if (HAS_DDI(i915) &&
> +		   intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
> +		crtc->scanline_offset = 2;
> +	} else {
> +		crtc->scanline_offset = 1;
> +	}
> +
> +	spin_unlock(&i915->uncore.lock);
> +	spin_unlock_irqrestore(&i915->drm.vblank_time_lock, irqflags);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.h b/drivers/gpu/drm/i915/display/intel_vblank.h
> index c9fea2c2a990..0884db7e76ae 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.h
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.h
> @@ -11,6 +11,7 @@
>  
>  struct drm_crtc;
>  struct intel_crtc;
> +struct intel_crtc_state;
>  
>  u32 i915_get_vblank_counter(struct drm_crtc *crtc);
>  u32 g4x_get_vblank_counter(struct drm_crtc *crtc);
> @@ -19,5 +20,6 @@ bool intel_crtc_get_vblank_timestamp(struct drm_crtc *crtc, int *max_error,
>  int intel_get_crtc_scanline(struct intel_crtc *crtc);
>  void intel_wait_for_pipe_scanline_stopped(struct intel_crtc *crtc);
>  void intel_wait_for_pipe_scanline_moving(struct intel_crtc *crtc);
> +void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state);
>  
>  #endif /* __INTEL_VBLANK_H__ */

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Extract intel_crtc_scanline_offset()

[Jani Nikula]

On Mon, 06 Mar 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Pull the scanline_offset calculation into its own function. Might
> have further use for this later with DSB scanline waits.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_vblank.c | 89 +++++++++++----------
>  1 file changed, 48 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> index 48bf3923af11..f8bf9810527d 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -441,6 +441,53 @@ void intel_wait_for_pipe_scanline_moving(struct intel_crtc *crtc)
>  	wait_for_pipe_scanline_moving(crtc, true);
>  }
>  
> +static int intel_crtc_scanline_offset(const struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> +	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> +
> +	/*
> +	 * The scanline counter increments at the leading edge of hsync.
> +	 *
> +	 * On most platforms it starts counting from vtotal-1 on the
> +	 * first active line. That means the scanline counter value is
> +	 * always one less than what we would expect. Ie. just after
> +	 * start of vblank, which also occurs at start of hsync (on the
> +	 * last active line), the scanline counter will read vblank_start-1.
> +	 *
> +	 * On gen2 the scanline counter starts counting from 1 instead
> +	 * of vtotal-1, so we have to subtract one (or rather add vtotal-1
> +	 * to keep the value positive), instead of adding one.
> +	 *
> +	 * On HSW+ the behaviour of the scanline counter depends on the output
> +	 * type. For DP ports it behaves like most other platforms, but on HDMI
> +	 * there's an extra 1 line difference. So we need to add two instead of
> +	 * one to the value.
> +	 *
> +	 * On VLV/CHV DSI the scanline counter would appear to increment
> +	 * approx. 1/3 of a scanline before start of vblank. Unfortunately
> +	 * that means we can't tell whether we're in vblank or not while
> +	 * we're on that particular line. We must still set scanline_offset
> +	 * to 1 so that the vblank timestamps come out correct when we query
> +	 * the scanline counter from within the vblank interrupt handler.
> +	 * However if queried just before the start of vblank we'll get an
> +	 * answer that's slightly in the future.
> +	 */
> +	if (DISPLAY_VER(i915) == 2) {
> +		int vtotal;
> +
> +		vtotal = adjusted_mode->crtc_vtotal;
> +		if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
> +			vtotal /= 2;
> +
> +		return vtotal - 1;
> +	} else if (HAS_DDI(i915) && intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
> +		return 2;
> +	} else {
> +		return 1;
> +	}
> +}
> +
>  void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> @@ -479,47 +526,7 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
>  
>  	crtc->mode_flags = crtc_state->mode_flags;
>  
> -	/*
> -	 * The scanline counter increments at the leading edge of hsync.
> -	 *
> -	 * On most platforms it starts counting from vtotal-1 on the
> -	 * first active line. That means the scanline counter value is
> -	 * always one less than what we would expect. Ie. just after
> -	 * start of vblank, which also occurs at start of hsync (on the
> -	 * last active line), the scanline counter will read vblank_start-1.
> -	 *
> -	 * On gen2 the scanline counter starts counting from 1 instead
> -	 * of vtotal-1, so we have to subtract one (or rather add vtotal-1
> -	 * to keep the value positive), instead of adding one.
> -	 *
> -	 * On HSW+ the behaviour of the scanline counter depends on the output
> -	 * type. For DP ports it behaves like most other platforms, but on HDMI
> -	 * there's an extra 1 line difference. So we need to add two instead of
> -	 * one to the value.
> -	 *
> -	 * On VLV/CHV DSI the scanline counter would appear to increment
> -	 * approx. 1/3 of a scanline before start of vblank. Unfortunately
> -	 * that means we can't tell whether we're in vblank or not while
> -	 * we're on that particular line. We must still set scanline_offset
> -	 * to 1 so that the vblank timestamps come out correct when we query
> -	 * the scanline counter from within the vblank interrupt handler.
> -	 * However if queried just before the start of vblank we'll get an
> -	 * answer that's slightly in the future.
> -	 */
> -	if (DISPLAY_VER(i915) == 2) {
> -		int vtotal;
> -
> -		vtotal = adjusted_mode.crtc_vtotal;
> -		if (adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> -			vtotal /= 2;
> -
> -		crtc->scanline_offset = vtotal - 1;
> -	} else if (HAS_DDI(i915) &&
> -		   intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
> -		crtc->scanline_offset = 2;
> -	} else {
> -		crtc->scanline_offset = 1;
> -	}
> +	crtc->scanline_offset = intel_crtc_scanline_offset(crtc_state);
>  
>  	spin_unlock(&i915->uncore.lock);
>  	spin_unlock_irqrestore(&i915->drm.vblank_time_lock, irqflags);

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH 1/4] drm/i915: Update vblank timestamping stuff on seamless M/N change

[Ville Syrjälä]

On Tue, Mar 07, 2023 at 02:16:48PM +0200, Jani Nikula wrote:
> On Mon, 06 Mar 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > When we change the M/N values seamlessly during a fastset we should
> > also update the vblank timestamping stuff to make sure the vblank
> > timestamp corrections/guesstimations come out exact.
> >
> > Note that only crtc_clock and framedur_ns can actually end up
> > changing here during fastsets. Everything else we touch can
> > only change during full modesets.
> >
> > Technically we should try to do this exactly at the start of
> > vblank, but that would require some kind of double buffering
> > scheme. Let's skip that for now and just update things right
> > after the commit has been submitted to the hardware. This
> > means the information will be properly up to date when the
> > vblank irq handler goes to work. Only if someone ends up
> > querying some vblanky stuff in between the commit and start
> > of vblank may we see a slight discrepancy.
> >
> > Also this same problem really exists for the DRRS downclocking
> > stuff. But as that is supposed to be more or less transparent
> > to the user, and it only drops to low gear after a long delay
> > (1 sec currently) we probably don't have to worry about it.
> > Any time something is actively submitting updates DRRS will
> > remain in high gear and so the timestamping constants will
> > match the hardware state.
> >
> > Fixes: e6f29923c048 ("drm/i915: Allow M/N change during fastset on bdw+")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_crtc.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> > index b79a8834559f..41d381bbb57a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> > @@ -686,6 +686,14 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
> >  	 */
> >  	intel_vrr_send_push(new_crtc_state);
> >  
> > +	/*
> > +	 * Seamless M/N update may need to update frame timings.
> > +	 *
> > +	 * FIXME Should be synchronized with the start of vblank somehow...
> > +	 */
> > +	if (new_crtc_state->seamless_m_n && intel_crtc_needs_fastset(new_crtc_state))
> > +		intel_crtc_update_active_timings(new_crtc_state);
> 
> Side note, do we ensure somewhere that intel_crtc_needs_modeset() &&
> intel_crtc_needs_fastset() is never true?

commit 7de5b6b54630 ("drm/i915: Don't flag both full modeset and fastset
at the same time")

> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Thanks

> 
> > +
> >  	local_irq_enable();
> >  
> >  	if (intel_vgpu_active(dev_priv))
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 2/4] drm/i915: Add belts and suspenders locking for seamless M/N changes

[Ville Syrjälä]

On Tue, Mar 07, 2023 at 02:24:08PM +0200, Jani Nikula wrote:
> On Mon, 06 Mar 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Add some (probably overkill) locking to protect the vblank
> > timestamping constants updates during seamless M/N fastsets.
> >
> > As everything should be naturally aligned I think the individual
> > pieces should probably end up updating atomically enough. So this
> > is only really meant to guarantee everyone sees a consistent whole.
> >
> > All the drm_vblank.c usage is covered by vblank_time_lock,
> > and uncore.lock will take care of __intel_get_crtc_scanline()
> > that can also be called from outside the core vblank functionality.
> 
> The patch seems to do what it says on the box, but I increasingly
> dislike the use of uncore.lock for anything other than the nuts and
> bolts of uncore.

Yeah, it's not really great. Hence the TODO I left behind there.

> 
> BR,
> Jani.
> 
> >
> > Currently only crtc_clock and framedur_ns can change, but in
> > the future might fastset also across eg. vtotal/vblank_end
> > changes, so let's just grab the locks across the whole thing.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 24 +++++++++++++++++++-
> >  1 file changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index a1fbdf32bd21..020320468967 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -5908,6 +5908,8 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >  	struct drm_display_mode adjusted_mode;
> > +	int vmax_vblank_start = 0;
> > +	unsigned long irqflags;
> >  
> >  	drm_mode_init(&adjusted_mode, &crtc_state->hw.adjusted_mode);
> >  
> > @@ -5915,11 +5917,28 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
> >  		adjusted_mode.crtc_vtotal = crtc_state->vrr.vmax;
> >  		adjusted_mode.crtc_vblank_end = crtc_state->vrr.vmax;
> >  		adjusted_mode.crtc_vblank_start = intel_vrr_vmin_vblank_start(crtc_state);
> > -		crtc->vmax_vblank_start = intel_vrr_vmax_vblank_start(crtc_state);
> > +		vmax_vblank_start = intel_vrr_vmax_vblank_start(crtc_state);
> >  	}
> >  
> > +	/*
> > +	 * Belts and suspenders locking to guarantee everyone sees 100%
> > +	 * consistent state during fastset seamless refresh rate changes.
> > +	 *
> > +	 * vblank_time_lock takes care of all drm_vblank.c stuff, and
> > +	 * uncore.lock takes care of __intel_get_crtc_scanline() which
> > +	 * may get called elsewhere as well.
> > +	 *
> > +	 * TODO maybe just protect everything (including
> > +	 * __intel_get_crtc_scanline()) with vblank_time_lock?
> > +	 * Need to audit everything to make sure it's safe.
> > +	 */
> > +	spin_lock_irqsave(&dev_priv->drm.vblank_time_lock, irqflags);
> > +	spin_lock(&dev_priv->uncore.lock);
> > +
> >  	drm_calc_timestamping_constants(&crtc->base, &adjusted_mode);
> >  
> > +	crtc->vmax_vblank_start = vmax_vblank_start;
> > +
> >  	crtc->mode_flags = crtc_state->mode_flags;
> >  
> >  	/*
> > @@ -5963,6 +5982,9 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
> >  	} else {
> >  		crtc->scanline_offset = 1;
> >  	}
> > +
> > +	spin_unlock(&dev_priv->uncore.lock);
> > +	spin_unlock_irqrestore(&dev_priv->drm.vblank_time_lock, irqflags);
> >  }
> >  
> >  /*
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 2/4] drm/i915: Add belts and suspenders locking for seamless M/N changes

[Jani Nikula]

On Tue, 07 Mar 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Mar 07, 2023 at 02:24:08PM +0200, Jani Nikula wrote:
>> On Mon, 06 Mar 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > Add some (probably overkill) locking to protect the vblank
>> > timestamping constants updates during seamless M/N fastsets.
>> >
>> > As everything should be naturally aligned I think the individual
>> > pieces should probably end up updating atomically enough. So this
>> > is only really meant to guarantee everyone sees a consistent whole.
>> >
>> > All the drm_vblank.c usage is covered by vblank_time_lock,
>> > and uncore.lock will take care of __intel_get_crtc_scanline()
>> > that can also be called from outside the core vblank functionality.
>> 
>> The patch seems to do what it says on the box, but I increasingly
>> dislike the use of uncore.lock for anything other than the nuts and
>> bolts of uncore.
>
> Yeah, it's not really great. Hence the TODO I left behind there.

Okay,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>
>> 
>> BR,
>> Jani.
>> 
>> >
>> > Currently only crtc_clock and framedur_ns can change, but in
>> > the future might fastset also across eg. vtotal/vblank_end
>> > changes, so let's just grab the locks across the whole thing.
>> >
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_display.c | 24 +++++++++++++++++++-
>> >  1 file changed, 23 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> > index a1fbdf32bd21..020320468967 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> > @@ -5908,6 +5908,8 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
>> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> >  	struct drm_display_mode adjusted_mode;
>> > +	int vmax_vblank_start = 0;
>> > +	unsigned long irqflags;
>> >  
>> >  	drm_mode_init(&adjusted_mode, &crtc_state->hw.adjusted_mode);
>> >  
>> > @@ -5915,11 +5917,28 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
>> >  		adjusted_mode.crtc_vtotal = crtc_state->vrr.vmax;
>> >  		adjusted_mode.crtc_vblank_end = crtc_state->vrr.vmax;
>> >  		adjusted_mode.crtc_vblank_start = intel_vrr_vmin_vblank_start(crtc_state);
>> > -		crtc->vmax_vblank_start = intel_vrr_vmax_vblank_start(crtc_state);
>> > +		vmax_vblank_start = intel_vrr_vmax_vblank_start(crtc_state);
>> >  	}
>> >  
>> > +	/*
>> > +	 * Belts and suspenders locking to guarantee everyone sees 100%
>> > +	 * consistent state during fastset seamless refresh rate changes.
>> > +	 *
>> > +	 * vblank_time_lock takes care of all drm_vblank.c stuff, and
>> > +	 * uncore.lock takes care of __intel_get_crtc_scanline() which
>> > +	 * may get called elsewhere as well.
>> > +	 *
>> > +	 * TODO maybe just protect everything (including
>> > +	 * __intel_get_crtc_scanline()) with vblank_time_lock?
>> > +	 * Need to audit everything to make sure it's safe.
>> > +	 */
>> > +	spin_lock_irqsave(&dev_priv->drm.vblank_time_lock, irqflags);
>> > +	spin_lock(&dev_priv->uncore.lock);
>> > +
>> >  	drm_calc_timestamping_constants(&crtc->base, &adjusted_mode);
>> >  
>> > +	crtc->vmax_vblank_start = vmax_vblank_start;
>> > +
>> >  	crtc->mode_flags = crtc_state->mode_flags;
>> >  
>> >  	/*
>> > @@ -5963,6 +5982,9 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
>> >  	} else {
>> >  		crtc->scanline_offset = 1;
>> >  	}
>> > +
>> > +	spin_unlock(&dev_priv->uncore.lock);
>> > +	spin_unlock_irqrestore(&dev_priv->drm.vblank_time_lock, irqflags);
>> >  }
>> >  
>> >  /*
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH 1/4] drm/i915: Update vblank timestamping stuff on seamless M/N change

[Golani, Mitulkumar Ajitkumar]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville
> Syrjala
> Sent: 06 March 2023 20:59
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 1/4] drm/i915: Update vblank timestamping stuff
> on seamless M/N change
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> When we change the M/N values seamlessly during a fastset we should also
> update the vblank timestamping stuff to make sure the vblank timestamp
> corrections/guesstimations come out exact.
> 
> Note that only crtc_clock and framedur_ns can actually end up changing here
> during fastsets. Everything else we touch can only change during full
> modesets.
> 
> Technically we should try to do this exactly at the start of vblank, but that
> would require some kind of double buffering scheme. Let's skip that for now
> and just update things right after the commit has been submitted to the
> hardware. This means the information will be properly up to date when the
> vblank irq handler goes to work. Only if someone ends up querying some
> vblanky stuff in between the commit and start of vblank may we see a slight
> discrepancy.
> 
> Also this same problem really exists for the DRRS downclocking stuff. But as
> that is supposed to be more or less transparent to the user, and it only drops
> to low gear after a long delay
> (1 sec currently) we probably don't have to worry about it.
> Any time something is actively submitting updates DRRS will remain in high
> gear and so the timestamping constants will match the hardware state.
> 
> Fixes: e6f29923c048 ("drm/i915: Allow M/N change during fastset on bdw+")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_crtc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c
> b/drivers/gpu/drm/i915/display/intel_crtc.c
> index b79a8834559f..41d381bbb57a 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> @@ -686,6 +686,14 @@ void intel_pipe_update_end(struct intel_crtc_state
> *new_crtc_state)
>  	 */
>  	intel_vrr_send_push(new_crtc_state);
> 
> +	/*
> +	 * Seamless M/N update may need to update frame timings.
> +	 *
> +	 * FIXME Should be synchronized with the start of vblank somehow...
> +	 */
> +	if (new_crtc_state->seamless_m_n &&
> intel_crtc_needs_fastset(new_crtc_state))
> +		intel_crtc_update_active_timings(new_crtc_state);
> +
Hi Ville,

changes LGTM. Although have a question, are we suspecting any timing param changes after Push bit is sent ?

Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>

Thanks
>  	local_irq_enable();
> 
>  	if (intel_vgpu_active(dev_priv))
> --
> 2.39.2

Re: [Intel-gfx] [PATCH 3/4] drm/i915: Relocate intel_crtc_update_active_timings()

[Golani, Mitulkumar Ajitkumar]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville
> Syrjala
> Sent: 06 March 2023 20:59
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 3/4] drm/i915: Relocate
> intel_crtc_update_active_timings()
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Move intel_crtc_update_active_timings() into intel_vblank.c where it more
> properly belongs.
> 
> Also do the s/dev_priv/i915/ modernization rename while at it.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c  | 84 ------------------
> drivers/gpu/drm/i915/display/intel_display.h  |  1 -
>  .../drm/i915/display/intel_modeset_setup.c    |  1 +
>  drivers/gpu/drm/i915/display/intel_vblank.c   | 85 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_vblank.h   |  2 +
>  5 files changed, 88 insertions(+), 85 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 020320468967..add3bed3d2e1 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -5903,90 +5903,6 @@ int intel_modeset_all_pipes(struct
> intel_atomic_state *state,
>  	return 0;
>  }
> 
> -void intel_crtc_update_active_timings(const struct intel_crtc_state
> *crtc_state) -{
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	struct drm_display_mode adjusted_mode;
> -	int vmax_vblank_start = 0;
> -	unsigned long irqflags;
> -
> -	drm_mode_init(&adjusted_mode, &crtc_state->hw.adjusted_mode);
> -
> -	if (crtc_state->vrr.enable) {
> -		adjusted_mode.crtc_vtotal = crtc_state->vrr.vmax;
> -		adjusted_mode.crtc_vblank_end = crtc_state->vrr.vmax;
> -		adjusted_mode.crtc_vblank_start =
> intel_vrr_vmin_vblank_start(crtc_state);
> -		vmax_vblank_start =
> intel_vrr_vmax_vblank_start(crtc_state);
> -	}
> -
> -	/*
> -	 * Belts and suspenders locking to guarantee everyone sees 100%
> -	 * consistent state during fastset seamless refresh rate changes.
> -	 *
> -	 * vblank_time_lock takes care of all drm_vblank.c stuff, and
> -	 * uncore.lock takes care of __intel_get_crtc_scanline() which
> -	 * may get called elsewhere as well.
> -	 *
> -	 * TODO maybe just protect everything (including
> -	 * __intel_get_crtc_scanline()) with vblank_time_lock?
> -	 * Need to audit everything to make sure it's safe.
> -	 */
> -	spin_lock_irqsave(&dev_priv->drm.vblank_time_lock, irqflags);
> -	spin_lock(&dev_priv->uncore.lock);
> -
> -	drm_calc_timestamping_constants(&crtc->base, &adjusted_mode);
> -
> -	crtc->vmax_vblank_start = vmax_vblank_start;
> -
> -	crtc->mode_flags = crtc_state->mode_flags;
> -
> -	/*
> -	 * The scanline counter increments at the leading edge of hsync.
> -	 *
> -	 * On most platforms it starts counting from vtotal-1 on the
> -	 * first active line. That means the scanline counter value is
> -	 * always one less than what we would expect. Ie. just after
> -	 * start of vblank, which also occurs at start of hsync (on the
> -	 * last active line), the scanline counter will read vblank_start-1.
> -	 *
> -	 * On gen2 the scanline counter starts counting from 1 instead
> -	 * of vtotal-1, so we have to subtract one (or rather add vtotal-1
> -	 * to keep the value positive), instead of adding one.
> -	 *
> -	 * On HSW+ the behaviour of the scanline counter depends on the
> output
> -	 * type. For DP ports it behaves like most other platforms, but on
> HDMI
> -	 * there's an extra 1 line difference. So we need to add two instead of
> -	 * one to the value.
> -	 *
> -	 * On VLV/CHV DSI the scanline counter would appear to increment
> -	 * approx. 1/3 of a scanline before start of vblank. Unfortunately
> -	 * that means we can't tell whether we're in vblank or not while
> -	 * we're on that particular line. We must still set scanline_offset
> -	 * to 1 so that the vblank timestamps come out correct when we
> query
> -	 * the scanline counter from within the vblank interrupt handler.
> -	 * However if queried just before the start of vblank we'll get an
> -	 * answer that's slightly in the future.
> -	 */
> -	if (DISPLAY_VER(dev_priv) == 2) {
> -		int vtotal;
> -
> -		vtotal = adjusted_mode.crtc_vtotal;
> -		if (adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> -			vtotal /= 2;
> -
> -		crtc->scanline_offset = vtotal - 1;
> -	} else if (HAS_DDI(dev_priv) &&
> -		   intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
> -		crtc->scanline_offset = 2;
> -	} else {
> -		crtc->scanline_offset = 1;
> -	}
> -
> -	spin_unlock(&dev_priv->uncore.lock);
> -	spin_unlock_irqrestore(&dev_priv->drm.vblank_time_lock, irqflags);
> -}
> -
>  /*
>   * This implements the workaround described in the "notes" section of the
> mode
>   * set sequence documentation. When going from no pipes or single pipe to
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h
> b/drivers/gpu/drm/i915/display/intel_display.h
> index 50285fb4fcf5..ce97cb72f6d3 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -422,7 +422,6 @@ bool intel_crtc_get_pipe_config(struct
> intel_crtc_state *crtc_state);  bool intel_pipe_config_compare(const struct
> intel_crtc_state *current_config,
>  			       const struct intel_crtc_state *pipe_config,
>  			       bool fastset);
> -void intel_crtc_update_active_timings(const struct intel_crtc_state
> *crtc_state);
> 
>  void intel_plane_destroy(struct drm_plane *plane);  void
> i9xx_set_pipeconf(const struct intel_crtc_state *crtc_state); diff --git
> a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> index 60f71e6f0491..c51209a3d36a 100644
> --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> @@ -26,6 +26,7 @@
>  #include "intel_modeset_setup.h"
>  #include "intel_pch_display.h"
>  #include "intel_pm.h"
> +#include "intel_vblank.h"
>  #include "intel_wm.h"
>  #include "skl_watermark.h"
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c
> b/drivers/gpu/drm/i915/display/intel_vblank.c
> index 571f5dda1e66..48bf3923af11 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -8,6 +8,7 @@
>  #include "intel_de.h"
>  #include "intel_display_types.h"
>  #include "intel_vblank.h"
> +#include "intel_vrr.h"
> 
>  /*
>   * This timing diagram depicts the video signal in and @@ -439,3 +440,87
> @@ void intel_wait_for_pipe_scanline_moving(struct intel_crtc *crtc)  {
>  	wait_for_pipe_scanline_moving(crtc, true);  }
> +
> +void intel_crtc_update_active_timings(const struct intel_crtc_state
> +*crtc_state) {
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> +	struct drm_display_mode adjusted_mode;
> +	int vmax_vblank_start = 0;
> +	unsigned long irqflags;
> +
> +	drm_mode_init(&adjusted_mode, &crtc_state->hw.adjusted_mode);
> +
> +	if (crtc_state->vrr.enable) {
> +		adjusted_mode.crtc_vtotal = crtc_state->vrr.vmax;
> +		adjusted_mode.crtc_vblank_end = crtc_state->vrr.vmax;
> +		adjusted_mode.crtc_vblank_start =
> intel_vrr_vmin_vblank_start(crtc_state);
> +		vmax_vblank_start =
> intel_vrr_vmax_vblank_start(crtc_state);
> +	}
> +
> +	/*
> +	 * Belts and suspenders locking to guarantee everyone sees 100%
> +	 * consistent state during fastset seamless refresh rate changes.
> +	 *
> +	 * vblank_time_lock takes care of all drm_vblank.c stuff, and
> +	 * uncore.lock takes care of __intel_get_crtc_scanline() which
> +	 * may get called elsewhere as well.
> +	 *
> +	 * TODO maybe just protect everything (including
> +	 * __intel_get_crtc_scanline()) with vblank_time_lock?
> +	 * Need to audit everything to make sure it's safe.
> +	 */
> +	spin_lock_irqsave(&i915->drm.vblank_time_lock, irqflags);
> +	spin_lock(&i915->uncore.lock);
> +
> +	drm_calc_timestamping_constants(&crtc->base, &adjusted_mode);
> +
> +	crtc->vmax_vblank_start = vmax_vblank_start;
> +
> +	crtc->mode_flags = crtc_state->mode_flags;
> +
> +	/*
> +	 * The scanline counter increments at the leading edge of hsync.
> +	 *
> +	 * On most platforms it starts counting from vtotal-1 on the
> +	 * first active line. That means the scanline counter value is
> +	 * always one less than what we would expect. Ie. just after
> +	 * start of vblank, which also occurs at start of hsync (on the
> +	 * last active line), the scanline counter will read vblank_start-1.
> +	 *
> +	 * On gen2 the scanline counter starts counting from 1 instead
> +	 * of vtotal-1, so we have to subtract one (or rather add vtotal-1
> +	 * to keep the value positive), instead of adding one.
> +	 *
> +	 * On HSW+ the behaviour of the scanline counter depends on the
> output
> +	 * type. For DP ports it behaves like most other platforms, but on
> HDMI
> +	 * there's an extra 1 line difference. So we need to add two instead of
> +	 * one to the value.
> +	 *
> +	 * On VLV/CHV DSI the scanline counter would appear to increment
> +	 * approx. 1/3 of a scanline before start of vblank. Unfortunately
> +	 * that means we can't tell whether we're in vblank or not while
> +	 * we're on that particular line. We must still set scanline_offset
> +	 * to 1 so that the vblank timestamps come out correct when we
> query
> +	 * the scanline counter from within the vblank interrupt handler.
> +	 * However if queried just before the start of vblank we'll get an
> +	 * answer that's slightly in the future.
> +	 */
> +	if (DISPLAY_VER(i915) == 2) {
> +		int vtotal;
> +
> +		vtotal = adjusted_mode.crtc_vtotal;
> +		if (adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> +			vtotal /= 2;
> +
> +		crtc->scanline_offset = vtotal - 1;
> +	} else if (HAS_DDI(i915) &&
> +		   intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
> +		crtc->scanline_offset = 2;
> +	} else {
> +		crtc->scanline_offset = 1;
> +	}
> +
> +	spin_unlock(&i915->uncore.lock);
> +	spin_unlock_irqrestore(&i915->drm.vblank_time_lock, irqflags); }

Changes LGTM
Thanks

Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>

> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.h
> b/drivers/gpu/drm/i915/display/intel_vblank.h
> index c9fea2c2a990..0884db7e76ae 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.h
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.h
> @@ -11,6 +11,7 @@
> 
>  struct drm_crtc;
>  struct intel_crtc;
> +struct intel_crtc_state;
> 
>  u32 i915_get_vblank_counter(struct drm_crtc *crtc);
>  u32 g4x_get_vblank_counter(struct drm_crtc *crtc); @@ -19,5 +20,6 @@
> bool intel_crtc_get_vblank_timestamp(struct drm_crtc *crtc, int *max_error,
> int intel_get_crtc_scanline(struct intel_crtc *crtc);  void
> intel_wait_for_pipe_scanline_stopped(struct intel_crtc *crtc);  void
> intel_wait_for_pipe_scanline_moving(struct intel_crtc *crtc);
> +void intel_crtc_update_active_timings(const struct intel_crtc_state
> +*crtc_state);
> 
>  #endif /* __INTEL_VBLANK_H__ */
> --
> 2.39.2

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Extract intel_crtc_scanline_offset()

[Golani, Mitulkumar Ajitkumar]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville
> Syrjala
> Sent: 06 March 2023 20:59
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 4/4] drm/i915: Extract intel_crtc_scanline_offset()
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Pull the scanline_offset calculation into its own function. Might have further
> use for this later with DSB scanline waits.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_vblank.c | 89 +++++++++++----------
>  1 file changed, 48 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c
> b/drivers/gpu/drm/i915/display/intel_vblank.c
> index 48bf3923af11..f8bf9810527d 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -441,6 +441,53 @@ void intel_wait_for_pipe_scanline_moving(struct
> intel_crtc *crtc)
>  	wait_for_pipe_scanline_moving(crtc, true);  }
> 
> +static int intel_crtc_scanline_offset(const struct intel_crtc_state
> +*crtc_state) {
> +	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> +	const struct drm_display_mode *adjusted_mode =
> +&crtc_state->hw.adjusted_mode;
> +
> +	/*
> +	 * The scanline counter increments at the leading edge of hsync.
> +	 *
> +	 * On most platforms it starts counting from vtotal-1 on the
> +	 * first active line. That means the scanline counter value is
> +	 * always one less than what we would expect. Ie. just after
> +	 * start of vblank, which also occurs at start of hsync (on the
> +	 * last active line), the scanline counter will read vblank_start-1.
> +	 *
> +	 * On gen2 the scanline counter starts counting from 1 instead
> +	 * of vtotal-1, so we have to subtract one (or rather add vtotal-1
> +	 * to keep the value positive), instead of adding one.
> +	 *
> +	 * On HSW+ the behaviour of the scanline counter depends on the
> output
> +	 * type. For DP ports it behaves like most other platforms, but on
> HDMI
> +	 * there's an extra 1 line difference. So we need to add two instead of
> +	 * one to the value.
> +	 *
> +	 * On VLV/CHV DSI the scanline counter would appear to increment
> +	 * approx. 1/3 of a scanline before start of vblank. Unfortunately
> +	 * that means we can't tell whether we're in vblank or not while
> +	 * we're on that particular line. We must still set scanline_offset
> +	 * to 1 so that the vblank timestamps come out correct when we
> query
> +	 * the scanline counter from within the vblank interrupt handler.
> +	 * However if queried just before the start of vblank we'll get an
> +	 * answer that's slightly in the future.
> +	 */
> +	if (DISPLAY_VER(i915) == 2) {
> +		int vtotal;
> +
> +		vtotal = adjusted_mode->crtc_vtotal;
> +		if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
> +			vtotal /= 2;
> +
> +		return vtotal - 1;
> +	} else if (HAS_DDI(i915) && intel_crtc_has_type(crtc_state,
> INTEL_OUTPUT_HDMI)) {
> +		return 2;
> +	} else {
> +		return 1;
> +	}
> +}
> +

changes LGTM. 
Thanks

Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>

>  void intel_crtc_update_active_timings(const struct intel_crtc_state
> *crtc_state)  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> @@ -479,47 +526,7 @@ void intel_crtc_update_active_timings(const struct
> intel_crtc_state *crtc_state)
> 
>  	crtc->mode_flags = crtc_state->mode_flags;
> 
> -	/*
> -	 * The scanline counter increments at the leading edge of hsync.
> -	 *
> -	 * On most platforms it starts counting from vtotal-1 on the
> -	 * first active line. That means the scanline counter value is
> -	 * always one less than what we would expect. Ie. just after
> -	 * start of vblank, which also occurs at start of hsync (on the
> -	 * last active line), the scanline counter will read vblank_start-1.
> -	 *
> -	 * On gen2 the scanline counter starts counting from 1 instead
> -	 * of vtotal-1, so we have to subtract one (or rather add vtotal-1
> -	 * to keep the value positive), instead of adding one.
> -	 *
> -	 * On HSW+ the behaviour of the scanline counter depends on the
> output
> -	 * type. For DP ports it behaves like most other platforms, but on
> HDMI
> -	 * there's an extra 1 line difference. So we need to add two instead of
> -	 * one to the value.
> -	 *
> -	 * On VLV/CHV DSI the scanline counter would appear to increment
> -	 * approx. 1/3 of a scanline before start of vblank. Unfortunately
> -	 * that means we can't tell whether we're in vblank or not while
> -	 * we're on that particular line. We must still set scanline_offset
> -	 * to 1 so that the vblank timestamps come out correct when we
> query
> -	 * the scanline counter from within the vblank interrupt handler.
> -	 * However if queried just before the start of vblank we'll get an
> -	 * answer that's slightly in the future.
> -	 */
> -	if (DISPLAY_VER(i915) == 2) {
> -		int vtotal;
> -
> -		vtotal = adjusted_mode.crtc_vtotal;
> -		if (adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> -			vtotal /= 2;
> -
> -		crtc->scanline_offset = vtotal - 1;
> -	} else if (HAS_DDI(i915) &&
> -		   intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
> -		crtc->scanline_offset = 2;
> -	} else {
> -		crtc->scanline_offset = 1;
> -	}
> +	crtc->scanline_offset = intel_crtc_scanline_offset(crtc_state);
> 
>  	spin_unlock(&i915->uncore.lock);
>  	spin_unlock_irqrestore(&i915->drm.vblank_time_lock, irqflags);
> --
> 2.39.2

Re: [Intel-gfx] [PATCH 1/4] drm/i915: Update vblank timestamping stuff on seamless M/N change

[Ville Syrjälä]

On Fri, Mar 10, 2023 at 07:01:18PM +0000, Golani, Mitulkumar Ajitkumar wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville
> > Syrjala
> > Sent: 06 March 2023 20:59
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [Intel-gfx] [PATCH 1/4] drm/i915: Update vblank timestamping stuff
> > on seamless M/N change
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > When we change the M/N values seamlessly during a fastset we should also
> > update the vblank timestamping stuff to make sure the vblank timestamp
> > corrections/guesstimations come out exact.
> > 
> > Note that only crtc_clock and framedur_ns can actually end up changing here
> > during fastsets. Everything else we touch can only change during full
> > modesets.
> > 
> > Technically we should try to do this exactly at the start of vblank, but that
> > would require some kind of double buffering scheme. Let's skip that for now
> > and just update things right after the commit has been submitted to the
> > hardware. This means the information will be properly up to date when the
> > vblank irq handler goes to work. Only if someone ends up querying some
> > vblanky stuff in between the commit and start of vblank may we see a slight
> > discrepancy.
> > 
> > Also this same problem really exists for the DRRS downclocking stuff. But as
> > that is supposed to be more or less transparent to the user, and it only drops
> > to low gear after a long delay
> > (1 sec currently) we probably don't have to worry about it.
> > Any time something is actively submitting updates DRRS will remain in high
> > gear and so the timestamping constants will match the hardware state.
> > 
> > Fixes: e6f29923c048 ("drm/i915: Allow M/N change during fastset on bdw+")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_crtc.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c
> > b/drivers/gpu/drm/i915/display/intel_crtc.c
> > index b79a8834559f..41d381bbb57a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> > @@ -686,6 +686,14 @@ void intel_pipe_update_end(struct intel_crtc_state
> > *new_crtc_state)
> >  	 */
> >  	intel_vrr_send_push(new_crtc_state);
> > 
> > +	/*
> > +	 * Seamless M/N update may need to update frame timings.
> > +	 *
> > +	 * FIXME Should be synchronized with the start of vblank somehow...
> > +	 */
> > +	if (new_crtc_state->seamless_m_n &&
> > intel_crtc_needs_fastset(new_crtc_state))
> > +		intel_crtc_update_active_timings(new_crtc_state);
> > +
> Hi Ville,
> 
> changes LGTM. Although have a question, are we suspecting any timing param changes after Push bit is sent ?

Push is only used with VRR, at which points the real timings
can change of course as the hw terminates the vblank early.
But our notion of active timings will not change (they've already
been correctly set up for VRR when we enabled VRR).

> 
> Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> 
> Thanks
> >  	local_irq_enable();
> > 
> >  	if (intel_vgpu_active(dev_priv))
> > --
> > 2.39.2
> 

-- 
Ville Syrjälä
Intel

[Intel-gfx] ✗ Fi.CI.BUILD: failure for series starting with [1/4] drm/i915: Update vblank timestamping stuff on seamless M/N change (rev4)

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: Update vblank timestamping stuff on seamless M/N change (rev4)
URL   : https://patchwork.freedesktop.org/series/114720/
State : failure

== Summary ==

Error: patch https://patchwork.freedesktop.org/api/1.0/series/114720/revisions/4/mbox/ not applied
Applying: drm/i915: Update vblank timestamping stuff on seamless M/N change
Applying: drm/i915: Add belts and suspenders locking for seamless M/N changes
Applying: drm/i915: Relocate intel_crtc_update_active_timings()
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/display/intel_display.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 drm/i915: Relocate intel_crtc_update_active_timings()
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".