Re: [2/5] WineD3D: Add an internal event query finish function
On 26 January 2010 11:51, Stefan Dösinger <stefan(a)codeweavers.com> wrote:
+HRESULT wined3d_event_query_finish(IWineD3DQuery* iface) { Please don't create function prototypes (or other code) by copy/paste. Also, why does this take a IWineD3DQuery pointer in the first place? You only really access wined3d_event_query fields.
+ /* A glFinish does not reliably wait for draws in other contexts. The caller has + * to find its own way to cope with the thread switch + */ ... + /* Since this is an internal call the caller should know that event Query support is faked */ + FIXME("(%p): Event query not supported, calling glFinish\n", This); + wglFinish(); + return WINED3D_OK; + } This is inconsistent. Also, note that event queries are pretty generic things, you could use them for something else than draws as well.
+ if (!gl_info->supported[APPLE_FENCE] && !gl_info->supported[NV_FENCE] && gl_info->supported[ARB_SYNC]) This doesn't make sense. Of course if you just used wined3d_event_query instead of IWineD3DQuery you wouldn't even need this at all, you could just let context_alloc_event_query() fail.
Just for reference, glFinish() behaviour is a bit more subtle than you make it sound. It flushes GL commands for the current context to the GPU, and then waits for the GPU to finish execution up to that point. It won't flush command buffers for other contexts, but anything that's already been submitted to the GPU before the glFinish() will have been executed when the glFinish() returns. I didn't look at the other patches much, but I think you want to look them over a bit before resending as well.
Am 26.01.2010 um 12:44 schrieb Henri Verbeet:
On 26 January 2010 11:51, Stefan Dösinger <stefan(a)codeweavers.com> wrote:
+HRESULT wined3d_event_query_finish(IWineD3DQuery* iface) { Please don't create function prototypes (or other code) by copy/paste. Also, why does this take a IWineD3DQuery pointer in the first place? You only really access wined3d_event_query fields. To avoid that the buffer code has to look inside IWineD3DQueryImpl * See below about creating a wined3d_event_query directly.
+ /* A glFinish does not reliably wait for draws in other contexts. The caller has + * to find its own way to cope with the thread switch + */ ... + /* Since this is an internal call the caller should know that event Query support is faked */ + FIXME("(%p): Event query not supported, calling glFinish\n", This); + wglFinish(); + return WINED3D_OK; + } This is inconsistent. Ok
+ if (!gl_info->supported[APPLE_FENCE] && !gl_info->supported[NV_FENCE] && gl_info->supported[ARB_SYNC]) This doesn't make sense. Of course if you just used wined3d_event_query instead of IWineD3DQuery you wouldn't even need this at all, you could just let context_alloc_event_query() fail. I see there's a ! missing before the ARB_SYNC, but I guess your point is the entire line.
If I let the buffer code create a wined3d_event_query then I'll have to create internal functions like this for _Issue and _GetData as well, and the real IWineD3DEventQueryImpl_Issue and IWineD3DEventQueryImpl_GetData would just be simple wrappers. I think that doesn't make sense since IWineD3DEventQuery is a wine(not wined3d) internal API as well. The event query implementation would offer two interfaces that provide the same functionality. If you're only concerned about checking for faked event queries we could move the event and occlusion query faking into d3d9 and let WineD3D just fail normal query creation if there is no GL support. D3d9 is the only client library that needs faked event and occlusion query support. D3d9 has enough information to return dummy data on its own if wined3d doesn't provide the real queries. Do you have other concerns that would require a wined3d internal and wined3d external event query interface?
On 26 January 2010 13:42, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote:
If I let the buffer code create a wined3d_event_query then I'll have to create internal functions like this for _Issue and _GetData as well, and the real IWineD3DEventQueryImpl_Issue and IWineD3DEventQueryImpl_GetData would just be simple wrappers. I think that doesn't make sense since IWineD3DEventQuery is a wine(not wined3d) internal API as well. The event query implementation would offer two interfaces that provide the same functionality.
If you're only concerned about checking for faked event queries we could move the event and occlusion query faking into d3d9 and let WineD3D just fail normal query creation if there is no GL support. D3d9 is the only client library that needs faked event and occlusion query support. D3d9 has enough information to return dummy data on its own if wined3d doesn't provide the real queries.
Do you have other concerns that would require a wined3d internal and wined3d external event query interface?
Not for this patch. In the long term I'm not sure if we want to stick with the current COM-style public wined3d interface, but that's mostly a separate consideration. As for this actual patch / series, I'm sure there are plenty of different ways you could implement this, but it seems to me that using IWineD3DQuery is one of the impractical ones. Using wined3d_event_query is just a suggestion, if you can make things work correctly in some other way that's of course fine as well.
Am 26.01.2010 um 14:26 schrieb Henri Verbeet:
Using wined3d_event_query is just a suggestion, if you can make things work correctly in some other way that's of course fine as well. This is a bit vague, but here is an updated set of patches that includes your suggestions:
Patch 2 moves the event query faking out of the way. That allows me to remove all GL extension checks, and the buffer code just checks if it can create a d3d event query. (Patch 1 removes some dead code I ran into). Once Mesa supports ARB_sync we can kill this code entirely and tell people to update their drivers. The only deeper change in patch 3 is that I am passing GL_SYNC_FLUSH_COMMANDS_BIT. I think this is reasonable when we're waiting for drawing to complete.
+HRESULT wined3d_event_query_finish(IWineD3DQuery* iface) { Please don't create function prototypes (or other code) by copy/paste. Mind elaborating? If you're talking about the placing of the {, I changed this. I think we should at some point change the style of the remaining files(or at least low hanging fruits) to get rid of the existing formatting vs new wined3d style issues. I'm getting tired of those formatting problems.
The other patches aren't really changed. Patch 5 uses CreateQuery to find out if event queries are supported, which brings a few GL locking changes to avoid calling CreateQuery with the lock held.
On 26 January 2010 20:22, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote:
Patch 2 moves the event query faking out of the way. That allows me to remove all GL extension checks, and the buffer code just checks if it can create a d3d event query. (Patch 1 removes some dead code I ran into). Once Mesa supports ARB_sync we can kill this code entirely and tell people to update their drivers.
I'm not sure if it's an improvement over the previous patches or not.
The only deeper change in patch 3 is that I am passing GL_SYNC_FLUSH_COMMANDS_BIT. I think this is reasonable when we're waiting for drawing to complete.
You shouldn't need that. It only flushes for the current context anyway.
+HRESULT wined3d_event_query_finish(IWineD3DQuery* iface) { Please don't create function prototypes (or other code) by copy/paste. Mind elaborating? If you're talking about the placing of the {, I changed this. I think we should at some point change the style of the remaining files(or at least low hanging fruits) to get rid of the existing formatting vs new wined3d style issues. I'm getting tired of those formatting problems.
You clearly copied this from some other prototype in query.c, the placement of the brace and the * for example are inconsistent with the rest of the code. The same goes for e.g. some of the debug prints that are consistent with the places they were copied from, instead of with each other. Although those would be minor style issues that could be fixed, I think the deeper issue is that it is a bad way to write code.
The other patches aren't really changed. Patch 5 uses CreateQuery to find out if event queries are supported, which brings a few GL locking changes to avoid calling CreateQuery with the lock held.
It also points out another problem with using IWineD3DQuery, you don't have a real parent object to pass to CreateQuery(). Queries probably don't really need a parent, but it does highlight that the interface was never meant for wined3d internal use. It's also the kind of thing you should be able to spot yourself when you write something like "IWineD3DDevice_CreateQuery(iface, WINED3DQUERYTYPE_EVENT, &cur->query, NULL);".
+/* The caller provides a context and GL locking and binds the buffer */ +static void buffer_sync_apple(struct wined3d_buffer *This, DWORD flags) +{ ... + LEAVE_GL(); + hr = wined3d_event_query_finish(This->query); + ENTER_GL(); ... + LEAVE_GL(); + IWineD3DQuery_Release(This->query); + ENTER_GL(); ... +} Don't do that. Patch 6 adds another one of those.
Am 27.01.2010 um 11:19 schrieb Henri Verbeet:
The only deeper change in patch 3 is that I am passing GL_SYNC_FLUSH_COMMANDS_BIT. I think this is reasonable when we're waiting for drawing to complete.
You shouldn't need that. It only flushes for the current context anyway. From section 5.2.2 in the extension: If the sync object being blocked upon will not be signaled in finite time (for example, by an associated fence command issued previously, but not yet flushed to the graphics pipeline), then ClientWaitSync may hang forever. I don't know if there is any driver that waits forever for more commands before flushing at some point, but I noticed that with single buffering and no glFlush or glFinish fglrx keeps queuing commands until it runs out of memory. So I think we need it.
For multiple contexts we need a glFlush after draws, you're working on that for different reasons anyway. Related, but subject for a different patch: We don't handle D3DGETDATA_FLUSH in either event or occlusion queries. Afaics this only matters for NV_fence and ARB_sync since APPLE_fence and occlusion queries already take care of that on the GL side.
It also points out another problem with using IWineD3DQuery, you don't have a real parent object to pass to CreateQuery(). Queries probably don't really need a parent, but it does highlight that the interface was never meant for wined3d internal use. It's also the kind of thing you should be able to spot yourself when you write something like "IWineD3DDevice_CreateQuery(iface, WINED3DQUERYTYPE_EVENT, &cur->query, NULL);". I did of course notice that but didn't consider it much of an issue since the Query parent is entirely unused, except for Query_GetParent, which is never called. The parents cause issues whenever there is a mismatch between the client side object structure and the wined3d structure. E.g. ddraw has to create dummy parent objects because there's no swapchain or texture object(yeah, I know I run into hot water whenever I bring up ddraw).
I'd say we should get rid of the parent objects entirely and client libs can use SetPrivateData/GetPrivateData to find their interfaces in places like GetRenderTarget, GetTexture, etc. That way we don't make any assumptions about client library objects. That's something for a different patch though - I am happy with the NULL for now, or start by removing the query parent.
+/* The caller provides a context and GL locking and binds the buffer */ +static void buffer_sync_apple(struct wined3d_buffer *This, DWORD flags) +{ ... + LEAVE_GL(); + hr = wined3d_event_query_finish(This->query); + ENTER_GL(); ... + LEAVE_GL(); + IWineD3DQuery_Release(This->query); + ENTER_GL(); ... +} Don't do that. Patch 6 adds another one of those. I disliked the idea of having a LEAVE_G(); buffer_sync(); ENTER_GL() in the caller, when the two most common cases(NOOVERWRITE and DISCARD) don't need them, but ok.
On 27 January 2010 13:07, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote:
Am 27.01.2010 um 11:19 schrieb Henri Verbeet:
You shouldn't need that. It only flushes for the current context anyway. From section 5.2.2 in the extension: If the sync object being blocked upon will not be signaled in finite time (for example, by an associated fence command issued previously, but not yet flushed to the graphics pipeline), then ClientWaitSync may hang forever. I don't know if there is any driver that waits forever for more commands before flushing at some point, but I noticed that with single buffering and no glFlush or glFinish fglrx keeps queuing commands until it runs out of memory. So I think we need it.
You certainly need to flush, but that's not the point. Using GL_SYNC_FLUSH_COMMANDS is either unnecessary if you already flush after glFenceSync(), or doesn't solve the problem for fences in different contexts if you don't. Note that the existing code doesn't have to flush after glFenceSync() because we use a 0 timeout in GetData().
I'd say we should get rid of the parent objects entirely and client libs can use SetPrivateData/GetPrivateData to find their interfaces in places like GetRenderTarget, GetTexture, etc. That way we don't make any assumptions about client library objects.
There would be some problems with such a scheme, but it's really not relevant to this series. I still think you're trying way too hard to fit IWineD3DQuery into a role for which it was never meant, for no good reason.
+/* The caller provides a context and GL locking and binds the buffer */ +static void buffer_sync_apple(struct wined3d_buffer *This, DWORD flags) +{ ... + LEAVE_GL(); + hr = wined3d_event_query_finish(This->query); + ENTER_GL(); ... + LEAVE_GL(); + IWineD3DQuery_Release(This->query); + ENTER_GL(); ... +} Don't do that. Patch 6 adds another one of those. I disliked the idea of having a LEAVE_G(); buffer_sync(); ENTER_GL() in the caller, when the two most common cases(NOOVERWRITE and DISCARD) don't need them, but ok.
You shouldn't just move the problem to the caller either, of course.
Okay, here's another proposal, this time based on a new wined3d_event_query interface. There are two event query interfaces now, wined3d_event_query and IWineD3DEventQuery. The former does pretty much all the work, the latter is just a COM wrapper. The only functionality added by IWineD3DEventQuery is the event query faking if we have no GL extension available(It is handled in wined3d_event_query temporarily to aid the transition. Patch 3 moves it into its final place). A difference to your original proposal is that wined3d_event_query always takes care of activating the context. It will also take care of remembering which queries are finished to improve some multithreading cases and avoid issues if the driver doesn't like a query to be finished twice. On the buffer side things are essentially unchanged, except that I removed the check if queries are supported. As long as the app never requests synchronization we can do just fine without event query support, and if the app wants synchronization we can fall back later anyway. Concerning GL_SYNC_FLUSH_COMMANDS:
You certainly need to flush, but that's not the point. Using GL_SYNC_FLUSH_COMMANDS is either unnecessary if you already flush after glFenceSync(), or doesn't solve the problem for fences in different contexts if you don't. Note that the existing code doesn't have to flush after glFenceSync() because we use a 0 timeout in GetData(). The existing code has to honor D3DGETDATA_FLUSH if the app passes it. The 0 timeout doesn't help us if the app does something like this
BOOL data; do { IWineD3DQuery_GetData(event_query, D3DGETDATA_FLUSH, &data); } while(!data); in a single threaded environment.
On 16 February 2010 14:59, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote:
Patch 1:
if (!pData || !dwSize) return S_OK;
...
+ if (data) This should be unnecessary, since you already checked "pData" above. Otherwise the patch looks reasonable.
Patch 4:
+ GLenum gl_ret = GL_EXTCALL(glClientWaitSync(query->object.sync, GL_SYNC_FLUSH_COMMANDS_BIT, ~0U)); ... + /* We don't expect a timeout for a ~584 year wait */ Actually, ~0U would be slightly more than 4 seconds.
Patch 7:
+ for(i = 0; i < This->num_buffer_queries; i++) + { + wined3d_event_query_issue(This->buffer_queries[i], This); + } You should do this before the flush, otherwise the query might never finish if it's in a different context.
Concerning GL_SYNC_FLUSH_COMMANDS:
You certainly need to flush, but that's not the point. Using GL_SYNC_FLUSH_COMMANDS is either unnecessary if you already flush after glFenceSync(), or doesn't solve the problem for fences in different contexts if you don't. Note that the existing code doesn't have to flush after glFenceSync() because we use a 0 timeout in GetData(). The existing code has to honor D3DGETDATA_FLUSH if the app passes it. The 0 timeout doesn't help us if the app does something like this
BOOL data; do { IWineD3DQuery_GetData(event_query, D3DGETDATA_FLUSH, &data); } while(!data);
in a single threaded environment.
Sure, but if you want queries to work reliably across contexts you need to flush after issuing them.
participants (2)
-
Henri Verbeet -
Stefan Dösinger