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

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

[Marcin Slusarz]

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;
+
+	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
+	case 0:
+		break;
+	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
+	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
+		timestamp_types++;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (flags & ~(DRM_PANTHOR_TIMESTAMP_GPU |
+		      DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK |
+		      DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
+		      DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT |
+		      DRM_PANTHOR_TIMESTAMP_FREQ |
+		      DRM_PANTHOR_TIMESTAMP_DURATION))
+		return -EINVAL;
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
+		timestamp_types++;
+	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
+		timestamp_types++;
+
+	minimize_interruption =
+		(flags & DRM_PANTHOR_TIMESTAMP_DURATION) ||
+		(timestamp_types >= 2);
 
 	ret = panthor_device_resume_and_get(ptdev);
 	if (ret)
 		return ret;
 
+	if (flags & DRM_PANTHOR_TIMESTAMP_FREQ) {
 #ifdef CONFIG_ARM_ARCH_TIMER
-	arg->timestamp_frequency = arch_timer_get_cntfrq();
+		arg->timestamp_frequency = arch_timer_get_cntfrq();
 #else
-	arg->timestamp_frequency = 0;
+		arg->timestamp_frequency = 0;
 #endif
-	arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
-	arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
+	} else {
+		arg->timestamp_frequency = 0;
+	}
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_OFFSET)
+		arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
+	else
+		arg->timestamp_offset = 0;
+
+	if (minimize_interruption) {
+		preempt_disable();
+		local_irq_save(irq_flags);
+	}
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
+		query_start_time = local_clock();
+	else
+		query_start_time = 0;
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
+		arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
+	else
+		arg->current_timestamp = 0;
+
+	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
+	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
+		ktime_get_ts64(&cpu_ts);
+		break;
+	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
+		ktime_get_raw_ts64(&cpu_ts);
+		break;
+	}
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
+		arg->cycle_count = gpu_read64_counter(ptdev, GPU_CYCLE_COUNT);
+	else
+		arg->cycle_count = 0;
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
+		arg->duration_nsec = local_clock() - query_start_time;
+	else
+		arg->duration_nsec = 0;
+
+	if (minimize_interruption) {
+		local_irq_restore(irq_flags);
+		preempt_enable();
+	}
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
+		timens_add_monotonic(&cpu_ts);
+
+		arg->cpu_timestamp_sec = cpu_ts.tv_sec;
+		arg->cpu_timestamp_nsec = cpu_ts.tv_nsec;
+	} else {
+		arg->cpu_timestamp_sec = 0;
+		arg->cpu_timestamp_nsec = 0;
+	}
 
 	pm_runtime_put(ptdev->base.dev);
 	return 0;
