[PATCH] drm/nouveau/disp/r535: Add scanline position support + head state support

[PATCH] drm/nouveau/disp/r535: Add scanline position support + head state support

[Lyude Paul]

That's right! It looks like this never actually got finished, something
which I just noticed today when I saw this fun message spamming one of my
test machine's kernel logs when enabling display debug output for nouveau:

  [drm:drm_crtc_vblank_helper_get_vblank_timestamp_internal] crtc 0 : scanoutpos query failed.

So it looks like we've been falling back to DRM's core fallback for a while
now, whoops.

So, while it seems that we do have the option of doing this through GSP -
that doesn't seem like a great idea. Mainly because reading this from GSP
would involve a lot more latency then we should have for vblank handling
due to the RPC communication. So instead of implementing that, just use
gv100_head_state and gv100_head_rgpos for implementing .state and .rgpos.
It seems to work perfectly fine!

Fixes: 9e9944449023 ("drm/nouveau/disp/r535: initial support")
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Timur Tabi <ttabi@nvidia.com>
Cc: Ben Skeggs <bskeggs@nvidia.com>
Cc: James Jones <jajones@nvidia.com>
Cc: Faith Ekstrand <faith.ekstrand@collabora.com>
Cc: Suraj Kandpal <suraj.kandpal@intel.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Aaron Kling <webgeek1234@gmail.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Zhang Enpei <zhang.enpei@zte.com.cn>
Cc: <stable@vger.kernel.org> # v6.7+
Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/nouveau/nvkm/engine/disp/gv100.c       | 4 ++--
 drivers/gpu/drm/nouveau/nvkm/engine/disp/head.h        | 2 ++
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/disp.c | 8 ++------
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/disp/gv100.c b/drivers/gpu/drm/nouveau/nvkm/engine/disp/gv100.c
index dbd984da75014..0608266188d3e 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/disp/gv100.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/disp/gv100.c
@@ -253,7 +253,7 @@ gv100_head_vblank_get(struct nvkm_head *head)
 	nvkm_mask(device, 0x611d80 + (head->id * 4), 0x00000004, 0x00000004);
 }
 
-static void
+void
 gv100_head_rgpos(struct nvkm_head *head, u16 *hline, u16 *vline)
 {
 	struct nvkm_device *device = head->disp->engine.subdev.device;
@@ -263,7 +263,7 @@ gv100_head_rgpos(struct nvkm_head *head, u16 *hline, u16 *vline)
 	*hline = nvkm_rd32(device, 0x616334 + hoff) & 0x0000ffff;
 }
 
-static void
+void
 gv100_head_state(struct nvkm_head *head, struct nvkm_head_state *state)
 {
 	struct nvkm_device *device = head->disp->engine.subdev.device;
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/disp/head.h b/drivers/gpu/drm/nouveau/nvkm/engine/disp/head.h
index 856252bf559a4..b642729c254fe 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/disp/head.h
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/disp/head.h
@@ -53,6 +53,8 @@ void gf119_head_rgclk(struct nvkm_head *, int);
 
 int gv100_head_cnt(struct nvkm_disp *, unsigned long *);
 int gv100_head_new(struct nvkm_disp *, int id);
+void gv100_head_state(struct nvkm_head *head, struct nvkm_head_state *state);
+void gv100_head_rgpos(struct nvkm_head *head, u16 *hline, u16 *vline);
 
 #define HEAD_MSG(h,l,f,a...) do {                                              \
 	struct nvkm_head *_h = (h);                                            \
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/disp.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/disp.c
index 6e63df816d855..49a1eef9bdf14 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/disp.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/disp.c
@@ -625,14 +625,10 @@ r535_head_vblank_get(struct nvkm_head *head)
 	nvkm_mask(device, 0x611d80 + (head->id * 4), 0x00000002, 0x00000002);
 }
 
