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

["Pottumuttu, Sai Teja" <sai.teja.pottumuttu@intel.com>]

On 10-10-2024 18:54, Lucas De Marchi wrote:
> On Thu, Oct 10, 2024 at 03:33:30PM +0530, Pottumuttu, Sai Teja wrote:
>>
>> On 10-10-2024 09:18, 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.
>>>
>>> Signed-off-by: Lucas De Marchi <lucas.demarchi@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 a1f4cc25bea68..d86959076b078 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,
>>
>> Nit: Probably the function name should be hwe_read_timestamps (s at 
>> last), just like the previous name and also as its reads multiple 
>> timestamps I believe.
>
> I went back and forth on the name... this function is actually reading
> the engine timestamp and the additional args for CPU timestamp/delta are
> only reporting "how long did it take to read the engine timestamp"
>
> so.... the change was intentional.

Got it, from this pov the name makes sense. Thanks for the clarification.

- Sai Teja

>
>>
>> With that LGTM,
>>
>> Reviewed-by: Sai Teja Pottumuttu <sai.teja.pottumuttu@intel.com>
>
> thanks
> Lucas De Marchi
>
>>
>>> +           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);
>>>      do {
>>> @@ -155,13 +152,8 @@ query_engine_cycles(struct xe_device *xe,
>>>      if (xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL))
>>>          return -EIO;
>>> -    __read_timestamps(gt,
>>> -              RING_TIMESTAMP(hwe->mmio_base),
>>> -              RING_TIMESTAMP_UDW(hwe->mmio_base),
>>> -              &resp.engine_cycles,
>>> -              &resp.cpu_timestamp,
>>> -              &resp.cpu_delta,
>>> -              cpu_clock);
>>> +    hwe_read_timestamp(hwe, &resp.engine_cycles, &resp.cpu_timestamp,
>>> +               &resp.cpu_delta, cpu_clock);
>>>      xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);