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

[Riana Tauro <riana.tauro@intel.com>]

On 6/6/2024 10:04 AM, Ghimiray, Himal Prasad wrote:
> 
> 
> 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@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).
>>
>> 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.

Hi Himal

xe_gt_idle_disable_c6 issue is fixed in 
https://patchwork.freedesktop.org/series/133519/ and RB'd.

Will push it

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