[PATCH 2/5] d3d10/effect: Implement numeric pass properties updates.

[Matteo Bruni]

On Fri, Oct 29, 2021 at 9:01 PM Nikolay Sivov <nsivov at codeweavers.com> wrote:
>
> On 10/29/21 9:35 PM, Matteo Bruni wrote:
> > On Thu, Oct 28, 2021 at 9:46 AM Nikolay Sivov <nsivov at codeweavers.com> wrote:
> >> Signed-off-by: Nikolay Sivov <nsivov at codeweavers.com>
> >> ---
> >>  dlls/d3d10/d3d10_private.h |   9 +
> >>  dlls/d3d10/effect.c        | 341 ++++++++++++++++++++++++++-----------
> >>  dlls/d3d10/tests/effect.c  | 207 ++++++++++++++++++----
> >>  3 files changed, 420 insertions(+), 137 deletions(-)
> >>
> >> diff --git a/dlls/d3d10/d3d10_private.h b/dlls/d3d10/d3d10_private.h
> >> index 11b3b4e9482..f8e415f860b 100644
> >> --- a/dlls/d3d10/d3d10_private.h
> >> +++ b/dlls/d3d10/d3d10_private.h
> >> @@ -95,6 +95,13 @@ struct d3d10_effect_shader_variable
> >>      unsigned int isinline : 1;
> >>  };
> >>
> >> +struct d3d10_effect_prop_dependencies
> >> +{
> >> +    struct d3d10_effect_prop_dependency *entries;
> >> +    SIZE_T count;
> >> +    SIZE_T capacity;
> >> +};
> >> +
> >>  struct d3d10_effect_sampler_desc
> >>  {
> >>      D3D10_SAMPLER_DESC desc;
> >> @@ -118,6 +125,7 @@ struct d3d10_effect_state_object_variable
> >>          ID3D10SamplerState *sampler;
> >>          IUnknown *object;
> >>      } object;
> >> +    struct d3d10_effect_prop_dependencies dependencies;
> >>  };
> >>
> >>  struct d3d10_effect_resource_variable
> >> @@ -217,6 +225,7 @@ struct d3d10_effect_pass
> >>      char *name;
> >>      struct d3d10_effect_annotations annotations;
> >>
> >> +    struct d3d10_effect_prop_dependencies dependencies;
> >>      struct d3d10_effect_pass_shader_desc vs;
> >>      struct d3d10_effect_pass_shader_desc ps;
> >>      struct d3d10_effect_pass_shader_desc gs;
> >> diff --git a/dlls/d3d10/effect.c b/dlls/d3d10/effect.c
> >> index 23374fdb48f..34a5eb1b701 100644
> >> --- a/dlls/d3d10/effect.c
> >> +++ b/dlls/d3d10/effect.c
> >> @@ -168,6 +168,27 @@ enum d3d10_effect_container_type
> >>      D3D10_C_SAMPLER,
> >>  };
> >>
> >> +struct d3d10_effect_prop_dependency
> >> +{
> >> +    unsigned int id;
> >> +    unsigned int idx;
> >> +    unsigned int operation;
> >> +    union
> >> +    {
> >> +        struct
> >> +        {
> >> +            struct d3d10_effect_variable *v;
> >> +            unsigned int offset;
> >> +        } var;
> >> +    } u;
> >> +};
> > It should be possible to use anonymous unions now in d3d10, right?
> > Not that you necessarily have to do that here, depending on what's
> > going to happen to this struct in the following patches.
>
> It's extended in 5/5. We don't currently use anonymous unions there, so
> I'm simply following existing pattern.

Right, I guess this seemed as good a time as any to start using them.
Either way it's fine to me.

