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

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

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?

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