[PATCH vkd3d 1/2] tests: Fix shader tests crashes on WARP driver.
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- tests/d3d12_test_utils.h | 3 +++ tests/shader_runner_d3d12.c | 10 +++++----- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/d3d12_test_utils.h b/tests/d3d12_test_utils.h index 5d900f8..1f54e10 100644 --- a/tests/d3d12_test_utils.h +++ b/tests/d3d12_test_utils.h @@ -956,6 +956,7 @@ struct test_context ID3D12CommandQueue *queue; ID3D12CommandAllocator *allocator; ID3D12GraphicsCommandList *list; + ID3D12PipelineState *pso; D3D12_RESOURCE_DESC render_target_desc; ID3D12Resource *render_target; @@ -1092,6 +1093,8 @@ static inline void destroy_test_context_(unsigned int line, struct test_context ID3D12CommandAllocator_Release(context->allocator); ID3D12CommandQueue_Release(context->queue); ID3D12GraphicsCommandList_Release(context->list); + if (context->pso) + ID3D12PipelineState_Release(context->pso); refcount = ID3D12Device_Release(context->device); ok_(line)(!refcount, "ID3D12Device has %u references left.\n", (unsigned int)refcount); diff --git a/tests/shader_runner_d3d12.c b/tests/shader_runner_d3d12.c index d404895..283a264 100644 --- a/tests/shader_runner_d3d12.c +++ b/tests/shader_runner_d3d12.c @@ -341,7 +341,6 @@ static void parse_test_directive(struct shader_context *context, const char *lin D3D12_STATIC_SAMPLER_DESC static_samplers[1]; static const float clear_color[4]; unsigned int uniform_index; - ID3D12PipelineState *pso; HRESULT hr; size_t i; @@ -419,9 +418,11 @@ static void parse_test_directive(struct shader_context *context, const char *lin hr = create_root_signature(context->c.device, &root_signature_desc, &context->c.root_signature); ok(hr == S_OK, "Failed to create root signature, hr %#x.\n", hr); - pso = create_pipeline_state(context->c.device, context->c.root_signature, + if (context->c.pso) + ID3D12PipelineState_Release(context->c.pso); + context->c.pso = create_pipeline_state(context->c.device, context->c.root_signature, context->c.render_target_desc.Format, NULL, &ps, NULL); - if (!pso) + if (!context->c.pso) return; ID3D12GraphicsCommandList_SetGraphicsRootSignature(command_list, context->c.root_signature); @@ -437,9 +438,8 @@ static void parse_test_directive(struct shader_context *context, const char *lin ID3D12GraphicsCommandList_RSSetViewports(command_list, 1, &context->c.viewport); ID3D12GraphicsCommandList_IASetPrimitiveTopology(command_list, D3D_PRIMITIVE_TOPOLOGY_TRIANGLELIST); ID3D12GraphicsCommandList_ClearRenderTargetView(command_list, context->c.rtv, clear_color, 0, NULL); - ID3D12GraphicsCommandList_SetPipelineState(command_list, pso); + ID3D12GraphicsCommandList_SetPipelineState(command_list, context->c.pso); ID3D12GraphicsCommandList_DrawInstanced(command_list, 3, 1, 0, 0); - ID3D12PipelineState_Release(pso); transition_resource_state(command_list, context->c.render_target, D3D12_RESOURCE_STATE_RENDER_TARGET, D3D12_RESOURCE_STATE_COPY_SOURCE); } -- 2.33.0
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- Makefile.am | 1 + libs/vkd3d-shader/hlsl.h | 1 + libs/vkd3d-shader/hlsl.y | 7 +++++++ libs/vkd3d-shader/hlsl_sm4.c | 4 ++++ tests/floor.shader_test | 26 ++++++++++++++++++++++++++ 5 files changed, 39 insertions(+) create mode 100644 tests/floor.shader_test diff --git a/Makefile.am b/Makefile.am index be3d8ec..a4d188b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -60,6 +60,7 @@ vkd3d_shader_tests = \ tests/cast-to-int.shader_test \ tests/cast-to-uint.shader_test \ tests/conditional.shader_test \ + tests/floor.shader_test \ tests/hlsl-array-dimension.shader_test \ tests/hlsl-bool-cast.shader_test \ tests/hlsl-clamp.shader_test \ diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index e7bdb45..636d27c 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -293,6 +293,7 @@ enum hlsl_ir_expr_op HLSL_OP1_DSX, HLSL_OP1_DSY, HLSL_OP1_EXP2, + HLSL_OP1_FLOOR, HLSL_OP1_FRACT, HLSL_OP1_LOG2, HLSL_OP1_LOGIC_NOT, diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index ff35c09..4ef8d79 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1646,6 +1646,12 @@ static bool intrinsic_max(struct hlsl_ctx *ctx, return !!add_binary_arithmetic_expr(ctx, params->instrs, HLSL_OP2_MAX, params->args[0], params->args[1], &loc); } +static bool intrinsic_floor(struct hlsl_ctx *ctx, + const struct parse_initializer *params, struct vkd3d_shader_location loc) +{ + return !!add_unary_arithmetic_expr(ctx, params->instrs, HLSL_OP1_FLOOR, params->args[0], &loc); +} + static bool intrinsic_pow(struct hlsl_ctx *ctx, const struct parse_initializer *params, struct vkd3d_shader_location loc) { @@ -1690,6 +1696,7 @@ intrinsic_functions[] = {"abs", 1, true, intrinsic_abs}, {"clamp", 3, true, intrinsic_clamp}, {"cross", 2, true, intrinsic_cross}, + {"floor", 1, true, intrinsic_floor}, {"max", 2, true, intrinsic_max}, {"pow", 2, true, intrinsic_pow}, {"round", 1, true, intrinsic_round}, diff --git a/libs/vkd3d-shader/hlsl_sm4.c b/libs/vkd3d-shader/hlsl_sm4.c index c0c26f8..6f07f97 100644 --- a/libs/vkd3d-shader/hlsl_sm4.c +++ b/libs/vkd3d-shader/hlsl_sm4.c @@ -1297,6 +1297,10 @@ static void write_sm4_expr(struct hlsl_ctx *ctx, write_sm4_unary_op(buffer, VKD3D_SM4_OP_EXP, &expr->node, arg1, 0); break; + case HLSL_OP1_FLOOR: + write_sm4_unary_op(buffer, VKD3D_SM4_OP_ROUND_NI, &expr->node, arg1, 0); + break; + case HLSL_OP1_LOG2: write_sm4_unary_op(buffer, VKD3D_SM4_OP_LOG, &expr->node, arg1, 0); break; diff --git a/tests/floor.shader_test b/tests/floor.shader_test new file mode 100644 index 0000000..0593eaf --- /dev/null +++ b/tests/floor.shader_test @@ -0,0 +1,26 @@ +[pixel shader] +float4 main(uniform float4 u) : sv_target +{ + return floor(u); +} + +[test] +uniform 0 float4 -0.5 6.5 7.5 3.4 +draw quad +probe all rgba (-1.0, 6.0, 7.0, 3.0) 4 + + + +[pixel shader] +float4 main(uniform float4 u) : sv_target +{ + float a = floor(u.r); + int2 b = floor(u.gb); + float4 res = float4(b, a, u.a); + return floor(res); +} + +[test] +uniform 0 float4 -0.5 6.5 7.5 3.4 +draw quad +probe all rgba (6.0, 7.0, -1.0, 3.0) 4 -- 2.33.0
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- For the record, add_unary_arithmetic_expr is not completely correct here, because, for example, it produces an int4 output type when input is int4, while native would convert to float4. However, integer floor is not implemented anyway, and other intrinsics are broken in the same way, so there's not reason to delay this patch. Eventually I think it would make sense to have another helper called something like "add_unary_float_arithmetic_expr" that always converts its input to half4 or float4. On 11/12/21 20:44, Nikolay Sivov wrote:
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- Makefile.am | 1 + libs/vkd3d-shader/hlsl.h | 1 + libs/vkd3d-shader/hlsl.y | 7 +++++++ libs/vkd3d-shader/hlsl_sm4.c | 4 ++++ tests/floor.shader_test | 26 ++++++++++++++++++++++++++ 5 files changed, 39 insertions(+) create mode 100644 tests/floor.shader_test
diff --git a/Makefile.am b/Makefile.am index be3d8ec..a4d188b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -60,6 +60,7 @@ vkd3d_shader_tests = \ tests/cast-to-int.shader_test \ tests/cast-to-uint.shader_test \ tests/conditional.shader_test \ + tests/floor.shader_test \ tests/hlsl-array-dimension.shader_test \ tests/hlsl-bool-cast.shader_test \ tests/hlsl-clamp.shader_test \ diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index e7bdb45..636d27c 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -293,6 +293,7 @@ enum hlsl_ir_expr_op HLSL_OP1_DSX, HLSL_OP1_DSY, HLSL_OP1_EXP2, + HLSL_OP1_FLOOR, HLSL_OP1_FRACT, HLSL_OP1_LOG2, HLSL_OP1_LOGIC_NOT, diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index ff35c09..4ef8d79 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1646,6 +1646,12 @@ static bool intrinsic_max(struct hlsl_ctx *ctx, return !!add_binary_arithmetic_expr(ctx, params->instrs, HLSL_OP2_MAX, params->args[0], params->args[1], &loc); }
+static bool intrinsic_floor(struct hlsl_ctx *ctx, + const struct parse_initializer *params, struct vkd3d_shader_location loc) +{ + return !!add_unary_arithmetic_expr(ctx, params->instrs, HLSL_OP1_FLOOR, params->args[0], &loc); +} + static bool intrinsic_pow(struct hlsl_ctx *ctx, const struct parse_initializer *params, struct vkd3d_shader_location loc) { @@ -1690,6 +1696,7 @@ intrinsic_functions[] = {"abs", 1, true, intrinsic_abs}, {"clamp", 3, true, intrinsic_clamp}, {"cross", 2, true, intrinsic_cross}, + {"floor", 1, true, intrinsic_floor}, {"max", 2, true, intrinsic_max}, {"pow", 2, true, intrinsic_pow}, {"round", 1, true, intrinsic_round}, diff --git a/libs/vkd3d-shader/hlsl_sm4.c b/libs/vkd3d-shader/hlsl_sm4.c index c0c26f8..6f07f97 100644 --- a/libs/vkd3d-shader/hlsl_sm4.c +++ b/libs/vkd3d-shader/hlsl_sm4.c @@ -1297,6 +1297,10 @@ static void write_sm4_expr(struct hlsl_ctx *ctx, write_sm4_unary_op(buffer, VKD3D_SM4_OP_EXP, &expr->node, arg1, 0); break;
+ case HLSL_OP1_FLOOR: + write_sm4_unary_op(buffer, VKD3D_SM4_OP_ROUND_NI, &expr->node, arg1, 0); + break; + case HLSL_OP1_LOG2: write_sm4_unary_op(buffer, VKD3D_SM4_OP_LOG, &expr->node, arg1, 0); break; diff --git a/tests/floor.shader_test b/tests/floor.shader_test new file mode 100644 index 0000000..0593eaf --- /dev/null +++ b/tests/floor.shader_test @@ -0,0 +1,26 @@ +[pixel shader] +float4 main(uniform float4 u) : sv_target +{ + return floor(u); +} + +[test] +uniform 0 float4 -0.5 6.5 7.5 3.4 +draw quad +probe all rgba (-1.0, 6.0, 7.0, 3.0) 4 + + + +[pixel shader] +float4 main(uniform float4 u) : sv_target +{ + float a = floor(u.r); + int2 b = floor(u.gb); + float4 res = float4(b, a, u.a); + return floor(res); +} + +[test] +uniform 0 float4 -0.5 6.5 7.5 3.4 +draw quad +probe all rgba (6.0, 7.0, -1.0, 3.0) 4
On 12/13/21 12:03, Giovanni Mascellani wrote:
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- For the record, add_unary_arithmetic_expr is not completely correct here, because, for example, it produces an int4 output type when input is int4, while native would convert to float4.
However, integer floor is not implemented anyway, and other intrinsics are broken in the same way, so there's not reason to delay this patch.
Eventually I think it would make sense to have another helper called something like "add_unary_float_arithmetic_expr" that always converts its input to half4 or float4.
I don't think it needs to convert anything. For integer input it simply should returns it as a result, for floor()/round()/trunc()/ceil(). After that normal cast should happen, resulting in ftoi/itof. Or maybe for integer case it's enough to return cast node to arg type. By the way, is it possible to get textual output right away, without going through dxbc -> disasm?
Hi, On 13/12/21 10:56, Nikolay Sivov wrote:
For the record, add_unary_arithmetic_expr is not completely correct here, because, for example, it produces an int4 output type when input is int4, while native would convert to float4.
However, integer floor is not implemented anyway, and other intrinsics are broken in the same way, so there's not reason to delay this patch.
Eventually I think it would make sense to have another helper called something like "add_unary_float_arithmetic_expr" that always converts its input to half4 or float4.
I don't think it needs to convert anything. For integer input it simply should returns it as a result, for floor()/round()/trunc()/ceil(). After that normal cast should happen, resulting in ftoi/itof. Or maybe for integer case it's enough to return cast node to arg type.
As far as I can see it really returns a float4 for an int4 input. This matters, for example, if function overloading is done on the output (and possibly, in edge cases, for very big integers).
By the way, is it possible to get textual output right away, without going through dxbc -> disasm?
What I usually do is enable logging and run some .shader_test that compiles and run that code. It will dump to the log both the IR and the compiled code. Of course is still compilation and disassembling, but you don't have to do it by hand. OTOH, I think you're not the first one to ask for that thing (hlsl -> d3d_asm directly in vkd3d-compiler), so it might eventually happen. Giovanni.
On 12/13/21 13:09, Giovanni Mascellani wrote:
Hi,
On 13/12/21 10:56, Nikolay Sivov wrote:
For the record, add_unary_arithmetic_expr is not completely correct here, because, for example, it produces an int4 output type when input is int4, while native would convert to float4.
However, integer floor is not implemented anyway, and other intrinsics are broken in the same way, so there's not reason to delay this patch.
Eventually I think it would make sense to have another helper called something like "add_unary_float_arithmetic_expr" that always converts its input to half4 or float4.
I don't think it needs to convert anything. For integer input it simply should returns it as a result, for floor()/round()/trunc()/ceil(). After that normal cast should happen, resulting in ftoi/itof. Or maybe for integer case it's enough to return cast node to arg type.
As far as I can see it really returns a float4 for an int4 input. This matters, for example, if function overloading is done on the output (and possibly, in edge cases, for very big integers). How do you check that?
By the way, is it possible to get textual output right away, without going through dxbc -> disasm?
What I usually do is enable logging and run some .shader_test that compiles and run that code. It will dump to the log both the IR and the compiled code. Of course is still compilation and disassembling, but you don't have to do it by hand.
OTOH, I think you're not the first one to ask for that thing (hlsl -> d3d_asm directly in vkd3d-compiler), so it might eventually happen.
Giovanni.
Hi, On 13/12/21 11:12, Nikolay Sivov wrote:
How do you check that?
https://shader-playground.timjones.io/5cb163a31149f937714c6faf940b027f --- int f(int4 x) { return 0; } int f(float4 x) { return 1; } float4 PSMain() : SV_TARGET { return f(floor(int4(1, 2, 3, 4))); } --- This will return 1. Giovanni.
On 12/13/21 13:09, Giovanni Mascellani wrote:
Hi,
On 13/12/21 10:56, Nikolay Sivov wrote:
For the record, add_unary_arithmetic_expr is not completely correct here, because, for example, it produces an int4 output type when input is int4, while native would convert to float4.
However, integer floor is not implemented anyway, and other intrinsics are broken in the same way, so there's not reason to delay this patch.
Eventually I think it would make sense to have another helper called something like "add_unary_float_arithmetic_expr" that always converts its input to half4 or float4.
I don't think it needs to convert anything. For integer input it simply should returns it as a result, for floor()/round()/trunc()/ceil(). After that normal cast should happen, resulting in ftoi/itof. Or maybe for integer case it's enough to return cast node to arg type.
As far as I can see it really returns a float4 for an int4 input. This matters, for example, if function overloading is done on the output (and possibly, in edge cases, for very big integers).
Something like attached diff?
By the way, is it possible to get textual output right away, without going through dxbc -> disasm?
What I usually do is enable logging and run some .shader_test that compiles and run that code. It will dump to the log both the IR and the compiled code. Of course is still compilation and disassembling, but you don't have to do it by hand.
OTOH, I think you're not the first one to ask for that thing (hlsl -> d3d_asm directly in vkd3d-compiler), so it might eventually happen.
Giovanni.
Hi, On 13/12/21 12:15, Nikolay Sivov wrote:
Something like attached diff?
That looks sensible, except that it should be:
if (type->base_type != HLSL_TYPE_FLOAT && type->base_type != HLSL_TYPE_HALF)
We don't really handle half values so far (and it's not clear how they differ from floats), but at least for typing I guess it makes sense to do the right thing, given that it's easy. Thanks, Giovanni.
On 12/13/21 14:23, Giovanni Mascellani wrote:
Hi,
On 13/12/21 12:15, Nikolay Sivov wrote:
Something like attached diff?
That looks sensible, except that it should be:
if (type->base_type != HLSL_TYPE_FLOAT && type->base_type != HLSL_TYPE_HALF)
We don't really handle half values so far (and it's not clear how they differ from floats), but at least for typing I guess it makes sense to do the right thing, given that it's easy.
The way you tested with overloaded functions, half is handled the same as int.
Thanks, Giovanni.
Hi, On 13/12/21 12:32, Nikolay Sivov wrote:
The way you tested with overloaded functions, half is handled the same as int.
My test indicates the contrary: https://shader-playground.timjones.io/6666eefc39a6d91f3f5fab55f78995c4 What program did you test? Giovanni.
On 12/13/21 14:45, Giovanni Mascellani wrote:
Hi,
On 13/12/21 12:32, Nikolay Sivov wrote:
The way you tested with overloaded functions, half is handled the same as int.
My test indicates the contrary:
https://shader-playground.timjones.io/6666eefc39a6d91f3f5fab55f78995c4
What program did you test?
Yeah, you're right, I messed up something.
Giovanni.
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> On 11/12/21 20:44, Nikolay Sivov wrote:
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- tests/d3d12_test_utils.h | 3 +++ tests/shader_runner_d3d12.c | 10 +++++----- 2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/tests/d3d12_test_utils.h b/tests/d3d12_test_utils.h index 5d900f8..1f54e10 100644 --- a/tests/d3d12_test_utils.h +++ b/tests/d3d12_test_utils.h @@ -956,6 +956,7 @@ struct test_context ID3D12CommandQueue *queue; ID3D12CommandAllocator *allocator; ID3D12GraphicsCommandList *list; + ID3D12PipelineState *pso;
D3D12_RESOURCE_DESC render_target_desc; ID3D12Resource *render_target; @@ -1092,6 +1093,8 @@ static inline void destroy_test_context_(unsigned int line, struct test_context ID3D12CommandAllocator_Release(context->allocator); ID3D12CommandQueue_Release(context->queue); ID3D12GraphicsCommandList_Release(context->list); + if (context->pso) + ID3D12PipelineState_Release(context->pso);
refcount = ID3D12Device_Release(context->device); ok_(line)(!refcount, "ID3D12Device has %u references left.\n", (unsigned int)refcount); diff --git a/tests/shader_runner_d3d12.c b/tests/shader_runner_d3d12.c index d404895..283a264 100644 --- a/tests/shader_runner_d3d12.c +++ b/tests/shader_runner_d3d12.c @@ -341,7 +341,6 @@ static void parse_test_directive(struct shader_context *context, const char *lin D3D12_STATIC_SAMPLER_DESC static_samplers[1]; static const float clear_color[4]; unsigned int uniform_index; - ID3D12PipelineState *pso; HRESULT hr; size_t i;
@@ -419,9 +418,11 @@ static void parse_test_directive(struct shader_context *context, const char *lin hr = create_root_signature(context->c.device, &root_signature_desc, &context->c.root_signature); ok(hr == S_OK, "Failed to create root signature, hr %#x.\n", hr);
- pso = create_pipeline_state(context->c.device, context->c.root_signature, + if (context->c.pso) + ID3D12PipelineState_Release(context->c.pso); + context->c.pso = create_pipeline_state(context->c.device, context->c.root_signature, context->c.render_target_desc.Format, NULL, &ps, NULL); - if (!pso) + if (!context->c.pso) return;
ID3D12GraphicsCommandList_SetGraphicsRootSignature(command_list, context->c.root_signature); @@ -437,9 +438,8 @@ static void parse_test_directive(struct shader_context *context, const char *lin ID3D12GraphicsCommandList_RSSetViewports(command_list, 1, &context->c.viewport); ID3D12GraphicsCommandList_IASetPrimitiveTopology(command_list, D3D_PRIMITIVE_TOPOLOGY_TRIANGLELIST); ID3D12GraphicsCommandList_ClearRenderTargetView(command_list, context->c.rtv, clear_color, 0, NULL); - ID3D12GraphicsCommandList_SetPipelineState(command_list, pso); + ID3D12GraphicsCommandList_SetPipelineState(command_list, context->c.pso); ID3D12GraphicsCommandList_DrawInstanced(command_list, 3, 1, 0, 0); - ID3D12PipelineState_Release(pso); transition_resource_state(command_list, context->c.render_target, D3D12_RESOURCE_STATE_RENDER_TARGET, D3D12_RESOURCE_STATE_COPY_SOURCE); }
On Sat, 11 Dec 2021 at 20:44, Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
@@ -419,9 +418,11 @@ static void parse_test_directive(struct shader_context *context, const char *lin hr = create_root_signature(context->c.device, &root_signature_desc, &context->c.root_signature); ok(hr == S_OK, "Failed to create root signature, hr %#x.\n", hr);
- pso = create_pipeline_state(context->c.device, context->c.root_signature, + if (context->c.pso) + ID3D12PipelineState_Release(context->c.pso); + context->c.pso = create_pipeline_state(context->c.device, context->c.root_signature, context->c.render_target_desc.Format, NULL, &ps, NULL); - if (!pso) + if (!context->c.pso) return;
I suppose that's an improvement in the sense that it should work in practice for most of our existing tests, but it doesn't strike me as entirely correct either. In particular, the restriction is that we can't destroy the state object until all command lists referencing it have finished executing on the GPU. That mostly works out in practice because the typical pattern is to do a "draw", followed by a "probe", in which case the "probe" will wait for the command list to finish executing, but I could certainly imagine writing tests where we do multiple draws before probing.
On 12/15/21 20:57, Henri Verbeet wrote:
On Sat, 11 Dec 2021 at 20:44, Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
@@ -419,9 +418,11 @@ static void parse_test_directive(struct shader_context *context, const char *lin hr = create_root_signature(context->c.device, &root_signature_desc, &context->c.root_signature); ok(hr == S_OK, "Failed to create root signature, hr %#x.\n", hr);
- pso = create_pipeline_state(context->c.device, context->c.root_signature, + if (context->c.pso) + ID3D12PipelineState_Release(context->c.pso); + context->c.pso = create_pipeline_state(context->c.device, context->c.root_signature, context->c.render_target_desc.Format, NULL, &ps, NULL); - if (!pso) + if (!context->c.pso) return;
I suppose that's an improvement in the sense that it should work in practice for most of our existing tests, but it doesn't strike me as entirely correct either. In particular, the restriction is that we can't destroy the state object until all command lists referencing it have finished executing on the GPU. That mostly works out in practice because the typical pattern is to do a "draw", followed by a "probe", in which case the "probe" will wait for the command list to finish executing, but I could certainly imagine writing tests where we do multiple draws before probing. Would it be correct in terms of test results to flush current command list, on repeated "draw"s? I see there is ::Reset() that according to docs will reinitialize recording, if last Close() succeeded. Test could use "pso != NULL" as a marker that "probe" wasn't used since last draw, and close-exec-wait-reset right before starting new recording.
Another brute force option is to simply allow for arbitrary number of state objects in test context, accumulated indefinitely until next "probe".
On Thu, 16 Dec 2021 at 21:12, Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
On 12/15/21 20:57, Henri Verbeet wrote:
On Sat, 11 Dec 2021 at 20:44, Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
@@ -419,9 +418,11 @@ static void parse_test_directive(struct shader_context *context, const char *lin hr = create_root_signature(context->c.device, &root_signature_desc, &context->c.root_signature); ok(hr == S_OK, "Failed to create root signature, hr %#x.\n", hr);
- pso = create_pipeline_state(context->c.device, context->c.root_signature, + if (context->c.pso) + ID3D12PipelineState_Release(context->c.pso); + context->c.pso = create_pipeline_state(context->c.device, context->c.root_signature, context->c.render_target_desc.Format, NULL, &ps, NULL); - if (!pso) + if (!context->c.pso) return;
I suppose that's an improvement in the sense that it should work in practice for most of our existing tests, but it doesn't strike me as entirely correct either. In particular, the restriction is that we can't destroy the state object until all command lists referencing it have finished executing on the GPU. That mostly works out in practice because the typical pattern is to do a "draw", followed by a "probe", in which case the "probe" will wait for the command list to finish executing, but I could certainly imagine writing tests where we do multiple draws before probing. Would it be correct in terms of test results to flush current command list, on repeated "draw"s? I see there is ::Reset() that according to docs will reinitialize recording, if last Close() succeeded. Test could use "pso != NULL" as a marker that "probe" wasn't used since last draw, and close-exec-wait-reset right before starting new recording.
Another brute force option is to simply allow for arbitrary number of state objects in test context, accumulated indefinitely until next "probe".
Right, either of those approaches should work. The latter would likely be a bit more efficient, although I don't think we're overly concerned about the performance of the tests (yet). In case of the first approach, note how get_texture_readback_with_command_list() (used by the "probe" implementation) does this: - Close the existing command list with ID3D12GraphicsCommandList_Close(). - Execute the command list with exec_command_list(). - Wait for the queue to become idle with wait_queue_idle(). We then call reset_command_list() to restart recording.
participants (3)
-
Giovanni Mascellani -
Henri Verbeet -
Nikolay Sivov