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

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

On 24-09-2024 22:57, Nilawar, Badal wrote:
> Hi Himal,
> 
> On 24-09-2024 17:46, Himal Prasad Ghimiray wrote:
>> If an acknowledgment timeout occurs for a domain awake request, do not
> 
> s/acknowledgement/forcewake acknowledgement/

Ok.

> 
>> 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()
>>
>> v3
>> - Use explicit type for mask (Michal/Badal)
>> - Improve kernel-doc (Michal)
>> - Use unsigned int instead of abusing enum (Michal)
>>
>> v5
>> - Use unsigned int for return (MattB/Badal/Rodrigo)
>> - use xe_gt_WARN for domain awake ack failure (Badal/Rodrigo)
>>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> 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 | 37 +++++++++++++++++++++++-------
>>   drivers/gpu/drm/xe/xe_force_wake.h |  4 ++--
>>   2 files changed, 31 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/ 
>> xe_force_wake.c
>> index a64c14757c84..d190aa93be90 100644
>> --- a/drivers/gpu/drm/xe/xe_force_wake.c
>> +++ b/drivers/gpu/drm/xe/xe_force_wake.c
>> @@ -150,28 +150,49 @@ static int domain_sleep_wait(struct xe_gt *gt,
>>                        (ffs(tmp__) - 1))) && \
>>                        domain__->reg_ctl.addr)
>> -int xe_force_wake_get(struct xe_force_wake *fw,
>> -              enum xe_force_wake_domains domains)
>> +/**
>> + * xe_force_wake_get() : Increase the domain refcount
>> + * @fw: struct xe_force_wake
>> + * @domains: forcewake domains to get refcount on
>> + *
>> + * This function takes references for the input @domains and wakes 
>> them if
>> + * they are asleep.
>> + *
>> + * Return: mask of refcount increased domains. 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.
>> + */
> 
> Here especially for FORCEWAKE_ALL case caller has to strictly compare 
> return mask with FORCEWAKE_ALL instead of just if(!ret) and return. 
> Which is fine.

That is true, if(!ret) works for all individual domains but not for 
FORCEWAKE_ALL, whereas if(ret != domain) works for any domain(including 
FORCEWAKE_ALL).

In subsequent patches I have used (!ret) for individual domains.

> 
>> +unsigned 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;
>> +    unsigned int tmp, ret, awake_rqst = 0, awake_failed = 0;
>>       unsigned long flags;
>> -    int ret = 0;
>>       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) {
>> +            fw->awake_domains |= BIT(domain->id);
>> +        } else {
>> +            awake_failed |= BIT(domain->id);
>> +            --domain->ref;
>> +        }
>>       }
>> -    fw->awake_domains |= woken;
>> +    ret = (domains & ~awake_failed);
>>       spin_unlock_irqrestore(&fw->lock, flags);
>> +    xe_gt_WARN(gt, awake_failed, "domain%s %#x failed to 
>> acknowledgment awake\n",
> 
> Forcewake domain%s failed to acknowledge awake request.

ok.

> 
>> +           str_plural(hweight_long(awake_failed)), awake_failed);
>> +
>>       return ret;
>>   }
>> diff --git a/drivers/gpu/drm/xe/xe_force_wake.h b/drivers/gpu/drm/xe/ 
>> xe_force_wake.h
>> index a2577672f4e3..6c1ade39139b 100644
>> --- a/drivers/gpu/drm/xe/xe_force_wake.h
>> +++ b/drivers/gpu/drm/xe/xe_force_wake.h
>> @@ -15,8 +15,8 @@ void xe_force_wake_init_gt(struct xe_gt *gt,
>>                  struct xe_force_wake *fw);
>>   void xe_force_wake_init_engines(struct xe_gt *gt,
>>                   struct xe_force_wake *fw);
>> -int xe_force_wake_get(struct xe_force_wake *fw,
>> -              enum xe_force_wake_domains domains);
>> +unsigned int xe_force_wake_get(struct xe_force_wake *fw,
>> +                   enum xe_force_wake_domains domains);
> 
> Looks good to me.
> Reviewed-by: Badal Nilawar <badal.nilawar@intel.com>

Thanks for the review.

> 
> Regards,
> Badal
> 
>>   int xe_force_wake_put(struct xe_force_wake *fw,
>>                 enum xe_force_wake_domains domains);
>