Re: [PATCH] drm/panthor: extend timestamp query with flags

[Steven Price <steven.price@arm.com>]

On 18/03/2026 14:51, Marcin Ślusarz wrote:
> On Wed, Mar 18, 2026 at 01:10:30PM +0100, Boris Brezillon wrote:
>> On Wed, 18 Mar 2026 12:29:52 +0100
>> Marcin Slusarz <marcin.slusarz@arm.com> wrote:
>>
>>> Flags now control which data user space wants to query,
>>> there is more information sources, and there's ability
>>> to query duration of multiple timestamp reads.
>>>
>>> New sources:
>>> - CPU's monotonic,
>>> - CPU's monotonic raw,
>>> - GPU's cycle count
>>>
>>> These changes should make the implementation of
>>> VK_KHR_calibrated_timestamps more accurate and much simpler.
>>>
>>> Signed-off-by: Marcin Slusarz <marcin.slusarz@arm.com>
>>> ---
>>> This is counter proposal to https://lore.kernel.org/all/20250916200751.3999354-1-olvaffe@gmail.com/
>>> ---
>>>  drivers/gpu/drm/panthor/panthor_drv.c | 124 ++++++++++++++++++++++++--
>>>  include/uapi/drm/panthor_drm.h        |  51 ++++++++++-
>>>  2 files changed, 166 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
>>> index 165dddfde6ca..19ede20a578e 100644
>>> --- a/drivers/gpu/drm/panthor/panthor_drv.c
>>> +++ b/drivers/gpu/drm/panthor/panthor_drv.c
>>> @@ -13,7 +13,9 @@
>>>  #include <linux/pagemap.h>
>>>  #include <linux/platform_device.h>
>>>  #include <linux/pm_runtime.h>
>>> +#include <linux/sched/clock.h>
>>>  #include <linux/time64.h>
>>> +#include <linux/time_namespace.h>
>>>  
>>>  #include <drm/drm_auth.h>
>>>  #include <drm/drm_debugfs.h>
>>> @@ -762,21 +764,123 @@ static void panthor_submit_ctx_cleanup(struct panthor_submit_ctx *ctx,
>>>  }
>>>  
>>>  static int panthor_query_timestamp_info(struct panthor_device *ptdev,
>>> -					struct drm_panthor_timestamp_info *arg)
>>> +					struct drm_panthor_timestamp_info *arg,
>>> +					u32 size)
>>>  {
>>>  	int ret;
>>> +	u32 flags;
>>> +	unsigned long irq_flags;
>>> +	struct timespec64 cpu_ts;
>>> +	u64 query_start_time;
>>> +	bool minimize_interruption;
>>> +	u32 timestamp_types = 0;
>>> +
>>> +	if (size >= offsetof(struct drm_panthor_timestamp_info, pad1) + sizeof(arg->pad1) &&
>>> +			arg->pad1 != 0)
>>> +		return -EINVAL;
>>> +
>>> +	if (size >= offsetof(struct drm_panthor_timestamp_info, flags) + sizeof(arg->flags))
>>> +		flags = arg->flags;
>>> +	else
>>> +		flags = DRM_PANTHOR_TIMESTAMP_GPU |
>>> +			DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
>>> +			DRM_PANTHOR_TIMESTAMP_FREQ;
>>
>> How about we add a DRM_PANTHOR_TIMESTAMP_ADVANCED_QUERY flag that tells
>> the driver whether the default should be picked or not instead of this
>> weird is-this-the-new-or-old-struct detection based on the size.
> 
> Well, as is, we would read uninitialized data from kernel stack if
> user passed old struct with the original size. It's fixable, but
> I'm not sure why you think checking size to detect the use of new
> interface is weird. I thought it's a pretty standard thing.

What you need is copy_struct_from_user() - it will zero any fields that
user space didn't provide. So adding a flags field to the end of the
struct will be guaranteed to be zero with old (binary of) user space.

This is the standard way of extending an API. If user space is
recompiled with new headers then user space will pass in the larger size
(because it uses sizeof()), but will zero initialise any fields that it
doesn't know about. If you look purely at the size passed by userspace
then the sizeof() will be wrong and no flags will get set.

> If the conclusion will be that checking size must be dropped, then
> I think looking at flags being non-zero would be enough - there's
> no need for new special flag that says other bits mean something.

That would be fine if we don't want the 'default' flags behaviour you
have above. So either GPU/GPU_OFFSET/FREQ are unconditionally enabled or
you need to reverse the meaning of those flags. Or of course go with
Boris's suggestion of a flag to enable the new behaviour.

[...]

>>>  /**
>>>   * struct drm_panthor_timestamp_info - Timestamp information
>>>   *
>>> @@ -421,11 +450,29 @@ struct drm_panthor_timestamp_info {
>>>  	 */
>>>  	__u64 timestamp_frequency;
>>>  
>>> -	/** @current_timestamp: The current timestamp. */
>>> +	/** @current_timestamp: The current GPU timestamp. */
>>>  	__u64 current_timestamp;
>>>  
>>> -	/** @timestamp_offset: The offset of the timestamp timer. */
>>> +	/** @timestamp_offset: The offset of the GPU timestamp timer. */
>>>  	__u64 timestamp_offset;
>>> +
>>> +	/** @flags: Bitmask of drm_panthor_timestamp_info_flags. */
>>> +	__u32 flags;
>>> +
>>> +	/** @duration_nsec: Duration of time query. */
>>> +	__u32 duration_nsec;
>>> +
>>> +	/** @cycle_count: Value of GPU_CYCLE_COUNT. */
>>> +	__u64 cycle_count;
>>> +
>>> +	/** @cpu_timestamp_sec: Seconds part of CPU timestamp. */
>>> +	__u64 cpu_timestamp_sec;
>>> +
>>> +	/** @cpu_timestamp_nsec: Nanseconds part of CPU timestamp. */
>>> +	__u32 cpu_timestamp_nsec;
>>> +
>>> +	/** @pad1: Padding, MBZ. */
>>> +	__u32 pad1;
>>
>> Let's re-purpose the existing pad field into flags, move duration_nsec after
>> cpu_timestamp_nsec, and get rid of this pad1.
> 
> I'm not sure I understand. Do you want me to extend flags to u64?
> What's the point of that?

I'm not sure I necessarily understand Boris's comment either, but I
would suggest making flags u64 would be better.

By shuffling things around to have a u64 flags you no longer have any
padding fields. And the unused part of the flags will be naturally
checked for being 0 rather than the explicit check for pad1 you
currently have.

Not a big deal to me - but it's easier to just avoid padding fields
where possible as they often get overlooked in the validation.

Thanks,
Steve