[PATCH 0/7] drm/i915: VRR with the pipe joiner

[PATCH 0/7] drm/i915: VRR with the pipe joiner

[ElXreno]

This series allows VRR on modes that need the pipe joiner, such as
3840x2400@240 on the eDP panels of current 18" laptops. So far such
modes are kept on the fixed refresh rate timing generator.

What was missing is the sequencing of the transcoder level VRR
operations (timings, enable, disable, push) against the pipe level
updates of the joined pipes [1][2]; joiner + fixed_rr went in as the
first step [3], commit 0ec46988ae6c ("drm/i915/vrr: Allow fixed_rr
with pipe joiner"). Here the sequencing is done on the MMIO path: all
joined pipes are armed inside one vblank evasion critical section on
the primary, which then sends the push.

Tested on one machine (BOE NE180QA1-MM1 on Arrow Lake-HX, DSC, pipes
A+B) with a backport to 7.3.4. On drm-tip it is build tested only.

 - patches 1 and 2 fix two int overflows that a 2342.4 MHz pixel
   clock runs into
 - patch 3 leaves the VRR timings, enable, disable and push to the
   joiner primary
 - patches 4 and 5 arm all joined pipes inside one vblank evasion
   critical section when VRR is in use, with the push at its end
 - patch 6 makes the per pipe vblank evasion that is left look at the
   scanline counter of the primary
 - patch 7 allows VRR with the joiner and keeps such commits off the
   DSB

Why not the DSB: each pipe runs its own DSB and nothing orders them
against the push. A port that only skips the push on the secondaries
gave "DSB 0 timed out waiting for idle" on the secondary 13 times in
27 minutes, details in patch 7. I don't know whether the DSB of the
primary could write the plane registers of the other pipes, which
would allow one DSB for all joined pipes. If it can, that is the
better end state and I'd be glad to hear it.

Why one critical section: with one evasion per pipe the secondary can
latch at the vmax boundary a frame ahead of the primary. Sampling
PLANE_SURFLIVE of both pipes through MMIO, the halves of the screen
showed different frames for one frame in 9 of 20777 frames over
three runs with the per pipe sections forced.

With the series, same machine and method:

 - no such frame in 18028 frames of plain flips with VRR on, no DSB,
   vblank evasion or push errors
 - the joined critical section takes 15 us on average for the two
   pipes, against the 100 us the evasion allows; 49 of 71983 took 60
   to 188 us on a stalled MMIO write, which the per pipe sections
   without VRR show as well (patch 5)
 - the flip event follows the new surface going live by a median
   12 us, and content at a steady 240 fps is paced at 4.17 ms at p50,
   p95 and p99
 - VRR can be toggled at run time. With LUTs loaded, as here, turning
   it off is a joined commit with the colour DSBs, turning it on
   takes the per pipe sections with the vblank worker

Still on the per pipe sections with VRR and the joiner, and so still
able to latch the pipes a frame apart: modesets, the atomic fallback
of a legacy cursor update, async flips, DDB changes, and commits that
need the vblank worker for the LUT load. On the platforms with double
buffered LUTs the latter stay joined and load the LUTs of all joined
pipes with MMIO inside the critical section, which I could not
measure. LRR and DC balance are kept off with the joiner. The commit
sequence of a joined configuration without VRR and the guardband
computation are not changed.

Not tested: IGT, anything on drm-tip beyond the build (i915 and xe),
LNL and later (PSR/Panel Replay with VRR and their guardband terms,
double buffered LUTs, the always on VRR timing generator), PSR2/Panel
Replay with the joiner, the ultrajoiner, anything but eDP, a colour
update that changes the LUTs while VRR stays on, and the refresh
rate change that patch 1 fixes. Suspend/resume was only run on an
earlier version.

The base EDID range of the panel ends at 120 Hz, 240 Hz is only in
its DisplayID Adaptive-Sync block, so testing this needs "drm/edid:
Read the monitor range from the DisplayID Adaptive-Sync block", sent
separately to dri-devel [4].

On tooling, per Documentation/process/generated-content.rst: this
series was written with an LLM coding assistant (Claude Code). It did
the source and mailing list archive analysis, wrote the patches, the
changelogs and the measurement scripts, and ran the measurements on my
machine under my direction. The scripts are in [5]. I have reviewed
all of it and take responsibility for the result.

[1] https://lore.kernel.org/intel-gfx/20250224061717.1095226-14-ankit.k.nautiyal@intel.com/
[2] https://lore.kernel.org/intel-gfx/Z7N9RHhQDltsygA_@intel.com/
[3] https://lore.kernel.org/intel-gfx/Z78vx24DwelqHTc9@intel.com/
[4] https://lore.kernel.org/r/20261114010749.1949247-1-elxreno@gmail.com
[5] https://gist.github.com/ElXreno/c2e6e6f34b056efaf99c238772cf74a3

ElXreno (7):
  drm/i915/panel: Fix the pixel clock overflow in the VRR vtotal
  drm/i915/vrr: Fix the pixel clock overflow in the vmax computation
  drm/i915/vrr: Leave the transcoder to the joiner primary
  drm/i915: Split intel_update_crtc() into its stages
  drm/i915: Arm joined pipes within one vblank evasion for VRR
  drm/i915: Evade the vblank on the scanline of the joiner primary
  drm/i915/vrr: Allow VRR with the pipe joiner

 drivers/gpu/drm/i915/display/intel_color.c   |   4 +
 drivers/gpu/drm/i915/display/intel_crtc.c    |   2 +-
 drivers/gpu/drm/i915/display/intel_crtc.h    |   1 +
 drivers/gpu/drm/i915/display/intel_display.c | 189 +++++++++++++++----
 drivers/gpu/drm/i915/display/intel_panel.c   |   4 +-
 drivers/gpu/drm/i915/display/intel_vblank.c  |  12 +-
 drivers/gpu/drm/i915/display/intel_vblank.h  |   2 +-
 drivers/gpu/drm/i915/display/intel_vrr.c     |  56 ++++--
 drivers/gpu/drm/i915/display/intel_vrr.h     |   1 +
 9 files changed, 217 insertions(+), 54 deletions(-)


base-commit: f40cc5e715fe9e7a5e61f7cd44f63f68c1625f2c
prerequisite-patch-id: c7f9956bca76130b71364377ca611051a16f466c
-- 
2.56.0

[PATCH 1/7] drm/i915/panel: Fix the pixel clock overflow in the VRR vtotal

[ElXreno]

intel_panel_compute_config_vrr() multiplies the pixel clock in kHz by
1000 in a signed int. A 3840x2400@240 timing has a 2342.4 MHz pixel
clock, the product overflows, and the vertical total computed for a
refresh rate other than that of the fixed mode comes out negative.

Do the multiplication in 64 bits.

This needs a fixed mode above 2147 MHz, so one that uses the joiner,
inside the VRR range of the panel, and a request for another refresh
rate that ends up on that fixed mode, as the allow_modeset=false path
does for a mode that is VRR compatible with the current one.

Fixes: a2da67028cd0 ("drm/i915: Allow arbitrary refresh rates with VRR eDP panels")
Signed-off-by: ElXreno <elxreno@gmail.com>
---
 drivers/gpu/drm/i915/display/intel_panel.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
index 81e638d0c7b3..157be3153c7e 100644
--- a/drivers/gpu/drm/i915/display/intel_panel.c
+++ b/drivers/gpu/drm/i915/display/intel_panel.c
@@ -277,8 +277,8 @@ static int intel_panel_compute_config_vrr(struct intel_atomic_state *state,
 		int vsync_end_offset = adjusted_mode->vtotal - adjusted_mode->vsync_end;
 
 		adjusted_mode->vtotal =
-			DIV_ROUND_CLOSEST(adjusted_mode->clock * 1000,
-					  adjusted_mode->htotal * vrefresh);
+			DIV_ROUND_CLOSEST_ULL(mul_u32_u32(adjusted_mode->clock, 1000),
+					      adjusted_mode->htotal * vrefresh);
 
 		adjusted_mode->vsync_start = adjusted_mode->vtotal - vsync_start_offset;
 		adjusted_mode->vsync_end = adjusted_mode->vtotal - vsync_end_offset;
-- 
2.56.0

Re: [PATCH 1/7] drm/i915/panel: Fix the pixel clock overflow in the VRR vtotal

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/i915/panel: commit Signed-off-by trailer uses handle
  instead of real name
--

commit a81f42546a1b676a6932361ec102d9c524823aa2
Author: ElXreno <elxreno@gmail.com>

> drm/i915/panel: Fix the pixel clock overflow in the VRR vtotal
>
> intel_panel_compute_config_vrr() multiplies the pixel clock in kHz by
> 1000 in a signed int. A 3840x2400@240 timing has a 2342.4 MHz pixel
> clock, the product overflows, and the vertical total computed for a
> refresh rate other than that of the fixed mode comes out negative.
>
> Do the multiplication in 64 bits.
>
> This needs a fixed mode above 2147 MHz, so one that uses the joiner,
> inside the VRR range of the panel, and a request for another refresh
> rate that ends up on that fixed mode, as the allow_modeset=false path
> does for a mode that is VRR compatible with the current one.
>
> Fixes: a2da67028cd0 ("drm/i915: Allow arbitrary refresh rates with VRR eDP panels")
> Signed-off-by: ElXreno <elxreno@gmail.com>

[Severity: High]
Both the Author header and the Signed-off-by trailer use the single-word
handle ElXreno rather than a real full name. Documentation/process/submitting-patches.rst
requires contributors to sign off using a known real identity under the
Developer's Certificate of Origin rather than a pseudonym or handle.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114014335.2132756-1-elxreno@gmail.com?part=1

[PATCH 2/7] drm/i915/vrr: Fix the pixel clock overflow in the vmax computation

[ElXreno]

intel_vrr_compute_vmax() multiplies crtc_clock, which is in kHz, by
1000 in a signed int. A 3840x2400@240 timing has a 2342.4 MHz pixel
clock, so the product overflows, the quotient turns negative and vmax
is clamped to crtc_vtotal. vmin then equals vmax and VRR is never
enabled for the mode, although it is within the range of the panel.

Do the multiplication in 64 bits.

No Fixes tag: intel_vrr_compute_vmax() is only called for a mode
within the VRR range, such a pixel clock needs the joiner, and
intel_vrr_compute_config() takes joined modes out of the range so
far.

Signed-off-by: ElXreno <elxreno@gmail.com>
---
 drivers/gpu/drm/i915/display/intel_vrr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
index e59345bd1d33..a5a5d11380a4 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.c
+++ b/drivers/gpu/drm/i915/display/intel_vrr.c
@@ -416,8 +416,8 @@ int intel_vrr_compute_vmax(struct intel_connector *connector,
 	const struct drm_display_info *info = &connector->base.display_info;
 	int vmax;
 
-	vmax = adjusted_mode->crtc_clock * 1000 /
-		(adjusted_mode->crtc_htotal * info->monitor_range.min_vfreq);
+	vmax = div_u64(mul_u32_u32(adjusted_mode->crtc_clock, 1000),
+		       adjusted_mode->crtc_htotal * info->monitor_range.min_vfreq);
 	vmax = max_t(int, vmax, adjusted_mode->crtc_vtotal);
 
 	return vmax;
-- 
2.56.0

Re: [PATCH 2/7] drm/i915/vrr: Fix the pixel clock overflow in the vmax computation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/i915/vrr: use of handle instead of real name in
  Signed-off-by trailer
--

commit a171d275708d23dc260c42db008a450a9d345462
Author: ElXreno <elxreno@gmail.com>

> drm/i915/vrr: Fix the pixel clock overflow in the vmax computation
>
> intel_vrr_compute_vmax() multiplies crtc_clock, which is in kHz, by
> 1000 in a signed int. A 3840x2400@240 timing has a 2342.4 MHz pixel
> clock, so the product overflows, the quotient turns negative and vmax
> is clamped to crtc_vtotal. vmin then equals vmax and VRR is never
> enabled for the mode, although it is within the range of the panel.
>
> Do the multiplication in 64 bits.
>
> No Fixes tag: intel_vrr_compute_vmax() is only called for a mode
> within the VRR range, such a pixel clock needs the joiner, and
> intel_vrr_compute_config() takes joined modes out of the range so
> far.
>
> Signed-off-by: ElXreno <elxreno@gmail.com>

[Severity: High]
Could the Author and Signed-off-by lines be updated to use a real full name
rather than a handle?  Documentation/process/submitting-patches.rst requires
a known identity rather than a pseudonym or single-word username for the
Developer Certificate of Origin.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114014335.2132756-1-elxreno@gmail.com?part=2

[PATCH 3/7] drm/i915/vrr: Leave the transcoder to the joiner primary

[ElXreno]

All joined pipes run off the transcoder of the primary, and the crtc
states of the secondaries are copies of the primary's state, the VRR
state included. The commit therefore reaches the transcoder level VRR
functions for every joined pipe: intel_pre_update_crtc() calls
intel_vrr_set_transcoder_timings() for each crtc whose VRR parameters
changed, intel_pre_plane_update() and commit_pipe_post_planes() call
intel_vrr_disable() and intel_vrr_enable(), and both commit paths
call intel_vrr_send_push() and intel_vrr_check_push_sent() per crtc.

Do these once, from the primary. When disabling VRR also switch the
active timings of all joined pipes as soon as the transcoder has left
VRR mode, rather than leaving the secondaries on the VRR timings
until their own turn comes.

The check in intel_vrr_set_transcoder_timings() drops the second,
identical programming of the transcoder that a joined fixed refresh
rate configuration gets today. Everything else depends on vrr.enable,
which is never set with the joiner yet, so there is no other
functional change. In particular the push a secondary sends for the
PSR and Panel Replay frame change is left as it is.

The checks in intel_vrr_set_transcoder_timings(), intel_vrr_send_push(),
intel_vrr_enable() and intel_vrr_disable() are the ones Ankit Nautiyal
posted in "drm/i915/vrr: Handle joiner with vrr", last in [1].

[1] https://lore.kernel.org/intel-gfx/20250224061717.1095226-14-ankit.k.nautiyal@intel.com/

Suggested-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Signed-off-by: ElXreno <elxreno@gmail.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 15 ++++++++++++--
 drivers/gpu/drm/i915/display/intel_vrr.c     | 21 ++++++++++++++++++++
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 9a503b25ae47..20b1103e3de4 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -1237,10 +1237,21 @@ static void intel_pre_plane_update(struct intel_atomic_state *state,
 
 	intel_psr_pre_plane_update(state, crtc);
 
-	if (intel_crtc_vrr_disabling(state, crtc)) {
+	/* The joined pipes are handled in one go from the primary */
+	if (intel_crtc_vrr_disabling(state, crtc) &&
+	    !intel_crtc_is_joiner_secondary(old_crtc_state)) {
+		struct intel_crtc *pipe_crtc;
+
 		intel_vrr_disable(old_crtc_state);
 		intel_vrr_dcb_reset(old_crtc_state, crtc);
-		intel_crtc_update_active_timings(old_crtc_state, false);
+
+		for_each_intel_crtc_in_pipe_mask(display, pipe_crtc,
+						 intel_crtc_joined_pipe_mask(old_crtc_state)) {
+			const struct intel_crtc_state *old_pipe_crtc_state =
+				intel_atomic_get_old_crtc_state(state, pipe_crtc);
+
+			intel_crtc_update_active_timings(old_pipe_crtc_state, false);
+		}
 
 		/*
 		 * VRR is being disabled seamlessly (no modeset, Panel Replay
diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
index a5a5d11380a4..668c495e7407 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.c
+++ b/drivers/gpu/drm/i915/display/intel_vrr.c
@@ -19,6 +19,7 @@
 #include "intel_de.h"
 #include "intel_dip.h"
 #include "intel_dip_regs.h"
+#include "intel_display.h"
 #include "intel_display_limits.h"
 #include "intel_display_regs.h"
 #include "intel_display_types.h"
@@ -674,6 +675,10 @@ void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state)
 	if (!HAS_VRR(display))
 		return;
 
+	/* The joined pipes drive one transcoder, so program it once. */
+	if (intel_crtc_is_joiner_secondary(crtc_state))
+		return;
+
 	/*
 	 * Bspec says:
 	 * "(note: VRR needs to be programmed after
@@ -773,6 +778,13 @@ void intel_vrr_send_push(struct intel_dsb *dsb,
 	if (!crtc_state->vrr.enable && !intel_psr_use_trans_push(crtc_state))
 		return;
 
+	/*
+	 * A VRR push latches all joined pipes at once, it is up to the
+	 * primary to send it once every one of them has been armed.
+	 */
+	if (crtc_state->vrr.enable && intel_crtc_is_joiner_secondary(crtc_state))
+		return;
+
 	if (dsb)
 		intel_dsb_nonpost_start(dsb);
 
@@ -793,6 +805,9 @@ void intel_vrr_check_push_sent(struct intel_dsb *dsb,
 	if (!crtc_state->vrr.enable)
 		return;
 
+	if (intel_crtc_is_joiner_secondary(crtc_state))
+		return;
+
 	/*
 	 * Make sure the push send bit has cleared. This should
 	 * already be the case as long as the caller makes sure
@@ -1005,6 +1020,9 @@ void intel_vrr_enable(const struct intel_crtc_state *crtc_state)
 	if (!crtc_state->vrr.enable)
 		return;
 
+	if (intel_crtc_is_joiner_secondary(crtc_state))
+		return;
+
 	intel_vrr_set_vrr_timings(crtc_state);
 	intel_vrr_enable_dc_balancing(crtc_state);
 
@@ -1019,6 +1037,9 @@ void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state)
 	if (!old_crtc_state->vrr.enable)
 		return;
 
+	if (intel_crtc_is_joiner_secondary(old_crtc_state))
+		return;
+
 	if (!intel_vrr_always_use_vrr_tg(display))
 		intel_vrr_tg_disable(old_crtc_state);
 
-- 
2.56.0

Re: [PATCH 3/7] drm/i915/vrr: Leave the transcoder to the joiner primary

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/i915/vrr: Signed-off-by trailer uses handle instead of real
  full name
--

commit 6f78fb0ff83ff8c9f4f0ac58ba24164db71f007e
Author: ElXreno <elxreno@gmail.com>

> drm/i915/vrr: Leave the transcoder to the joiner primary
>
> All joined pipes run off the transcoder of the primary, and the crtc
> states of the secondaries are copies of the primary's state, the VRR
> state included. The commit therefore reaches the transcoder level VRR
> functions for every joined pipe: intel_pre_update_crtc() calls
> intel_vrr_set_transcoder_timings() for each crtc whose VRR parameters
> changed, intel_pre_plane_update() and commit_pipe_post_planes() call
> intel_vrr_disable() and intel_vrr_enable(), and both commit paths
> call intel_vrr_send_push() and intel_vrr_check_push_sent() per crtc.
>
> Do these once, from the primary. When disabling VRR also switch the
> active timings of all joined pipes as soon as the transcoder has left
> VRR mode, rather than leaving the secondaries on the VRR timings
> until their own turn comes.
>
> The check in intel_vrr_set_transcoder_timings() drops the second,
> identical programming of the transcoder that a joined fixed refresh
> rate configuration gets today. Everything else depends on vrr.enable,
> which is never set with the joiner yet, so there is no other
> functional change. In particular the push a secondary sends for the
> PSR and Panel Replay frame change is left as it is.
>
> The checks in intel_vrr_set_transcoder_timings(), intel_vrr_send_push(),
> intel_vrr_enable() and intel_vrr_disable() are the ones Ankit Nautiyal
> posted in "drm/i915/vrr: Handle joiner with vrr", last in [1].
>
> [1] https://lore.kernel.org/intel-gfx/20250224061717.1095226-14-ankit.k.nautiyal@intel.com/
>
> Suggested-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Signed-off-by: ElXreno <elxreno@gmail.com>

[Severity: High]
The Signed-off-by trailer uses a single-word handle ("ElXreno") rather
than a real full name, which does not meet the Developer's Certificate
of Origin requirements in Documentation/process/submitting-patches.rst.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114014335.2132756-1-elxreno@gmail.com?part=3

[PATCH 4/7] drm/i915: Split intel_update_crtc() into its stages

[ElXreno]

Pull the arming of a pipe on the MMIO path and the bookkeeping that
follows the commit of a pipe out of intel_update_crtc(), into
commit_pipe_arm() and intel_post_update_crtc(). The next patch arms
several joined pipes inside one vblank evasion critical section and
needs them separately.

No functional change.

Signed-off-by: ElXreno <elxreno@gmail.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 82 ++++++++++++--------
 1 file changed, 50 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 20b1103e3de4..aa03f674d93b 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7087,46 +7087,29 @@ static void intel_pre_update_crtc(struct intel_atomic_state *state,
 	}
 }
 
-static void intel_update_crtc(struct intel_atomic_state *state,
-			      struct intel_crtc *crtc)
+static void commit_pipe_arm(struct intel_atomic_state *state,
+			    struct intel_crtc *crtc)
 {
-	const struct intel_crtc_state *old_crtc_state =
-		intel_atomic_get_old_crtc_state(state, crtc);
-	struct intel_crtc_state *new_crtc_state =
+	const struct intel_crtc_state *new_crtc_state =
 		intel_atomic_get_new_crtc_state(state, crtc);
 
-	switch (new_crtc_state->commit_type) {
-	case INTEL_COMMIT_FLIPQ:
-		intel_flipq_enable(new_crtc_state);
-
-		intel_crtc_prepare_vblank_event(new_crtc_state, &crtc->flipq_event);
-
-		intel_flipq_add(crtc, INTEL_FLIPQ_PLANE_1, 0, INTEL_DSB_0,
-				new_crtc_state->dsb_commit);
-		break;
-
-	case INTEL_COMMIT_DSB:
-		intel_crtc_prepare_vblank_event(new_crtc_state, &crtc->dsb_event);
-
+	if (new_crtc_state->dsb_commit)
 		intel_dsb_commit(new_crtc_state->dsb_commit);
-		break;
 
-	case INTEL_COMMIT_MMIO:
-		/* Perform vblank evasion around commit operation */
-		intel_pipe_update_start(state, crtc);
-
-		if (new_crtc_state->dsb_commit)
-			intel_dsb_commit(new_crtc_state->dsb_commit);
+	commit_pipe_pre_planes(state, crtc);
 
-		commit_pipe_pre_planes(state, crtc);
+	intel_crtc_planes_update_arm(NULL, state, crtc);
 
-		intel_crtc_planes_update_arm(NULL, state, crtc);
-
-		commit_pipe_post_planes(state, crtc);
+	commit_pipe_post_planes(state, crtc);
+}
 
-		intel_pipe_update_end(state, crtc);
-		break;
-	}
+static void intel_post_update_crtc(struct intel_atomic_state *state,
+				   struct intel_crtc *crtc)
+{
+	const struct intel_crtc_state *old_crtc_state =
+		intel_atomic_get_old_crtc_state(state, crtc);
+	struct intel_crtc_state *new_crtc_state =
+		intel_atomic_get_new_crtc_state(state, crtc);
 
 	/*
 	 * VRR/Seamless M/N update may need to update frame timings.
@@ -7168,6 +7151,41 @@ static void intel_update_crtc(struct intel_atomic_state *state,
 	}
 }
 
+static void intel_update_crtc(struct intel_atomic_state *state,
+			      struct intel_crtc *crtc)
+{
+	struct intel_crtc_state *new_crtc_state =
+		intel_atomic_get_new_crtc_state(state, crtc);
+
+	switch (new_crtc_state->commit_type) {
+	case INTEL_COMMIT_FLIPQ:
+		intel_flipq_enable(new_crtc_state);
+
+		intel_crtc_prepare_vblank_event(new_crtc_state, &crtc->flipq_event);
+
+		intel_flipq_add(crtc, INTEL_FLIPQ_PLANE_1, 0, INTEL_DSB_0,
+				new_crtc_state->dsb_commit);
+		break;
+
+	case INTEL_COMMIT_DSB:
+		intel_crtc_prepare_vblank_event(new_crtc_state, &crtc->dsb_event);
+
+		intel_dsb_commit(new_crtc_state->dsb_commit);
+		break;
+
+	case INTEL_COMMIT_MMIO:
+		/* Perform vblank evasion around commit operation */
+		intel_pipe_update_start(state, crtc);
+
+		commit_pipe_arm(state, crtc);
+
+		intel_pipe_update_end(state, crtc);
+		break;
+	}
+
+	intel_post_update_crtc(state, crtc);
+}
+
 static void intel_old_crtc_state_disables(struct intel_atomic_state *state,
 					  struct intel_crtc *crtc)
 {
-- 
2.56.0

Re: [PATCH 4/7] drm/i915: Split intel_update_crtc() into its stages

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/i915: Signed-off-by trailer uses handle instead of real
  full name
--

commit 4c5cd64a7c5678be3ce749725bc8055689ef0bfc
Author: ElXreno <elxreno@gmail.com>

> drm/i915: Split intel_update_crtc() into its stages
>
> Pull the arming of a pipe on the MMIO path and the bookkeeping that
> follows the commit of a pipe out of intel_update_crtc(), into
> commit_pipe_arm() and intel_post_update_crtc(). The next patch arms
> several joined pipes inside one vblank evasion critical section and
> needs them separately.
>
> No functional change.
>
> Signed-off-by: ElXreno <elxreno@gmail.com>

[Severity: High]
The Author and Signed-off-by lines use a handle ("ElXreno") rather than a
real full name, which does not meet the Developer's Certificate of Origin
requirements in Documentation/process/submitting-patches.rst.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114014335.2132756-1-elxreno@gmail.com?part=4

[PATCH 5/7] drm/i915: Arm joined pipes within one vblank evasion for VRR

[ElXreno]

All joined pipes run off the transcoder of the primary, so one vblank
latches every one of them. On the MMIO path each pipe nevertheless
goes through its own intel_pipe_update_start()/intel_pipe_update_end()
critical section, one pipe after another. When the vblank falls
between two of them the earlier pipe latches its new state a frame
ahead of the later one, and for that frame the halves of the screen
come from different frames.

With VRR that gets worse, as the commit ends the frame itself: the
push at the end of the primary's section latches whatever has been
armed by then. Commit the joined pipes together when VRR is in use:
do the vblank evasion once, on the primary, arm all of them inside
that critical section and let the primary send the push at its end.
The evasion accounting, the trace events and the atomic update
failure check then run once as well, for the primary.

Armed this way the critical section takes 15 us on average for two
pipes on Arrow Lake (71983 commits at 240 fps: 71918 under 30 us, 16
between 30 and 60 us, 49 between 60 and 188 us), against the 100 us
the evasion allows for. The window is not scaled with the number of
pipes. The 49 are single MMIO writes that stall, not more work: the
instruction count is that of an ordinary section, the CPU stays in
C0, there is no SMI or NMI, and the register differs from case to
case. The per pipe sections of the same mode without VRR and with
the DSB turned off show the same stalls (15 of 72002 sections over
60 us), so they do not come from arming two pipes in one section.
None of them ran into a vblank.

The per pipe sequence is kept where the critical section has per
pipe work attached or the pipes need sequencing against others:
modesets, the atomic fallback of a legacy cursor update, async flips,
commits that need the vblank worker for the LUT load and commits that
change the DDB allocation of a joined pipe. The vblank worker is
needed on the platforms without double buffered LUTs, when a colour
update or a fastset has LUTs to load that can neither be preloaded
nor be left to a colour DSB. Commits that go through the DSB or the
flip queue are not affected, and neither is the commit sequence of a
joined configuration without VRR. The latter has the same race
against the fixed vblank, it is left alone only to keep this change
to the case that can be tested with it.

On the platforms with double buffered LUTs there is no vblank worker.
Where no colour DSB is used the LUTs are loaded with MMIO inside the
critical section, which here is the joined one and then loads them
for all joined pipes. I have no such hardware and have not tested
that.

No functional change yet, VRR is not allowed with the joiner.

Signed-off-by: ElXreno <elxreno@gmail.com>
---
 drivers/gpu/drm/i915/display/intel_crtc.c    |  2 +-
 drivers/gpu/drm/i915/display/intel_crtc.h    |  1 +
 drivers/gpu/drm/i915/display/intel_display.c | 88 ++++++++++++++++++++
 3 files changed, 90 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
index 139c9dae536e..f41616a060b5 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc.c
@@ -487,7 +487,7 @@ int intel_crtc_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
 	return 0;
 }
 
-static bool intel_crtc_needs_vblank_work(const struct intel_crtc_state *crtc_state)
+bool intel_crtc_needs_vblank_work(const struct intel_crtc_state *crtc_state)
 {
 	struct intel_display *display = to_intel_display(crtc_state);
 
diff --git a/drivers/gpu/drm/i915/display/intel_crtc.h b/drivers/gpu/drm/i915/display/intel_crtc.h
index 12507b51ee77..0478394b01e8 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc.h
+++ b/drivers/gpu/drm/i915/display/intel_crtc.h
@@ -50,6 +50,7 @@ void intel_pipe_update_start(struct intel_atomic_state *state,
 			     struct intel_crtc *crtc);
 void intel_pipe_update_end(struct intel_atomic_state *state,
 			   struct intel_crtc *crtc);
+bool intel_crtc_needs_vblank_work(const struct intel_crtc_state *crtc_state);
 void intel_wait_for_vblank_workers(struct intel_atomic_state *state);
 struct intel_crtc *intel_first_crtc(struct intel_display *display);
 struct intel_crtc *intel_crtc_for_pipe(struct intel_display *display,
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index aa03f674d93b..1fda1bb15674 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7151,12 +7151,100 @@ static void intel_post_update_crtc(struct intel_atomic_state *state,
 	}
 }
 
+/*
+ * All joined pipes run off the transcoder of the primary, so one
+ * vblank latches every one of them, and with VRR the commit itself
+ * ends the frame with a push. The arming registers of all joined
+ * pipes then have to be written within a single vblank evasion
+ * critical section, with the push at its end, or the pipes may
+ * latch in different frames.
+ */
+static bool intel_crtc_needs_joined_update(struct intel_atomic_state *state,
+					   struct intel_crtc *crtc)
+{
+	struct intel_display *display = to_intel_display(state);
+	const struct intel_crtc_state *old_crtc_state =
+		intel_atomic_get_old_crtc_state(state, crtc);
+	const struct intel_crtc_state *new_crtc_state =
+		intel_atomic_get_new_crtc_state(state, crtc);
+	struct intel_crtc *pipe_crtc;
+
+	if (!new_crtc_state->joiner_pipes)
+		return false;
+
+	/* FIXME do this for the fixed refresh rate case as well */
+	if (!old_crtc_state->vrr.enable && !new_crtc_state->vrr.enable)
+		return false;
+
+	/* FIXME the cursor unpin works are set up per pipe */
+	if (state->base.legacy_cursor_update)
+		return false;
+
+	for_each_intel_crtc_in_pipe_mask(display, pipe_crtc,
+					 intel_crtc_joined_pipe_mask(new_crtc_state)) {
+		const struct intel_crtc_state *old_pipe_crtc_state =
+			intel_atomic_get_old_crtc_state(state, pipe_crtc);
+		const struct intel_crtc_state *new_pipe_crtc_state =
+			intel_atomic_get_new_crtc_state(state, pipe_crtc);
+
+		/* The DSB and the flip queue do their own vblank evasion */
+		if (new_pipe_crtc_state->commit_type != INTEL_COMMIT_MMIO)
+			return false;
+
+		if (intel_crtc_needs_modeset(new_pipe_crtc_state) ||
+		    new_pipe_crtc_state->do_async_flip)
+			return false;
+
+		/* FIXME the vblank work is set up per pipe */
+		if (intel_crtc_needs_vblank_work(new_pipe_crtc_state))
+			return false;
+
+		/*
+		 * A changed DDB allocation must be sequenced
+		 * against the other pipes one pipe at a time.
+		 */
+		if (!skl_ddb_entry_equal(&old_pipe_crtc_state->wm.skl.ddb,
+					 &new_pipe_crtc_state->wm.skl.ddb))
+			return false;
+	}
+
+	return true;
+}
+
+static void intel_update_joined_crtcs(struct intel_atomic_state *state,
+				      struct intel_crtc *primary_crtc)
+{
+	struct intel_display *display = to_intel_display(state);
+	const struct intel_crtc_state *new_crtc_state =
+		intel_atomic_get_new_crtc_state(state, primary_crtc);
+	u8 pipe_mask = intel_crtc_joined_pipe_mask(new_crtc_state);
+	struct intel_crtc *pipe_crtc;
+
+	/* Perform vblank evasion around the commit of all joined pipes */
+	intel_pipe_update_start(state, primary_crtc);
+
+	for_each_intel_crtc_in_pipe_mask_reverse(display, pipe_crtc, pipe_mask)
+		commit_pipe_arm(state, pipe_crtc);
+
+	intel_pipe_update_end(state, primary_crtc);
+
+	for_each_intel_crtc_in_pipe_mask_reverse(display, pipe_crtc, pipe_mask)
+		intel_post_update_crtc(state, pipe_crtc);
+}
+
 static void intel_update_crtc(struct intel_atomic_state *state,
 			      struct intel_crtc *crtc)
 {
 	struct intel_crtc_state *new_crtc_state =
 		intel_atomic_get_new_crtc_state(state, crtc);
 
+	if (intel_crtc_needs_joined_update(state, crtc)) {
+		/* The secondaries are committed along with the primary */
+		if (intel_crtc_is_joiner_primary(new_crtc_state))
+			intel_update_joined_crtcs(state, crtc);
+		return;
+	}
+
 	switch (new_crtc_state->commit_type) {
 	case INTEL_COMMIT_FLIPQ:
 		intel_flipq_enable(new_crtc_state);
-- 
2.56.0

Re: [PATCH 5/7] drm/i915: Arm joined pipes within one vblank evasion for VRR

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/i915: Signed-off-by trailer uses handle ElXreno instead of
  real name
--

commit fc87993e7c0d725b8ec9527a36ed996a89438f35
Author: ElXreno <elxreno@gmail.com>

> drm/i915: Arm joined pipes within one vblank evasion for VRR
>
> All joined pipes run off the transcoder of the primary, so one vblank
> latches every one of them. On the MMIO path each pipe nevertheless
> goes through its own intel_pipe_update_start()/intel_pipe_update_end()
> critical section, one pipe after another. When the vblank falls
> between two of them the earlier pipe latches its new state a frame
> ahead of the later one, and for that frame the halves of the screen
> come from different frames.
>
> With VRR that gets worse, as the commit ends the frame itself: the
> push at the end of the primary's section latches whatever has been
> armed by then. Commit the joined pipes together when VRR is in use:
> do the vblank evasion once, on the primary, arm all of them inside
> that critical section and let the primary send the push at its end.
> The evasion accounting, the trace events and the atomic update
> failure check then run once as well, for the primary.
>
> Armed this way the critical section takes 15 us on average for two
> pipes on Arrow Lake (71983 commits at 240 fps: 71918 under 30 us, 16
> between 30 and 60 us, 49 between 60 and 188 us), against the 100 us
> the evasion allows for. The window is not scaled with the number of
> pipes. The 49 are single MMIO writes that stall, not more work: the
> instruction count is that of an ordinary section, the CPU stays in
> C0, there is no SMI or NMI, and the register differs from case to
> case. The per pipe sections of the same mode without VRR and with
> the DSB turned off show the same stalls (15 of 72002 sections over
> 60 us), so they do not come from arming two pipes in one section.
> None of them ran into a vblank.
>
> The per pipe sequence is kept where the critical section has per
> pipe work attached or the pipes need sequencing against others:
> modesets, the atomic fallback of a legacy cursor update, async flips,
> commits that need the vblank worker for the LUT load and commits that
> change the DDB allocation of a joined pipe. The vblank worker is
> needed on the platforms without double buffered LUTs, when a colour
> update or a fastset has LUTs to load that can neither be preloaded
> nor be left to a colour DSB. Commits that go through the DSB or the
> flip queue are not affected, and neither is the commit sequence of a
> joined configuration without VRR. The latter has the same race
> against the fixed vblank, it is left alone only to keep this change
> to the case that can be tested with it.
>
> On the platforms with double buffered LUTs there is no vblank worker.
> Where no colour DSB is used the LUTs are loaded with MMIO inside the
> critical section, which here is the joined one and then loads them
> for all joined pipes. I have no such hardware and have not tested
> that.
>
> No functional change yet, VRR is not allowed with the joiner.
>
> Signed-off-by: ElXreno <elxreno@gmail.com>

[Severity: High]
The Author field and Signed-off-by trailer use a single-word handle
(ElXreno) instead of a real full name.  Per
Documentation/process/submitting-patches.rst, patches must be signed off
using a known identity and real full name rather than a pseudonym or
handle.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114014335.2132756-1-elxreno@gmail.com?part=5

[PATCH 6/7] drm/i915: Evade the vblank on the scanline of the joiner primary

[ElXreno]

The scanline counter of a joiner secondary pipe was observed to wrap
at 8192 lines on Arrow Lake, with pipes A and B joined. Sampling
PIPEDSL of both pipes, they agree up to line 8191, which is the
largest value pipe B ever returns, while pipe A counts on up to the
vmax vtotal. From line 8192 on pipe B reads 8192 lines less than
pipe A. This comes from measurement only, I have no documentation for
it, and I could not check whether it is a property of the joiner
secondary or of that pipe.

It is harmless as long as vtotal stays below 8192, which every fixed
refresh rate mode that needs the joiner does. With VRR the vmax
vtotal can be larger, 12200 for a 3840x2400@240 mode with a 48 Hz
lower limit. The vmax vblank start at line 12160 then reads as 3968
on the secondary, vblank evasion on that pipe never sees its evasion
window, and the pipe gets armed across the vblank:

 Atomic update failure on pipe B (start=48480 end=48481) time 8 us, min 12101, max 12159, scanline start 3965, end 3969

All joined pipes run off the timings of the primary, so look at the
scanline counter of the primary when evading the vblank on a
secondary. This matters for the commits that still take the per pipe
critical sections with the joiner.

Only the evasion is changed. The vblank timestamps of a secondary
still come from its own counter, and so does the end scanline in the
message above, while the start scanline is now the primary's.

Signed-off-by: ElXreno <elxreno@gmail.com>
---
 drivers/gpu/drm/i915/display/intel_vblank.c | 12 ++++++++++--
 drivers/gpu/drm/i915/display/intel_vblank.h |  2 +-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
index fc9d1e14ec87..6654ef590da6 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.c
+++ b/drivers/gpu/drm/i915/display/intel_vblank.c
@@ -11,6 +11,7 @@
 #include "intel_color.h"
 #include "intel_crtc.h"
 #include "intel_de.h"
+#include "intel_display.h"
 #include "intel_display_jiffies.h"
 #include "intel_display_regs.h"
 #include "intel_display_types.h"
@@ -683,6 +684,13 @@ void intel_vblank_evade_init(const struct intel_crtc_state *old_crtc_state,
 	/* TODO: maybe just use the active timings here? */
 	crtc_state = pre_commit_crtc_state(old_crtc_state, new_crtc_state);
 
+	/*
+	 * The scanline counter of a joiner secondary pipe has been
+	 * seen to wrap at 8192 lines (on ARL), which the VRR vmax can
+	 * exceed. The primary runs off the same timings and counts on.
+	 */
+	evade->scanline_crtc = intel_primary_crtc(crtc_state);
+
 	adjusted_mode = &crtc_state->hw.adjusted_mode;
 
 	if (crtc->mode_flags & I915_MODE_FLAG_VRR) {
@@ -738,7 +746,7 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
 		 */
 		prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
 
-		scanline = intel_get_crtc_scanline(crtc);
+		scanline = intel_get_crtc_scanline(evade->scanline_crtc);
 		if (scanline < evade->min || scanline > evade->max)
 			break;
 
@@ -774,7 +782,7 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
 	 * FIXME figure out if BXT+ DSI suffers from this as well
 	 */
 	while (evade->need_vlv_dsi_wa && scanline == evade->vblank_start)
-		scanline = intel_get_crtc_scanline(crtc);
+		scanline = intel_get_crtc_scanline(evade->scanline_crtc);
 
 	return scanline;
 }
diff --git a/drivers/gpu/drm/i915/display/intel_vblank.h b/drivers/gpu/drm/i915/display/intel_vblank.h
index 98d04cacd65f..28b4c458fa18 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.h
+++ b/drivers/gpu/drm/i915/display/intel_vblank.h
@@ -16,7 +16,7 @@ struct intel_crtc;
 struct intel_crtc_state;
 
 struct intel_vblank_evade_ctx {
-	struct intel_crtc *crtc;
+	struct intel_crtc *crtc, *scanline_crtc;
 	int min, max, vblank_start;
 	bool need_vlv_dsi_wa;
 };
-- 
2.56.0

Re: [PATCH 6/7] drm/i915: Evade the vblank on the scanline of the joiner primary

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/i915: Signed-off-by trailer uses handle instead of real
  name
--

commit 516746727da5577decb0038cc22a5dddd9cbc589
Author: ElXreno <elxreno@gmail.com>

> drm/i915: Evade the vblank on the scanline of the joiner primary
>
> The scanline counter of a joiner secondary pipe was observed to wrap
> at 8192 lines on Arrow Lake, with pipes A and B joined. Sampling
> PIPEDSL of both pipes, they agree up to line 8191, which is the
> largest value pipe B ever returns, while pipe A counts on up to the
> vmax vtotal. From line 8192 on pipe B reads 8192 lines less than
> pipe A. This comes from measurement only, I have no documentation for
> it, and I could not check whether it is a property of the joiner
> secondary or of that pipe.
>
> It is harmless as long as vtotal stays below 8192, which every fixed
> refresh rate mode that needs the joiner does. With VRR the vmax
> vtotal can be larger, 12200 for a 3840x2400@240 mode with a 48 Hz
> lower limit. The vmax vblank start at line 12160 then reads as 3968
> on the secondary, vblank evasion on that pipe never sees its evasion
> window, and the pipe gets armed across the vblank:
>
>  Atomic update failure on pipe B (start=48480 end=48481) time 8 us, min 12101, max 12159, scanline start 3965, end 3969
>
> All joined pipes run off the timings of the primary, so look at the
> scanline counter of the primary when evading the vblank on a
> secondary. This matters for the commits that still take the per pipe
> critical sections with the joiner.
>
> Only the evasion is changed. The vblank timestamps of a secondary
> still come from its own counter, and so does the end scanline in the
> message above, while the start scanline is now the primary's.
>
> Signed-off-by: ElXreno <elxreno@gmail.com>

[Severity: High]
Documentation/process/submitting-patches.rst requires a real name in the
Signed-off-by trailer and does not permit handles or pseudonyms. Could
this be updated to use a full real name in both the Author header and
Signed-off-by line?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114014335.2132756-1-elxreno@gmail.com?part=6

[PATCH 7/7] drm/i915/vrr: Allow VRR with the pipe joiner

[ElXreno]

VRR has been limited to the fixed refresh rate timing generator with
the joiner, as the transcoder level VRR operations were not sequenced
against the pipe level updates of the joined pipes. That keeps VRR
away from every mode that needs the joiner, such as 3840x2400@240 on
the eDP panels of current 18" laptops.

The transcoder is now programmed only by the joiner primary, and on
the MMIO path all joined pipes are armed inside one vblank evasion
critical section, with the push sent at its end. Take that path for
VRR with the joiner and let VRR through.

A commit whose new state has VRR enabled with the joiner does not use
the DSB. The push must not be sent before every joined pipe has been
armed, but each pipe runs its own DSB and nothing orders them against
each other. Leaving the push to the DSB of the primary, on the
grounds that it is committed last, is not enough. The secondary's DSB
waits for the safe window after it has armed (DSB_OPCODE_WAIT_VBLANKS
with the safe window chicken bits, then the wait in
intel_dsb_wait_for_delayed_vblank()). When the primary's push ends
the safe window before the secondary's DSB gets there, that DSB waits
for the safe window of the next frame, and it is still busy when
intel_dsb_wait() gives it 1 ms after the flip of the primary has
completed:

 [CRTC:271:pipe B] DSB 0 timed out waiting for idle (current head=0x100, head=0x0, tail=0x140)

That is what a port which only skips the push on the secondaries
gives, 13 times in 27 minutes of desktop use on Arrow Lake. The DSB
of a secondary can't check on the push either: commit c3571a239e10
("drm/i915/vrr: Check that the push send bit is clear after delayed
vblank") notes that reading TRANS_PUSH from it ends in a DSB response
timeout.

The LUT load has the same ordering problem: a colour DSB either sends
the push itself or has to be ordered against the push that the CPU
sends, on each pipe. So no colour DSB is prepared for such a commit
either, and the LUTs are loaded the way they are without one: ahead
of the commit when they can be preloaded, by the vblank worker after
it otherwise, in which case the commit takes the per pipe critical
sections, or with MMIO inside the critical section on the platforms
with double buffered LUTs. The commit that turns VRR off has no VRR
push to order and keeps its colour DSBs, which are then started from
inside the joined critical section.

One thing changes for joined modes even while VRR is off, as it
follows vrr.in_range: the sink is told to ignore the MSA timing
parameters during link training, as for any other mode within the
VRR range.

intel_vrr_compute_config() does not ask for LRR with the joiner, and
DC balance is kept off, each with a FIXME. The vblank delay fastset
that reuses the LRR code is not touched: where it applies (inherited
state, or the always on VRR timing generator) it still writes the
transcoder timings from every joined pipe, with identical values.

Signed-off-by: ElXreno <elxreno@gmail.com>
---
 drivers/gpu/drm/i915/display/intel_color.c   |  4 +++
 drivers/gpu/drm/i915/display/intel_display.c |  4 +++
 drivers/gpu/drm/i915/display/intel_vrr.c     | 31 ++++++++++++--------
 drivers/gpu/drm/i915/display/intel_vrr.h     |  1 +
 4 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 4070dc8d97d9..08d1c999637e 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -2023,6 +2023,10 @@ void intel_color_prepare_commit(struct intel_atomic_state *state,
 	if (!crtc_state->pre_csc_lut && !crtc_state->post_csc_lut)
 		return;
 
+	/* TODO sequence the LUT load vs. the VRR push for joined pipes via DSB */
+	if (intel_vrr_needs_mmio_commit(crtc_state))
+		return;
+
 	if (HAS_DOUBLE_BUFFERED_LUT(display))
 		crtc_state->dsb_color = intel_dsb_prepare(state, crtc, INTEL_DSB_0, 1024);
 	else
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 1fda1bb15674..8f30337c8967 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7681,6 +7681,10 @@ intel_atomic_commit_type(struct intel_atomic_state *state,
 	if (new_crtc_state->do_async_flip)
 		return INTEL_COMMIT_MMIO;
 
+	/* TODO sequence the VRR push for joined pipes via DSB */
+	if (intel_vrr_needs_mmio_commit(new_crtc_state))
+		return INTEL_COMMIT_MMIO;
+
 	/* TODO handle VRR with flip queue */
 	if (intel_flipq_supported(display) &&
 	    !new_crtc_state->vrr.enable &&
diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
index 668c495e7407..6543c140e2b5 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.c
+++ b/drivers/gpu/drm/i915/display/intel_vrr.c
@@ -450,6 +450,10 @@ intel_vrr_dc_balance_compute_config(struct intel_crtc_state *crtc_state)
 	    !crtc_state->vrr.enable || !display->params.enable_dc_balance)
 		return;
 
+	/* FIXME figure out DC balance with joined pipes */
+	if (crtc_state->joiner_pipes)
+		return;
+
 	crtc_state->vrr.dc_balance.vmax = crtc_state->vrr.vmax;
 	crtc_state->vrr.dc_balance.vmin = crtc_state->vrr.vmin;
 	crtc_state->vrr.dc_balance.max_increase =
@@ -495,21 +499,11 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
 	crtc_state->vrr.in_range =
 		intel_vrr_is_in_range(connector, drm_mode_vrefresh(adjusted_mode));
 
-	/*
-	 * Allow fixed refresh rate with VRR Timing Generator.
-	 * For now set the vrr.in_range to 0, to allow fixed_rr but skip actual
-	 * VRR and LRR.
-	 * #TODO For actual VRR with joiner, we need to figure out how to
-	 * correctly sequence transcoder level stuff vs. pipe level stuff
-	 * in the commit.
-	 */
-	if (crtc_state->joiner_pipes)
-		crtc_state->vrr.in_range = false;
-
 	vmin = intel_vrr_compute_vmin(crtc_state);
 
 	if (crtc_state->vrr.in_range) {
-		if (HAS_LRR(display))
+		/* FIXME sequence the LRR timing update for joined pipes */
+		if (HAS_LRR(display) && !crtc_state->joiner_pipes)
 			crtc_state->update_lrr = true;
 		vmax = intel_vrr_compute_vmax(connector, adjusted_mode);
 	} else {
@@ -832,6 +826,19 @@ void intel_vrr_check_push_sent(struct intel_dsb *dsb,
 	}
 }
 
+/*
+ * A VRR push latches all joined pipes at once, so it may be sent only
+ * after every one of them has been armed. Each pipe runs its own DSB
+ * and nothing orders the DSBs of the joined pipes against each other,
+ * so they can't sequence that between themselves. Such commits take
+ * the MMIO path, where the joined pipes are armed inside one vblank
+ * evasion critical section.
+ */
+bool intel_vrr_needs_mmio_commit(const struct intel_crtc_state *crtc_state)
+{
+	return crtc_state->vrr.enable && crtc_state->joiner_pipes;
+}
+
 bool intel_vrr_is_push_sent(const struct intel_crtc_state *crtc_state)
 {
 	struct intel_display *display = to_intel_display(crtc_state);
diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.h
index 19c7990be1b2..a72ffba47a80 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.h
+++ b/drivers/gpu/drm/i915/display/intel_vrr.h
@@ -32,6 +32,7 @@ void intel_vrr_check_push_sent(struct intel_dsb *dsb,
 			       const struct intel_crtc_state *crtc_state);
 void intel_vrr_dcb_increment_flip_count(struct intel_crtc_state *crtc_state,
 					struct intel_crtc *crtc);
+bool intel_vrr_needs_mmio_commit(const struct intel_crtc_state *crtc_state);
 bool intel_vrr_is_push_sent(const struct intel_crtc_state *crtc_state);
 void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state);
 void intel_vrr_psr_frame_change_enable(const struct intel_crtc_state *crtc_state);
-- 
2.56.0

Re: [PATCH 7/7] drm/i915/vrr: Allow VRR with the pipe joiner

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] drm/i915/vrr: use of handle instead of real name in
  Signed-off-by trailer
- [High] drm/i915/vrr: NULL deref in intel_crtc_needs_joined_update() on
  DDB change
- [High] drm/i915/vrr: broken VRR push sequencing on joined pipe DDB
  updates
--

commit 9fa0dc648f78658da0255782a3558fb950bbaa59
Author: ElXreno <elxreno@gmail.com>

> drm/i915/vrr: Allow VRR with the pipe joiner
>
> VRR has been limited to the fixed refresh rate timing generator with
> the joiner, as the transcoder level VRR operations were not sequenced
> against the pipe level updates of the joined pipes. That keeps VRR
> away from every mode that needs the joiner, such as 3840x2400@240 on
> the eDP panels of current 18" laptops.
>
> The transcoder is now programmed only by the joiner primary, and on
> the MMIO path all joined pipes are armed inside one vblank evasion
> critical section, with the push sent at its end. Take that path for
> VRR with the joiner and let VRR through.
>
> A commit whose new state has VRR enabled with the joiner does not use
> the DSB. The push must not be sent before every joined pipe has been
> armed, but each pipe runs its own DSB and nothing orders them against
> each other. Leaving the push to the DSB of the primary, on the
> grounds that it is committed last, is not enough. The secondary's DSB
> waits for the safe window after it has armed (DSB_OPCODE_WAIT_VBLANKS
> with the safe window chicken bits, then the wait in
> intel_dsb_wait_for_delayed_vblank()). When the primary's push ends
> the safe window before the secondary's DSB gets there, that DSB waits
> for the safe window of the next frame, and it is still busy when
> intel_dsb_wait() gives it 1 ms after the flip of the primary has
> completed:
>
>  [CRTC:271:pipe B] DSB 0 timed out waiting for idle (current head=0x100, head=0x0, tail=0x140)
>
> That is what a port which only skips the push on the secondaries
> gives, 13 times in 27 minutes of desktop use on Arrow Lake. The DSB
> of a secondary can't check on the push either: commit c3571a239e10
> ("drm/i915/vrr: Check that the push send bit is clear after delayed
> vblank") notes that reading TRANS_PUSH from it ends in a DSB response
> timeout.
>
> The LUT load has the same ordering problem: a colour DSB either sends
> the push itself or has to be ordered against the push that the CPU
> sends, on each pipe. So no colour DSB is prepared for such a commit
> either, and the LUTs are loaded the way they are without one: ahead
> of the commit when they can be preloaded, by the vblank worker after
> it otherwise, in which case the commit takes the per pipe critical
> sections, or with MMIO inside the critical section on the platforms
> with double buffered LUTs. The commit that turns VRR off has no VRR
> push to order and keeps its colour DSBs, which are then started from
> inside the joined critical section.
>
> One thing changes for joined modes even while VRR is off, as it
> follows vrr.in_range: the sink is told to ignore the MSA timing
> parameters during link training, as for any other mode within the
> VRR range.
>
> intel_vrr_compute_config() does not ask for LRR with the joiner, and
> DC balance is kept off, each with a FIXME. The vblank delay fastset
> that reuses the LRR code is not touched: where it applies (inherited
> state, or the always on VRR timing generator) it still writes the
> transcoder timings from every joined pipe, with identical values.
>
> Signed-off-by: ElXreno <elxreno@gmail.com>

[Severity: High]
Documentation/process/submitting-patches.rst requires a real full name in
the Author header and Signed-off-by trailer rather than a single-word handle
or nickname.

> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 668c495e74075..6543c140e2b58 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
[ ... ]
> @@ -495,21 +499,11 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>  	crtc_state->vrr.in_range =
>  		intel_vrr_is_in_range(connector, drm_mode_vrefresh(adjusted_mode));
>  
> -	/*
> -	 * Allow fixed refresh rate with VRR Timing Generator.
> -	 * For now set the vrr.in_range to 0, to allow fixed_rr but skip actual
> -	 * VRR and LRR.
> -	 * #TODO For actual VRR with joiner, we need to figure out how to
> -	 * correctly sequence transcoder level stuff vs. pipe level stuff
> -	 * in the commit.
> -	 */
> -	if (crtc_state->joiner_pipes)
> -		crtc_state->vrr.in_range = false;

[Severity: High]
Can allowing vrr.enable on joined pipes trigger a NULL pointer dereference
in intel_crtc_needs_joined_update() when another pipe's modeset changes the
DDB allocation of only one of the joined pipes?

On DISPLAY_VER >= 13, PIPE_A uses DBUF_S1|DBUF_S2 while PIPE_B shares
DBUF_S3|DBUF_S4 with PIPE_C. If PIPE_A and PIPE_B are joined with VRR
active and an atomic commit enables or disables PIPE_C,
intel_joiner_add_affected_crtcs() runs before watermark calculation when
neither PIPE_A nor PIPE_B is in the atomic state. Later,
intel_compute_global_watermarks() -> skl_compute_wm() -> skl_compute_ddb()
-> skl_crtc_allocate_ddb() adds only PIPE_B to the atomic state because
PIPE_A's DDB slices and entry did not change. During commit,
intel_update_crtc() calls intel_crtc_needs_joined_update(), which loops over
intel_crtc_joined_pipe_mask(new_crtc_state) and dereferences NULL for
PIPE_A:

drivers/gpu/drm/i915/display/intel_display.c:intel_crtc_needs_joined_update() {
    ...
	for_each_intel_crtc_in_pipe_mask(display, pipe_crtc,
					 intel_crtc_joined_pipe_mask(new_crtc_state)) {
		const struct intel_crtc_state *old_pipe_crtc_state =
			intel_atomic_get_old_crtc_state(state, pipe_crtc);
		const struct intel_crtc_state *new_pipe_crtc_state =
			intel_atomic_get_new_crtc_state(state, pipe_crtc);

		/* The DSB and the flip queue do their own vblank evasion */
		if (new_pipe_crtc_state->commit_type != INTEL_COMMIT_MMIO)
			return false;
    ...
}

[Severity: High]
When a joined pipe's DDB allocation changes while VRR is enabled, does
falling back to per-pipe updates in skl_commit_modeset_enables() break VRR
push sequencing?

In intel_crtc_needs_joined_update(), a changed wm.skl.ddb on any joined pipe
returns false so skl_commit_modeset_enables() commits the pipes one at a
time, while intel_vrr_send_push() unconditionally returns early when
intel_crtc_is_joiner_secondary(crtc_state) is true:

drivers/gpu/drm/i915/display/intel_display.c:skl_commit_modeset_enables() {
    ...
			intel_update_crtc(state, crtc);

			if (!skl_ddb_entry_equal(&new_crtc_state->wm.skl.ddb,
						 &old_crtc_state->wm.skl.ddb) &&
			    (update_pipes | modeset_pipes))
				intel_crtc_wait_for_next_vblank(crtc);
    ...
}

When the joiner secondary is committed first and its DDB changed,
intel_update_crtc() arms the secondary without sending a VRR push, and
skl_commit_modeset_enables() immediately waits for the next vblank before
updating the primary pipe, stalling until VMAX and latching the secondary a
frame ahead of the primary. Conversely, if skl_ddb_allocation_overlaps()
orders the joiner primary before the secondary, the primary sends its push
and waits for vblank in the first pass, and the secondary's subsequent
update never receives a push at all.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114014335.2132756-1-elxreno@gmail.com?part=7