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

[Boris Brezillon <boris.brezillon@collabora.com>]

On Wed, 18 Mar 2026 17:27:26 +0100
Marcin Ślusarz <marcin.slusarz@arm.com> wrote:

> 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.

Works too. If zero is not a valid combination, it can be used to encode
the previous default behavior. I'm fine with that as long as it's
documented in the uAPI doc.

> 
> > > 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?