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

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

On 03-10-2024 21:40, Ghimiray, Himal Prasad wrote:
> 
> 
> 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;

How about we simply return ref_incr at this point? Then, in patch 2, 
given that we have a helper function available, we can validate ref_incr 
against fw->initialized_domains, particularly for XE_FORCEWAKE_ALL, 
within that helper function.

Regards,
Badal

>>>   }
>>>   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)
>>>   };
>>>   /**
>>
>