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

[Matt Roper <matthew.d.roper@intel.com>]

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

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.


Matt

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

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation