* [PATCH 0/3] drm/panfrost: Wire cycle counters and timestamp info to userspace
@ 2024-08-07 16:08 Mary Guillemard
2024-08-07 16:08 ` [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters Mary Guillemard
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Mary Guillemard @ 2024-08-07 16:08 UTC (permalink / raw)
To: linux-kernel; +Cc: dri-devel, kernel, Mary Guillemard
Mali has hardware cycle counters and GPU timestamps available for
profiling.
This patch series adds support for cycle counters propagation and
also new timestamp info parameters.
Those new changes to the uAPI will be used in Mesa to implement
timestamp queries for OpenGL and Vulkan.
Mary Guillemard (3):
drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY
parameters
drm/panfrost: Add cycle counter job requirement
drm/panfrost: Handle JD_REQ_CYCLE_COUNT
drivers/gpu/drm/panfrost/panfrost_drv.c | 25 +++++++++++++++++++++++--
drivers/gpu/drm/panfrost/panfrost_job.c | 10 ++++++++++
include/uapi/drm/panfrost_drm.h | 3 +++
3 files changed, 36 insertions(+), 2 deletions(-)
base-commit: f7f3ddb6e5c8dc7b621fd8c0903ea42190d67452
--
2.45.2
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters 2024-08-07 16:08 [PATCH 0/3] drm/panfrost: Wire cycle counters and timestamp info to userspace Mary Guillemard @ 2024-08-07 16:08 ` Mary Guillemard 2024-08-08 10:23 ` Steven Price 2024-08-08 13:06 ` kernel test robot 2024-08-07 16:08 ` [PATCH 2/3] drm/panfrost: Add cycle counter job requirement Mary Guillemard 2024-08-07 16:08 ` [PATCH 3/3] drm/panfrost: Handle JD_REQ_CYCLE_COUNT Mary Guillemard 2 siblings, 2 replies; 9+ messages in thread From: Mary Guillemard @ 2024-08-07 16:08 UTC (permalink / raw) To: linux-kernel Cc: dri-devel, kernel, Mary Guillemard, Boris Brezillon, Rob Herring, Steven Price, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter 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(+) 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 { -- 2.45.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters 2024-08-07 16:08 ` [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters Mary Guillemard @ 2024-08-08 10:23 ` Steven Price 2024-08-08 10:46 ` Mary Guillemard 2024-08-08 13:06 ` kernel test robot 1 sibling, 1 reply; 9+ messages in thread From: Steven Price @ 2024-08-08 10:23 UTC (permalink / raw) To: Mary Guillemard, linux-kernel Cc: dri-devel, kernel, Boris Brezillon, Rob Herring, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter 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 { ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters 2024-08-08 10:23 ` Steven Price @ 2024-08-08 10:46 ` Mary Guillemard 0 siblings, 0 replies; 9+ messages in thread From: Mary Guillemard @ 2024-08-08 10:46 UTC (permalink / raw) To: Steven Price Cc: linux-kernel, dri-devel, kernel, Boris Brezillon, Rob Herring, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters 2024-08-07 16:08 ` [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters Mary Guillemard 2024-08-08 10:23 ` Steven Price @ 2024-08-08 13:06 ` kernel test robot 1 sibling, 0 replies; 9+ messages in thread From: kernel test robot @ 2024-08-08 13:06 UTC (permalink / raw) To: Mary Guillemard, linux-kernel Cc: llvm, oe-kbuild-all, dri-devel, kernel, Mary Guillemard, Boris Brezillon, Rob Herring, Steven Price, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter 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 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] drm/panfrost: Add cycle counter job requirement 2024-08-07 16:08 [PATCH 0/3] drm/panfrost: Wire cycle counters and timestamp info to userspace Mary Guillemard 2024-08-07 16:08 ` [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters Mary Guillemard @ 2024-08-07 16:08 ` Mary Guillemard 2024-08-08 10:23 ` Steven Price 2024-08-07 16:08 ` [PATCH 3/3] drm/panfrost: Handle JD_REQ_CYCLE_COUNT Mary Guillemard 2 siblings, 1 reply; 9+ messages in thread From: Mary Guillemard @ 2024-08-07 16:08 UTC (permalink / raw) To: linux-kernel Cc: dri-devel, kernel, Mary Guillemard, Boris Brezillon, Rob Herring, Steven Price, David Airlie, Daniel Vetter, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann Extend the uAPI with a new job requirement flag for cycle counters. This requirement is used by userland to indicate that a job requires cycle counters or system timestamp to be propagated. (for use with write value timestamp jobs) We cannot enable cycle counters unconditionally as this would result in an increase of GPU power consumption. As a result, they should be left off unless required by the application. This new uAPI will be used in Mesa to implement timestamp queries and VK_KHR_shader_clock. Signed-off-by: Mary Guillemard <mary.guillemard@collabora.com> --- include/uapi/drm/panfrost_drm.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h index 52b050e2b660..568724be6628 100644 --- a/include/uapi/drm/panfrost_drm.h +++ b/include/uapi/drm/panfrost_drm.h @@ -40,6 +40,7 @@ extern "C" { #define DRM_IOCTL_PANFROST_PERFCNT_DUMP DRM_IOW(DRM_COMMAND_BASE + DRM_PANFROST_PERFCNT_DUMP, struct drm_panfrost_perfcnt_dump) #define PANFROST_JD_REQ_FS (1 << 0) +#define PANFROST_JD_REQ_CYCLE_COUNT (1 << 1) /** * struct drm_panfrost_submit - ioctl argument for submitting commands to the 3D * engine. -- 2.45.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] drm/panfrost: Add cycle counter job requirement 2024-08-07 16:08 ` [PATCH 2/3] drm/panfrost: Add cycle counter job requirement Mary Guillemard @ 2024-08-08 10:23 ` Steven Price 0 siblings, 0 replies; 9+ messages in thread From: Steven Price @ 2024-08-08 10:23 UTC (permalink / raw) To: Mary Guillemard, linux-kernel Cc: dri-devel, kernel, Boris Brezillon, Rob Herring, David Airlie, Daniel Vetter, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann On 07/08/2024 17:08, Mary Guillemard wrote: > Extend the uAPI with a new job requirement flag for cycle > counters. This requirement is used by userland to indicate that a job > requires cycle counters or system timestamp to be propagated. (for use > with write value timestamp jobs) > > We cannot enable cycle counters unconditionally as this would result in > an increase of GPU power consumption. As a result, they should be left > off unless required by the application. > > This new uAPI will be used in Mesa to implement timestamp queries and > VK_KHR_shader_clock. > > Signed-off-by: Mary Guillemard <mary.guillemard@collabora.com> Makes sense, but I'm not sure why this needs to be it's own patch - I'd squash this with the next one where it actually gets used. Steve > --- > include/uapi/drm/panfrost_drm.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h > index 52b050e2b660..568724be6628 100644 > --- a/include/uapi/drm/panfrost_drm.h > +++ b/include/uapi/drm/panfrost_drm.h > @@ -40,6 +40,7 @@ extern "C" { > #define DRM_IOCTL_PANFROST_PERFCNT_DUMP DRM_IOW(DRM_COMMAND_BASE + DRM_PANFROST_PERFCNT_DUMP, struct drm_panfrost_perfcnt_dump) > > #define PANFROST_JD_REQ_FS (1 << 0) > +#define PANFROST_JD_REQ_CYCLE_COUNT (1 << 1) > /** > * struct drm_panfrost_submit - ioctl argument for submitting commands to the 3D > * engine. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] drm/panfrost: Handle JD_REQ_CYCLE_COUNT 2024-08-07 16:08 [PATCH 0/3] drm/panfrost: Wire cycle counters and timestamp info to userspace Mary Guillemard 2024-08-07 16:08 ` [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters Mary Guillemard 2024-08-07 16:08 ` [PATCH 2/3] drm/panfrost: Add cycle counter job requirement Mary Guillemard @ 2024-08-07 16:08 ` Mary Guillemard 2024-08-08 10:23 ` Steven Price 2 siblings, 1 reply; 9+ messages in thread From: Mary Guillemard @ 2024-08-07 16:08 UTC (permalink / raw) To: linux-kernel Cc: dri-devel, kernel, Mary Guillemard, Boris Brezillon, Rob Herring, Steven Price, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter If a job requires cycle counters or system timestamps propagation, we must enable cycle counting before issuing a job and disable it right after the job completes. Since this extends the uAPI and because userland needs a way to advertise features like VK_KHR_shader_clock conditionally, we bumps the driver minor version. Signed-off-by: Mary Guillemard <mary.guillemard@collabora.com> --- drivers/gpu/drm/panfrost/panfrost_drv.c | 8 ++++++-- drivers/gpu/drm/panfrost/panfrost_job.c | 10 ++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index d94c9bf5a7f9..fe983defdfdf 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -21,6 +21,8 @@ #include "panfrost_gpu.h" #include "panfrost_perfcnt.h" +#define JOB_REQUIREMENTS (PANFROST_JD_REQ_FS | PANFROST_JD_REQ_CYCLE_COUNT) + static bool unstable_ioctls; module_param_unsafe(unstable_ioctls, bool, 0600); @@ -262,7 +264,7 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data, if (!args->jc) return -EINVAL; - if (args->requirements && args->requirements != PANFROST_JD_REQ_FS) + if (args->requirements && args->requirements & ~JOB_REQUIREMENTS) return -EINVAL; if (args->out_sync > 0) { @@ -601,6 +603,8 @@ static const struct file_operations panfrost_drm_driver_fops = { * - 1.0 - initial interface * - 1.1 - adds HEAP and NOEXEC flags for CREATE_BO * - 1.2 - adds AFBC_FEATURES query + * - 1.3 - adds JD_REQ_CYCLE_COUNT job requirement for SUBMIT + * - adds SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY queries */ static const struct drm_driver panfrost_drm_driver = { .driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ, @@ -614,7 +618,7 @@ static const struct drm_driver panfrost_drm_driver = { .desc = "panfrost DRM", .date = "20180908", .major = 1, - .minor = 2, + .minor = 3, .gem_create_object = panfrost_gem_create_object, .gem_prime_import_sg_table = panfrost_gem_prime_import_sg_table, diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index df49d37d0e7e..d8c215c0c672 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -159,6 +159,9 @@ panfrost_dequeue_job(struct panfrost_device *pfdev, int slot) struct panfrost_job *job = pfdev->jobs[slot][0]; WARN_ON(!job); + if (job->requirements & PANFROST_JD_REQ_CYCLE_COUNT) + panfrost_cycle_counter_put(pfdev); + if (job->is_profiled) { if (job->engine_usage) { job->engine_usage->elapsed_ns[slot] += @@ -219,6 +222,9 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js) panfrost_job_write_affinity(pfdev, job->requirements, js); + if (job->requirements & PANFROST_JD_REQ_CYCLE_COUNT) + panfrost_cycle_counter_get(pfdev); + /* start MMU, medium priority, cache clean/flush on end, clean/flush on * start */ cfg |= JS_CONFIG_THREAD_PRI(8) | @@ -693,8 +699,12 @@ panfrost_reset(struct panfrost_device *pfdev, spin_lock(&pfdev->js->job_lock); for (i = 0; i < NUM_JOB_SLOTS; i++) { for (j = 0; j < ARRAY_SIZE(pfdev->jobs[0]) && pfdev->jobs[i][j]; j++) { + if (pfdev->jobs[i][j]->requirements & PANFROST_JD_REQ_CYCLE_COUNT) + panfrost_cycle_counter_put(pfdev); + if (pfdev->jobs[i][j]->is_profiled) panfrost_cycle_counter_put(pfdev->jobs[i][j]->pfdev); + pm_runtime_put_noidle(pfdev->dev); panfrost_devfreq_record_idle(&pfdev->pfdevfreq); } -- 2.45.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] drm/panfrost: Handle JD_REQ_CYCLE_COUNT 2024-08-07 16:08 ` [PATCH 3/3] drm/panfrost: Handle JD_REQ_CYCLE_COUNT Mary Guillemard @ 2024-08-08 10:23 ` Steven Price 0 siblings, 0 replies; 9+ messages in thread From: Steven Price @ 2024-08-08 10:23 UTC (permalink / raw) To: Mary Guillemard, linux-kernel Cc: dri-devel, kernel, Boris Brezillon, Rob Herring, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter On 07/08/2024 17:08, Mary Guillemard wrote: > If a job requires cycle counters or system timestamps propagation, we > must enable cycle counting before issuing a job and disable it right > after the job completes. > > Since this extends the uAPI and because userland needs a way to advertise > features like VK_KHR_shader_clock conditionally, we bumps the driver > minor version. > > Signed-off-by: Mary Guillemard <mary.guillemard@collabora.com> > --- > drivers/gpu/drm/panfrost/panfrost_drv.c | 8 ++++++-- > drivers/gpu/drm/panfrost/panfrost_job.c | 10 ++++++++++ > 2 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c > index d94c9bf5a7f9..fe983defdfdf 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c > @@ -21,6 +21,8 @@ > #include "panfrost_gpu.h" > #include "panfrost_perfcnt.h" > > +#define JOB_REQUIREMENTS (PANFROST_JD_REQ_FS | PANFROST_JD_REQ_CYCLE_COUNT) > + > static bool unstable_ioctls; > module_param_unsafe(unstable_ioctls, bool, 0600); > > @@ -262,7 +264,7 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data, > if (!args->jc) > return -EINVAL; > > - if (args->requirements && args->requirements != PANFROST_JD_REQ_FS) > + if (args->requirements && args->requirements & ~JOB_REQUIREMENTS) I think this can be simplified to just: if (args->requirements & ~JOB_REQUIREMENTS) > return -EINVAL; > > if (args->out_sync > 0) { > @@ -601,6 +603,8 @@ static const struct file_operations panfrost_drm_driver_fops = { > * - 1.0 - initial interface > * - 1.1 - adds HEAP and NOEXEC flags for CREATE_BO > * - 1.2 - adds AFBC_FEATURES query > + * - 1.3 - adds JD_REQ_CYCLE_COUNT job requirement for SUBMIT > + * - adds SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY queries Obviously needs updating if the first patch is dropped. > */ > static const struct drm_driver panfrost_drm_driver = { > .driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ, > @@ -614,7 +618,7 @@ static const struct drm_driver panfrost_drm_driver = { > .desc = "panfrost DRM", > .date = "20180908", > .major = 1, > - .minor = 2, > + .minor = 3, > > .gem_create_object = panfrost_gem_create_object, > .gem_prime_import_sg_table = panfrost_gem_prime_import_sg_table, > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c > index df49d37d0e7e..d8c215c0c672 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_job.c > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c > @@ -159,6 +159,9 @@ panfrost_dequeue_job(struct panfrost_device *pfdev, int slot) > struct panfrost_job *job = pfdev->jobs[slot][0]; > > WARN_ON(!job); > + if (job->requirements & PANFROST_JD_REQ_CYCLE_COUNT) > + panfrost_cycle_counter_put(pfdev); > + > if (job->is_profiled) { > if (job->engine_usage) { > job->engine_usage->elapsed_ns[slot] += > @@ -219,6 +222,9 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js) > > panfrost_job_write_affinity(pfdev, job->requirements, js); > > + if (job->requirements & PANFROST_JD_REQ_CYCLE_COUNT) > + panfrost_cycle_counter_get(pfdev); > + > /* start MMU, medium priority, cache clean/flush on end, clean/flush on > * start */ > cfg |= JS_CONFIG_THREAD_PRI(8) | > @@ -693,8 +699,12 @@ panfrost_reset(struct panfrost_device *pfdev, > spin_lock(&pfdev->js->job_lock); > for (i = 0; i < NUM_JOB_SLOTS; i++) { > for (j = 0; j < ARRAY_SIZE(pfdev->jobs[0]) && pfdev->jobs[i][j]; j++) { > + if (pfdev->jobs[i][j]->requirements & PANFROST_JD_REQ_CYCLE_COUNT) > + panfrost_cycle_counter_put(pfdev); > + > if (pfdev->jobs[i][j]->is_profiled) > panfrost_cycle_counter_put(pfdev->jobs[i][j]->pfdev); > + > pm_runtime_put_noidle(pfdev->dev); > panfrost_devfreq_record_idle(&pfdev->pfdevfreq); > } This looks correct, but it would be nice to combine the is_profiled and REQ_CYCLE_COUNT paths. We end up with the odd situation of taking two reference counts (per job) if global profiling is enabled and the flag is specified. To be honest it looks as if there could be a bit of a cleanup by changing panfrost_reset() to use panfrost_dequeue_job(). I'm not sure why it was written that way. Steve ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-08-08 13:08 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-08-07 16:08 [PATCH 0/3] drm/panfrost: Wire cycle counters and timestamp info to userspace Mary Guillemard 2024-08-07 16:08 ` [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters Mary Guillemard 2024-08-08 10:23 ` Steven Price 2024-08-08 10:46 ` Mary Guillemard 2024-08-08 13:06 ` kernel test robot 2024-08-07 16:08 ` [PATCH 2/3] drm/panfrost: Add cycle counter job requirement Mary Guillemard 2024-08-08 10:23 ` Steven Price 2024-08-07 16:08 ` [PATCH 3/3] drm/panfrost: Handle JD_REQ_CYCLE_COUNT Mary Guillemard 2024-08-08 10:23 ` Steven Price
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox