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

[Ghimiray, Himal Prasad]

On 06-06-2024 02:39, Matt Roper wrote:
> On Tue, Jun 04, 2024 at 04:22:00PM +0530, Nilawar, Badal wrote:
>>
>>
>> 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 at intel.com>
>>>>>>> Cc: Badal Nilawar <badal.nilawar at intel.com>
>>>>>>> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray at 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).
> 
> We just grab FORCEWAKE_ALL because we're lazy and don't want to add the
> code complexity to figure out the exact subset of power domains
> that are actually needed (which may vary by platform).  We usually do
> FORCEWAKE_ALL in places like device initialization or suspend/resume
> that aren't in a hot path and are only going to take a couple
> miliseconds total.  If multiple levels of the call stack grab forcewake
> redundantly, that's fine; forcewake is reference counted, so the calls
> lower in the callstack just increment the reference count and return
> immediately, as we'd expect (assuming every get has a paired put).


Agreed, the subsequent calls to xe_forcewake_get() and 
xe_forcewake_put() merely increment and decrement the reference count. 
However, if we are confident that the caller is already managing 
xe_forcewake_get()/put() properly and the function operates 
synchronously, would it be reasonable to acquire spinlocks solely for 
the purpose of incrementing and decrementing the reference count?


> 
> xe_force_wake_assert_held() is intended for places where we know we need
> a specific forcewake domain and need to make sure the function never
> accidentally gets called from somewhere that the domain wasn't already
> held.  I don't think calling it with FORCEWAKE_ALL make sense since that
> implies you don't actually know which domains were necessary; if you do
> that it will just impair our ability to do more focused forcewake
> acquisition in the future.

I believe this is the gap: After xe_force_wake_get of FORCEWAKE_ALL, the 
assumption is xe_force_wake_assert_held can handle the enum 
FORCEWAKE_ALL to confirm whether all domains are awake or not. However, 
this is broken: the function is written in a way that it can't handle 
more than one domain at a time.

For example, the caller of xe_gt_idle_disable_c6 uses force_wake_get 
with all domains and simply relies on 
xe_force_wake_assert_held(gt_to_fw(gt), XE_FORCEWAKE_ALL); within 
xe_gt_idle_disable_c6 to proceed with register write, without actually 
caring for actual domain it needs.

If we see no real use of xe_force_wake_assert_held with 
XE_FORCEWAKE_ALL, I will proceed with dropping patches [2/3] and [3/3] 
from https://lore.kernel.org/intel-xe/ZmDhQJLrleUjetIX@intel.com/T/#t 
and will add a BUILD_BUG_ON if the user calls xe_force_wake_assert_held 
with more than one domain.

And from BSPEC, it looks like xe_force_wake_assert_held inside 
xe_gt_idle_disable_c6 should use XE_FORCEWAKE_GT.


BR
Himal


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