Re: [Intel-gfx] [PATCH] i915/pmu: Wire GuC backend to per-client busyness

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

On Wed, Aug 31, 2022 at 01:25:11PM -0700, Dixit, Ashutosh wrote:
>On Fri, 26 Aug 2022 09:33:08 -0700, Umesh Nerlige Ramappa wrote:
>>
>
>Hi Umesh,
>
>Just to communicate my thoughts I have posted this patch on top of your
>patch:
>
>[1] https://patchwork.freedesktop.org/series/107983/
>
>Could you please take a look at that and see if it makes sense.
>
>> On Thu, Aug 25, 2022 at 06:44:50PM -0700, Dixit, Ashutosh wrote:
>> > On Thu, 04 Aug 2022 16:21:25 -0700, Umesh Nerlige Ramappa wrote:
>> >
>> > Hi Umesh, I am fairly new to this code so some questions will be below will
>> > be newbie questions, thanks for bearing with me.
>> >
>> >> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
>> >> index 654a092ed3d6..e2d70a9fdac0 100644
>> >> --- a/drivers/gpu/drm/i915/gt/intel_context.c
>> >> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
>> >> @@ -576,16 +576,24 @@ void intel_context_bind_parent_child(struct intel_context *parent,
>> >>	child->parallel.parent = parent;
>> >>  }
>> >>
>> >> -u64 intel_context_get_total_runtime_ns(const struct intel_context *ce)
>> >> +u64 intel_context_get_total_runtime_ns(struct intel_context *ce)
>> >>  {
>> >>	u64 total, active;
>> >>
>> >> +	if (ce->ops->update_stats)
>> >> +		ce->ops->update_stats(ce);
>> >> +
>> >>	total = ce->stats.runtime.total;
>> >>	if (ce->ops->flags & COPS_RUNTIME_CYCLES)
>> >>		total *= ce->engine->gt->clock_period_ns;
>> >>
>> >>	active = READ_ONCE(ce->stats.active);
>> >> -	if (active)
>> >> +	/*
>> >> +	 * When COPS_RUNTIME_ACTIVE_TOTAL is set for ce->cops, the backend
>> >> +	 * already provides the total active time of the context, so skip this
>> >> +	 * calculation when this flag is set.
>> >> +	 */
>> >> +	if (active && !(ce->ops->flags & COPS_RUNTIME_ACTIVE_TOTAL))
>> >>		active = intel_context_clock() - active;
>> >>
>> >>	return total + active;
>> >
>> > /snip/
>> >
>> >> @@ -1396,6 +1399,10 @@ static void guc_timestamp_ping(struct work_struct *wrk)
>> >>	with_intel_runtime_pm(&gt->i915->runtime_pm, wakeref)
>> >>		__update_guc_busyness_stats(guc);
>> >>
>> >> +	/* adjust context stats for overflow */
>> >> +	xa_for_each(&guc->context_lookup, index, ce)
>> >> +		__guc_context_update_clks(ce);
>> >
>> > What is the reason for calling __guc_context_update_clks() periodically
>> > from guc_timestamp_ping() since it appears we should just be able to call
>> > __guc_context_update_clks() from intel_context_get_total_runtime_ns() to
>> > update 'active'? Is the reason for calling __guc_context_update_clks()
>> > periodically that the calculations in __guc_context_update_clks() become
>> > invalid if the counters overflow?
>>
>> Correct, these are 32-bit counters and the worker just tracks overflow.
>
>OK.
>
>>
>> >
>> >> +
>> >>	intel_gt_reset_unlock(gt, srcu);
>> >>
>> >>	mod_delayed_work(system_highpri_wq, &guc->timestamp.work,
>> >> @@ -1469,6 +1476,56 @@ void intel_guc_busyness_unpark(struct intel_gt *gt)
>> >>			 guc->timestamp.ping_delay);
>> >>  }
>> >>
>> >> +static void __guc_context_update_clks(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;
>> >> +	ktime_t unused;
>> >> +
>> >> +	spin_lock_irqsave(&guc->timestamp.lock, flags);
>> >> +
>> >> +	/*
>> >> +	 * GPU updates ce->lrc_reg_state[CTX_TIMESTAMP] when context is switched
>> >> +	 * out, however GuC updates PPHWSP offsets below. Hence KMD (CPU)
>> >> +	 * relies on GuC and GPU for busyness calculations. Due to this, A
>> >> +	 * potential race was highlighted in an earlier review that can lead to
>> >> +	 * double accounting of busyness. While the solution to this is a wip,
>> >> +	 * busyness is still usable for platforms running GuC submission.
>> >> +	 */
>> >> +	pphwsp = ((void *)ce->lrc_reg_state) - LRC_STATE_OFFSET;
>> >> +	last_switch = READ_ONCE(pphwsp[PPHWSP_GUC_CONTEXT_USAGE_STAMP_LO]);
>> >> +	engine_id = READ_ONCE(pphwsp[PPHWSP_GUC_CONTEXT_USAGE_ENGINE_ID]);
>> >> +
>> >> +	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);
>> >
>> > Should not need WRITE_ONCE to update regular memory. Not even sure we need
>> > READ_ONCE above.
>>
>> Not sure I checked what they do. I was thinking these are needed for the
>> memory ordering (as in be sure that start_gt_clk is updated before
>> active).
>
>As long as our operations are done under correct locks we don't have to
>worry about memory ordering. That is one of the reasons I am doing
>everything under the spinlock in [1].
>
>>
>> >
>> >> +	} else {
>> >> +		lrc_update_runtime(ce);
>> >
>> > As was being discussed, should not need this here in this function. See
>> > below too.
>>
>> In short, I added this here so that a query for busyness following idle can
>> be obtained immediately. For GuC backend, the context is unpinned after
>> disabling scheduling on that context and that is asynchronous.  Also if
>> there are more requests on that context, the scheduling may not be disabled
>> and unpin may not happen, so updated runtime would only be seen much much
>> later.
>>
>> It is still safe to call from here because we know that the context is not
>> active and has switched out. If it did switch in while we were reading
>> this, that's still fine, we would only report the value stored in the
>> context image.
>
>Agreed, but in [1] I have made this unconditional, not sure if you will
>agree or see problems with that.

That would get called every second (default intel_gpu_top query 
internal) for a long running workload. multiply that with all active 
contexts.
>
>>
>> >
>> >> +	}
>> >> +
>> >> +	spin_unlock_irqrestore(&guc->timestamp.lock, flags);
>> >> +}
>> >> +
>> >> +static void guc_context_update_stats(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);
>> >
>> > Why do these need to be initialized to 0? Looks like the calculations in
>> > __guc_context_update_clks() will work even if we don't do this? Also I
>> > didn't follow the 'if (!intel_context_pin_if_active(ce))' check.
>>
>> __guc_context_update_clks accesses the context image, so we need to make
>> sure it's pinned. pin if active will not sleep/wait, so we can use it in
>> this path.
>
>I have added pinning in [1].
>
>> if context is not active, then we update the active stats to 0.
>
>In [1] active is just a local variable and I don't touch ce->stats.active
>at all.
>
>> >> +		return;
>> >> +	}
>> >> +
>> >> +	__guc_context_update_clks(ce);
>> >> +	intel_context_unpin(ce);
>> >> +}
>> >> +
>> >>  static inline bool
>> >>  submission_disabled(struct intel_guc *guc)
>> >>  {
>> >> @@ -2723,6 +2780,7 @@ static void guc_context_unpin(struct intel_context *ce)
>> >>  {
>> >>	struct intel_guc *guc = cce_to_guc(ce);
>> >>
>> >> +	lrc_update_runtime(ce);
>> >
>> > How about moving this into lrc_unpin() since that gets called from all guc
>> > context types (parent/child/virtual).
>>
>> looks like lrc_unpin is called from context_unpin path.
>>
>> Same as above: for GuC, the context_unpin is an async operation and may not
>> happen if there are multiple requests in queue.
>
>In [1] I have left lrc_unpin in guc_context_unpin but changed to
>lrc_update_runtime_locked.

 From your rfc patch, I like
- the idea of not touching ce->stats.active
- having the update_stats return u64
- not doing a rmw for start_gt_clk

With those changes, we are only accessing total in ce->stats, so we 
don't really need a lrc_update_runtime_locked.

Thanks,
Umesh


>
>Thanks.
>--
>Ashutosh