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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 07/09/2022 16:03, Dixit, Ashutosh wrote:
> 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.

Yeah it would be conceptually nice to keep that query light weight. 
Doing too much work to query accumulated state, like in case of pinning 
the unpinned contexts, feels a bit over the top. Hopefully there aren't 
any nasty races which would make this hard.

Regards,

Tvrtko