[Mesa-dev] [PATCH 1/3] i965: Ensure that intel_bufferobj_map_range meets alignment guarantees

[Ian Romanick]

On 01/06/2014 10:43 AM, Kenneth Graunke wrote:
> On 01/03/2014 04:51 PM, Ian Romanick wrote:
>> From: Ian Romanick <ian.d.romanick at intel.com>
>>
>> No piglit regressions on IVB.
>>
>> With minor tweaks to the arb_map_buffer_alignment-map-invalidate-range
>> test (disable the extension check, set alignment to 64 instead of
>> querying), the i965 driver would fail the test without this patch (as
>> predicted by Eric).  With this patch, it passes.
>>
>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>> Cc: Eric Anholt <eric at anholt.net>
>> Cc: Siavash Eliasi <siavashserver at gmail.com>
>> ---
>>  src/mesa/drivers/dri/i965/intel_buffer_objects.c | 28 ++++++++++++++++++------
>>  1 file changed, 21 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_buffer_objects.c b/src/mesa/drivers/dri/i965/intel_buffer_objects.c
>> index cab805a..7bf8cae 100644
>> --- a/src/mesa/drivers/dri/i965/intel_buffer_objects.c
>> +++ b/src/mesa/drivers/dri/i965/intel_buffer_objects.c
>> @@ -359,20 +359,28 @@ intel_bufferobj_map_range(struct gl_context * ctx,
>>      */
>>     if ((access & GL_MAP_INVALIDATE_RANGE_BIT) &&
>>         drm_intel_bo_busy(intel_obj->buffer)) {
>> +      /* Ensure that the base alignment of the allocation meets the alignment
>> +       * guarantees the driver has advertised to the application.
>> +       */
>> +      const unsigned alignment = MAX2(64, ctx->Const.MinMapBufferAlignment);
> 
> The MAX2(64, ...) seems like unnecessary complexity.  We want the
> alignment to be ctx->Const.MinMapBufferAlignment (which should be bumped
> to >= 64 in a follow on patch).

I can do that.  I put that in mostly for testing before Siavash's
patches land.

> With that removed, patch 1 is:
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
> 
> Nice work.
> 
> I'd love to see a follow-up patch that:
> 1. Sets ctx->Const.MinMapBufferAlignment to 64
>    (in brw_context.c/brw_initialize_context_constants)
> 2. Enables the extension (in intel_extensions.c)
> 
> I know you'd like to turn it on for all drivers, but we may as well
> claim victory here in the meantime.
> 
>> +      const unsigned extra = (uintptr_t) offset % alignment;
>> +
>>        if (access & GL_MAP_FLUSH_EXPLICIT_BIT) {
>> -	 intel_obj->range_map_buffer = malloc(length);
>> -	 obj->Pointer = intel_obj->range_map_buffer;
>> +         intel_obj->range_map_buffer = _mesa_align_malloc(length + extra,
>> +                                                          alignment);
>> +         obj->Pointer = intel_obj->range_map_buffer + extra;
>>        } else {
>>  	 intel_obj->range_map_bo = drm_intel_bo_alloc(brw->bufmgr,
>>  						      "range map",
>> -						      length, 64);
>> +                                                      length + extra,
>> +                                                      alignment);
>>  	 if (!(access & GL_MAP_READ_BIT)) {
>>  	    drm_intel_gem_bo_map_gtt(intel_obj->range_map_bo);
>>  	 } else {
>>  	    drm_intel_bo_map(intel_obj->range_map_bo,
>>  			     (access & GL_MAP_WRITE_BIT) != 0);
>>  	 }
>> -	 obj->Pointer = intel_obj->range_map_bo->virtual;
>> +	 obj->Pointer = intel_obj->range_map_bo->virtual + extra;
>>        }
>>        return obj->Pointer;
>>     }
>> @@ -424,7 +432,11 @@ intel_bufferobj_flush_mapped_range(struct gl_context *ctx,
>>  
>>     temp_bo = drm_intel_bo_alloc(brw->bufmgr, "range map flush", length, 64);
>>  
>> -   drm_intel_bo_subdata(temp_bo, 0, length, intel_obj->range_map_buffer);
>> +   /* Use obj->Pointer instead of intel_obj->range_map_buffer because the
>> +    * former points to the actual mapping while the latter may be offset to
>> +    * meet alignment guarantees.
>> +    */
>> +   drm_intel_bo_subdata(temp_bo, 0, length, obj->Pointer);
>>  
>>     intel_emit_linear_blit(brw,
>>  			  intel_obj->buffer, obj->Offset + offset,
>> @@ -456,14 +468,16 @@ intel_bufferobj_unmap(struct gl_context * ctx, struct gl_buffer_object *obj)
>>         * usage inside of a batchbuffer.
>>         */
>>        intel_batchbuffer_emit_mi_flush(brw);
>> -      free(intel_obj->range_map_buffer);
>> +      _mesa_align_free(intel_obj->range_map_buffer);
>>        intel_obj->range_map_buffer = NULL;
>>     } else if (intel_obj->range_map_bo != NULL) {
>> +      const unsigned extra = obj->Pointer - intel_obj->range_map_bo->virtual;
>> +
>>        drm_intel_bo_unmap(intel_obj->range_map_bo);
>>  
>>        intel_emit_linear_blit(brw,
>>  			     intel_obj->buffer, obj->Offset,
>> -			     intel_obj->range_map_bo, 0,
>> +			     intel_obj->range_map_bo, extra,
>>  			     obj->Length);
>>        intel_bufferobj_mark_gpu_usage(intel_obj, obj->Offset, obj->Length);
>>  
>>