Re: [PATCH 3/5] wined3d: Temporarily remove surface PBO support.
On 24 November 2013 00:18, Stefan Dösinger <stefan(a)codeweavers.com> wrote:
The implementation is strange and fairly ineffective in doing what PBOs are supposed to do - avoiding pipeline stalls and redundant copying. Removing it will make restructuring surfaces easier and avoid temporary regressions. It will be re-added later. Is there a good reason this can't just be fixed first?
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 2013-11-24 11:30, schrieb Henri Verbeet:
On 24 November 2013 00:18, Stefan Dösinger <stefan(a)codeweavers.com> wrote:
The implementation is strange and fairly ineffective in doing what PBOs are supposed to do - avoiding pipeline stalls and redundant copying. Removing it will make restructuring surfaces easier and avoid temporary regressions. It will be re-added later. Is there a good reason this can't just be fixed first? It's certainly possible, but it comes with a greater risk of regressions and I estimate that it will require a week of duplicated work. The current surface location management will go away and I prefer to migrate to the volume-style location management at the lowest possible complexity instead of spending time fixing the old code first.
-----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJSkoF4AAoJEN0/YqbEcdMwiFYP/AvCne+hWU/yX47hce7o97bu 2EhgbVN+hd8qVLyb7ovZ/VULjfS2f+9TVsJMdatdP3RFSuWQbsheeioLPGlQuv+8 L7pKZ5PJ48to2Eq6EeTQx4hUrZRYCYOOUMocM1iMdCnGEXBBSzg0VOieyJmiVzqu HAH9YY+h0HvtJ2nR3ncDRDXB/CSV15a9xTY8bjunw/4jgvCUTyBZR7kjo3PzbnCe YUUfwl3DVRWDuXoFeeWrqw314rGkEnKW5ArOYM7iOlCMNS1UoLDsTHYL1RTkjS+F lHv2NqEaaMCu6UzZGaS0wY+clzP681K7qbMeVbnTZcNBKSUl1ZeePUwSzFKHlpK3 0YPV9tfyRQ85Vx8/HuV4sXZMVWKvgeNr+mn1X5m/SRLc22PZ4RzfMk8rYfUMOhB3 1UU/J759e011ZYXINMAhFpWG8nRPZc9uZERP8eF35cuuuHQ9gbGVqeSJVzRpnvql fMwUVrCX0lj1AZpSxc+SebPR6h/Kg5xFwp03gDsw+iqOPbyEzoWUW3ADrm56lOaR aAV8bEkEdNnGywdaDV1xX/5Q/Dn0kyTW81bjQBdHPWJoS8GD00RCIb05PK0EgukO lkbJ4LM6F7Wc3RUu6Sb3+E28pq59ya01gbIC3fZMuquhfAnZx5sbxHuk5+nLVD+F iQl1WawDTWoFQ8RO7f4I =Zg4P -----END PGP SIGNATURE-----
On 24 November 2013 23:45, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
Am 2013-11-24 11:30, schrieb Henri Verbeet:
Is there a good reason this can't just be fixed first? It's certainly possible, but it comes with a greater risk of regressions and I estimate that it will require a week of duplicated That shouldn't be true, I think.
work. The current surface location management will go away and I prefer to migrate to the volume-style location management at the lowest possible complexity instead of spending time fixing the old code first.
I'm not quite sure what you mean here, the volume and surface resource location management functions should be pretty much the same. The exceptions to that are of course things like PBOs and perhaps DCs, but fixing that is pretty much what this is about. I think it's also worth pointing out that the way the volume code uses PBOs isn't a whole lot better than what the surface code does.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 2013-11-25 12:02, schrieb Henri Verbeet:
On 24 November 2013 23:45, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
Am 2013-11-24 11:30, schrieb Henri Verbeet:
Is there a good reason this can't just be fixed first? It's certainly possible, but it comes with a greater risk of regressions and I estimate that it will require a week of duplicated That shouldn't be true, I think. I'll give it another try and see where it leads me.
I've attached the current state of the surface patches for reference. Patches 2 and 26 are simpler without PBOs, and I'll have to duplicate patches 27-29, 33, 41 and 50 in surface.c and then delete or move the code in separate patches. I'd appreciate it if you could take a look at the entire patchset and see if there's anything else you don't like conceptually or if there's another major reordering you want.
I think it's also worth pointing out that the way the volume code uses PBOs isn't a whole lot better than what the surface code does. How would you prefer PBOs to work?
My patchset replaces the xFLAG_PBO flags with a map_binding field and adds resource_get_memory similar to buffer_get_memory. This will also take care of user memory and dib sections. Eventually we want to use PBOs for sysmem surfaces and volumes so the driver doesn't have to create an extra data copy in UpdateTexture / UpdateSurface. This requires ARB_buffer_storage, and I don't think it makes sense to implement this before buffer, surface and volume location and GL BO handling is merged. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJSk3RXAAoJEN0/YqbEcdMwbR0P/2627l6lJFpCVzxRaeOl9GkR d9SERwsYHPYjAEkos6YIkiujRmTi7KakIFkUtzP5rZoB9lhguJUDXpFZw2tBhKVY An4LB/cQDRML2i2F0DdCvfdvgC5kH2206KWTst9icOE8ovqNhbUbBYDszsiG0GIA ft/VjfI7R3+0QvbwsPL4PcgKaI/G5GBzrMzUU7MgdMHB9zcVynakPS7AxY+H2XCQ 8lG2f1AmJ50PLF4UUw2xXgUluF9yzTyqijnvwmsrRaDurL6Bly7DG5IZj83A48J7 9qVJq53LUIeATg8ZBJPKqJy0bs8wfyhXpGMRAmgTEgqaxfJc5eZvI8ppSqZUUWHL PriHNmaBm5hEyGbIvOdRjHhzXET9eIGKQBnTm04EEkRSAneLtmTtuSE75tQgEQiN RSPjvIbYmuXBda8SVn4ZyqFyusUvbSo5HZoFlEgDwyl54tKPcF/IICklcOk/YA/+ 7OAYXmgfkkIZLTOo2IobpWKu1PTU2yIb9ivq4RZp8dWhsemje4v+vseZjEBrIZaG zQHyGd3S3ii/fssQRu+vHNrMQKOtbR9Au9rKeBVSPoU9kuHA0uwoJQPFLu4dw51z zvXKRkwMTBXf5OeLRlw4/YHoRsTzIlKsaZ1tElUugx0gfFIC+TrJe17BeLVgFWnt ex/VAj17impWVBtgmQTJ =XN+B -----END PGP SIGNATURE-----
On 25 November 2013 17:01, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
I've attached the current state of the surface patches for reference. Patches 2 and 26 are simpler without PBOs, and I'll have to duplicate patches 27-29, 33, 41 and 50 in surface.c and then delete or move the code in separate patches.
I'd appreciate it if you could take a look at the entire patchset and see if there's anything else you don't like conceptually or if there's another major reordering you want.
Ok, but it may take a while.
I think it's also worth pointing out that the way the volume code uses PBOs isn't a whole lot better than what the surface code does. How would you prefer PBOs to work?
I haven't quite decided yet. One consideration is that if we want the usage hint to glBufferDataARB() to make sense, we can't necessarily use the same PBO both for downloading and uploading data. Another is essentially the various variants of d3d10's UpdateSubResource(), and d3d9's GetRenderTargetData() / GetFrontBufferData(). We'd obviously like to avoid stalling on those, but it's not clear to me if that's always possible. For example, always creating PBO's for sysmem surfaces and then uploading from there probably isn't going to work very well if the application just creates a single sysmem surface and uses that for updating different default pool surfaces with UpdateSurface(), unless perhaps it also uses DISCARD on the sysmem surface. Perhaps that's ok, but I don't think we really know at this point. At some point I considered a scheme where the device has a pool of PBOs and the resource update function grab one from there when needed. That probably has its own problems, but I haven't thought it all the way through either.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 2013-11-25 18:21, schrieb Henri Verbeet:
I haven't quite decided yet. One consideration is that if we want the usage hint to glBufferDataARB() to make sense, we can't necessarily use the same PBO both for downloading and uploading data. The usage hint is gone from GL_ARB_buffer_storage. Do you really think it matters? Do we have any evidence that suggests that using one PBO to download and upload data causes problems? Even if it does cause problems we should check if it causes more problems than using one surface to download and upload in d3d9.
You know the AMD GPU and r600g driver pretty well. Is there anything in the hw or driver that suggests that the usage really matters and/or that using a PBO for PACK_BUFFER and UNPACK_BUFFER causes problems?
Another is essentially the various variants of d3d10's UpdateSubResource(), and d3d9's GetRenderTargetData() / GetFrontBufferData(). We'd obviously like to avoid stalling on those, but it's not clear to me if that's always possible. For example, always creating PBO's for sysmem surfaces and then uploading from there probably isn't going to work very well if the application just creates a single sysmem surface and uses that for updating different default pool surfaces with UpdateSurface(), unless perhaps it also uses DISCARD on the sysmem surface. Perhaps that's ok, but I don't think we really know at this point. Most applications that use UpdateSurface / UpdateTexture that I've worked with in the command Stream development use DISCARD on the sysmem surface. I'd expect the application to stall on native d3d as well if it doesn't use DISCARD, but it is difficult to test.
The exception here are intro videos. Apps do all sorts of stupid things, including just using a D3DPOOL_MANAGED texture and calling it a day. It doesn't really matter because pure video playback doesn't queue up lots of commands anyway. The one thing that matters performance-wise is not stalling the pipeline on map. I have not seen a single application that uses D3DPOOL_DEFAULT D3DUSAGE_DYNAMIC textures with DISCARD, except for one case of intro video playback. Not sure which game this was, I'd have to re-check them all. Even the applications that use UpdateSurface / UpdateTexture in real rendering don't stream a lot of data compared to the use of dynamic buffers, usually it's a texture update every other frame. According to my testing, the only applications that profit from PBOs(in their current state, and in my cs tree) are Warcraft 3 and UT2004. They abuse backbuffer->map(READONLY) as a glFinish replacement, and loading the data into a PBO instead of sysmem gives them a ~10% performance boost.
At some point I considered a scheme where the device has a pool of PBOs and the resource update function grab one from there when needed. That probably has its own problems, but I haven't thought it all the way through either. I plan to add a BO pool to the command stream patches to handle DISCARD, but I think that's only partially related to what you've considered.
-----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJSk5WsAAoJEN0/YqbEcdMwr7AP+QGOK7WWNbe07foHaa1G4GPq 2EWpJUnLbxVySZISO70gIrbrE8LPlJf5gQpi1b8UdyUrYTIkp9AFiSpcQYKTtwLE RO4SmwDA3h2OfSbAdjFiVFeZQ1C2oBBdxq1sGx/s9cZcgtXZ/dcaOkyxeEAqkJT2 KhPt4Aaoibv4i3kUXhhBo7x87Rl4qqFRPxe8XLFvkTXJ2OpibyQEV9pYkxmvSkjY BrtV1b3Nt3WXRoSfKsgqWUBx+QlxRVNhO3BaTq6tXvdVq5sF+Mr9gS5hIeyPnkC1 H2UQv79bYybGVlupcCvZV6mxaEVig+krlIGxTyge90wPWuxqwNWJzyl03Jg2dqpk /5uIZyuj877yciNnOY8NhU/5vBIFESZrNH4zWcV6n1TB6Jc2pkGlfw20uj8Zs5Ei BkRMD3EuLhoTL6zG6k+Etmzwrex2qielEM7GNBGlbxxeLEJ+otCQkTTCeItTk62w yRtoIWZTp76a0N82XHKXHVl2C99Wxxr4N5lfu7XJeqJ6SPPxi/XRz0Q9o/l1kPQC 98Buc1FYQtR19go3ogETlKCIp7hB6FXzzIgebdPcoG0YBV+11dIiCzT1gAu5r9BO 3mE53y3NiEl5TAzeuEdPZOiPtKBVTj3QmV6texOTE/hU/lJyKal3FqqZTHXT0QCD TKlJhfAzO1tGEptUf9GQ =G+/U -----END PGP SIGNATURE-----
On 25 November 2013 19:23, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
You know the AMD GPU and r600g driver pretty well. Is there anything in the hw or driver that suggests that the usage really matters and/or that using a PBO for PACK_BUFFER and UNPACK_BUFFER causes problems?
The Mesa Gallium drivers just care about the STATIC/DYNAMIC/DRAW part of the usage hint, and not so much about READ/DRAW/COPY. I can't say if that will necessarily always stay that way though. I know the proprietary NVIDIA driver does at least complain through ARB_debug_output if the detected usage doesn't match the usage hint.
On 25 November 2013 17:01, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
I've attached the current state of the surface patches for reference. Patches 2 and 26 are simpler without PBOs, and I'll have to duplicate patches 27-29, 33, 41 and 50 in surface.c and then delete or move the code in separate patches.
I'd appreciate it if you could take a look at the entire patchset and see if there's anything else you don't like conceptually or if there's another major reordering you want.
I reviewed about half of this, and only very roughly: - In patch 05/72 you remove DIB maps. Similar to this patch, I'd prefer fixing things in an incremental way, instead of essentially rewriting the entire thing. - In patch 14/72 you introduce wined3d_resource_invalidate_location(). It's not clear to me that this makes sense for container resources like textures in that form. The way I was going to handle that was to create a distinction between resources (textures, buffers) and subresources (surfaces, volumes, buffers). - I don't like patch 15/72 at all. I think this should pretty much just be a matter of doing "wined3d_resource_invalidate_location(subresource->container);" in the subresource. - The context restoration in patch 18-20/72 is pretty nasty. We should probably introduce a variant of context_release() that takes the wined3d context to restore as a second parameter instead. As a general theme, I think the better approach is to first unify how things work between e.g. surfaces and volumes, and then merge them on the resource/subresource level, as opposed to first adding duplicate functionality to resources that only works for specific resource types.
participants (2)
-
Henri Verbeet -
Stefan Dösinger