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

[Ian Romanick]

On 10/10/2011 11:17 AM, Eric Anholt wrote:
> On Thu,  6 Oct 2011 14:18:35 -0700, Chad Versace<chad at chad-versace.us>  wrote:
>> 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.
>
>> diff --git a/src/mesa/drivers/dri/intel/intel_regions.c b/src/mesa/drivers/dri/intel/intel_regions.c
>> index 7faf4ca..d46c470 100644
>> --- a/src/mesa/drivers/dri/intel/intel_regions.c
>> +++ b/src/mesa/drivers/dri/intel/intel_regions.c
>> @@ -111,15 +111,17 @@ debug_backtrace(void)
>>   GLubyte *
>>   intel_region_map(struct intel_context *intel, struct intel_region *region)
>>   {
>> -   intel_flush(&intel->ctx);
>> -
>>      _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;
>>      }
>
> There's a more interesting change in here, which is the move of the
> flush inside of the if statement.
>
> We agreed that moving it in was correct, but it would be nice to record
> why we thought it was correct.  The thinking I recall was:
>
> "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.  Thus we should assert that that
> doesn't happen, which means that the flush is only needed on first map
> of the buffer."

I was going to comment that the first patch change behavior.  Having the 
above text as a comment in the code is a good idea.