panfrost: Fix ->set_vertex_buffers() for partial vertex bufs updates

Submitted by Boris Brezillon on April 12, 2019, 7:20 a.m.

Details

Message ID 20190412072031.17061-1-boris.brezillon@collabora.com
State New
Headers show
Series "panfrost: Fix ->set_vertex_buffers() for partial vertex bufs updates" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Boris Brezillon April 12, 2019, 7:20 a.m.
The caller of ->set_vertex_buffers() might want to update only a subset
of the currently active vertex buffers, hence the start_slot and
num_buffers arguments.
The panfrost driver was assuming that the whole set of vertex buffers
were to be freed/updated each time this hook is called, which leads to
issues when the caller want to update only some of those buffers (this
is what the gallium-cso layer does when it saves/restores the vertex
buf context).

Fix the logic to allow partial vertex bufs updates.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Hi,

I faced this issue while trying to make gallium-hud work with the
panfrost driver. I'm still struggling to fix at least 3 remaining
issues:

- the Y coordinates are inverted, and the HUD is displayed on the
  bottom-left corner instead of top-left corner, plus it's mirrored
- transparency/alpha-blending does not work correctly (I get black
  squares instead of semi transparent grey ones)
- the fonts shader (or something related to it) does not seem to work
  (no legend on the graph)

If anyone has an idea where this could come from, please let me know.

Thanks,

Boris
---
 src/gallium/drivers/panfrost/pan_context.c | 34 ++++++++++++----------
 src/gallium/drivers/panfrost/pan_context.h |  3 +-
 2 files changed, 20 insertions(+), 17 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/drivers/panfrost/pan_context.c b/src/gallium/drivers/panfrost/pan_context.c
index 9f401b1a7a12..7c346e475269 100644
--- a/src/gallium/drivers/panfrost/pan_context.c
+++ b/src/gallium/drivers/panfrost/pan_context.c
@@ -742,9 +742,13 @@  panfrost_emit_vertex_data(struct panfrost_context *ctx)
         union mali_attr attrs[PIPE_MAX_ATTRIBS];
 
         unsigned invocation_count = MALI_NEGATIVE(ctx->payload_tiler.prefix.invocation_count);
+        unsigned vertex_buffer_count = 0;
+
+        for (int i = 0; i < ARRAY_SIZE(ctx->vertex_buffers); ++i) {
+                struct pipe_vertex_buffer *buf = ctx->vertex_buffers[i];
+                if (!buf)
+                        continue;
 
-        for (int i = 0; i < ctx->vertex_buffer_count; ++i) {
-                struct pipe_vertex_buffer *buf = &ctx->vertex_buffers[i];
                 struct panfrost_resource *rsrc = (struct panfrost_resource *) (buf->buffer.resource);
 
                 /* Let's figure out the layout of the attributes in memory so
@@ -782,9 +786,10 @@  panfrost_emit_vertex_data(struct panfrost_context *ctx)
                 } else {
                         /* Leave unset? */
                 }
+                vertex_buffer_count++;
         }
 
-        ctx->payload_vertex.postfix.attributes = panfrost_upload_transient(ctx, attrs, ctx->vertex_buffer_count * sizeof(union mali_attr));
+        ctx->payload_vertex.postfix.attributes = panfrost_upload_transient(ctx, attrs, vertex_buffer_count * sizeof(union mali_attr));
 
         panfrost_emit_varying_descriptor(ctx, invocation_count);
 }
@@ -1817,21 +1822,20 @@  panfrost_set_vertex_buffers(
         const struct pipe_vertex_buffer *buffers)
 {
         struct panfrost_context *ctx = pan_context(pctx);
-        assert(num_buffers <= PIPE_MAX_ATTRIBS);
+        unsigned i;
+        assert(start_slot + num_buffers <= PIPE_MAX_ATTRIBS);
 
-        /* XXX: Dirty tracking? etc */
-        if (buffers) {
-                size_t sz = sizeof(buffers[0]) * num_buffers;
-                ctx->vertex_buffers = malloc(sz);
-                ctx->vertex_buffer_count = num_buffers;
-                memcpy(ctx->vertex_buffers, buffers, sz);
-        } else {
-                if (ctx->vertex_buffers) {
-                        free(ctx->vertex_buffers);
-                        ctx->vertex_buffers = NULL;
+        for (i = start_slot; i < start_slot + num_buffers; i++) {
+                if (ctx->vertex_buffers[i]) {
+                        free(ctx->vertex_buffers[i]);
+                        ctx->vertex_buffers[i] = NULL;
                 }
 
-                ctx->vertex_buffer_count = 0;
+                if (!buffers)
+                        continue;
+
+                ctx->vertex_buffers[i] = malloc(sizeof(*buffers));
+                memcpy(ctx->vertex_buffers[i], buffers + i, sizeof(*buffers));
         }
 }
 
diff --git a/src/gallium/drivers/panfrost/pan_context.h b/src/gallium/drivers/panfrost/pan_context.h
index d071da1c62fa..2a10fede334a 100644
--- a/src/gallium/drivers/panfrost/pan_context.h
+++ b/src/gallium/drivers/panfrost/pan_context.h
@@ -187,8 +187,7 @@  struct panfrost_context {
 
         struct panfrost_vertex_state *vertex;
 
-        struct pipe_vertex_buffer *vertex_buffers;
-        unsigned vertex_buffer_count;
+        struct pipe_vertex_buffer *vertex_buffers[PIPE_MAX_ATTRIBS];
 
         struct panfrost_sampler_state *samplers[PIPE_SHADER_TYPES][PIPE_MAX_SAMPLERS];
         unsigned sampler_count[PIPE_SHADER_TYPES];

Comments

Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>

Looks great! pan_context.c is definitely some of the most fragile code
in the driver right now, so I'm thrilled to get some attention to clean
it up :)

(Not tested by me yet -- I won't be able to test until next week)
The patches I just posted to the list conflict with this but I think do
what you want. Could you try those?

Thanks,
-AR