etna: more robust cmdbuffer flush handling

When something goes wrong during reserve, abort with an error, as it means
that the command buffer is in inconsistent state.

Also work around an apparent Vivante kernel driver bug that results in
hangs.

4 files changed, 48 insertions, 21 deletions

diff --git a/doc/kernel_interface.md b/doc/kernel_interface.md
index fb33e93..a2b2ef5 100644
--- a/doc/kernel_interface.md
+++ b/doc/kernel_interface.md
@@ -423,8 +423,9 @@ To enable profiling, the kernel most have been built with `VIVANTE_PROFILER` ena
     
 HW profiling registers can be read using the command `READ_ALL_PROFILE_REGISTERS`.
 
-There are also the commands `GET_PROFILE_SETTING` and `SET_PROFILE_SETTING`, apparently for logging to files, 
-but these aren't even implemented in the kernel drivers.
+There are also the commands `GET_PROFILE_SETTING` and `SET_PROFILE_SETTING`, which set a flag for 
+logging to a file (`vprofiler.xml` by default), but this flag doesn't do anything in the kernel driver,
+likely it's meant to be read out by the user space driver.
 
 This will return a structure `gcsPROFILER_COUNTERS`, defined in `GC_HAL_PROFILER.h`, which has the following timers:
 
diff --git a/native/driver/etna_pipe.c b/native/driver/etna_pipe.c
index ef97eab..01ed584 100644
--- a/native/driver/etna_pipe.c
+++ b/native/driver/etna_pipe.c
@@ -1559,10 +1559,13 @@ static void etna_pipe_flush(struct pipe_context *pipe,
     {
         if(etna_fence_new(pipe->screen, priv->ctx, fence) != ETNA_OK)
         {
-            printf("etna_pipe_flush: could not create fence\n");
+            printf("Error: %s: could not create fence\n", __func__);
         }
     }
-    etna_flush(priv->ctx);
+    if(etna_flush(priv->ctx) != ETNA_OK)
+    {
+        printf("Error: %s: etna_flush failed, GPU may be in unpredictable state\n", __func__);
+    }
 }
 
 static struct pipe_sampler_view *etna_pipe_create_sampler_view(struct pipe_context *pipe,
diff --git a/native/etnaviv/etna.c b/native/etnaviv/etna.c
index a6106e3..99b6d2d 100644
--- a/native/etnaviv/etna.c
+++ b/native/etnaviv/etna.c
@@ -46,11 +46,14 @@
 //#define DEBUG
 //#define DEBUG_CMDBUF
 
-/* TODO: don't forget to handle FE.VERTEX_ELEMENT_CONFIG (0x0600....0x0063c) specially;
- * fields need to be written in the right order, and only as many should be written as there are
- * used vertex elements.
- * header: contextbuf[contextbuf_addr[i].index - 1] where contextbuf_addr[i].address == 0x600
- */
+/* Maximum number of flushes without queuing a signal (per command buffer).
+   If this amount is reached, we roll to the next command buffer,
+   which automatically queues a signal.
+   XXX works around driver bug on (at least) cubox, for which drivers
+       is this not needed? Not urgent as this does not result in a
+       deducible performance impact.
+*/
+#define ETNA_MAX_UNSIGNALED_FLUSHES (40)
 
 /* Initialize kernel GPU context and state map */
 #ifdef GCABI_HAS_CONTEXT
@@ -302,24 +305,35 @@ int _etna_reserve_internal(struct etna_ctx *ctx, size_t n)
 #endif
     if((ctx->offset*4 + END_COMMIT_CLEARANCE) > COMMAND_BUFFER_SIZE)
     {
-        printf("%s: Command buffer overflow!\n", __func__);
+        printf("%s: Command buffer overflow! This is likely a programming error in the GPU driver.\n", __func__);
         abort();
     }
     if(ctx->cur_buf != -1)
     {
-#ifdef DEBUG
-        printf("Submitting old buffer\n");
+#if 0
+        printf("Submitting old buffer %i\n", ctx->cur_buf);
 #endif
         /* Queue signal to signify when buffer is available again */
-        if(etna_queue_signal(ctx->queue, ctx->cmdbuf_sig[ctx->cur_buf], VIV_WHERE_COMMAND) != 0)
-            return ETNA_INTERNAL_ERROR;
+        if((status = etna_queue_signal(ctx->queue, ctx->cmdbuf_sig[ctx->cur_buf], VIV_WHERE_COMMAND)) != ETNA_OK)
+        {
+            printf("%s: queue signal for old buffer failed: %i\n", __func__, status);
+            abort(); /* buffer is in invalid state XXX need some kind of recovery */
+        }
         /* Otherwise, if there is something to be committed left in the current command buffer, commit it */
         if((status = etna_flush(ctx)) != ETNA_OK)
-            return status;
+        {
+            printf("%s: reserve failed: %i\n", __func__, status);
+            abort(); /* buffer is in invalid state XXX need some kind of recovery */
+        }
     }
 
     /* Move on to next buffer if not enough free in current one */
-    status = switch_next_buffer(ctx);
+    if((status = switch_next_buffer(ctx)) != ETNA_OK)
+    {
+        printf("%s: can't switch to next command buffer: %i\n", __func__, status);
+        abort(); /* Buffer is in invalid state XXX need some kind of recovery.
+                    This could involve waiting and re-uploading the context state. */
+    }
     return status;
 }
 
