[v2,23/37] panfrost: Make panfrost_batch->bos a hash table

Submitted by Boris Brezillon on Sept. 16, 2019, 9:37 a.m.

Details

Message ID 20190916093715.32203-24-boris.brezillon@collabora.com
State Accepted
Commit a8bd265cefcf3fcf1613c6a79874f8733c6dd6c0
Headers show
Series "panfrost: Support batch pipelining" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Boris Brezillon Sept. 16, 2019, 9:37 a.m.
So we can store the flags as data and keep the BO as a key. This way
we keep track of the type of access done on BOs.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 src/gallium/drivers/panfrost/pan_job.c | 33 +++++++++++++++++---------
 src/gallium/drivers/panfrost/pan_job.h |  2 +-
 2 files changed, 23 insertions(+), 12 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/drivers/panfrost/pan_job.c b/src/gallium/drivers/panfrost/pan_job.c
index 6332529b2f9b..739f36a593f1 100644
--- a/src/gallium/drivers/panfrost/pan_job.c
+++ b/src/gallium/drivers/panfrost/pan_job.c
@@ -44,9 +44,8 @@  panfrost_create_batch(struct panfrost_context *ctx,
 
         batch->ctx = ctx;
 
-        batch->bos = _mesa_set_create(batch,
-                                      _mesa_hash_pointer,
-                                      _mesa_key_pointer_equal);
+        batch->bos = _mesa_hash_table_create(batch, _mesa_hash_pointer,
+                                             _mesa_key_pointer_equal);
 
         batch->minx = batch->miny = ~0;
         batch->maxx = batch->maxy = 0;
@@ -67,10 +66,8 @@  panfrost_free_batch(struct panfrost_batch *batch)
 
         struct panfrost_context *ctx = batch->ctx;
 
-        set_foreach(batch->bos, entry) {
-                struct panfrost_bo *bo = (struct panfrost_bo *)entry->key;
-                panfrost_bo_unreference(bo);
-        }
+        hash_table_foreach(batch->bos, entry)
+                panfrost_bo_unreference((struct panfrost_bo *)entry->key);
 
         _mesa_hash_table_remove_key(ctx->batches, &batch->key);
 
@@ -138,11 +135,25 @@  panfrost_batch_add_bo(struct panfrost_batch *batch, struct panfrost_bo *bo,
         if (!bo)
                 return;
 
-        if (_mesa_set_search(batch->bos, bo))
+        struct hash_entry *entry;
+        uint32_t old_flags = 0;
+
+        entry = _mesa_hash_table_search(batch->bos, bo);
+        if (!entry) {
+                entry = _mesa_hash_table_insert(batch->bos, bo,
+                                                (void *)(uintptr_t)flags);
+                panfrost_bo_reference(bo);
+	} else {
+                old_flags = (uintptr_t)entry->data;
+        }
+
+        assert(entry);
+
+        if (old_flags == flags)
                 return;
 
-        panfrost_bo_reference(bo);
-        _mesa_set_add(batch->bos, bo);
+        flags |= old_flags;
+        entry->data = (void *)(uintptr_t)flags;
 }
 
 void panfrost_batch_add_fbo_bos(struct panfrost_batch *batch)
@@ -376,7 +387,7 @@  panfrost_batch_submit_ioctl(struct panfrost_batch *batch,
         bo_handles = calloc(batch->bos->entries, sizeof(*bo_handles));
         assert(bo_handles);
 
-        set_foreach(batch->bos, entry) {
+        hash_table_foreach(batch->bos, entry) {
                 struct panfrost_bo *bo = (struct panfrost_bo *)entry->key;
                 assert(bo->gem_handle > 0);
                 bo_handles[submit.bo_handle_count++] = bo->gem_handle;
diff --git a/src/gallium/drivers/panfrost/pan_job.h b/src/gallium/drivers/panfrost/pan_job.h
index 0b37a3131e86..3f2cf1a999f3 100644
--- a/src/gallium/drivers/panfrost/pan_job.h
+++ b/src/gallium/drivers/panfrost/pan_job.h
@@ -98,7 +98,7 @@  struct panfrost_batch {
         unsigned job_index;
 
         /* BOs referenced -- will be used for flushing logic */
-        struct set *bos;
+        struct hash_table *bos;
 
         /* Current transient BO */
 	struct panfrost_bo *transient_bo;

Comments

What if flags = 0?

On Mon, Sep 16, 2019 at 11:37:01AM +0200, Boris Brezillon wrote:
> So we can store the flags as data and keep the BO as a key. This way
> we keep track of the type of access done on BOs.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  src/gallium/drivers/panfrost/pan_job.c | 33 +++++++++++++++++---------
>  src/gallium/drivers/panfrost/pan_job.h |  2 +-
>  2 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/src/gallium/drivers/panfrost/pan_job.c b/src/gallium/drivers/panfrost/pan_job.c
> index 6332529b2f9b..739f36a593f1 100644
> --- a/src/gallium/drivers/panfrost/pan_job.c
> +++ b/src/gallium/drivers/panfrost/pan_job.c
> @@ -44,9 +44,8 @@ panfrost_create_batch(struct panfrost_context *ctx,
>  
>          batch->ctx = ctx;
>  
> -        batch->bos = _mesa_set_create(batch,
> -                                      _mesa_hash_pointer,
> -                                      _mesa_key_pointer_equal);
> +        batch->bos = _mesa_hash_table_create(batch, _mesa_hash_pointer,
> +                                             _mesa_key_pointer_equal);
>  
>          batch->minx = batch->miny = ~0;
>          batch->maxx = batch->maxy = 0;
> @@ -67,10 +66,8 @@ panfrost_free_batch(struct panfrost_batch *batch)
>  
>          struct panfrost_context *ctx = batch->ctx;
>  
> -        set_foreach(batch->bos, entry) {
> -                struct panfrost_bo *bo = (struct panfrost_bo *)entry->key;
> -                panfrost_bo_unreference(bo);
> -        }
> +        hash_table_foreach(batch->bos, entry)
> +                panfrost_bo_unreference((struct panfrost_bo *)entry->key);
>  
>          _mesa_hash_table_remove_key(ctx->batches, &batch->key);
>  
> @@ -138,11 +135,25 @@ panfrost_batch_add_bo(struct panfrost_batch *batch, struct panfrost_bo *bo,
>          if (!bo)
>                  return;
>  
> -        if (_mesa_set_search(batch->bos, bo))
> +        struct hash_entry *entry;
> +        uint32_t old_flags = 0;
> +
> +        entry = _mesa_hash_table_search(batch->bos, bo);
> +        if (!entry) {
> +                entry = _mesa_hash_table_insert(batch->bos, bo,
> +                                                (void *)(uintptr_t)flags);
> +                panfrost_bo_reference(bo);
> +	} else {
> +                old_flags = (uintptr_t)entry->data;
> +        }
> +
> +        assert(entry);
> +
> +        if (old_flags == flags)
>                  return;
>  
> -        panfrost_bo_reference(bo);
> -        _mesa_set_add(batch->bos, bo);
> +        flags |= old_flags;
> +        entry->data = (void *)(uintptr_t)flags;
>  }
>  
>  void panfrost_batch_add_fbo_bos(struct panfrost_batch *batch)
> @@ -376,7 +387,7 @@ panfrost_batch_submit_ioctl(struct panfrost_batch *batch,
>          bo_handles = calloc(batch->bos->entries, sizeof(*bo_handles));
>          assert(bo_handles);
>  
> -        set_foreach(batch->bos, entry) {
> +        hash_table_foreach(batch->bos, entry) {
>                  struct panfrost_bo *bo = (struct panfrost_bo *)entry->key;
>                  assert(bo->gem_handle > 0);
>                  bo_handles[submit.bo_handle_count++] = bo->gem_handle;
> diff --git a/src/gallium/drivers/panfrost/pan_job.h b/src/gallium/drivers/panfrost/pan_job.h
> index 0b37a3131e86..3f2cf1a999f3 100644
> --- a/src/gallium/drivers/panfrost/pan_job.h
> +++ b/src/gallium/drivers/panfrost/pan_job.h
> @@ -98,7 +98,7 @@ struct panfrost_batch {
>          unsigned job_index;
>  
>          /* BOs referenced -- will be used for flushing logic */
> -        struct set *bos;
> +        struct hash_table *bos;
>  
>          /* Current transient BO */
>  	struct panfrost_bo *transient_bo;
> -- 
> 2.21.0
On Mon, 16 Sep 2019 10:00:13 -0400
Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:

> What if flags = 0?

Not sure what you have in mind. 0 would be a valid value (though not
really useful since that just means the BO is private and we don't give
any information on the type of access done on this BO). If you're
worried about having hentry->data = (uintptr_t)0 (IOW hentry->data =
NULL), I don't see the problem.

> 
> On Mon, Sep 16, 2019 at 11:37:01AM +0200, Boris Brezillon wrote:
> > So we can store the flags as data and keep the BO as a key. This way
> > we keep track of the type of access done on BOs.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  src/gallium/drivers/panfrost/pan_job.c | 33 +++++++++++++++++---------
> >  src/gallium/drivers/panfrost/pan_job.h |  2 +-
> >  2 files changed, 23 insertions(+), 12 deletions(-)
> > 
> > diff --git a/src/gallium/drivers/panfrost/pan_job.c b/src/gallium/drivers/panfrost/pan_job.c
> > index 6332529b2f9b..739f36a593f1 100644
> > --- a/src/gallium/drivers/panfrost/pan_job.c
> > +++ b/src/gallium/drivers/panfrost/pan_job.c
> > @@ -44,9 +44,8 @@ panfrost_create_batch(struct panfrost_context *ctx,
> >  
> >          batch->ctx = ctx;
> >  
> > -        batch->bos = _mesa_set_create(batch,
> > -                                      _mesa_hash_pointer,
> > -                                      _mesa_key_pointer_equal);
> > +        batch->bos = _mesa_hash_table_create(batch, _mesa_hash_pointer,
> > +                                             _mesa_key_pointer_equal);
> >  
> >          batch->minx = batch->miny = ~0;
> >          batch->maxx = batch->maxy = 0;
> > @@ -67,10 +66,8 @@ panfrost_free_batch(struct panfrost_batch *batch)
> >  
> >          struct panfrost_context *ctx = batch->ctx;
> >  
> > -        set_foreach(batch->bos, entry) {
> > -                struct panfrost_bo *bo = (struct panfrost_bo *)entry->key;
> > -                panfrost_bo_unreference(bo);
> > -        }
> > +        hash_table_foreach(batch->bos, entry)
> > +                panfrost_bo_unreference((struct panfrost_bo *)entry->key);
> >  
> >          _mesa_hash_table_remove_key(ctx->batches, &batch->key);
> >  
> > @@ -138,11 +135,25 @@ panfrost_batch_add_bo(struct panfrost_batch *batch, struct panfrost_bo *bo,
> >          if (!bo)
> >                  return;
> >  
> > -        if (_mesa_set_search(batch->bos, bo))
> > +        struct hash_entry *entry;
> > +        uint32_t old_flags = 0;
> > +
> > +        entry = _mesa_hash_table_search(batch->bos, bo);
> > +        if (!entry) {
> > +                entry = _mesa_hash_table_insert(batch->bos, bo,
> > +                                                (void *)(uintptr_t)flags);
> > +                panfrost_bo_reference(bo);
> > +	} else {
> > +                old_flags = (uintptr_t)entry->data;
> > +        }
> > +
> > +        assert(entry);
> > +
> > +        if (old_flags == flags)
> >                  return;
> >  
> > -        panfrost_bo_reference(bo);
> > -        _mesa_set_add(batch->bos, bo);
> > +        flags |= old_flags;
> > +        entry->data = (void *)(uintptr_t)flags;
> >  }
> >  
> >  void panfrost_batch_add_fbo_bos(struct panfrost_batch *batch)
> > @@ -376,7 +387,7 @@ panfrost_batch_submit_ioctl(struct panfrost_batch *batch,
> >          bo_handles = calloc(batch->bos->entries, sizeof(*bo_handles));
> >          assert(bo_handles);
> >  
> > -        set_foreach(batch->bos, entry) {
> > +        hash_table_foreach(batch->bos, entry) {
> >                  struct panfrost_bo *bo = (struct panfrost_bo *)entry->key;
> >                  assert(bo->gem_handle > 0);
> >                  bo_handles[submit.bo_handle_count++] = bo->gem_handle;
> > diff --git a/src/gallium/drivers/panfrost/pan_job.h b/src/gallium/drivers/panfrost/pan_job.h
> > index 0b37a3131e86..3f2cf1a999f3 100644
> > --- a/src/gallium/drivers/panfrost/pan_job.h
> > +++ b/src/gallium/drivers/panfrost/pan_job.h
> > @@ -98,7 +98,7 @@ struct panfrost_batch {
> >          unsigned job_index;
> >  
> >          /* BOs referenced -- will be used for flushing logic */
> > -        struct set *bos;
> > +        struct hash_table *bos;
> >  
> >          /* Current transient BO */
> >  	struct panfrost_bo *transient_bo;
> > -- 
> > 2.21.0
Well, the hash tables strongly assume you're not using NULLs for things.

See _mesa_hash_table_set_deleted_key for how to change that behaviour.

On Mon, Sep 16, 2019 at 04:17:37PM +0200, Boris Brezillon wrote:
> On Mon, 16 Sep 2019 10:00:13 -0400
> Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:
> 
> > What if flags = 0?
> 
> Not sure what you have in mind. 0 would be a valid value (though not
> really useful since that just means the BO is private and we don't give
> any information on the type of access done on this BO). If you're
> worried about having hentry->data = (uintptr_t)0 (IOW hentry->data =
> NULL), I don't see the problem.
> 
> > 
> > On Mon, Sep 16, 2019 at 11:37:01AM +0200, Boris Brezillon wrote:
> > > So we can store the flags as data and keep the BO as a key. This way
> > > we keep track of the type of access done on BOs.
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > ---
> > >  src/gallium/drivers/panfrost/pan_job.c | 33 +++++++++++++++++---------
> > >  src/gallium/drivers/panfrost/pan_job.h |  2 +-
> > >  2 files changed, 23 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/src/gallium/drivers/panfrost/pan_job.c b/src/gallium/drivers/panfrost/pan_job.c
> > > index 6332529b2f9b..739f36a593f1 100644
> > > --- a/src/gallium/drivers/panfrost/pan_job.c
> > > +++ b/src/gallium/drivers/panfrost/pan_job.c
> > > @@ -44,9 +44,8 @@ panfrost_create_batch(struct panfrost_context *ctx,
> > >  
> > >          batch->ctx = ctx;
> > >  
> > > -        batch->bos = _mesa_set_create(batch,
> > > -                                      _mesa_hash_pointer,
> > > -                                      _mesa_key_pointer_equal);
> > > +        batch->bos = _mesa_hash_table_create(batch, _mesa_hash_pointer,
> > > +                                             _mesa_key_pointer_equal);
> > >  
> > >          batch->minx = batch->miny = ~0;
> > >          batch->maxx = batch->maxy = 0;
> > > @@ -67,10 +66,8 @@ panfrost_free_batch(struct panfrost_batch *batch)
> > >  
> > >          struct panfrost_context *ctx = batch->ctx;
> > >  
> > > -        set_foreach(batch->bos, entry) {
> > > -                struct panfrost_bo *bo = (struct panfrost_bo *)entry->key;
> > > -                panfrost_bo_unreference(bo);
> > > -        }
> > > +        hash_table_foreach(batch->bos, entry)
> > > +                panfrost_bo_unreference((struct panfrost_bo *)entry->key);
> > >  
> > >          _mesa_hash_table_remove_key(ctx->batches, &batch->key);
> > >  
> > > @@ -138,11 +135,25 @@ panfrost_batch_add_bo(struct panfrost_batch *batch, struct panfrost_bo *bo,
> > >          if (!bo)
> > >                  return;
> > >  
> > > -        if (_mesa_set_search(batch->bos, bo))
> > > +        struct hash_entry *entry;
> > > +        uint32_t old_flags = 0;
> > > +
> > > +        entry = _mesa_hash_table_search(batch->bos, bo);
> > > +        if (!entry) {
> > > +                entry = _mesa_hash_table_insert(batch->bos, bo,
> > > +                                                (void *)(uintptr_t)flags);
> > > +                panfrost_bo_reference(bo);
> > > +	} else {
> > > +                old_flags = (uintptr_t)entry->data;
> > > +        }
> > > +
> > > +        assert(entry);
> > > +
> > > +        if (old_flags == flags)
> > >                  return;
> > >  
> > > -        panfrost_bo_reference(bo);
> > > -        _mesa_set_add(batch->bos, bo);
> > > +        flags |= old_flags;
> > > +        entry->data = (void *)(uintptr_t)flags;
> > >  }
> > >  
> > >  void panfrost_batch_add_fbo_bos(struct panfrost_batch *batch)
> > > @@ -376,7 +387,7 @@ panfrost_batch_submit_ioctl(struct panfrost_batch *batch,
> > >          bo_handles = calloc(batch->bos->entries, sizeof(*bo_handles));
> > >          assert(bo_handles);
> > >  
> > > -        set_foreach(batch->bos, entry) {
> > > +        hash_table_foreach(batch->bos, entry) {
> > >                  struct panfrost_bo *bo = (struct panfrost_bo *)entry->key;
> > >                  assert(bo->gem_handle > 0);
> > >                  bo_handles[submit.bo_handle_count++] = bo->gem_handle;
> > > diff --git a/src/gallium/drivers/panfrost/pan_job.h b/src/gallium/drivers/panfrost/pan_job.h
> > > index 0b37a3131e86..3f2cf1a999f3 100644
> > > --- a/src/gallium/drivers/panfrost/pan_job.h
> > > +++ b/src/gallium/drivers/panfrost/pan_job.h
> > > @@ -98,7 +98,7 @@ struct panfrost_batch {
> > >          unsigned job_index;
> > >  
> > >          /* BOs referenced -- will be used for flushing logic */
> > > -        struct set *bos;
> > > +        struct hash_table *bos;
> > >  
> > >          /* Current transient BO */
> > >  	struct panfrost_bo *transient_bo;
> > > -- 
> > > 2.21.0
On Mon, 16 Sep 2019 15:26:12 -0400
Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:

> Well, the hash tables strongly assume you're not using NULLs for things.
> 
> See _mesa_hash_table_set_deleted_key for how to change that behaviour.

Maybe I'm missing something, but AFAICT it's the key field that requires
special care, not the data one.
Ah, perhaps, yes. My bad.

On Tue, Sep 17, 2019 at 12:18:17AM +0200, Boris Brezillon wrote:
> On Mon, 16 Sep 2019 15:26:12 -0400
> Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:
> 
> > Well, the hash tables strongly assume you're not using NULLs for things.
> > 
> > See _mesa_hash_table_set_deleted_key for how to change that behaviour.
> 
> Maybe I'm missing something, but AFAICT it's the key field that requires
> special care, not the data one.