public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH v2] drm/loongson: stop to set get_vblank_timestamp for LS7A1000 CRTC
@ 2026-03-21  9:28 Icenowy Zheng
  2026-03-21  9:29 ` Icenowy Zheng
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Icenowy Zheng @ 2026-03-21  9:28 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
	Simona Vetter, Jianmin Lv, Qianhai Wu, Huacai Chen, Mingcong Bai,
	Xi Ruoyao
  Cc: dri-devel, linux-kernel, Icenowy Zheng

As there's no known hardware capability about querying vblank on the
LS7A1000 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 LS7A1000.

This solves a kernel warning when booting Linux 7.0-rc3 on a
Loongson-3A4000+LS7A1000 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>
---
Changes in v2:
- Re-formatted Loongson product model numbers per request from Huacai.

 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


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] drm/loongson: stop to set get_vblank_timestamp for LS7A1000 CRTC
  2026-03-21  9:28 [PATCH v2] drm/loongson: stop to set get_vblank_timestamp for LS7A1000 CRTC Icenowy Zheng
@ 2026-03-21  9:29 ` Icenowy Zheng
  2026-03-21 17:09 ` Claude review: " Claude Code Review Bot
  2026-03-21 17:09 ` Claude Code Review Bot
  2 siblings, 0 replies; 4+ messages in thread
From: Icenowy Zheng @ 2026-03-21  9:29 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
	Simona Vetter, Jianmin Lv, Qianhai Wu, Huacai Chen, Mingcong Bai,
	Xi Ruoyao
  Cc: dri-devel, linux-kernel

在 2026-03-21六的 17:28 +0800,Icenowy Zheng写道:
> As there's no known hardware capability about querying vblank on the
> LS7A1000 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 LS7A1000.
> 
> This solves a kernel warning when booting Linux 7.0-rc3 on a
> Loongson-3A4000+LS7A1000 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>

Oh forgot to attach when crafting v2:

```
Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
```

> ---
> Changes in v2:
> - Re-formatted Loongson product model numbers per request from
> Huacai.
> 
>  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,
>  };
>  


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Claude review: drm/loongson: stop to set get_vblank_timestamp for LS7A1000 CRTC
  2026-03-21  9:28 [PATCH v2] drm/loongson: stop to set get_vblank_timestamp for LS7A1000 CRTC Icenowy Zheng
  2026-03-21  9:29 ` Icenowy Zheng
  2026-03-21 17:09 ` Claude review: " Claude Code Review Bot
@ 2026-03-21 17:09 ` Claude Code Review Bot
  2 siblings, 0 replies; 4+ messages in thread
From: Claude Code Review Bot @ 2026-03-21 17:09 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/loongson: stop to set get_vblank_timestamp for LS7A1000 CRTC
Author: Icenowy Zheng <zhengxingda@iscas.ac.cn>
Patches: 2
Reviewed: 2026-03-22T03:09:17.309159

---

This is a single-patch fix for a kernel warning on Loongson LS7A1000 hardware. The patch removes `.get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp` from `ls7a1000_crtc_funcs`, preventing the DRM core from attempting high-precision vblank timestamping on this CRTC.

The fix is reasonable as a pragmatic workaround but deserves some scrutiny — the LS7A1000 hardware **does** appear to have scan position registers (`LSDC_CRTC0_SCAN_POS_REG` / `LSDC_CRTC1_SCAN_POS_REG`) and the shared `lsdc_crtc_helper_funcs` still sets `.get_scanout_position = lsdc_crtc_get_scanout_position` for ls7a1000 CRTCs. So the hardware capability is there in some form.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Claude review: drm/loongson: stop to set get_vblank_timestamp for LS7A1000 CRTC
  2026-03-21  9:28 [PATCH v2] drm/loongson: stop to set get_vblank_timestamp for LS7A1000 CRTC Icenowy Zheng
  2026-03-21  9:29 ` Icenowy Zheng
@ 2026-03-21 17:09 ` Claude Code Review Bot
  2026-03-21 17:09 ` Claude Code Review Bot
  2 siblings, 0 replies; 4+ messages in thread
From: Claude Code Review Bot @ 2026-03-21 17:09 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**The fix itself is correct in what it does** — removing `.get_vblank_timestamp` from the CRTC funcs table will cause the DRM core to fall back to default software-based vblank timestamping, avoiding the `drm_WARN_ON_ONCE()` at `drm_vblank.c:747` that fires when `crtc_clock == 0` during early boot (before mode is configured).

**However, I have a few concerns:**

1. **Root cause clarity**: The commit message says "no known hardware capability about querying vblank on the LS7A1000 display controller," but the driver currently has `get_scan_pos` ops populated in `ls7a1000_crtc_hw_ops` (at `lsdc_crtc.c:289,300`), and the shared `lsdc_crtc_helper_funcs` (used for both ls7a1000 and ls7a2000 at line 978) still sets `.get_scanout_position`. If the scan position registers truly don't work on ls7a1000, then `get_scan_pos` should also be removed from `ls7a1000_crtc_hw_ops` and ideally a separate `lsdc_crtc_helper_funcs` without `get_scanout_position` should be used for ls7a1000. Leaving dead code behind is confusing.

2. **Is the real problem just a timing issue?** The warning at `drm_vblank.c:747` fires when `mode->crtc_clock == 0`, which happens before mode is configured. This is a known issue pattern — it's not that the hardware can't do scanout position queries, it's that the timestamp function is called before the CRTC has a valid mode. The `ls7a2000_crtc_funcs` at line 739 still has `.get_vblank_timestamp` set — does ls7a2000 not hit this same warning? If so, the difference might be that ls7a2000 has `.get_vblank_counter` (line 736) which could affect the code path. This should be clarified.

3. **Incomplete cleanup**: If the scan position hardware truly doesn't work on ls7a1000, then `lsdc_crtc0_scan_pos` and `lsdc_crtc1_scan_pos` will still be called via the shared `lsdc_crtc_helper_funcs.get_scanout_position` in other code paths (e.g., `drm_crtc_accurate_vblank_count()` can use `.get_scanout_position` directly). A more thorough fix would also address the helper funcs.

4. **Minor**: The subject line "stop to set" reads oddly — "stop setting" or "remove" would be more natural English.

**Summary**: The one-line deletion will suppress the warning and is safe to apply. But the commit message's rationale about "no known hardware capability" contradicts the existing code that populates `get_scan_pos` for ls7a1000. I'd like clarification on whether the scan position registers are truly unreliable on ls7a1000, or whether this is just a timing issue during early boot. If the former, more cleanup is warranted; if the latter, a more targeted fix (or a better commit message) would be appropriate.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-03-21 17:09 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-21  9:28 [PATCH v2] drm/loongson: stop to set get_vblank_timestamp for LS7A1000 CRTC Icenowy Zheng
2026-03-21  9:29 ` Icenowy Zheng
2026-03-21 17:09 ` Claude review: " Claude Code Review Bot
2026-03-21 17:09 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox