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

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

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

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

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

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

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

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