[PATCH libdrm] etnaviv: Use hash table to track BO indexes

[Marek Vasut]

On 6/27/19 5:09 PM, Wladimir J. van der Laan wrote:
> On Tue, Jun 04, 2019 at 01:39:29AM +0200, Marek Vasut wrote:
>> Use hash table instead of ad-hoc arrays.
>>
>> Signed-off-by: Marek Vasut <marex at denx.de>
>> Cc: Christian Gmeiner <christian.gmeiner at gmail.com>
>> Cc: Lucas Stach <l.stach at pengutronix.de>
>> ---
>>  etnaviv/etnaviv_bo.c         |  6 +++---
>>  etnaviv/etnaviv_cmd_stream.c | 31 ++++++++++++++++++++++---------
>>  etnaviv/etnaviv_priv.h       | 17 ++++++++++-------
>>  3 files changed, 35 insertions(+), 19 deletions(-)
> 
> I do not know how to quantify the performance difference here, is using a hash
> table faster than an ad-hoc array for the typically small sized arrays here?
> I guess so (but this doubt is why I've never done this change myself).
> 
> Reviewed-by: Wladimir J. van der Laan <laanwj at gmail.com>

Maybe you want to look at
https://gitlab.freedesktop.org/mesa/mesa/merge_requests/1190

I updated this patch against mesa master, apparently the libdrm-etnaviv
bits were folded into mesa now.

>> diff --git a/etnaviv/etnaviv_bo.c b/etnaviv/etnaviv_bo.c
>> index 43ce6b4e..28ad3162 100644
>> --- a/etnaviv/etnaviv_bo.c
>> +++ b/etnaviv/etnaviv_bo.c
>> @@ -44,14 +44,14 @@ drm_private void bo_del(struct etna_bo *bo)
>>  	if (bo->map)
>>  		drm_munmap(bo->map, bo->size);
>>  
>> -	if (bo->name)
>> -		drmHashDelete(bo->dev->name_table, bo->name);
>> -
>>  	if (bo->handle) {
>>  		struct drm_gem_close req = {
>>  			.handle = bo->handle,
>>  		};
>>  
>> +		if (bo->name)
>> +			drmHashDelete(bo->dev->name_table, bo->name);
>> +
>>  		drmHashDelete(bo->dev->handle_table, bo->handle);
>>  		drmIoctl(bo->dev->fd, DRM_IOCTL_GEM_CLOSE, &req);
>>  	}
>> diff --git a/etnaviv/etnaviv_cmd_stream.c b/etnaviv/etnaviv_cmd_stream.c
>> index 261777b0..f550b2ff 100644
>> --- a/etnaviv/etnaviv_cmd_stream.c
>> +++ b/etnaviv/etnaviv_cmd_stream.c
>> @@ -61,6 +61,7 @@ drm_public struct etna_cmd_stream *etna_cmd_stream_new(struct etna_pipe *pipe,
>>  		void *priv)
>>  {
>>  	struct etna_cmd_stream_priv *stream = NULL;
>> +	struct etna_device *dev = pipe->gpu->dev;
>>  
>>  	if (size == 0) {
>>  		ERROR_MSG("invalid size of 0");
>> @@ -86,6 +87,7 @@ drm_public struct etna_cmd_stream *etna_cmd_stream_new(struct etna_pipe *pipe,
>>  	stream->pipe = pipe;
>>  	stream->reset_notify = reset_notify;
>>  	stream->reset_notify_priv = priv;
>> +	stream->seqno = ++dev->stream_cnt;
> 
> Do we have to catch integer overflow here?
> It's very unlikely for there to have been 2^32 streams, but if it happens it might be good to at least
> warn.

I don't think so.

If you allocated (2^(machine word size))-1 streams, you would exhaust
your memory long before that. And this can actually roll over, since if
you were to allocate streams one after the other and then free them,
their numbers would just increment without colliding.

I guess what could happen here would be something like, you allocate
stream #0 , then allocate/free (2^(machine word size)) - 2 streams and
then allocating the (2^(machine word size)) - 1th stream would end up
allocating new stream with the same stream count (?).

So maybe we should redo this and track streams with a hashmap (?). But
that's way out of scope of this patch ; and would have to be fixed in
freedreno too.

-- 
Best regards,
Marek Vasut