Re: [PATCH v6 02/25] drm/xe/forcewake: Add a helper xe_force_wake_ref_has_domain()

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

On 01-10-2024 01:34, Michal Wajdeczko wrote:
> 
> 
> On 30.09.2024 07:31, Himal Prasad Ghimiray wrote:
>> The helper xe_force_wake_ref_has_domain() checks if the input domain
>> has been successfully reference-counted and awakened in the reference
>> returned by the xe_force_wake_get() call.
> 
> hmm
> but at this point of the series, xe_force_wake_get() still returns errno

Yup, will remove  returned by the xe_force_wake_get() call.

> 
>>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Badal Nilawar <badal.nilawar@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_force_wake.h | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_force_wake.h b/drivers/gpu/drm/xe/xe_force_wake.h
>> index a2577672f4e3..de720881a300 100644
>> --- a/drivers/gpu/drm/xe/xe_force_wake.h
>> +++ b/drivers/gpu/drm/xe/xe_force_wake.h
>> @@ -46,4 +46,23 @@ xe_force_wake_assert_held(struct xe_force_wake *fw,
>>   	xe_gt_assert(fw->gt, fw->awake_domains & domain);
>>   }
>>   
>> +/**
>> + * xe_force_wake_ref_has_domain - verifies if the domains are in fw_ref
>> + * @fw_ref : Return of xe_force_wake_get()
> 
> for now you may only refer to this only as: "the force_wake reference"
> and once you update xe_force_wake_get() you may say there that it
> returns "a force_wake reference" that could be checked by this helper

Ok

> 
>> + * @domain : forcewake domains to verify
> 
> domain or domains ?
> 
>> + *
>> + * xe_force_wake_ref_has_domain() confirms whether xe_force_wake_get()
>> + * is success or not for @domain.
> 
> This function confirms whether the @fw_ref includes a reference to the
> specified @domain.

OK

> 
>> + * In the event of a failure, if the caller chooses to abort or return,
>> + * they must call xe_force_wake_put(fw, @fw_ref) if the @domain
>> + * parameter is XE_FORCE_WAKE_ALL
> 
> hmm, this last sentence is IMO not applicable here, it should be rather
> placed in xe_force_wake_get() doc that may refer to this function

Sure

> 
>> + *
>> + * Return: true on domain is refcounted.
> 
> s/on/if

ok

> 
>> + */
>> +static inline bool
>> +xe_force_wake_ref_has_domain(unsigned int fw_ref, enum xe_force_wake_domains domain)
>> +{
>> +	return (fw_ref & domain);
> 
> you don't need these ( )

ok

> 
>> +}
>> +
>>   #endif
>