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

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Wed, Aug 31, 2022 at 12:33:55PM -0700, Ashutosh Dixit wrote:
>1. Do all ce->stats updates and reads under guc->timestamp.lock

Other than the question about READ_ONCE/WRITE_ONCE, I am not sure I 
understand what's broken in the locking logic here. Can you please add 
some description? thanks

>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);

This is an improvement that we can add and eventually, we can make this 
a GuC specific vfunc. Doing so may also remove the 
COPS_RUNTIME_ACTIVE_TOTAL option that I added to GuC specific context 
ops.

>
> 	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);

context pinning logic needs to be separated for this (ping) path vs. the 
query path - intel_context_get_total_runtime_ns().
>
> 	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);

intel_context_pin can sleep and we are not allowed to sleep in this path 
- intel_context_get_total_runtime_ns(), however we can sleep in the ping 
   worker path, so ideally we want to separate it out for the 2 paths.

> 	spin_lock_irqsave(&guc->timestamp.lock, flags);
>
>+	lrc_update_runtime(ce);
>+	total = ce->stats.runtime.total;

For long running contexts and frequenct queries, calling this ^ when a 
context is active does not add value. I would just call it in the else 
like before.

>+
> 	/*
> 	 * 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);

Makes sense. Earlier it was a rmw, now it's just a write to 
ce->stats.runtime.start_gt_clk.

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

return ce->stats.runtime.total + active;

and then drop the local copy of total by bringing back the else{}.

> }
>
>-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);

 From where lrc_update_runtime_locked is being called, I would assume 
that the context is already pinned (until unpin_guc_id is called).

Thanks,
Umesh

> }
>
>@@ -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
>