Re: [PATCH 1/3] wined3d: Remove poll list.

[Jan Sikorski]

Hi,
> On 23 Nov 2021, at 12:46, Henri Verbeet  wrote:
>
> On Mon, 22 Nov 2021 at 13:46, Jan Sikorski  wrote:
> > Poll with callbacks from application threads instead.
> > Signed-off-by: Jan Sikorski
> >
> > dlls/wined3d/cs.c              |  73 ++-------
> > dlls/wined3d/query.c           | 284 ++++++++++++++++++++++-----------
> > dlls/wined3d/wined3d_private.h |   6 +-
> > 3 files changed, 212 insertions(+), 151 deletions(-)
>
> I happen to know the background of this series of patches because of
> private conversations we've had, but for the benefit of both the
> commit message and the rest of the mailing list, please assume I
> don't. Why would we want to do this? What problem does this patch
> solve?

Right, I guess it could use some explanation.
> I also don't think it's ideal to lead with a patch removing the
> existing mechanism. The way we generally do these kinds of rewrites is
> to first introduce the new mechanism, then move users of the old
> mechanism over to the new mechanism, and only remove the old mechanism
> once it has become unused.

OK, I think this patch could be split into a couple more patches, so that removing the list goes after we can poll using the new way, with only a little intermediate mess.
> > -static void wined3d_cs_exec_query_issue(struct wined3d_cs *cs, const void *data)
> > +void wined3d_cs_run_priority_callback(struct wined3d_cs *cs, void (*callback)(void *object), void *object)
> > {
> > - const struct wined3d_cs_query_issue *op = data;
> > - struct wined3d_query *query = op->query;
> > - BOOL poll;
> > - struct wined3d_cs_callback *op;
> > - poll = query->query_ops->query_issue(query, op->flags);
> > - op = wined3d_device_context_require_space(&cs->c, sizeof(*op), WINED3D_CS_QUEUE_MAP);
> > - op->opcode = WINED3D_CS_OP_CALLBACK;
> > - op->callback = callback;
> > - op->object = object;
> > - if (!cs->thread)
> > -
> >    return;
> > - wined3d_device_context_submit(&cs->c, WINED3D_CS_QUEUE_MAP);
> > - wined3d_cs_finish(cs, WINED3D_CS_QUEUE_MAP);
> >
> > +}
>
> The wined3d_cs_finish() call is effectively going to make polling
> queries synchronous, at least as far as WINED3D_CS_QUEUE_MAP is
> concerned. That doesn't seem desirable.

If we can’t call directly into OpenGL on the application thread reasonably, I’m not sure how much better can this get. Just removing the wined3d_cs_finish() and getting the result asynchronously may actually make it worse, if a game knows the queries should be done by the time it is called, it will probably just busy wait for it by calling GetData() repeatedly.
That said, I don’t think it is so bad, because for occlusion queries, which are the bulk I guess, we probably use the buffer method; and other users of WINED3D_CS_QUEUE_MAP are synchronous too, so it shouldn't clog unless there’s more threads involved. It ran well with the couple games I tested, so there’s that too, but supposedly we could hit a pessimistic case where it’s worse.. Do you have a better way in mind?

Thanks,
- Jan