On Wed, 18 Mar 2026 15:20:18 +0000 Steven Price <[email protected]> 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 <[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. > > > > 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?).
