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
