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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Wed, 07 Sep 2022 00:28:48 -0700, Tvrtko Ursulin wrote:
>
> On 06/09/2022 19:29, Umesh Nerlige Ramappa wrote:
> > On Thu, Sep 01, 2022 at 04:55:22PM -0700, Dixit, Ashutosh wrote:
> >> On Wed, 31 Aug 2022 15:45:49 -0700, Umesh Nerlige Ramappa wrote:
>
> [snip]
>
> >>> >
> >>> >    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.
> >>
> >> Do we know which intel_context_get_total_runtime_ns() call is not allowed
> >> to sleep? E.g. the code path from i915_drm_client_fdinfo() is allowed to
> >> sleep. As mentioned I have done this is v2 of RFC patch which I think is
> >> sufficient, but a more complicated scheme (which I think we can avoid for
> >> now) would be to pin in code paths when sleeping is allowed.
> >>
> >
> > Hmm, maybe intel_context_get_total_runtime_ns can sleep, not sure why I
> > was assuming that this falls in the perf_pmu path. This is now in the
> > drm_fdinfo query path. + Tvrtko.
> >
> > @Tvrtko, any idea if intel_context_get_total_runtime_ns path can sleep?
>
> Not at the moment - it calls it from a lockless (rcu) section when walking
> all the contexts belonging to a client. Idea being performance queries
> should have minimum effect on a running system.

Hmm my bad, missing the rcu and assuming a userspace thread will be able to
sleep.

> I think it would be possible to change in theory but not sure how much
> work. There is a hard need to sleep in there or what?

GuC contexts need to be pinned/unpinned which can sleep but investigating
if we can return a previously computed busyness when we cannot pin/sleep.

Thanks.
--
Ashutosh