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

[Marek Vasut]

On 6/28/19 1:27 PM, Wladimir J. van der Laan wrote:
>> 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.
> 
> Thanks!
> 
>>>>  	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 (?).
> 
> This is the scenario that I was imagining, in which two separate streams could
> collide with the same seq no (resulting in weird glitches in bo tracking), but yes, it
> seems very unlikely.

So looking at the code, why do we even track some "seqno"s at all?

Wouldn't it be enough to replace int current_stream_seqno with struct
etna_cmd_stream *current_stream and just track the stream pointer
itself? Then all these seqno parts go away.

Like this:

diff --git a/src/etnaviv/drm/etnaviv_cmd_stream.c
b/src/etnaviv/drm/etnaviv_cmd_stream.c
index 9c25e4330c1..c4a26b2672c 100644
--- a/src/etnaviv/drm/etnaviv_cmd_stream.c
+++ b/src/etnaviv/drm/etnaviv_cmd_stream.c
@@ -87,7 +87,6 @@ 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;

        return &stream->base;

@@ -152,7 +151,7 @@ static uint32_t bo2idx(struct etna_cmd_stream
*stream, struct etna_bo *bo,

        pthread_mutex_lock(&idx_lock);

-       if (bo->current_stream_seqno == priv->seqno) {
+       if (bo->current_stream == stream) {
                idx = bo->idx;
        } else {
                void *val;
@@ -169,7 +168,7 @@ static uint32_t bo2idx(struct etna_cmd_stream
*stream, struct etna_bo *bo,
                        drmHashInsert(priv->bo_table, bo->handle, val);
                }

-               bo->current_stream_seqno = priv->seqno;
+               bo->current_stream = stream;
                bo->idx = idx;
        }
        pthread_mutex_unlock(&idx_lock);
@@ -221,7 +220,7 @@ static void flush(struct etna_cmd_stream *stream,
int in_fence_fd,
        for (uint32_t i = 0; i < priv->nr_bos; i++) {
                struct etna_bo *bo = priv->bos[i];

-               bo->current_stream_seqno = 0;
+               bo->current_stream = NULL;
                etna_bo_del(bo);
        }

diff --git a/src/etnaviv/drm/etnaviv_priv.h b/src/etnaviv/drm/etnaviv_priv.h
index ab69c37be62..4e400ae369b 100644
--- a/src/etnaviv/drm/etnaviv_priv.h
+++ b/src/etnaviv/drm/etnaviv_priv.h
@@ -106,7 +106,7 @@ struct etna_bo {
         * last emitted on (since that will probably also be the next ring
         * it is emitted on).
         */
-       unsigned int current_stream_seqno;
+       struct etna_cmd_stream *current_stream;
        uint32_t idx;

        int reuse;
@@ -155,7 +155,6 @@ struct etna_cmd_stream_priv {
        void (*reset_notify)(struct etna_cmd_stream *stream, void *priv);
        void *reset_notify_priv;

-       unsigned int seqno;
        void *bo_table;
 };



-- 
Best regards,
Marek Vasut