[Intel-gfx] [RFC PATCH 2/2] Fix per client busyness locking

Ashutosh Dixit
2022-08-31T19:34:06Z

1. Do all ce->stats updates and reads under guc->timestamp.lock
2. Pin context image before reading
3. Merge __guc_context_update_clks and guc_context_update_stats into a
single function
4. Call lrc_update_runtime() unconditionally in guc_context_update_stats
5. Seems no need to update ce->stats.active with this approach

Some of the above steps may not be correct or complete but the idea is to
discuss/improve the original patch.

Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
drivers/gpu/drm/i915/gt/intel_context.c | 2 +-
drivers/gpu/drm/i915/gt/intel_context_types.h | 2 +-
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 41 ++++++++++---------
3 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
index e2d70a9fdac0..c004f676de27 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -581,7 +581,7 @@ u64 intel_context_get_total_runtime_ns(struct intel_context *ce)
u64 total, active;

if (ce->ops->update_stats)
- ce->ops->update_stats(ce);
+ return ce->ops->update_stats(ce);

total = ce->stats.runtime.total;
if (ce->ops->flags & COPS_RUNTIME_CYCLES)
diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
index f7ff4c7d81c7..699435bfff99 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -59,7 +59,7 @@ struct intel_context_ops {

void (*sched_disable)(struct intel_context *ce);

- void (*update_stats)(struct intel_context *ce);
+ u64 (*update_stats)(struct intel_context *ce);

void (*reset)(struct intel_context *ce);
void (*destroy)(struct kref *kref);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index bee8cf10f749..40d0edaf2e0a 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -1376,7 +1376,6 @@ static void __update_guc_busyness_stats(struct intel_guc *guc)
spin_unlock_irqrestore(&guc->timestamp.lock, flags);
}

-static void __guc_context_update_clks(struct intel_context *ce);
static void guc_timestamp_ping(struct work_struct *wrk)
{
struct intel_guc *guc = container_of(wrk, typeof(*guc),
@@ -1401,7 +1400,8 @@ static void guc_timestamp_ping(struct work_struct *wrk)

/* adjust context stats for overflow */
xa_for_each(&guc->context_lookup, index, ce)
- __guc_context_update_clks(ce);
+ if (ce->ops->update_stats)
+ ce->ops->update_stats(ce);

intel_gt_reset_unlock(gt, srcu);

@@ -1476,17 +1476,21 @@ void intel_guc_busyness_unpark(struct intel_gt *gt)
guc->timestamp.ping_delay);
}

-static void __guc_context_update_clks(struct intel_context *ce)
+static u64 guc_context_update_stats(struct intel_context *ce)
{
struct intel_guc *guc = ce_to_guc(ce);
struct intel_gt *gt = ce->engine->gt;
u32 *pphwsp, last_switch, engine_id;
- u64 start_gt_clk, active;
unsigned long flags;
+ u64 total, active = 0;
ktime_t unused;

+ intel_context_pin(ce);
spin_lock_irqsave(&guc->timestamp.lock, flags);

+ lrc_update_runtime(ce);
+ total = ce->stats.runtime.total;
+
/*
* GPU updates ce->lrc_reg_state[CTX_TIMESTAMP] when context is switched
* out, however GuC updates PPHWSP offsets below. Hence KMD (CPU)
@@ -1502,27 +1506,26 @@ static void __guc_context_update_clks(struct intel_context *ce)
guc_update_pm_timestamp(guc, &unused);

if (engine_id != 0xffffffff && last_switch) {
- start_gt_clk = READ_ONCE(ce->stats.runtime.start_gt_clk);
- __extend_last_switch(guc, &start_gt_clk, last_switch);
- active = intel_gt_clock_interval_to_ns(gt, guc->timestamp.gt_stamp - start_gt_clk);
- WRITE_ONCE(ce->stats.runtime.start_gt_clk, start_gt_clk);
- WRITE_ONCE(ce->stats.active, active);
- } else {
- lrc_update_runtime(ce);
+ __extend_last_switch(guc, &ce->stats.runtime.start_gt_clk, last_switch);
+ active = intel_gt_clock_interval_to_ns(gt,
+ guc->timestamp.gt_stamp - ce->stats.runtime.start_gt_clk);
}

spin_unlock_irqrestore(&guc->timestamp.lock, flags);
+ intel_context_unpin(ce);
+
+ return total + active;
}

-static void guc_context_update_stats(struct intel_context *ce)
+void lrc_update_runtime_locked(struct intel_context *ce)
{
- if (!intel_context_pin_if_active(ce)) {
- WRITE_ONCE(ce->stats.runtime.start_gt_clk, 0);
- WRITE_ONCE(ce->stats.active, 0);
- return;
- }
+ struct intel_guc *guc = ce_to_guc(ce);
+ unsigned long flags;

- __guc_context_update_clks(ce);
+ intel_context_pin(ce);
+ spin_lock_irqsave(&guc->timestamp.lock, flags);
+ lrc_update_runtime(ce);
+ spin_unlock_irqrestore(&guc->timestamp.lock, flags);
intel_context_unpin(ce);
}

@@ -2780,7 +2783,7 @@ static void guc_context_unpin(struct intel_context *ce)
{
struct intel_guc *guc = ce_to_guc(ce);

- lrc_update_runtime(ce);
+ lrc_update_runtime_locked(ce);
unpin_guc_id(guc, ce);
lrc_unpin(ce);

--
2.34.1