[Mesa-dev] [PATCH 08/16] i965: Pass flags to brw_bo_map_*

[Matt Turner]

On Fri, May 26, 2017 at 1:50 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Wednesday, May 24, 2017 1:04:50 PM PDT Matt Turner wrote:
>> brw_bo_map_cpu() took a write_enable arg, but it wasn't always clear
>> whether we were also planning to read from the buffer. I kept everything
>> semantically identical by passing only MAP_READ or MAP_READ | MAP_WRITE
>> depending on the write_enable argument.
>>
>> The other flags are not used yet, but MAP_ASYNC for instance, will be
>> used in a later patch to remove the need for a separate
>> brw_bo_map_unsynchronized() function.
> [snip]
>> diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.h b/src/mesa/drivers/dri/i965/brw_bufmgr.h
>> index 3dbde21..831da69 100644
>> --- a/src/mesa/drivers/dri/i965/brw_bufmgr.h
>> +++ b/src/mesa/drivers/dri/i965/brw_bufmgr.h
>> @@ -173,13 +173,23 @@ void brw_bo_reference(struct brw_bo *bo);
>>   */
>>  void brw_bo_unreference(struct brw_bo *bo);
>>
>> +/* Must match MapBufferRange interface (for convenience) */
>> +#define MAP_READ        GL_MAP_READ_BIT
>> +#define MAP_WRITE       GL_MAP_WRITE_BIT
>> +#define MAP_ASYNC       GL_MAP_UNSYNCHRONIZED_BIT
>> +#define MAP_PERSISTENT  GL_MAP_PERSISTENT_BIT
>> +#define MAP_COHERENT    GL_MAP_COHERENT_BIT
>> +/* internal */
>> +#define MAP_INTERNAL_MASK       (0xff << 24)
>> +#define MAP_RAW                 (0x01 << 24)
>> +
>>  /**
>>   * Maps the buffer into userspace.
>>   *
>>   * This function will block waiting for any existing execution on the
>>   * buffer to complete, first.  The resulting mapping is returned.
>>   */
>> -MUST_CHECK void *brw_bo_map_cpu(struct brw_context *brw, struct brw_bo *bo, int write_enable);
>> +MUST_CHECK void *brw_bo_map_cpu(struct brw_context *brw, struct brw_bo *bo, unsigned flags);
>>
>>  /**
>>   * Reduces the refcount on the userspace mapping of the buffer
> [snip]
>> diff --git a/src/mesa/drivers/dri/i965/intel_buffer_objects.c b/src/mesa/drivers/dri/i965/intel_buffer_objects.c
>> index 090a38c..cf6382d 100644
>> --- a/src/mesa/drivers/dri/i965/intel_buffer_objects.c
>> +++ b/src/mesa/drivers/dri/i965/intel_buffer_objects.c
>> @@ -328,6 +328,13 @@ brw_map_buffer_range(struct gl_context *ctx,
>>
>>     assert(intel_obj);
>>
>> +   STATIC_ASSERT(GL_MAP_UNSYNCHRONIZED_BIT == MAP_ASYNC);
>> +   STATIC_ASSERT(GL_MAP_WRITE_BIT == MAP_WRITE);
>> +   STATIC_ASSERT(GL_MAP_READ_BIT == MAP_READ);
>> +   STATIC_ASSERT(GL_MAP_PERSISTENT_BIT == MAP_PERSISTENT);
>> +   STATIC_ASSERT(GL_MAP_COHERENT_BIT == MAP_COHERENT);
>> +   assert((access & MAP_INTERNAL_MASK) == 0);
>> +
>>     /* _mesa_MapBufferRange (GL entrypoint) sets these, but the vbo module also
>>      * internally uses our functions directly.
>>      */
>> @@ -390,10 +397,9 @@ brw_map_buffer_range(struct gl_context *ctx,
>>                                                            alignment);
>>        void *map;
>>        if (brw->has_llc) {
>> -         map = brw_bo_map_cpu(brw, intel_obj->range_map_bo[index],
>> -                              (access & GL_MAP_WRITE_BIT) != 0);
>> +         map = brw_bo_map_cpu(brw, intel_obj->range_map_bo[index], access);
>>        } else {
>> -         map = brw_bo_map_gtt(brw, intel_obj->range_map_bo[index]);
>> +         map = brw_bo_map_gtt(brw, intel_obj->range_map_bo[index], access);
>>        }
>>        obj->Mappings[index].Pointer = map + intel_obj->map_extra[index];
>>        return obj->Mappings[index].Pointer;
>> @@ -408,10 +414,10 @@ brw_map_buffer_range(struct gl_context *ctx,
>>        map = brw_bo_map_unsynchronized(brw, intel_obj->buffer);
>>     } else if (!brw->has_llc && (!(access & GL_MAP_READ_BIT) ||
>>                                (access & GL_MAP_PERSISTENT_BIT))) {
>> -      map = brw_bo_map_gtt(brw, intel_obj->buffer);
>> +      map = brw_bo_map_gtt(brw, intel_obj->buffer, access);
>>        mark_buffer_inactive(intel_obj);
>>     } else {
>> -      map = brw_bo_map_cpu(brw, intel_obj->buffer, (access & GL_MAP_WRITE_BIT) != 0);
>> +      map = brw_bo_map_cpu(brw, intel_obj->buffer, access);
>>        mark_buffer_inactive(intel_obj);
>>     }
>>
>
> One thing that's a bit odd here..."access" may contain several bits
> we don't use in brw_bo_map(), and don't #define MAP_* aliases for:
>
>     GL_MAP_INVALIDATE_RANGE_BIT
>     GL_MAP_INVALIDATE_BUFFER_BIT
>     GL_MAP_FLUSH_EXPLICIT_BIT
>
> I guess it's harmless to pass them along...so if you think that's okay,
> I don't really have a problem with it...just thought it might be worth
> pointing out...

Yeah... seems like we can use these a lot more if and when we start
clflushing in userspace. I've put it on my TODO list.