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

[sashiko-bot@kernel.org]

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