From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: accel/ivpu: Add support for limiting NPU frequency Date: Sun, 12 Apr 2026 12:24:15 +1000 Message-ID: In-Reply-To: <20260408130827.2082946-1-andrzej.kacprowski@linux.intel.com> References: <20260408130827.2082946-1-andrzej.kacprowski@linux.intel.com> <20260408130827.2082946-1-andrzej.kacprowski@linux.intel.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 **Bug: Duplicate declaration in ivpu_jsm_msg.h** The header has the function declared twice (lines 600-601 of the mbox): ```c int ivpu_jsm_msg_freq_config(struct ivpu_device *vdev, u16 min_ratio, u16 p= n_ratio, u16 max_ratio); int ivpu_jsm_msg_freq_config(struct ivpu_device *vdev, u16 min_ratio, u16 p= n_ratio, u16 max_ratio); ``` This is an obvious copy-paste error. Remove the duplicate. **Bug: Wrong path in obsolete ABI documentation** In `Documentation/ABI/obsolete/sysfs-driver-ivpu` (line 146): ``` Description: Legacy alias for /sys/bus/pci/drivers/intel_vpu/.../freq/hw_mi= n_freq. Shows maximum frequency in MHz of the NPU's data processing unit. ``` The attribute is `npu_max_frequency_mhz` and shows the **maximum** frequenc= y, but the description says it's an alias for `freq/hw_min_freq`. It should= be `freq/hw_max_freq`. The code confirms the alias uses `hw_max_freq_show`: ```c static struct device_attribute dev_attr_npu_max_frequency_mhz =3D __ATTR(npu_max_frequency_mhz, 0444, hw_max_freq_show, NULL); ``` **Type inconsistency in ivpu_jsm_msg_freq_config** The function is declared and defined with `u16` parameters: ```c int ivpu_jsm_msg_freq_config(struct ivpu_device *vdev, u16 min_ratio, u16 p= n_ratio, u16 max_ratio) ``` But it's called from `ivpu_hw_btrs_cfg_freq_set` which passes `u8` values, = and the struct fields (`cfg_min_ratio`, `cfg_max_ratio`, `pn_ratio`) are al= l `u8`. The JSM payload fields (`vpu_ipc_msg_payload_freq_config`) are `u32= `. The `u16` parameter type serves no purpose =E2=80=94 it should be `u8` t= o match the callers and the hw_info struct, avoiding confusion about the ex= pected value range. **dpu_mhz_to_pll_ratio_lnl is LNL-specific but used generically** The function `dpu_mhz_to_pll_ratio_lnl` (line 463) uses the LNL conversion = factor (`PLL_REF_CLK_FREQ_MHZ / 2` =3D 25): ```c static u8 dpu_mhz_to_pll_ratio_lnl(u32 freq_mhz) { return clamp_t(u32, freq_mhz / (PLL_REF_CLK_FREQ_MHZ / 2), 0, U8_MAX); } ``` This is called from both `ivpu_hw_btrs_cfg_max_freq_set` and `ivpu_hw_btrs_= cfg_min_freq_set`, which are gated in sysfs to 50XX+ only. While 50XX+ is L= NL generation, naming it `_lnl` and having no platform check in the btrs-le= vel functions is fragile. If a future caller forgets the gating, it would s= ilently use the wrong conversion for MTL. At minimum, rename to drop the `_= lnl` suffix since it's used as the general implementation, or add an `ivpu_= hw_btrs_gen()` check inside the set functions. **Missing locking for cfg_min_ratio/cfg_max_ratio** The `cfg_min_ratio` and `cfg_max_ratio` fields are read and written without= any synchronization. Consider this race: 1. Thread A calls `set_min_freq_store` =E2=86=92 `ivpu_hw_btrs_cfg_min_freq= _set` =E2=86=92 reads `vdev->hw->pll.cfg_max_ratio` 2. Thread B calls `set_max_freq_store` =E2=86=92 `ivpu_hw_btrs_cfg_max_freq= _set` =E2=86=92 writes `vdev->hw->pll.cfg_max_ratio` Both threads enter `ivpu_hw_btrs_cfg_freq_set`, which sends a JSM message a= nd then updates both fields without atomicity. This can lead to inconsisten= t state. A mutex should protect the read-modify-write of these fields. **Stale comment in set_max_freq_store** Line 754 of the mbox: ```c /* Convert MHz to Hz and set max frequency */ ``` The function passes MHz directly to `ivpu_hw_btrs_cfg_max_freq_set`, not Hz= . The comment is wrong and should be removed. **sysfs_add_file_to_group files not covered by devm cleanup** The `freq/` attribute group is added via `devm_device_add_group`, which han= dles cleanup automatically. But the conditional attributes (`set_min_freq`,= `set_max_freq`) are added via `sysfs_add_file_to_group`, which has no devm= variant. These files won't be automatically removed when the device is unb= ound. You need either: - A manual cleanup path using `sysfs_remove_file_from_group`, or - Include these attributes in the group's `is_visible` callback to conditio= nally show them instead of conditionally adding them. The `is_visible` approach would be cleaner =E2=80=94 define a single group = with all attributes and use `.is_visible` to hide `set_min_freq`/`set_max_f= req` on pre-50XX hardware. **Boot path placement concern** The patch context shows `ivpu_hw_btrs_cfg_freq_init` is added inside a cond= itional block. The current drm-next tree has this as `!ivpu_fw_is_warm_boot= (vdev)`, meaning the frequency configuration would not be re-sent to firmwa= re after warm boot (resume). If the firmware doesn't preserve frequency set= tings across warm boot, user-configured limits will be silently lost after = suspend/resume. Please verify whether this is intentional. **Minor: const qualifier missing on ivpu_freq_attr_group** ```c static struct attribute_group ivpu_freq_attr_group =3D { ``` Should be `static const struct attribute_group`. **Minor: Precision regression for PARAM_CORE_CLOCK_RATE on MTL** The old code computed frequency in Hz directly using 64-bit arithmetic: ```c (ratio * 50000000ull * 2) / 3 // old, in Hz ``` The new code computes MHz first, then converts back to Hz: ```c (ratio * 50 * 2) / 3 * 1000000 // new ``` For MTL ratios not evenly divisible by 3 (after multiplication), this intro= duces rounding error up to ~333 KHz in the `DRM_IVPU_PARAM_CORE_CLOCK_RATE`= ioctl return value. While small, this is a user-visible UAPI behavioral ch= ange. --- Generated by Claude Code Patch Reviewer