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

[Marcin Ślusarz <marcin.slusarz@arm.com>]

On Wed, Mar 18, 2026 at 05:06:50PM +0100, Boris Brezillon wrote:
> On Wed, 18 Mar 2026 15:20:18 +0000
> Steven Price <steven.price@arm.com> wrote:
> 
> > 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 ^.

Ok, I'm convinced. Will do that in the next version.

> > 
> > 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.
> 
> Right, if you don't want the extra ADVANCED_QUERY flag, the GPU,
> GPU_OFFSET and FREQ flags need to be opt-out, but that's a bit
> confusing if the other flags are opt-in.

Flags == 0 doesn't make any sense, so we can translate 0 to
the combination of flags that matches previous behavior.

> > 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.
> 
> I was confused by the fact the field was named pad1, and I assumed
> there was a pad field already present in the struct, which is why I
> suggested re-purposing that one instead of adding a new field that
> would in turn require extra padding. Given there's no pre-existing
> padding, I'd rename pad1 into pad and call it a day.

I named it that way to make sure that future padding fields are named
consistently.

> > 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.
> 
> We certainly want to ensure they are, this way we can re-purpose
> existing padding fields instead of adding new ones when we need to
> extend the logic.

I don't know why, but https://docs.kernel.org/process/botching-up-ioctls.html
suggests that both seconds and nanoseconds should be 64-bit, so maybe
we could extend cpu_timestamp_nsec and forget about this?