gallium/osmesa: Fix flushing and Y-flipping of the depth buffer.

We were returning a pointer to use-after-free the depth buffer, not
updating it in after future rendering, and also not y flipping it.  A
little refactor to mostly reuse the color buffer's path makes it easy to
do it all right.

Adds a unit test to check for these bugs.

Closes: #885
Reviewed-by: Adam Jackson <ajax@redhat.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/7886>

2 files changed, 115 insertions, 49 deletions

diff --git a/src/gallium/frontends/osmesa/osmesa.c b/src/gallium/frontends/osmesa/osmesa.c
index 42cc8256027..eae487c5c03 100644
--- a/src/gallium/frontends/osmesa/osmesa.c
+++ b/src/gallium/frontends/osmesa/osmesa.c
@@ -101,6 +101,13 @@ struct osmesa_context
 
    struct osmesa_buffer *current_buffer;
 
+   /* Storage for depth/stencil, if the user has requested access.  The backing
+    * driver always has its own storage for the actual depth/stencil, which we
+    * have to transfer in and out.
+    */
+   void *zs;
+   unsigned zs_stride;
+
    enum pipe_format depth_stencil_format, accum_format;
 
    GLenum format;         /*< User-specified context format */
@@ -176,6 +183,43 @@ get_st_manager(void)
    return stmgr;
 }
 
+/* Reads the color or depth buffer from the backing context to either the user storage
+ * (color buffer) or our temporary (z/s)
+ */
+static void
+osmesa_read_buffer(OSMesaContext osmesa, struct pipe_resource *res, void *dst,
+                   int dst_stride, bool y_up)
+{
+   struct pipe_context *pipe = osmesa->stctx->pipe;
+
+   struct pipe_box box;
+   u_box_2d(0, 0, res->width0, res->height0, &box);
+
+   struct pipe_transfer *transfer = NULL;
+   ubyte *src = pipe->transfer_map(pipe, res, 0, PIPE_MAP_READ, &box,
+                                   &transfer);
+
+   /*
+    * Copy the color buffer from the resource to the user's buffer.
+    */
+
+   if (y_up) {
+      /* need to flip image upside down */
+      dst = (ubyte *)dst + (res->height0 - 1) * dst_stride;
+      dst_stride = -dst_stride;
+   }
+
+   unsigned bpp = util_format_get_blocksize(res->format);
+   for (unsigned y = 0; y < res->height0; y++)
+   {
+      memcpy(dst, src, bpp * res->width0);
+      dst = (ubyte *)dst + dst_stride;
+      src += transfer->stride;
+   }
+
+   pipe->transfer_unmap(pipe, transfer);
+}
+
 
 /**
  * Given an OSMESA_x format and a GL_y type, return the best
@@ -316,13 +360,8 @@ osmesa_st_framebuffer_flush_front(struct st_context_iface *stctx,
 {
    OSMesaContext osmesa = OSMesaGetCurrentContext();
    struct osmesa_buffer *osbuffer = stfbi_to_osbuffer(stfbi);
-   struct pipe_context *pipe = stctx->pipe;
    struct pipe_resource *res = osbuffer->textures[statt];
-   struct pipe_transfer *transfer = NULL;
-   struct pipe_box box;
-   void *map;
-   ubyte *src, *dst;
-   unsigned y, bytes, bpp;
+   unsigned bpp;
    int dst_stride;
 
    if (osmesa->pp) {
@@ -347,37 +386,21 @@ osmesa_st_framebuffer_flush_front(struct st_context_iface *stctx,
       pp_run(osmesa->pp, res, res, zsbuf);
    }
 
-   u_box_2d(0, 0, res->width0, res->height0, &box);
-
-   map = pipe->transfer_map(pipe, res, 0, PIPE_MAP_READ, &box,
-                            &transfer);
-
-   /*
-    * Copy the color buffer from the resource to the user's buffer.
-    */
+   /* Snapshot the color buffer to the user's buffer. */
    bpp = util_format_get_blocksize(osbuffer->visual.color_format);
-   src = map;
-   dst = osbuffer->map;
    if (osmesa->user_row_length)
       dst_stride = bpp * osmesa->user_row_length;
    else
       dst_stride = bpp * osbuffer->width;
-   bytes = bpp * res->width0;
 
-   if (osmesa->y_up) {
-      /* need to flip image upside down */
-      dst = dst + (res->height0 - 1) * dst_stride;
-      dst_stride = -dst_stride;
-   }
+   osmesa_read_buffer(osmesa, res, osbuffer->map, dst_stride, osmesa->y_up);
 
-   for (y = 0; y < res->height0; y++) {
-      memcpy(dst, src, bytes);
-      dst += dst_stride;
-      src += transfer->stride;
+   /* If the user has requested the Z/S buffer, then snapshot that one too. */
+   if (osmesa->zs) {
+      osmesa_read_buffer(osmesa, osbuffer->textures[ST_ATTACHMENT_DEPTH_STENCIL],
+                         osmesa->zs, osmesa->zs_stride, true);
    }
 
-   pipe->transfer_unmap(pipe, transfer);
-
    return true;
 }
 
@@ -730,6 +753,7 @@ OSMesaDestroyContext(OSMesaContext osmesa)
    if (osmesa) {
       pp_free(osmesa->pp);
       osmesa->stctx->destroy(osmesa->stctx);
+      free(osmesa->zs);
       FREE(osmesa);
    }
 }
