[PATCH 1/2] wined3d: Acquire sampler references for command lists by inspecting CS packets.

[Henri Verbeet]

On Tue, 23 Nov 2021 at 14:26, Jan Sikorski <jsikorski at codeweavers.com> wrote:
> ---
> This is meant to be followed by more patches that manage references to
> all the other objects that need to be kept alive for command lists.
>
> That includes: all the ones handled similarly to samplers (i.e. the ones
> that we put into dedicated arrays); wined3d_resources, which are
> currently acquired on every draw/dispatch call; and views on these
> resources, which are not handled at all at this point.
>
> Apart from simplifying the code to some degree, it will fix the size
> problem of wined3d_resources arrays, which can get pretty large when the
> application is doing many repeated draw/dispatch calls.
> ---
>  dlls/wined3d/cs.c | 71 +++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 68 insertions(+), 3 deletions(-)
>
Conceptually, I think this patch is fine.

> +static void wined3d_decref_packet_stream_objects(const uint8_t *data, size_t data_size)
> +{
> +    size_t start = 0;
> +    unsigned int i;
> +
> +    while (start < data_size)
> +    {
> +        const struct wined3d_cs_packet *packet = (const struct wined3d_cs_packet *)&data[start];
> +        enum wined3d_cs_op opcode = *(const enum wined3d_cs_op *)packet->data;
> +
> +        switch (opcode)
> +        {
> +            case WINED3D_CS_OP_SET_SAMPLERS:
> +            {
> +                struct wined3d_cs_set_samplers *cs_set
> +                        = (struct wined3d_cs_set_samplers *)packet->data;
> +                for (i = 0; i < cs_set->count; ++i)
> +                {
> +                    if (cs_set->samplers[i])
> +                        wined3d_sampler_decref(cs_set->samplers[i]);
> +                }
> +                break;
> +            }
> +            default:
> +                break;
> +        }
> +
> +        start += offsetof(struct wined3d_cs_packet, data[packet->size]);
> +    }
> +}
> +
> +static void wined3d_incref_packet_objects(struct wined3d_cs_packet *packet)
> +{
> +    enum wined3d_cs_op opcode;
> +    unsigned int i;
> +
> +    opcode = *(const enum wined3d_cs_op *)packet->data;
> +
> +    switch (opcode)
> +    {
> +        case WINED3D_CS_OP_SET_SAMPLERS:
> +        {
> +            struct wined3d_cs_set_samplers *cs_set
> +                    = (struct wined3d_cs_set_samplers *)packet->data;
> +            for (i = 0; i < cs_set->count; ++i)
> +            {
> +                if (cs_set->samplers[i])
> +                    wined3d_sampler_incref(cs_set->samplers[i]);
> +            }
> +            break;
> +        }
> +        default:
> +            break;
> +    }
> +}
> +
The naming/structuring of these is a bit awkward, I think. Following
the usual scheme, wined3d_incref_packet_objects() would be called
wined3d_cs_packet_incref_objects(). It's counterpart would then be
wined3d_cs_packet_decref_objects(). The introduction of
wined3d_cs_packet_decref_objects() would make
wined3d_decref_packet_stream_objects() a fairly straightforward loop;
we could just inline it in its callers. A helper to extract a packet
from a stream would further simplify it, and we could then use that
helper in wined3d_cs_run() as well.

The name "cs_set" is a bit odd, I think. The conventional name for
these is just "op". If we wanted to use a more verbose name,
"set_samplers" after the operation name would also make sense, but
it's not clear to me what "cs_set" refers to.

>  static void wined3d_deferred_context_submit(struct wined3d_device_context *context, enum wined3d_cs_queue_id queue_id)
>  {
> -    assert(queue_id == WINED3D_CS_QUEUE_DEFAULT);
> +    struct wined3d_deferred_context *deferred = wined3d_deferred_context_from_context(context);
> +    struct wined3d_cs_packet *packet;
>
> -    /* Nothing to do. */
> +    assert(queue_id == WINED3D_CS_QUEUE_DEFAULT);
> +    packet = (struct wined3d_cs_packet *)((BYTE *)deferred->data + deferred->data_size);
> +    deferred->data_size += packet->size + offsetof(struct wined3d_cs_packet, data[0]);
> +    wined3d_incref_packet_objects(packet);
>  }
>
"packet = (struct wined3d_cs_packet *)&((uint8_t
*)deferred->data)[deferred->data_size];", arguably.

Not so much an issue with this patch itself, but now that we're doing
these kind of size calculations in more places, perhaps it's also
worth introducing helper functions for them. (E.g., "offsetof(struct
wined3d_cs_packet, data[0])" is the size of the packet header, and
that may not be immediately obvious to a casual observer.)