Hi Mary,
On 28/08/2024 09:24, Mary Guillemard wrote:
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. */
Sorry, late nitpick here, maybe add "or 0 if unknown" or something similar?
+ __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)
Agreed, we can leave that for later.
--
Mary
Do you have a link you can share with the userspace side of things?
Assuming userland has been reviewed and is ready for merging, and the
small nitpick above fixed,
Reviewed-by: Mihail Atanassov <mihail.atanassov@xxxxxxx>
--
Mihail Atanassov <mihail.atanassov@xxxxxxx>
