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

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

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?

> > > 
> > > > > +		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?).

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

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

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!  /
> >   ---------------
> >     ¯\_(ツ)_/¯

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