Re: [PATCH] drm/xe: Fix xe_force_wake_assert_held for enum XE_FORCEWAKE_ALL

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

On 04-06-2024 02:33, Matt Roper wrote:
> On Thu, May 30, 2024 at 10:09:30PM +0530, Ghimiray, Himal Prasad wrote:
>>
>> On 30-05-2024 20:14, Nilawar, Badal wrote:
>>>
>>>
>>> On 30-05-2024 19:51, Nilawar, Badal wrote:
>>>>
>>>>
>>>> On 30-05-2024 19:55, Himal Prasad Ghimiray wrote:
>>>>> Make sure that the assertion condition covers the wakefulness of all
>>>>> domains for XE_FORCEWAKE_ALL.
>>>>>
>>>>> Fixes: c73acc1eeba5 ("drm/xe: Use Xe assert macros instead of
>>>>> XE_WARN_ON macro")
>>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>>> Cc: Badal Nilawar <badal.nilawar@intel.com>
>>>>> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/xe/xe_force_wake.h | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/xe/xe_force_wake.h
>>>>> b/drivers/gpu/drm/xe/xe_force_wake.h
>>>>> index 83cb157da7cc..9008928b187f 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_force_wake.h
>>>>> +++ b/drivers/gpu/drm/xe/xe_force_wake.h
>>>>> @@ -32,7 +32,7 @@ static inline void
>>>>>    xe_force_wake_assert_held(struct xe_force_wake *fw,
>>>>>                  enum xe_force_wake_domains domain)
>>>>>    {
>>>>> -    xe_gt_assert(fw->gt, fw->awake_domains & domain);
>>>>> +    xe_gt_assert(fw->gt, (fw->awake_domains & domain) == domain);
>>>> This will always assert for when domain FORCEWAKE_ALL (0xFF).
>>>> Not all the platforms support all the domains.
>>>> e.g. MTL GT0 support GT and RENDER domain. So for forcewake all use
>>>> case only bits for GT and RENDER will be set.
>>> I think to handle this correctly in struct xe_force_wake you can add new
>>> enum xe_force_wake_domains supported_domains to hold bitmap of supported
>>> forcewake domains. Use this bit map to check appropriate domains are
>>> set.
>>
>> Hi Badal,
>>
>> Thanks for taking time to review this. Agreed the check should be based on
>> supported domains.  Will look into this.
> 
> I guess the real question here is why we'd ever be passing
> XE_FORCEWAKE_ALL to xe_force_wake_assert_held().  That assertion is used
> to sanity check that we're actually holding a necessary power domain
> before performing some operation that relies on it.  Nothing in the
> hardware should ever actually _need_ every single forcewake to be held
> at once; we just tend to grab XE_FORCEWAKE_ALL in some places of the
> code because it's simpler to just blindly grab everything at once (even
> the ones we don't truly need) than it is to figure out the specific set
> of domains that will get used.

In the save/restore code path, both at the top level and in subsequent 
levels, xe_forcewake_get() is called with XE_FORCEWAKE_ALL, as I believe 
it accesses registers from different domains. In my opinion at 
subsequent levels we should 
%s/xe_forcewake_get/xe_force_wake_assert_held(XE_FORCEWAKE_ALL).

Regards,
Badal
> 
> 
> Matt
> 
>>
>> BR
>>
>> Himal
>>
>>
>>>
>>> Regards,
>>> Badal
>>>>
>>>> Regards,
>>>> Badal
>>>>>    }
>>>>>    #endif
>