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

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

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.

Okay, so the problem seems to be that we've always treated DEV_QUERY
args as write-only, and we don't have this copy_struct_from_user() trick
on the args yet. Given the opt-out/in discussion, and the fact we want
something that's read-write (reads flags, outputs timestamps and other
props), I'd be tempted to make that a separate DEV_QUERY
(DRM_PANTHOR_DEV_QUERY_ADVANCED_TIMESTAMP_INFO?).