I'm not sure about this, why do we want this?
Am 27.11.2013 um 17:05 schrieb Henri Verbeet hverbeet@gmail.com:
I'm not sure about this, why do we want this?
To upload converted formats directly to the destination in update_texture without blitting to the unlockable default pool destination, something along the lines of the attached diff. Patch 4 interacts with this code move, this is why I’ve moved this patch ahead a bit.
On 27 November 2013 17:16, Stefan Dösinger stefandoesinger@gmail.com wrote:
Am 27.11.2013 um 17:05 schrieb Henri Verbeet hverbeet@gmail.com:
I'm not sure about this, why do we want this?
To upload converted formats directly to the destination in update_texture without blitting to the unlockable default pool destination, something along the lines of the attached diff. Patch 4 interacts with this code move, this is why I’ve moved this patch ahead a bit.
I think we almost always want to do format conversion in the blitter.
Am 27.11.2013 um 17:29 schrieb Henri Verbeet hverbeet@gmail.com:
I think we almost always want to do format conversion in the blitter.
How do you intend to handle the sysmem access restrictions? I don’t think we want to load a non-dynamic DEFAULT pool surface or volume into LOCATION_SYSMEM or LOCATION_BUFFER.
On 27 November 2013 17:37, Stefan Dösinger stefandoesinger@gmail.com wrote:
Am 27.11.2013 um 17:29 schrieb Henri Verbeet hverbeet@gmail.com:
I think we almost always want to do format conversion in the blitter.
How do you intend to handle the sysmem access restrictions? I don’t think we want to load a non-dynamic DEFAULT pool surface or volume into LOCATION_SYSMEM or LOCATION_BUFFER.
Staging resources. If we're going to enforce access restrictions we're going to need those anyway for blits that only the CPU blitter can do.
Am 27.11.2013 um 17:46 schrieb Henri Verbeet hverbeet@gmail.com:
Staging resources. If we're going to enforce access restrictions we're going to need those anyway for blits that only the CPU blitter can do.
I don’t see how that helps here. This isn’t about a WINED3DFMT_R8G8B8A8_UNORM to WINED3DFMT_R32G32B32A32_FLOAT blit, this about uploading formats that have a conversion function to handle missing GL support (e.g. WINED3DFMT_R16G16_FLOAT without TEXTURE_RG or WINED3DFMT_R16G16_SNORM). Unless we add some WINED3DFMT enum values to express those conversions I don’t see how a staging resource helps here.
I don’t think there are any d3d8/9 blit operations that our GPU blitters cannot handle. There’s one exception - Partial CopyRects copies from a render target to a sysmem surface. With a small modification to the NP2 repacking code surface_download_data can handle that. A staging resource is a viable alternative here, but this is unrelated to this patch. Ddraw blits don’t matter because all ddraw surfaces are mappable.
The conv-update.diff patch has one potential problem: An application could create a R16G16_SNORM surface in D3DPOOL_DEFAULT with D3DUSAGE_DYNAMIC, update it with update_surface and then map it. If that ever happens we can easily handle this with something like
if(converted && destination->resource.valid_access & ACCESS_CPU) wined3d_surface_blt(destination, source, ...); else { surface_get_data(source, &data); surface_upload_from_surface(destination, data); }
But until we find an application with such an odd behavior I’m fine with the existing FIXME.
On 27 November 2013 18:08, Stefan Dösinger stefandoesinger@gmail.com wrote:
I don’t see how that helps here. This isn’t about a WINED3DFMT_R8G8B8A8_UNORM to WINED3DFMT_R32G32B32A32_FLOAT blit, this about uploading formats that have a conversion function to handle missing GL support (e.g. WINED3DFMT_R16G16_FLOAT without TEXTURE_RG or WINED3DFMT_R16G16_SNORM). Unless we add some WINED3DFMT enum values to express those conversions I don’t see how a staging resource helps here.
Doing conversions in the blitter allows them to be done on either the GPU or the CPU, depending on what's appropriate. I also think it's just the right place conceptually. It's correct that this would require some work on the blitter to be able to correctly express some of the conversions.
I don’t think there are any d3d8/9 blit operations that our GPU blitters cannot handle.
But not all blitters are necessarily always available. E.g. d3d8 level hardware generally doesn't have useful fragment shaders.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2013-11-27 18:56, schrieb Henri Verbeet:
Doing conversions in the blitter allows them to be done on either the GPU or the CPU, depending on what's appropriate.
I don't think it is worth the trouble. Since we introduced the SNORM and RG conversion code it has never been a performance problem. I haven't seen any application that streams SNORM or RG data. The return of investment would be nonexistent from a user point of view.
While optionally doing the conversion on the GPU sounds nice on paper, it means adding another codepath where one of them won't get proper testing, no matter how well it is integrated.
I also think it's just the right place conceptually. It's correct that this would require some work on the blitter to be able to correctly express some of the conversions.
I disagree, for a number of reasons:
The GL format conversion has to work on volumes as well. Do we want to draw them into the blitter?
The similarities between the GL format conversion and the surface->surface blitter are limited. Both convert formats, but the former doesn't have to care about sizes, stretching etc. The amount of conversion logic that can be shared is limited. Maybe the RG->RGB expansion code can be shared, if d3d9::StretchRect allows a RG_FLOAT->RGBA_FLOAT blit. But the algorithm that does that is really simple.
surface_blt is a god call already by Microsoft's design. I don't think we should add more to it.
On the other hand, the patch we are discussing isolates the GL format conversion in surface_upload_data and volume_upload_data. The conv-update.diff patch frees the location management from conversion constraints. I believe those changes are reasonable improvements.
-
Henri Verbeet -
Stefan Dösinger