2010/1/4 Stefan Dösinger stefan@codeweavers.com:
- if(This->query.context) context_free_event_query(&This->query);
- This->query.context = NULL;
context_free_event_query() already clears the context field.
if(This->resource.device->adapter->gl_info.supported[APPLE_FENCE])
You already have a gl_info pointer in buffer_create_buffer_object(), you're using it a few lines up. I'm not sure you can test for APPLE_fence to determine if event queries are available.
+static inline void buffer_sync_apple(struct wined3d_buffer *This, struct wined3d_context *context, DWORD flags)
Do you have a good reason for adding that inline? I think it mostly just causes the compiler to not notice when the function is unused.
/* TODO: Test and implement D3DLOCK_DONOTWAIT */GL_EXTCALL(glFinishFenceAPPLE(This->query.id));checkGLcall("glFinishFenceAPPLE(This->query.id)");
The buffer code can't assume an event query is implemented with APPLE_fence.
GL_EXTCALL(glSetFenceAPPLE(cur->query.id));checkGLcall("glSetFenceAPPLE(cur->query.id)");
...and neither can the draw code. The assumption is currently true of course, but that's just an accident of the code order in context_alloc_event_query(). Adding e.g. ARB_sync support to event queries will break that.
Am 05.01.2010 um 11:50 schrieb Henri Verbeet:
if(This->resource.device->adapter->gl_info.supported[APPLE_FENCE])I'm not sure you can test for APPLE_fence to determine if event queries are available.
directx.c disables NV_fence if APPLE_fence is available. We prefer the APPLE extension over the NV one due to the interactions with other APPLE extensions, and I think we should prefer it over the ARB one for the same reason.
The buffer code can't assume an event query is implemented with APPLE_fence.
Since it is the preferred fence extension and the above check only accepts APPLE_fence it can assume this.
That said, I first thought that APPLE_flush_buffer_range requires APPLE_fence, but this is not the case - the extension says to use glFinish() if APPLE_fence is not supported. I assume NV_fence would work just fine too, even though this is not explicitly mentioned(NV_fence is 5 years older than APPLE_flush_buffer_range).
I can make the code more flexible and allow NV_fence or potentially ARB_sync as well, but so far I haven't seen a mac that doesn't support APPLE_fence, and I haven't seen a mac that supports NV_fence. I expect that when Apple ships support for ARB_sync, they'll also support ARB_map_buffer_range(with a nicer sync API than APPLE_). Thus the sync code without APPLE_fence would be dead code and it's better to just disable the manual fencing if APPLE_sync is not available.
2010/1/5 Stefan Dösinger stefandoesinger@gmx.at:
Am 05.01.2010 um 11:50 schrieb Henri Verbeet:
- if(This->resource.device->adapter->gl_info.supported[APPLE_FENCE])
I'm not sure you can test for APPLE_fence to determine if event queries are available.
directx.c disables NV_fence if APPLE_fence is available. We prefer the APPLE extension over the NV one due to the interactions with other APPLE extensions, and I think we should prefer it over the ARB one for the same reason.
Well, ARB_sync works across contexts...
The buffer code can't assume an event query is implemented with APPLE_fence.
Since it is the preferred fence extension and the above check only accepts APPLE_fence it can assume this.
No. Regardless of the assumption being true or not, it's an implementation detail of event queries. You're not supposed to depend on those. Of course you shouldn't be poking at event query internals from the buffer code anyway, APPLE_fence or not.
I can make the code more flexible and allow NV_fence or potentially ARB_sync as well, but so far I haven't seen a mac that doesn't support APPLE_fence, and I haven't seen a mac that supports NV_fence. I expect that when Apple ships support for ARB_sync, they'll also support ARB_map_buffer_range(with a nicer sync API than APPLE_). Thus the sync code without APPLE_fence would be dead code and it's better to just disable the manual fencing if APPLE_sync is not available.
See above. This is more of an argument for adding ARB_map_buffer_range support first though.
After the discussion on IRC I wasn't quite sure what you intended with wined3d_event_query_wait(), but maybe it was something like this:
I've based the sync code on IWineD3DQuery event queries now rather than using the GL APIs. This way the buffer code is insulated from the thread switch issues, the different fence extensions etc.
Issues: * D3D doesn't have a glFinishFence equivalent, just something like glTestFence, which needs to be polled. I guess that's where wined3d_event_query_wait comes into play
* Patch 4 changes the query behavior for apps. If the app just wonders if it can do some more work before sending more D3D commands then this is going to hurt. If the app implements some synchronization around D3DLOCK_NOOVERWRITE then we need this anyway. A separate function would help there as well, although the two functions would then have inconsistent behavior.
* Refcounting. The query holds a ref to the device, the buffer holds a ref to the query, and the device holds a ref to the buffer(if the buffer is set as stream source). However, it should still work because the d3d9 counts are unaffected, and when the app destroys the d3d9 device the stateblock refs are freed which breaks the circle. I could of course add a non-COM structure for event queries and use that, but that shouldn't be needed.
2010/1/5 Stefan Dösinger stefandoesinger@gmx.at:
After the discussion on IRC I wasn't quite sure what you intended with wined3d_event_query_wait(), but maybe it was something like this:
More something along the lines of the attached patch, actually. There are of course some intermediate steps.
-
Henri Verbeet -
Stefan Dösinger