[PATCH v10 2/4] drm/xe: Add locks in gtidle code

[Lucas De Marchi]

On Thu, Dec 19, 2024 at 05:19:08PM -0800, Vinay Belgaumkar wrote:
>Another entry point is added so PMU code can call into the gtidle
>method for residency. Add locking so that parallel calls to this
>method from sysfs and PMU don't give inconsistent gtidle states
>for c6 residency.

The update of the residency is racy regardless of having another entry
point. If you said "another entrypoint is **going** to be added, making
the problem worse", then it'd be ok. I'd reword it into something like:

The update of the residency values need to be protected by a lock to
avoid multiple entrypoints, for example when multiple userspace clients
read the sysfs file. Other in-kernel clients are going to be added to
sample these values, making the problem worse.

>Suggested-by: Lucas De Marchi <lucas.demarchi at intel.com>
>Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
>---
> drivers/gpu/drm/xe/xe_gt_idle.c       | 23 ++++++++++++++++++++---
> drivers/gpu/drm/xe/xe_gt_idle.h       |  1 +
> drivers/gpu/drm/xe/xe_gt_idle_types.h |  3 +++
> 3 files changed, 24 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_gt_idle.c b/drivers/gpu/drm/xe/xe_gt_idle.c
>index fd80afeef56a..1f32ac964abe 100644
>--- a/drivers/gpu/drm/xe/xe_gt_idle.c
>+++ b/drivers/gpu/drm/xe/xe_gt_idle.c
>@@ -69,6 +69,8 @@ static u64 get_residency_ms(struct xe_gt_idle *gtidle, u64 cur_residency)
> {
> 	u64 delta, overflow_residency, prev_residency;
>
>+	lockdep_assert_held(&gtidle->lock);
>+
> 	overflow_residency = BIT_ULL(32);

it seems this overflow logic is a a little bit over-engineered.
If the counter overflows on u32, we could had just used u32 for the
math and get the wrap around for free.


with the updated commit message:

	Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>

Lucas De Marchi

>
> 	/*
>@@ -273,8 +275,21 @@ static ssize_t idle_status_show(struct device *dev,
>
> 	return sysfs_emit(buff, "%s\n", gt_idle_state_to_string(state));
> }
>-static DEVICE_ATTR_RO(idle_status);
>
>+u64 xe_gt_idle_residency_msec(struct xe_gt_idle *gtidle)
>+{
>+	struct xe_guc_pc *pc = gtidle_to_pc(gtidle);
>+	u64 residency;
>+	unsigned long flags;
>+
>+	raw_spin_lock_irqsave(&gtidle->lock, flags);
>+	residency = get_residency_ms(gtidle, gtidle->idle_residency(pc));
>+	raw_spin_unlock_irqrestore(&gtidle->lock, flags);
>+
>+	return residency;
>+}
>+
>+static DEVICE_ATTR_RO(idle_status);
> static ssize_t idle_residency_ms_show(struct device *dev,
> 				      struct device_attribute *attr, char *buff)
> {
>@@ -283,10 +298,10 @@ static ssize_t idle_residency_ms_show(struct device *dev,
> 	u64 residency;
>
> 	xe_pm_runtime_get(pc_to_xe(pc));
>-	residency = gtidle->idle_residency(pc);
>+	residency = xe_gt_idle_residency_msec(gtidle);
> 	xe_pm_runtime_put(pc_to_xe(pc));
>
>-	return sysfs_emit(buff, "%llu\n", get_residency_ms(gtidle, residency));
>+	return sysfs_emit(buff, "%llu\n", residency);
> }
> static DEVICE_ATTR_RO(idle_residency_ms);
>
>@@ -329,6 +344,8 @@ int xe_gt_idle_init(struct xe_gt_idle *gtidle)
> 	if (!kobj)
> 		return -ENOMEM;
>
>+	raw_spin_lock_init(&gtidle->lock);
>+
> 	if (xe_gt_is_media_type(gt)) {
> 		snprintf(gtidle->name, sizeof(gtidle->name), "gt%d-mc", gt->info.id);
> 		gtidle->idle_residency = xe_guc_pc_mc6_residency;
>diff --git a/drivers/gpu/drm/xe/xe_gt_idle.h b/drivers/gpu/drm/xe/xe_gt_idle.h
>index 4455a6501cb0..591a01e181bc 100644
>--- a/drivers/gpu/drm/xe/xe_gt_idle.h
>+++ b/drivers/gpu/drm/xe/xe_gt_idle.h
>@@ -17,5 +17,6 @@ void xe_gt_idle_disable_c6(struct xe_gt *gt);
> void xe_gt_idle_enable_pg(struct xe_gt *gt);
> void xe_gt_idle_disable_pg(struct xe_gt *gt);
> int xe_gt_idle_pg_print(struct xe_gt *gt, struct drm_printer *p);
>+u64 xe_gt_idle_residency_msec(struct xe_gt_idle *gtidle);
>
> #endif /* _XE_GT_IDLE_H_ */
>diff --git a/drivers/gpu/drm/xe/xe_gt_idle_types.h b/drivers/gpu/drm/xe/xe_gt_idle_types.h
>index b8b297a3f884..a3667c567f8a 100644
>--- a/drivers/gpu/drm/xe/xe_gt_idle_types.h
>+++ b/drivers/gpu/drm/xe/xe_gt_idle_types.h
>@@ -6,6 +6,7 @@
> #ifndef _XE_GT_IDLE_SYSFS_TYPES_H_
> #define _XE_GT_IDLE_SYSFS_TYPES_H_
>
>+#include <linux/spinlock.h>
> #include <linux/types.h>
>
> struct xe_guc_pc;
>@@ -31,6 +32,8 @@ struct xe_gt_idle {
> 	u64 cur_residency;
> 	/** @prev_residency: previous residency counter */
> 	u64 prev_residency;
>+	/** @lock: Lock protecting idle residency counters */
>+	raw_spinlock_t lock;
> 	/** @idle_status: get the current idle state */
> 	enum xe_gt_idle_state (*idle_status)(struct xe_guc_pc *pc);
> 	/** @idle_residency: get idle residency counter */
>-- 
>2.38.1
>