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 7A1000 CRTC Date: Mon, 16 Mar 2026 11:56:47 +1000 Message-ID: In-Reply-To: <20260314165421.1201790-1-zhengxingda@iscas.ac.cn> References: <20260314165421.1201790-1-zhengxingda@iscas.ac.cn> <20260314165421.1201790-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 **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 7A10= 00 display controller But the 7A1000 **does** have scanout position querying support. The `ls7a10= 00_crtc_hw_ops` array provides `.get_scan_pos` for both CRTCs (lines 289, 3= 01), 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_pos= ition`) to 7A1000 CRTCs. So the hardware and driver **do** support scanout = position queries, which is all that `drm_crtc_vblank_helper_get_vblank_time= stamp` 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 c= ounter doesn't work), but scanout position is a different mechanism from th= e vblank counter =E2=80=94 and the 7A1000 does support scanout position que= ries. **Concern: This is likely a workaround, not a proper fix.** The author reports a kernel warning on boot "with injected EDID for replica= ting the display timing set by the firmware." The warning is not included i= n the commit message, making it hard to assess the actual root cause. If th= e 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 =3D=3D 0`), then the proper fix would be to ensure correc= t 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_gettimeo= fday()`-based timestamps, which are much less precise. This affects composi= tors and applications that rely on accurate vblank timing (e.g., Wayland co= mpositors, video playback). This is a regression for 7A1000 systems where s= canout position queries work correctly. **Recommendations:** 1. Include the actual kernel warning/backtrace in the commit message so rev= iewers can assess the root cause. 2. Investigate whether the issue is specific to the "injected EDID" scenari= o (incorrect `crtc_clock` or `crtc_htotal`/`crtc_vtotal` values) rather tha= n a fundamental hardware limitation. 3. If `get_scan_pos` truly doesn't work on 7A1000 hardware, then `.get_scan= out_position` should **also** be removed from the helper funcs for 7A1000 (= perhaps by using a separate `ls7a1000_crtc_helper_funcs` without it). Leavi= ng `get_scanout_position` in the helpers while removing `get_vblank_timesta= mp` from the CRTC funcs is inconsistent. 4. Note that the 7A2000 at line 738 retains both `.get_vblank_counter` and = `.get_vblank_timestamp` =E2=80=94 the difference being the working hardware= vblank counter, but that is orthogonal to the scanout position mechanism u= sed by `get_vblank_timestamp`. --- Generated by Claude Code Patch Reviewer