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

[Lucas De Marchi]

On Fri, Dec 20, 2024 at 01:15:07PM -0800, Belgaumkar, Vinay wrote:
>
>On 12/20/2024 1:09 PM, Matthew Brost wrote:
>>On Fri, Dec 20, 2024 at 12:59:25PM -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.
>>>
>>>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);
>>>  	/*
>>>@@ -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);
>>Why raw spin lock rather than just spin lock?
>
>Lucas suggested this as  the perf subsystem uses raw_spinlocks, so we 
>cannot use spinlocks when the perf calls into Xe.

correct. Any entrypoint via perf may be via a hardirq and always use a
raw_spinlock. All the other PMUs I could find (except i915, that is
wrong) use raw_spinlock

More info:
https://lore.kernel.org/intel-xe/20241212093123.GV21636@noisy.programming.kicks-ass.net/

Alternative is to have a worker where the actual read happens and make
the perf entrypoint to read that value from memory. But then that value
will need to be protected via a raw_spinlock anyway.

Lucas De Marchi