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

[sashiko-bot@kernel.org]

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