[Mesa-dev] [PATCH] intel: Assert that no batch is emitted if a region is mapped [v2]

[Chad Versace]

What I would prefer to assert is that, for each region that is currently
mapped, no batch is emitted that uses that region's bo. However, it's much
easier to implement this big hammer.

Observe that this requires that the batch flush in intel_region_map() be
moved to within the map_refcount guard.

v2: Add comments (borrowed from anholt's reply) explaining why the
assertion is a good idea.

CC: Eric Anholt <eric at anholt.net>
CC: Ian Romanick <idr at freedesktop.org>
Signed-off-by: Chad Versace <chad at chad-versace.us>
---
 src/mesa/drivers/dri/intel/intel_batchbuffer.c |    7 +++++++
 src/mesa/drivers/dri/intel/intel_context.h     |    8 ++++++++
 src/mesa/drivers/dri/intel/intel_regions.c     |   18 +++++++++++++++++-
 3 files changed, 32 insertions(+), 1 deletions(-)

diff --git a/src/mesa/drivers/dri/intel/intel_batchbuffer.c b/src/mesa/drivers/dri/intel/intel_batchbuffer.c
index 37c13c9..8dfae677 100644
--- a/src/mesa/drivers/dri/intel/intel_batchbuffer.c
+++ b/src/mesa/drivers/dri/intel/intel_batchbuffer.c
@@ -148,6 +148,13 @@ void
 _intel_batchbuffer_flush(struct intel_context *intel,
 			 const char *file, int line)
 {
+   /* No batch should be emitted that uses a mapped region, because that would
+    * cause the map to be incoherent with GPU rendering done by the
+    * batchbuffer. To ensure that condition, we assert a condition that is
+    * stronger but easier to implement: that *no* region is mapped.
+    */
+   assert(intel->num_mapped_regions == 0);
+
    if (intel->batch.used == 0)
       return;
 
diff --git a/src/mesa/drivers/dri/intel/intel_context.h b/src/mesa/drivers/dri/intel/intel_context.h
index eb78c00..f3d0bcc 100644
--- a/src/mesa/drivers/dri/intel/intel_context.h
+++ b/src/mesa/drivers/dri/intel/intel_context.h
@@ -287,6 +287,14 @@ struct intel_context
     */
    GLboolean is_front_buffer_reading;
 
+   /**
+    * Count of intel_regions that are mapped.
+    *
+    * This allows us to assert that no batch buffer is emitted if a
+    * region is mapped.
+    */
+   int num_mapped_regions;
+
    GLboolean use_texture_tiling;
    GLboolean use_early_z;
 
diff --git a/src/mesa/drivers/dri/intel/intel_regions.c b/src/mesa/drivers/dri/intel/intel_regions.c
index 7faf4ca..67142a1 100644
--- a/src/mesa/drivers/dri/intel/intel_regions.c
+++ b/src/mesa/drivers/dri/intel/intel_regions.c
@@ -111,15 +111,28 @@ debug_backtrace(void)
 GLubyte *
 intel_region_map(struct intel_context *intel, struct intel_region *region)
 {
-   intel_flush(&intel->ctx);
+   /* We have the region->map_refcount controlling mapping of the BO because
+    * in software fallbacks we may end up mapping the same buffer multiple
+    * times on Mesa's behalf, so we refcount our mappings to make sure that
+    * the pointer stays valid until the end of the unmap chain.  However, we
+    * must not emit any batchbuffers between the start of mapping and the end
+    * of unmapping, or further use of the map will be incoherent with the GPU
+    * rendering done by that batchbuffer. Hence we assert in
+    * intel_batchbuffer_flush() that that doesn't happen, which means that the
+    * flush is only needed on first map of the buffer.
+    */
 
    _DBG("%s %p\n", __FUNCTION__, region);
    if (!region->map_refcount++) {
+      intel_flush(&intel->ctx);
+
       if (region->tiling != I915_TILING_NONE)
 	 drm_intel_gem_bo_map_gtt(region->bo);
       else
 	 drm_intel_bo_map(region->bo, GL_TRUE);
+
       region->map = region->bo->virtual;
+      ++intel->num_mapped_regions;
    }
 
    return region->map;
@@ -134,7 +147,10 @@ intel_region_unmap(struct intel_context *intel, struct intel_region *region)
 	 drm_intel_gem_bo_unmap_gtt(region->bo);
       else
 	 drm_intel_bo_unmap(region->bo);
+
       region->map = NULL;
+      --intel->num_mapped_regions;
+      assert(intel->num_mapped_regions >= 0);
    }
 }
 
-- 
1.7.6.4