Re: [PATCH 2/5] wined3d: Handle CS op emission from CS op handlers.

[Sebastian Lackner]

On 17.02.2017 14:08, Henri Verbeet wrote:
> Signed-off-by: Henri Verbeet
>
> 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;
>
> };

[PATCH 2/5] wined3d: Handle CS op emission from CS op handlers.

[Henri Verbeet]

On 17 February 2017 at 16:12, Sebastian Lackner  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.

[PATCH 2/5] wined3d: Handle CS op emission from CS op handlers.

[Sebastian Lackner]

On 17.02.2017 16:38, Henri Verbeet wrote:
> On 17 February 2017 at 16:12, Sebastian Lackner  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.

[PATCH 2/5] wined3d: Handle CS op emission from CS op handlers.

[Henri Verbeet]

On 17 February 2017 at 16:59, Sebastian Lackner  wrote:
> I don't think fixing this requires a more complex solution, see my proposed patch.

I think that should work, yeah.

[PATCH 2/5] wined3d: Handle CS op emission from CS op handlers.

[Stefan Dösinger]

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

[PATCH 2/5] wined3d: Handle CS op emission from CS op handlers.

[Sebastian Lackner]

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