Re: [PATCH] drm/panthor: Add DEV_QUERY_TIMESTAMP_INFO dev query

[Boris Brezillon <boris.brezillon@collabora.com>]

Hi Mihail,

On Thu, 8 Aug 2024 12:41:05 +0300
Mihail Atanassov <mihail.atanassov@arm.com> wrote:

> > 
> > +/** + * 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;  
> 
> As it stands, this query has nothing to do with the actual GPU so
> doesn't really belong here.
> 
> It'd be more valuable, and can maybe give better calibration results
> than querying the system timestamp separately in userspace, if you
> reported all of:
>   * the system timer value
>   * the system timer frequency
>   * the GPU timer value
>   * the GPU timer frequency (because it _could_ be different in some 
> systems)

Duh, I wish this wasn't the case and all SoC vendors went for the
arch-timer which guarantees the consistency of the timestamp on the GPU
and CPU. But let's say this is a case we need to support, wouldn't it
be more useful to do the CPU/GPU calibration kernel side (basically at
init/resume time) and then expose the formula describing the
relationship between those 2 things:

CPU_time = GPU_time * GPU_to_CPU_mul / GPU_to_CPU_div +
	   GPU_to_CPU_offset;

>   * the GPU timer offset

Assuming you're talking about the TIMESTAMP_OFFSET register, my
understanding is that this offset should be set by the kernel driver to
account for any disjoint caused by suspend/resume cycles, or any
design-specific offset between the arch-timer and the timer feeding the
GPU timestamp block (hopefully the arch-timer is directly propagated to
the GPU though). The timestamp read by the GPU/CPU already has this
offset added, so I'm not sure I understand what's the value of exposing
it to userspace. As long as the CPU/GPU timestamps are consistent,
userspace probably doesn't care, but I might be missing something.

> 
> > +}; + /** * struct drm_panthor_dev_query - Arguments passed to
> > DRM_PANTHOR_IOCTL_DEV_QUERY */
> > 
> > base-commit: f7f3ddb6e5c8dc7b621fd8c0903ea42190d67452  
>