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

[Marcin Ślusarz <marcin.slusarz@arm.com>]

On Tue, Mar 24, 2026 at 10:41:56AM +0000, Liviu Dudau wrote:
> 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.

Up to 7 CPU clock sources and 0 as no CPU clock, but yes, this is the intent.

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

It's not that I don't want to add more documentation, I just don't
understand what else needs to be explained.

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

Done