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

[Ghimiray, Himal Prasad]

On 31-05-2024 20:56, Rodrigo Vivi wrote:
> On Fri, May 31, 2024 at 07:26:33PM +0530, Ghimiray, Himal Prasad wrote:
>>     On 31-05-2024 18:49, Rodrigo Vivi wrote:
>>                                                                                  
>>     On Fri, May 31, 2024 at 12:18:45PM +0530, Himal Prasad Ghimiray wrote:
>>                                                                                  
>>     >Make sure that the assertion condition covers the wakefulness of all
>>     >supported domains for XE_FORCEWAKE_ALL.
>>                                                                                  
>>     The most important part is missing here: why? what issue are we trying to solve?
>>                                                                                  
>>     Hi Rodrigo,
>>                                                                                  
> Please avoid Outlook...
>
>>     AFAIU,  xe_force_wake_assert_held is designed to trigger an assertion if
>>     the provided domain is not awake, which works correctly for individual
>>     domains. However, the assertion condition becomes incorrect for
>>     XE_FORCEWAKE_ALL.
>>                                                                                  
>>     For instance, if we assume all domains are in sleep mode, invoking
>>     xe_force_wake_get(fw, XE_FORCEWAKE_GT) will only awaken the "gt" domain.
>>     Subsequently, another function needs that all domains are awake and
>>     utilizes xe_force_wake_assert_held(fw, XE_FORCEWAKE_ALL).
>>                                                                                  
>>     In this scenario, the condition will inaccurately return success because
>>     fw->awake_domains (0x1) & XE_FORCEWAKE_ALL (0xFF) will still be 0x1 and
>>     Ideally it should have asserted.
> It makes total sense. Could you please ensure that the commit message include
> this important reasoning?!


sure. Will update the commit message.


>
> Then, the other commit also needs to explain why a simple
> (XE_FORCEWAKE_ALL & fw->awake_domains) == XE_FORCEWAKE_ALL doesn't work so
> the supported/initialized domains needed to be created.


ok.


>
> Btw, perhaps initialized_domains is better then supported?


Ok. Will use initialized_domains.


>
>>                                                                                  
>>                                                                                  
>>                                                                                  
>>     >Cc: Rodrigo Vivi [1]<rodrigo.vivi at intel.com>
>>     >Cc: Badal Nilawar [2]<badal.nilawar at intel.com>
>>     >Signed-off-by: Himal Prasad Ghimiray [3]<himal.prasad.ghimiray at intel.com>
>>     >---
>>     > drivers/gpu/drm/xe/xe_force_wake.h | 7 ++++++-
>>     > 1 file changed, 6 insertions(+), 1 deletion(-)
>>                                                                                  
>>     >diff --git a/drivers/gpu/drm/xe/xe_force_wake.h b/drivers/gpu/drm/xe/xe_force_wake.h
>>     >index 83cb157da7cc..4c986d72cba7 100644
>>     >--- a/drivers/gpu/drm/xe/xe_force_wake.h
>>     >+++ b/drivers/gpu/drm/xe/xe_force_wake.h
>>     >@@ -32,7 +32,12 @@ 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);
>>     >+       enum xe_force_wake_domains is_awake;
>>     >+
>>     >+       is_awake = (domain == XE_FORCEWAKE_ALL) ?
>>     >+                   fw->supported_domains : domain;
>>     >+
>>     >+       xe_gt_assert(fw->gt, (fw->awake_domains & is_awake) == is_awake);
>>     > }
>>     >
>>     > #endif
>>     >--
>>     >2.25.1
>>
>> References
>>
>>     Visible links
>>     1. mailto:rodrigo.vivi at intel.com
>>     2. mailto:badal.nilawar at intel.com
>>     3. mailto:himal.prasad.ghimiray at intel.com