From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/loongson: stop to set get_vblank_timestamp for LS7A1000 CRTC Date: Sun, 22 Mar 2026 03:09:17 +1000 Message-ID: In-Reply-To: <20260321092804.3519447-1-zhengxingda@iscas.ac.cn> References: <20260321092804.3519447-1-zhengxingda@iscas.ac.cn> <20260321092804.3519447-1-zhengxingda@iscas.ac.cn> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **The fix itself is correct in what it does** =E2=80=94 removing `.get_vbla= nk_timestamp` from the CRTC funcs table will cause the DRM core to fall bac= k to default software-based vblank timestamping, avoiding the `drm_WARN_ON_= ONCE()` at `drm_vblank.c:747` that fires when `crtc_clock =3D=3D 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 capab= ility about querying vblank on the LS7A1000 display controller," but the dr= iver 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 fo= r 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_s= can_pos` should also be removed from `ls7a1000_crtc_hw_ops` and ideally a s= eparate `lsdc_crtc_helper_funcs` without `get_scanout_position` should be u= sed 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 =3D=3D 0`, which happens before mode is= configured. This is a known issue pattern =E2=80=94 it's not that the hard= ware 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 =E2=80=94 does ls7a2000 not hit = this same warning? If so, the difference might be that ls7a2000 has `.get_v= blank_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 sti= ll be called via the shared `lsdc_crtc_helper_funcs.get_scanout_position` i= n other code paths (e.g., `drm_crtc_accurate_vblank_count()` can use `.get_= scanout_position` directly). A more thorough fix would also address the hel= per funcs. 4. **Minor**: The subject line "stop to set" reads oddly =E2=80=94 "stop se= tting" 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 capabil= ity" contradicts the existing code that populates `get_scan_pos` for ls7a10= 00. I'd like clarification on whether the scan position registers are truly= unreliable on ls7a1000, or whether this is just a timing issue during earl= y boot. If the former, more cleanup is warranted; if the latter, a more tar= geted fix (or a better commit message) would be appropriate. --- Generated by Claude Code Patch Reviewer