[Intel-xe] [PATCH v2 2/3] drm/xe: fix xe_device_mem_access_get() race
Matthew Auld
matthew.auld at intel.com
Fri May 5 18:03:48 UTC 2023
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, and then wrap the whole thing with a lock
to ensure callers are serialized.
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.
Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/258
Signed-off-by: Matthew Auld <matthew.auld at intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>
Cc: Matthew Brost <matthew.brost at intel.com>
---
drivers/gpu/drm/xe/xe_device.c | 11 +++--------
drivers/gpu/drm/xe/xe_device_types.h | 5 ++++-
drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c | 4 ++++
drivers/gpu/drm/xe/xe_guc_ct.c | 13 +++++++------
drivers/gpu/drm/xe/xe_pm.c | 9 ++-------
drivers/gpu/drm/xe/xe_pm.h | 2 +-
6 files changed, 21 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index 01c497bcf9a5..0a18b41a0e1a 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -406,17 +406,12 @@ u32 xe_device_ccs_bytes(struct xe_device *xe, u64 size)
void xe_device_mem_access_get(struct xe_device *xe)
{
- bool resumed = xe_pm_runtime_resume_if_suspended(xe);
-
mutex_lock(&xe->mem_access.lock);
- if (xe->mem_access.ref++ == 0)
- xe->mem_access.hold_rpm = xe_pm_runtime_get_if_active(xe);
+ if (xe->mem_access.ref == 0)
+ xe->mem_access.hold_rpm = xe_pm_runtime_resume_and_get(xe);
+ xe->mem_access.ref++;
mutex_unlock(&xe->mem_access.lock);
- /* The usage counter increased if device was immediately resumed */
- if (resumed)
- xe_pm_runtime_put(xe);
-
XE_WARN_ON(xe->mem_access.ref == S32_MAX);
}
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index 59462933f67a..9e37189d5745 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -256,7 +256,10 @@ struct xe_device {
* triggering additional actions when they occur.
*/
struct {
- /** @lock: protect the ref count */
+ /**
+ * @lock: Serialize xe_device_mem_access users,
+ * and protect the below internal state, like @ref.
+ */
struct mutex lock;
/** @ref: ref count of memory accesses */
s32 ref;
diff --git a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
index 604f189dbd70..e769af7e7cdb 100644
--- a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
+++ b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
@@ -5,6 +5,7 @@
#include "xe_gt_tlb_invalidation.h"
+#include "xe_device.h"
#include "xe_gt.h"
#include "xe_guc.h"
#include "xe_guc_ct.h"
@@ -112,6 +113,8 @@ static int send_tlb_invalidation(struct xe_guc *guc,
* in order which they currently are, if that changes the algorithm will
* need to be updated.
*/
+
+ xe_device_mem_access_get(gt->xe);
mutex_lock(&guc->ct.lock);
seqno = gt->tlb_invalidation.seqno;
if (fence) {
@@ -140,6 +143,7 @@ static int send_tlb_invalidation(struct xe_guc *guc,
if (ret < 0 && fence)
invalidation_fence_signal(fence);
mutex_unlock(&guc->ct.lock);
+ xe_device_mem_access_put(gt->xe);
return ret;
}
diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
index 9055ff133a7c..579d7f341f13 100644
--- a/drivers/gpu/drm/xe/xe_guc_ct.c
+++ b/drivers/gpu/drm/xe/xe_guc_ct.c
@@ -494,26 +494,22 @@ static int __guc_ct_send_locked(struct xe_guc_ct *ct, const u32 *action,
}
}
- xe_device_mem_access_get(ct_to_xe(ct));
retry:
ret = has_room(ct, len + GUC_CTB_HDR_LEN, g2h_len);
if (unlikely(ret))
- goto put_wa;
+ goto out;
ret = h2g_write(ct, action, len, g2h_fence ? g2h_fence->seqno : 0,
!!g2h_fence);
if (unlikely(ret)) {
if (ret == -EAGAIN)
goto retry;
- goto put_wa;
+ goto out;
}
g2h_reserve_space(ct, g2h_len, num_g2h);
xe_guc_notify(ct_to_guc(ct));
-put_wa:
- xe_device_mem_access_put(ct_to_xe(ct));
out:
-
return ret;
}
@@ -535,6 +531,7 @@ static int guc_ct_send_locked(struct xe_guc_ct *ct, const u32 *action, u32 len,
XE_BUG_ON(g2h_len && g2h_fence);
lockdep_assert_held(&ct->lock);
+ xe_device_assert_mem_access(ct_to_xe(ct));
try_again:
ret = __guc_ct_send_locked(ct, action, len, g2h_len, num_g2h,
@@ -602,10 +599,14 @@ static int guc_ct_send(struct xe_guc_ct *ct, const u32 *action, u32 len,
XE_BUG_ON(g2h_len && g2h_fence);
+ xe_device_mem_access_get(ct_to_xe(ct));
+
mutex_lock(&ct->lock);
ret = guc_ct_send_locked(ct, action, len, g2h_len, num_g2h, g2h_fence);
mutex_unlock(&ct->lock);
+ xe_device_mem_access_put(ct_to_xe(ct));
+
return ret;
}
diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
index b7b57f10ba25..b2ffa001e6f7 100644
--- a/drivers/gpu/drm/xe/xe_pm.c
+++ b/drivers/gpu/drm/xe/xe_pm.c
@@ -210,14 +210,9 @@ int xe_pm_runtime_put(struct xe_device *xe)
return pm_runtime_put_autosuspend(xe->drm.dev);
}
-/* Return true if resume operation happened and usage count was increased */
-bool xe_pm_runtime_resume_if_suspended(struct xe_device *xe)
+bool xe_pm_runtime_resume_and_get(struct xe_device *xe)
{
- /* In case we are suspended we need to immediately wake up */
- if (pm_runtime_suspended(xe->drm.dev))
- return !pm_runtime_resume_and_get(xe->drm.dev);
-
- return false;
+ return !pm_runtime_resume_and_get(xe->drm.dev);
}
int xe_pm_runtime_get_if_active(struct xe_device *xe)
diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
index 6a885585f653..1b4c15b5e71a 100644
--- a/drivers/gpu/drm/xe/xe_pm.h
+++ b/drivers/gpu/drm/xe/xe_pm.h
@@ -19,7 +19,7 @@ int xe_pm_runtime_suspend(struct xe_device *xe);
int xe_pm_runtime_resume(struct xe_device *xe);
int xe_pm_runtime_get(struct xe_device *xe);
int xe_pm_runtime_put(struct xe_device *xe);
-bool xe_pm_runtime_resume_if_suspended(struct xe_device *xe);
+bool xe_pm_runtime_resume_and_get(struct xe_device *xe);
int xe_pm_runtime_get_if_active(struct xe_device *xe);
#endif
--
2.40.0
More information about the Intel-xe
mailing list