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

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

On 05-06-2024 00:18, Rodrigo Vivi wrote:
> On Mon, Jun 03, 2024 at 02:31:45PM +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>
>> ---
>>   drivers/gpu/drm/xe/regs/xe_gt_regs.h |  7 ++++---
>>   drivers/gpu/drm/xe/xe_force_wake.c   | 18 ++++++++++++------
>>   2 files changed, 16 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..441747f2d28a 100644
>> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> @@ -487,9 +487,10 @@
>>   	((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_THREAD_XE			BIT(0)
> 
> Please keep it as FORCEWAKE_KERNEL since we already know that is
> the multi-thread one reserved for kernel/kmd usage (xe, i915, windows,...)

Agreed. Will fix it.

> 
>> +#define   FORCEWAKE_THREAD_XE_MASK		BIT(16)
> 
> please do not define a bit, but a mask macro.
> 
> #define FORCEWAKE_MT_MASK(bit)  BIT(bit + 16)
> 
> just to be 100% with spec which says: "Note that mask bit is the data bit
> offset + 16"

Makes sense. Will update it.

> 
> with that changed:
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Thanks.

> 
> 
>>   
>>   #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..64721c01f30d 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_THREAD_XE,
>> +			    FORCEWAKE_THREAD_XE_MASK);
>>   	} 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_THREAD_XE,
>> +			    FORCEWAKE_THREAD_XE_MASK);
>>   	}
>>   }
>>   
>> @@ -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_THREAD_XE,
>> +			    FORCEWAKE_THREAD_XE_MASK);
>>   
>>   	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_THREAD_XE,
>> +			    FORCEWAKE_THREAD_XE_MASK);
>>   	}
>>   
>>   	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_THREAD_XE,
>> +			    FORCEWAKE_THREAD_XE_MASK);
>>   	}
>>   
>>   	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_THREAD_XE,
>> +			    FORCEWAKE_THREAD_XE_MASK);
>>   }
>>   
>>   static void domain_wake(struct xe_gt *gt, struct xe_force_wake_domain *domain)
>> -- 
>> 2.25.1
>>