Re: [PATCH 2/3] drm/xe/vf: Return error code from xe_hw_engine_read_timestamp()

["Bernatowicz, Marcin" <marcin.bernatowicz@linux.intel.com>]

On 2/4/2025 7:53 PM, Michal Wajdeczko wrote:
> 
> 
> On 04.02.2025 19:05, Marcin Bernatowicz wrote:
>> Modify xe_hw_engine_read_timestamp() to return an error code instead
>> of a u64 timestamp. Return -EOPNOTSUPP when called from Virtual Function
>> (VF) mode to avoid reading the inaccessible RING_TIMESTAMP register.
>>
>> Update function signature to pass the timestamp via a pointer argument.
>>
>> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_hw_engine.c | 10 ++++++++--
>>   drivers/gpu/drm/xe/xe_hw_engine.h |  2 +-
>>   2 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
>> index fc447751fe78..ab91c15f3a8b 100644
>> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
>> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
>> @@ -981,9 +981,15 @@ const char *xe_hw_engine_class_to_str(enum xe_engine_class class)
>>   	return NULL;
>>   }
>>   
>> -u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe)
>> +int xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe, u64 *gpu_timestamp)
>>   {
>> -	return xe_mmio_read64_2x32(&hwe->gt->mmio, RING_TIMESTAMP(hwe->mmio_base));
>> +	if (IS_SRIOV_VF(gt_to_xe(hwe->gt)))
>> +		return -EOPNOTSUPP;
> 
> there seems to be 1 caller of this function
> is it worth to make change here instead of fixing the caller ?
> 
Fixing the function seems more reasonable than tracking the caller(s).
However, considering the subsequent comments, this patch should be dropped.

>> +
>> +	*gpu_timestamp = xe_mmio_read64_2x32(&hwe->gt->mmio,
>> +					     RING_TIMESTAMP(hwe->mmio_base));
>> +
>> +	return 0;
>>   }
>>   
>>   enum xe_force_wake_domains xe_hw_engine_to_fw_domain(struct xe_hw_engine *hwe)
>> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h b/drivers/gpu/drm/xe/xe_hw_engine.h
>> index 6b5f9fa2a594..df377da82770 100644
>> --- a/drivers/gpu/drm/xe/xe_hw_engine.h
>> +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
>> @@ -73,7 +73,7 @@ static inline bool xe_hw_engine_is_valid(struct xe_hw_engine *hwe)
>>   }
>>   
>>   const char *xe_hw_engine_class_to_str(enum xe_engine_class class);
>> -u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe);
>> +int xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe, u64 *gpu_timestamp);
>>   enum xe_force_wake_domains xe_hw_engine_to_fw_domain(struct xe_hw_engine *hwe);
>>   
>>   void xe_hw_engine_mmio_write32(struct xe_hw_engine *hwe, struct xe_reg reg, u32 val);
> 
> and this patch will not compile without 3/3

Indeed, my bad

>