From: Henri Verbeet Subject: Re: [PATCH v2 1/2] vkd3d: Deal correctly with SM 5.1 register spaces. Message-Id: Date: Tue, 26 Nov 2019 20:38:31 +0330 In-Reply-To: <20191118143517.10233-1-post@arntzen-software.no> References: <20191118143517.10233-1-post@arntzen-software.no> On Mon, 18 Nov 2019 at 18:11, Hans-Kristian Arntzen wrote: > Resource index is found in idx[0] in SM 5.0, but idx[1] when using SM > 5.1, and register space is encoded reparately. An rb_tree keeps track of > the internal resource index idx[0] and can map that to space/binding as > required when emitting SPIR-V. > > For this to work, we must also make UAV counters register space aware. > In earlier implementation, UAV counter mask was assumed to correlate 1:1 > with register_index, which breaks on SM 5.1. > This doesn't quite apply on current vkd3d git, but after resolving the conflicts, I get soft GPU lockups in the d3d12 tests. (I.e., "*ERROR* ring gfx timeout, but soft recovered") I didn't do a lot of debugging into that, but potentially could if you are unable to reproduce the issue yourself. Generally speaking, it seems this patch does a bunch of things and could be split. (E.g. introducing the "register_index" field in struct vkd3d_shader_semantic doesn't really depend on the changes to the public vkd3d-shader interface.) > +/* Extends vkd3d_shader_interface_info. */ > +struct vkd3d_shader_effective_uav_counter_binding_info > +{ > + enum vkd3d_shader_structure_type type; > + const void *next; > + > + unsigned int *uav_register_spaces; > + unsigned int *uav_register_bindings; > + unsigned int uav_counter_count; > +}; > + Why does that extend vkd3d_shader_interface_info? This seems like something that should be part of vkd3d_shader_scan_info. > +struct vkd3d_shader_scan_info_binding > +{ > + unsigned int register_space; > + unsigned int register_idx; > +}; > + This is entirely unused. > +static bool shader_is_sm_5_1(const struct vkd3d_dxbc_compiler *compiler) > +{ > + return (compiler->shader_version.major * 100 + compiler->shader_version.minor) >= 501; > +} Does the backend really need to know that the source was a shader model 5.1 shader? > - if (cb->register_space) > - FIXME("Unhandled register space %u.\n", cb->register_space); > + if (shader_is_sm_5_1(compiler)) > + { > + struct vkd3d_sm51_symbol *sym; > + sym = vkd3d_calloc(1, sizeof(*sym)); > + sym->key.idx = reg->idx[0].offset; > + sym->key.descriptor_type = VKD3D_SHADER_DESCRIPTOR_TYPE_CBV; > + sym->register_space = instruction->declaration.sampler.register_space; > + sym->resource_idx = instruction->declaration.sampler.register_index; > + if (rb_put(&compiler->sm51_resource_table, &sym->key, &sym->entry) == -1) > + vkd3d_free(sym); > + } > @@ -5294,8 +5438,18 @@ static void vkd3d_dxbc_compiler_emit_dcl_resource_structured(struct vkd3d_dxbc_c > const struct vkd3d_shader_register *reg = &resource->reg.reg; > unsigned int stride = resource->byte_stride; > > - if (resource->register_space) > - FIXME("Unhandled register space %u.\n", resource->register_space); > + if (shader_is_sm_5_1(compiler)) > + { > + struct vkd3d_sm51_symbol *sym; > + sym = vkd3d_calloc(1, sizeof(*sym)); > + sym->key.idx = resource->reg.reg.idx[0].offset; > + sym->key.descriptor_type = resource->reg.reg.type == VKD3DSPR_UAV ? VKD3D_SHADER_DESCRIPTOR_TYPE_UAV : VKD3D_SHADER_DESCRIPTOR_TYPE_SRV; > + sym->register_space = resource->register_space; > + sym->resource_idx = resource->register_index; > + if (rb_put(&compiler->sm51_resource_table, &sym->key, &sym->entry) == -1) > + vkd3d_free(sym); > + } > + This would probably benefit from a helper function, but I also imagine it could simply add the register space information to the existing vkd3d_symbol.