Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/imagination: inverted PVR_FEATURE_VALUE() check in layout_is_mars() - [High] drm/imagination: wrong default in must_wait_fw_idle_before_stop() --
commit 37bd66d7d521adbf18d35a370cc34ba7ca02c2dd Author: Alessio Belle <[email protected]> drm/imagination: Limit access to the META firmware interface This commit abstracts META firmware-specific operations behind struct pvr_fw_defs callbacks and properties so that pvr_fw_startstop.c no longer accesses the META firmware interface directly. It also renames pvr_rogue_meta.h to pvr_rogue_meta_internal.h and removes pvr_fw_meta.h. > diff --git a/drivers/gpu/drm/imagination/pvr_fw.h > b/drivers/gpu/drm/imagination/pvr_fw.h > index 4b25291135b69..610406668bbb2 100644 > --- a/drivers/gpu/drm/imagination/pvr_fw.h > +++ b/drivers/gpu/drm/imagination/pvr_fw.h > @@ -184,6 +184,23 @@ struct pvr_fw_defs { > */ > void (*irq_clear)(struct pvr_device *pvr_dev); > > + /** > + * @must_wait_fw_idle: Check if the FW stop sequence should wait for > hardware > + * to report FW idle before proceeding. > + * > + * @pvr_dev: Target PowerVR device. > + * @wait[out]: Whether the FW stop sequence should wait for FW idle. > + * > + * This function is optional. The assumption is that we must always wait > + * for the firmware to reach idle; implement this function and return > + * %false to alter this behavior. > + * > + * Returns: > + * * 0 on success. > + * * Any appropriate error on failure. > + */ > + int (*must_wait_fw_idle)(struct pvr_device *pvr_dev, bool *wait); [ ... ] > diff --git a/drivers/gpu/drm/imagination/pvr_fw_startstop.c > b/drivers/gpu/drm/imagination/pvr_fw_startstop.c > index 98ca2692239d0..9a617f78256ed 100644 > --- a/drivers/gpu/drm/imagination/pvr_fw_startstop.c > +++ b/drivers/gpu/drm/imagination/pvr_fw_startstop.c [ ... ] > @@ -199,6 +197,30 @@ pvr_fw_start(struct pvr_device *pvr_dev) > return err; > } > > +static inline bool layout_is_mars(struct pvr_device *pvr_dev) > +{ > + u64 layout_mars_value = 0; > + > + if (PVR_FEATURE_VALUE(pvr_dev, layout_mars, &layout_mars_value)) > + return layout_mars_value > 0; > + > + return false; > +} [Severity: High] Is the PVR_FEATURE_VALUE() check inverted in layout_is_mars()? In drivers/gpu/drm/imagination/pvr_device.h, PVR_FEATURE_VALUE() returns 0 when the feature is present and -EINVAL when it is absent: #define PVR_FEATURE_VALUE(pvr_dev, feature, value_out) \ ({ \ struct pvr_device *_pvr_dev = pvr_dev; \ int _ret = -EINVAL; \ if (_pvr_dev->features.has_##feature) { \ *(value_out) = _pvr_dev->features.feature; \ _ret = 0; \ } \ _ret; \ }) Prior to this change, pvr_fw_stop() checked PVR_FEATURE_VALUE(...) == 0. Without the == 0 comparison, layout_is_mars() returns false when has_layout_mars is true (since PVR_FEATURE_VALUE() returns 0), and also returns false when has_layout_mars is false (since layout_mars_value remains 0). As a result, when pvr_fw_stop() calls layout_is_mars(), layout_mars is always false, causing pvr_fw_stop() on cores with the layout_mars feature to poll powered-down ROGUE_CR_SIDEKICK_IDLE and ROGUE_CR_SLC_IDLE registers and skip polling ROGUE_CR_MARS_IDLE before asserting ROGUE_CR_SOFT_RESET. > + > +static inline int must_wait_fw_idle_before_stop(struct pvr_device *pvr_dev) > +{ > + bool wait_fw_idle = false; > + int err; > + > + if (pvr_dev->fw_dev.defs->must_wait_fw_idle) { > + err = pvr_dev->fw_dev.defs->must_wait_fw_idle(pvr_dev, > &wait_fw_idle); > + if (err) > + return err; > + } > + > + return wait_fw_idle; > +} [Severity: High] Should wait_fw_idle be initialized to true instead of false in must_wait_fw_idle_before_stop()? The kernel-doc for must_wait_fw_idle in struct pvr_fw_defs states that the callback is optional and that waiting for the firmware to reach idle is the default behavior when it is not implemented. Neither pvr_fw_defs_mips in drivers/gpu/drm/imagination/pvr_fw_mips.c nor pvr_fw_defs_riscv in drivers/gpu/drm/imagination/pvr_fw_riscv.c implements must_wait_fw_idle. Because wait_fw_idle defaults to false here, calling pvr_fw_stop() -> must_wait_fw_idle_before_stop() on MIPS or RISC-V devices returns 0, causing pvr_fw_stop() to skip polling ROGUE_CR_SIDEKICK_IDLE for ROGUE_CR_SIDEKICK_IDLE_GARTEN_EN before asserting ROGUE_CR_SOFT_RESET, whereas previously skip_garten_idle defaulted to false. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20261029-prep-meta-fw-arch-independent-v2-0-b9f5d69b7...@imgtec.com?part=4
