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 <ma...@denx.de>
> Cc: Christian Gmeiner <christian.gmei...@gmail.com>
> Cc: Lucas Stach <l.st...@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 <laa...@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
