October 2021 - Wine-Devel - list.winehq.org

[PATCH] vkd3d-shader: Fix array size for dimension strings

by Joshua Ashton

Encountered this while I was messing around adding 2DMS support. Signed-off-by: Joshua Ashton <joshua(a)froggi.es> --- libs/vkd3d-shader/hlsl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 9bce6be..82c021a 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -911,7 +911,7 @@ struct vkd3d_string_buffer *hlsl_type_to_string(struct hlsl_ctx *ctx, const stru case HLSL_CLASS_OBJECT: { - static const char dimensions[5][HLSL_SAMPLER_DIM_MAX + 1] = + static const char dimensions[HLSL_SAMPLER_DIM_MAX + 1][5] = { [HLSL_SAMPLER_DIM_1D] = "1D", [HLSL_SAMPLER_DIM_2D] = "2D", -- 2.33.1

Oct. 30, 2021

Re: [PATCH 5/5] d3d10/effect: Add initial support for indexing expressions.

by Nikolay Sivov

On 10/29/21 10:29 PM, Matteo Bruni wrote: > On Thu, Oct 28, 2021 at 9:46 AM Nikolay Sivov <nsivov(a)codeweavers.com> wrote: >> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> >> --- >> dlls/d3d10/effect.c | 560 +++++++++++++++++++++++++++++++++++++- >> dlls/d3d10/tests/effect.c | 114 ++++++++ >> 2 files changed, 672 insertions(+), 2 deletions(-) > My general question / concern on this patch is whether there is room > for code sharing of some kind with d3dx9. Aside from the practical > issue that there is no proper place to put the shared code at the > moment (there is still hope I'll send patches for that eventually), my > feeling is that we could reuse some helpers at least, with some > moderate work. > > I'm okay with duplicating stuff like this for the time being, as long > as we keep the door open to merging code back afterwards. It would certainly be nice if it matched d3dx9 variant. I think only FXLC data format matches so far, not sure if fully or not, regarding variable addressing. Literal constants from CLI4 are different (hence name change), CTAB has same format, but data is packed - you address individual components, not registers. Then there is D3DXGetShaderConstantTable() integration in d3dx9, which we can't use easily, unless you share more, after some reshuffling. Register tables are numbered differently too, and addressing is different. From what I've tried it's using {table_idx,idx} to address components. Next thing is that immediate results are doubles in d3dx9, and I don't see this in d3d10. > >> diff --git a/dlls/d3d10/effect.c b/dlls/d3d10/effect.c >> index 003060598a3..5e93fdddde3 100644 >> --- a/dlls/d3d10/effect.c >> +++ b/dlls/d3d10/effect.c >> @@ -30,6 +30,9 @@ WINE_DEFAULT_DEBUG_CHANNEL(d3d10); >> ((DWORD)(ch2) << 16) | ((DWORD)(ch3) << 24 )) >> #define TAG_DXBC MAKE_TAG('D', 'X', 'B', 'C') >> #define TAG_FX10 MAKE_TAG('F', 'X', '1', '0') >> +#define TAG_FXLC MAKE_TAG('F', 'X', 'L', 'C') >> +#define TAG_CLI4 MAKE_TAG('C', 'L', 'I', '4') >> +#define TAG_CTAB MAKE_TAG('C', 'T', 'A', 'B') >> >> #define D3D10_FX10_TYPE_COLUMN_SHIFT 11 >> #define D3D10_FX10_TYPE_COLUMN_MASK (0x7 << D3D10_FX10_TYPE_COLUMN_SHIFT) >> @@ -168,6 +171,92 @@ enum d3d10_effect_container_type >> D3D10_C_SAMPLER, >> }; >> >> +struct preshader_instr >> +{ >> + unsigned int comp_count : 16; >> + unsigned int reserved : 4; >> + unsigned int opcode : 11; >> + unsigned int scalar : 1; >> +}; >> + >> +typedef float (*pres_op_func)(float **args, unsigned int n); >> + >> +struct preshader_op_info >> +{ >> + unsigned short idx; >> + unsigned short opcode; >> + char name[16]; >> + pres_op_func func; >> +}; >> + >> +static float pres_ftou(float **args, unsigned int n) >> +{ >> + unsigned int u = *args[0]; >> + return *(float *)&u; >> +} >> + >> +static float pres_add(float **args, unsigned int n) >> +{ >> + return *args[0] + *args[1]; >> +} >> + >> +enum preshader_op >> +{ >> + D3D10_PRESHADER_OP_FTOU = 0, >> + D3D10_PRESHADER_OP_ADD, >> + D3D10_PRESHADER_OP_MAX, >> +}; > It looks like you meant to use D3D10_PRESHADER_OP_MAX purely as a > sentinel but that's confusing at best (in fact there is a MAX op in > d3dx9 at least). I can see now how this is confusing, didn't think of max(), because I didn't have to deal with it. > > I also have a nitpick: moving the definition of preshader_op_info > right here, just above the definition of preshader_ops[], seems nicer. > >> +static const struct preshader_op_info preshader_ops[] = >> +{ >> + { D3D10_PRESHADER_OP_FTOU, 0x133, "ftou", pres_ftou }, >> + { D3D10_PRESHADER_OP_ADD, 0x204, "add", pres_add }, >> +}; >> + >> +struct d3d10_ctab_var >> +{ >> + struct d3d10_effect_variable *v; >> + unsigned int offset; >> + unsigned int length; >> +}; >> + >> +struct d3d10_reg_table >> +{ >> + union >> + { >> + float *f; >> + DWORD *dword; >> + char *byte; >> + } u; >> + unsigned int count; >> +}; >> + >> +enum d3d10_reg_table_type >> +{ >> + D3D10_REG_TABLE_CONSTANTS = 1, >> + D3D10_REG_TABLE_CB = 2, >> + D3D10_REG_TABLE_RESULT = 4, >> + D3D10_REG_TABLE_TEMP = 7, >> +}; >> + >> +struct d3d10_effect_preshader >> +{ >> + struct d3d10_reg_table constants; >> + struct d3d10_reg_table cb; >> + struct d3d10_reg_table result; >> + struct d3d10_reg_table temp; >> + struct d3d10_reg_table code; >> + >> + struct d3d10_ctab_var *vars; >> + unsigned int vars_count; >> +}; > It might make sense to organize the various struct d3d10_reg_table > entries into a single array indexed by an enum d3d10_reg_table_type > value. It should allow some simplification down the line, like for > example... Yes, I wanted to do it like that, or maybe even started in earlier versions. Natural way to refer to them would be with d3d10_reg_table_type, which is rather sparse. Maybe they do have tables 3, 5, and 6, but I haven't found them yet. And "code" one is obviously different. > >> +static void d3d10_effect_preshader_clear(struct d3d10_effect_preshader *p) >> +{ >> + d3d10_reg_table_clear(&p->constants); >> + d3d10_reg_table_clear(&p->cb); >> + d3d10_reg_table_clear(&p->result); >> + d3d10_reg_table_clear(&p->temp); >> + d3d10_reg_table_clear(&p->code); >> + heap_free(p->vars); >> + p->vars = NULL; >> + p->vars_count = 0; >> +} > ... here, or... > >> + >> +static float * d3d10_effect_preshader_get_reg_ptr(const struct d3d10_effect_preshader *p, >> + enum d3d10_reg_table_type regt, unsigned int offset) >> +{ >> + switch (regt) >> + { >> + case D3D10_REG_TABLE_CONSTANTS: >> + return p->constants.u.f + offset; >> + case D3D10_REG_TABLE_CB: >> + return p->cb.u.f + offset; >> + case D3D10_REG_TABLE_RESULT: >> + return p->result.u.f + offset; >> + case D3D10_REG_TABLE_TEMP: >> + return p->temp.u.f + offset; >> + default: >> + return NULL; >> + } >> +} > ... here, this one should become trivial. You want to NULL-check the > table before accessing it, either explicitly or with an assert. > >> @@ -543,6 +748,39 @@ static void d3d10_effect_update_dependent_props(struct d3d10_effect_prop_depende >> >> break; >> >> + case D3D10_EOO_INDEX_EXPRESSION: >> + >> + v = d->u.index_expr.v; >> + >> + /* Evaluate index. */ >> + if (FAILED(hr = d3d10_effect_preshader_eval(&d->u.index_expr.index))) >> + { >> + WARN("Failed to evaluate index expression, hr %#x.\n", hr); >> + return; >> + } > That kind of comment doesn't seem very helpful. > >> + >> + variable_idx = *d->u.index_expr.index.result.u.dword; >> + >> + if (variable_idx >= v->type->element_count) >> + { >> + WARN("Expression evaluated to invalid index value %u, array %s of size %u.\n", >> + variable_idx, debugstr_a(v->name), v->type->element_count); >> + return; >> + } >> + >> + switch (property_info->type) >> + { >> + case D3D10_SVT_VERTEXSHADER: >> + case D3D10_SVT_PIXELSHADER: >> + case D3D10_SVT_GEOMETRYSHADER: >> + ((void **)dst)[d->idx] = v; >> + *dst_index = variable_idx; >> + break; >> + default: >> + ((void **)dst)[d->idx] = &v->elements[variable_idx]; >> + } >> + break; >> + >> default: >> FIXME("Unsupported property update for %u.\n", d->operation); >> } >> +static HRESULT parse_fx10_ctab(void *ctx, const char *data, unsigned int data_size) >> +{ >> + struct d3d10_preshader_parse_context *context = ctx; >> + struct d3d10_effect_preshader *p = context->preshader; >> + struct ctab_header >> + { >> + DWORD size; >> + DWORD creator; >> + DWORD version; >> + DWORD constants; >> + DWORD constantinfo; >> + DWORD flags; >> + DWORD target; >> + } header; >> + struct ctab_const_info >> + { >> + DWORD name; >> + WORD register_set; >> + WORD register_index; >> + WORD register_count; >> + WORD reserved; >> + DWORD typeinfo; >> + DWORD default_value; >> + } *info; >> + unsigned int i, info_offset, unused, cb_reg_count = 0; >> + const char *ptr = data; >> + const char *name; >> + size_t name_len; >> + HRESULT hr; >> + >> + if (data_size < sizeof(header)) >> + { >> + WARN("Invalid constant table size %u.\n", data_size); >> + return E_FAIL; >> + } >> + >> + read_dword(&ptr, &unused); >> + read_dword(&ptr, &unused); >> + read_dword(&ptr, &unused); >> + read_dword(&ptr, &p->vars_count); >> + read_dword(&ptr, &info_offset); >> + read_dword(&ptr, &unused); >> + read_dword(&ptr, &unused); > Any clue about all those unused DWORDs? It's ctab_header. > >> + >> + if (!require_space(info_offset, p->vars_count, sizeof(*info) / sizeof(DWORD), >> + data_size)) >> + { >> + WARN("Invalid constant info section offset %#x.\n", info_offset); >> + return E_FAIL; >> + } >> + >> + TRACE("Variable count %u.\n", p->vars_count); >> + >> + if (!(p->vars = heap_calloc(p->vars_count, sizeof(*p->vars)))) >> + return E_OUTOFMEMORY; >> + >> + /* Collect variables used in expression. */ >> + info = (struct ctab_const_info *)(data + info_offset); >> + for (i = 0; i < p->vars_count; ++i, ++info) >> + { >> + if (!fx10_get_string(data, data_size, info->name, &name, &name_len)) >> + return E_FAIL; >> + >> + if (!(p->vars[i].v = d3d10_effect_get_variable_by_name(context->effect, name))) >> + { >> + WARN("Couldn't find variable %s.\n", debugstr_a(name)); >> + return E_FAIL; >> + } >> + >> + /* 4 components per register */ >> + p->vars[i].offset = info->register_index * 4; >> + p->vars[i].length = info->register_count * 4; >> + >> + cb_reg_count = max(cb_reg_count, info->register_index + info->register_count); >> + } >> + >> + /* Allocate contiguous "constant buffer" for all referenced variables. */ >> + if (FAILED(hr = d3d10_reg_table_allocate(&p->cb, cb_reg_count * 4))) >> + { >> + WARN("Failed to allocate variables buffer.\n"); >> + return hr; >> + } >> + >> + return S_OK; >> +}

Oct. 29, 2021

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

by Matteo Bruni

On Fri, Oct 29, 2021 at 9:01 PM Nikolay Sivov <nsivov(a)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(a)codeweavers.com> wrote: > >> Signed-off-by: Nikolay Sivov <nsivov(a)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.

Oct. 29, 2021

Re: [PATCH 5/5] d3d10/effect: Add initial support for indexing expressions.

by Matteo Bruni

On Thu, Oct 28, 2021 at 9:46 AM Nikolay Sivov <nsivov(a)codeweavers.com> wrote: > > Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> > --- > dlls/d3d10/effect.c | 560 +++++++++++++++++++++++++++++++++++++- > dlls/d3d10/tests/effect.c | 114 ++++++++ > 2 files changed, 672 insertions(+), 2 deletions(-) My general question / concern on this patch is whether there is room for code sharing of some kind with d3dx9. Aside from the practical issue that there is no proper place to put the shared code at the moment (there is still hope I'll send patches for that eventually), my feeling is that we could reuse some helpers at least, with some moderate work. I'm okay with duplicating stuff like this for the time being, as long as we keep the door open to merging code back afterwards. > diff --git a/dlls/d3d10/effect.c b/dlls/d3d10/effect.c > index 003060598a3..5e93fdddde3 100644 > --- a/dlls/d3d10/effect.c > +++ b/dlls/d3d10/effect.c > @@ -30,6 +30,9 @@ WINE_DEFAULT_DEBUG_CHANNEL(d3d10); > ((DWORD)(ch2) << 16) | ((DWORD)(ch3) << 24 )) > #define TAG_DXBC MAKE_TAG('D', 'X', 'B', 'C') > #define TAG_FX10 MAKE_TAG('F', 'X', '1', '0') > +#define TAG_FXLC MAKE_TAG('F', 'X', 'L', 'C') > +#define TAG_CLI4 MAKE_TAG('C', 'L', 'I', '4') > +#define TAG_CTAB MAKE_TAG('C', 'T', 'A', 'B') > > #define D3D10_FX10_TYPE_COLUMN_SHIFT 11 > #define D3D10_FX10_TYPE_COLUMN_MASK (0x7 << D3D10_FX10_TYPE_COLUMN_SHIFT) > @@ -168,6 +171,92 @@ enum d3d10_effect_container_type > D3D10_C_SAMPLER, > }; > > +struct preshader_instr > +{ > + unsigned int comp_count : 16; > + unsigned int reserved : 4; > + unsigned int opcode : 11; > + unsigned int scalar : 1; > +}; > + > +typedef float (*pres_op_func)(float **args, unsigned int n); > + > +struct preshader_op_info > +{ > + unsigned short idx; > + unsigned short opcode; > + char name[16]; > + pres_op_func func; > +}; > + > +static float pres_ftou(float **args, unsigned int n) > +{ > + unsigned int u = *args[0]; > + return *(float *)&u; > +} > + > +static float pres_add(float **args, unsigned int n) > +{ > + return *args[0] + *args[1]; > +} > + > +enum preshader_op > +{ > + D3D10_PRESHADER_OP_FTOU = 0, > + D3D10_PRESHADER_OP_ADD, > + D3D10_PRESHADER_OP_MAX, > +}; It looks like you meant to use D3D10_PRESHADER_OP_MAX purely as a sentinel but that's confusing at best (in fact there is a MAX op in d3dx9 at least). I also have a nitpick: moving the definition of preshader_op_info right here, just above the definition of preshader_ops[], seems nicer. > +static const struct preshader_op_info preshader_ops[] = > +{ > + { D3D10_PRESHADER_OP_FTOU, 0x133, "ftou", pres_ftou }, > + { D3D10_PRESHADER_OP_ADD, 0x204, "add", pres_add }, > +}; > + > +struct d3d10_ctab_var > +{ > + struct d3d10_effect_variable *v; > + unsigned int offset; > + unsigned int length; > +}; > + > +struct d3d10_reg_table > +{ > + union > + { > + float *f; > + DWORD *dword; > + char *byte; > + } u; > + unsigned int count; > +}; > + > +enum d3d10_reg_table_type > +{ > + D3D10_REG_TABLE_CONSTANTS = 1, > + D3D10_REG_TABLE_CB = 2, > + D3D10_REG_TABLE_RESULT = 4, > + D3D10_REG_TABLE_TEMP = 7, > +}; > + > +struct d3d10_effect_preshader > +{ > + struct d3d10_reg_table constants; > + struct d3d10_reg_table cb; > + struct d3d10_reg_table result; > + struct d3d10_reg_table temp; > + struct d3d10_reg_table code; > + > + struct d3d10_ctab_var *vars; > + unsigned int vars_count; > +}; It might make sense to organize the various struct d3d10_reg_table entries into a single array indexed by an enum d3d10_reg_table_type value. It should allow some simplification down the line, like for example... > +static void d3d10_effect_preshader_clear(struct d3d10_effect_preshader *p) > +{ > + d3d10_reg_table_clear(&p->constants); > + d3d10_reg_table_clear(&p->cb); > + d3d10_reg_table_clear(&p->result); > + d3d10_reg_table_clear(&p->temp); > + d3d10_reg_table_clear(&p->code); > + heap_free(p->vars); > + p->vars = NULL; > + p->vars_count = 0; > +} ... here, or... > + > +static float * d3d10_effect_preshader_get_reg_ptr(const struct d3d10_effect_preshader *p, > + enum d3d10_reg_table_type regt, unsigned int offset) > +{ > + switch (regt) > + { > + case D3D10_REG_TABLE_CONSTANTS: > + return p->constants.u.f + offset; > + case D3D10_REG_TABLE_CB: > + return p->cb.u.f + offset; > + case D3D10_REG_TABLE_RESULT: > + return p->result.u.f + offset; > + case D3D10_REG_TABLE_TEMP: > + return p->temp.u.f + offset; > + default: > + return NULL; > + } > +} ... here, this one should become trivial. You want to NULL-check the table before accessing it, either explicitly or with an assert. > @@ -543,6 +748,39 @@ static void d3d10_effect_update_dependent_props(struct d3d10_effect_prop_depende > > break; > > + case D3D10_EOO_INDEX_EXPRESSION: > + > + v = d->u.index_expr.v; > + > + /* Evaluate index. */ > + if (FAILED(hr = d3d10_effect_preshader_eval(&d->u.index_expr.index))) > + { > + WARN("Failed to evaluate index expression, hr %#x.\n", hr); > + return; > + } That kind of comment doesn't seem very helpful. > + > + variable_idx = *d->u.index_expr.index.result.u.dword; > + > + if (variable_idx >= v->type->element_count) > + { > + WARN("Expression evaluated to invalid index value %u, array %s of size %u.\n", > + variable_idx, debugstr_a(v->name), v->type->element_count); > + return; > + } > + > + switch (property_info->type) > + { > + case D3D10_SVT_VERTEXSHADER: > + case D3D10_SVT_PIXELSHADER: > + case D3D10_SVT_GEOMETRYSHADER: > + ((void **)dst)[d->idx] = v; > + *dst_index = variable_idx; > + break; > + default: > + ((void **)dst)[d->idx] = &v->elements[variable_idx]; > + } > + break; > + > default: > FIXME("Unsupported property update for %u.\n", d->operation); > } > +static HRESULT parse_fx10_ctab(void *ctx, const char *data, unsigned int data_size) > +{ > + struct d3d10_preshader_parse_context *context = ctx; > + struct d3d10_effect_preshader *p = context->preshader; > + struct ctab_header > + { > + DWORD size; > + DWORD creator; > + DWORD version; > + DWORD constants; > + DWORD constantinfo; > + DWORD flags; > + DWORD target; > + } header; > + struct ctab_const_info > + { > + DWORD name; > + WORD register_set; > + WORD register_index; > + WORD register_count; > + WORD reserved; > + DWORD typeinfo; > + DWORD default_value; > + } *info; > + unsigned int i, info_offset, unused, cb_reg_count = 0; > + const char *ptr = data; > + const char *name; > + size_t name_len; > + HRESULT hr; > + > + if (data_size < sizeof(header)) > + { > + WARN("Invalid constant table size %u.\n", data_size); > + return E_FAIL; > + } > + > + read_dword(&ptr, &unused); > + read_dword(&ptr, &unused); > + read_dword(&ptr, &unused); > + read_dword(&ptr, &p->vars_count); > + read_dword(&ptr, &info_offset); > + read_dword(&ptr, &unused); > + read_dword(&ptr, &unused); Any clue about all those unused DWORDs? > + > + if (!require_space(info_offset, p->vars_count, sizeof(*info) / sizeof(DWORD), > + data_size)) > + { > + WARN("Invalid constant info section offset %#x.\n", info_offset); > + return E_FAIL; > + } > + > + TRACE("Variable count %u.\n", p->vars_count); > + > + if (!(p->vars = heap_calloc(p->vars_count, sizeof(*p->vars)))) > + return E_OUTOFMEMORY; > + > + /* Collect variables used in expression. */ > + info = (struct ctab_const_info *)(data + info_offset); > + for (i = 0; i < p->vars_count; ++i, ++info) > + { > + if (!fx10_get_string(data, data_size, info->name, &name, &name_len)) > + return E_FAIL; > + > + if (!(p->vars[i].v = d3d10_effect_get_variable_by_name(context->effect, name))) > + { > + WARN("Couldn't find variable %s.\n", debugstr_a(name)); > + return E_FAIL; > + } > + > + /* 4 components per register */ > + p->vars[i].offset = info->register_index * 4; > + p->vars[i].length = info->register_count * 4; > + > + cb_reg_count = max(cb_reg_count, info->register_index + info->register_count); > + } > + > + /* Allocate contiguous "constant buffer" for all referenced variables. */ > + if (FAILED(hr = d3d10_reg_table_allocate(&p->cb, cb_reg_count * 4))) > + { > + WARN("Failed to allocate variables buffer.\n"); > + return hr; > + } > + > + return S_OK; > +}

Oct. 29, 2021

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

by Nikolay Sivov

On 10/29/21 9:35 PM, Matteo Bruni wrote: > On Thu, Oct 28, 2021 at 9:46 AM Nikolay Sivov <nsivov(a)codeweavers.com> wrote: >> Signed-off-by: Nikolay Sivov <nsivov(a)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.

Oct. 29, 2021

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

by Matteo Bruni

On Thu, Oct 28, 2021 at 9:46 AM Nikolay Sivov <nsivov(a)codeweavers.com> wrote: > > Signed-off-by: Nikolay Sivov <nsivov(a)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. > @@ -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. > +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 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? > @@ -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. > @@ -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... > 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.

Oct. 29, 2021

Re: [PATCH 4/5] d3d10/effect: Reduce duplication when releasing shader objects.

by Matteo Bruni

Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com>

Oct. 29, 2021

Re: [PATCH 3/5] d3d10/effect: Get rid of interface pointer casts in IsValid().

by Matteo Bruni

Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com> --- This patch could have been split too but I guess I don't care enough in this particular case.

Oct. 29, 2021

Re: [PATCH 1/5] d3d10/effect: Remove a cast in IsValid() for SRV variables.

by Matteo Bruni

Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com>

Oct. 29, 2021

[PATCH 2/2] server: Remove dr7 PTRACE_POKEUSER workaround.

by Rémi Bernon

It is needed for Linux Kernel version 4.18, and has been fixed in 4.19. Also see https://bugzilla.kernel.org/show_bug.cgi?id=200965 for a more detailed description of the regression and the fix. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46472 Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- server/ptrace.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/server/ptrace.c b/server/ptrace.c index b9dd9e9539a..1875e1dfd56 100644 --- a/server/ptrace.c +++ b/server/ptrace.c @@ -627,9 +627,6 @@ void set_thread_context( struct thread *thread, const context_t *context, unsign if (!suspend_for_ptrace( thread )) return; - /* force all breakpoint lengths to 1, workaround for kernel bug 200965 */ - ptrace( PTRACE_POKEUSER, pid, DR_OFFSET(7), 0x11110055 ); - switch (context->machine) { case IMAGE_FILE_MACHINE_I386: -- 2.33.0

Oct. 29, 2021