[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zs7d2-NHTy-A_oLL@kuroko.kudu-justice.ts.net>
Date: Wed, 28 Aug 2024 10:24:40 +0200
From: Mary Guillemard <mary.guillemard@...labora.com>
To: Mihail Atanassov <mihail.atanassov@....com>
Cc: linux-kernel@...r.kernel.org, kernel@...labora.com,
Christopher Healy <healych@...zon.com>,
Boris Brezillon <boris.brezillon@...labora.com>,
Steven Price <steven.price@....com>,
Liviu Dudau <liviu.dudau@....com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
dri-devel@...ts.freedesktop.org, nd@....com
Subject: Re: [PATCH v3] drm/panthor: Add DEV_QUERY_TIMESTAMP_INFO dev query
Hi Mihail,
On Mon, Aug 19, 2024 at 03:25:48PM +0100, Mihail Atanassov wrote:
> > diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> > index 926b1deb1116..944f5618aa05 100644
> > --- a/include/uapi/drm/panthor_drm.h
> > +++ b/include/uapi/drm/panthor_drm.h
> > @@ -260,6 +260,9 @@ enum drm_panthor_dev_query_type {
> > /** @DRM_PANTHOR_DEV_QUERY_CSIF_INFO: Query command-stream interface information. */
> > DRM_PANTHOR_DEV_QUERY_CSIF_INFO,
> > +
> > + /** @DRM_PANTHOR_DEV_QUERY_TIMESTAMP_INFO: Query timestamp information. */
> > + DRM_PANTHOR_DEV_QUERY_TIMESTAMP_INFO,
> > };
> > /**
> > @@ -377,6 +380,22 @@ struct drm_panthor_csif_info {
> > __u32 pad;
> > };
> > +/**
> > + * struct drm_panthor_timestamp_info - Timestamp information
> > + *
> > + * Structure grouping all queryable information relating to the GPU timestamp.
> > + */
> > +struct drm_panthor_timestamp_info {
> > + /** @timestamp_frequency: The frequency of the timestamp timer. */
> > + __u64 timestamp_frequency;
> > +
> > + /** @current_timestamp: The current timestamp. */
> > + __u64 current_timestamp;
> > +
> > + /** @timestamp_offset: The offset of the timestamp timer. */
> > + __u64 timestamp_offset;
>
> Are you sure you don't want the cntvct_el0 and cntfrq_el0 values here, too,
> as separate fields? While they're not values coming from the GPU, they are
> related. Getting the lowest delta possible between querying one timestamp
> register and the other is quite useful for tooling. Requiring userspace to
> do two separate queries, one for CPU time and one for GPU time, doesn't feel
> optimal, your max deviation will necessarily be higher that way.
>
Exposing arch dependent values in the uapi feels wrong to me too but
maybe that could be done with just the cpu timestamp with a frequency of
1 on other platform if needed?
In any cases, I think that if we ever need it we can extend this query
with more values. (as we are retrocompatible thanks to PANTHOR_UOBJ_SET)
--
Mary
Powered by blists - more mailing lists