Re: [PATCH] drm/panthor: add query for calibrated timstamp info

["Marcin Ślusarz" <marcin.slusarz@arm.com>]

On Thu, Oct 02, 2025 at 06:10:11PM -0700, Chia-I Wu wrote:
> On Thu, Sep 25, 2025 at 2:06 AM Marcin Ślusarz <marcin.slusarz@arm.com> wrote:
> ...
> > Backward compatibility was achieved by adding new fields at the end of
> > struct drm_panthor_timestamp_info, and relying on the fact that if user
> > space passes smaller object it will be silently truncated.
> I chose a new query because userspace does not zero-initialize
> drm_panthor_timestamp_info. We will get garbage if we add an input
> field to the struct.

Kernel knows the size of the struct that userspace passed, so to hit
this user space would have to use the new header with the old code,
which AFAIK isn't possible because mesa imports Panthor UAPI header.
So either we'll get old struct with small size, or new struct with new
size and all fields properly initialized.

> 
> But this is a non-issue if we agree to do it this way, and make sure
> userspace zero-initialize before it updates the uapi header.
> 
> >
> > Obtaining all kind of timing information with a single syscall might
> > be a bit too much, when user space might be interested only in some
> > data and not the complete view, so I'd propose this as a solution:
> >
> > 1) Extend existing query in backward compatible manner, by adding new
> > fields at the end.
> > 2) Add flags, cpu timestamp, cycle count, and duration.
> > 3) Flags would be:
> > DRM_PANTHOR_TIMESTAMP_GPU (1<<0)
> > DRM_PANTHOR_TIMESTAMP_CPU (1<<1)
> > DRM_PANTHOR_TIMESTAMP_OFFSET (1<<2)
> > DRM_PANTHOR_TIMESTAMP_FREQ (1<<3)
> > DRM_PANTHOR_TIMESTAMP_DURATION (1<<4)
> > DRM_PANTHOR_TIMESTAMP_SAME_TIME (1<<5)
> >
> > DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC (0<<8)
> > DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW (1<<8)
> > DRM_PANTHOR_TIMESTAMP_CPU_REALTIME (2<<8)
> > DRM_PANTHOR_TIMESTAMP_CPU_BOOTTIME (3<<8)
> > DRM_PANTHOR_TIMESTAMP_CPU_TAI (4<<8)
> >
> > and DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK would be (7<<8).
> >
> > If flags is 0 it would become
> > (DRM_PANTHOR_TIMESTAMP_GPU |
> >  DRM_PANTHOR_TIMESTAMP_OFFSET |
> >  DRM_PANTHOR_TIMESTAMP_FREQ)
> It is more typical to have NO_GPU/NO_OFFSET/NO_FREQ, but I think
> handling 0 specially can work too.

I mean, when userspace will pass old struct without flags, kernel will
set these 3 bits. We won't need to special case flags == 0 from user
space, because it will either be set correctly, or not existent in
the structure. "flags is 0" was a not well explained shortcut, sorry
about that.

> >
> > For VK_KHR_calibrated_timestamps flags would be set as
> > (DRM_PANTHOR_TIMESTAMP_GPU |
> >  DRM_PANTHOR_TIMESTAMP_CPU |
> >  DRM_PANTHOR_TIMESTAMP_DURATION |
> >  DRM_PANTHOR_TIMESTAMP_SAME_TIME |
> >  (raw ? DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW : DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC))
> >
> > 4) The core of the functionality would query all required timing
> > information with preemption and irqs disabled iif SAME_TIME flag is set.
> > Probably we should exclude OFFSET and FREQ from that.
> >
> > Why also interrupts disabled?
> > Recently we discovered that unrelated devices can raise interrupts for
> > so long that the assumption of timestamps being taken at the same time
> > completely breaks down (they are hundreds of microseconds apart).
> >
> > What do you think?
> I am happy to use your version. Do you plan to work on the userpsace
> change as well? Otherwise, I can update my userspace change to use
> your version as well.

No, I won't work on the user space part. Do you want me to create
kernel patch that will implement the above approach, or do you want
to do this? I can start working on that probably next week.

Cheers,
Marcin