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

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

On 06-06-2024 11:00, Riana Tauro wrote:
> 
> 
> 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, for updating.

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