> >> +static void d3d10_effect_update_dependent_props(struct d3d10_effect_prop_dependencies *deps,
> >> +        void *container)
> >> +{
> >> +    const struct d3d10_effect_state_property_info *property_info;
> >> +    struct d3d10_effect_prop_dependency *d;
> >> +    struct d3d10_effect_variable *v;
> >> +    unsigned int i, j, count;
> >> +    uint32_t value;
> >> +    void *dst;
> >> +
> >> +    for (i = 0; i < deps->count; ++i)
> >> +    {
> >> +        d = &deps->entries[i];
> >> +
> >> +        property_info = &property_infos[d->id];
> >> +
> >> +        dst = (char *)container + property_info->offset;
> >> +
> >> +        switch (d->operation)
> >> +        {
> >> +            case D3D10_EOO_VAR:
> >> +            case D3D10_EOO_CONST_INDEX:
> >> +
> >> +                v = d->u.var.v;
> >> +
> >> +                count = v->type->type_class == D3D10_SVC_VECTOR ? 4 : 1;
> >> +
> >> +                for (j = 0; j < count; ++j)
> >> +                {
> >> +                    d3d10_effect_variable_get_raw_value(v, &value, d->u.var.offset + j * sizeof(value), sizeof(value));
> >> +                    d3d10_effect_read_numeric_value(value, v->type->basetype, property_info->type, dst, j);
> >> +                }
> >> +
> >> +                break;
> >> +
> >> +            default:
> >> +                FIXME("Unsupported property update for %u.\n", d->operation);
> >> +        }
> >> +    }
> >> +}
> >> +
> > It would be nice to have dirty flags to avoid unnecessary
> > recomputation. For example, something like the "changed" flag we have
> > for buffers but referring to variables and their values. Problem is,
> > there are a few quirks (e.g. SetRawValue(), those wild out-of-bounds
> > accesses) that make this quite problematic. So I can be convinced to
> > let it go :)
> It would be useful for expressions I imagine, but even for expression it
> might be hard to quantify if all an expression does is a single ftou(),
> which is apparently common.
>
> For value and index updates, I haven't measured obviously, because I
> didn't implement it.
>
> The whole picture is:
>
> ---
> foreach()
> {
>    get_value();
>    put_value();
> }
> use updated fields
> ---
>
> which with change tracking will extend to
>
> ---
> foreach()
> {
>    if (changed)
>    {
>        get_value();
>        put_value();
>        replace_change_marker();
>    }
> }
> use updated fields
> ---
>
> So when nothing changes it will still need to compare for each entry,
> and get/put is reduced to 4 byte read/write most of the time I think, if
> not always. Because e.g. setting things like arrays like blend factor
> produces complex expression, even if you set one element.
>
> Regarding SetRawValue(), yes, the way it works is unfortunate, basically
> you'll have to track variables in constant buffer objects, so when
> SetRawValue is called on a buffer you mark them all as changed. But then
> when you do a larger than necessary raw value write on a variable, you
> potentially change adjacent variables too. Easy way is to mark all as
> changed when writing to buffers, and this and following (by offset) when
> writing to variables.
>
> So that's additional overhead necessary to make property updates respect
> changed/dirty states.

Yeah, sounds like a lot of work for some pretty dubious benefit.

> >
> > It would be something for a separate patch anyway.
> >
> >> @@ -1695,6 +1793,14 @@ static BOOL read_value_list(const char *data, size_t data_size, DWORD offset,
> >>                  *(void **)out_data = &null_shader_resource_variable;
> >>                  break;
> >>
> >> +            case D3D10_SVT_DEPTHSTENCIL:
> >> +                *(void **)out_data = &null_depth_stencil_variable;
> >> +                break;
> >> +
> >> +            case D3D10_SVT_BLEND:
> >> +                *(void **)out_data = &null_blend_variable;
> >> +                break;
> >> +
> >>              default:
> >>                  FIXME("Unhandled out_type %#x.\n", out_type);
> >>                  return FALSE;
> > Would this also make sense as a separate patch?
> It depends on how you look at it, e.g. setting stencil ref int with NULL
> object produces records like this, so it's sort of related. By I can
> split a much as necessary of course.

Yeah, I think it can stand on its own but it's not too ugly to have it in here.

> >> @@ -1745,21 +1851,31 @@ static BOOL is_object_property_type_matching(const struct d3d10_effect_state_pro
> >>      }
> >>  }
> >>
> >> +static HRESULT d3d10_effect_add_prop_dependency(struct d3d10_effect_prop_dependencies *d,
> >> +        const struct d3d10_effect_prop_dependency *dep)
> >> +{
> >> +    if (!d3d_array_reserve((void **)&d->entries, &d->capacity, d->count + 1, sizeof(*d->entries)))
> >> +        return E_OUTOFMEMORY;
> >> +
> >> +    d->entries[d->count++] = *dep;
> >> +
> >> +    return S_OK;
> >> +}
> >> +
> > We probably want to go for the usual exponential growth pattern here.
> > This separate helper is nice in that it can hide that kind of details
> > away.
>
> d3d_array_reserve() already does something like that, conditionally
> doubling sizes. You mean it's not enough?

No you're right, somehow I misread that.