Re: [PATCH v3] drm/panthor: extend timestamp query with flags

[Liviu Dudau <liviu.dudau@arm.com>]

On Mon, Mar 23, 2026 at 05:12:01PM +0100, Marcin Ślusarz wrote:
> On Mon, Mar 23, 2026 at 01:16:30PM +0000, Liviu Dudau wrote:
> > On Thu, Mar 19, 2026 at 04:33:48PM +0100, Marcin Ślusarz wrote:
> > > 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.
> > 
> > OK, are you going to re-spin?
> 
> Yes, as soon as we get to some conclusion.
> 
> > > > > 
> > > > > > > +		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,
> > 
> > I understand that you want to make sure that user space doesn't insert values into the
> > flags and then pretends that because we didn't return an error that's now part of the
> > ABI. But then maybe you should not reserve the extra bit now if you don't know how
> > it's going to be used (in other words, why not make TIMESTAMP_CPU_TYPE_MASK  3 << 1?).
> 
> I think it makes sense to reserve this bit now to make it easier to
> extend the interface if we ever will need to. Changing uapi definition
> is painful enough, so reserving this will rule out misuses of the current
> value. Another point is that we would have to validate that the bit is 0
> anyway, so I don't see why we can't include that bit in the field that
> is supposed to go with.

You do validate the bits in the if (flags & ~VALID_TIMESTAMP_QUERY_FLAGS) test if the
QUERY_FLAGS bitfield is updated.

> 
> > > 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.
> > 
> > I am confused about why TIMESTAMP_CPU is a continous range [0-7] while the rest of the
> > flags are bits in a bitmask. A note should be added to the panthor_drm.h file to
> > explain this.
> 
> Well, just look at the definition of struct drm_panthor_timestamp_info -
> there's one to one relation between field and a DRM_PANTHOR_TIMESTAMP_* flag,
> with an exception that CPU timestamp types have only one field (well,
> a pair of "seconds" and "nanoseconds", but from logical perspective it's
> only one value). There's no known reason why anyone would want to query
> multiple CPU timestamps together with various GPU counters from _GPU_ query.

OK, so what I'm hearing is: we're reserving space for up to 6 CPU clock sources (zero is
no CPU clock) but you can have only one active at any time (because we can't think of any
reason why you would want more at the same time) and we currently only define two sources
because that's what user space cares about.

If you agree with this then this will be recorded in the thread and we can go back in the
future and reference it.

> 
> > 
> > > 
> > > 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.
> > 
> > It only makes sense if you explain that TIMESTAMP_CPU values are in a range. I kept reading
> > the different versions of the patch thinking that they are bits in a bitmask, with
> > TIMESTAMP_CPU_NONE being the logical state of not selecting any of the sources.
> 
> That should be resolved with the documentation update below, right?

The documentation doesn't say anything about the intent for future values for CPU sources, but
if you don't want to add more in the documentation at least we have this thread as reference.

I don't want to turn this into a bikeshedding thread, so please do a v4 with the switch case updated
and the documentation addition and I will ACK that.

Best regards,
Liviu

> 
> > 
> > Best regards,
> > Liviu
> > 
> > 
> > > 
> > > > > 
> > > > > > > +
> > > > > > > +	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!  /
  ---------------
    ¯\_(ツ)_/¯