Mesa (master): intel: Assert that no batch is emitted if a region is mapped

[Chad Versace]

Module: Mesa
Branch: master
Commit: e9adfa2ba1af9c3579b25327335c47118b6c7c3f
URL:    http://cgit.freedesktop.org/mesa/mesa/commit/?id=e9adfa2ba1af9c3579b25327335c47118b6c7c3f

Author: Chad Versace <chad at chad-versace.us>
Date:   Thu Oct  6 14:18:35 2011 -0700

intel: Assert that no batch is emitted if a region is mapped

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.

Reviewed-by: Eric Anholt <eric at anholt.net>
Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
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..8dfae67 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 cf7ab9e..cb316e8 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);
    }
 }