@@ -344,6 +358,10 @@ int etna_flush(struct etna_ctx *ctx)
 #ifdef DEBUG_CMDBUF
     etna_dump_cmd_buffer(ctx);
 #endif
+    if(!queue_first)
+        ctx->flushes += 1;
+    else
+        ctx->flushes = 0;
     if((status = viv_commit(ctx->conn, cur_buf, ctx->ctx, queue_first)) != 0)
     {
 #ifdef DEBUG
@@ -370,16 +388,18 @@ int etna_flush(struct etna_ctx *ctx)
     /* TODO: if context was used, queue it to be freed later, and initialize new context buffer */
     cur_buf->startOffset = cur_buf->offset + END_COMMIT_CLEARANCE;
     cur_buf->offset = cur_buf->startOffset + BEGIN_COMMIT_CLEARANCE;
-    if((cur_buf->offset + END_COMMIT_CLEARANCE) >= COMMAND_BUFFER_SIZE)
+
+    if((cur_buf->offset + END_COMMIT_CLEARANCE) >= COMMAND_BUFFER_SIZE ||
+       ctx->flushes > ETNA_MAX_UNSIGNALED_FLUSHES)
     {
         /* nothing more fits in buffer, prevent warning about buffer overflow
            on next etna_reserve.
          */
-        ctx->offset = (COMMAND_BUFFER_SIZE - END_COMMIT_CLEARANCE) / 4;
-    } else {
-        /* set writing offset for next etna_reserve */
-        ctx->offset = cur_buf->offset / 4;
+        cur_buf->startOffset = cur_buf->offset = COMMAND_BUFFER_SIZE - END_COMMIT_CLEARANCE;
     }
+    /* Set writing offset for next etna_reserve. For convenience this is 
+       stored as an index instead of a byte offset.  */
+    ctx->offset = cur_buf->offset / 4;
 #ifdef DEBUG
 #ifdef GCABI_HAS_CONTEXT
     printf("  New start offset: %x New offset: %x Contextbuffer used: %i\n", cur_buf->startOffset, cur_buf->offset, *ctx->ctx.inUse);
@@ -392,6 +412,7 @@ nothing_to_do:
     /* Nothing in command buffer; but there may be kernel commands to submit. Do this seperately. */
     if(queue_first != NULL)
     {
+        ctx->flushes = 0;
         if((status = viv_event_commit(ctx->conn, queue_first)) != 0)
         {
 #ifdef DEBUG
diff --git a/native/etnaviv/etna.h b/native/etnaviv/etna.h
index cdcf728..1c3cc80 100644
--- a/native/etnaviv/etna.h
+++ b/native/etnaviv/etna.h
@@ -104,6 +104,8 @@ struct etna_ctx {
     struct _gcoCMDBUF *cmdbuf[NUM_COMMAND_BUFFERS];
     /* sync signals for command buffers */
     int cmdbuf_sig[NUM_COMMAND_BUFFERS];
+    /* number of unsignalled flushes (used to work around kernel bug) */
+    int flushes;
     /* context */
     void *ctx;
     /* command queue */