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

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

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

[Claude Code Review Bot]

Patch Review

This is the latest version with Boris's Reviewed-by. Detailed review:

**Bug: `timens_add_monotonic` applied to `CLOCK_MONOTONIC_RAW`**

```c
	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;
```

`timens_add_monotonic()` adds the time namespace monotonic offset (from `include/linux/time_namespace.h:78`):
```c
	*ts = timespec64_add(*ts, ns_offsets->monotonic);
```

This is **incorrect** when `DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW` is selected. `CLOCK_MONOTONIC_RAW` is explicitly immune to time namespace adjustments — that's its defining characteristic versus `CLOCK_MONOTONIC`. The kernel's own `posix-timers.c` and `clock_gettime` paths do not apply timens offsets to `CLOCK_MONOTONIC_RAW`. This should be:

```c
	if ((flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) ==
	     DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC) {
		timens_add_monotonic(&cpu_ts);
	}
```

Or simply skip `timens_add_monotonic` for the RAW case.

**Minor: `duration_nsec` is `__u32`**

```c
	/** @duration_nsec: Duration of time query. */
	__u32 duration_nsec;
```

A `__u32` can hold ~4.29 seconds of nanoseconds, which is plenty for this use case (a few register reads). However, `local_clock()` returns `u64`, and the subtraction `local_clock() - query_start_time` produces a `u64` that gets silently truncated:

```c
		arg->duration_nsec = local_clock() - query_start_time;
```

This is practically fine since the query should complete in microseconds, but documenting the maximum representable duration or using a cast to make the truncation explicit would be cleaner.

**Observation: preempt_disable + local_irq_save ordering**

```c
	if (minimize_interruption) {
		preempt_disable();
		local_irq_save(irq_flags);
	}
```

This ordering is fine — `local_irq_save` implicitly disables preemption, so the explicit `preempt_disable` first is redundant but harmless. The restore order (irq_restore then preempt_enable) is correct.

**Observation: `ktime_get_ts64` with IRQs disabled**

Calling `ktime_get_ts64()` and `ktime_get_raw_ts64()` with interrupts disabled is safe — they use seqcount-based reads of the timekeeping data and don't sleep or take locks that require IRQs.

**Style: "Nanseconds" typo**

```c
	/** @cpu_timestamp_nsec: Nanseconds part of CPU timestamp. */
```

Should be "Nanoseconds".

**Observation: backward compatibility is correct**

The `copy_struct_from_user` + flags=0 default approach works well:
- Old userspace passes `args->size` = 24 (original struct size)
- `copy_struct_from_user` zero-fills the new fields including `flags`
- `flags == 0` triggers the default path returning the same data as before
- `PANTHOR_UOBJ_SET` copies back only 24 bytes, omitting new fields
- Old userspace is unaware of the extension

**Observation: CPU_TYPE_MASK reserves 3 bits (values 0-7) but only uses 0-2**

This is good forward-thinking design, reserving space for future CPU clock types (e.g., `CLOCK_BOOTTIME` could be value 3).

**Summary for v4**: The patch is well-designed with one real bug (`timens_add_monotonic` applied unconditionally to both MONOTONIC and MONOTONIC_RAW) and one typo. The backward compatibility approach is sound and the IRQ-disable section for timestamp correlation is appropriate. The `timens_add_monotonic` bug should be fixed before merging.

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

This is the current version. Review follows:

**1. Typo in UAPI header**

```c
/** @cpu_timestamp_nsec: Nanseconds part of CPU timestamp. */
```
Should be "Nanoseconds".

**2. `duration_nsec` is `__u32` but assigned from `u64` subtraction**

```c
	if (flags & DRM_PANTHOR_TIMESTAMP_DURATION)
		arg->duration_nsec = local_clock() - query_start_time;
```

`local_clock()` returns `u64`, so the subtraction result is `u64` and gets silently truncated to `__u32`. While a u32 in nanoseconds (~4.2 seconds) is more than enough for a few MMIO reads, the truncation should be documented in the UAPI header comment for `duration_nsec`, or alternatively consider making it `__u64` for future-proofing since there's space (the struct has a `__u64 cpu_timestamp_nsec` right after, so alignment works either way).

**3. `gpu_read64_counter` in an IRQ-disabled, preempt-disabled section contains a retry loop**

```c
static inline u64 gpu_read64_counter(struct panthor_device *ptdev, u32 reg)
{
	u32 lo, hi1, hi2;
	do {
		hi1 = gpu_read(ptdev, reg + 4);
		lo = gpu_read(ptdev, reg);
		hi2 = gpu_read(ptdev, reg + 4);
	} while (hi1 != hi2);
	return lo | ((u64)hi2 << 32);
}
```

The patch calls this up to twice (once for `GPU_TIMESTAMP`, once for `GPU_CYCLE_COUNT`) inside `local_irq_save`/`preempt_disable`. The retry loop is theoretically unbounded. In practice the high word changes extremely rarely, so this is very unlikely to be an issue, but worth noting.

**4. Ordering: `gpu_read64` for `GPU_TIMESTAMP_OFFSET` is done outside the critical section**

```c
	if (flags & DRM_PANTHOR_TIMESTAMP_GPU_OFFSET)
		arg->timestamp_offset = gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET);

	if (minimize_interruption) {
		preempt_disable();
		local_irq_save(irq_flags);
	}
```

