| Message ID | 20240807160900.149154-2-mary.guillemard@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panfrost: Wire cycle counters and timestamp info to userspace | expand |
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 {
Hello Steve, > 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. I initially didn't saw the register to get the GPU timestamp and wrongly assumed I would have to query it from the generic arch timer. I will make SYSTEM_TIMESTAMP returns the value of the timestamp register on v2 and keep SYSTEM_TIMESTAMP_FREQUENCY the same way as it is at the moment. Thanks for the review, Mary
Hi Mary, kernel test robot noticed the following build errors: [auto build test ERROR on f7f3ddb6e5c8dc7b621fd8c0903ea42190d67452] url: https://github.com/intel-lab-lkp/linux/commits/Mary-Guillemard/drm-panfrost-Add-SYSTEM_TIMESTAMP-and-SYSTEM_TIMESTAMP_FREQUENCY-parameters/20240808-032938 base: f7f3ddb6e5c8dc7b621fd8c0903ea42190d67452 patch link: https://lore.kernel.org/r/20240807160900.149154-2-mary.guillemard%40collabora.com patch subject: [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters config: arm-defconfig (https://download.01.org/0day-ci/archive/20240808/202408082014.XKxle32n-lkp@intel.com/config) compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240808/202408082014.XKxle32n-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202408082014.XKxle32n-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/gpu/drm/panfrost/panfrost_drv.c:75:18: error: implicit declaration of function '__arch_counter_get_cntvct' is invalid in C99 [-Werror,-Wimplicit-function-declaration] param->value = __arch_counter_get_cntvct(); ^ >> drivers/gpu/drm/panfrost/panfrost_drv.c:83:18: error: implicit declaration of function 'arch_timer_get_cntfrq' is invalid in C99 [-Werror,-Wimplicit-function-declaration] param->value = arch_timer_get_cntfrq(); ^ 2 errors generated. vim +/__arch_counter_get_cntvct +75 drivers/gpu/drm/panfrost/panfrost_drv.c 26 27 static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct drm_file *file) 28 { 29 struct drm_panfrost_get_param *param = data; 30 struct panfrost_device *pfdev = ddev->dev_private; 31 32 if (param->pad != 0) 33 return -EINVAL; 34 35 #define PANFROST_FEATURE(name, member) \ 36 case DRM_PANFROST_PARAM_ ## name: \ 37 param->value = pfdev->features.member; \ 38 break 39 #define PANFROST_FEATURE_ARRAY(name, member, max) \ 40 case DRM_PANFROST_PARAM_ ## name ## 0 ... \ 41 DRM_PANFROST_PARAM_ ## name ## max: \ 42 param->value = pfdev->features.member[param->param - \ 43 DRM_PANFROST_PARAM_ ## name ## 0]; \ 44 break 45 46 switch (param->param) { 47 PANFROST_FEATURE(GPU_PROD_ID, id); 48 PANFROST_FEATURE(GPU_REVISION, revision); 49 PANFROST_FEATURE(SHADER_PRESENT, shader_present); 50 PANFROST_FEATURE(TILER_PRESENT, tiler_present); 51 PANFROST_FEATURE(L2_PRESENT, l2_present); 52 PANFROST_FEATURE(STACK_PRESENT, stack_present); 53 PANFROST_FEATURE(AS_PRESENT, as_present); 54 PANFROST_FEATURE(JS_PRESENT, js_present); 55 PANFROST_FEATURE(L2_FEATURES, l2_features); 56 PANFROST_FEATURE(CORE_FEATURES, core_features); 57 PANFROST_FEATURE(TILER_FEATURES, tiler_features); 58 PANFROST_FEATURE(MEM_FEATURES, mem_features); 59 PANFROST_FEATURE(MMU_FEATURES, mmu_features); 60 PANFROST_FEATURE(THREAD_FEATURES, thread_features); 61 PANFROST_FEATURE(MAX_THREADS, max_threads); 62 PANFROST_FEATURE(THREAD_MAX_WORKGROUP_SZ, 63 thread_max_workgroup_sz); 64 PANFROST_FEATURE(THREAD_MAX_BARRIER_SZ, 65 thread_max_barrier_sz); 66 PANFROST_FEATURE(COHERENCY_FEATURES, coherency_features); 67 PANFROST_FEATURE(AFBC_FEATURES, afbc_features); 68 PANFROST_FEATURE_ARRAY(TEXTURE_FEATURES, texture_features, 3); 69 PANFROST_FEATURE_ARRAY(JS_FEATURES, js_features, 15); 70 PANFROST_FEATURE(NR_CORE_GROUPS, nr_core_groups); 71 PANFROST_FEATURE(THREAD_TLS_ALLOC, thread_tls_alloc); 72 73 case DRM_PANFROST_PARAM_SYSTEM_TIMESTAMP: 74 #ifdef CONFIG_ARM_ARCH_TIMER > 75 param->value = __arch_counter_get_cntvct(); 76 #else 77 param->value = 0; 78 #endif 79 break; 80 81 case DRM_PANFROST_PARAM_SYSTEM_TIMESTAMP_FREQUENCY: 82 #ifdef CONFIG_ARM_ARCH_TIMER > 83 param->value = arch_timer_get_cntfrq(); 84 #else 85 param->value = 0; 86 #endif 87 break; 88 89 default: 90 return -EINVAL; 91 } 92 93 return 0; 94 } 95
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 {
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. This new uAPI will be used in Mesa to implement timestamp queries and VK_KHR_calibrated_timestamps. 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(+)