@@ -914,33 +938,22 @@ OSMesaGetDepthBuffer(OSMesaContext c, GLint *width, GLint *height,
                      GLint *bytesPerValue, void **buffer)
 {
    struct osmesa_buffer *osbuffer = c->current_buffer;
-   struct pipe_context *pipe = c->stctx->pipe;
    struct pipe_resource *res = osbuffer->textures[ST_ATTACHMENT_DEPTH_STENCIL];
-   struct pipe_transfer *transfer = NULL;
-   struct pipe_box box;
-
-   /*
-    * Note: we can't really implement this function with gallium as
-    * we did for swrast.  We can't just map the resource and leave it
-    * mapped (and there's no OSMesaUnmapDepthBuffer() function) so
-    * we unmap the buffer here and return a 'stale' pointer.  This should
-    * actually be OK in most cases where the caller of this function
-    * immediately uses the pointer.
-    */
-
-   u_box_2d(0, 0, res->width0, res->height0, &box);
-
-   *buffer = pipe->transfer_map(pipe, res, 0, PIPE_MAP_READ, &box,
-                                &transfer);
-   if (!*buffer) {
-      return GL_FALSE;
-   }
 
    *width = res->width0;
    *height = res->height0;
    *bytesPerValue = util_format_get_blocksize(res->format);
 
-   pipe->transfer_unmap(pipe, transfer);
+   if (!c->zs) {
+      c->zs_stride = *width * *bytesPerValue;
+      c->zs = calloc(c->zs_stride, *height);
+      if (!c->zs)
+         return GL_FALSE;
+
+      osmesa_read_buffer(c, res, c->zs, c->zs_stride, true);
+   }
+
+   *buffer = c->zs;
 
    return GL_TRUE;
 }
diff --git a/src/gallium/targets/osmesa/test-render.cpp b/src/gallium/targets/osmesa/test-render.cpp
index 2e5ff962d8e..1a720f0cd0e 100644
--- a/src/gallium/targets/osmesa/test-render.cpp
+++ b/src/gallium/targets/osmesa/test-render.cpp
@@ -162,3 +162,56 @@ INSTANTIATE_TEST_CASE_P(
    ),
    name_params
 );
+
+TEST(OSMesaRenderTest, depth)
+{
+   std::unique_ptr<osmesa_context, decltype(&OSMesaDestroyContext)> ctx{
+      OSMesaCreateContextExt(OSMESA_RGB_565, 24, 8, 0, NULL), &OSMesaDestroyContext};
+   ASSERT_TRUE(ctx);
+
+   int w = 3, h = 2;
+   uint8_t pixels[4096 * h * 2] = {0}; /* different cpp from our depth! */
+   auto ret = OSMesaMakeCurrent(ctx.get(), &pixels, GL_UNSIGNED_SHORT_5_6_5, w, h);
+   ASSERT_EQ(ret, GL_TRUE);
+
+   /* Expand the row length for the color buffer so we can see that it doesn't affect depth. */
+   OSMesaPixelStore(OSMESA_ROW_LENGTH, 4096);
+
+   uint32_t *depth;
+   GLint dw, dh, depth_cpp;
+   ASSERT_EQ(true, OSMesaGetDepthBuffer(ctx.get(), &dw, &dh, &depth_cpp, (void **)&depth));
+
+   ASSERT_EQ(dw, w);
+   ASSERT_EQ(dh, h);
+   ASSERT_EQ(depth_cpp, 4);
+
+   glClearDepth(1.0);
+   glClear(GL_DEPTH_BUFFER_BIT);
+   glFinish();
+   EXPECT_EQ(depth[w * 0 + 0], 0x00ffffff);
+   EXPECT_EQ(depth[w * 0 + 1], 0x00ffffff);
+   EXPECT_EQ(depth[w * 1 + 0], 0x00ffffff);
+   EXPECT_EQ(depth[w * 1 + 1], 0x00ffffff);
+
+   /* Scissor to the top half and clear */
+   glEnable(GL_SCISSOR_TEST);
+   glScissor(0, 1, 2, 1);
+   glClearDepth(0.0);
+   glClear(GL_DEPTH_BUFFER_BIT);
+   glFinish();
+   EXPECT_EQ(depth[w * 0 + 0], 0x00ffffff);
+   EXPECT_EQ(depth[w * 0 + 1], 0x00ffffff);
+   EXPECT_EQ(depth[w * 1 + 0], 0x00000000);
+   EXPECT_EQ(depth[w * 1 + 1], 0x00000000);
+
+   /* Y_UP didn't affect depth buffer orientation in classic osmesa. */
+   OSMesaPixelStore(OSMESA_Y_UP, false);
+   glScissor(0, 1, 1, 1);
+   glClearDepth(1.0);
+   glClear(GL_DEPTH_BUFFER_BIT);
+   glFinish();
+   EXPECT_EQ(depth[w * 0 + 0], 0x00ffffff);
+   EXPECT_EQ(depth[w * 0 + 1], 0x00ffffff);
+   EXPECT_EQ(depth[w * 1 + 0], 0x00ffffff);
+   EXPECT_EQ(depth[w * 1 + 1], 0x00000000);
+}