On 22 August 2013 23:22, Stefan Dösinger stefan@codeweavers.com wrote:
@@ -145,6 +158,9 @@ static DWORD volume_access_from_location(DWORD location) case WINED3D_LOCATION_TEXTURE_RGB: return WINED3D_RESOURCE_ACCESS_GPU;
case WINED3D_LOCATION_BUFFER:return WINED3D_RESOURCE_ACCESS_CPU | WINED3D_RESOURCE_ACCESS_GPU;
Why does loading into a PBO require CPU access?
case WINED3D_LOCATION_BUFFER:if (!volume->pbo || !(volume->flags & WINED3D_VFLAG_PBO)){ERR("Trying to load WINED3D_LOCATION_BUFFER without setting it up first.\n");return;}
I think there should never be a PBO without WINED3D_VFLAG_PBO being set. There's also something to be said for leaving out the return, since that would at least in theory allow the compiler to drop the extra checks when compiled without debug messages. That probably applies to ERRs in general, since they're either never supposed to happen, or not supposed to be an ERR.
+/* Context activation is done by the caller. */ +static void wined3d_volume_prepare_pbo(struct wined3d_volume *volume, struct wined3d_context *context) +{
- const struct wined3d_gl_info *gl_info = context->gl_info;
- if (volume->pbo)
return;- GL_EXTCALL(glGenBuffersARB(1, &volume->pbo));
- checkGLcall("glGenBuffersARB(1, &volume->pbo)");
- GL_EXTCALL(glBindBufferARB(GL_PIXEL_UNPACK_BUFFER_ARB, volume->pbo));
- checkGLcall("glBindBufferARB(GL_PIXEL_UNPACK_BUFFER_ARB, volume->pbo)");
- GL_EXTCALL(glBufferDataARB(GL_PIXEL_UNPACK_BUFFER_ARB, volume->resource.size, NULL, GL_STREAM_DRAW_ARB));
- checkGLcall("glBufferDataARB(GL_PIXEL_UNPACK_BUFFER_ARB, volume->resource.size, NULL, GL_STREAM_DRAW_ARB");
- GL_EXTCALL(glBindBufferARB(GL_PIXEL_UNPACK_BUFFER_ARB, 0));
- checkGLcall("glBindBufferARB(GL_PIXEL_UNPACK_BUFFER_ARB, 0)");
- TRACE("Created PBO %u for volume %p.\n", volume->pbo, volume);
+}
I think one checkGLcall() per function is enough in general. That's even more true these days with ARB_debug_output that will usually give more specific error messages than checkGLcall() anyway. Applies to a couple of other places as well.
Am 23.08.2013 um 11:34 schrieb Henri Verbeet hverbeet@gmail.com:
Why does loading into a PBO require CPU access?
The only reason why we want to store a volume in a PBO is to map it, which requires CPU access. If the volume doesn't allow CPU access I don't see a reason to load it into a PBO.
Well, there's RGB<->sRGB loading in case GL_EXT_texture_sRGB_decode isn't supported. I am handing that with a temporary heap allocation and don't go through the tracked locations to keep the matter simple. The correct fix for this problem, if it happens, is to implement sRGB_decode in the driver.
case WINED3D_LOCATION_BUFFER:if (!volume->pbo || !(volume->flags & WINED3D_VFLAG_PBO)){ERR("Trying to load WINED3D_LOCATION_BUFFER without setting it up first.\n");return;}I think there should never be a PBO without WINED3D_VFLAG_PBO being set.
Yup, but it's an ERR path, so neither condition is supposed to ever be true. The thinking behind checking both is as a safeguard for future code changes. I'm checking resource.allocatedMemory and resource.heap_memory in the sysmem case for the same reason.
There's also something to be said for leaving out the return, since that would at least in theory allow the compiler to drop the extra checks when compiled without debug messages.
Good point.
On 23 August 2013 12:04, Stefan Dösinger stefan@codeweavers.com wrote:
Am 23.08.2013 um 11:34 schrieb Henri Verbeet hverbeet@gmail.com:
Why does loading into a PBO require CPU access?
The only reason why we want to store a volume in a PBO is to map it, which requires CPU access. If the volume doesn't allow CPU access I don't see a reason to load it into a PBO.
Yes, but that's more of a higher level policy and wouldn't hold for e.g. VBOs. You handle that by just not setting WINED3D_VFLAG_PBO for those volumes.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2013-08-23 12:29, schrieb Henri Verbeet:
On 23 August 2013 12:04, Stefan Dösinger stefan@codeweavers.com wrote:
Am 23.08.2013 um 11:34 schrieb Henri Verbeet hverbeet@gmail.com:
Why does loading into a PBO require CPU access?
The only reason why we want to store a volume in a PBO is to map it, which requires CPU access. If the volume doesn't allow CPU access I don't see a reason to load it into a PBO.
Yes, but that's more of a higher level policy and wouldn't hold for e.g. VBOs. You handle that by just not setting WINED3D_VFLAG_PBO for those volumes.
The rules for surfaces and volumes aren't applicable to buffers anyway. A default pool buffer without D3DUSAGE_DYNAMIC is mappable for example.
On 23 August 2013 13:54, Stefan Dösinger stefan@codeweavers.com wrote:
The rules for surfaces and volumes aren't applicable to buffers anyway. A default pool buffer without D3DUSAGE_DYNAMIC is mappable for example.
I don't think the differences are insurmountable, but the main point is still that I think this is the wrong level to enforce that kind of policy.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2013-08-23 14:01, schrieb Henri Verbeet:
On 23 August 2013 13:54, Stefan Dösinger stefan@codeweavers.com wrote:
The rules for surfaces and volumes aren't applicable to buffers anyway. A default pool buffer without D3DUSAGE_DYNAMIC is mappable for example.
I don't think the differences are insurmountable, but the main point is still that I think this is the wrong level to enforce that kind of policy.
Ok.
-
Henri Verbeet -
Stefan Dösinger