On Wed, 18 Mar 2026 12:29:52 +0100 Marcin Slusarz <[email protected]> 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 <[email protected]> > --- > This is counter proposal to > https://lore.kernel.org/all/[email protected]/ > --- > 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, ×tamp_info); > + if (copy_from_user(×tamp_info, > + (const void __user __force *) args->pointer, > + args->size)) > + return -EFAULT; > + > + ret = panthor_query_timestamp_info(ptdev, ×tamp_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. > }; > > /**
