diff options
author | Matthew Auld <matthew.auld@intel.com> | 2023-07-19 11:38:03 +0300 |
---|---|---|
committer | Rodrigo Vivi <rodrigo.vivi@intel.com> | 2023-12-21 19:37:35 +0300 |
commit | a00b8f1aae43c46658de0f7f55d8a65acb002159 (patch) | |
tree | 275ba77984d30eba9424ecf54c5a7b6ea24f35f6 /drivers/gpu/drm/xe/xe_device.c | |
parent | 09d88e3beb64b8d2e3043fef72dda0df62487e44 (diff) | |
download | linux-a00b8f1aae43c46658de0f7f55d8a65acb002159.tar.xz |
drm/xe: fix xe_device_mem_access_get() races
It looks like there is at least one race here, given that the
pm_runtime_suspended() check looks to return false if we are in the
process of suspending the device (RPM_SUSPENDING vs RPM_SUSPENDED). We
later also do xe_pm_runtime_get_if_active(), but since the device is
suspending or has now suspended, this doesn't do anything either.
Following from this we can potentially return from
xe_device_mem_access_get() with the device suspended or about to be,
leading to broken behaviour.
Attempt to fix this by always grabbing the runtime ref when our internal
ref transitions from 0 -> 1. The hard part is then dealing with the
runtime_pm callbacks also calling xe_device_mem_access_get() and
deadlocking, which the pm_runtime_suspended() check prevented.
v2:
- ct->lock looks to be primed with fs_reclaim, so holding that and then
allocating memory will cause lockdep to complain. Now that we
unconditionally grab the mem_access.lock around mem_access_{get,put}, we
need to change the ordering wrt to grabbing the ct->lock, since some of
the runtime_pm routines can allocate memory (or at least that's what
lockdep seems to suggest). Hopefully not a big deal. It might be that
there were already issues with this, just that the atomics where
"hiding" the potential issues.
v3:
- Use Thomas Hellström' idea with tracking the active task that is
executing in the resume or suspend callback, in order to avoid
recursive resume/suspend calls deadlocking on itself.
- Split the ct->lock change.
v4:
- Add smb_mb() around accessing the pm_callback_task for extra safety.
(Thomas Hellström)
v5:
- Clarify the kernel-doc for the mem_access.lock, given that it is quite
strange in what it protects (data vs code). The real motivation is to
aid lockdep. (Rodrigo Vivi)
v6:
- Split out the lock change. We still want this as a lockdep aid but
only for the xe_device_mem_access_get() path. Sticking a lock on the
put() looks be a no-go, also the runtime_put() there is always async.
- Now that the lock is gone move to atomics and rely on the pm code
serialising multiple callers on the 0 -> 1 transition.
- g2h_worker_func() looks to be the next issue, given that
suspend-resume callbacks are using CT, so try to handle that.
v7:
- Add xe_device_mem_access_get_if_ongoing(), and use it in
g2h_worker_func().
v8 (Anshuman):
- Just always grab the rpm, instead of just on the 0 -> 1 transition,
which is a lot clearer and simplifies the code quite a bit.
v9:
- Make sure we also adjust the CT fast-path with if-active.
Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/258
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Anshuman Gupta <anshuman.gupta@intel.com>
Acked-by: Anshuman Gupta <anshuman.gupta@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Diffstat (limited to 'drivers/gpu/drm/xe/xe_device.c')
-rw-r--r-- | drivers/gpu/drm/xe/xe_device.c | 60 |
1 files changed, 47 insertions, 13 deletions
diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c index bd2e10952989..47401bb49958 100644 --- a/drivers/gpu/drm/xe/xe_device.c +++ b/drivers/gpu/drm/xe/xe_device.c @@ -364,33 +364,67 @@ u32 xe_device_ccs_bytes(struct xe_device *xe, u64 size) DIV_ROUND_UP(size, NUM_BYTES_PER_CCS_BYTE) : 0; } +bool xe_device_mem_access_ongoing(struct xe_device *xe) +{ + if (xe_pm_read_callback_task(xe) != NULL) + return true; + + return atomic_read(&xe->mem_access.ref); +} + +void xe_device_assert_mem_access(struct xe_device *xe) +{ + XE_WARN_ON(!xe_device_mem_access_ongoing(xe)); +} + bool xe_device_mem_access_get_if_ongoing(struct xe_device *xe) { - return atomic_inc_not_zero(&xe->mem_access.ref); + bool active; + + if (xe_pm_read_callback_task(xe) == current) + return true; + + active = xe_pm_runtime_get_if_active(xe); + if (active) { + int ref = atomic_inc_return(&xe->mem_access.ref); + + XE_WARN_ON(ref == S32_MAX); + } + + return active; } void xe_device_mem_access_get(struct xe_device *xe) { - bool resumed = xe_pm_runtime_resume_if_suspended(xe); - int ref = atomic_inc_return(&xe->mem_access.ref); - - if (ref == 1) - xe->mem_access.hold_rpm = xe_pm_runtime_get_if_active(xe); + int ref; + + /* + * This looks racy, but should be fine since the pm_callback_task only + * transitions from NULL -> current (and back to NULL again), during the + * runtime_resume() or runtime_suspend() callbacks, for which there can + * only be a single one running for our device. We only need to prevent + * recursively calling the runtime_get or runtime_put from those + * callbacks, as well as preventing triggering any access_ongoing + * asserts. + */ + if (xe_pm_read_callback_task(xe) == current) + return; - /* The usage counter increased if device was immediately resumed */ - if (resumed) - xe_pm_runtime_put(xe); + xe_pm_runtime_get(xe); + ref = atomic_inc_return(&xe->mem_access.ref); XE_WARN_ON(ref == S32_MAX); } void xe_device_mem_access_put(struct xe_device *xe) { - bool hold = xe->mem_access.hold_rpm; - int ref = atomic_dec_return(&xe->mem_access.ref); + int ref; + + if (xe_pm_read_callback_task(xe) == current) + return; - if (!ref && hold) - xe_pm_runtime_put(xe); + ref = atomic_dec_return(&xe->mem_access.ref); + xe_pm_runtime_put(xe); XE_WARN_ON(ref < 0); } |