-static void
-r535_head_state(struct nvkm_head *head, struct nvkm_head_state *state)
-{
-}
-
 static const struct nvkm_head_func
 r535_head = {
-	.state = r535_head_state,
+	.state = gv100_head_state,
+	.rgpos = gv100_head_rgpos,
 	.vblank_get = r535_head_vblank_get,
 	.vblank_put = r535_head_vblank_put,
 };

base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
-- 
2.54.0

Claude review: drm/nouveau/disp/r535: Add scanline position support + head state support

[Claude Code Review Bot]

Overall Series Review

Subject: drm/nouveau/disp/r535: Add scanline position support + head state support
Author: Lyude Paul <lyude@redhat.com>
Patches: 1
Reviewed: 2026-05-05T12:01:33.971366

---

This is a single-patch fix from Lyude Paul that adds scanline position (`.rgpos`) and head state (`.state`) support to the r535 GSP-RM display path in nouveau. The r535 code path previously had an empty `.state` stub and no `.rgpos` at all, causing `drm_crtc_vblank_helper_get_vblank_timestamp_internal` to fall back to the less accurate DRM core fallback for vblank timestamps.

The fix is clean and minimal: it removes the `static` keyword from `gv100_head_state()` and `gv100_head_rgpos()` in `gv100.c`, adds their declarations to `head.h`, and wires them into the `r535_head` function table. This is the same pattern used by the non-GSP gv100 head, so it's well-established code being reused.

The approach of reading the MMIO registers directly (bypassing GSP RPC) is well-justified in the commit message — vblank/scanline queries are latency-sensitive and don't need to go through the GSP firmware.

**Verdict: Looks good.** This is a correct, well-reasoned bug fix suitable for stable backport.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/nouveau/disp/r535: Add scanline position support + head state support

[Claude Code Review Bot]

Patch Review

**Correctness: Good**

The register offsets used in `gv100_head_rgpos` (0x616330, 0x616334) and `gv100_head_state` (0x682064, 0x682068, 0x68206c, 0x682070, 0x68200c, 0x682004) are display head registers that have been stable across GV100 and later generations. The r535 GSP-RM path covers Turing (TU102) and later GPUs, all of which are post-GV100, so these register offsets are appropriate. The existing `gv100_head` function table (`gv100.c:300`) uses the exact same `.state` and `.rgpos` functions, confirming compatibility.

**The fix itself:**

1. `gv100.c` — Removing `static` from `gv100_head_rgpos` and `gv100_head_state` is the minimal change needed to export them. Good.

2. `head.h` — The new declarations:
   ```c
   void gv100_head_state(struct nvkm_head *head, struct nvkm_head_state *state);
   void gv100_head_rgpos(struct nvkm_head *head, u16 *hline, u16 *vline);
   ```
   These are placed correctly alongside the other `gv100_head_*` declarations and match the function signatures in `nvkm_head_func`.

3. `r535/disp.c` — The empty stub removal and wiring:
   ```c
   r535_head = {
       .state = gv100_head_state,
       .rgpos = gv100_head_rgpos,
       .vblank_get = r535_head_vblank_get,
       .vblank_put = r535_head_vblank_put,
   };
   ```
   Clean removal of the no-op `r535_head_state` and addition of `.rgpos`. The function table now mirrors `gv100_head` (minus `.rgclk` which isn't needed in the GSP path since GSP handles clock setup).

**Minor nits (non-blocking):**

- The header declarations use named parameters (`struct nvkm_head *head`, `u16 *hline`) while the existing declarations in the same header use unnamed parameters (`struct nvkm_head *`, `u16 *`). Compare with `nv50_head_rgpos` at `head.h:48`. This is purely cosmetic and doesn't affect correctness, but matching the existing style would be slightly cleaner.

**Stable tag:** The `Cc: <stable@vger.kernel.org> # v6.7+` is appropriate since the Fixes tag points to the r535 initial support commit which landed in v6.7.

**Reviewed-by worthy:** Yes. The patch is correct, minimal, and well-motivated.

---
Generated by Claude Code Patch Reviewer