Re: [PATCH v4 4/7] drm/panthor: Introduce sampling sessions to handle userspace clients

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

Hello Steve, 

Thanks for taking a look. Looks like my rebase wasn't great, and I missed a patch
removing some of the GEM functions. Will get that fixed. 

Kind regards,
Lukas

On 20/06/2025 16:28, Steven Price wrote:
> Hi Lukas,
> 
> I was going to try testing this out, but it doesn't look functional. See
> below.
> 
> On 16/05/2025 16:49, Lukas Zapolskas wrote:
> [...]
>> diff --git a/drivers/gpu/drm/panthor/panthor_perf.c b/drivers/gpu/drm/panthor/panthor_perf.c
>> index 9365ce9fed04..15fa533731f3 100644
>> --- a/drivers/gpu/drm/panthor/panthor_perf.c
>> +++ b/drivers/gpu/drm/panthor/panthor_perf.c
>> @@ -2,13 +2,177 @@
>>  /* Copyright 2023 Collabora Ltd */
>>  /* Copyright 2025 Arm ltd. */
>>  
>> -#include <linux/bitops.h>
>> +#include <drm/drm_gem.h>
>>  #include <drm/panthor_drm.h>
>> +#include <linux/bitops.h>
>> +#include <linux/circ_buf.h>
>>  
>>  #include "panthor_device.h"
>>  #include "panthor_fw.h"
>>  #include "panthor_perf.h"
>>  
>> +/**
>> + * PANTHOR_PERF_EM_BITS - Number of bits in a user-facing enable mask. This must correspond
>> + *                        to the maximum number of counters available for selection on the newest
>> + *                        Mali GPUs (128 as of the Mali-Gx15).
>> + */
>> +#define PANTHOR_PERF_EM_BITS (BITS_PER_TYPE(u64) * 2)
>> +
>> +enum panthor_perf_session_state {
>> +	/** @PANTHOR_PERF_SESSION_ACTIVE: The session is active and can be used for sampling. */
>> +	PANTHOR_PERF_SESSION_ACTIVE = 0,
>> +
>> +	/**
>> +	 * @PANTHOR_PERF_SESSION_OVERFLOW: The session encountered an overflow in one of the
>> +	 *                                 counters during the last sampling period. This flag
>> +	 *                                 gets propagated as part of samples emitted for this
>> +	 *                                 session, to ensure the userspace client can gracefully
>> +	 *                                 handle this data corruption.
>> +	 */
>> +	PANTHOR_PERF_SESSION_OVERFLOW,
>> +
>> +	/* Must be last */
>> +	PANTHOR_PERF_SESSION_MAX,
>> +};
>> +
>> +struct panthor_perf_enable_masks {
>> +	/**
>> +	 * @mask: Array of bitmasks indicating the counters userspace requested, where
>> +	 *        one bit represents a single counter. Used to build the firmware configuration
>> +	 *        and ensure that userspace clients obtain only the counters they requested.
>> +	 */
>> +	unsigned long mask[DRM_PANTHOR_PERF_BLOCK_MAX][BITS_TO_LONGS(PANTHOR_PERF_EM_BITS)];
>> +};
>> +
>> +struct panthor_perf_counter_block {
>> +	struct drm_panthor_perf_block_header header;
>> +	u64 counters[];
>> +};
> 
> I think something has gone rather wrong in a rebasing. This struct was
> already added in patch 2. So this causes a build error (that the kernel
> test robot caught too).
> 
> [...]
>> @@ -72,6 +236,122 @@ static void panthor_perf_info_init(struct panthor_device *ptdev)
>>  	perf_info->sample_size = session_get_user_sample_size(perf_info);
>>  }
>>  
>> +static struct panthor_perf_enable_masks *panthor_perf_create_em(struct drm_panthor_perf_cmd_setup
>> +		*setup_args)
> 
> There's some code style mis-formatting like this - which is then fixed
> up in patch 5. So it looks like you've applied fixups to the wrong commit.
> 
> Also this series will need rebasing because there's some upstream
> changes that it's now conflicting with. The base commit looks pretty
> ancient now.
> 
> Thanks,
> Steve
>