Re: [PATCH 1/3] wined3d: Prepare the destination location in surface_load_location.
On 3 November 2015 at 23:55, Stefan Dösinger <stefan(a)codeweavers.com> wrote:
Instead of its sub-functions. See comments in patch 3 for details about surface_blt_fbo.
This fixes bug 39536. surface_load_location didn't prepare the multisample renderbuffer.
The next patches will consistently move the responsibility to the caller of surface_load_location. This isn't necessarily wrong on its own, although I wonder if it's really worth it. I don't think it makes the API easier to use correctly, and I don't think there's going to be much of a performance impact.
For fixing bug 39536 this is wrong though. If I'm reading the terminal output there correctly, the issue is that load_location() is called with WINED3D_LOCATION_RB_MULTISAMPLE, which it doesn't know how to handle. This patch will hide the issue by simply always calling prepare_location(). I'd have to check if multisample surfaces are supposed to have defined content after creation and presents, but perhaps what you need to do here is to add proper WINED3D_LOCATION_DISCARDED handling for multisample surfaces, similar to depth stencil surfaces.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 Am 2015-11-04 um 12:30 schrieb Henri Verbeet:
The next patches will consistently move the responsibility to the caller of surface_load_location. This isn't necessarily wrong on its own, although I wonder if it's really worth it. I don't think it makes the API easier to use correctly, and I don't think there's going to be much of a performance impact. I'm also happy to keep the prepare call inside load_location, which would just mean dropping patch 3. Right now it's a mix of the caller being responsible (sysmem, renderbuffers) and surface_load_location being responsible (texture). I don't think that's what we want.
The argument for making the callers responsible is that some places like map and clear need to call prepare_location manually anyway.
For fixing bug 39536 this is wrong though. If I'm reading the terminal output there correctly, the issue is that load_location() is called with WINED3D_LOCATION_RB_MULTISAMPLE, which it doesn't know how to handle. This patch will hide the issue by simply always calling prepare_location(). I'd have to check if multisample surfaces are supposed to have defined content after creation and presents, but perhaps what you need to do here is to add proper WINED3D_LOCATION_DISCARDED handling for multisample surfaces, similar to depth stencil surfaces. Your reading of the logs is correct, although the surface is in sysmem, not discarded. But the renderbuffer init problem existed before 1ca9dfc8, and as you say it still exists after my patches and surface_load_location writes the same FIXME after my patches.
I could implement uploads from sysmem (going through a non-multisampled texture or using glDrawPixels), but what we probably really want is some sort of WINED3D_LOCATION_ZEROED location that will init whatever location is used first to zero (probably using GL_ARB_clear_texture where applicable), and also better WINED3D_LOCATION_DISCARD handling (presents, discard rendertargets). All those need more thought and aren't necessarily related to creating the storage, so I decided to fix the creation problem and leave the initial upload the way it was / is. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJWOfW+AAoJEN0/YqbEcdMwnx4P/i+Avm6+5Pb3aPeYsCgEdKrK ryB/nWd7mc+jbBPh5qZPUgbON15nMiYmFr3gFZSZoecrXeHVgdPGpO+7bvX7UCeG hu6glQWi69GmCdfnnB1YUZYfoolunrPVetEatgg6+HlFDA3F6DYCKHLyaWA+guN6 esmj25cTSivaGSGeAUunDoG2XtfPUvn5iKSdO7ZJ1NbKetPgC7Td3PDjBrmH1fDh NRXs4pDZZYSoIlgM4PTp9tA4VKzWnwwkd90LFSZE75hjgpCYQRHZCYmamJUj2xDe 2ea2zROLbSX0sTr5hwet6iBE59ik7dj5mRFWrFwg1DSnnFiBBHlD7SPIDVyjQPNu t5r9jGNHXM9OX1k8alXqmrlICRp1EOoeFkIu2rh3jIq0w0J934nPAwXydEU+E8jk YASIibQn7fvoU98KWzCTrf0DUrm9AHeI2D+EIoJa/akDxEseGsts8CQW/ZQ7dy2O aCdJpnb41ldYCHPTkeHohcIYaSaSL8adpyZ9tAwyI93vK+I42yjsr5wZw9lBL20j kVj8ptggfXlNdq9r6gbbn5DjuGyvNvXaeWCJAOPkC8Sxl45LoG1Hl7S6SLg8Andg fyCsp7C+qTT+X5Hbdvj1ispK80bELgim2Ae9YhSNbqd+58QqO+piYOyrM9vwxPrJ P6wng1mq47j2t3AYYa4J =IpYF -----END PGP SIGNATURE-----
On 4 November 2015 at 13:10, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
I'm also happy to keep the prepare call inside load_location, which would just mean dropping patch 3. Right now it's a mix of the caller being responsible (sysmem, renderbuffers) and surface_load_location being responsible (texture). I don't think that's what we want.
The caller is not responsible in case of renderbuffers. The general rule is that if you're going to be reading/writing from/to a location you need to call either load_location() or prepare_location(). The sysmem handling does indeed violate that in some places, those are bugs.
Your reading of the logs is correct, although the surface is in sysmem, not discarded. But the renderbuffer init problem existed Well yes, but perhaps it should be in WINED3D_LOCATION_DISCARDED.
I could implement uploads from sysmem (going through a non-multisampled texture or using glDrawPixels), but what we probably really want is some sort of WINED3D_LOCATION_ZEROED location that will init whatever location is used first to zero (probably using We'll want that at some point, but I seem to recall that the initial contents of multisample surfaces is undefined. If it isn't it should only be a couple of lines to add support for WINED3D_LOCATION_RB_MULTISAMPLE to surface_load_location().
participants (2)
-
Henri Verbeet -
Stefan Dösinger