Re: [RFC v2 7/8] drm/panthor: Add suspend/resume handling for the performance counters

[Lukas Zapolskas <lukas.zapolskas@arm.com>]

On 27/01/2025 20:06, Adrián Larumbe wrote:
> On 11.12.2024 16:50, Lukas Zapolskas wrote:
>> Signed-off-by: Lukas Zapolskas <lukas.zapolskas@arm.com>
>> ---
>>   drivers/gpu/drm/panthor/panthor_device.c |  3 +
>>   drivers/gpu/drm/panthor/panthor_perf.c   | 86 ++++++++++++++++++++++++
>>   drivers/gpu/drm/panthor/panthor_perf.h   |  2 +
>>   3 files changed, 91 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
>> index 1a81a436143b..69536fbdb5ef 100644
>> --- a/drivers/gpu/drm/panthor/panthor_device.c
>> +++ b/drivers/gpu/drm/panthor/panthor_device.c
>> @@ -475,6 +475,7 @@ int panthor_device_resume(struct device *dev)
>>   		ret = drm_WARN_ON(&ptdev->base, panthor_fw_resume(ptdev));
>>   		if (!ret) {
>>   			panthor_sched_resume(ptdev);
>> +			panthor_perf_resume(ptdev);
>>   		} else {
>>   			panthor_mmu_suspend(ptdev);
>>   			panthor_gpu_suspend(ptdev);
>> @@ -543,6 +544,7 @@ int panthor_device_suspend(struct device *dev)
>>   	    drm_dev_enter(&ptdev->base, &cookie)) {
>>   		cancel_work_sync(&ptdev->reset.work);
>>   
>> +		panthor_perf_suspend(ptdev);
>>   		/* We prepare everything as if we were resetting the GPU.
>>   		 * The end of the reset will happen in the resume path though.
>>   		 */
>> @@ -561,6 +563,7 @@ int panthor_device_suspend(struct device *dev)
>>   			panthor_mmu_resume(ptdev);
>>   			drm_WARN_ON(&ptdev->base, panthor_fw_resume(ptdev));
>>   			panthor_sched_resume(ptdev);
>> +			panthor_perf_resume(ptdev);
>>   			drm_dev_exit(cookie);
>>   		}
>>   
>> diff --git a/drivers/gpu/drm/panthor/panthor_perf.c b/drivers/gpu/drm/panthor/panthor_perf.c
>> index d62d97c448da..727e66074eab 100644
>> --- a/drivers/gpu/drm/panthor/panthor_perf.c
>> +++ b/drivers/gpu/drm/panthor/panthor_perf.c
>> @@ -433,6 +433,17 @@ static void panthor_perf_em_zero(struct panthor_perf_enable_masks *em)
>>   		bitmap_zero(em->mask[i], PANTHOR_PERF_EM_BITS);
>>   }
>>   
>> +static bool panthor_perf_em_empty(const struct panthor_perf_enable_masks *const em)
>> +{
>> +	bool empty = true;
>> +	size_t i = 0;
>> +
>> +	for (i = DRM_PANTHOR_PERF_BLOCK_FW; i <= DRM_PANTHOR_PERF_BLOCK_LAST; i++)
>> +		empty &= bitmap_empty(em->mask[i], PANTHOR_PERF_EM_BITS);
>> +
>> +	return empty;
>> +}
>> +
>>   static void panthor_perf_destroy_em_kref(struct kref *em_kref)
>>   {
>>   	struct panthor_perf_enable_masks *em = container_of(em_kref, typeof(*em), refs);
>> @@ -1652,6 +1663,81 @@ 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;
>> +
>> +	if (!panthor_perf_em_empty(sampler->em)) {
>> +		guard(mutex)(&sampler->config_lock);
>> +		panthor_perf_fw_write_em(sampler, sampler->em);
>> +	}
> 
> Aren't panthor_perf_em_empty(sampler->em) and !atomic_read(&sampler->enabled_clients) functionally equivalent?
> 
Hadn't thought about that before, but it may be the case. It
makes a slight difference for adding a new session to the
sampler, where we need to keep track of both the
previous and the current mask, as well as removing a session,
where the order of operation becomes a little awkward if
we use them to mean the same thing.

The sampler's enable mask is seen as somewhat disposable
in the case of removing a session, since we cannot just
remove the counters requested by that session and be done
with it. This would lead to counters that are requested
by other sessions being deleted. So we zero out the
enable mask and then recreate it using all of the enable
masks from the other sessions.

>> +
>> +	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);
>> +}
>> +
>>   /**
>>    * panthor_perf_unplug - Terminate the performance counter subsystem.
>>    * @ptdev: Panthor device.
>> diff --git a/drivers/gpu/drm/panthor/panthor_perf.h b/drivers/gpu/drm/panthor/panthor_perf.h
>> index 3485e4a55e15..a22a511a0809 100644
>> --- a/drivers/gpu/drm/panthor/panthor_perf.h
>> +++ b/drivers/gpu/drm/panthor/panthor_perf.h
>> @@ -16,6 +16,8 @@ struct panthor_perf;
>>   void panthor_perf_info_init(struct panthor_device *ptdev);
>>   
>>   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,
>> -- 
>> 2.25.1
> 
> Adrian Larumbe