[PATCH 5/5] d3d10/effect: Add initial support for indexing expressions.
Nikolay Sivov
nsivov at codeweavers.com
Fri Oct 29 15:03:49 CDT 2021
On 10/29/21 10:29 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/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;
>> +}
More information about the wine-devel
mailing list