On Thu, Mar 19, 2026 at 03:17:36PM +0000, Liviu Dudau wrote:
> On Thu, Mar 19, 2026 at 01:39:40PM +0100, Marcin Ślusarz wrote:
> > 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.
>
> The if () test eliminates the default case, and if you change the switch to:
>
> switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_NONE) {
> case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC:
> case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW:
> timestamp_types++;
> break;
> }
>
> then it should be equivalent, right?
We need the default case to detect garbage values in the part of flags
that ands with DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK.
DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK is 7 << 1,
DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC is 1 << 1,
DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW is 2 << 1,
so 3 << 1, 4 << 1, 5 << 1, 6 << 1, 7 << 1 are all invalid values that need
to be rejected.
And since DRM_PANTHOR_TIMESTAMP_CPU_NONE is 0 << 1, we need it too in
the switch to not be caught by the default case.
> >
> > > > +
> > > > + 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
>
> Yes, looks good to me.
>
> Best regards,
> Liviu
>
> > */
> > __u32 flags;
> >
>
> --
> ====================
> | I would like to |
> | fix the world, |
> | but they're not |
> | giving me the |
> \ source code! /
> ---------------
> ¯\_(ツ)_/¯
