linux-kernel - Re: [PATCH v3] drm/panthor: Add DEV_QUERY_TIMESTAMP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ee2c315c-b15b-4ac7-8cea-c331a60a3374@arm.com>
Date: Wed, 28 Aug 2024 11:09:09 +0100
From: Mihail Atanassov <mihail.atanassov@....com>
To: Mary Guillemard <mary.guillemard@...labora.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 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@....com>

-- 
Mihail Atanassov <mihail.atanassov@....com>