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/imagination: Implement handling of context reset notification Date: Fri, 13 Mar 2026 14:16:58 +1000 Message-ID: In-Reply-To: <20260312-b4-firmware-context-reset-notification-handling-v2-3-aec5a64cb06f@imgtec.com> References: <20260312-b4-firmware-context-reset-notification-handling-v2-0-aec5a64cb06f@imgtec.com> <20260312-b4-firmware-context-reset-notification-handling-v2-3-aec5a64cb06f@imgtec.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 This is the main patch. It adds a new `pvr_dump.c` / `pvr_dump.h` and hooks= the `ROGUE_FWIF_FWCCB_CMD_CONTEXT_RESET_NOTIFICATION` case into `process_f= wccb_command()`. **Issues and observations:** 1. **Typo in commit message:** "deecoding" should be "decoding": > `part of handling hardware recovery (HWR) events deecoding the message` 2. **Missing Co-authored-by tag format:** The `Co-authored-by:` tag is pres= ent but there is no `Signed-off-by` from Sarah Walker. Kernel convention ty= pically expects a `Signed-off-by` from all co-authors to certify DCO compli= ance. 3. **`get_dm_name()` comment about PVR_FWIF_DM_TDM:** The code has: ```c /* PVR_FWIF_DM_TDM has the same index, but is discriminated by a device = feature */ case PVR_FWIF_DM_2D: return "2D or TDM"; ``` Since `PVR_FWIF_DM_2D` and `PVR_FWIF_DM_TDM` both equal 1 (confirmed in = `pvr_rogue_fwif_common.h:26,31`), you can't have separate `case` labels. Th= e "2D or TDM" string is a reasonable compromise, but it could be improved b= y checking the device features at runtime (via `pvr_dev`) to print the corr= ect name. The `pvr_device` is already passed to the caller. This is a minor= nit though =E2=80=94 the current approach is functional. 4. **Cast from u32 to enum in `get_reset_reason_desc` call:** ```c get_reset_reason_desc((enum rogue_context_reset_reason)data->reset_reaso= n) ``` This is fine functionally, but the `default: return "Unknown"` case in `= get_reset_reason_desc()` properly handles unexpected values, which is good.= However, this cast slightly contradicts the spirit of patch 2 which change= d the field to `u32` precisely because enum sizes aren't fixed. Consider ac= cepting a `u32` parameter in `get_reset_reason_desc()` instead, which would= be more consistent. This is a minor style point. 5. **`pvr_dump.c` / `pvr_dump.h` naming:** The file name `pvr_dump` is quit= e generic. Right now it only contains context reset notification handling. = If more dump-type functionality is expected, the name is fine. If this is s= pecifically about FW CCB notification handling, a more specific name might = be clearer. This is subjective and not blocking. 6. **Alignment of function parameter continuation in pvr_ccb.c:** ```c pvr_dump_context_reset_notification(pvr_dev, &cmd->cmd_data.cmd_context_reset_notification); ``` The continuation line alignment looks slightly off (tabs vs spaces mix),= but this matches the surrounding code style in the file, so it's acceptabl= e. 7. **Log level consideration:** All notifications are printed via `drm_info= ()`. For high-frequency HWR events this could be noisy. A rate-limited vari= ant (`drm_info_ratelimited()`) or using `drm_dbg()` for the detail lines mi= ght be worth considering, but this is a design choice the maintainers can w= eigh in on. **Overall:** The series is straightforward and correct. The main actionable= items are the commit message typo and the missing `Signed-off-by` from the= co-author. --- Generated by Claude Code Patch Reviewer