[prev in list] [next in list] [prev in thread] [next in thread]
List: wine-devel
Subject: Re: [PATCH 2/5] d3d10/effect: Implement numeric pass properties updates.
From: Matteo Bruni <matteo.mystral () gmail ! com>
Date: 2021-10-29 19:39:14
Message-ID: CABvNrtNGS2wbiTfYJnm=uLa7k-GVghrjZcd8Jcdov7p5qun0zA () mail ! gmail ! com
[Download RAW message or body]
On Fri, Oct 29, 2021 at 9:01 PM Nikolay Sivov <nsivov@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@codeweavers.com> wrote:
> > > Signed-off-by: Nikolay Sivov <nsivov@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.
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic