On Thu, 19 Mar 2026 09:25:51 +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]> > --- > 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 <[email protected]> > --- > 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, ×tamp_info); > + ret = copy_struct_from_user(×tamp_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, ×tamp_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; > }; > > /**
