Re: [PATCH 2/5] wined3d: Handle CS op emission from CS op handlers.
On 17.02.2017 14:08, Henri Verbeet wrote:
Signed-off-by: Henri Verbeet <hverbeet(a)codeweavers.com> --- dlls/wined3d/cs.c | 37 +++++++++++++++++++++++++++++-------- dlls/wined3d/wined3d_private.h | 2 +- 2 files changed, 30 insertions(+), 9 deletions(-)
diff --git a/dlls/wined3d/cs.c b/dlls/wined3d/cs.c index 55706cd..10905c0 100644 --- a/dlls/wined3d/cs.c +++ b/dlls/wined3d/cs.c @@ -1576,26 +1576,47 @@ static void (* const wined3d_cs_op_handlers[])(struct wined3d_cs *cs, const void
static void *wined3d_cs_st_require_space(struct wined3d_cs *cs, size_t size) { - if (size > cs->data_size) + if (size > (cs->data_size - cs->end)) { + size_t new_size; void *new_data;
- size = max( size, cs->data_size * 2 ); - if (!(new_data = HeapReAlloc(GetProcessHeap(), 0, cs->data, size))) + new_size = max(size, cs->data_size * 2); + if (!cs->end) + new_data = HeapReAlloc(GetProcessHeap(), 0, cs->data, new_size); + else + new_data = HeapAlloc(GetProcessHeap(), 0, new_size); + if (!new_data) return NULL;
- cs->data_size = size; + cs->data_size = new_size; + cs->start = cs->end = 0; cs->data = new_data; }
- return cs->data; + cs->end += size; + + return (BYTE *)cs->data + cs->start; }
static void wined3d_cs_st_submit(struct wined3d_cs *cs) { - enum wined3d_cs_op opcode = *(const enum wined3d_cs_op *)cs->data; - - wined3d_cs_op_handlers[opcode](cs, cs->data); + enum wined3d_cs_op opcode; + size_t start; + BYTE *data; + + data = cs->data; + start = cs->start; + cs->start = cs->end; + opcode = *(const enum wined3d_cs_op *)&data[start]; + wined3d_cs_op_handlers[opcode](cs, &data[start]); + if (!start) + { + if (cs->data != data) + HeapFree(GetProcessHeap(), 0, data); + else + cs->start = cs->end = 0; + }
Is it intentional that you are not updating cs->start and cs->end in any other situation? This might lead to unnecessary memory allocations because start / end is not set back before the block is fully empty. Here an example what happens if wined3d_device_delete_opengl_contexts_cs is called with lots of resources (with this patch http://ix.io/dP5): fixme:d3d:wined3d_device_delete_opengl_contexts_cs -> unloading resources fixme:d3d:wined3d_cs_st_require_space start = 12 fixme:d3d:wined3d_cs_st_require_space start = 20 fixme:d3d:wined3d_cs_st_require_space start = 28 [...] fixme:d3d:wined3d_cs_st_require_space start = 4068 fixme:d3d:wined3d_cs_st_require_space start = 4076 fixme:d3d:wined3d_cs_st_require_space start = 4084 fixme:d3d:wined3d_cs_st_require_space allocated new block 0x579fdb70 fixme:d3d:wined3d_cs_st_require_space start = 0 fixme:d3d:wined3d_cs_st_require_space start = 0 fixme:d3d:wined3d_cs_st_require_space start = 0 [..] In this case, with the buffer basically almost empty, there shouldn't be any need to do memory allocations.
}
static void wined3d_cs_st_push_constants(struct wined3d_cs *cs, enum wined3d_push_constants p, diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h index 54adf26..49fba06 100644 --- a/dlls/wined3d/wined3d_private.h +++ b/dlls/wined3d/wined3d_private.h @@ -3170,7 +3170,7 @@ struct wined3d_cs struct wined3d_fb_state fb; struct wined3d_state state;
- size_t data_size; + size_t data_size, start, end; void *data; };
On 17 February 2017 at 16:12, Sebastian Lackner <sebastian(a)fds-team.de> wrote:
Is it intentional that you are not updating cs->start and cs->end in any other situation? Somewhat, in the sense that I cared more about simplicity than making it optimal at this point. But there's no reason not to fix the issue.
On 17.02.2017 16:38, Henri Verbeet wrote:
On 17 February 2017 at 16:12, Sebastian Lackner <sebastian(a)fds-team.de> wrote:
Is it intentional that you are not updating cs->start and cs->end in any other situation? Somewhat, in the sense that I cared more about simplicity than making it optimal at this point. But there's no reason not to fix the issue.
I don't think fixing this requires a more complex solution, see my proposed patch.
Hi Henri, What is your long term plan regarding this? I'm sure you are aware that my last version avoided emitting CS command from inside the CS and the ringbuffer code is designed around having a single producer and single consumer. Is this patch intended as a temporary measure, or are you planning to retain the ability to emit CS commands from the CS thread? Stefan
On 17.02.2017 20:07, Stefan Dösinger wrote:
Hi Henri,
What is your long term plan regarding this? I'm sure you are aware that my last version avoided emitting CS command from inside the CS and the ringbuffer code is designed around having a single producer and single consumer.
I don't know if its planned as a permanent solution, but emitting commands on the command stream thread (and dispatching them immediately) does not really mean that we can't use a ringbuffer (or any other queue) for commands from other threads. We only have to be careful that wait_idle() doesn't cause a deadlock. ;)
Is this patch intended as a temporary measure, or are you planning to retain the ability to emit CS commands from the CS thread?
Stefan
participants (3)
-
Henri Verbeet -
Sebastian Lackner -
Stefan Dösinger