[PATCH 8/8] wined3d: Try to allocate new Vulkan BOs from the client thread for DISCARD maps.

[Zebediah Figura]

On 11/4/21 11:19 AM, Matteo Bruni wrote:
> On Thu, Nov 4, 2021 at 4:45 PM Zebediah Figura <zfigura at codeweavers.com> wrote:
>>
>> On 11/4/21 8:17 AM, Henri Verbeet wrote:
>>> On Wed, 3 Nov 2021 at 21:47, Zebediah Figura <zfigura at codeweavers.com> wrote:
>>>> On 11/3/21 12:11 PM, Henri Verbeet wrote:
>>>>> On Wed, 3 Nov 2021 at 00:37, Zebediah Figura <zfigura at codeweavers.com> wrote:
>>>>>> +static bool adapter_vk_alloc_bo(struct wined3d_device *device, struct wined3d_resource *resource,
>>>>>> +        unsigned int sub_resource_idx, struct wined3d_bo_address *addr)
>>>>>> +{
>>>>>> +    wined3d_not_from_cs(device->cs);
>>>>>> +
>>>>>> +    if (resource->type == WINED3D_RTYPE_BUFFER)
>>>>>> +    {
>>>>>> +        struct wined3d_buffer_vk *buffer_vk = wined3d_buffer_vk(buffer_from_resource(resource));
>>>>>> +        struct wined3d_client_bo_vk *client_bo;
>>>>>> +
>>>>>> +        if (!(client_bo = heap_alloc(sizeof(*client_bo))))
>>>>>> +            return false;
>>>>>> +
>>>>>> +        if (!wined3d_buffer_vk_create_buffer_object(buffer_vk, NULL, &client_bo->bo))
>>>>>> +        {
>>>>>> +            heap_free(client_bo);
>>>>>> +            return false;
>>>>>> +        }
>>>>>> +
>>>>>> +        if (!client_bo->bo.b.map_ptr)
>>>>>> +        {
>>>>>> +            struct wined3d_client_bo_vk_map_ctx ctx = {.device = device, .client_bo = client_bo};
>>>>>> +
>>>>>> +            WARN_(d3d_perf)("BO %p (chunk %p, slab %p) is not persistently mapped.\n", &client_bo->bo,
>>>>>> +                    client_bo->bo.memory ? client_bo->bo.memory->chunk : NULL, client_bo->bo.slab);
>>>>>> +
>>>>>> +            wined3d_cs_map_object(device->cs, wined3d_client_bo_vk_map_cs, &ctx);
>>>>>> +            wined3d_cs_finish(device->cs, WINED3D_CS_QUEUE_MAP);
>>>>>> +        }
>>>>> wined3d_cs_map_object() almost sounds like it would emit a
>>>>> WINED3D_CS_OP_MAP, somewhat like wined3d_device_context_map() back
>>>>> when that was still called wined3d_cs_map()...
>>>>
>>>> Indeed, except that WINED3D_CS_OP_MAP takes the resource. I don't think
>>>> we want to change that, either, although we could create a new CS op
>>>> instead of using WINED3D_CS_OP_CALLBACK.
>>>>
>>> Yeah, I'm not suggesting we change WINED3D_CS_OP_MAP. It's somewhat
>>> moot for the Vulkan backend now, but I would argue that if we would
>>> have to go through the CS to map the bo anyway, we might as well
>>> either give up and just map the resource through the CS, or allocate
>>> the staging memory on the CPU.
>>
>> I think the motivation for not just doing that was to make sure that we
>> actually did allocate new memory, instead of repeatedly going through
>> the slow path to use the same mapped memory. I don't remember if this
>> happened in practice, but there's a comment to similar effect on one of
>> Matteo's patches...
> 
> That was more or less the idea, although the whole mechanism was a bit
> different (and with GL posing more constraints). Specifically, we
> can't / don't want to make GL calls from non-CS threads, but at the
> same time we want to be able to create new BOs for DISCARD maps
> (either because we use a separate BO for each wined3d buffer DISCARD
> map or because we're suballocating and there is no free space but we
> want to trigger the allocation of a new BO to suballocate from -
> basically the same as the non-slab case of
> wined3d_context_vk_create_bo()).
> So yeah, I don't think you have to care about that case here in the VK
> callback and it's probably nicer to do what Henri suggested i.e. go
> explicitly through the CS for a "slow" alloc, since that way the
> fallback is in generic code.
> 
> Assuming I understood the whole thing correctly, I'm not up to speed
> with this as much as I'd like...
> 

For Vulkan I think it doesn't matter, since we can just map from the 
client thread. For GL we have to to map from the CS thread, but if we 
just map the old resource via wined3d_resource_map(), we'll use 
&wined3d_buffer_gl.bo instead of allocating new memory, which means that 
the client will continue to have no accessible memory to return for a 
discard map. Repeat ad infinitum.

Allocating sysmem would help, but my understanding is that expanding the 
available GPU memory pool would be better.

That's the theoretical problem; I don't remember if it caused problems 
in practice, but I have a suspicion that it did...