Re: [PATCH v5 03/21] gpu: host1x: Show number of pending waiters in debugfs

Mikko Perttunen
2021-03-26T14:35:12Z

On 3/23/21 12:16 PM, Thierry Reding wrote:
> On Mon, Jan 11, 2021 at 03:00:01PM +0200, Mikko Perttunen wrote:
>> Show the number of pending waiters in the debugfs status file.
>> This is useful for testing to verify that waiters do not leak
>> or accumulate incorrectly.
>>
>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>> ---
>> drivers/gpu/host1x/debug.c | 14 +++++++++++---
>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/host1x/debug.c b/drivers/gpu/host1x/debug.c
>> index 1b4997bda1c7..8a14880c61bb 100644
>> --- a/drivers/gpu/host1x/debug.c
>> +++ b/drivers/gpu/host1x/debug.c
>> @@ -69,6 +69,7 @@ static int show_channel(struct host1x_channel *ch, void *data, bool show_fifo)
>>
>> static void show_syncpts(struct host1x *m, struct output *o)
>> {
>> + struct list_head *pos;
>> unsigned int i;
>>
>> host1x_debug_output(o, "---- syncpts ----\n");
>> @@ -76,12 +77,19 @@ static void show_syncpts(struct host1x *m, struct output *o)
>> for (i = 0; i < host1x_syncpt_nb_pts(m); i++) {
>> u32 max = host1x_syncpt_read_max(m->syncpt + i);
>> u32 min = host1x_syncpt_load(m->syncpt + i);
>> + unsigned int waiters = 0;
>>
>> - if (!min && !max)
>> + spin_lock(&m->syncpt[i].intr.lock);
>> + list_for_each(pos, &m->syncpt[i].intr.wait_head)
>> + waiters++;
>> + spin_unlock(&m->syncpt[i].intr.lock);
>
> Would it make sense to keep a running count so that we don't have to
> compute it here?

Considering this is just a debug facility, I think I prefer not adding a
new field just for it.

>
>> +
>> + if (!min && !max && !waiters)
>> continue;
>>
>> - host1x_debug_output(o, "id %u (%s) min %d max %d\n",
>> - i, m->syncpt[i].name, min, max);
>> + host1x_debug_output(o,
>> + "id %u (%s) min %d max %d (%d waiters)\n",
>> + i, m->syncpt[i].name, min, max, waiters);
>
> Or alternatively, would it be useful to collect a bit more information
> about waiters so that when they leak we get a better understanding of
> which ones leak?
>
> It doesn't look like we currently have much information in struct
> host1x_waitlist to identify waiters, but perhaps that can be extended?

I added this patch mainly for use with integration tests, so they can
verify no waiters leaked in negative tests. I think let's put off adding
other information until there's some need for it.

Mikko

>
> Thierry
>