Re: [PATCH v2 01/23] drm/xe: Error handling in xe_force_wake_get()

["Nilawar, Badal" <badal.nilawar@intel.com>]

On 13-09-2024 18:47, Ghimiray, Himal Prasad wrote:
> 
> 
> On 13-09-2024 16:56, Michal Wajdeczko wrote:
>>
>>
>> On 13.09.2024 05:59, Ghimiray, Himal Prasad wrote:
>>>
>>>
>>> On 13-09-2024 03:01, Michal Wajdeczko wrote:
>>>>
>>>>
>>>> On 12.09.2024 21:15, Himal Prasad Ghimiray wrote:
>>>>> If an acknowledgment timeout occurs for a domain awake request, do not
>>>>> increment the reference count for the domain. This ensures that
>>>>> subsequent _get calls do not incorrectly assume the domain is 
>>>>> awake. The
>>>>> return value is a mask of domains whose reference counts were
>>>>> incremented, and these domains need to be released using
>>>>> xe_force_wake_put.
>>>>>
>>>>> The caller needs to compare the return value with the input domains to
>>>>> determine the success or failure of the operation and decide 
>>>>> whether to
>>>>> continue or return accordingly.
>>>>>
>>>>> While at it, add simple kernel-doc for xe_force_wake_get()
>>>>>
>>>>> Cc: Badal Nilawar <badal.nilawar@intel.com>
>>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>>>> Cc: Nirmoy Das <nirmoy.das@intel.com>
>>>>> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/xe/xe_force_wake.c | 35 ++++++++++++++++++++++++ 
>>>>> +-----
>>>>>    1 file changed, 29 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/xe/xe_force_wake.c
>>>>> b/drivers/gpu/drm/xe/xe_force_wake.c
>>>>> index a64c14757c84..fa42d652d23f 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_force_wake.c
>>>>> +++ b/drivers/gpu/drm/xe/xe_force_wake.c
>>>>> @@ -150,26 +150,49 @@ static int domain_sleep_wait(struct xe_gt *gt,
>>>>>                         (ffs(tmp__) - 1))) && \
>>>>>                         domain__->reg_ctl.addr)
>>>>>    +/**
>>>>> + * xe_force_wake_get : Increase the domain refcount; if it was 0
>>>>> initially, wake the domain
>>>>
>>>> while likely this is still recognized by the kernel-doc tool, this is
>>>> not correct notation for the function() documentation
>>>
>>>
>>> I assume you are suggesting %s/xe_force_wake_get/xe_force_wake_get()
>>> will fix it.
>>>
>>>
>>>>
>>>> [1]
>>>> https://docs.kernel.org/doc-guide/kernel-doc.html#function- 
>>>> documentation
>>>>
>>>>> + * @fw: struct xe_force_wake
>>>>> + * @domains: forcewake domains to get refcount on
>>>>> + *
>>>>> + * Increment refcount for the force-wake domain. If the domain is
>>>>> + * asleep, awaken it and wait for acknowledgment within the specified
>>>>> + * timeout. If a timeout occurs, decrement the refcount.
>>>>
>>>> not sure if doc shall be 1:1 of low level implementation details
>>>
>>> Does this sound okay ?
>>> This function takes references for the input @domains and wakes them if
>>> they are asleep.
>>>
>>>>
>>>>> + * The caller should compare the return value with the @domains to
>>>>> + * determine the success or failure of the operation.
>>>>> + *
>>>>> + * Return: mask of refcount increased domains.
>>>>
>>>> if we return a 'mask' then maybe it should be of 'unsigned int' type?
>>>
>>> Agreed. Will fix in next version.
>>>
>>>>
>>>>> If the return value is
>>>>> + * equal to the input parameter @domains, the operation is considered
>>>>> + * successful. Otherwise, the operation is considered a failure, and
>>>>> + * the caller should handle the failure case, potentially returning
>>>>> + * -ETIMEDOUT.
>>>>
>>>> it looks that all problems with the nice API is due to the
>>>> XE_FORCEWAKE_ALL that is not a single domain ID and requires extra care
>>>>
>>>> maybe there should be different pair of functions:
>>>
>>> I am not convinced with different pair of functions:
>>>
>>> In current implementation:
>>>
>>> int mask = xe_force_wake_get(fw, domains)
>>> if (mask != domains) {
>>>      Non critical path continue with warning;
>>>       or
>>>      critical path:
>>>          xe_force_wake_put(fw, mask);
>>>          return -ETIMEDOUT;
>>> }
>>>
>>> do_ops;
>>> xe_force_wake_put(fw, mask);
>>> return err;
>>>
>>> Above flow remains intact irrespective of individual domains or
>>> FORCEWAKE_ALL.
>>>
>>> In case of individual domains if (mask != domains) can be replaced with
>>> (!mask) and user can avoid xe_force_wake_put(fw, mask) in failure path
>>> since mask is 0;
>>
>> so maybe we should have (by reinventing i915?):
>>
>> // opaque, but zero means failure/no domains are awake
>> typedef unsigned long xe_wakeref_t;
>>
>>
>> // caller should test for ref != 0
>> // but shall call put if ref != 0
>> xe_wakeref_t xe_force_wake_get(fw, enum xe_force_wake_domains d)
>>
>> // safe to call with ref == 0
>> void xe_force_wake_put(fw, xe_wakeref_t ref)
>>
>>
>> // helpers for critical work that must be sure about domain
>>
>> // compares opaque ref with explicit domain != ALL
>> // can be used by the code that obtained the ref
>> bool xe_wakeref_has_domain(xe_wakeref_t, enum xe_force_wake_domains d)
>>
>> // compares fw with explicit domain != ALL
>> // can be used by the code that does not have direct access to the ref
>> bool xe_force_wake_is_awake(fw, enum xe_force_wake_domains d)
>>
>>
>> // helpers for checking correctness
>> void xe_force_wake_assert_held(fw, enum xe_force_wake_domains d)
>>
>>
>> then usage would be:
>>
>> xe_wakeref_t ref;
>>
>> ref = xe_force_wake_get(fw, d);
>> if (ref) {
>>     // ...
>>     xe_force_wake_put(fw, ref);
>> }
>>
>> or:
>>
>> xe_wakeref_t ref;
>>
>> ref = xe_force_wake_get(fw, ALL);
>> if (xe_wakeref_has_domain(ref, d1))
>>     // ... critical work1
>> if (xe_wakeref_has_domain(ref, d2))
>>     // ... critical work2
>> xe_force_wake_put(fw, ref);
>>
>>
>> so above will be very similar to what you have but by having explicit
>> types IMO it will help connect all functions into proper use-case flow
> 
> 
> Agreed implementation/usage will be same, will use explicit type for 
> clarity.
> IMO typedef unsigned int xe_wakeref_t is sufficient instead of
> typedef unsigned long xe_wakeref_t;

I agree with this.

Regards,
Badal
> 
> 
>>
>>>
>>>
>>>>
>>>> // for single domain where ret=0 is success, ret<0 is error
>>>
>>> This leads to caller only calling xe_force_wake_put incase of get
>>> success. so in case of caller continuing with failure, he will need to
>>> ensure the put is not called.
>>>
>>> for example:
>>> int ret;
>>>
>>> ret = xe_force_wake_get(fw, DOMAIN_GT);
>>> XE_WARN_ON(ret)
>>> if(!ret)
>>>      xe_force_wake_put(fw, DOMAIN_GT);
>>>
>>>> int xe_force_wake_get(fw, enum xe_force_wake_domain_id id);
>>>> void xe_force_wake_put(fw, enum xe_force_wake_domain_id id);
>>>>
>>>> and
>>>>
>>>> // for all domain where ret=0 is success, ret<0 is error
>>>> int int xe_force_wake_get_all(fw);
>>>> void xe_force_wake_put_all(fw);
>>>
>>> In case of xe_force_wake_get_all(fw) failure, how the caller will know
>>> which domains got awake and which failed ?
>>>
>>> ret = xe_force_wake_get_all(fw);
>>> if(!ret)
>>>     No way to put awake domains to sleep
>>
>> in case of failure, it would be the responsibility of the
>> xe_force_wake_get_all() to put all partial awakes immediately, since it
>> failed to awake all requested domains (same as in single domain case)
>>
>> but let's drop this idea
>>
>>>
>>>>
>>>> and
>>>>
>>>> // input: mask of domains, return: mask of domain
>>>> unsigned int xe_force_wake_get_mask(fw, mask);
>>>> void xe_force_wake_put_mask(fw, mask);
>>>>
>>>> this last one can be just main implementation (static or public if we
>>>> really want to continue with random set of enabled domains)
>>>>
>>>>> + */
>>>>>    int xe_force_wake_get(struct xe_force_wake *fw,
>>>>>                  enum xe_force_wake_domains domains)
>>>>>    {
>>>>>        struct xe_gt *gt = fw->gt;
>>>>>        struct xe_force_wake_domain *domain;
>>>>> -    enum xe_force_wake_domains tmp, woken = 0;
>>>>> +    enum xe_force_wake_domains tmp, awake_rqst = 0, awake_ack = 0;
>>>>
>>>> it looks that you're abusing even more all enum variables by treating
>>>> them as plain integers
>>>
>>> Miss at my end. Will address them in next version.
>>>
>>>>
>>>>>        unsigned long flags;
>>>>> -    int ret = 0;
>>>>> +    int ret = domains;
>>>>>          spin_lock_irqsave(&fw->lock, flags);
>>>>>        for_each_fw_domain_masked(domain, domains, fw, tmp) {
>>>>>            if (!domain->ref++) {
>>>>> -            woken |= BIT(domain->id);
>>>>> +            awake_rqst |= BIT(domain->id);
>>>>>                domain_wake(gt, domain);
>>>>>            }
>>>>>        }
>>>>> -    for_each_fw_domain_masked(domain, woken, fw, tmp) {
>>>>> -        ret |= domain_wake_wait(gt, domain);
>>>>> +    for_each_fw_domain_masked(domain, awake_rqst, fw, tmp) {
>>>>> +        if (domain_wake_wait(gt, domain) == 0) {
>>>>> +            awake_ack |= BIT(domain->id);
>>>>> +        } else {
>>>>> +            ret &= ~BIT(domain->id);
>>>>> +            --domain->ref;
>>>>> +        }
>>>>>        }
>>>>> -    fw->awake_domains |= woken;
>>>>> +
>>>>> +    fw->awake_domains |= awake_ack;
>>>>>        spin_unlock_irqrestore(&fw->lock, flags);
>>>>>          return ret;