[PATCH v2 7/7] wined3d: Handle NOOVERWRITE maps on persistently mapped Vulkan buffers from the client thread.

[Zebediah Figura]

On 10/13/21 4:03 PM, Henri Verbeet wrote:
> On Tue, 12 Oct 2021 at 23:31, Zebediah Figura <zfigura at codeweavers.com> wrote:
>> +static void adapter_gl_flush_bo_address(struct wined3d_context *context,
>> +        const struct wined3d_const_bo_address *data, size_t size)
>> +{
>> +    FIXME("context %p, data %s, size %zu, stub!\n", context, debug_const_bo_address(data), size);
>> +}
>> +
> Note that we do have an implementation for this in
> wined3d_context_gl_unmap_bo_address().

I left it out of this patch for simplicity (this patch is already 
large), but I'm certainly open to adding it in.

> 
>> +static void flush_buffer_range(struct wined3d_context_vk *context_vk,
>> +        struct wined3d_bo_vk *bo, unsigned int offset, unsigned int size)
>> +{
>> +    struct wined3d_device_vk *device_vk = wined3d_device_vk(context_vk->c.device);
>> +    const struct wined3d_vk_info *vk_info = context_vk->vk_info;
>> +    VkMappedMemoryRange range;
>> +
>> +    range.sType = VK_STRUCTURE_TYPE_MAPPED_MEMORY_RANGE;
>> +    range.pNext = NULL;
>> +    range.memory = bo->vk_memory;
>> +    range.offset = bo->b.memory_offset + offset;
>> +    range.size = size;
>> +    VK_CALL(vkFlushMappedMemoryRanges(device_vk->vk_device, 1, &range));
>> +}
>> +
> flush_buffer_range() doesn't actually operate on a buffer though...

Fair enough; I'll rename it ;-)

> 
>> @@ -938,6 +938,7 @@ static HRESULT buffer_resource_sub_resource_map(struct wined3d_resource *resourc
>>                   addr.buffer_object = buffer->buffer_object;
>>                   addr.addr = 0;
>>                   buffer->map_ptr = wined3d_context_map_bo_address(context, &addr, resource->size, flags);
>> +                buffer->resource.client.addr = addr;
>>
> Does that update need to be atomic? I.e., is there any risk of the
> application thread seeing e.g. a new "addr.buffer_object" in
> combination with an old "addr.addr"? I think the idea is that we're
> protected from that by needing to go through the CS if the buffer
> isn't mapped yet, right?

Right, the client thread is waiting for this operation to complete and 
holding a mutex until it does, so it should be thread-safe.

I can add a comment to that effect if it'd help.

> 
>>   static bool wined3d_cs_map_upload_bo(struct wined3d_device_context *context, struct wined3d_resource *resource,
>>           unsigned int sub_resource_idx, struct wined3d_map_desc *map_desc, const struct wined3d_box *box, uint32_t flags)
>>   {
>> -    /* FIXME: We would like to return mapped or newly allocated memory here. */
>> +    /* Limit NOOVERWRITE maps to buffers for now; there are too many ways that
>> +     * a texture can be invalidated to even count. */
>> +    if (wined3d_map_persistent() && resource->type == WINED3D_RTYPE_BUFFER && (flags & WINED3D_MAP_NOOVERWRITE))
>> +    {
>> +        struct wined3d_client_resource *client = &resource->client;
>> +        const struct wined3d_bo *bo;
>> +        uint8_t *map_ptr;
>> +
>> +        bo = (const struct wined3d_bo *)client->addr.buffer_object;
>> +        map_ptr = bo ? bo->map_ptr : NULL;
>> +        map_ptr += (uintptr_t)client->addr.addr;
>> +
>> +        if (!map_ptr)
>> +        {
>> +            TRACE("Sub-resource is not persistently mapped.\n");
>> +            return false;
>> +        }
>> +
> So if "bo" is NULL, "map_ptr" can in theory end up being non-NULL
> here. I guess that in practice "client->addr.addr" is always going to
> be NULL if "bo" is, but in that case there's not much point in adding
> the offset before checking "map_ptr" either.
> 
>> +        wined3d_resource_get_sub_resource_map_pitch(resource, sub_resource_idx,
>> +                &map_desc->row_pitch, &map_desc->slice_pitch);
>> +
>> +        client->mapped_upload.addr = *wined3d_const_bo_address(&client->addr);
>> +        client->mapped_upload.flags = 0;
>> +        if (bo)
>> +            map_ptr += bo->memory_offset;
> Then we account for "bo" potentially being NULL here,
> 
>> +        map_desc->data = resource_offset_map_pointer(resource, sub_resource_idx, map_ptr, box);
>> +
>> +        if (!bo->coherent)
>> +            client->mapped_upload.flags |= UPLOAD_BO_UPLOAD_ON_UNMAP;
>> +
> but not here.
> 

I tried to account for the possibility that the persistent address for a 
buffer could be sysmem (which isn't done yet, but I'm inclined to 
understand is a reasonable potentiality), in which case the "!map_ptr" 
check should be correct as-is, but the "!bo->coherent" is clearly wrong. 
I'll fix the latter...