Re: [PATCH v3 1/3] drm/xe: Cleanup force wake registers bit definitions

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

On Wed, Jun 05, 2024 at 11:14:48PM +0530, Ghimiray, Himal Prasad wrote:
> 
> 
> On 05-06-2024 22:58, Rodrigo Vivi wrote:
> > On Wed, Jun 05, 2024 at 03:20:03PM +0530, Himal Prasad Ghimiray wrote:
> > > - Remove unused bit definitions.
> > > - Driver uses BIT(0) for waking/sleeping the domain and since the
> > > registers are masked respective mask bit BIT(16) needs to be set. Use
> > > defines for these bits and use them in domain initialization.
> > > 
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Badal Nilawar <badal.nilawar@intel.com>
> > > Suggested-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > > Reviewed-by: Badal Nilawar <badal.nilawar@intel.com>
> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >   drivers/gpu/drm/xe/regs/xe_gt_regs.h |  8 +++++---
> > >   drivers/gpu/drm/xe/xe_force_wake.c   | 18 ++++++++++++------
> > >   2 files changed, 17 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> > > index d09b2473259f..47c26c37608d 100644
> > > --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> > > +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> > > @@ -487,9 +487,11 @@
> > >   	((ccs) << ((cslice) * CCS_MODE_CSLICE_WIDTH))
> > >   #define FORCEWAKE_ACK_GT			XE_REG(0x130044)
> > > -#define   FORCEWAKE_KERNEL			BIT(0)
> > > -#define   FORCEWAKE_USER			BIT(1)
> > > -#define   FORCEWAKE_KERNEL_FALLBACK		BIT(15)
> > > +
> > > +/* Applicable for all FORCEWAKE_DOMAIN and FORCEWAKE_ACK_DOMAIN regs */
> > > +#define   FORCEWAKE_KERNEL			0
> > > +#define   FORCEWAKE_MT(bit)			BIT(bit)
> > > +#define   FORCEWAKE_MT_MASK(bit)		BIT((bit) + 16)
> > >   #define MTL_MEDIA_PERF_LIMIT_REASONS		XE_REG(0x138030)
> > >   #define MTL_MEDIA_MC6				XE_REG(0x138048)
> > > diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
> > > index 9bbe8a5040da..54279c3814af 100644
> > > --- a/drivers/gpu/drm/xe/xe_force_wake.c
> > > +++ b/drivers/gpu/drm/xe/xe_force_wake.c
> > > @@ -52,13 +52,15 @@ void xe_force_wake_init_gt(struct xe_gt *gt, struct xe_force_wake *fw)
> > >   			    XE_FW_DOMAIN_ID_GT,
> > >   			    FORCEWAKE_GT,
> > >   			    FORCEWAKE_ACK_GT_MTL,
> > > -			    BIT(0), BIT(16));
> > > +			    FORCEWAKE_MT(FORCEWAKE_KERNEL),
> > > +			    FORCEWAKE_MT_MASK(FORCEWAKE_KERNEL));
> > 
> > hmm.... looking at this now I believe it would be better to just pass the FORCEWAKE_KERNEL bit
> > number as param and then use the MT and MT_MASK inside the domain_init function...
> 
> Hmm makes sense. Do we expect to use any other bit apart from
> FORCEWAKE_KERNEL? If not, how about not passing FORCEWAKE_KERNEL at all and
> instead directly using MT and MT_MASK inside the domain_init function?

even better indeed!

> 
> > 
> > but up you... my rv-b remains whatever you decide.
> > 
> > >   	} else {
> > >   		domain_init(&fw->domains[XE_FW_DOMAIN_ID_GT],
> > >   			    XE_FW_DOMAIN_ID_GT,
> > >   			    FORCEWAKE_GT,
> > >   			    FORCEWAKE_ACK_GT,
> > > -			    BIT(0), BIT(16));
> > > +			    FORCEWAKE_MT(FORCEWAKE_KERNEL),
> > > +			    FORCEWAKE_MT_MASK(FORCEWAKE_KERNEL));
> > >   	}
> > >   }
> > > @@ -74,7 +76,8 @@ void xe_force_wake_init_engines(struct xe_gt *gt, struct xe_force_wake *fw)
> > >   			    XE_FW_DOMAIN_ID_RENDER,
> > >   			    FORCEWAKE_RENDER,
> > >   			    FORCEWAKE_ACK_RENDER,
> > > -			    BIT(0), BIT(16));
> > > +			    FORCEWAKE_MT(FORCEWAKE_KERNEL),
> > > +			    FORCEWAKE_MT_MASK(FORCEWAKE_KERNEL));
> > >   	for (i = XE_HW_ENGINE_VCS0, j = 0; i <= XE_HW_ENGINE_VCS7; ++i, ++j) {
> > >   		if (!(gt->info.engine_mask & BIT(i)))
> > > @@ -84,7 +87,8 @@ void xe_force_wake_init_engines(struct xe_gt *gt, struct xe_force_wake *fw)
> > >   			    XE_FW_DOMAIN_ID_MEDIA_VDBOX0 + j,
> > >   			    FORCEWAKE_MEDIA_VDBOX(j),
> > >   			    FORCEWAKE_ACK_MEDIA_VDBOX(j),
> > > -			    BIT(0), BIT(16));
> > > +			    FORCEWAKE_MT(FORCEWAKE_KERNEL),
> > > +			    FORCEWAKE_MT_MASK(FORCEWAKE_KERNEL));
> > >   	}
> > >   	for (i = XE_HW_ENGINE_VECS0, j = 0; i <= XE_HW_ENGINE_VECS3; ++i, ++j) {
> > > @@ -95,7 +99,8 @@ void xe_force_wake_init_engines(struct xe_gt *gt, struct xe_force_wake *fw)
> > >   			    XE_FW_DOMAIN_ID_MEDIA_VEBOX0 + j,
> > >   			    FORCEWAKE_MEDIA_VEBOX(j),
> > >   			    FORCEWAKE_ACK_MEDIA_VEBOX(j),
> > > -			    BIT(0), BIT(16));
> > > +			    FORCEWAKE_MT(FORCEWAKE_KERNEL),
> > > +			    FORCEWAKE_MT_MASK(FORCEWAKE_KERNEL));
> > >   	}
> > >   	if (gt->info.engine_mask & BIT(XE_HW_ENGINE_GSCCS0))
> > > @@ -103,7 +108,8 @@ void xe_force_wake_init_engines(struct xe_gt *gt, struct xe_force_wake *fw)
> > >   			    XE_FW_DOMAIN_ID_GSC,
> > >   			    FORCEWAKE_GSC,
> > >   			    FORCEWAKE_ACK_GSC,
> > > -			    BIT(0), BIT(16));
> > > +			    FORCEWAKE_MT(FORCEWAKE_KERNEL),
> > > +			    FORCEWAKE_MT_MASK(FORCEWAKE_KERNEL));
> > >   }
> > >   static void domain_wake(struct xe_gt *gt, struct xe_force_wake_domain *domain)
> > > -- 
> > > 2.25.1
> > >