Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] mmio: fixed 10us delay in long waits causes hrtimer/context switch storms --
commit c92012bfe0677557677a0dcb08c9a338f44452fd Author: Alan Previn <[email protected]> Subject: drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums This commit replaces the custom wait implementation in the driver with the standard poll_timeout_us and poll_timeout_us_atomic macros. It drops the atomic flag from xe_mmio_wait32 parameters, updating callers and adding explicit _atomic variants where needed. > diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c > --- a/drivers/gpu/drm/xe/xe_guc.c > +++ b/drivers/gpu/drm/xe/xe_guc.c [ ... ] > @@ -1574,7 +1574,7 @@ int xe_guc_mmio_send_recv(struct xe_guc *guc, const u32 > *request, > BUILD_BUG_ON((GUC_HXG_TYPE_RESPONSE_SUCCESS ^ > GUC_HXG_TYPE_RESPONSE_FAILURE) != 1); > > ret = xe_mmio_wait32(mmio, reply_reg, resp_mask, resp_mask, > - 2000000, &header, false); > + 2000000, &header); [Severity: High] Does this change cause an excessive number of hrtimers or context switches for long waits? For example, in xe_guc_mmio_send_recv(), the 2,000,000us timeout here could result in ~200,000 sleep/wake cycles if the hardware is slow or hits the timeout. The previous custom wait implementation used an exponential backoff to handle these long wait periods without flooding the system. [ ... ] > diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c > --- a/drivers/gpu/drm/xe/xe_mmio.c > +++ b/drivers/gpu/drm/xe/xe_mmio.c [ ... ] > int xe_mmio_wait32(struct xe_mmio *mmio, struct xe_reg reg, u32 mask, u32 > val, u32 timeout_us, > - u32 *out_val, bool atomic) > + u32 *out_val) > { > - return __xe_mmio_wait32(mmio, reg, mask, val, timeout_us, out_val, > atomic, true); > + u32 read; > + int ret; > + > + ret = poll_timeout_us(read = xe_mmio_read32(mmio, reg), (read & mask) > == val, > + 10, timeout_us, false); [Severity: High] Will hardcoding a 10us sleep interval here cause context switch storms and PCIe bus flooding during long operations? Certain callers like __xe_driver_flr() wait up to 3 seconds, and xe_gsc_wait_for_proxy_init_done() waits up to 1 second. Using a fixed 10us interval means those paths might invoke usleep_range() tens of thousands of times if the full timeout is reached. Could this degrade system performance, and does it warrant re-introducing an exponential backoff scaling for longer wait periods instead of a fixed interval? > + if (out_val) > + *out_val = read; > + > + return ret; > } [ ... ] > int xe_mmio_wait32_not(struct xe_mmio *mmio, struct xe_reg reg, u32 mask, > u32 val, u32 timeout_us, > - u32 *out_val, bool atomic) > + u32 *out_val) > { > - return __xe_mmio_wait32(mmio, reg, mask, val, timeout_us, out_val, > atomic, false); > + u32 read; > + int ret; > + > + ret = poll_timeout_us(read = xe_mmio_read32(mmio, reg), (read & mask) > != val, > + 10, timeout_us, false); [Severity: High] Similar to xe_mmio_wait32() above, does applying a fixed 10us sleep interval in xe_mmio_wait32_not() risk hrtimer storms for callers with large timeouts? > + if (out_val) > + *out_val = read; > + > + return ret; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
