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

Thu Jun 6 05:19:55 UTC 2024

On Thu, Jun 06, 2024 at 10:04:33AM GMT, 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 

totally agree with Matt here. that call doesn't make sense and is the
one to be fixed.

>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.

yep, that is the fix we need.

Lucas De Marchi

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