The `GPU_TIMESTAMP_OFFSET` read happens before disabling preemption/IRQs. If userspace requests both `GPU_OFFSET` and `GPU` timestamp in the same query, the offset and the timestamp might not be perfectly correlated. This is probably fine since the offset is presumably static or slow-changing, but the design rationale should be documented.

**5. `timens_add_monotonic` called for `MONOTONIC_RAW` — correct but potentially surprising**

```c
	if (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
		timens_add_monotonic(&cpu_ts);
		...
	}
```

This applies the time namespace monotonic offset to both `CLOCK_MONOTONIC` and `CLOCK_MONOTONIC_RAW` timestamps. This matches the kernel's own `posix_get_monotonic_raw()` behavior in `kernel/time/posix-timers.c`, so it's correct. However, a brief comment here would help readers understand this is intentional and follows kernel convention.

**6. Missing `PANTHOR_UOBJ_MIN_SIZE` update consideration**

The `PANTHOR_UOBJ_MIN_SIZE` for `drm_panthor_timestamp_info` remains anchored at `current_timestamp` (line 175 of the existing code). Since the struct is extended with new fields after `timestamp_offset`, old userspace that passes `size = 24` (the old struct size) will:
- `copy_struct_from_user`: copy 24 bytes, zero-fill the rest → `flags == 0` → legacy path. Correct.
- `PANTHOR_UOBJ_SET`: copy back `min(24, new_size) = 24` bytes. Correct.

This works, but the `PANTHOR_UOBJ_MIN_SIZE` min-size is currently set to end at `current_timestamp` (offset 16), not `timestamp_offset` (offset 24). This means userspace could theoretically pass a 16-byte struct and get no `timestamp_offset` back. This was already the case before this patch, so not a regression — just noting the pre-existing behavior.

**7. Multi-line comment style**

```c
	/* 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.
```

Kernel comment style for multi-line comments prefers:
```c
	/*
	 * 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.
```

**8. `ktime_get_ts64` / `ktime_get_raw_ts64` called with IRQs disabled**

These functions are safe to call with IRQs disabled (they use seqcount-based lockless reads), so this is fine.

**9. No `default` case in the CPU type switch inside the critical section**

```c
	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;
	}
```

v3 correctly added `default: break;` here (v2 was missing it). Good.

**10. No validation that args->size is large enough for copy_struct_from_user before powering up GPU**

In the current flow, `copy_struct_from_user` succeeds even for very small `args->size` (zero-fills the rest), then `panthor_query_timestamp_info` powers up the GPU and does MMIO reads, and only then `PANTHOR_UOBJ_SET` would reject the small `usr_size`. This matches the pre-existing behavior (the old code also powered up the GPU before the size check in PANTHOR_UOBJ_SET), so it's not a regression. Minor optimization opportunity for future work.

**Summary of actionable items:**
1. Fix "Nanseconds" → "Nanoseconds" typo
2. Consider making `duration_nsec` `__u64` or document truncation behavior
3. Fix multi-line comment style to match kernel convention
4. Add a brief comment explaining `timens_add_monotonic` applies to both clock types

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

Superseded by v3. Remaining issue fixed in v3:
- Used `(const void __user __force *) args->pointer` cast — fixed to `u64_to_user_ptr` in v3.
- Missing braces on multi-line statements — fixed in v3.
- Missing `VALID_TIMESTAMP_QUERY_FLAGS` define — added in v3.

---
Generated by Claude Code Patch Reviewer

[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!  /
  ---------------
    ¯\_(ツ)_/¯

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

Claude review: drm/panthor: extend timestamp query with flags

[Claude Code Review Bot]

Overall Series Review

Subject: drm/panthor: extend timestamp query with flags
Author: Marcin Slusarz <marcin.slusarz@arm.com>
Patches: 16
Reviewed: 2026-03-22T04:24:21.326403

---

This mbox contains three versions (v1, v2, v3) of the same patch: "drm/panthor: extend timestamp query with flags". The patch extends the `DRM_PANTHOR_DEV_QUERY_TIMESTAMP_INFO` ioctl to support flags that let userspace selectively request GPU timestamps, CPU timestamps (CLOCK_MONOTONIC or CLOCK_MONOTONIC_RAW), GPU cycle counts, and query duration measurement. The motivation is to improve `VK_KHR_calibrated_timestamps` support. I'll focus the detailed review on **v3** as the latest version, with notes on the evolution.

The overall approach is reasonable — extending an existing query struct with flags and new fields while maintaining backward compatibility. However, there are several concerns around the UAPI design, the `timens_add_monotonic` usage with interrupts disabled, and the `copy_struct_from_user` interaction with panthor's existing UOBJ infrastructure.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/panthor: extend timestamp query with flags

[Claude Code Review Bot]

Patch Review

Superseded by v2/v3. Key issues that were fixed in later versions:
- Used raw `copy_from_user` instead of `copy_struct_from_user` — fixed in v2.
- Used `(const void __user __force *) args->pointer` cast instead of `u64_to_user_ptr` — fixed in v3.
- Had a `__u32 pad1` field and `__u32 cpu_timestamp_nsec` — fixed in v2 (pad removed, nsec widened to u64).
- Passed `size` as a separate parameter to `panthor_query_timestamp_info` and used offset-based size checks — simplified in v2 to use flags directly.

---
Generated by Claude Code Patch Reviewer

[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 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!  /
  ---------------
    ¯\_(ツ)_/¯