[PATCH] drm/xe: Fix xe_force_wake_assert_held for enum XE_FORCEWAKE_ALL
Riana Tauro
riana.tauro at intel.com
Thu Jun 6 05:30:40 UTC 2024
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 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.
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
>>>>
>>
More information about the Intel-xe
mailing list