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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aOOQBHX7sKrqx7Sv@e129842.arm.com>
Date: Mon, 6 Oct 2025 11:46:44 +0200
From: Marcin Ślusarz <marcin.slusarz@....com>
To: Chia-I Wu <olvaffe@...il.com>
Cc: Boris Brezillon <boris.brezillon@...labora.com>,
	Steven Price <steven.price@....com>,
	Liviu Dudau <liviu.dudau@....com>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
	Grant Likely <grant.likely@...aro.org>,
	Heiko Stuebner <heiko@...ech.de>, dri-devel@...ts.freedesktop.org,
	linux-kernel@...r.kernel.org, nd@....com,
	Lukas Zapolskas <lukas.zapolskas@....com>
Subject: Re: [PATCH] drm/panthor: add query for calibrated timstamp info

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@....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