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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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?!

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.

Btw, perhaps initialized_domains is better then supported?

>                                                                                 
>                                                                                 
>                                                                                 
>    >Cc: Rodrigo Vivi [1]<rodrigo.vivi@intel.com>                                
>    >Cc: Badal Nilawar [2]<badal.nilawar@intel.com>                              
>    >Signed-off-by: Himal Prasad Ghimiray [3]<himal.prasad.ghimiray@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@intel.com
>    2. mailto:badal.nilawar@intel.com
>    3. mailto:himal.prasad.ghimiray@intel.com