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

[Zebediah Figura]

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...

> 
>> (Now that I think about it, *is* there any sort of a rule as to what
>> deserves WINED3D_CS_OP_CALLBACK and what doesn't? I know that currently
>> it's just a matter of init and destroy, but it seeems a bit arbitrary on
>> reflection.)
>>
> For the most part, it's things that I don't think should have their
> own wined3d_cs_op. WINED3D_CS_OP_CALLBACK is a little ugly, but it
> works well enough for the moment. I suppose we could do something
> along the lines of the following:
> 
>      struct wined3d_cs_resource_ops
>      {
>          void (*cs_resource_init)(struct wined3d_cs_resource *resource);
>          void (*cs_resource_cleanup)(struct wined3d_cs_resource *resource);
>      };
> 
>      struct wined3d_cs_resource
>      {
>          struct wined3d_cs_resource_ops *ops;
>      };
> 
> and then
> 
>      struct wined3d_cs_init_resource
>      {
>          enum wined3d_cs_op opcode;
>          struct wined3d_cs_resource *resource;
>      };
> 
>      static void wined3d_cs_exec_init_resource(struct wined3d_cs *cs,
> const void *data)
>      {
>          const struct wined3d_cs_init_resource *op = data;
> 
>          op->resource->ops->cs_resource_init(op->resource);
>      }
> 
> and equivalent cleanup variants. Something like wined3d_sampler_vk
> would then include a wined3d_cs_resource structure, and call
> "wined3d_cs_init_object(device->cs, &sampler_vk->cs_resource);". That
> ends up being largely equivalent to the current scheme, although
> perhaps a little more structured.
> 

Seems reasonable.