Re: [PATCH v6 03/25] drm/xe: Error handling in xe_force_wake_get()

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

On 03-10-2024 17:53, Nilawar, Badal wrote:
> 
> 
> On 30-09-2024 11:01, Himal Prasad Ghimiray wrote:
>> If an acknowledgment timeout occurs for a forcewake 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 that got refcounted,
>> and these domains need to be provided for subsequent xe_force_wake_put
>> call.
>>
>> 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)
>>
>> v6
>> - Change XE_FORCEWAKE_ALL to single bit, this helps accommodate
>> actually refcounted domains in return. (Michal)
>> - Modify commit message and warn message (Badal)
>> - Remove unnecessary information in kernel-doc (Michal)
>>
>> 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>
>> Reviewed-by: Badal Nilawar <badal.nilawar@intel.com>
>> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_force_wake.c       | 45 ++++++++++++++++++------
>>   drivers/gpu/drm/xe/xe_force_wake.h       |  4 +--
>>   drivers/gpu/drm/xe/xe_force_wake_types.h |  2 +-
>>   3 files changed, 38 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/ 
>> xe_force_wake.c
>> index 5ce9e912818a..7f358e42c5d4 100644
>> --- a/drivers/gpu/drm/xe/xe_force_wake.c
>> +++ b/drivers/gpu/drm/xe/xe_force_wake.c
>> @@ -160,29 +160,54 @@ 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.If requested domain is ALL then only
>> + * applicable/initialized domains will be considered for refcount and 
>> it is
>> + * a caller responsibilty to check returned ref if it includes any 
>> specific
>> + * domain by using xe_force_wake_ref_has_domain() function. caller 
>> must call
>> + * xe_force_wake_put() function to decrease incremented refcounts.
>> + *
>> + * Return: opaque reference to woken domains or zero if none of 
>> requested
>> + * domains were awake.
>> + */
>> +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 ref_incr = 0, awake_rqst = 0, awake_failed = 0;
>> +    unsigned int tmp, ref_rqst;
>>       unsigned long flags;
>> -    int ret = 0;
> 
> As we have fw->initialized_domains lets add check if valid domain is 
> being passed and assert otherwise.

Sounds good check to have. Will add it.


> 
> Regards,
> Badal
> 
>> +    ref_rqst = (domains == XE_FORCEWAKE_ALL) ? fw- 
>> >initialized_domains : domains;
>>       spin_lock_irqsave(&fw->lock, flags);
>> -    for_each_fw_domain_masked(domain, domains, fw, tmp) {
>> +    for_each_fw_domain_masked(domain, ref_rqst, fw, tmp) {
>>           if (!domain->ref++) {
>> -            woken |= BIT(domain->id);
>> +            awake_rqst |= BIT(domain->id);
>>               domain_wake(gt, domain);
>>           }
>> +        ref_incr |= BIT(domain->id);
>>       }
>> -    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;
>> +    ref_incr &= ~awake_failed;
>>       spin_unlock_irqrestore(&fw->lock, flags);
>> -    return ret;
>> +    xe_gt_WARN(gt, awake_failed, "Forcewake domain%s %#x failed to 
>> acknowledge awake request\n",
>> +           str_plural(hweight_long(awake_failed)), awake_failed);
>> +
>> +    return (ref_incr == fw->initialized_domains) ? ref_incr | 
>> XE_FORCEWAKE_ALL : ref_incr;
>>   }
>>   int xe_force_wake_put(struct xe_force_wake *fw,
>> diff --git a/drivers/gpu/drm/xe/xe_force_wake.h b/drivers/gpu/drm/xe/ 
>> xe_force_wake.h
>> index de720881a300..eb638128952d 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);
>>   int xe_force_wake_put(struct xe_force_wake *fw,
>>                 enum xe_force_wake_domains domains);
>> diff --git a/drivers/gpu/drm/xe/xe_force_wake_types.h b/drivers/gpu/ 
>> drm/xe/xe_force_wake_types.h
>> index fde17dc3d01e..899fbbcb3ea9 100644
>> --- a/drivers/gpu/drm/xe/xe_force_wake_types.h
>> +++ b/drivers/gpu/drm/xe/xe_force_wake_types.h
>> @@ -48,7 +48,7 @@ enum xe_force_wake_domains {
>>       XE_FW_MEDIA_VEBOX2    = BIT(XE_FW_DOMAIN_ID_MEDIA_VEBOX2),
>>       XE_FW_MEDIA_VEBOX3    = BIT(XE_FW_DOMAIN_ID_MEDIA_VEBOX3),
>>       XE_FW_GSC        = BIT(XE_FW_DOMAIN_ID_GSC),
>> -    XE_FORCEWAKE_ALL    = BIT(XE_FW_DOMAIN_ID_COUNT) - 1
>> +    XE_FORCEWAKE_ALL    = BIT(XE_FW_DOMAIN_ID_COUNT)
>>   };
>>   /**
>