[PATCH 3/3] wined3d: Don't initialize system memory for buffers.

[Zebediah Figura (she/her)]

On 1/26/22 07:47, Henri Verbeet wrote:
> On Tue, 25 Jan 2022 at 03:35, Zebediah Figura <zfigura at codeweavers.com> wrote:
>> This fixes test_map_synchronisation() on 64-bit machines, broken after
>> 194b47b4fd92dda8ebf24e88ca7a14fc926c84ab.
>>
> How is it broken, and why does that happen?

The test creates a buffer, maps it once, then maps it again with 
NOOVERWRITE while the GPU is still drawing, expecting the new data to be 
read by the GPU during the draw. On 32-bit machines, and 64-bit machines 
before 194b47b4fd9, we do the following:

First map: uses SYSMEM since the BO is not created yet
Draw: upload to VBO
Second map: map the existing VBO with GL_MAP_UNSYNCHRONIZED_BIT

After 194b47b4fd9, we don't use GL_MAP_UNSYNCHRONIZED_BIT since the 
buffer has READ access, which means that the second map will be 
synchronized and wait for the draw to complete.

After this patch, we do the following:

First map: create and map a VBO (not unsynchronized, but coherent and 
persistently mapped)
Draw: use mapped VBO
Second map: write to existing (coherent) VBO, which is unsynchronized

I'll make sure to include this explanation in the patch next revision.

Of course, although this motivated the patch, it seems like a good idea 
regardless. In most cases we don't want SYSMEM for buffer objects, 
especially for newer d3d versions, and we should avoid the overhead of 
allocating it and keeping it around.


It may also be that we want to translate D3DVBCAPS_WRITEONLY into 
~WINED3D_RESOURCE_ACCESS_MAP_R. My limited understanding is that this 
doesn't contradict test_vb_writeonly() per se, but rather would only be 
a bad idea performance-wise if ddraw applications rely on reading from 
such mapped buffers in critical paths. I don't know whether that's the case.

> 
>> @@ -1233,7 +1233,7 @@ static HRESULT wined3d_buffer_init(struct wined3d_buffer *buffer, struct wined3d
>>       }
>>       buffer->buffer_ops = buffer_ops;
>>       buffer->structure_byte_stride = desc->structure_byte_stride;
>> -    buffer->locations = data ? WINED3D_LOCATION_DISCARDED : WINED3D_LOCATION_SYSMEM;
>> +    buffer->locations = WINED3D_LOCATION_DISCARDED;
>>
> I think avoiding WINED3D_LOCATION_SYSMEM would be great, but I'm not
> as convinced about using WINED3D_LOCATION_DISCARDED for that purpose.
> I'm not sure we've ever confirmed that buffer resources should be
> zeroed after creation, but we have for texture resources; it would not
> surprise me if this change would break some applications.
> 
> My preferred way of handling this would be to introduce
> WINED3D_LOCATION_CLEARED, which would effectively defer clearing the
> resource until loading it into another location. When replacing the
> entire (sub-)resource with e.g. a DISCARD map or sub-resource update,
> we can then just skip that clear. This approach would also allow us to
> avoid zeroing texture resources on creation, an in case of the Vulkan
> renderer, we could even extend that a bit further and merge
> render-target clears with starting render passes by using
> VK_ATTACHMENT_LOAD_OP_CLEAR and appropriate clear values.

I had in fact had that same idea :-)

I left off using it for buffers because it wasn't clear to me that it 
was necessary. Thus far we had only found evidence that it was necessary 
for textures (and perhaps just swapchain textures?)

Unfortunately I don't currently have access to any Windows machines 
(especially older machines), so I can't verify that buffers (or 
textures, for that matter) are implicitly cleared on initialization.