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@anholt.net>
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
Signed-off-by: Chad Versace <chad@chad-versace.us>

diff --git a/src/mesa/drivers/dri/intel/intel_batchbuffer.c b/src/mesa/drivers/dri/intel/intel_batchbuffer.c
index 37c13c967ab6f082d033f5c16eb2885421668b4e..8dfae677e8a9ca0d7d6397e633aeb214ce06c6f2 100644 (file)
--- 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 cf7ab9e665a2fd72f8546a241ab5e104e2ab60f0..cb316e82bedaa941b8245fbfc4de306e08a79950 100644 (file)
--- 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 7faf4ca40f519493671ae8bd04340373a2e5ea1b..67142a131620ddbdb00600fc2ce4f86412e6f834 100644 (file)
--- 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);
    }
 }