[PATCH] drm/loongson: stop to set get_vblank_timestamp for 7A1000 CRTC

[PATCH] drm/loongson: stop to set get_vblank_timestamp for 7A1000 CRTC

[Icenowy Zheng]

As there's no known hardware capability about querying vblank on the
7A1000 display controller, setting get_vblank_timestamp will mislead the
kernel about the support of DC-backed high precision vblank query.

Drop this function pointer in the CRTC function table for 7A1000.

This solves a kernel warning when booting Linux 7.0-rc3 on a
3A4000+7A1000 Haier Boyue G51 laptop (with injected EDID for replicating
the display timing set by the firmware).

Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
---
 drivers/gpu/drm/loongson/lsdc_crtc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/loongson/lsdc_crtc.c b/drivers/gpu/drm/loongson/lsdc_crtc.c
index 587fbe285e9ef..b3af8e0cdb15f 100644
--- a/drivers/gpu/drm/loongson/lsdc_crtc.c
+++ b/drivers/gpu/drm/loongson/lsdc_crtc.c
@@ -721,7 +721,6 @@ static const struct drm_crtc_funcs ls7a1000_crtc_funcs = {
 	.late_register = lsdc_crtc_late_register,
 	.enable_vblank = lsdc_crtc_enable_vblank,
 	.disable_vblank = lsdc_crtc_disable_vblank,
-	.get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp,
 	.atomic_print_state = lsdc_crtc_atomic_print_state,
 };
 
-- 
2.52.0

Claude review: drm/loongson: stop to set get_vblank_timestamp for 7A1000 CRTC

[Claude Code Review Bot]

Overall Series Review

Subject: drm/loongson: stop to set get_vblank_timestamp for 7A1000 CRTC
Author: Icenowy Zheng <zhengxingda@iscas.ac.cn>
Patches: 1
Reviewed: 2026-03-16T11:56:46.669899

---

This is a single-patch series that removes the `.get_vblank_timestamp` callback from the `ls7a1000_crtc_funcs` table in the Loongson DRM driver. The stated rationale is that the 7A1000 display controller lacks hardware capability for vblank querying, and setting this callback misleads the kernel.

However, the patch's rationale is questionable. The 7A1000 **does** have `get_scan_pos` hardware ops implemented (lines 289, 301 in `lsdc_crtc.c`), and the shared `lsdc_crtc_helper_funcs` (line 939) sets `.get_scanout_position = lsdc_crtc_get_scanout_position` for both 7A1000 and 7A2000 CRTCs. The `drm_crtc_vblank_helper_get_vblank_timestamp` function simply calls `get_scanout_position` from the helper funcs, so the 7A1000 does have the necessary plumbing in place.

The patch may paper over a real bug rather than fix the root cause. The warning the author encounters is more likely caused by `drm_calc_timestamping_constants()` not being called before vblank timestamp queries are made, or by the mode's `crtc_clock` being zero when using firmware-injected EDID timings. Removing the timestamp callback entirely downgrades vblank precision for **all** 7A1000 users rather than fixing the actual issue.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/loongson: stop to set get_vblank_timestamp for 7A1000 CRTC

[Claude Code Review Bot]

Patch Review

**Concern: The commit message rationale doesn't match the code.**

The commit message states:
> As there's no known hardware capability about querying vblank on the 7A1000 display controller

But the 7A1000 **does** have scanout position querying support. The `ls7a1000_crtc_hw_ops` array provides `.get_scan_pos` for both CRTCs (lines 289, 301), and the driver hooks `lsdc_crtc_get_scanout_position` (line 898) which calls `ops->get_scan_pos()` at line 923. The `ls7a1000_crtc_init` function at line 977 adds `lsdc_crtc_helper_funcs` (which includes `get_scanout_position`) to 7A1000 CRTCs. So the hardware and driver **do** support scanout position queries, which is all that `drm_crtc_vblank_helper_get_vblank_timestamp` needs.

The comment at lines 307-313 is more nuanced:
```c
/*
 * The 32-bit hardware vblank counter has been available since LS7A2000
 * and LS2K2000. The counter increases even though the CRTC is disabled,
 * it will be reset only if the CRTC is being soft reset.
 * Those registers are also readable for ls7a1000, but its value does not
 * change.
 */
```

This explains why 7A1000 doesn't have `.get_vblank_counter` (the hardware counter doesn't work), but scanout position is a different mechanism from the vblank counter — and the 7A1000 does support scanout position queries.

**Concern: This is likely a workaround, not a proper fix.**

The author reports a kernel warning on boot "with injected EDID for replicating the display timing set by the firmware." The warning is not included in the commit message, making it hard to assess the actual root cause. If the warning comes from `drm_calc_timestamping_constants()` or from `drm_crtc_vblank_helper_get_vblank_timestamp_internal()` due to invalid mode timing (e.g., `crtc_clock == 0`), then the proper fix would be to ensure correct timestamping constants are computed, not to disable high-precision vblank timestamps entirely.

**Concern: This degrades functionality for all 7A1000 users.**

Removing `get_vblank_timestamp` means the kernel falls back to `do_gettimeofday()`-based timestamps, which are much less precise. This affects compositors and applications that rely on accurate vblank timing (e.g., Wayland compositors, video playback). This is a regression for 7A1000 systems where scanout position queries work correctly.

**Recommendations:**

1. Include the actual kernel warning/backtrace in the commit message so reviewers can assess the root cause.
2. Investigate whether the issue is specific to the "injected EDID" scenario (incorrect `crtc_clock` or `crtc_htotal`/`crtc_vtotal` values) rather than a fundamental hardware limitation.
3. If `get_scan_pos` truly doesn't work on 7A1000 hardware, then `.get_scanout_position` should **also** be removed from the helper funcs for 7A1000 (perhaps by using a separate `ls7a1000_crtc_helper_funcs` without it). Leaving `get_scanout_position` in the helpers while removing `get_vblank_timestamp` from the CRTC funcs is inconsistent.
4. Note that the 7A2000 at line 738 retains both `.get_vblank_counter` and `.get_vblank_timestamp` — the difference being the working hardware vblank counter, but that is orthogonal to the scanout position mechanism used by `get_vblank_timestamp`.

---
Generated by Claude Code Patch Reviewer