| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8a65cc81-92cf-48e9-90b7-645a1fc94ef8@arm.com>
Date: Fri, 25 Jul 2025 10:26:46 +0100
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
Subject: Re: [PATCH v4 6/7] drm/panthor: Add suspend, resume and reset
handling
On 18/07/2025 16:01, Adrián Larumbe wrote:
> On 16.05.2025 16:49, Lukas Zapolskas wrote:
>> The sampler must disable and re-enable counter sampling around suspends,
>> and must re-program the FW interface after a reset to avoid losing
>> data.
>>
>> Signed-off-by: Lukas Zapolskas <lukas.zapolskas@....com>
>> ---
>> drivers/gpu/drm/panthor/panthor_device.c | 7 +-
>> drivers/gpu/drm/panthor/panthor_perf.c | 102 +++++++++++++++++++++++
>> drivers/gpu/drm/panthor/panthor_perf.h | 6 ++
>> 3 files changed, 114 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
>> index 7ac985d44655..92624a8717c5 100644
>> --- a/drivers/gpu/drm/panthor/panthor_device.c
>> +++ b/drivers/gpu/drm/panthor/panthor_device.c
>> @@ -139,6 +139,7 @@ static void panthor_device_reset_work(struct work_struct *work)
>> if (!drm_dev_enter(&ptdev->base, &cookie))
>> return;
>>
>> + panthor_perf_pre_reset(ptdev);
>> panthor_sched_pre_reset(ptdev);
>> panthor_fw_pre_reset(ptdev, true);
>> panthor_mmu_pre_reset(ptdev);
>> @@ -148,6 +149,7 @@ static void panthor_device_reset_work(struct work_struct *work)
>> ret = panthor_fw_post_reset(ptdev);
>> atomic_set(&ptdev->reset.pending, 0);
>> panthor_sched_post_reset(ptdev, ret != 0);
>> + panthor_perf_post_reset(ptdev);
>> drm_dev_exit(cookie);
>>
>> if (ret) {
>> @@ -496,8 +498,10 @@ int panthor_device_resume(struct device *dev)
>> ret = panthor_device_resume_hw_components(ptdev);
>> }
>>
>> - if (!ret)
>> + if (!ret) {
>> panthor_sched_resume(ptdev);
>> + panthor_perf_resume(ptdev);
>> + }
>>
>> drm_dev_exit(cookie);
>>
>> @@ -561,6 +565,7 @@ int panthor_device_suspend(struct device *dev)
>> /* We prepare everything as if we were resetting the GPU.
>> * The end of the reset will happen in the resume path though.
>> */
>> + panthor_perf_suspend(ptdev);
>> panthor_sched_suspend(ptdev);
>> panthor_fw_suspend(ptdev);
>> panthor_mmu_suspend(ptdev);
>> diff --git a/drivers/gpu/drm/panthor/panthor_perf.c b/drivers/gpu/drm/panthor/panthor_perf.c
>> index 97603b168d2d..438319cf71ab 100644
>> --- a/drivers/gpu/drm/panthor/panthor_perf.c
>> +++ b/drivers/gpu/drm/panthor/panthor_perf.c
>> @@ -1845,6 +1845,76 @@ void panthor_perf_session_destroy(struct panthor_file *pfile, struct panthor_per
>> }
>> }
>>
>> +static int panthor_perf_sampler_resume(struct panthor_perf_sampler *sampler)
>> +{
>> + int ret;
>> +
>> + if (!atomic_read(&sampler->enabled_clients))
>> + return 0;
>> +
>> + ret = panthor_perf_fw_start_sampling(sampler->ptdev);
>> + if (ret)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +static int panthor_perf_sampler_suspend(struct panthor_perf_sampler *sampler)
>> +{
>> + int ret;
>> +
>> + if (!atomic_read(&sampler->enabled_clients))
>> + return 0;
>> +
>> + ret = panthor_perf_fw_stop_sampling(sampler->ptdev);
>> + if (ret)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * panthor_perf_suspend - Prepare the performance counter subsystem for system suspend.
>> + * @ptdev: Panthor device.
>> + *
>> + * Indicate to the performance counters that the system is suspending.
>> + *
>> + * This function must not be used to handle MCU power state transitions: just before MCU goes
>> + * from on to any inactive state, an automatic sample will be performed by the firmware, and
>> + * the performance counter firmware state will be restored on warm boot.
>> + *
>> + * Return: 0 on success, negative error code on failure.
>> + */
>> +int panthor_perf_suspend(struct panthor_device *ptdev)
>> +{
>> + struct panthor_perf *perf = ptdev->perf;
>> +
>> + if (!perf)
>> + return 0;
>> +
>> + return panthor_perf_sampler_suspend(&perf->sampler);
>> +}
>> +
>> +/**
>> + * panthor_perf_resume - Resume the performance counter subsystem after system resumption.
>> + * @ptdev: Panthor device.
>> + *
>> + * Indicate to the performance counters that the system has resumed. This must not be used
>> + * to handle MCU state transitions, for the same reasons as detailed in the kerneldoc for
>> + * @panthor_perf_suspend.
>> + *
>> + * Return: 0 on success, negative error code on failure.
>> + */
>> +int panthor_perf_resume(struct panthor_device *ptdev)
>> +{
>> + struct panthor_perf *perf = ptdev->perf;
>> +
>> + if (!perf)
>> + return 0;
>> +
>> + return panthor_perf_sampler_resume(&perf->sampler);
>> +}
>
> In the two previous functions, you return an int, but you never used it
> from where they're called.
Thanks, will drop the return values from the perf_{suspend,resume} functions.
> Also, in both of them, for the sake of
> coherence, I'd get rid of the *sampler* subcalls because later in
> 'panthor_perf_pre_reset' and 'panthor_perf_post_reset' you manipulate the
> sampler directly without referring it to another function. The functions
> are short enough for us to be able to inline the content of
> 'panthor_perf_sampler_resume' into 'panthor_perf_resume'.
>
Will do.
>> +
>> /**
>> * panthor_perf_unplug - Terminate the performance counter subsystem.
>> * @ptdev: Panthor device.
>> @@ -1878,3 +1948,35 @@ void panthor_perf_unplug(struct panthor_device *ptdev)
>>
>> ptdev->perf = NULL;
>> }
>> +
>> +void panthor_perf_pre_reset(struct panthor_device *ptdev)
>> +{
>> + struct panthor_perf_sampler *sampler;
>> +
>> + if (!ptdev || !ptdev->perf)
>> + return;
>> +
>> + sampler = &ptdev->perf->sampler;
>> +
>> + if (!atomic_read(&sampler->enabled_clients))
>> + return;
>> +
>> + panthor_perf_fw_stop_sampling(sampler->ptdev);
>> +}
>> +
>> +void panthor_perf_post_reset(struct panthor_device *ptdev)
>> +{
>> + struct panthor_perf_sampler *sampler;
>> +
>> + if (!ptdev || !ptdev->perf)
>> + return;
>
> In both this function and the preceding one, ptdev is meant to be
> available by the time they're called, so I'd turn the check of ptdev not
> being null into a drm_WARN().
>
I'll drop the check for the ptdev entirely, since it looks like there will
be other issues before these functions are even called if it's null, and
add the drm_WARN_ON_ONCE for the perf pointer, since that should also be
initialized by this point.
>> +
>> + sampler = &ptdev->perf->sampler;
>> +
>> + if (!atomic_read(&sampler->enabled_clients))
>> + return;
>> +
>> + panthor_perf_fw_write_sampler_config(sampler);
>> +
>> + panthor_perf_fw_start_sampling(sampler->ptdev);
>> +}
>> diff --git a/drivers/gpu/drm/panthor/panthor_perf.h b/drivers/gpu/drm/panthor/panthor_perf.h
>> index c482198b6fbd..fc08a5440a35 100644
>> --- a/drivers/gpu/drm/panthor/panthor_perf.h
>> +++ b/drivers/gpu/drm/panthor/panthor_perf.h
>> @@ -13,6 +13,8 @@ struct panthor_file;
>> struct panthor_perf;
>>
>> int panthor_perf_init(struct panthor_device *ptdev);
>> +int panthor_perf_suspend(struct panthor_device *ptdev);
>> +int panthor_perf_resume(struct panthor_device *ptdev);
>> void panthor_perf_unplug(struct panthor_device *ptdev);
>>
>> int panthor_perf_session_setup(struct panthor_device *ptdev, struct panthor_perf *perf,
>> @@ -30,5 +32,9 @@ void panthor_perf_session_destroy(struct panthor_file *pfile, struct panthor_per
>>
>> void panthor_perf_report_irq(struct panthor_device *ptdev, u32 status);
>>
>> +void panthor_perf_pre_reset(struct panthor_device *ptdev);
>> +
>> +void panthor_perf_post_reset(struct panthor_device *ptdev);
>> +
>> #endif /* __PANTHOR_PERF_H__ */
>>
>> --
>> 2.33.0.dirty
>
>
> Adrian Larumbe
Powered by blists - more mailing lists