[PATCH 2/5] d3d10/effect: Implement numeric pass properties updates.
Nikolay Sivov
nsivov at codeweavers.com
Fri Oct 29 14:01:41 CDT 2021
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.
>> @@ -386,6 +407,148 @@ static const char *debug_d3d10_shader_variable_type(D3D10_SHADER_VARIABLE_TYPE t
>>
>> #undef WINE_D3D10_TO_STR
>>
>> +static HRESULT d3d10_effect_variable_get_raw_value(struct d3d10_effect_variable *v,
>> + void *data, unsigned int offset, unsigned int count)
>> +{
>> + BOOL is_buffer;
>> +
>> + is_buffer = v->type->basetype == D3D10_SVT_CBUFFER || v->type->basetype == D3D10_SVT_TBUFFER;
>> +
>> + if (v->type->type_class == D3D10_SVC_OBJECT && !is_buffer)
>> + {
>> + WARN("Not supported on object variables of type %s.\n",
>> + debug_d3d10_shader_variable_type(v->type->basetype));
>> + return D3DERR_INVALIDCALL;
>> + }
>> +
>> + if (!is_buffer)
>> + {
>> + offset += v->buffer_offset;
>> + v = v->buffer;
>> + }
>> +
>> + memcpy(data, v->u.buffer.local_buffer + offset, count);
>> +
>> + return S_OK;
>> +}
>> +
>> +static BOOL read_float_value(DWORD value, D3D_SHADER_VARIABLE_TYPE in_type, float *out_data, UINT idx)
> I think you forgot to update these helpers after you introduced
> d3d10_effect_read_numeric_value(), specifically WRT replacing DWORD
> with uint32_t for value and UINT with unsigned int for idx.
>
> Actually, it's probably nicer if you split the introduction of that
> helper to a separate patch. You could also change the argument types
> of read_value_list() in the same patch.
Yes, will do.
>
>> +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.
>
> 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.
>
>> @@ -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?
>> @@ -4873,6 +5004,7 @@ static BOOL get_value_as_bool(void *src_data, D3D10_SHADER_VARIABLE_TYPE src_typ
>> {
>> case D3D10_SVT_FLOAT:
>> case D3D10_SVT_INT:
>> + case D3D10_SVT_UINT:
>> case D3D10_SVT_BOOL:
>> if (*(DWORD *)src_data)
>> return -1;
>> @@ -4893,6 +5025,7 @@ static int get_value_as_int(void *src_data, D3D10_SHADER_VARIABLE_TYPE src_type)
>> return (int)(*(float *)src_data);
>>
>> case D3D10_SVT_INT:
>> + case D3D10_SVT_UINT:
>> return *(int *)src_data;
>>
>> case D3D10_SVT_BOOL:
>> @@ -4911,6 +5044,7 @@ static float get_value_as_float(void *src_data, D3D10_SHADER_VARIABLE_TYPE src_t
>> return *(float *)src_data;
>>
>> case D3D10_SVT_INT:
>> + case D3D10_SVT_UINT:
>> return (float)(*(int *)src_data);
>>
>> case D3D10_SVT_BOOL:
>> @@ -4941,6 +5075,7 @@ static void get_vector_as_type(BYTE *dst_data, D3D_SHADER_VARIABLE_TYPE dst_type
>> break;
>>
>> case D3D10_SVT_INT:
>> + case D3D10_SVT_UINT:
>> *(int *)dst_data_dword = get_value_as_int(src_data_dword, src_type);
>> break;
>>
> These hunks should probably be split to a separate patch...
That's true, will do.
>
>> diff --git a/dlls/d3d10/tests/effect.c b/dlls/d3d10/tests/effect.c
>> index a8a64a6ed0d..b379fc7bddc 100644
>> --- a/dlls/d3d10/tests/effect.c
>> +++ b/dlls/d3d10/tests/effect.c
>> @@ -4843,43 +4843,32 @@ cbuffer cb
>> float f0, f_a[2];
>> int i0, i_a[2];
>> bool b0, b_a[2];
>> + uint i1, i1_a[2];
>> };
>> #endif
>> static DWORD fx_test_scalar_variable[] =
>> {
>> - 0x43425844, 0xe4da4aa6, 0x1380ddc5, 0x445edad5,
>> - 0x08581666, 0x00000001, 0x0000020b, 0x00000001,
>> - 0x00000024, 0x30315846, 0x000001df, 0xfeff1001,
>> - 0x00000001, 0x00000006, 0x00000000, 0x00000000,
>> - 0x00000000, 0x00000000, 0x00000000, 0x000000d3,
>> - 0x00000000, 0x00000000, 0x00000000, 0x00000000,
>> - 0x00000000, 0x00000000, 0x00000000, 0x00000000,
>> - 0x00000000, 0x00000000, 0x00000000, 0x66006263,
>> - 0x74616f6c, 0x00000700, 0x00000100, 0x00000000,
>> - 0x00000400, 0x00001000, 0x00000400, 0x00090900,
>> - 0x00306600, 0x00000007, 0x00000001, 0x00000002,
>> - 0x00000014, 0x00000010, 0x00000008, 0x00000909,
>> - 0x00615f66, 0x00746e69, 0x0000004c, 0x00000001,
>> - 0x00000000, 0x00000004, 0x00000010, 0x00000004,
>> - 0x00000911, 0x4c003069, 0x01000000, 0x02000000,
>> - 0x14000000, 0x10000000, 0x08000000, 0x11000000,
>> - 0x69000009, 0x6200615f, 0x006c6f6f, 0x0000008f,
>> - 0x00000001, 0x00000000, 0x00000004, 0x00000010,
>> - 0x00000004, 0x00000921, 0x8f003062, 0x01000000,
>> - 0x02000000, 0x14000000, 0x10000000, 0x08000000,
>> - 0x21000000, 0x62000009, 0x0400615f, 0x70000000,
>> - 0x00000000, 0x06000000, 0xff000000, 0x00ffffff,
>> - 0x29000000, 0x0d000000, 0x00000000, 0x00000000,
>> - 0x00000000, 0x00000000, 0x00000000, 0x48000000,
>> - 0x2c000000, 0x00000000, 0x10000000, 0x00000000,
>> - 0x00000000, 0x00000000, 0x6c000000, 0x50000000,
>> - 0x00000000, 0x24000000, 0x00000000, 0x00000000,
>> - 0x00000000, 0x8b000000, 0x6f000000, 0x00000000,
>> - 0x30000000, 0x00000000, 0x00000000, 0x00000000,
>> - 0xb0000000, 0x94000000, 0x00000000, 0x44000000,
>> - 0x00000000, 0x00000000, 0x00000000, 0xcf000000,
>> - 0xb3000000, 0x00000000, 0x50000000, 0x00000000,
>> - 0x00000000, 0x00000000, 0x00000000,
>> + 0x43425844, 0x7d97f44c, 0x1da4b110, 0xb710407e, 0x26750c1c, 0x00000001, 0x00000288, 0x00000001,
>> + 0x00000024, 0x30315846, 0x0000025c, 0xfeff1001, 0x00000001, 0x00000008, 0x00000000, 0x00000000,
>> + 0x00000000, 0x00000000, 0x00000000, 0x00000118, 0x00000000, 0x00000000, 0x00000000, 0x00000000,
>> + 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x66006263,
>> + 0x74616f6c, 0x00000700, 0x00000100, 0x00000000, 0x00000400, 0x00001000, 0x00000400, 0x00090900,
>> + 0x00306600, 0x00000007, 0x00000001, 0x00000002, 0x00000014, 0x00000010, 0x00000008, 0x00000909,
>> + 0x00615f66, 0x00746e69, 0x0000004c, 0x00000001, 0x00000000, 0x00000004, 0x00000010, 0x00000004,
>> + 0x00000911, 0x4c003069, 0x01000000, 0x02000000, 0x14000000, 0x10000000, 0x08000000, 0x11000000,
>> + 0x69000009, 0x6200615f, 0x006c6f6f, 0x0000008f, 0x00000001, 0x00000000, 0x00000004, 0x00000010,
>> + 0x00000004, 0x00000921, 0x8f003062, 0x01000000, 0x02000000, 0x14000000, 0x10000000, 0x08000000,
>> + 0x21000000, 0x62000009, 0x7500615f, 0x00746e69, 0x000000d3, 0x00000001, 0x00000000, 0x00000004,
>> + 0x00000010, 0x00000004, 0x00000919, 0xd3003169, 0x01000000, 0x02000000, 0x14000000, 0x10000000,
>> + 0x08000000, 0x19000000, 0x69000009, 0x00615f31, 0x00000004, 0x00000090, 0x00000000, 0x00000008,
>> + 0xffffffff, 0x00000000, 0x00000029, 0x0000000d, 0x00000000, 0x00000000, 0x00000000, 0x00000000,
>> + 0x00000000, 0x00000048, 0x0000002c, 0x00000000, 0x00000010, 0x00000000, 0x00000000, 0x00000000,
>> + 0x0000006c, 0x00000050, 0x00000000, 0x00000024, 0x00000000, 0x00000000, 0x00000000, 0x0000008b,
>> + 0x0000006f, 0x00000000, 0x00000030, 0x00000000, 0x00000000, 0x00000000, 0x000000b0, 0x00000094,
>> + 0x00000000, 0x00000044, 0x00000000, 0x00000000, 0x00000000, 0x000000cf, 0x000000b3, 0x00000000,
>> + 0x00000050, 0x00000000, 0x00000000, 0x00000000, 0x000000f4, 0x000000d8, 0x00000000, 0x00000064,
>> + 0x00000000, 0x00000000, 0x00000000, 0x00000113, 0x000000f7, 0x00000000, 0x00000070, 0x00000000,
>> + 0x00000000, 0x00000000,
>> };
>>
>> static void test_scalar_methods(ID3D10EffectScalarVariable *var, D3D10_SHADER_VARIABLE_TYPE type,
>> @@ -5146,6 +5135,7 @@ static void test_effect_scalar_variable(void)
>> {"f0", D3D10_SVT_FLOAT},
>> {"i0", D3D10_SVT_INT},
>> {"b0", D3D10_SVT_BOOL},
>> + {"i1", D3D10_SVT_UINT},
>> {"f_a", D3D10_SVT_FLOAT, TRUE},
>> {"i_a", D3D10_SVT_INT, TRUE},
>> {"b_a", D3D10_SVT_BOOL, TRUE},
>> @@ -5178,7 +5168,7 @@ static void test_effect_scalar_variable(void)
>> effect_desc.ConstantBuffers);
>> ok(effect_desc.SharedConstantBuffers == 0, "Unexpected shared constant buffers count %u.\n",
>> effect_desc.SharedConstantBuffers);
>> - ok(effect_desc.GlobalVariables == 6, "Unexpected global variables count %u.\n",
>> + ok(effect_desc.GlobalVariables == 8, "Unexpected global variables count %u.\n",
>> effect_desc.GlobalVariables);
>> ok(effect_desc.SharedGlobalVariables == 0, "Unexpected shared global variables count %u.\n",
>> effect_desc.SharedGlobalVariables);
> ... together with this test change.
More information about the wine-devel
mailing list