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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
