[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a6791291-e40f-4a41-a84a-74afeaecf0e5@arm.com>
Date: Wed, 26 Mar 2025 14:36:03 +0000
From: Lukas Zapolskas <lukas.zapolskas@....com>
To: Adrián Larumbe <adrian.larumbe@...labora.com>
Cc: 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>, Simona Vetter <simona@...ll.ch>,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
Mihail Atanassov <mihail.atanassov@....com>, nd@....com
Subject: Re: [RFC v2 3/8] drm/panthor: Add panthor_perf_init and
panthor_perf_unplug
(please excuse the line-wrapping on the previous email,
it seems that it was soft-wrapped).
On 27/01/2025 12:46, Adrián Larumbe wrote:
> On 11.12.2024 16:50, Lukas Zapolskas wrote:
>> Added the panthor_perf system initialization and unplug code to allow
>> for the handling of userspace sessions to be added in follow-up patches.
>>
>> Signed-off-by: Lukas Zapolskas <lukas.zapolskas@....com>
>> ---
>> drivers/gpu/drm/panthor/panthor_device.c | 7 +++
>> drivers/gpu/drm/panthor/panthor_device.h | 5 +-
>> drivers/gpu/drm/panthor/panthor_perf.c | 77 ++++++++++++++++++++++++
>> drivers/gpu/drm/panthor/panthor_perf.h | 3 +
>> 4 files changed, 91 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
>> index 00f7b8ce935a..1a81a436143b 100644
>> --- a/drivers/gpu/drm/panthor/panthor_device.c
>> +++ b/drivers/gpu/drm/panthor/panthor_device.c
>> @@ -19,6 +19,7 @@
>> #include "panthor_fw.h"
>> #include "panthor_gpu.h"
>> #include "panthor_mmu.h"
>> +#include "panthor_perf.h"
>> #include "panthor_regs.h"
>> #include "panthor_sched.h"
>>
>> @@ -97,6 +98,7 @@ void panthor_device_unplug(struct panthor_device *ptdev)
>> /* Now, try to cleanly shutdown the GPU before the device resources
>> * get reclaimed.
>> */
>> + panthor_perf_unplug(ptdev);
>> panthor_sched_unplug(ptdev);
>> panthor_fw_unplug(ptdev);
>> panthor_mmu_unplug(ptdev);
>> @@ -262,6 +264,10 @@ int panthor_device_init(struct panthor_device *ptdev)
>> if (ret)
>> goto err_unplug_fw;
>>
>> + ret = panthor_perf_init(ptdev);
>> + if (ret)
>> + goto err_unplug_fw;
>> +
>> /* ~3 frames */
>> pm_runtime_set_autosuspend_delay(ptdev->base.dev, 50);
>> pm_runtime_use_autosuspend(ptdev->base.dev);
>> @@ -275,6 +281,7 @@ int panthor_device_init(struct panthor_device *ptdev)
>>
>> err_disable_autosuspend:
>> pm_runtime_dont_use_autosuspend(ptdev->base.dev);
>> + panthor_perf_unplug(ptdev);
>> panthor_sched_unplug(ptdev);
>>
>> err_unplug_fw:
>> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
>> index 636542c1dcbd..aca33d03036c 100644
>> --- a/drivers/gpu/drm/panthor/panthor_device.h
>> +++ b/drivers/gpu/drm/panthor/panthor_device.h
>> @@ -26,7 +26,7 @@ struct panthor_heap_pool;
>> struct panthor_job;
>> struct panthor_mmu;
>> struct panthor_fw;
>> -struct panthor_perfcnt;
>> +struct panthor_perf;
>> struct panthor_vm;
>> struct panthor_vm_pool;
>>
>> @@ -137,6 +137,9 @@ struct panthor_device {
>> /** @devfreq: Device frequency scaling management data. */
>> struct panthor_devfreq *devfreq;
>>
>> + /** @perf: Performance counter management data. */
>> + struct panthor_perf *perf;
>> +
>> /** @unplug: Device unplug related fields. */
>> struct {
>> /** @lock: Lock used to serialize unplug operations. */
>> diff --git a/drivers/gpu/drm/panthor/panthor_perf.c b/drivers/gpu/drm/panthor/panthor_perf.c
>> index 0e3d769c1805..e0dc6c4b0cf1 100644
>> --- a/drivers/gpu/drm/panthor/panthor_perf.c
>> +++ b/drivers/gpu/drm/panthor/panthor_perf.c
>> @@ -13,6 +13,24 @@
>> #include "panthor_perf.h"
>> #include "panthor_regs.h"
>>
>> +struct panthor_perf {
>> + /**
>> + * @block_set: The global counter set configured onto the HW.
>> + */
>> + u8 block_set;
>
> I think this field is not used in any further patches. Only in the sampler
> struct definition later on you include the same field and assign it from
> the ioctl setup arguments.
Will have to correct that, it should be used for the FW programming and
checking whether a session creation request can be satisfied.
>
>> + /** @next_session: The ID of the next session. */
>> + u32 next_session;
>> +
>> + /** @session_range: The number of sessions supported at a time. */
>> + struct xa_limit session_range;
>> +
>> + /**
>> + * @sessions: Global map of sessions, accessed by their ID.
>> + */
>> + struct xarray sessions;
>> +};
>> +
>> /**
>> * PANTHOR_PERF_COUNTERS_PER_BLOCK - On CSF architectures pre-11.x, the number of counters
>> * per block was hardcoded to be 64. Arch 11.0 onwards supports the PRFCNT_FEATURES GPU register,
>> @@ -45,3 +63,62 @@ void panthor_perf_info_init(struct panthor_device *ptdev)
>> perf_info->shader_blocks = hweight64(ptdev->gpu_info.shader_present);
>> }
>>
>> +/**
>> + * panthor_perf_init - Initialize the performance counter subsystem.
>> + * @ptdev: Panthor device
>> + *
>> + * The performance counters require the FW interface to be available to setup the
>> + * sampling ringbuffers, so this must be called only after FW is initialized.
>> + *
>> + * Return: 0 on success, negative error code on failure.
>> + */
>> +int panthor_perf_init(struct panthor_device *ptdev)
>> +{
>> + struct panthor_perf *perf;
>> +
>> + if (!ptdev)
>> + return -EINVAL;
>> +
>> + perf = devm_kzalloc(ptdev->base.dev, sizeof(*perf), GFP_KERNEL);
>> + if (ZERO_OR_NULL_PTR(perf))
>> + return -ENOMEM;
>> +
>> + xa_init_flags(&perf->sessions, XA_FLAGS_ALLOC);
>> +
>> + /* Currently, we only support a single session at a time. */
>> + perf->session_range = (struct xa_limit) {
>> + .min = 0,
>> + .max = 1,
>> + };
>
> I guess at the moment we only allow a single session because periodic sampling
> isn't yet implemented. Does that mean multisession support will not be made
> available for manual samplers in the future?
The RFC was intended to purely give a functional implementation without
periodic or multi-client sampling. Multi-client will be available for
both periodic and manual sessions, once periodic is implemented.
>> +
>> + drm_info(&ptdev->base, "Performance counter subsystem initialized");
>> +
>> + ptdev->perf = perf;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * panthor_perf_unplug - Terminate the performance counter subsystem.
>> + * @ptdev: Panthor device.
>> + *
>> + * This function will terminate the performance counter control structures and any remaining
>> + * sessions, after waiting for any pending interrupts.
>> + */
>> +void panthor_perf_unplug(struct panthor_device *ptdev)
>> +{
>> + struct panthor_perf *perf = ptdev->perf;
>> +
>> + if (!perf)
>> + return;
>> +
>> + if (!xa_empty(&perf->sessions))
>> + drm_err(&ptdev->base,
>> + "Performance counter sessions active when unplugging the driver!");
>> +
>> + xa_destroy(&perf->sessions);
>> +
>> + devm_kfree(ptdev->base.dev, ptdev->perf);
>
> If we always call devm_kfree, then what is the point of allocating ptdev->perf
> with devm_kzalloc?
You're right, it's not necessary. The panthor_perf object could in
theory be allocated with devm, since the lifetime matches that of the
panthor_device, but it's simpler to drop the devm allocations.
>
>> + ptdev->perf = NULL;
>> +}
>> diff --git a/drivers/gpu/drm/panthor/panthor_perf.h b/drivers/gpu/drm/panthor/panthor_perf.h
>> index cff537a370c9..90af8b18358c 100644
>> --- a/drivers/gpu/drm/panthor/panthor_perf.h
>> +++ b/drivers/gpu/drm/panthor/panthor_perf.h
>> @@ -9,4 +9,7 @@ struct panthor_device;
>>
>> void panthor_perf_info_init(struct panthor_device *ptdev);
>>
>> +int panthor_perf_init(struct panthor_device *ptdev);
>> +void panthor_perf_unplug(struct panthor_device *ptdev);
>> +
>> #endif /* __PANTHOR_PERF_H__ */
>> --
>> 2.25.1
>
>
> Adrian Larumbe
Powered by blists - more mailing lists