Re: [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters

[Steven Price <steven.price@arm.com>]

On 07/08/2024 17:08, Mary Guillemard wrote:
> Expose system timestamp and frequency supported by the GPU.
> 
> Mali uses the generic arch timer as GPU system time so we currently
> wire cntvct_el0 and cntfrq_el0 respectively to those parameters.
> We could have directly read those values from userland but handling
> this here allows us to be future proof in case of errata related to
> timers for example.

I'm not sure what the benefit is for the kernel driver providing these
(and only on some systems)? The user space driver is still going to need
code to deal with the problematic systems.

> This new uAPI will be used in Mesa to implement timestamp queries and
> VK_KHR_calibrated_timestamps.

VK_KHR_calibrated_timestamps says it should query 'quasi simultaneously
from two time domains' - but this is purely providing an interface to
read a single counter at a time.

It _may_ be useful to report the GPU's view of time and at the same time
(or as near as possible) the architectural counters. That can be used to
deal with drift between the GPU's counters and arch counters[1].

In general we try to avoid providing an interface to something which is
unrelated to the GPU, especially when user space already has a mechanism.

Steve

[1] See Mihail's response to the panthor patches for details of
differences that might occur.

> Signed-off-by: Mary Guillemard <mary.guillemard@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c | 17 +++++++++++++++++
>  include/uapi/drm/panfrost_drm.h         |  2 ++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 671eed4ad890..d94c9bf5a7f9 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -69,6 +69,23 @@ static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct
>  		PANFROST_FEATURE_ARRAY(JS_FEATURES, js_features, 15);
>  		PANFROST_FEATURE(NR_CORE_GROUPS, nr_core_groups);
>  		PANFROST_FEATURE(THREAD_TLS_ALLOC, thread_tls_alloc);
> +
> +	case DRM_PANFROST_PARAM_SYSTEM_TIMESTAMP:
> +#ifdef CONFIG_ARM_ARCH_TIMER
> +		param->value = __arch_counter_get_cntvct();
> +#else
> +		param->value = 0;
> +#endif
> +		break;
> +
> +	case DRM_PANFROST_PARAM_SYSTEM_TIMESTAMP_FREQUENCY:
> +#ifdef CONFIG_ARM_ARCH_TIMER
> +		param->value = arch_timer_get_cntfrq();
> +#else
> +		param->value = 0;
> +#endif
> +		break;
> +
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
> index 9f231d40a146..52b050e2b660 100644
> --- a/include/uapi/drm/panfrost_drm.h
> +++ b/include/uapi/drm/panfrost_drm.h
> @@ -172,6 +172,8 @@ enum drm_panfrost_param {
>  	DRM_PANFROST_PARAM_NR_CORE_GROUPS,
>  	DRM_PANFROST_PARAM_THREAD_TLS_ALLOC,
>  	DRM_PANFROST_PARAM_AFBC_FEATURES,
> +	DRM_PANFROST_PARAM_SYSTEM_TIMESTAMP,
> +	DRM_PANFROST_PARAM_SYSTEM_TIMESTAMP_FREQUENCY,
>  };
>  
>  struct drm_panfrost_get_param {