| Message ID | 20190603233929.23048-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [libdrm] etnaviv: Use hash table to track BO indexes | expand |
Am Di., 4. Juni 2019 um 01:40 Uhr schrieb Marek Vasut <marex@denx.de>: > > Use hash table instead of ad-hoc arrays. > Please re-spin this patch in mesa on top of latest master. If you see a real benefit for setups where newest libdrm with older mesa gets used I will push this patch too. > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Christian Gmeiner <christian.gmeiner@gmail.com> > Cc: Lucas Stach <l.stach@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(-) > > 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; > > return &stream->base; > > @@ -150,18 +152,24 @@ static uint32_t bo2idx(struct etna_cmd_stream *stream, struct etna_bo *bo, > > pthread_mutex_lock(&idx_lock); > > - if (bo->current_stream == stream) { > + if (bo->current_stream_seqno == priv->seqno) { > idx = bo->idx; > } else { > - /* slow-path: */ > - for (idx = 0; idx < priv->nr_bos; idx++) > - if (priv->bos[idx] == bo) > - break; > - if (idx == priv->nr_bos) { > - /* not found */ > + void *val; > + > + if (!priv->bo_table) > + priv->bo_table = drmHashCreate(); Would it make sense to move this to etna_cmd_stream_new(..)? I am not sure if there is ever a stream without any bo attached to it. > + > + if (!drmHashLookup(priv->bo_table, bo->handle, &val)) { > + /* found */ > + idx = (uint32_t)(uintptr_t)val; > + } else { > idx = append_bo(stream, bo); > + val = (void *)(uintptr_t)idx; > + drmHashInsert(priv->bo_table, bo->handle, val); > } > - bo->current_stream = stream; > + > + bo->current_stream_seqno = priv->seqno; > bo->idx = idx; > } > pthread_mutex_unlock(&idx_lock); > @@ -213,10 +221,15 @@ 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 = NULL; > + bo->current_stream_seqno = 0; > etna_bo_del(bo); > } > > + if (priv->bo_table) { > + drmHashDestroy(priv->bo_table); > + priv->bo_table = NULL; > + } > + > if (out_fence_fd) > *out_fence_fd = req.fence_fd; > } > diff --git a/etnaviv/etnaviv_priv.h b/etnaviv/etnaviv_priv.h > index eef7f49c..bc82324e 100644 > --- a/etnaviv/etnaviv_priv.h > +++ b/etnaviv/etnaviv_priv.h > @@ -76,6 +76,8 @@ struct etna_device { > struct etna_bo_cache bo_cache; > > int closefd; /* call close(fd) upon destruction */ > + > + unsigned int stream_cnt; > }; > > drm_private void etna_bo_cache_init(struct etna_bo_cache *cache); > @@ -98,14 +100,12 @@ struct etna_bo { > uint64_t offset; /* offset to mmap() */ > atomic_t refcnt; > > - /* in the common case, a bo won't be referenced by more than a single > - * command stream. So to avoid looping over all the bo's in the > - * reloc table to find the idx of a bo that might already be in the > - * table, we cache the idx in the bo. But in order to detect the > - * slow-path where bo is ref'd in multiple streams, we also must track > - * the current_stream for which the idx is valid. See bo2idx(). > + /* > + * To avoid excess hashtable lookups, cache the stream this bo was > + * last emitted on (since that will probably also be the next ring > + * it is emitted on). > */ > - struct etna_cmd_stream *current_stream; > + unsigned int current_stream_seqno; > uint32_t idx; > > int reuse; > @@ -153,6 +153,9 @@ struct etna_cmd_stream_priv { > /* notify callback if buffer reset happened */ > void (*reset_notify)(struct etna_cmd_stream *stream, void *priv); > void *reset_notify_priv; > + > + unsigned int seqno; > + void *bo_table; > }; > > struct etna_perfmon { > -- > 2.20.1 >
On 6/27/19 5:26 AM, Christian Gmeiner wrote: > Am Di., 4. Juni 2019 um 01:40 Uhr schrieb Marek Vasut <marex@denx.de>: >> >> Use hash table instead of ad-hoc arrays. >> > > Please re-spin this patch in mesa on top of latest master. If you see > a real benefit for setups where > newest libdrm with older mesa gets used I will push this patch too. I have usecase with libdrm 2.4.97 + mesa 19.1 , so it would be nice. >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Christian Gmeiner <christian.gmeiner@gmail.com> >> Cc: Lucas Stach <l.stach@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(-) >> >> 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; >> >> return &stream->base; >> >> @@ -150,18 +152,24 @@ static uint32_t bo2idx(struct etna_cmd_stream *stream, struct etna_bo *bo, >> >> pthread_mutex_lock(&idx_lock); >> >> - if (bo->current_stream == stream) { >> + if (bo->current_stream_seqno == priv->seqno) { >> idx = bo->idx; >> } else { >> - /* slow-path: */ >> - for (idx = 0; idx < priv->nr_bos; idx++) >> - if (priv->bos[idx] == bo) >> - break; >> - if (idx == priv->nr_bos) { >> - /* not found */ >> + void *val; >> + >> + if (!priv->bo_table) >> + priv->bo_table = drmHashCreate(); > > Would it make sense to move this to etna_cmd_stream_new(..)? I am not > sure if there is ever a stream > without any bo attached to it. There can be one after flush, no? I try to keep this implemented the same way as the freedreno one, but I can drop this part.
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@denx.de> > Cc: Christian Gmeiner <christian.gmeiner@gmail.com> > Cc: Lucas Stach <l.stach@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@gmail.com> > 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. > return &stream->base; > > @@ -150,18 +152,24 @@ static uint32_t bo2idx(struct etna_cmd_stream *stream, struct etna_bo *bo, > > pthread_mutex_lock(&idx_lock); > > - if (bo->current_stream == stream) { > + if (bo->current_stream_seqno == priv->seqno) { > idx = bo->idx; > } else { > - /* slow-path: */ > - for (idx = 0; idx < priv->nr_bos; idx++) > - if (priv->bos[idx] == bo) > - break; > - if (idx == priv->nr_bos) { > - /* not found */ > + void *val; > + > + if (!priv->bo_table) > + priv->bo_table = drmHashCreate(); > + > + if (!drmHashLookup(priv->bo_table, bo->handle, &val)) { > + /* found */ > + idx = (uint32_t)(uintptr_t)val; > + } else { > idx = append_bo(stream, bo); > + val = (void *)(uintptr_t)idx; > + drmHashInsert(priv->bo_table, bo->handle, val); > } > - bo->current_stream = stream; > + > + bo->current_stream_seqno = priv->seqno; > bo->idx = idx; > } > pthread_mutex_unlock(&idx_lock); > @@ -213,10 +221,15 @@ 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 = NULL; > + bo->current_stream_seqno = 0; > etna_bo_del(bo); > } > > + if (priv->bo_table) { > + drmHashDestroy(priv->bo_table); > + priv->bo_table = NULL; > + } > + > if (out_fence_fd) > *out_fence_fd = req.fence_fd; > } > diff --git a/etnaviv/etnaviv_priv.h b/etnaviv/etnaviv_priv.h > index eef7f49c..bc82324e 100644 > --- a/etnaviv/etnaviv_priv.h > +++ b/etnaviv/etnaviv_priv.h > @@ -76,6 +76,8 @@ struct etna_device { > struct etna_bo_cache bo_cache; > > int closefd; /* call close(fd) upon destruction */ > + > + unsigned int stream_cnt; > }; > > drm_private void etna_bo_cache_init(struct etna_bo_cache *cache); > @@ -98,14 +100,12 @@ struct etna_bo { > uint64_t offset; /* offset to mmap() */ > atomic_t refcnt; > > - /* in the common case, a bo won't be referenced by more than a single > - * command stream. So to avoid looping over all the bo's in the > - * reloc table to find the idx of a bo that might already be in the > - * table, we cache the idx in the bo. But in order to detect the > - * slow-path where bo is ref'd in multiple streams, we also must track > - * the current_stream for which the idx is valid. See bo2idx(). > + /* > + * To avoid excess hashtable lookups, cache the stream this bo was > + * last emitted on (since that will probably also be the next ring > + * it is emitted on). > */ > - struct etna_cmd_stream *current_stream; > + unsigned int current_stream_seqno; > uint32_t idx; > > int reuse; > @@ -153,6 +153,9 @@ struct etna_cmd_stream_priv { > /* notify callback if buffer reset happened */ > void (*reset_notify)(struct etna_cmd_stream *stream, void *priv); > void *reset_notify_priv; > + > + unsigned int seqno; > + void *bo_table; > }; > > struct etna_perfmon { > -- > 2.20.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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@denx.de> >> Cc: Christian Gmeiner <christian.gmeiner@gmail.com> >> Cc: Lucas Stach <l.stach@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@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.
> 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. Kind regards, Wladimir
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; };
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; return &stream->base; @@ -150,18 +152,24 @@ static uint32_t bo2idx(struct etna_cmd_stream *stream, struct etna_bo *bo, pthread_mutex_lock(&idx_lock); - if (bo->current_stream == stream) { + if (bo->current_stream_seqno == priv->seqno) { idx = bo->idx; } else { - /* slow-path: */ - for (idx = 0; idx < priv->nr_bos; idx++) - if (priv->bos[idx] == bo) - break; - if (idx == priv->nr_bos) { - /* not found */ + void *val; + + if (!priv->bo_table) + priv->bo_table = drmHashCreate(); + + if (!drmHashLookup(priv->bo_table, bo->handle, &val)) { + /* found */ + idx = (uint32_t)(uintptr_t)val; + } else { idx = append_bo(stream, bo); + val = (void *)(uintptr_t)idx; + drmHashInsert(priv->bo_table, bo->handle, val); } - bo->current_stream = stream; + + bo->current_stream_seqno = priv->seqno; bo->idx = idx; } pthread_mutex_unlock(&idx_lock); @@ -213,10 +221,15 @@ 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 = NULL; + bo->current_stream_seqno = 0; etna_bo_del(bo); } + if (priv->bo_table) { + drmHashDestroy(priv->bo_table); + priv->bo_table = NULL; + } + if (out_fence_fd) *out_fence_fd = req.fence_fd; } diff --git a/etnaviv/etnaviv_priv.h b/etnaviv/etnaviv_priv.h index eef7f49c..bc82324e 100644 --- a/etnaviv/etnaviv_priv.h +++ b/etnaviv/etnaviv_priv.h @@ -76,6 +76,8 @@ struct etna_device { struct etna_bo_cache bo_cache; int closefd; /* call close(fd) upon destruction */ + + unsigned int stream_cnt; }; drm_private void etna_bo_cache_init(struct etna_bo_cache *cache); @@ -98,14 +100,12 @@ struct etna_bo { uint64_t offset; /* offset to mmap() */ atomic_t refcnt; - /* in the common case, a bo won't be referenced by more than a single - * command stream. So to avoid looping over all the bo's in the - * reloc table to find the idx of a bo that might already be in the - * table, we cache the idx in the bo. But in order to detect the - * slow-path where bo is ref'd in multiple streams, we also must track - * the current_stream for which the idx is valid. See bo2idx(). + /* + * To avoid excess hashtable lookups, cache the stream this bo was + * last emitted on (since that will probably also be the next ring + * it is emitted on). */ - struct etna_cmd_stream *current_stream; + unsigned int current_stream_seqno; uint32_t idx; int reuse; @@ -153,6 +153,9 @@ struct etna_cmd_stream_priv { /* notify callback if buffer reset happened */ void (*reset_notify)(struct etna_cmd_stream *stream, void *priv); void *reset_notify_priv; + + unsigned int seqno; + void *bo_table; }; struct etna_perfmon {
Use hash table instead of ad-hoc arrays. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Christian Gmeiner <christian.gmeiner@gmail.com> Cc: Lucas Stach <l.stach@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(-)