On Thu, Mar 19, 2026 at 11:43:45AM +0000, Liviu Dudau wrote:
> Hi Marcin,
>
> On Thu, Mar 19, 2026 at 12:00:53PM +0100, Marcin Slusarz wrote:
> > ...
> > +#define VALID_TIMESTAMP_QUERY_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)
> > +
> > 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 {
> > + /*
> > + * If flags are 0, then ask for the same things that we asked
> > + * for before flags were added.
> > + */
> > + flags = DRM_PANTHOR_TIMESTAMP_GPU |
> > + DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
> > + DRM_PANTHOR_TIMESTAMP_FREQ;
> > + }
> > +
> > + switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) {
> > + case 0:
Umm, this should be DRM_PANTHOR_TIMESTAMP_CPU_NONE.
> > + break;
> > + case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> > + case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> > + timestamp_types++;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + if (flags & ~VALID_TIMESTAMP_QUERY_FLAGS)
> > + return -EINVAL;
>
> Can we move this check before the switch and simplify the switch itself to
> only do the timestamp_types increment?
DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK is bit field that holds individual
clock type values, so we still need to validate the bit field.
> > +
> > + 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.
> > + */
>
> This comment makes me think that user can ask for both CPU_MONOTONIC and
> CPU_MONOTONIC_RAW timestamps, but the code is built to make them exclusive.
> Can we document better what sources can be requested simultaneously?
Somethine like this?
diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
index 8a46ef040c3d..0e455d91e77d 100644
--- a/include/uapi/drm/panthor_drm.h
+++ b/include/uapi/drm/panthor_drm.h
@@ -466,6 +466,11 @@ struct drm_panthor_timestamp_info {
* DRM_PANTHOR_TIMESTAMP_GPU |
* DRM_PANTHOR_TIMESTAMP_GPU_OFFSET |
* DRM_PANTHOR_TIMESTAMP_FREQ
+ *
+ * Note: these flags are exclusive to each other (only one can be used):
+ * - DRM_PANTHOR_TIMESTAMP_CPU_NONE
+ * - DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC
+ * - DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW
*/
__u32 flags;
