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/rockchip: vop2: Add clock rate mode check Date: Sun, 12 Apr 2026 10:38:57 +1000 Message-ID: In-Reply-To: <20260409-vop2-clk-rate-check-v2-1-b207cfc427d7@collabora.com> References: <20260409-vop2-clk-rate-check-v2-1-b207cfc427d7@collabora.com> <20260409-vop2-clk-rate-check-v2-1-b207cfc427d7@collabora.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Positive aspects:** - Straightforward approach using the existing `mode_valid` callback pattern= , consistent with the existing `max_output.width` check just above. - Naming (`max_pixel_clock_rate`) is clear and distinguishes from the VOP i= nput clock. - Values for rk3568 (600/200/150 MHz) and rk3588 VP3 (150 MHz) look reasona= ble and straightforward. **Issues:** 1. **rk3576 VP0 multiplier inconsistency (medium concern):** The rk3576 VP0= sets `max_pixel_clock_rate =3D 600000000U * 4` (2.4 GHz effective), but it= s `pixel_rate` is set to `2` (line 785), not 4. The rk3576 `set_intf_mux` a= t line 1496 divides by `port_pix_rate` (which is 2), and when `crtc_clock >= 600000` it applies `dclk_div =3D 2`. This suggests the hardware does 2 pix= els per cycle, not 4, giving a max effective pixel clock of 600 MHz * 2 =3D= 1.2 GHz. The `* 4` multiplier may be too permissive for rk3576, allowing m= odes the hardware cannot actually drive. This should be confirmed with Andy= Yan =E2=80=94 it might need to be `* 2` instead. 2. **rk3588 VP0-VP2 have no `pixel_rate` set (informational):** The rk3588 = video ports VP0-VP2 get `max_pixel_clock_rate =3D 600000000U * 4` but don't= set the `pixel_rate` field (defaulting to 0). The rk3588 code path uses a = separate `rk3588_calc_cru_cfg()` function that hardcodes `dclk_core_rate = =3D v_pixclk >> 2` rather than reading `pixel_rate`, so this is correct beh= avior =E2=80=94 but it's worth noting the asymmetry with rk3576 for future = maintainability. 3. **No guard against `max_pixel_clock_rate =3D=3D 0` (minor):** If a futur= e SoC variant is added and the developer forgets to set `max_pixel_clock_ra= te`, the field defaults to 0, and the check: ```c if (mode->clock > vp->data->max_pixel_clock_rate / 1000) return MODE_CLOCK_HIGH; ``` would reject **all** modes (since `mode->clock` is always > 0). This is = a fail-closed behavior which is arguably safe (you'd notice immediately), b= ut a defensive guard like: ```c if (vp->data->max_pixel_clock_rate && mode->clock > vp->data->max_pixel_clock_rate / 1000) ``` would be more forgiving during bringup of new SoCs. Alternatively, a `WA= RN_ON(!vp->data->max_pixel_clock_rate)` during probe would catch the missin= g initialization early. 4. **Integer arithmetic precision (nit):** The comparison `mode->clock > vp= ->data->max_pixel_clock_rate / 1000` truncates via integer division. For th= e values used here (all multiples of 1,000,000), this is fine. But if a fut= ure value isn't evenly divisible by 1000, the truncation would make the lim= it slightly stricter than intended. Not an issue with current values. 5. **Commit message clarity (nit):** The commit message says "up to 4 pixel= s in parallel when needed resulting in the extra multiplier" but doesn't ex= plain *which* specific ports get the multiplier and why. Since this is the = core question the RFC is asking about, it would help reviewers to have a ta= ble in the commit message mapping each SoC/VP to its base clock, multiplier= , and effective max pixel clock. --- Generated by Claude Code Patch Reviewer