@@ -851,7 +955,12 @@ static int panthor_ioctl_dev_query(struct drm_device *ddev, void *data, struct d
 		return PANTHOR_UOBJ_SET(args->pointer, args->size, ptdev->csif_info);
 
 	case DRM_PANTHOR_DEV_QUERY_TIMESTAMP_INFO:
-		ret = panthor_query_timestamp_info(ptdev, &timestamp_info);
+		if (copy_from_user(&timestamp_info,
+				   (const void __user __force *) args->pointer,
+				   args->size))
+			return -EFAULT;
+
+		ret = panthor_query_timestamp_info(ptdev, &timestamp_info, args->size);
 
 		if (ret)
 			return ret;
@@ -1680,6 +1789,7 @@ static void panthor_debugfs_init(struct drm_minor *minor)
  *       - adds DRM_IOCTL_PANTHOR_BO_SYNC ioctl
  *       - adds DRM_IOCTL_PANTHOR_BO_QUERY_INFO ioctl
  *       - adds drm_panthor_gpu_info::selected_coherency
+ * - 1.8 - extends DEV_QUERY_TIMESTAMP_INFO with flags
  */
 static const struct drm_driver panthor_drm_driver = {
 	.driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ |
@@ -1693,7 +1803,7 @@ static const struct drm_driver panthor_drm_driver = {
 	.name = "panthor",
 	.desc = "Panthor DRM driver",
 	.major = 1,
-	.minor = 7,
+	.minor = 8,
 
 	.gem_create_object = panthor_gem_create_object,
 	.gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table,
diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
index b401ac585d6a..c869e8b95ecd 100644
--- a/include/uapi/drm/panthor_drm.h
+++ b/include/uapi/drm/panthor_drm.h
@@ -409,6 +409,35 @@ struct drm_panthor_csif_info {
 	__u32 pad;
 };
 
+/**
+ * enum drm_panthor_timestamp_info_flags - drm_panthor_timestamp_info.flags
+ */
+enum drm_panthor_timestamp_info_flags {
+	/** @DRM_PANTHOR_TIMESTAMP_GPU: Query GPU time. */
+	DRM_PANTHOR_TIMESTAMP_GPU = 1 << 0,
+
+	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC: Query CPU time using CLOCK_MONOTONIC. */
+	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC = 1 << 1,
+
+	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW: Query CPU time using CLOCK_MONOTONIC_RAW. */
+	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW = 2 << 1,
+
+	/** @DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK: Space reserved for CPU clock type. */
+	DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK = 7 << 1,
+
+	/** @DRM_PANTHOR_TIMESTAMP_GPU_OFFSET: Query GPU offset. */
+	DRM_PANTHOR_TIMESTAMP_GPU_OFFSET = 1 << 4,
+
+	/** @DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT: Query GPU cycle count. */
+	DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT = 1 << 5,
+
+	/** @DRM_PANTHOR_TIMESTAMP_FREQ: Query timestamp frequency. */
+	DRM_PANTHOR_TIMESTAMP_FREQ = 1 << 6,
+
+	/** @DRM_PANTHOR_TIMESTAMP_DURATION: Return duration of time query. */
+	DRM_PANTHOR_TIMESTAMP_DURATION = 1 << 7,
+};
+
 /**
  * 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;
 };
 
 /**
-- 
2.34.1

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

[Boris Brezillon]

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.

	if (args->flags & DRM_PANTHOR_TIMESTAMP_ADVANCED_QUERY)
		flags = args->flags;
	else
		flags = DRM_PANTHOR_TIMESTAMP_GPU |
			DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
			DRM_PANTHOR_TIMESTAMP_FREQ;

> +
> +	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> +	case 0:
> +		break;
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> +		timestamp_types++;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (flags & ~(DRM_PANTHOR_TIMESTAMP_GPU |
> +		      DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK |
> +		      DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> +		      DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT |
> +		      DRM_PANTHOR_TIMESTAMP_FREQ |
> +		      DRM_PANTHOR_TIMESTAMP_DURATION))
> +		return -EINVAL;
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
> +		timestamp_types++;
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
> +		timestamp_types++;
> +
> +	minimize_interruption =
> +		(flags & DRM_PANTHOR_TIMESTAMP_DURATION) ||
> +		(timestamp_types >= 2);

It's probably worth a comment explaining where the >= 2 comes from
and why query duration measurement needs to be done with interrupts
disabled (though the latter is a bit more obvious than the former).

>  
>  	ret = panthor_device_resume_and_get(ptdev);
>  	if (ret)
>  		return ret;
>  
> +	if (flags & DRM_PANTHOR_TIMESTAMP_FREQ) {
>  #ifdef CONFIG_ARM_ARCH_TIMER
> -	arg->timestamp_frequency = arch_timer_get_cntfrq();
> +		arg->timestamp_frequency = arch_timer_get_cntfrq();
>  #else
> -	arg->timestamp_frequency = 0;
> +		arg->timestamp_frequency = 0;
>  #endif
> -	arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
> -	arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
> +	} else {
> +		arg->timestamp_frequency = 0;
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_OFFSET)
> +		arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
> +	else
> +		arg->timestamp_offset = 0;
> +
> +	if (minimize_interruption) {
> +		preempt_disable();
> +		local_irq_save(irq_flags);
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
> +		query_start_time = local_clock();
> +	else
> +		query_start_time = 0;
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
> +		arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
> +	else
> +		arg->current_timestamp = 0;
> +
> +	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> +		ktime_get_ts64(&cpu_ts);
> +		break;
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> +		ktime_get_raw_ts64(&cpu_ts);
> +		break;

	default:
		break;
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
> +		arg->cycle_count = gpu_read64_counter(ptdev, GPU_CYCLE_COUNT);
> +	else
> +		arg->cycle_count = 0;
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
> +		arg->duration_nsec = local_clock() - query_start_time;
> +	else
> +		arg->duration_nsec = 0;
> +
> +	if (minimize_interruption) {
> +		local_irq_restore(irq_flags);
> +		preempt_enable();
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> +		timens_add_monotonic(&cpu_ts);
> +
> +		arg->cpu_timestamp_sec = cpu_ts.tv_sec;
> +		arg->cpu_timestamp_nsec = cpu_ts.tv_nsec;
> +	} else {
> +		arg->cpu_timestamp_sec = 0;
> +		arg->cpu_timestamp_nsec = 0;
> +	}
>  
>  	pm_runtime_put(ptdev->base.dev);
>  	return 0;
> @@ -851,7 +955,12 @@ static int panthor_ioctl_dev_query(struct drm_device *ddev, void *data, struct d
>  		return PANTHOR_UOBJ_SET(args->pointer, args->size, ptdev->csif_info);
>  
>  	case DRM_PANTHOR_DEV_QUERY_TIMESTAMP_INFO:
> -		ret = panthor_query_timestamp_info(ptdev, &timestamp_info);
> +		if (copy_from_user(&timestamp_info,
> +				   (const void __user __force *) args->pointer,
> +				   args->size))
> +			return -EFAULT;
> +
> +		ret = panthor_query_timestamp_info(ptdev, &timestamp_info, args->size);
>  

nit: drop the blank line.

>  		if (ret)
>  			return ret;
> @@ -1680,6 +1789,7 @@ static void panthor_debugfs_init(struct drm_minor *minor)
>   *       - adds DRM_IOCTL_PANTHOR_BO_SYNC ioctl
>   *       - adds DRM_IOCTL_PANTHOR_BO_QUERY_INFO ioctl
>   *       - adds drm_panthor_gpu_info::selected_coherency
> + * - 1.8 - extends DEV_QUERY_TIMESTAMP_INFO with flags
>   */
>  static const struct drm_driver panthor_drm_driver = {
>  	.driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ |
> @@ -1693,7 +1803,7 @@ static const struct drm_driver panthor_drm_driver = {
>  	.name = "panthor",
>  	.desc = "Panthor DRM driver",
>  	.major = 1,
> -	.minor = 7,
> +	.minor = 8,
>  
>  	.gem_create_object = panthor_gem_create_object,
>  	.gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table,
> diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> index b401ac585d6a..c869e8b95ecd 100644
> --- a/include/uapi/drm/panthor_drm.h
> +++ b/include/uapi/drm/panthor_drm.h
> @@ -409,6 +409,35 @@ struct drm_panthor_csif_info {
>  	__u32 pad;
>  };
>  
> +/**
> + * enum drm_panthor_timestamp_info_flags - drm_panthor_timestamp_info.flags
> + */
> +enum drm_panthor_timestamp_info_flags {
> +	/** @DRM_PANTHOR_TIMESTAMP_GPU: Query GPU time. */
> +	DRM_PANTHOR_TIMESTAMP_GPU = 1 << 0,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC: Query CPU time using CLOCK_MONOTONIC. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC = 1 << 1,

Can we define

	DRM_PANTHOR_TIMESTAMP_CPU_NONE = 0 << 1,

for completeness.

> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW: Query CPU time using CLOCK_MONOTONIC_RAW. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW = 2 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK: Space reserved for CPU clock type. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK = 7 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_GPU_OFFSET: Query GPU offset. */
> +	DRM_PANTHOR_TIMESTAMP_GPU_OFFSET = 1 << 4,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT: Query GPU cycle count. */
> +	DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT = 1 << 5,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_FREQ: Query timestamp frequency. */
> +	DRM_PANTHOR_TIMESTAMP_FREQ = 1 << 6,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_DURATION: Return duration of time query. */
> +	DRM_PANTHOR_TIMESTAMP_DURATION = 1 << 7,

	/**
	 * @DRM_PANTHOR_TIMESTAMP_ADVANCED_QUERY: Advanced query requested.
	 *
	 * When missing, flags should be 0, which is equivalent to
	 *  DRM_PANTHOR_TIMESTAMP_ADVANCED_QUERY |
	 *  DRM_PANTHOR_TIMESTAMP_GPU |
	 *  DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
	 *  DRM_PANTHOR_TIMESTAMP_FREQ
	 */
	DRM_PANTHOR_TIMESTAMP_ADVANCED_QUERY = 1 << 30,

> +};
> +
>  /**
>   * 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.

>  };
>  
>  /**

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

[Marcin Ślusarz]

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.

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.

> 	if (args->flags & DRM_PANTHOR_TIMESTAMP_ADVANCED_QUERY)
> 		flags = args->flags;
> 	else
> 		flags = DRM_PANTHOR_TIMESTAMP_GPU |
> 			DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> 			DRM_PANTHOR_TIMESTAMP_FREQ;
> 
> > +
> > +	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> > +	case 0:
> > +		break;
> > +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> > +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> > +		timestamp_types++;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (flags & ~(DRM_PANTHOR_TIMESTAMP_GPU |
> > +		      DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK |
> > +		      DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> > +		      DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT |
> > +		      DRM_PANTHOR_TIMESTAMP_FREQ |
> > +		      DRM_PANTHOR_TIMESTAMP_DURATION))
> > +		return -EINVAL;
> > +
> > +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
> > +		timestamp_types++;
> > +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
> > +		timestamp_types++;
> > +
> > +	minimize_interruption =
> > +		(flags & DRM_PANTHOR_TIMESTAMP_DURATION) ||
> > +		(timestamp_types >= 2);
> 
> It's probably worth a comment explaining where the >= 2 comes from
> and why query duration measurement needs to be done with interrupts
> disabled (though the latter is a bit more obvious than the former).

ack

> >  
> >  	ret = panthor_device_resume_and_get(ptdev);
> >  	if (ret)
> >  		return ret;
> >  
> > +	if (flags & DRM_PANTHOR_TIMESTAMP_FREQ) {
> >  #ifdef CONFIG_ARM_ARCH_TIMER
> > -	arg->timestamp_frequency = arch_timer_get_cntfrq();
> > +		arg->timestamp_frequency = arch_timer_get_cntfrq();
> >  #else
> > -	arg->timestamp_frequency = 0;
> > +		arg->timestamp_frequency = 0;
> >  #endif
> > -	arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
> > -	arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
> > +	} else {
> > +		arg->timestamp_frequency = 0;
> > +	}
> > +
> > +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_OFFSET)
> > +		arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
> > +	else
> > +		arg->timestamp_offset = 0;
> > +
> > +	if (minimize_interruption) {
> > +		preempt_disable();
> > +		local_irq_save(irq_flags);
> > +	}
> > +
> > +	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
> > +		query_start_time = local_clock();
> > +	else
> > +		query_start_time = 0;
> > +
> > +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
> > +		arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
> > +	else
> > +		arg->current_timestamp = 0;
> > +
> > +	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> > +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> > +		ktime_get_ts64(&cpu_ts);
> > +		break;
> > +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> > +		ktime_get_raw_ts64(&cpu_ts);
> > +		break;
> 
> 	default:
> 		break;

ack

> > +	}
> > +
> > +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
> > +		arg->cycle_count = gpu_read64_counter(ptdev, GPU_CYCLE_COUNT);
> > +	else
> > +		arg->cycle_count = 0;
> > +
> > +	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
> > +		arg->duration_nsec = local_clock() - query_start_time;
> > +	else
> > +		arg->duration_nsec = 0;
> > +
> > +	if (minimize_interruption) {
> > +		local_irq_restore(irq_flags);
> > +		preempt_enable();
> > +	}
> > +
> > +	if (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> > +		timens_add_monotonic(&cpu_ts);
> > +
> > +		arg->cpu_timestamp_sec = cpu_ts.tv_sec;
> > +		arg->cpu_timestamp_nsec = cpu_ts.tv_nsec;
> > +	} else {
> > +		arg->cpu_timestamp_sec = 0;
> > +		arg->cpu_timestamp_nsec = 0;
> > +	}
> >  
> >  	pm_runtime_put(ptdev->base.dev);
> >  	return 0;
> > @@ -851,7 +955,12 @@ static int panthor_ioctl_dev_query(struct drm_device *ddev, void *data, struct d
> >  		return PANTHOR_UOBJ_SET(args->pointer, args->size, ptdev->csif_info);
> >  
> >  	case DRM_PANTHOR_DEV_QUERY_TIMESTAMP_INFO:
> > -		ret = panthor_query_timestamp_info(ptdev, &timestamp_info);
> > +		if (copy_from_user(&timestamp_info,
> > +				   (const void __user __force *) args->pointer,
> > +				   args->size))

Umm, now I see that I forgot to explicitly validate args->size. It's done
inside of copy_from_user, but it relies on __builtin_object_size doing
the right thing. Will do that in v2.

> > +			return -EFAULT;
> > +
> > +		ret = panthor_query_timestamp_info(ptdev, &timestamp_info, args->size);
> >  
> 
> nit: drop the blank line.

ack

> >  		if (ret)
> >  			return ret;
> > @@ -1680,6 +1789,7 @@ static void panthor_debugfs_init(struct drm_minor *minor)
> >   *       - adds DRM_IOCTL_PANTHOR_BO_SYNC ioctl
> >   *       - adds DRM_IOCTL_PANTHOR_BO_QUERY_INFO ioctl
> >   *       - adds drm_panthor_gpu_info::selected_coherency
> > + * - 1.8 - extends DEV_QUERY_TIMESTAMP_INFO with flags
> >   */
> >  static const struct drm_driver panthor_drm_driver = {
> >  	.driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ |
> > @@ -1693,7 +1803,7 @@ static const struct drm_driver panthor_drm_driver = {
> >  	.name = "panthor",
> >  	.desc = "Panthor DRM driver",
> >  	.major = 1,
> > -	.minor = 7,
> > +	.minor = 8,
> >  
> >  	.gem_create_object = panthor_gem_create_object,
> >  	.gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table,
> > diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> > index b401ac585d6a..c869e8b95ecd 100644
> > --- a/include/uapi/drm/panthor_drm.h
> > +++ b/include/uapi/drm/panthor_drm.h
> > @@ -409,6 +409,35 @@ struct drm_panthor_csif_info {
> >  	__u32 pad;
> >  };
> >  
> > +/**
> > + * enum drm_panthor_timestamp_info_flags - drm_panthor_timestamp_info.flags
> > + */
> > +enum drm_panthor_timestamp_info_flags {
> > +	/** @DRM_PANTHOR_TIMESTAMP_GPU: Query GPU time. */
> > +	DRM_PANTHOR_TIMESTAMP_GPU = 1 << 0,
> > +
> > +	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC: Query CPU time using CLOCK_MONOTONIC. */
> > +	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC = 1 << 1,
> 
> Can we define
> 
> 	DRM_PANTHOR_TIMESTAMP_CPU_NONE = 0 << 1,
> 
> for completeness.

Yeah, good idea.

> 
> > +
> > +	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW: Query CPU time using CLOCK_MONOTONIC_RAW. */
> > +	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW = 2 << 1,
> > +
> > +	/** @DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK: Space reserved for CPU clock type. */
> > +	DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK = 7 << 1,
> > +
> > +	/** @DRM_PANTHOR_TIMESTAMP_GPU_OFFSET: Query GPU offset. */
> > +	DRM_PANTHOR_TIMESTAMP_GPU_OFFSET = 1 << 4,
> > +
> > +	/** @DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT: Query GPU cycle count. */
> > +	DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT = 1 << 5,
> > +
> > +	/** @DRM_PANTHOR_TIMESTAMP_FREQ: Query timestamp frequency. */
> > +	DRM_PANTHOR_TIMESTAMP_FREQ = 1 << 6,
> > +
> > +	/** @DRM_PANTHOR_TIMESTAMP_DURATION: Return duration of time query. */
> > +	DRM_PANTHOR_TIMESTAMP_DURATION = 1 << 7,
> 
> 	/**
> 	 * @DRM_PANTHOR_TIMESTAMP_ADVANCED_QUERY: Advanced query requested.
> 	 *
> 	 * When missing, flags should be 0, which is equivalent to
> 	 *  DRM_PANTHOR_TIMESTAMP_ADVANCED_QUERY |
> 	 *  DRM_PANTHOR_TIMESTAMP_GPU |
> 	 *  DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> 	 *  DRM_PANTHOR_TIMESTAMP_FREQ
> 	 */
> 	DRM_PANTHOR_TIMESTAMP_ADVANCED_QUERY = 1 << 30,
> 
> > +};
> > +
> >  /**
> >   * 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?

> 
> >  };
> >  
> >  /**
> 

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

[Steven Price]

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

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

[Boris Brezillon]

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

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

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

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

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

[Marcin Ślusarz]

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?

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

[Boris Brezillon]

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

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

[Boris Brezillon]

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?

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

[Marcin Slusarz]

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>
---
Changes in v2:
 - added DRM_PANTHOR_TIMESTAMP_CPU_NONE
 - cpu_timestamp_nsec extended to u64
 - pad1 removed
 - copy_from_user -> copy_struct_from_user
 - looking at size replaced by looking at flags
 - more comments
---
 drivers/gpu/drm/panthor/panthor_drv.c | 125 ++++++++++++++++++++++++--
 include/uapi/drm/panthor_drm.h        |  58 +++++++++++-
 2 files changed, 175 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
index 165dddfde6ca..b6a100ce389d 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>
@@ -765,18 +767,123 @@ static int panthor_query_timestamp_info(struct panthor_device *ptdev,
 					struct drm_panthor_timestamp_info *arg)
 {
 	int ret;
+	u32 flags;
+	unsigned long irq_flags;
+	struct timespec64 cpu_ts;
+	u64 query_start_time;
+	bool minimize_interruption;
+	u32 timestamp_types = 0;
+
+	if (arg->flags != 0)
+		flags = arg->flags;
+	else
+		flags = DRM_PANTHOR_TIMESTAMP_GPU |
+			DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
+			DRM_PANTHOR_TIMESTAMP_FREQ;
+
+	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
+	case 0:
+		break;
+	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
+	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
+		timestamp_types++;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (flags & ~(DRM_PANTHOR_TIMESTAMP_GPU |
+		      DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK |
+		      DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
+		      DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT |
+		      DRM_PANTHOR_TIMESTAMP_FREQ |
+		      DRM_PANTHOR_TIMESTAMP_DURATION))
+		return -EINVAL;
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
+		timestamp_types++;
+	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
+		timestamp_types++;
+
+	/* If user asked to obtain timestamps from more than one source,
+	 * then it very likely means they want them to be as close as possible.
+	 * If they asked for duration, then that likely means that they
+	 * want to know how long obtaining timestamp takes, without random
+	 * events, like process scheduling or interrupts.
+	 */
+	minimize_interruption =
+		(flags & DRM_PANTHOR_TIMESTAMP_DURATION) ||
+		(timestamp_types >= 2);
 
 	ret = panthor_device_resume_and_get(ptdev);
 	if (ret)
 		return ret;
 
+	if (flags & DRM_PANTHOR_TIMESTAMP_FREQ) {
 #ifdef CONFIG_ARM_ARCH_TIMER
-	arg->timestamp_frequency = arch_timer_get_cntfrq();
+		arg->timestamp_frequency = arch_timer_get_cntfrq();
 #else
-	arg->timestamp_frequency = 0;
+		arg->timestamp_frequency = 0;
 #endif
-	arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
-	arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
+	} else {
+		arg->timestamp_frequency = 0;
+	}
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_OFFSET)
+		arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
+	else
+		arg->timestamp_offset = 0;
+
+	if (minimize_interruption) {
+		preempt_disable();
+		local_irq_save(irq_flags);
+	}
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
+		query_start_time = local_clock();
+	else
+		query_start_time = 0;
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
+		arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
+	else
+		arg->current_timestamp = 0;
+
+	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
+	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
+		ktime_get_ts64(&cpu_ts);
+		break;
+	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
+		ktime_get_raw_ts64(&cpu_ts);
+		break;
+	default:
+		break;
+	}
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
+		arg->cycle_count = gpu_read64_counter(ptdev, GPU_CYCLE_COUNT);
+	else
+		arg->cycle_count = 0;
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
+		arg->duration_nsec = local_clock() - query_start_time;
+	else
+		arg->duration_nsec = 0;
+
+	if (minimize_interruption) {
+		local_irq_restore(irq_flags);
+		preempt_enable();
+	}
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
+		timens_add_monotonic(&cpu_ts);
+
+		arg->cpu_timestamp_sec = cpu_ts.tv_sec;
+		arg->cpu_timestamp_nsec = cpu_ts.tv_nsec;
+	} else {
+		arg->cpu_timestamp_sec = 0;
+		arg->cpu_timestamp_nsec = 0;
+	}
 
 	pm_runtime_put(ptdev->base.dev);
 	return 0;
@@ -851,8 +958,13 @@ static int panthor_ioctl_dev_query(struct drm_device *ddev, void *data, struct d
 		return PANTHOR_UOBJ_SET(args->pointer, args->size, ptdev->csif_info);
 
 	case DRM_PANTHOR_DEV_QUERY_TIMESTAMP_INFO:
-		ret = panthor_query_timestamp_info(ptdev, &timestamp_info);
+		ret = copy_struct_from_user(&timestamp_info, sizeof(timestamp_info),
+					    (const void __user __force *) args->pointer,
+					    args->size);
+		if (ret)
+			return ret;
 
+		ret = panthor_query_timestamp_info(ptdev, &timestamp_info);
 		if (ret)
 			return ret;
 
@@ -1680,6 +1792,7 @@ static void panthor_debugfs_init(struct drm_minor *minor)
  *       - adds DRM_IOCTL_PANTHOR_BO_SYNC ioctl
  *       - adds DRM_IOCTL_PANTHOR_BO_QUERY_INFO ioctl
  *       - adds drm_panthor_gpu_info::selected_coherency
+ * - 1.8 - extends DEV_QUERY_TIMESTAMP_INFO with flags
  */
 static const struct drm_driver panthor_drm_driver = {
 	.driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ |
@@ -1693,7 +1806,7 @@ static const struct drm_driver panthor_drm_driver = {
 	.name = "panthor",
 	.desc = "Panthor DRM driver",
 	.major = 1,
-	.minor = 7,
+	.minor = 8,
 
 	.gem_create_object = panthor_gem_create_object,
 	.gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table,
diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
index b401ac585d6a..8a46ef040c3d 100644
--- a/include/uapi/drm/panthor_drm.h
+++ b/include/uapi/drm/panthor_drm.h
@@ -409,6 +409,38 @@ struct drm_panthor_csif_info {
 	__u32 pad;
 };
 
+/**
+ * enum drm_panthor_timestamp_info_flags - drm_panthor_timestamp_info.flags
+ */
+enum drm_panthor_timestamp_info_flags {
+	/** @DRM_PANTHOR_TIMESTAMP_GPU: Query GPU time. */
+	DRM_PANTHOR_TIMESTAMP_GPU = 1 << 0,
+
+	/** @DRM_PANTHOR_TIMESTAMP_CPU_NONE: Don't query CPU time. */
+	DRM_PANTHOR_TIMESTAMP_CPU_NONE = 0 << 1,
+
+	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC: Query CPU time using CLOCK_MONOTONIC. */
+	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC = 1 << 1,
+
+	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW: Query CPU time using CLOCK_MONOTONIC_RAW. */
+	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW = 2 << 1,
+
+	/** @DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK: Space reserved for CPU clock type. */
+	DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK = 7 << 1,
+
+	/** @DRM_PANTHOR_TIMESTAMP_GPU_OFFSET: Query GPU offset. */
+	DRM_PANTHOR_TIMESTAMP_GPU_OFFSET = 1 << 4,
+
+	/** @DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT: Query GPU cycle count. */
+	DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT = 1 << 5,
+
+	/** @DRM_PANTHOR_TIMESTAMP_FREQ: Query timestamp frequency. */
+	DRM_PANTHOR_TIMESTAMP_FREQ = 1 << 6,
+
+	/** @DRM_PANTHOR_TIMESTAMP_DURATION: Return duration of time query. */
+	DRM_PANTHOR_TIMESTAMP_DURATION = 1 << 7,
+};
+
 /**
  * struct drm_panthor_timestamp_info - Timestamp information
  *
@@ -421,11 +453,33 @@ 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.
+	 *
+	 * If set to 0, then it is interpreted as:
+	 *  DRM_PANTHOR_TIMESTAMP_GPU |
+	 *  DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
+	 *  DRM_PANTHOR_TIMESTAMP_FREQ
+	 */
+	__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. */
+	__u64 cpu_timestamp_nsec;
 };
 
 /**
-- 
2.34.1

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

[Boris Brezillon]

On Thu, 19 Mar 2026 09:25:51 +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>
> ---
> Changes in v2:
>  - added DRM_PANTHOR_TIMESTAMP_CPU_NONE
>  - cpu_timestamp_nsec extended to u64
>  - pad1 removed
>  - copy_from_user -> copy_struct_from_user
>  - looking at size replaced by looking at flags
>  - more comments

Looks good to me, just a few minor things, but once addressed, this is

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/gpu/drm/panthor/panthor_drv.c | 125 ++++++++++++++++++++++++--
>  include/uapi/drm/panthor_drm.h        |  58 +++++++++++-
>  2 files changed, 175 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
> index 165dddfde6ca..b6a100ce389d 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>
> @@ -765,18 +767,123 @@ static int panthor_query_timestamp_info(struct panthor_device *ptdev,
>  					struct drm_panthor_timestamp_info *arg)
>  {
>  	int ret;
> +	u32 flags;
> +	unsigned long irq_flags;
> +	struct timespec64 cpu_ts;
> +	u64 query_start_time;
> +	bool minimize_interruption;
> +	u32 timestamp_types = 0;
> +
> +	if (arg->flags != 0)
> +		flags = arg->flags;
> +	else
> +		flags = DRM_PANTHOR_TIMESTAMP_GPU |
> +			DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> +			DRM_PANTHOR_TIMESTAMP_FREQ;

nit: I like to add explicit {} for multi-line statements under
conditional blocks.

	if (arg->flags != 0) {
		flags = arg->flags;
	} else {
		flags = DRM_PANTHOR_TIMESTAMP_GPU |
			DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
			DRM_PANTHOR_TIMESTAMP_FREQ;
	}

and maybe add a comment to repeat what the uAPI doc says.

> +
> +	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> +	case 0:
> +		break;
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> +		timestamp_types++;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (flags & ~(DRM_PANTHOR_TIMESTAMP_GPU |
> +		      DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK |
> +		      DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> +		      DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT |
> +		      DRM_PANTHOR_TIMESTAMP_FREQ |
> +		      DRM_PANTHOR_TIMESTAMP_DURATION))

Let's define a VALID_TIMESTAMP_QUERY_FLAGS like we do for other ioctls.

> +		return -EINVAL;
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
> +		timestamp_types++;
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
> +		timestamp_types++;
> +
> +	/* If user asked to obtain timestamps from more than one source,
> +	 * then it very likely means they want them to be as close as possible.
> +	 * If they asked for duration, then that likely means that they
> +	 * want to know how long obtaining timestamp takes, without random
> +	 * events, like process scheduling or interrupts.
> +	 */
> +	minimize_interruption =
> +		(flags & DRM_PANTHOR_TIMESTAMP_DURATION) ||
> +		(timestamp_types >= 2);
>  
>  	ret = panthor_device_resume_and_get(ptdev);
>  	if (ret)
>  		return ret;
>  
> +	if (flags & DRM_PANTHOR_TIMESTAMP_FREQ) {
>  #ifdef CONFIG_ARM_ARCH_TIMER
> -	arg->timestamp_frequency = arch_timer_get_cntfrq();
> +		arg->timestamp_frequency = arch_timer_get_cntfrq();
>  #else
> -	arg->timestamp_frequency = 0;
> +		arg->timestamp_frequency = 0;
>  #endif
> -	arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
> -	arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
> +	} else {
> +		arg->timestamp_frequency = 0;
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_OFFSET)
> +		arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
> +	else
> +		arg->timestamp_offset = 0;
> +
> +	if (minimize_interruption) {
> +		preempt_disable();
> +		local_irq_save(irq_flags);
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
> +		query_start_time = local_clock();
> +	else
> +		query_start_time = 0;
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
> +		arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
> +	else
> +		arg->current_timestamp = 0;
> +
> +	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> +		ktime_get_ts64(&cpu_ts);
> +		break;
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> +		ktime_get_raw_ts64(&cpu_ts);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
> +		arg->cycle_count = gpu_read64_counter(ptdev, GPU_CYCLE_COUNT);
> +	else
> +		arg->cycle_count = 0;
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
> +		arg->duration_nsec = local_clock() - query_start_time;
> +	else
> +		arg->duration_nsec = 0;
> +
> +	if (minimize_interruption) {
> +		local_irq_restore(irq_flags);
> +		preempt_enable();
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> +		timens_add_monotonic(&cpu_ts);
> +
> +		arg->cpu_timestamp_sec = cpu_ts.tv_sec;
> +		arg->cpu_timestamp_nsec = cpu_ts.tv_nsec;
> +	} else {
> +		arg->cpu_timestamp_sec = 0;
> +		arg->cpu_timestamp_nsec = 0;
> +	}
>  
>  	pm_runtime_put(ptdev->base.dev);
>  	return 0;
> @@ -851,8 +958,13 @@ static int panthor_ioctl_dev_query(struct drm_device *ddev, void *data, struct d
>  		return PANTHOR_UOBJ_SET(args->pointer, args->size, ptdev->csif_info);
>  
>  	case DRM_PANTHOR_DEV_QUERY_TIMESTAMP_INFO:
> -		ret = panthor_query_timestamp_info(ptdev, &timestamp_info);
> +		ret = copy_struct_from_user(&timestamp_info, sizeof(timestamp_info),
> +					    (const void __user __force *) args->pointer,

					    u64_to_user_ptr(args->pointer),

> +					    args->size);

I wonder if we should have a PANTHOR_UOBJ_GET() that checks the
input size against the minimum struct size ever exposed by the driver.
Here we are covered by the following PANTHOR_UOBJ_SET() so that's
probably not a huge issue, dunno.

> +		if (ret)
> +			return ret;
>  
> +		ret = panthor_query_timestamp_info(ptdev, &timestamp_info);
>  		if (ret)
>  			return ret;
>  
> @@ -1680,6 +1792,7 @@ static void panthor_debugfs_init(struct drm_minor *minor)
>   *       - adds DRM_IOCTL_PANTHOR_BO_SYNC ioctl
>   *       - adds DRM_IOCTL_PANTHOR_BO_QUERY_INFO ioctl
>   *       - adds drm_panthor_gpu_info::selected_coherency
> + * - 1.8 - extends DEV_QUERY_TIMESTAMP_INFO with flags
>   */
>  static const struct drm_driver panthor_drm_driver = {
>  	.driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ |
> @@ -1693,7 +1806,7 @@ static const struct drm_driver panthor_drm_driver = {
>  	.name = "panthor",
>  	.desc = "Panthor DRM driver",
>  	.major = 1,
> -	.minor = 7,
> +	.minor = 8,
>  
>  	.gem_create_object = panthor_gem_create_object,
>  	.gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table,
> diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> index b401ac585d6a..8a46ef040c3d 100644
> --- a/include/uapi/drm/panthor_drm.h
> +++ b/include/uapi/drm/panthor_drm.h
> @@ -409,6 +409,38 @@ struct drm_panthor_csif_info {
>  	__u32 pad;
>  };
>  
> +/**
> + * enum drm_panthor_timestamp_info_flags - drm_panthor_timestamp_info.flags
> + */
> +enum drm_panthor_timestamp_info_flags {
> +	/** @DRM_PANTHOR_TIMESTAMP_GPU: Query GPU time. */
> +	DRM_PANTHOR_TIMESTAMP_GPU = 1 << 0,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_NONE: Don't query CPU time. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_NONE = 0 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC: Query CPU time using CLOCK_MONOTONIC. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC = 1 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW: Query CPU time using CLOCK_MONOTONIC_RAW. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW = 2 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK: Space reserved for CPU clock type. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK = 7 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_GPU_OFFSET: Query GPU offset. */
> +	DRM_PANTHOR_TIMESTAMP_GPU_OFFSET = 1 << 4,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT: Query GPU cycle count. */
> +	DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT = 1 << 5,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_FREQ: Query timestamp frequency. */
> +	DRM_PANTHOR_TIMESTAMP_FREQ = 1 << 6,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_DURATION: Return duration of time query. */
> +	DRM_PANTHOR_TIMESTAMP_DURATION = 1 << 7,
> +};
> +
>  /**
>   * struct drm_panthor_timestamp_info - Timestamp information
>   *
> @@ -421,11 +453,33 @@ 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.
> +	 *
> +	 * If set to 0, then it is interpreted as:
> +	 *  DRM_PANTHOR_TIMESTAMP_GPU |
> +	 *  DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> +	 *  DRM_PANTHOR_TIMESTAMP_FREQ
> +	 */
> +	__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. */
> +	__u64 cpu_timestamp_nsec;
>  };
>  
>  /**

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

[Marcin Slusarz]

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>
---
Changes in v3:
 - {} around multiline statements
 - added comment about default flags
 - added VALID_TIMESTAMP_QUERY_FLAGS
 - used u64_to_user_ptr
Changes in v2:
 - added DRM_PANTHOR_TIMESTAMP_CPU_NONE
 - cpu_timestamp_nsec extended to u64
 - pad1 removed
 - copy_from_user -> copy_struct_from_user
 - looking at size replaced by looking at flags
 - more comments
---
 drivers/gpu/drm/panthor/panthor_drv.c | 134 ++++++++++++++++++++++++--
 include/uapi/drm/panthor_drm.h        |  58 ++++++++++-
 2 files changed, 184 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
index 165dddfde6ca..029ef3295b06 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>
@@ -761,22 +763,135 @@ static void panthor_submit_ctx_cleanup(struct panthor_submit_ctx *ctx,
 	kvfree(ctx->jobs);
 }
 
+#define VALID_TIMESTAMP_QUERY_FLAGS \
+		(DRM_PANTHOR_TIMESTAMP_GPU | \
+		 DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK | \
+		 DRM_PANTHOR_TIMESTAMP_GPU_OFFSET | \
+		 DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT | \
+		 DRM_PANTHOR_TIMESTAMP_FREQ | \
+		 DRM_PANTHOR_TIMESTAMP_DURATION)
+
 static int panthor_query_timestamp_info(struct panthor_device *ptdev,
 					struct drm_panthor_timestamp_info *arg)
 {
 	int ret;
+	u32 flags;
+	unsigned long irq_flags;
+	struct timespec64 cpu_ts;
+	u64 query_start_time;
+	bool minimize_interruption;
+	u32 timestamp_types = 0;
+
+	if (arg->flags != 0) {
+		flags = arg->flags;
+	} else {
+		/*
+		 * If flags are 0, then ask for the same things that we asked
+		 * for before flags were added.
+		 */
+		flags = DRM_PANTHOR_TIMESTAMP_GPU |
+			DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
+			DRM_PANTHOR_TIMESTAMP_FREQ;
+	}
+
+	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
+	case 0:
+		break;
+	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
+	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
+		timestamp_types++;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (flags & ~VALID_TIMESTAMP_QUERY_FLAGS)
+		return -EINVAL;
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
+		timestamp_types++;
+	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
+		timestamp_types++;
+
+	/* If user asked to obtain timestamps from more than one source,
+	 * then it very likely means they want them to be as close as possible.
+	 * If they asked for duration, then that likely means that they
+	 * want to know how long obtaining timestamp takes, without random
+	 * events, like process scheduling or interrupts.
+	 */
+	minimize_interruption =
+		(flags & DRM_PANTHOR_TIMESTAMP_DURATION) ||
+		(timestamp_types >= 2);
 
 	ret = panthor_device_resume_and_get(ptdev);
 	if (ret)
 		return ret;
 
+	if (flags & DRM_PANTHOR_TIMESTAMP_FREQ) {
 #ifdef CONFIG_ARM_ARCH_TIMER
-	arg->timestamp_frequency = arch_timer_get_cntfrq();
+		arg->timestamp_frequency = arch_timer_get_cntfrq();
 #else
-	arg->timestamp_frequency = 0;
+		arg->timestamp_frequency = 0;
 #endif
-	arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
-	arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
+	} else {
+		arg->timestamp_frequency = 0;
+	}
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_OFFSET)
+		arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
+	else
+		arg->timestamp_offset = 0;
+
+	if (minimize_interruption) {
+		preempt_disable();
+		local_irq_save(irq_flags);
+	}
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
+		query_start_time = local_clock();
+	else
+		query_start_time = 0;
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
+		arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
+	else
+		arg->current_timestamp = 0;
+
+	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
+	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
+		ktime_get_ts64(&cpu_ts);
+		break;
+	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
+		ktime_get_raw_ts64(&cpu_ts);
+		break;
+	default:
+		break;
+	}
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
+		arg->cycle_count = gpu_read64_counter(ptdev, GPU_CYCLE_COUNT);
+	else
+		arg->cycle_count = 0;
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
+		arg->duration_nsec = local_clock() - query_start_time;
+	else
+		arg->duration_nsec = 0;
+
+	if (minimize_interruption) {
+		local_irq_restore(irq_flags);
+		preempt_enable();
+	}
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
+		timens_add_monotonic(&cpu_ts);
+
+		arg->cpu_timestamp_sec = cpu_ts.tv_sec;
+		arg->cpu_timestamp_nsec = cpu_ts.tv_nsec;
+	} else {
+		arg->cpu_timestamp_sec = 0;
+		arg->cpu_timestamp_nsec = 0;
+	}
 
 	pm_runtime_put(ptdev->base.dev);
 	return 0;
@@ -851,8 +966,14 @@ static int panthor_ioctl_dev_query(struct drm_device *ddev, void *data, struct d
 		return PANTHOR_UOBJ_SET(args->pointer, args->size, ptdev->csif_info);
 
 	case DRM_PANTHOR_DEV_QUERY_TIMESTAMP_INFO:
-		ret = panthor_query_timestamp_info(ptdev, &timestamp_info);
+		ret = copy_struct_from_user(&timestamp_info,
+					    sizeof(timestamp_info),
+					    u64_to_user_ptr(args->pointer),
+					    args->size);
+		if (ret)
+			return ret;
 
+		ret = panthor_query_timestamp_info(ptdev, &timestamp_info);
 		if (ret)
 			return ret;
 
@@ -1680,6 +1801,7 @@ static void panthor_debugfs_init(struct drm_minor *minor)
  *       - adds DRM_IOCTL_PANTHOR_BO_SYNC ioctl
  *       - adds DRM_IOCTL_PANTHOR_BO_QUERY_INFO ioctl
  *       - adds drm_panthor_gpu_info::selected_coherency
+ * - 1.8 - extends DEV_QUERY_TIMESTAMP_INFO with flags
  */
 static const struct drm_driver panthor_drm_driver = {
 	.driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ |
@@ -1693,7 +1815,7 @@ static const struct drm_driver panthor_drm_driver = {
 	.name = "panthor",
 	.desc = "Panthor DRM driver",
 	.major = 1,
-	.minor = 7,
+	.minor = 8,
 
 	.gem_create_object = panthor_gem_create_object,
 	.gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table,
diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
index b401ac585d6a..8a46ef040c3d 100644
--- a/include/uapi/drm/panthor_drm.h
+++ b/include/uapi/drm/panthor_drm.h
@@ -409,6 +409,38 @@ struct drm_panthor_csif_info {
 	__u32 pad;
 };
 
+/**
+ * enum drm_panthor_timestamp_info_flags - drm_panthor_timestamp_info.flags
+ */
+enum drm_panthor_timestamp_info_flags {
+	/** @DRM_PANTHOR_TIMESTAMP_GPU: Query GPU time. */
+	DRM_PANTHOR_TIMESTAMP_GPU = 1 << 0,
+
+	/** @DRM_PANTHOR_TIMESTAMP_CPU_NONE: Don't query CPU time. */
+	DRM_PANTHOR_TIMESTAMP_CPU_NONE = 0 << 1,
+
+	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC: Query CPU time using CLOCK_MONOTONIC. */
+	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC = 1 << 1,
+
+	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW: Query CPU time using CLOCK_MONOTONIC_RAW. */
+	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW = 2 << 1,
+
+	/** @DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK: Space reserved for CPU clock type. */
+	DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK = 7 << 1,
+
+	/** @DRM_PANTHOR_TIMESTAMP_GPU_OFFSET: Query GPU offset. */
+	DRM_PANTHOR_TIMESTAMP_GPU_OFFSET = 1 << 4,
+
+	/** @DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT: Query GPU cycle count. */
+	DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT = 1 << 5,
+
+	/** @DRM_PANTHOR_TIMESTAMP_FREQ: Query timestamp frequency. */
+	DRM_PANTHOR_TIMESTAMP_FREQ = 1 << 6,
+
+	/** @DRM_PANTHOR_TIMESTAMP_DURATION: Return duration of time query. */
+	DRM_PANTHOR_TIMESTAMP_DURATION = 1 << 7,
+};
+
 /**
  * struct drm_panthor_timestamp_info - Timestamp information
  *
@@ -421,11 +453,33 @@ 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.
+	 *
+	 * If set to 0, then it is interpreted as:
+	 *  DRM_PANTHOR_TIMESTAMP_GPU |
+	 *  DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
+	 *  DRM_PANTHOR_TIMESTAMP_FREQ
+	 */
+	__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. */
+	__u64 cpu_timestamp_nsec;
 };
 
 /**
-- 
2.34.1

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

[Boris Brezillon]

On Thu, 19 Mar 2026 12:00:53 +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>

Still

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
> Changes in v3:
>  - {} around multiline statements
>  - added comment about default flags
>  - added VALID_TIMESTAMP_QUERY_FLAGS
>  - used u64_to_user_ptr
> Changes in v2:
>  - added DRM_PANTHOR_TIMESTAMP_CPU_NONE
>  - cpu_timestamp_nsec extended to u64
>  - pad1 removed
>  - copy_from_user -> copy_struct_from_user
>  - looking at size replaced by looking at flags
>  - more comments
> ---
>  drivers/gpu/drm/panthor/panthor_drv.c | 134 ++++++++++++++++++++++++--
>  include/uapi/drm/panthor_drm.h        |  58 ++++++++++-
>  2 files changed, 184 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
> index 165dddfde6ca..029ef3295b06 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>
> @@ -761,22 +763,135 @@ static void panthor_submit_ctx_cleanup(struct panthor_submit_ctx *ctx,
>  	kvfree(ctx->jobs);
>  }
>  
> +#define VALID_TIMESTAMP_QUERY_FLAGS \
> +		(DRM_PANTHOR_TIMESTAMP_GPU | \
> +		 DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK | \
> +		 DRM_PANTHOR_TIMESTAMP_GPU_OFFSET | \
> +		 DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT | \
> +		 DRM_PANTHOR_TIMESTAMP_FREQ | \
> +		 DRM_PANTHOR_TIMESTAMP_DURATION)
> +
>  static int panthor_query_timestamp_info(struct panthor_device *ptdev,
>  					struct drm_panthor_timestamp_info *arg)
>  {
>  	int ret;
> +	u32 flags;
> +	unsigned long irq_flags;
> +	struct timespec64 cpu_ts;
> +	u64 query_start_time;
> +	bool minimize_interruption;
> +	u32 timestamp_types = 0;
> +
> +	if (arg->flags != 0) {
> +		flags = arg->flags;
> +	} else {
> +		/*
> +		 * If flags are 0, then ask for the same things that we asked
> +		 * for before flags were added.
> +		 */
> +		flags = DRM_PANTHOR_TIMESTAMP_GPU |
> +			DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> +			DRM_PANTHOR_TIMESTAMP_FREQ;
> +	}
> +
> +	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> +	case 0:
> +		break;
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> +		timestamp_types++;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (flags & ~VALID_TIMESTAMP_QUERY_FLAGS)
> +		return -EINVAL;
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
> +		timestamp_types++;
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
> +		timestamp_types++;
> +
> +	/* If user asked to obtain timestamps from more than one source,
> +	 * then it very likely means they want them to be as close as possible.
> +	 * If they asked for duration, then that likely means that they
> +	 * want to know how long obtaining timestamp takes, without random
> +	 * events, like process scheduling or interrupts.
> +	 */
> +	minimize_interruption =
> +		(flags & DRM_PANTHOR_TIMESTAMP_DURATION) ||
> +		(timestamp_types >= 2);
>  
>  	ret = panthor_device_resume_and_get(ptdev);
>  	if (ret)
>  		return ret;
>  
> +	if (flags & DRM_PANTHOR_TIMESTAMP_FREQ) {
>  #ifdef CONFIG_ARM_ARCH_TIMER
> -	arg->timestamp_frequency = arch_timer_get_cntfrq();
> +		arg->timestamp_frequency = arch_timer_get_cntfrq();
>  #else
> -	arg->timestamp_frequency = 0;
> +		arg->timestamp_frequency = 0;
>  #endif
> -	arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
> -	arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
> +	} else {
> +		arg->timestamp_frequency = 0;
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_OFFSET)
> +		arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
> +	else
> +		arg->timestamp_offset = 0;
> +
> +	if (minimize_interruption) {
> +		preempt_disable();
> +		local_irq_save(irq_flags);
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
> +		query_start_time = local_clock();
> +	else
> +		query_start_time = 0;
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
> +		arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
> +	else
> +		arg->current_timestamp = 0;
> +
> +	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> +		ktime_get_ts64(&cpu_ts);
> +		break;
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> +		ktime_get_raw_ts64(&cpu_ts);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
> +		arg->cycle_count = gpu_read64_counter(ptdev, GPU_CYCLE_COUNT);
> +	else
> +		arg->cycle_count = 0;
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
> +		arg->duration_nsec = local_clock() - query_start_time;
> +	else
> +		arg->duration_nsec = 0;
> +
> +	if (minimize_interruption) {
> +		local_irq_restore(irq_flags);
> +		preempt_enable();
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> +		timens_add_monotonic(&cpu_ts);
> +
> +		arg->cpu_timestamp_sec = cpu_ts.tv_sec;
> +		arg->cpu_timestamp_nsec = cpu_ts.tv_nsec;
> +	} else {
> +		arg->cpu_timestamp_sec = 0;
> +		arg->cpu_timestamp_nsec = 0;
> +	}
>  
>  	pm_runtime_put(ptdev->base.dev);
>  	return 0;
> @@ -851,8 +966,14 @@ static int panthor_ioctl_dev_query(struct drm_device *ddev, void *data, struct d
>  		return PANTHOR_UOBJ_SET(args->pointer, args->size, ptdev->csif_info);
>  
>  	case DRM_PANTHOR_DEV_QUERY_TIMESTAMP_INFO:
> -		ret = panthor_query_timestamp_info(ptdev, &timestamp_info);
> +		ret = copy_struct_from_user(&timestamp_info,
> +					    sizeof(timestamp_info),
> +					    u64_to_user_ptr(args->pointer),
> +					    args->size);
> +		if (ret)
> +			return ret;
>  
> +		ret = panthor_query_timestamp_info(ptdev, &timestamp_info);
>  		if (ret)
>  			return ret;
>  
> @@ -1680,6 +1801,7 @@ static void panthor_debugfs_init(struct drm_minor *minor)
>   *       - adds DRM_IOCTL_PANTHOR_BO_SYNC ioctl
>   *       - adds DRM_IOCTL_PANTHOR_BO_QUERY_INFO ioctl
>   *       - adds drm_panthor_gpu_info::selected_coherency
> + * - 1.8 - extends DEV_QUERY_TIMESTAMP_INFO with flags
>   */
>  static const struct drm_driver panthor_drm_driver = {
>  	.driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ |
> @@ -1693,7 +1815,7 @@ static const struct drm_driver panthor_drm_driver = {
>  	.name = "panthor",
>  	.desc = "Panthor DRM driver",
>  	.major = 1,
> -	.minor = 7,
> +	.minor = 8,
>  
>  	.gem_create_object = panthor_gem_create_object,
>  	.gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table,
> diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> index b401ac585d6a..8a46ef040c3d 100644
> --- a/include/uapi/drm/panthor_drm.h
> +++ b/include/uapi/drm/panthor_drm.h
> @@ -409,6 +409,38 @@ struct drm_panthor_csif_info {
>  	__u32 pad;
>  };
>  
> +/**
> + * enum drm_panthor_timestamp_info_flags - drm_panthor_timestamp_info.flags
> + */
> +enum drm_panthor_timestamp_info_flags {
> +	/** @DRM_PANTHOR_TIMESTAMP_GPU: Query GPU time. */
> +	DRM_PANTHOR_TIMESTAMP_GPU = 1 << 0,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_NONE: Don't query CPU time. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_NONE = 0 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC: Query CPU time using CLOCK_MONOTONIC. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC = 1 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW: Query CPU time using CLOCK_MONOTONIC_RAW. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW = 2 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK: Space reserved for CPU clock type. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK = 7 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_GPU_OFFSET: Query GPU offset. */
> +	DRM_PANTHOR_TIMESTAMP_GPU_OFFSET = 1 << 4,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT: Query GPU cycle count. */
> +	DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT = 1 << 5,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_FREQ: Query timestamp frequency. */
> +	DRM_PANTHOR_TIMESTAMP_FREQ = 1 << 6,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_DURATION: Return duration of time query. */
> +	DRM_PANTHOR_TIMESTAMP_DURATION = 1 << 7,
> +};
> +
>  /**
>   * struct drm_panthor_timestamp_info - Timestamp information
>   *
> @@ -421,11 +453,33 @@ 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.
> +	 *
> +	 * If set to 0, then it is interpreted as:
> +	 *  DRM_PANTHOR_TIMESTAMP_GPU |
> +	 *  DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> +	 *  DRM_PANTHOR_TIMESTAMP_FREQ
> +	 */
> +	__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. */
> +	__u64 cpu_timestamp_nsec;
>  };
>  
>  /**

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

[Liviu Dudau]

Hi Marcin,

On Thu, Mar 19, 2026 at 12:00:53PM +0100, Marcin Slusarz 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>
> ---
> Changes in v3:
>  - {} around multiline statements
>  - added comment about default flags
>  - added VALID_TIMESTAMP_QUERY_FLAGS
>  - used u64_to_user_ptr
> Changes in v2:
>  - added DRM_PANTHOR_TIMESTAMP_CPU_NONE
>  - cpu_timestamp_nsec extended to u64
>  - pad1 removed
>  - copy_from_user -> copy_struct_from_user
>  - looking at size replaced by looking at flags
>  - more comments
> ---
>  drivers/gpu/drm/panthor/panthor_drv.c | 134 ++++++++++++++++++++++++--
>  include/uapi/drm/panthor_drm.h        |  58 ++++++++++-
>  2 files changed, 184 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
> index 165dddfde6ca..029ef3295b06 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>
> @@ -761,22 +763,135 @@ static void panthor_submit_ctx_cleanup(struct panthor_submit_ctx *ctx,
>  	kvfree(ctx->jobs);
>  }
>  
> +#define VALID_TIMESTAMP_QUERY_FLAGS \
> +		(DRM_PANTHOR_TIMESTAMP_GPU | \
> +		 DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK | \
> +		 DRM_PANTHOR_TIMESTAMP_GPU_OFFSET | \
> +		 DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT | \
> +		 DRM_PANTHOR_TIMESTAMP_FREQ | \
> +		 DRM_PANTHOR_TIMESTAMP_DURATION)
> +
>  static int panthor_query_timestamp_info(struct panthor_device *ptdev,
>  					struct drm_panthor_timestamp_info *arg)
>  {
>  	int ret;
> +	u32 flags;
> +	unsigned long irq_flags;
> +	struct timespec64 cpu_ts;
> +	u64 query_start_time;
> +	bool minimize_interruption;
> +	u32 timestamp_types = 0;
> +
> +	if (arg->flags != 0) {
> +		flags = arg->flags;
> +	} else {
> +		/*
> +		 * If flags are 0, then ask for the same things that we asked
> +		 * for before flags were added.
> +		 */
> +		flags = DRM_PANTHOR_TIMESTAMP_GPU |
> +			DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> +			DRM_PANTHOR_TIMESTAMP_FREQ;
> +	}
> +
> +	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> +	case 0:
> +		break;
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> +		timestamp_types++;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (flags & ~VALID_TIMESTAMP_QUERY_FLAGS)
> +		return -EINVAL;

Can we move this check before the switch and simplify the switch itself to only do the timestamp_types increment?

> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
> +		timestamp_types++;
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
> +		timestamp_types++;
> +
> +	/* If user asked to obtain timestamps from more than one source,
> +	 * then it very likely means they want them to be as close as possible.
> +	 * If they asked for duration, then that likely means that they
> +	 * want to know how long obtaining timestamp takes, without random
> +	 * events, like process scheduling or interrupts.
> +	 */

This comment makes me think that user can ask for both CPU_MONOTONIC and
CPU_MONOTONIC_RAW timestamps, but the code is built to make them exclusive.
Can we document better what sources can be requested simultaneously?

> +	minimize_interruption =
> +		(flags & DRM_PANTHOR_TIMESTAMP_DURATION) ||
> +		(timestamp_types >= 2);
>  
>  	ret = panthor_device_resume_and_get(ptdev);
>  	if (ret)
>  		return ret;
>  
> +	if (flags & DRM_PANTHOR_TIMESTAMP_FREQ) {
>  #ifdef CONFIG_ARM_ARCH_TIMER
> -	arg->timestamp_frequency = arch_timer_get_cntfrq();
> +		arg->timestamp_frequency = arch_timer_get_cntfrq();
>  #else
> -	arg->timestamp_frequency = 0;
> +		arg->timestamp_frequency = 0;
>  #endif
> -	arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
> -	arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
> +	} else {
> +		arg->timestamp_frequency = 0;
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_OFFSET)
> +		arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
> +	else
> +		arg->timestamp_offset = 0;
> +
> +	if (minimize_interruption) {
> +		preempt_disable();
> +		local_irq_save(irq_flags);
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
> +		query_start_time = local_clock();
> +	else
> +		query_start_time = 0;
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
> +		arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
> +	else
> +		arg->current_timestamp = 0;
> +
> +	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> +		ktime_get_ts64(&cpu_ts);
> +		break;
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> +		ktime_get_raw_ts64(&cpu_ts);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
> +		arg->cycle_count = gpu_read64_counter(ptdev, GPU_CYCLE_COUNT);
> +	else
> +		arg->cycle_count = 0;
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
> +		arg->duration_nsec = local_clock() - query_start_time;
> +	else
> +		arg->duration_nsec = 0;
> +
> +	if (minimize_interruption) {
> +		local_irq_restore(irq_flags);
> +		preempt_enable();
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> +		timens_add_monotonic(&cpu_ts);
> +
> +		arg->cpu_timestamp_sec = cpu_ts.tv_sec;
> +		arg->cpu_timestamp_nsec = cpu_ts.tv_nsec;
> +	} else {
> +		arg->cpu_timestamp_sec = 0;
> +		arg->cpu_timestamp_nsec = 0;
> +	}
>  
>  	pm_runtime_put(ptdev->base.dev);
>  	return 0;
> @@ -851,8 +966,14 @@ static int panthor_ioctl_dev_query(struct drm_device *ddev, void *data, struct d
>  		return PANTHOR_UOBJ_SET(args->pointer, args->size, ptdev->csif_info);
>  
>  	case DRM_PANTHOR_DEV_QUERY_TIMESTAMP_INFO:
> -		ret = panthor_query_timestamp_info(ptdev, &timestamp_info);
> +		ret = copy_struct_from_user(&timestamp_info,
> +					    sizeof(timestamp_info),
> +					    u64_to_user_ptr(args->pointer),
> +					    args->size);
> +		if (ret)
> +			return ret;
>  
> +		ret = panthor_query_timestamp_info(ptdev, &timestamp_info);
>  		if (ret)
>  			return ret;
>  
> @@ -1680,6 +1801,7 @@ static void panthor_debugfs_init(struct drm_minor *minor)
>   *       - adds DRM_IOCTL_PANTHOR_BO_SYNC ioctl
>   *       - adds DRM_IOCTL_PANTHOR_BO_QUERY_INFO ioctl
>   *       - adds drm_panthor_gpu_info::selected_coherency
> + * - 1.8 - extends DEV_QUERY_TIMESTAMP_INFO with flags
>   */
>  static const struct drm_driver panthor_drm_driver = {
>  	.driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ |
> @@ -1693,7 +1815,7 @@ static const struct drm_driver panthor_drm_driver = {
>  	.name = "panthor",
>  	.desc = "Panthor DRM driver",
>  	.major = 1,
> -	.minor = 7,
> +	.minor = 8,
>  
>  	.gem_create_object = panthor_gem_create_object,
>  	.gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table,
> diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> index b401ac585d6a..8a46ef040c3d 100644
> --- a/include/uapi/drm/panthor_drm.h
> +++ b/include/uapi/drm/panthor_drm.h
> @@ -409,6 +409,38 @@ struct drm_panthor_csif_info {
>  	__u32 pad;
>  };
>  
> +/**
> + * enum drm_panthor_timestamp_info_flags - drm_panthor_timestamp_info.flags
> + */
> +enum drm_panthor_timestamp_info_flags {
> +	/** @DRM_PANTHOR_TIMESTAMP_GPU: Query GPU time. */
> +	DRM_PANTHOR_TIMESTAMP_GPU = 1 << 0,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_NONE: Don't query CPU time. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_NONE = 0 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC: Query CPU time using CLOCK_MONOTONIC. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC = 1 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW: Query CPU time using CLOCK_MONOTONIC_RAW. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW = 2 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK: Space reserved for CPU clock type. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK = 7 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_GPU_OFFSET: Query GPU offset. */
> +	DRM_PANTHOR_TIMESTAMP_GPU_OFFSET = 1 << 4,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT: Query GPU cycle count. */
> +	DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT = 1 << 5,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_FREQ: Query timestamp frequency. */
> +	DRM_PANTHOR_TIMESTAMP_FREQ = 1 << 6,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_DURATION: Return duration of time query. */
> +	DRM_PANTHOR_TIMESTAMP_DURATION = 1 << 7,
> +};
> +
>  /**
>   * struct drm_panthor_timestamp_info - Timestamp information
>   *
> @@ -421,11 +453,33 @@ 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.
> +	 *
> +	 * If set to 0, then it is interpreted as:
> +	 *  DRM_PANTHOR_TIMESTAMP_GPU |
> +	 *  DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> +	 *  DRM_PANTHOR_TIMESTAMP_FREQ
> +	 */
> +	__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. */
> +	__u64 cpu_timestamp_nsec;
>  };
>  
>  /**
> -- 
> 2.34.1
> 

With those addressed,

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

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

[Marcin Ślusarz]

On Thu, Mar 19, 2026 at 11:43:45AM +0000, Liviu Dudau wrote:
> Hi Marcin,
> 
> On Thu, Mar 19, 2026 at 12:00:53PM +0100, Marcin Slusarz wrote:
> > ...
> > +#define VALID_TIMESTAMP_QUERY_FLAGS \
> > +		(DRM_PANTHOR_TIMESTAMP_GPU | \
> > +		 DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK | \
> > +		 DRM_PANTHOR_TIMESTAMP_GPU_OFFSET | \
> > +		 DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT | \
> > +		 DRM_PANTHOR_TIMESTAMP_FREQ | \
> > +		 DRM_PANTHOR_TIMESTAMP_DURATION)
> > +
> >  static int panthor_query_timestamp_info(struct panthor_device *ptdev,
> >  					struct drm_panthor_timestamp_info *arg)
> >  {
> >  	int ret;
> > +	u32 flags;
> > +	unsigned long irq_flags;
> > +	struct timespec64 cpu_ts;
> > +	u64 query_start_time;
> > +	bool minimize_interruption;
> > +	u32 timestamp_types = 0;
> > +
> > +	if (arg->flags != 0) {
> > +		flags = arg->flags;
> > +	} else {
> > +		/*
> > +		 * If flags are 0, then ask for the same things that we asked
> > +		 * for before flags were added.
> > +		 */
> > +		flags = DRM_PANTHOR_TIMESTAMP_GPU |
> > +			DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> > +			DRM_PANTHOR_TIMESTAMP_FREQ;
> > +	}
> > +
> > +	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> > +	case 0:

Umm, this should be DRM_PANTHOR_TIMESTAMP_CPU_NONE.

> > +		break;
> > +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> > +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> > +		timestamp_types++;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (flags & ~VALID_TIMESTAMP_QUERY_FLAGS)
> > +		return -EINVAL;
> 
> Can we move this check before the switch and simplify the switch itself to only do the timestamp_types increment?

DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK is bit field that holds individual
clock type values, so we still need to validate the bit field.

> > +
> > +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
> > +		timestamp_types++;
> > +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
> > +		timestamp_types++;
> > +
> > +	/* If user asked to obtain timestamps from more than one source,
> > +	 * then it very likely means they want them to be as close as possible.
> > +	 * If they asked for duration, then that likely means that they
> > +	 * want to know how long obtaining timestamp takes, without random
> > +	 * events, like process scheduling or interrupts.
> > +	 */
> 
> This comment makes me think that user can ask for both CPU_MONOTONIC and
> CPU_MONOTONIC_RAW timestamps, but the code is built to make them exclusive.
> Can we document better what sources can be requested simultaneously?

Somethine like this?

diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
index 8a46ef040c3d..0e455d91e77d 100644
--- a/include/uapi/drm/panthor_drm.h
+++ b/include/uapi/drm/panthor_drm.h
@@ -466,6 +466,11 @@ struct drm_panthor_timestamp_info {
         *  DRM_PANTHOR_TIMESTAMP_GPU |
         *  DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
         *  DRM_PANTHOR_TIMESTAMP_FREQ
+        *
+        * Note: these flags are exclusive to each other (only one can be used):
+        * - DRM_PANTHOR_TIMESTAMP_CPU_NONE
+        * - DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC
+        * - DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW
         */
        __u32 flags;
 

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

[Liviu Dudau]

On Thu, Mar 19, 2026 at 01:39:40PM +0100, Marcin Ślusarz wrote:
> On Thu, Mar 19, 2026 at 11:43:45AM +0000, Liviu Dudau wrote:
> > Hi Marcin,
> > 
> > On Thu, Mar 19, 2026 at 12:00:53PM +0100, Marcin Slusarz wrote:
> > > ...
> > > +#define VALID_TIMESTAMP_QUERY_FLAGS \
> > > +		(DRM_PANTHOR_TIMESTAMP_GPU | \
> > > +		 DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK | \
> > > +		 DRM_PANTHOR_TIMESTAMP_GPU_OFFSET | \
> > > +		 DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT | \
> > > +		 DRM_PANTHOR_TIMESTAMP_FREQ | \
> > > +		 DRM_PANTHOR_TIMESTAMP_DURATION)
> > > +
> > >  static int panthor_query_timestamp_info(struct panthor_device *ptdev,
> > >  					struct drm_panthor_timestamp_info *arg)
> > >  {
> > >  	int ret;
> > > +	u32 flags;
> > > +	unsigned long irq_flags;
> > > +	struct timespec64 cpu_ts;
> > > +	u64 query_start_time;
> > > +	bool minimize_interruption;
> > > +	u32 timestamp_types = 0;
> > > +
> > > +	if (arg->flags != 0) {
> > > +		flags = arg->flags;
> > > +	} else {
> > > +		/*
> > > +		 * If flags are 0, then ask for the same things that we asked
> > > +		 * for before flags were added.
> > > +		 */
> > > +		flags = DRM_PANTHOR_TIMESTAMP_GPU |
> > > +			DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> > > +			DRM_PANTHOR_TIMESTAMP_FREQ;
> > > +	}
> > > +
> > > +	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> > > +	case 0:
> 
> Umm, this should be DRM_PANTHOR_TIMESTAMP_CPU_NONE.
> 
> > > +		break;
> > > +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> > > +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> > > +		timestamp_types++;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (flags & ~VALID_TIMESTAMP_QUERY_FLAGS)
> > > +		return -EINVAL;
> > 
> > Can we move this check before the switch and simplify the switch itself to only do the timestamp_types increment?
> 
> DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK is bit field that holds individual
> clock type values, so we still need to validate the bit field.

The if () test eliminates the default case, and if you change the switch to:

	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_NONE) {
	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
		timestamp_types++;
		break;
	}

then it should be equivalent, right?

> 
> > > +
> > > +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
> > > +		timestamp_types++;
> > > +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
> > > +		timestamp_types++;
> > > +
> > > +	/* If user asked to obtain timestamps from more than one source,
> > > +	 * then it very likely means they want them to be as close as possible.
> > > +	 * If they asked for duration, then that likely means that they
> > > +	 * want to know how long obtaining timestamp takes, without random
> > > +	 * events, like process scheduling or interrupts.
> > > +	 */
> > 
> > This comment makes me think that user can ask for both CPU_MONOTONIC and
> > CPU_MONOTONIC_RAW timestamps, but the code is built to make them exclusive.
> > Can we document better what sources can be requested simultaneously?
> 
> Somethine like this?
> 
> diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> index 8a46ef040c3d..0e455d91e77d 100644
> --- a/include/uapi/drm/panthor_drm.h
> +++ b/include/uapi/drm/panthor_drm.h
> @@ -466,6 +466,11 @@ struct drm_panthor_timestamp_info {
>          *  DRM_PANTHOR_TIMESTAMP_GPU |
>          *  DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
>          *  DRM_PANTHOR_TIMESTAMP_FREQ
> +        *
> +        * Note: these flags are exclusive to each other (only one can be used):
> +        * - DRM_PANTHOR_TIMESTAMP_CPU_NONE
> +        * - DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC
> +        * - DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW

Yes, looks good to me.

Best regards,
Liviu

>          */
>         __u32 flags;
>  

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

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

[Marcin Ślusarz]

On Thu, Mar 19, 2026 at 03:17:36PM +0000, Liviu Dudau wrote:
> On Thu, Mar 19, 2026 at 01:39:40PM +0100, Marcin Ślusarz wrote:
> > On Thu, Mar 19, 2026 at 11:43:45AM +0000, Liviu Dudau wrote:
> > > Hi Marcin,
> > > 
> > > On Thu, Mar 19, 2026 at 12:00:53PM +0100, Marcin Slusarz wrote:
> > > > ...
> > > > +#define VALID_TIMESTAMP_QUERY_FLAGS \
> > > > +		(DRM_PANTHOR_TIMESTAMP_GPU | \
> > > > +		 DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK | \
> > > > +		 DRM_PANTHOR_TIMESTAMP_GPU_OFFSET | \
> > > > +		 DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT | \
> > > > +		 DRM_PANTHOR_TIMESTAMP_FREQ | \
> > > > +		 DRM_PANTHOR_TIMESTAMP_DURATION)
> > > > +
> > > >  static int panthor_query_timestamp_info(struct panthor_device *ptdev,
> > > >  					struct drm_panthor_timestamp_info *arg)
> > > >  {
> > > >  	int ret;
> > > > +	u32 flags;
> > > > +	unsigned long irq_flags;
> > > > +	struct timespec64 cpu_ts;
> > > > +	u64 query_start_time;
> > > > +	bool minimize_interruption;
> > > > +	u32 timestamp_types = 0;
> > > > +
> > > > +	if (arg->flags != 0) {
> > > > +		flags = arg->flags;
> > > > +	} else {
> > > > +		/*
> > > > +		 * If flags are 0, then ask for the same things that we asked
> > > > +		 * for before flags were added.
> > > > +		 */
> > > > +		flags = DRM_PANTHOR_TIMESTAMP_GPU |
> > > > +			DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> > > > +			DRM_PANTHOR_TIMESTAMP_FREQ;
> > > > +	}
> > > > +
> > > > +	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> > > > +	case 0:
> > 
> > Umm, this should be DRM_PANTHOR_TIMESTAMP_CPU_NONE.
> > 
> > > > +		break;
> > > > +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> > > > +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> > > > +		timestamp_types++;
> > > > +		break;
> > > > +	default:
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	if (flags & ~VALID_TIMESTAMP_QUERY_FLAGS)
> > > > +		return -EINVAL;
> > > 
> > > Can we move this check before the switch and simplify the switch itself to only do the timestamp_types increment?
> > 
> > DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK is bit field that holds individual
> > clock type values, so we still need to validate the bit field.
> 
> The if () test eliminates the default case, and if you change the switch to:
> 
> 	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_NONE) {
> 	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> 	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> 		timestamp_types++;
> 		break;
> 	}
> 
> then it should be equivalent, right?

We need the default case to detect garbage values in the part of flags
that ands with DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK.

DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK is 7 << 1,
DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC is 1 << 1,
DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW is 2 << 1,
so 3 << 1, 4 << 1, 5 << 1, 6 << 1, 7 << 1 are all invalid values that need
to be rejected.

And since DRM_PANTHOR_TIMESTAMP_CPU_NONE is 0 << 1, we need it too in
the switch to not be caught by the default case.

> > 
> > > > +
> > > > +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
> > > > +		timestamp_types++;
> > > > +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
> > > > +		timestamp_types++;
> > > > +
> > > > +	/* If user asked to obtain timestamps from more than one source,
> > > > +	 * then it very likely means they want them to be as close as possible.
> > > > +	 * If they asked for duration, then that likely means that they
> > > > +	 * want to know how long obtaining timestamp takes, without random
> > > > +	 * events, like process scheduling or interrupts.
> > > > +	 */
> > > 
> > > This comment makes me think that user can ask for both CPU_MONOTONIC and
> > > CPU_MONOTONIC_RAW timestamps, but the code is built to make them exclusive.
> > > Can we document better what sources can be requested simultaneously?
> > 
> > Somethine like this?
> > 
> > diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> > index 8a46ef040c3d..0e455d91e77d 100644
> > --- a/include/uapi/drm/panthor_drm.h
> > +++ b/include/uapi/drm/panthor_drm.h
> > @@ -466,6 +466,11 @@ struct drm_panthor_timestamp_info {
> >          *  DRM_PANTHOR_TIMESTAMP_GPU |
> >          *  DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> >          *  DRM_PANTHOR_TIMESTAMP_FREQ
> > +        *
> > +        * Note: these flags are exclusive to each other (only one can be used):
> > +        * - DRM_PANTHOR_TIMESTAMP_CPU_NONE
> > +        * - DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC
> > +        * - DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW
> 
> Yes, looks good to me.
> 
> Best regards,
> Liviu
> 
> >          */
> >         __u32 flags;
> >  
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯

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

[Liviu Dudau]

On Thu, Mar 19, 2026 at 04:33:48PM +0100, Marcin Ślusarz wrote:
> On Thu, Mar 19, 2026 at 03:17:36PM +0000, Liviu Dudau wrote:
> > On Thu, Mar 19, 2026 at 01:39:40PM +0100, Marcin Ślusarz wrote:
> > > On Thu, Mar 19, 2026 at 11:43:45AM +0000, Liviu Dudau wrote:
> > > > Hi Marcin,
> > > > 
> > > > On Thu, Mar 19, 2026 at 12:00:53PM +0100, Marcin Slusarz wrote:
> > > > > ...
> > > > > +#define VALID_TIMESTAMP_QUERY_FLAGS \
> > > > > +		(DRM_PANTHOR_TIMESTAMP_GPU | \
> > > > > +		 DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK | \
> > > > > +		 DRM_PANTHOR_TIMESTAMP_GPU_OFFSET | \
> > > > > +		 DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT | \
> > > > > +		 DRM_PANTHOR_TIMESTAMP_FREQ | \
> > > > > +		 DRM_PANTHOR_TIMESTAMP_DURATION)
> > > > > +
> > > > >  static int panthor_query_timestamp_info(struct panthor_device *ptdev,
> > > > >  					struct drm_panthor_timestamp_info *arg)
> > > > >  {
> > > > >  	int ret;
> > > > > +	u32 flags;
> > > > > +	unsigned long irq_flags;
> > > > > +	struct timespec64 cpu_ts;
> > > > > +	u64 query_start_time;
> > > > > +	bool minimize_interruption;
> > > > > +	u32 timestamp_types = 0;
> > > > > +
> > > > > +	if (arg->flags != 0) {
> > > > > +		flags = arg->flags;
> > > > > +	} else {
> > > > > +		/*
> > > > > +		 * If flags are 0, then ask for the same things that we asked
> > > > > +		 * for before flags were added.
> > > > > +		 */
> > > > > +		flags = DRM_PANTHOR_TIMESTAMP_GPU |
> > > > > +			DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> > > > > +			DRM_PANTHOR_TIMESTAMP_FREQ;
> > > > > +	}
> > > > > +
> > > > > +	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> > > > > +	case 0:
> > > 
> > > Umm, this should be DRM_PANTHOR_TIMESTAMP_CPU_NONE.

OK, are you going to re-spin?

> > > 
> > > > > +		break;
> > > > > +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> > > > > +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> > > > > +		timestamp_types++;
> > > > > +		break;
> > > > > +	default:
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	if (flags & ~VALID_TIMESTAMP_QUERY_FLAGS)
> > > > > +		return -EINVAL;
> > > > 
> > > > Can we move this check before the switch and simplify the switch itself to only do the timestamp_types increment?
> > > 
> > > DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK is bit field that holds individual
> > > clock type values, so we still need to validate the bit field.
> > 
> > The if () test eliminates the default case, and if you change the switch to:
> > 
> > 	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_NONE) {
> > 	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> > 	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> > 		timestamp_types++;
> > 		break;
> > 	}
> > 
> > then it should be equivalent, right?
> 
> We need the default case to detect garbage values in the part of flags
> that ands with DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK.
> 
> DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK is 7 << 1,

I understand that you want to make sure that user space doesn't insert values into the
flags and then pretends that because we didn't return an error that's now part of the
ABI. But then maybe you should not reserve the extra bit now if you don't know how
it's going to be used (in other words, why not make TIMESTAMP_CPU_TYPE_MASK  3 << 1?).

> DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC is 1 << 1,
> DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW is 2 << 1,
> so 3 << 1, 4 << 1, 5 << 1, 6 << 1, 7 << 1 are all invalid values that need
> to be rejected.

I am confused about why TIMESTAMP_CPU is a continous range [0-7] while the rest of the
flags are bits in a bitmask. A note should be added to the panthor_drm.h file to
explain this.

> 
> And since DRM_PANTHOR_TIMESTAMP_CPU_NONE is 0 << 1, we need it too in
> the switch to not be caught by the default case.

It only makes sense if you explain that TIMESTAMP_CPU values are in a range. I kept reading
the different versions of the patch thinking that they are bits in a bitmask, with
TIMESTAMP_CPU_NONE being the logical state of not selecting any of the sources.

Best regards,
Liviu


> 
> > > 
> > > > > +
> > > > > +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
> > > > > +		timestamp_types++;
> > > > > +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
> > > > > +		timestamp_types++;
> > > > > +
> > > > > +	/* If user asked to obtain timestamps from more than one source,
> > > > > +	 * then it very likely means they want them to be as close as possible.
> > > > > +	 * If they asked for duration, then that likely means that they
> > > > > +	 * want to know how long obtaining timestamp takes, without random
> > > > > +	 * events, like process scheduling or interrupts.
> > > > > +	 */
> > > > 
> > > > This comment makes me think that user can ask for both CPU_MONOTONIC and
> > > > CPU_MONOTONIC_RAW timestamps, but the code is built to make them exclusive.
> > > > Can we document better what sources can be requested simultaneously?
> > > 
> > > Somethine like this?
> > > 
> > > diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> > > index 8a46ef040c3d..0e455d91e77d 100644
> > > --- a/include/uapi/drm/panthor_drm.h
> > > +++ b/include/uapi/drm/panthor_drm.h
> > > @@ -466,6 +466,11 @@ struct drm_panthor_timestamp_info {
> > >          *  DRM_PANTHOR_TIMESTAMP_GPU |
> > >          *  DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> > >          *  DRM_PANTHOR_TIMESTAMP_FREQ
> > > +        *
> > > +        * Note: these flags are exclusive to each other (only one can be used):
> > > +        * - DRM_PANTHOR_TIMESTAMP_CPU_NONE
> > > +        * - DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC
> > > +        * - DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW
> > 
> > Yes, looks good to me.
> > 
> > Best regards,
> > Liviu
> > 
> > >          */
> > >         __u32 flags;
> > >  
> > 
> > -- 
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ¯\_(ツ)_/¯

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

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

[Marcin Ślusarz]

On Mon, Mar 23, 2026 at 01:16:30PM +0000, Liviu Dudau wrote:
> On Thu, Mar 19, 2026 at 04:33:48PM +0100, Marcin Ślusarz wrote:
> > On Thu, Mar 19, 2026 at 03:17:36PM +0000, Liviu Dudau wrote:
> > > On Thu, Mar 19, 2026 at 01:39:40PM +0100, Marcin Ślusarz wrote:
> > > > On Thu, Mar 19, 2026 at 11:43:45AM +0000, Liviu Dudau wrote:
> > > > > Hi Marcin,
> > > > > 
> > > > > On Thu, Mar 19, 2026 at 12:00:53PM +0100, Marcin Slusarz wrote:
> > > > > > ...
> > > > > > +#define VALID_TIMESTAMP_QUERY_FLAGS \
> > > > > > +		(DRM_PANTHOR_TIMESTAMP_GPU | \
> > > > > > +		 DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK | \
> > > > > > +		 DRM_PANTHOR_TIMESTAMP_GPU_OFFSET | \
> > > > > > +		 DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT | \
> > > > > > +		 DRM_PANTHOR_TIMESTAMP_FREQ | \
> > > > > > +		 DRM_PANTHOR_TIMESTAMP_DURATION)
> > > > > > +
> > > > > >  static int panthor_query_timestamp_info(struct panthor_device *ptdev,
> > > > > >  					struct drm_panthor_timestamp_info *arg)
> > > > > >  {
> > > > > >  	int ret;
> > > > > > +	u32 flags;
> > > > > > +	unsigned long irq_flags;
> > > > > > +	struct timespec64 cpu_ts;
> > > > > > +	u64 query_start_time;
> > > > > > +	bool minimize_interruption;
> > > > > > +	u32 timestamp_types = 0;
> > > > > > +
> > > > > > +	if (arg->flags != 0) {
> > > > > > +		flags = arg->flags;
> > > > > > +	} else {
> > > > > > +		/*
> > > > > > +		 * If flags are 0, then ask for the same things that we asked
> > > > > > +		 * for before flags were added.
> > > > > > +		 */
> > > > > > +		flags = DRM_PANTHOR_TIMESTAMP_GPU |
> > > > > > +			DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> > > > > > +			DRM_PANTHOR_TIMESTAMP_FREQ;
> > > > > > +	}
> > > > > > +
> > > > > > +	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> > > > > > +	case 0:
> > > > 
> > > > Umm, this should be DRM_PANTHOR_TIMESTAMP_CPU_NONE.
> 
> OK, are you going to re-spin?

Yes, as soon as we get to some conclusion.

> > > > 
> > > > > > +		break;
> > > > > > +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> > > > > > +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> > > > > > +		timestamp_types++;
> > > > > > +		break;
> > > > > > +	default:
> > > > > > +		return -EINVAL;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (flags & ~VALID_TIMESTAMP_QUERY_FLAGS)
> > > > > > +		return -EINVAL;
> > > > > 
> > > > > Can we move this check before the switch and simplify the switch itself to only do the timestamp_types increment?
> > > > 
> > > > DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK is bit field that holds individual
> > > > clock type values, so we still need to validate the bit field.
> > > 
> > > The if () test eliminates the default case, and if you change the switch to:
> > > 
> > > 	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_NONE) {
> > > 	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> > > 	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> > > 		timestamp_types++;
> > > 		break;
> > > 	}
> > > 
> > > then it should be equivalent, right?
> > 
> > We need the default case to detect garbage values in the part of flags
> > that ands with DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK.
> > 
> > DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK is 7 << 1,
> 
> I understand that you want to make sure that user space doesn't insert values into the
> flags and then pretends that because we didn't return an error that's now part of the
> ABI. But then maybe you should not reserve the extra bit now if you don't know how
> it's going to be used (in other words, why not make TIMESTAMP_CPU_TYPE_MASK  3 << 1?).

I think it makes sense to reserve this bit now to make it easier to
extend the interface if we ever will need to. Changing uapi definition
is painful enough, so reserving this will rule out misuses of the current
value. Another point is that we would have to validate that the bit is 0
anyway, so I don't see why we can't include that bit in the field that
is supposed to go with.

> > DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC is 1 << 1,
> > DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW is 2 << 1,
> > so 3 << 1, 4 << 1, 5 << 1, 6 << 1, 7 << 1 are all invalid values that need
> > to be rejected.
> 
> I am confused about why TIMESTAMP_CPU is a continous range [0-7] while the rest of the
> flags are bits in a bitmask. A note should be added to the panthor_drm.h file to
> explain this.

Well, just look at the definition of struct drm_panthor_timestamp_info -
there's one to one relation between field and a DRM_PANTHOR_TIMESTAMP_* flag,
with an exception that CPU timestamp types have only one field (well,
a pair of "seconds" and "nanoseconds", but from logical perspective it's
only one value). There's no known reason why anyone would want to query
multiple CPU timestamps together with various GPU counters from _GPU_ query.

> 
> > 
> > And since DRM_PANTHOR_TIMESTAMP_CPU_NONE is 0 << 1, we need it too in
> > the switch to not be caught by the default case.
> 
> It only makes sense if you explain that TIMESTAMP_CPU values are in a range. I kept reading
> the different versions of the patch thinking that they are bits in a bitmask, with
> TIMESTAMP_CPU_NONE being the logical state of not selecting any of the sources.

That should be resolved with the documentation update below, right?

> 
> Best regards,
> Liviu
> 
> 
> > 
> > > > 
> > > > > > +
> > > > > > +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
> > > > > > +		timestamp_types++;
> > > > > > +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
> > > > > > +		timestamp_types++;
> > > > > > +
> > > > > > +	/* If user asked to obtain timestamps from more than one source,
> > > > > > +	 * then it very likely means they want them to be as close as possible.
> > > > > > +	 * If they asked for duration, then that likely means that they
> > > > > > +	 * want to know how long obtaining timestamp takes, without random
> > > > > > +	 * events, like process scheduling or interrupts.
> > > > > > +	 */
> > > > > 
> > > > > This comment makes me think that user can ask for both CPU_MONOTONIC and
> > > > > CPU_MONOTONIC_RAW timestamps, but the code is built to make them exclusive.
> > > > > Can we document better what sources can be requested simultaneously?
> > > > 
> > > > Somethine like this?
> > > > 
> > > > diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> > > > index 8a46ef040c3d..0e455d91e77d 100644
> > > > --- a/include/uapi/drm/panthor_drm.h
> > > > +++ b/include/uapi/drm/panthor_drm.h
> > > > @@ -466,6 +466,11 @@ struct drm_panthor_timestamp_info {
> > > >          *  DRM_PANTHOR_TIMESTAMP_GPU |
> > > >          *  DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> > > >          *  DRM_PANTHOR_TIMESTAMP_FREQ
> > > > +        *
> > > > +        * Note: these flags are exclusive to each other (only one can be used):
> > > > +        * - DRM_PANTHOR_TIMESTAMP_CPU_NONE
> > > > +        * - DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC
> > > > +        * - DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW
> > > 
> > > Yes, looks good to me.
> > > 
> > > Best regards,
> > > Liviu
> > > 
> > > >          */
> > > >         __u32 flags;
> > > >  

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

[Liviu Dudau]

On Mon, Mar 23, 2026 at 05:12:01PM +0100, Marcin Ślusarz wrote:
> On Mon, Mar 23, 2026 at 01:16:30PM +0000, Liviu Dudau wrote:
> > On Thu, Mar 19, 2026 at 04:33:48PM +0100, Marcin Ślusarz wrote:
> > > On Thu, Mar 19, 2026 at 03:17:36PM +0000, Liviu Dudau wrote:
> > > > On Thu, Mar 19, 2026 at 01:39:40PM +0100, Marcin Ślusarz wrote:
> > > > > On Thu, Mar 19, 2026 at 11:43:45AM +0000, Liviu Dudau wrote:
> > > > > > Hi Marcin,
> > > > > > 
> > > > > > On Thu, Mar 19, 2026 at 12:00:53PM +0100, Marcin Slusarz wrote:
> > > > > > > ...
> > > > > > > +#define VALID_TIMESTAMP_QUERY_FLAGS \
> > > > > > > +		(DRM_PANTHOR_TIMESTAMP_GPU | \
> > > > > > > +		 DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK | \
> > > > > > > +		 DRM_PANTHOR_TIMESTAMP_GPU_OFFSET | \
> > > > > > > +		 DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT | \
> > > > > > > +		 DRM_PANTHOR_TIMESTAMP_FREQ | \
> > > > > > > +		 DRM_PANTHOR_TIMESTAMP_DURATION)
> > > > > > > +
> > > > > > >  static int panthor_query_timestamp_info(struct panthor_device *ptdev,
> > > > > > >  					struct drm_panthor_timestamp_info *arg)
> > > > > > >  {
> > > > > > >  	int ret;
> > > > > > > +	u32 flags;
> > > > > > > +	unsigned long irq_flags;
> > > > > > > +	struct timespec64 cpu_ts;
> > > > > > > +	u64 query_start_time;
> > > > > > > +	bool minimize_interruption;
> > > > > > > +	u32 timestamp_types = 0;
> > > > > > > +
> > > > > > > +	if (arg->flags != 0) {
> > > > > > > +		flags = arg->flags;
> > > > > > > +	} else {
> > > > > > > +		/*
> > > > > > > +		 * If flags are 0, then ask for the same things that we asked
> > > > > > > +		 * for before flags were added.
> > > > > > > +		 */
> > > > > > > +		flags = DRM_PANTHOR_TIMESTAMP_GPU |
> > > > > > > +			DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> > > > > > > +			DRM_PANTHOR_TIMESTAMP_FREQ;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> > > > > > > +	case 0:
> > > > > 
> > > > > Umm, this should be DRM_PANTHOR_TIMESTAMP_CPU_NONE.
> > 
> > OK, are you going to re-spin?
> 
> Yes, as soon as we get to some conclusion.
> 
> > > > > 
> > > > > > > +		break;
> > > > > > > +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> > > > > > > +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> > > > > > > +		timestamp_types++;
> > > > > > > +		break;
> > > > > > > +	default:
> > > > > > > +		return -EINVAL;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	if (flags & ~VALID_TIMESTAMP_QUERY_FLAGS)
> > > > > > > +		return -EINVAL;
> > > > > > 
> > > > > > Can we move this check before the switch and simplify the switch itself to only do the timestamp_types increment?
> > > > > 
> > > > > DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK is bit field that holds individual
> > > > > clock type values, so we still need to validate the bit field.
> > > > 
> > > > The if () test eliminates the default case, and if you change the switch to:
> > > > 
> > > > 	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_NONE) {
> > > > 	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> > > > 	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> > > > 		timestamp_types++;
> > > > 		break;
> > > > 	}
> > > > 
> > > > then it should be equivalent, right?
> > > 
> > > We need the default case to detect garbage values in the part of flags
> > > that ands with DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK.
> > > 
> > > DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK is 7 << 1,
> > 
> > I understand that you want to make sure that user space doesn't insert values into the
> > flags and then pretends that because we didn't return an error that's now part of the
> > ABI. But then maybe you should not reserve the extra bit now if you don't know how
> > it's going to be used (in other words, why not make TIMESTAMP_CPU_TYPE_MASK  3 << 1?).
> 
> I think it makes sense to reserve this bit now to make it easier to
> extend the interface if we ever will need to. Changing uapi definition
> is painful enough, so reserving this will rule out misuses of the current
> value. Another point is that we would have to validate that the bit is 0
> anyway, so I don't see why we can't include that bit in the field that
> is supposed to go with.

You do validate the bits in the if (flags & ~VALID_TIMESTAMP_QUERY_FLAGS) test if the
QUERY_FLAGS bitfield is updated.

> 
> > > DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC is 1 << 1,
> > > DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW is 2 << 1,
> > > so 3 << 1, 4 << 1, 5 << 1, 6 << 1, 7 << 1 are all invalid values that need
> > > to be rejected.
> > 
> > I am confused about why TIMESTAMP_CPU is a continous range [0-7] while the rest of the
> > flags are bits in a bitmask. A note should be added to the panthor_drm.h file to
> > explain this.
> 
> Well, just look at the definition of struct drm_panthor_timestamp_info -
> there's one to one relation between field and a DRM_PANTHOR_TIMESTAMP_* flag,
> with an exception that CPU timestamp types have only one field (well,
> a pair of "seconds" and "nanoseconds", but from logical perspective it's
> only one value). There's no known reason why anyone would want to query
> multiple CPU timestamps together with various GPU counters from _GPU_ query.

OK, so what I'm hearing is: we're reserving space for up to 6 CPU clock sources (zero is
no CPU clock) but you can have only one active at any time (because we can't think of any
reason why you would want more at the same time) and we currently only define two sources
because that's what user space cares about.

If you agree with this then this will be recorded in the thread and we can go back in the
future and reference it.

> 
> > 
> > > 
> > > And since DRM_PANTHOR_TIMESTAMP_CPU_NONE is 0 << 1, we need it too in
> > > the switch to not be caught by the default case.
> > 
> > It only makes sense if you explain that TIMESTAMP_CPU values are in a range. I kept reading
> > the different versions of the patch thinking that they are bits in a bitmask, with
> > TIMESTAMP_CPU_NONE being the logical state of not selecting any of the sources.
> 
> That should be resolved with the documentation update below, right?

The documentation doesn't say anything about the intent for future values for CPU sources, but
if you don't want to add more in the documentation at least we have this thread as reference.

I don't want to turn this into a bikeshedding thread, so please do a v4 with the switch case updated
and the documentation addition and I will ACK that.

Best regards,
Liviu

> 
> > 
> > Best regards,
> > Liviu
> > 
> > 
> > > 
> > > > > 
> > > > > > > +
> > > > > > > +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
> > > > > > > +		timestamp_types++;
> > > > > > > +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
> > > > > > > +		timestamp_types++;
> > > > > > > +
> > > > > > > +	/* If user asked to obtain timestamps from more than one source,
> > > > > > > +	 * then it very likely means they want them to be as close as possible.
> > > > > > > +	 * If they asked for duration, then that likely means that they
> > > > > > > +	 * want to know how long obtaining timestamp takes, without random
> > > > > > > +	 * events, like process scheduling or interrupts.
> > > > > > > +	 */
> > > > > > 
> > > > > > This comment makes me think that user can ask for both CPU_MONOTONIC and
> > > > > > CPU_MONOTONIC_RAW timestamps, but the code is built to make them exclusive.
> > > > > > Can we document better what sources can be requested simultaneously?
> > > > > 
> > > > > Somethine like this?
> > > > > 
> > > > > diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> > > > > index 8a46ef040c3d..0e455d91e77d 100644
> > > > > --- a/include/uapi/drm/panthor_drm.h
> > > > > +++ b/include/uapi/drm/panthor_drm.h
> > > > > @@ -466,6 +466,11 @@ struct drm_panthor_timestamp_info {
> > > > >          *  DRM_PANTHOR_TIMESTAMP_GPU |
> > > > >          *  DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> > > > >          *  DRM_PANTHOR_TIMESTAMP_FREQ
> > > > > +        *
> > > > > +        * Note: these flags are exclusive to each other (only one can be used):
> > > > > +        * - DRM_PANTHOR_TIMESTAMP_CPU_NONE
> > > > > +        * - DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC
> > > > > +        * - DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW
> > > > 
> > > > Yes, looks good to me.
> > > > 
> > > > Best regards,
> > > > Liviu
> > > > 
> > > > >          */
> > > > >         __u32 flags;
> > > > >  

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

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

[Marcin Slusarz]

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>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Changes in v4:
 - 0 replaced by DRM_PANTHOR_TIMESTAMP_CPU_NONE in CPU_TYPE_MASK switch
 - extended documentation for flags
Changes in v3:
 - {} around multiline statements
 - added comment about default flags
 - added VALID_TIMESTAMP_QUERY_FLAGS
 - used u64_to_user_ptr
Changes in v2:
 - added DRM_PANTHOR_TIMESTAMP_CPU_NONE
 - cpu_timestamp_nsec extended to u64
 - pad1 removed
 - copy_from_user -> copy_struct_from_user
 - looking at size replaced by looking at flags
 - more comments
---
 drivers/gpu/drm/panthor/panthor_drv.c | 134 ++++++++++++++++++++++++--
 include/uapi/drm/panthor_drm.h        |  63 +++++++++++-
 2 files changed, 189 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
index 165dddfde6ca..9e65af453119 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>
@@ -761,22 +763,135 @@ static void panthor_submit_ctx_cleanup(struct panthor_submit_ctx *ctx,
 	kvfree(ctx->jobs);
 }
 
+#define VALID_TIMESTAMP_QUERY_FLAGS \
+		(DRM_PANTHOR_TIMESTAMP_GPU | \
+		 DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK | \
+		 DRM_PANTHOR_TIMESTAMP_GPU_OFFSET | \
+		 DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT | \
+		 DRM_PANTHOR_TIMESTAMP_FREQ | \
+		 DRM_PANTHOR_TIMESTAMP_DURATION)
+
 static int panthor_query_timestamp_info(struct panthor_device *ptdev,
 					struct drm_panthor_timestamp_info *arg)
 {
 	int ret;
+	u32 flags;
+	unsigned long irq_flags;
+	struct timespec64 cpu_ts;
+	u64 query_start_time;
+	bool minimize_interruption;
+	u32 timestamp_types = 0;
+
+	if (arg->flags != 0) {
+		flags = arg->flags;
+	} else {
+		/*
+		 * If flags are 0, then ask for the same things that we asked
+		 * for before flags were added.
+		 */
+		flags = DRM_PANTHOR_TIMESTAMP_GPU |
+			DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
+			DRM_PANTHOR_TIMESTAMP_FREQ;
+	}
+
+	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
+	case DRM_PANTHOR_TIMESTAMP_CPU_NONE:
+		break;
+	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
+	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
+		timestamp_types++;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (flags & ~VALID_TIMESTAMP_QUERY_FLAGS)
+		return -EINVAL;
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
+		timestamp_types++;
+	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
+		timestamp_types++;
+
+	/* If user asked to obtain timestamps from more than one source,
+	 * then it very likely means they want them to be as close as possible.
+	 * If they asked for duration, then that likely means that they
+	 * want to know how long obtaining timestamp takes, without random
+	 * events, like process scheduling or interrupts.
+	 */
+	minimize_interruption =
+		(flags & DRM_PANTHOR_TIMESTAMP_DURATION) ||
+		(timestamp_types >= 2);
 
 	ret = panthor_device_resume_and_get(ptdev);
 	if (ret)
 		return ret;
 
+	if (flags & DRM_PANTHOR_TIMESTAMP_FREQ) {
 #ifdef CONFIG_ARM_ARCH_TIMER
-	arg->timestamp_frequency = arch_timer_get_cntfrq();
+		arg->timestamp_frequency = arch_timer_get_cntfrq();
 #else
-	arg->timestamp_frequency = 0;
+		arg->timestamp_frequency = 0;
 #endif
-	arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
-	arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
+	} else {
+		arg->timestamp_frequency = 0;
+	}
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_OFFSET)
+		arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
+	else
+		arg->timestamp_offset = 0;
+
+	if (minimize_interruption) {
+		preempt_disable();
+		local_irq_save(irq_flags);
+	}
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
+		query_start_time = local_clock();
+	else
+		query_start_time = 0;
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
+		arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
+	else
+		arg->current_timestamp = 0;
+
+	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
+	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
+		ktime_get_ts64(&cpu_ts);
+		break;
+	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
+		ktime_get_raw_ts64(&cpu_ts);
+		break;
+	default:
+		break;
+	}
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
+		arg->cycle_count = gpu_read64_counter(ptdev, GPU_CYCLE_COUNT);
+	else
+		arg->cycle_count = 0;
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
+		arg->duration_nsec = local_clock() - query_start_time;
+	else
+		arg->duration_nsec = 0;
+
+	if (minimize_interruption) {
+		local_irq_restore(irq_flags);
+		preempt_enable();
+	}
+
+	if (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
+		timens_add_monotonic(&cpu_ts);
+
+		arg->cpu_timestamp_sec = cpu_ts.tv_sec;
+		arg->cpu_timestamp_nsec = cpu_ts.tv_nsec;
+	} else {
+		arg->cpu_timestamp_sec = 0;
+		arg->cpu_timestamp_nsec = 0;
+	}
 
 	pm_runtime_put(ptdev->base.dev);
 	return 0;
@@ -851,8 +966,14 @@ static int panthor_ioctl_dev_query(struct drm_device *ddev, void *data, struct d
 		return PANTHOR_UOBJ_SET(args->pointer, args->size, ptdev->csif_info);
 
 	case DRM_PANTHOR_DEV_QUERY_TIMESTAMP_INFO:
-		ret = panthor_query_timestamp_info(ptdev, &timestamp_info);
+		ret = copy_struct_from_user(&timestamp_info,
+					    sizeof(timestamp_info),
+					    u64_to_user_ptr(args->pointer),
+					    args->size);
+		if (ret)
+			return ret;
 
+		ret = panthor_query_timestamp_info(ptdev, &timestamp_info);
 		if (ret)
 			return ret;
 
@@ -1680,6 +1801,7 @@ static void panthor_debugfs_init(struct drm_minor *minor)
  *       - adds DRM_IOCTL_PANTHOR_BO_SYNC ioctl
  *       - adds DRM_IOCTL_PANTHOR_BO_QUERY_INFO ioctl
  *       - adds drm_panthor_gpu_info::selected_coherency
+ * - 1.8 - extends DEV_QUERY_TIMESTAMP_INFO with flags
  */
 static const struct drm_driver panthor_drm_driver = {
 	.driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ |
@@ -1693,7 +1815,7 @@ static const struct drm_driver panthor_drm_driver = {
 	.name = "panthor",
 	.desc = "Panthor DRM driver",
 	.major = 1,
-	.minor = 7,
+	.minor = 8,
 
 	.gem_create_object = panthor_gem_create_object,
 	.gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table,
diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
index b401ac585d6a..0e455d91e77d 100644
--- a/include/uapi/drm/panthor_drm.h
+++ b/include/uapi/drm/panthor_drm.h
@@ -409,6 +409,38 @@ struct drm_panthor_csif_info {
 	__u32 pad;
 };
 
+/**
+ * enum drm_panthor_timestamp_info_flags - drm_panthor_timestamp_info.flags
+ */
+enum drm_panthor_timestamp_info_flags {
+	/** @DRM_PANTHOR_TIMESTAMP_GPU: Query GPU time. */
+	DRM_PANTHOR_TIMESTAMP_GPU = 1 << 0,
+
+	/** @DRM_PANTHOR_TIMESTAMP_CPU_NONE: Don't query CPU time. */
+	DRM_PANTHOR_TIMESTAMP_CPU_NONE = 0 << 1,
+
+	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC: Query CPU time using CLOCK_MONOTONIC. */
+	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC = 1 << 1,
+
+	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW: Query CPU time using CLOCK_MONOTONIC_RAW. */
+	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW = 2 << 1,
+
+	/** @DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK: Space reserved for CPU clock type. */
+	DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK = 7 << 1,
+
+	/** @DRM_PANTHOR_TIMESTAMP_GPU_OFFSET: Query GPU offset. */
+	DRM_PANTHOR_TIMESTAMP_GPU_OFFSET = 1 << 4,
+
+	/** @DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT: Query GPU cycle count. */
+	DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT = 1 << 5,
+
+	/** @DRM_PANTHOR_TIMESTAMP_FREQ: Query timestamp frequency. */
+	DRM_PANTHOR_TIMESTAMP_FREQ = 1 << 6,
+
+	/** @DRM_PANTHOR_TIMESTAMP_DURATION: Return duration of time query. */
+	DRM_PANTHOR_TIMESTAMP_DURATION = 1 << 7,
+};
+
 /**
  * struct drm_panthor_timestamp_info - Timestamp information
  *
@@ -421,11 +453,38 @@ 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.
+	 *
+	 * If set to 0, then it is interpreted as:
+	 *  DRM_PANTHOR_TIMESTAMP_GPU |
+	 *  DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
+	 *  DRM_PANTHOR_TIMESTAMP_FREQ
+	 *
+	 * Note: these flags are exclusive to each other (only one can be used):
+	 * - DRM_PANTHOR_TIMESTAMP_CPU_NONE
+	 * - DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC
+	 * - DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW
+	 */
+	__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. */
+	__u64 cpu_timestamp_nsec;
 };
 
 /**
-- 
2.34.1

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

[Marcin Ślusarz]

On Tue, Mar 24, 2026 at 10:41:56AM +0000, Liviu Dudau wrote:
> On Mon, Mar 23, 2026 at 05:12:01PM +0100, Marcin Ślusarz wrote:
> > On Mon, Mar 23, 2026 at 01:16:30PM +0000, Liviu Dudau wrote:
> > > On Thu, Mar 19, 2026 at 04:33:48PM +0100, Marcin Ślusarz wrote:
> > > > On Thu, Mar 19, 2026 at 03:17:36PM +0000, Liviu Dudau wrote:
> > > > > On Thu, Mar 19, 2026 at 01:39:40PM +0100, Marcin Ślusarz wrote:
> > > > > > On Thu, Mar 19, 2026 at 11:43:45AM +0000, Liviu Dudau wrote:
> > > > > > > Hi Marcin,
> > > > > > > 
> > > > > > > On Thu, Mar 19, 2026 at 12:00:53PM +0100, Marcin Slusarz wrote:
> > > > > > > > ...
> > > > > > > > +#define VALID_TIMESTAMP_QUERY_FLAGS \
> > > > > > > > +		(DRM_PANTHOR_TIMESTAMP_GPU | \
> > > > > > > > +		 DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK | \
> > > > > > > > +		 DRM_PANTHOR_TIMESTAMP_GPU_OFFSET | \
> > > > > > > > +		 DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT | \
> > > > > > > > +		 DRM_PANTHOR_TIMESTAMP_FREQ | \
> > > > > > > > +		 DRM_PANTHOR_TIMESTAMP_DURATION)
> > > > > > > > +
> > > > > > > >  static int panthor_query_timestamp_info(struct panthor_device *ptdev,
> > > > > > > >  					struct drm_panthor_timestamp_info *arg)
> > > > > > > >  {
> > > > > > > >  	int ret;
> > > > > > > > +	u32 flags;
> > > > > > > > +	unsigned long irq_flags;
> > > > > > > > +	struct timespec64 cpu_ts;
> > > > > > > > +	u64 query_start_time;
> > > > > > > > +	bool minimize_interruption;
> > > > > > > > +	u32 timestamp_types = 0;
> > > > > > > > +
> > > > > > > > +	if (arg->flags != 0) {
> > > > > > > > +		flags = arg->flags;
> > > > > > > > +	} else {
> > > > > > > > +		/*
> > > > > > > > +		 * If flags are 0, then ask for the same things that we asked
> > > > > > > > +		 * for before flags were added.
> > > > > > > > +		 */
> > > > > > > > +		flags = DRM_PANTHOR_TIMESTAMP_GPU |
> > > > > > > > +			DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> > > > > > > > +			DRM_PANTHOR_TIMESTAMP_FREQ;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> > > > > > > > +	case 0:
> > > > > > 
> > > > > > Umm, this should be DRM_PANTHOR_TIMESTAMP_CPU_NONE.
> > > 
> > > OK, are you going to re-spin?
> > 
> > Yes, as soon as we get to some conclusion.
> > 
> > > > > > 
> > > > > > > > +		break;
> > > > > > > > +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> > > > > > > > +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> > > > > > > > +		timestamp_types++;
> > > > > > > > +		break;
> > > > > > > > +	default:
> > > > > > > > +		return -EINVAL;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	if (flags & ~VALID_TIMESTAMP_QUERY_FLAGS)
> > > > > > > > +		return -EINVAL;
> > > > > > > 
> > > > > > > Can we move this check before the switch and simplify the switch itself to only do the timestamp_types increment?
> > > > > > 
> > > > > > DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK is bit field that holds individual
> > > > > > clock type values, so we still need to validate the bit field.
> > > > > 
> > > > > The if () test eliminates the default case, and if you change the switch to:
> > > > > 
> > > > > 	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_NONE) {
> > > > > 	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> > > > > 	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> > > > > 		timestamp_types++;
> > > > > 		break;
> > > > > 	}
> > > > > 
> > > > > then it should be equivalent, right?
> > > > 
> > > > We need the default case to detect garbage values in the part of flags
> > > > that ands with DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK.
> > > > 
> > > > DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK is 7 << 1,
> > > 
> > > I understand that you want to make sure that user space doesn't insert values into the
> > > flags and then pretends that because we didn't return an error that's now part of the
> > > ABI. But then maybe you should not reserve the extra bit now if you don't know how
> > > it's going to be used (in other words, why not make TIMESTAMP_CPU_TYPE_MASK  3 << 1?).
> > 
> > I think it makes sense to reserve this bit now to make it easier to
> > extend the interface if we ever will need to. Changing uapi definition
> > is painful enough, so reserving this will rule out misuses of the current
> > value. Another point is that we would have to validate that the bit is 0
> > anyway, so I don't see why we can't include that bit in the field that
> > is supposed to go with.
> 
> You do validate the bits in the if (flags & ~VALID_TIMESTAMP_QUERY_FLAGS) test if the
> QUERY_FLAGS bitfield is updated.
> 
> > 
> > > > DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC is 1 << 1,
> > > > DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW is 2 << 1,
> > > > so 3 << 1, 4 << 1, 5 << 1, 6 << 1, 7 << 1 are all invalid values that need
> > > > to be rejected.
> > > 
> > > I am confused about why TIMESTAMP_CPU is a continous range [0-7] while the rest of the
> > > flags are bits in a bitmask. A note should be added to the panthor_drm.h file to
> > > explain this.
> > 
> > Well, just look at the definition of struct drm_panthor_timestamp_info -
> > there's one to one relation between field and a DRM_PANTHOR_TIMESTAMP_* flag,
> > with an exception that CPU timestamp types have only one field (well,
> > a pair of "seconds" and "nanoseconds", but from logical perspective it's
> > only one value). There's no known reason why anyone would want to query
> > multiple CPU timestamps together with various GPU counters from _GPU_ query.
> 
> OK, so what I'm hearing is: we're reserving space for up to 6 CPU clock sources (zero is
> no CPU clock) but you can have only one active at any time (because we can't think of any
> reason why you would want more at the same time) and we currently only define two sources
> because that's what user space cares about.
> 
> If you agree with this then this will be recorded in the thread and we can go back in the
> future and reference it.

Up to 7 CPU clock sources and 0 as no CPU clock, but yes, this is the intent.

> 
> > 
> > > 
> > > > 
> > > > And since DRM_PANTHOR_TIMESTAMP_CPU_NONE is 0 << 1, we need it too in
> > > > the switch to not be caught by the default case.
> > > 
> > > It only makes sense if you explain that TIMESTAMP_CPU values are in a range. I kept reading
> > > the different versions of the patch thinking that they are bits in a bitmask, with
> > > TIMESTAMP_CPU_NONE being the logical state of not selecting any of the sources.
> > 
> > That should be resolved with the documentation update below, right?
> 
> The documentation doesn't say anything about the intent for future values for CPU sources, but
> if you don't want to add more in the documentation at least we have this thread as reference.

It's not that I don't want to add more documentation, I just don't
understand what else needs to be explained.

> 
> I don't want to turn this into a bikeshedding thread, so please do a v4 with the switch case updated
> and the documentation addition and I will ACK that.

Done

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

[Liviu Dudau]

On Tue, Mar 24, 2026 at 02:25:57PM +0100, Marcin Slusarz 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>
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> ---
> Changes in v4:
>  - 0 replaced by DRM_PANTHOR_TIMESTAMP_CPU_NONE in CPU_TYPE_MASK switch
>  - extended documentation for flags
> Changes in v3:
>  - {} around multiline statements
>  - added comment about default flags
>  - added VALID_TIMESTAMP_QUERY_FLAGS
>  - used u64_to_user_ptr
> Changes in v2:
>  - added DRM_PANTHOR_TIMESTAMP_CPU_NONE
>  - cpu_timestamp_nsec extended to u64
>  - pad1 removed
>  - copy_from_user -> copy_struct_from_user
>  - looking at size replaced by looking at flags
>  - more comments
> ---
>  drivers/gpu/drm/panthor/panthor_drv.c | 134 ++++++++++++++++++++++++--
>  include/uapi/drm/panthor_drm.h        |  63 +++++++++++-
>  2 files changed, 189 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
> index 165dddfde6ca..9e65af453119 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>
> @@ -761,22 +763,135 @@ static void panthor_submit_ctx_cleanup(struct panthor_submit_ctx *ctx,
>  	kvfree(ctx->jobs);
>  }
>  
> +#define VALID_TIMESTAMP_QUERY_FLAGS \
> +		(DRM_PANTHOR_TIMESTAMP_GPU | \
> +		 DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK | \
> +		 DRM_PANTHOR_TIMESTAMP_GPU_OFFSET | \
> +		 DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT | \
> +		 DRM_PANTHOR_TIMESTAMP_FREQ | \
> +		 DRM_PANTHOR_TIMESTAMP_DURATION)
> +
>  static int panthor_query_timestamp_info(struct panthor_device *ptdev,
>  					struct drm_panthor_timestamp_info *arg)
>  {
>  	int ret;
> +	u32 flags;
> +	unsigned long irq_flags;
> +	struct timespec64 cpu_ts;
> +	u64 query_start_time;
> +	bool minimize_interruption;
> +	u32 timestamp_types = 0;
> +
> +	if (arg->flags != 0) {
> +		flags = arg->flags;
> +	} else {
> +		/*
> +		 * If flags are 0, then ask for the same things that we asked
> +		 * for before flags were added.
> +		 */
> +		flags = DRM_PANTHOR_TIMESTAMP_GPU |
> +			DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> +			DRM_PANTHOR_TIMESTAMP_FREQ;
> +	}
> +
> +	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> +	case DRM_PANTHOR_TIMESTAMP_CPU_NONE:
> +		break;
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> +		timestamp_types++;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (flags & ~VALID_TIMESTAMP_QUERY_FLAGS)
> +		return -EINVAL;
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
> +		timestamp_types++;
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
> +		timestamp_types++;
> +
> +	/* If user asked to obtain timestamps from more than one source,
> +	 * then it very likely means they want them to be as close as possible.
> +	 * If they asked for duration, then that likely means that they
> +	 * want to know how long obtaining timestamp takes, without random
> +	 * events, like process scheduling or interrupts.
> +	 */
> +	minimize_interruption =
> +		(flags & DRM_PANTHOR_TIMESTAMP_DURATION) ||
> +		(timestamp_types >= 2);
>  
>  	ret = panthor_device_resume_and_get(ptdev);
>  	if (ret)
>  		return ret;
>  
> +	if (flags & DRM_PANTHOR_TIMESTAMP_FREQ) {
>  #ifdef CONFIG_ARM_ARCH_TIMER
> -	arg->timestamp_frequency = arch_timer_get_cntfrq();
> +		arg->timestamp_frequency = arch_timer_get_cntfrq();
>  #else
> -	arg->timestamp_frequency = 0;
> +		arg->timestamp_frequency = 0;
>  #endif
> -	arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
> -	arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
> +	} else {
> +		arg->timestamp_frequency = 0;
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_OFFSET)
> +		arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
> +	else
> +		arg->timestamp_offset = 0;
> +
> +	if (minimize_interruption) {
> +		preempt_disable();
> +		local_irq_save(irq_flags);
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
> +		query_start_time = local_clock();
> +	else
> +		query_start_time = 0;
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
> +		arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
> +	else
> +		arg->current_timestamp = 0;
> +
> +	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> +		ktime_get_ts64(&cpu_ts);
> +		break;
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> +		ktime_get_raw_ts64(&cpu_ts);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
> +		arg->cycle_count = gpu_read64_counter(ptdev, GPU_CYCLE_COUNT);
> +	else
> +		arg->cycle_count = 0;
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
> +		arg->duration_nsec = local_clock() - query_start_time;
> +	else
> +		arg->duration_nsec = 0;
> +
> +	if (minimize_interruption) {
> +		local_irq_restore(irq_flags);
> +		preempt_enable();
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> +		timens_add_monotonic(&cpu_ts);
> +
> +		arg->cpu_timestamp_sec = cpu_ts.tv_sec;
> +		arg->cpu_timestamp_nsec = cpu_ts.tv_nsec;
> +	} else {
> +		arg->cpu_timestamp_sec = 0;
> +		arg->cpu_timestamp_nsec = 0;
> +	}
>  
>  	pm_runtime_put(ptdev->base.dev);
>  	return 0;
> @@ -851,8 +966,14 @@ static int panthor_ioctl_dev_query(struct drm_device *ddev, void *data, struct d
>  		return PANTHOR_UOBJ_SET(args->pointer, args->size, ptdev->csif_info);
>  
>  	case DRM_PANTHOR_DEV_QUERY_TIMESTAMP_INFO:
> -		ret = panthor_query_timestamp_info(ptdev, &timestamp_info);
> +		ret = copy_struct_from_user(&timestamp_info,
> +					    sizeof(timestamp_info),
> +					    u64_to_user_ptr(args->pointer),
> +					    args->size);
> +		if (ret)
> +			return ret;
>  
> +		ret = panthor_query_timestamp_info(ptdev, &timestamp_info);
>  		if (ret)
>  			return ret;
>  
> @@ -1680,6 +1801,7 @@ static void panthor_debugfs_init(struct drm_minor *minor)
>   *       - adds DRM_IOCTL_PANTHOR_BO_SYNC ioctl
>   *       - adds DRM_IOCTL_PANTHOR_BO_QUERY_INFO ioctl
>   *       - adds drm_panthor_gpu_info::selected_coherency
> + * - 1.8 - extends DEV_QUERY_TIMESTAMP_INFO with flags
>   */
>  static const struct drm_driver panthor_drm_driver = {
>  	.driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ |
> @@ -1693,7 +1815,7 @@ static const struct drm_driver panthor_drm_driver = {
>  	.name = "panthor",
>  	.desc = "Panthor DRM driver",
>  	.major = 1,
> -	.minor = 7,
> +	.minor = 8,
>  
>  	.gem_create_object = panthor_gem_create_object,
>  	.gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table,
> diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> index b401ac585d6a..0e455d91e77d 100644
> --- a/include/uapi/drm/panthor_drm.h
> +++ b/include/uapi/drm/panthor_drm.h
> @@ -409,6 +409,38 @@ struct drm_panthor_csif_info {
>  	__u32 pad;
>  };
>  
> +/**
> + * enum drm_panthor_timestamp_info_flags - drm_panthor_timestamp_info.flags
> + */
> +enum drm_panthor_timestamp_info_flags {
> +	/** @DRM_PANTHOR_TIMESTAMP_GPU: Query GPU time. */
> +	DRM_PANTHOR_TIMESTAMP_GPU = 1 << 0,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_NONE: Don't query CPU time. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_NONE = 0 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC: Query CPU time using CLOCK_MONOTONIC. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC = 1 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW: Query CPU time using CLOCK_MONOTONIC_RAW. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW = 2 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK: Space reserved for CPU clock type. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK = 7 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_GPU_OFFSET: Query GPU offset. */
> +	DRM_PANTHOR_TIMESTAMP_GPU_OFFSET = 1 << 4,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT: Query GPU cycle count. */
> +	DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT = 1 << 5,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_FREQ: Query timestamp frequency. */
> +	DRM_PANTHOR_TIMESTAMP_FREQ = 1 << 6,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_DURATION: Return duration of time query. */
> +	DRM_PANTHOR_TIMESTAMP_DURATION = 1 << 7,
> +};
> +
>  /**
>   * struct drm_panthor_timestamp_info - Timestamp information
>   *
> @@ -421,11 +453,38 @@ 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.
> +	 *
> +	 * If set to 0, then it is interpreted as:
> +	 *  DRM_PANTHOR_TIMESTAMP_GPU |
> +	 *  DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> +	 *  DRM_PANTHOR_TIMESTAMP_FREQ
> +	 *
> +	 * Note: these flags are exclusive to each other (only one can be used):
> +	 * - DRM_PANTHOR_TIMESTAMP_CPU_NONE
> +	 * - DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC
> +	 * - DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW
> +	 */
> +	__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. */
> +	__u64 cpu_timestamp_nsec;
>  };
>  
>  /**
> -- 
> 2.34.1
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

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

[Liviu Dudau]

On Tue, Mar 24, 2026 at 02:25:57PM +0100, Marcin Slusarz 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>
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

Pushed to drm-misc-next

Best regards,
Liviu

> ---
> Changes in v4:
>  - 0 replaced by DRM_PANTHOR_TIMESTAMP_CPU_NONE in CPU_TYPE_MASK switch
>  - extended documentation for flags
> Changes in v3:
>  - {} around multiline statements
>  - added comment about default flags
>  - added VALID_TIMESTAMP_QUERY_FLAGS
>  - used u64_to_user_ptr
> Changes in v2:
>  - added DRM_PANTHOR_TIMESTAMP_CPU_NONE
>  - cpu_timestamp_nsec extended to u64
>  - pad1 removed
>  - copy_from_user -> copy_struct_from_user
>  - looking at size replaced by looking at flags
>  - more comments
> ---
>  drivers/gpu/drm/panthor/panthor_drv.c | 134 ++++++++++++++++++++++++--
>  include/uapi/drm/panthor_drm.h        |  63 +++++++++++-
>  2 files changed, 189 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
> index 165dddfde6ca..9e65af453119 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>
> @@ -761,22 +763,135 @@ static void panthor_submit_ctx_cleanup(struct panthor_submit_ctx *ctx,
>  	kvfree(ctx->jobs);
>  }
>  
> +#define VALID_TIMESTAMP_QUERY_FLAGS \
> +		(DRM_PANTHOR_TIMESTAMP_GPU | \
> +		 DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK | \
> +		 DRM_PANTHOR_TIMESTAMP_GPU_OFFSET | \
> +		 DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT | \
> +		 DRM_PANTHOR_TIMESTAMP_FREQ | \
> +		 DRM_PANTHOR_TIMESTAMP_DURATION)
> +
>  static int panthor_query_timestamp_info(struct panthor_device *ptdev,
>  					struct drm_panthor_timestamp_info *arg)
>  {
>  	int ret;
> +	u32 flags;
> +	unsigned long irq_flags;
> +	struct timespec64 cpu_ts;
> +	u64 query_start_time;
> +	bool minimize_interruption;
> +	u32 timestamp_types = 0;
> +
> +	if (arg->flags != 0) {
> +		flags = arg->flags;
> +	} else {
> +		/*
> +		 * If flags are 0, then ask for the same things that we asked
> +		 * for before flags were added.
> +		 */
> +		flags = DRM_PANTHOR_TIMESTAMP_GPU |
> +			DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> +			DRM_PANTHOR_TIMESTAMP_FREQ;
> +	}
> +
> +	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> +	case DRM_PANTHOR_TIMESTAMP_CPU_NONE:
> +		break;
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> +		timestamp_types++;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (flags & ~VALID_TIMESTAMP_QUERY_FLAGS)
> +		return -EINVAL;
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
> +		timestamp_types++;
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
> +		timestamp_types++;
> +
> +	/* If user asked to obtain timestamps from more than one source,
> +	 * then it very likely means they want them to be as close as possible.
> +	 * If they asked for duration, then that likely means that they
> +	 * want to know how long obtaining timestamp takes, without random
> +	 * events, like process scheduling or interrupts.
> +	 */
> +	minimize_interruption =
> +		(flags & DRM_PANTHOR_TIMESTAMP_DURATION) ||
> +		(timestamp_types >= 2);
>  
>  	ret = panthor_device_resume_and_get(ptdev);
>  	if (ret)
>  		return ret;
>  
> +	if (flags & DRM_PANTHOR_TIMESTAMP_FREQ) {
>  #ifdef CONFIG_ARM_ARCH_TIMER
> -	arg->timestamp_frequency = arch_timer_get_cntfrq();
> +		arg->timestamp_frequency = arch_timer_get_cntfrq();
>  #else
> -	arg->timestamp_frequency = 0;
> +		arg->timestamp_frequency = 0;
>  #endif
> -	arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
> -	arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
> +	} else {
> +		arg->timestamp_frequency = 0;
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_OFFSET)
> +		arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);
> +	else
> +		arg->timestamp_offset = 0;
> +
> +	if (minimize_interruption) {
> +		preempt_disable();
> +		local_irq_save(irq_flags);
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
> +		query_start_time = local_clock();
> +	else
> +		query_start_time = 0;
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU)
> +		arg->current_timestamp = gpu_read64_counter(ptdev, GPU_TIMESTAMP);
> +	else
> +		arg->current_timestamp = 0;
> +
> +	switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> +		ktime_get_ts64(&cpu_ts);
> +		break;
> +	case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> +		ktime_get_raw_ts64(&cpu_ts);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT)
> +		arg->cycle_count = gpu_read64_counter(ptdev, GPU_CYCLE_COUNT);
> +	else
> +		arg->cycle_count = 0;
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
> +		arg->duration_nsec = local_clock() - query_start_time;
> +	else
> +		arg->duration_nsec = 0;
> +
> +	if (minimize_interruption) {
> +		local_irq_restore(irq_flags);
> +		preempt_enable();
> +	}
> +
> +	if (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> +		timens_add_monotonic(&cpu_ts);
> +
> +		arg->cpu_timestamp_sec = cpu_ts.tv_sec;
> +		arg->cpu_timestamp_nsec = cpu_ts.tv_nsec;
> +	} else {
> +		arg->cpu_timestamp_sec = 0;
> +		arg->cpu_timestamp_nsec = 0;
> +	}
>  
>  	pm_runtime_put(ptdev->base.dev);
>  	return 0;
> @@ -851,8 +966,14 @@ static int panthor_ioctl_dev_query(struct drm_device *ddev, void *data, struct d
>  		return PANTHOR_UOBJ_SET(args->pointer, args->size, ptdev->csif_info);
>  
>  	case DRM_PANTHOR_DEV_QUERY_TIMESTAMP_INFO:
> -		ret = panthor_query_timestamp_info(ptdev, &timestamp_info);
> +		ret = copy_struct_from_user(&timestamp_info,
> +					    sizeof(timestamp_info),
> +					    u64_to_user_ptr(args->pointer),
> +					    args->size);
> +		if (ret)
> +			return ret;
>  
> +		ret = panthor_query_timestamp_info(ptdev, &timestamp_info);
>  		if (ret)
>  			return ret;
>  
> @@ -1680,6 +1801,7 @@ static void panthor_debugfs_init(struct drm_minor *minor)
>   *       - adds DRM_IOCTL_PANTHOR_BO_SYNC ioctl
>   *       - adds DRM_IOCTL_PANTHOR_BO_QUERY_INFO ioctl
>   *       - adds drm_panthor_gpu_info::selected_coherency
> + * - 1.8 - extends DEV_QUERY_TIMESTAMP_INFO with flags
>   */
>  static const struct drm_driver panthor_drm_driver = {
>  	.driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ |
> @@ -1693,7 +1815,7 @@ static const struct drm_driver panthor_drm_driver = {
>  	.name = "panthor",
>  	.desc = "Panthor DRM driver",
>  	.major = 1,
> -	.minor = 7,
> +	.minor = 8,
>  
>  	.gem_create_object = panthor_gem_create_object,
>  	.gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table,
> diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> index b401ac585d6a..0e455d91e77d 100644
> --- a/include/uapi/drm/panthor_drm.h
> +++ b/include/uapi/drm/panthor_drm.h
> @@ -409,6 +409,38 @@ struct drm_panthor_csif_info {
>  	__u32 pad;
>  };
>  
> +/**
> + * enum drm_panthor_timestamp_info_flags - drm_panthor_timestamp_info.flags
> + */
> +enum drm_panthor_timestamp_info_flags {
> +	/** @DRM_PANTHOR_TIMESTAMP_GPU: Query GPU time. */
> +	DRM_PANTHOR_TIMESTAMP_GPU = 1 << 0,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_NONE: Don't query CPU time. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_NONE = 0 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC: Query CPU time using CLOCK_MONOTONIC. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC = 1 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW: Query CPU time using CLOCK_MONOTONIC_RAW. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW = 2 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK: Space reserved for CPU clock type. */
> +	DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK = 7 << 1,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_GPU_OFFSET: Query GPU offset. */
> +	DRM_PANTHOR_TIMESTAMP_GPU_OFFSET = 1 << 4,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT: Query GPU cycle count. */
> +	DRM_PANTHOR_TIMESTAMP_GPU_CYCLE_COUNT = 1 << 5,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_FREQ: Query timestamp frequency. */
> +	DRM_PANTHOR_TIMESTAMP_FREQ = 1 << 6,
> +
> +	/** @DRM_PANTHOR_TIMESTAMP_DURATION: Return duration of time query. */
> +	DRM_PANTHOR_TIMESTAMP_DURATION = 1 << 7,
> +};
> +
>  /**
>   * struct drm_panthor_timestamp_info - Timestamp information
>   *
> @@ -421,11 +453,38 @@ 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.
> +	 *
> +	 * If set to 0, then it is interpreted as:
> +	 *  DRM_PANTHOR_TIMESTAMP_GPU |
> +	 *  DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> +	 *  DRM_PANTHOR_TIMESTAMP_FREQ
> +	 *
> +	 * Note: these flags are exclusive to each other (only one can be used):
> +	 * - DRM_PANTHOR_TIMESTAMP_CPU_NONE
> +	 * - DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC
> +	 * - DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW
> +	 */
> +	__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. */
> +	__u64 cpu_timestamp_nsec;
>  };
>  
>  /**
> -- 
> 2.34.1
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