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

[Matt Roper]

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.


Matt

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

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