[PATCH v2 2/3] drm/xe/query: Move timestamp reg to hwe_read_timestamp()

[Lucas De Marchi]

On Fri, Oct 11, 2024 at 02:17:15PM -0700, Matt Roper wrote:
>On Thu, Oct 10, 2024 at 08:56:17PM -0700, Lucas De Marchi wrote:
>> __read_timestamps() is actually reading the timestamp from a certain
>> hwe. Use it as parameter, move register declarations to be inside that
>> function and rename it.
>>
>> Reviewed-by: Sai Teja Pottumuttu <sai.teja.pottumuttu at intel.com>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_query.c | 22 +++++++---------------
>>  1 file changed, 7 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
>> index 07093af0b32e3..69307ff4146a1 100644
>> --- a/drivers/gpu/drm/xe/xe_query.c
>> +++ b/drivers/gpu/drm/xe/xe_query.c
>> @@ -85,16 +85,13 @@ static __ktime_func_t __clock_id_to_func(clockid_t clk_id)
>>  }
>>
>>  static void
>> -__read_timestamps(struct xe_gt *gt,
>> -		  struct xe_reg lower_reg,
>> -		  struct xe_reg upper_reg,
>> -		  u64 *engine_ts,
>> -		  u64 *cpu_ts,
>> -		  u64 *cpu_delta,
>> -		  __ktime_func_t cpu_clock)
>> +hwe_read_timestamp(struct xe_hw_engine *hwe, u64 *engine_ts, u64 *cpu_ts,
>> +		   u64 *cpu_delta, __ktime_func_t cpu_clock)
>>  {
>> -	struct xe_mmio *mmio = &gt->mmio;
>> +	struct xe_mmio *mmio = &hwe->gt->mmio;
>>  	u32 upper, lower, old_upper, loop = 0;
>> +	struct xe_reg upper_reg = RING_TIMESTAMP_UDW(hwe->mmio_base),
>> +		      lower_reg = RING_TIMESTAMP(hwe->mmio_base);
>>
>>  	upper = xe_mmio_read32(mmio, upper_reg);
>
>Not specifically related to this patch, but is there any reason why
>we're not using xe_mmio_read64_2x32() to read the timestamp?  It already
>has rollover logic specifically to handle timestamp reads like this.

yeah... I was thinking the same thing when implementing this: why are we
not re-using the _2x32() variant. But... see where the CPU timestamps
are taken. We can't simply take the CPU timestamp before and after
xe_mmio_read64_2x32() - it's in the middle of the calls with
configurable clock id (via vfunc).

>
>Anyway, this patch looks fine.
>
>Reviewed-by: Matt Roper <matthew.d.roper at intel.com>

thanks
Lucas De Marchi