Re: [PATCH v2 1/2] drm/xe/guc: Capture all available bits of GuC timestamp

[John Harrison <john.c.harrison@intel.com>]

On 10/24/2024 14:12, Matt Roper wrote:
> On Wed, Oct 23, 2024 at 05:25:53PM -0700, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> The extra bits are not hugely useful because the GuC log only uses
>> 32bit time stamps. But they exist so might as well provide them.
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
>> ---
>>   drivers/gpu/drm/xe/regs/xe_guc_regs.h | 3 ++-
>>   drivers/gpu/drm/xe/xe_guc_log.c       | 6 +++---
>>   drivers/gpu/drm/xe/xe_guc_log_types.h | 2 +-
>>   3 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/regs/xe_guc_regs.h b/drivers/gpu/drm/xe/regs/xe_guc_regs.h
>> index b27b73680c12..2118f7dec287 100644
>> --- a/drivers/gpu/drm/xe/regs/xe_guc_regs.h
>> +++ b/drivers/gpu/drm/xe/regs/xe_guc_regs.h
>> @@ -84,7 +84,8 @@
>>   #define   HUC_LOADING_AGENT_GUC			REG_BIT(1)
>>   #define   GUC_WOPCM_OFFSET_VALID		REG_BIT(0)
>>   #define GUC_MAX_IDLE_COUNT			XE_REG(0xc3e4)
>> -#define GUC_PMTIMESTAMP				XE_REG(0xc3e8)
>> +#define GUC_PMTIMESTAMP_LO			XE_REG(0xc3e8)
>> +#define GUC_PMTIMESTAMP_HI			XE_REG(0xc3ec)
> Do we actually need this change if we're not using _HI directly
> anywhere?
Define need?

It makes it obvious that the register read is correct in doing a 64_2x32 
operation. Otherwise, it can look like a bug that we are reading beyond 
the valid register definition. So while it might not be required from a 
compilation point of view, I think it is useful from a code readability 
point of view.

John.

>
>
> Matt
>
>>   
>>   #define GUC_SEND_INTERRUPT			XE_REG(0xc4c8)
>>   #define   GUC_SEND_TRIGGER			REG_BIT(0)
>> diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/xe_guc_log.c
>> index fead96216243..df4cfb698cdb 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_log.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_log.c
>> @@ -171,9 +171,9 @@ struct xe_guc_log_snapshot *xe_guc_log_snapshot_capture(struct xe_guc_log *log,
>>   
>>   	fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
>>   	if (!fw_ref) {
>> -		snapshot->stamp = ~0;
>> +		snapshot->stamp = ~0ULL;
>>   	} else {
>> -		snapshot->stamp = xe_mmio_read32(&gt->mmio, GUC_PMTIMESTAMP);
>> +		snapshot->stamp = xe_mmio_read64_2x32(&gt->mmio, GUC_PMTIMESTAMP_LO);
>>   		xe_force_wake_put(gt_to_fw(gt), fw_ref);
>>   	}
>>   	snapshot->ktime = ktime_get_boottime_ns();
>> @@ -205,7 +205,7 @@ void xe_guc_log_snapshot_print(struct xe_guc_log_snapshot *snapshot, struct drm_
>>   		   snapshot->ver_found.major, snapshot->ver_found.minor, snapshot->ver_found.patch,
>>   		   snapshot->ver_want.major, snapshot->ver_want.minor, snapshot->ver_want.patch);
>>   	drm_printf(p, "Kernel timestamp: 0x%08llX [%llu]\n", snapshot->ktime, snapshot->ktime);
>> -	drm_printf(p, "GuC timestamp: 0x%08X [%u]\n", snapshot->stamp, snapshot->stamp);
>> +	drm_printf(p, "GuC timestamp: 0x%08llX [%llu]\n", snapshot->stamp, snapshot->stamp);
>>   	drm_printf(p, "Log level: %u\n", snapshot->level);
>>   
>>   	remain = snapshot->size;
>> diff --git a/drivers/gpu/drm/xe/xe_guc_log_types.h b/drivers/gpu/drm/xe/xe_guc_log_types.h
>> index 4d57f8322efc..b3d5c72ac752 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_log_types.h
>> +++ b/drivers/gpu/drm/xe/xe_guc_log_types.h
>> @@ -27,7 +27,7 @@ struct xe_guc_log_snapshot {
>>   	/** @ktime: Kernel time the snapshot was taken */
>>   	u64 ktime;
>>   	/** @stamp: GuC timestamp at which the snapshot was taken */
>> -	u32 stamp;
>> +	u64 stamp;
>>   	/** @level: GuC log verbosity level */
>>   	u32 level;
>>   	/** @ver_found: GuC firmware version */
>> -- 
>> 2.47.0
>>