From: "Zebediah Figura (she/her)" Subject: Re: [PATCH 3/3] wined3d: Don't initialize system memory for buffers. Message-Id: <3ab8d43d-4e90-c56f-1fa0-03d3354919a6@codeweavers.com> Date: Wed, 26 Jan 2022 10:45:39 -0600 In-Reply-To: References: <20220125022039.2005817-1-zfigura@codeweavers.com> <20220125022039.2005817-3-zfigura@codeweavers.com> On 1/26/22 07:47, Henri Verbeet wrote: > On Tue, 25 Jan 2022 at 03:35, Zebediah Figura 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.