google / amber

Amber is a multi-API shader test framework
Apache License 2.0
188 stars 63 forks source link

Vulkan CTS and --deqp-compute-only flag #995

Closed pingpongcat closed 1 year ago

pingpongcat commented 1 year ago

Hello and welcome! Im working on delivering additional deqp-vk execution flag --deqp-compute-only=enable which will test Vulkan implementation on devices which expose only computation queues.

https://gerrit.khronos.org/c/vk-gl-cts/+/8851 (link i accessible only by registered Khronos members)

Current design works in 99,9% cases but one of our reviewers noticed that some of spirv tests that are amber dependent are running shaders through graphic pipeline which should be blocked. For example:

dEQP-VK.spirv_assembly.instruction.graphics.early_fragment.depth_less dEQP-VK.spirv_assembly.instruction.graphics.float32.comparison_3.modfstruct_vert

My question is, if it's possible and if so how much work it would take to add additional parameter to amber library call for this new computation only mode?

Graphic pipeline creation would then have to be rejected and some "Not support" feedback should be given in return. (Im of course theorizing here because I don't know amber, maybe there is a better approach which I do not see)

For us it's a blocker for this feature to arrive in CTS, It would be fantastic to hear opinion of someone experienced in amber.

Thank you in advance for all your help Marcin Zając Vulkan CTS developer

dj2 commented 1 year ago

Could this be done at the dEQP level and just not execute those Amber scripts when running with your new mode? The tests themselves are setup as graphics pipeline tests, so it seems weird to even try to run them?

dneto0 commented 1 year ago

I understand that the Vulkan CTS has a design principle that the CTS framework has to know whether it can run a test before it gets too deep into looking at the test.

See https://github.com/KhronosGroup/VK-GL-CTS/blob/main/external/vulkancts/modules/vulkan/amber/vktAmberTestCase.hpp#L77

// Check that the Vulkan implementation supports this test.
// We have the principle that client code in dEQP should independently
// determine if the test should be supported:
//  - If any of the extensions registered via |addRequirement| is not
//    supported then throw a NotSupported exception.
//  - Otherwise, we do a secondary sanity check depending on code inside
//    Amber itself: if the Amber test says it is not supported, then
//    throw an internal error exception.
// A function pointer for a custom checkSupport function can also be
// provided for a more sophisticated support check.
void checkSupport (Context& ctx) const override;

If I recall correctly, the checkSupport method is called before the Amber script file is parsed.

However, there is a later step, "validateRequirements" which could be useful. It parses the Amber script, and produces a "recipe". The recipe is the internal representation of the Amber script file. At that point we know if there are compute pipelines, or render pipelines or both. https://github.com/KhronosGroup/VK-GL-CTS/blob/main/external/vulkancts/modules/vulkan/amber/vktAmberTestCase.cpp#L476

But it looks like the "fail" case of validateRequirements is treated as an error rather than "don't run me"

dneto0 commented 1 year ago

But it looks like the "fail" case of validateRequirements is treated as an error rather than "don't run me"

Oh, looks like that's only tried in a verify coherency mode. See https://github.com/KhronosGroup/VK-GL-CTS/blob/main/framework/common/tcuApp.cpp#L78

dneto0 commented 1 year ago

Vulkan test examination occurs here, in vkt::TestCaseExecutor::init

https://github.com/KhronosGroup/VK-GL-CTS/blob/main/external/vulkancts/modules/vulkan/vktTestPackage.cpp#L417

vktCase->checkSupport(*m_context);   

vktCase->delayedInit();

m_progCollection.clear();
vktCase->initPrograms(sourceProgs);

The constraint is that checkSupport is a const method, and to say a test should not be run, it throws a NotSupportedError exception. The delayedInit call is non-constant, and that's where the Amber script is actually parsed. It throws an internal error if the script can't be parsed.

But, it too could throw a NotSupportedError, if it was given enough information about what pipelines are available. So I suggest:

  1. Add info to the vkt::Context object to say what kinds of pipelines can be executed.
  2. pass the context object into delayed Init
  3. teach delayedInit to throw NotSupportedError when the necessary pipelines are not available.

If there is some constraint where NotSupportedError can only be run from const object, then have delayedInit return true if a delayed check support should be run:

   vktCase->checkSupport(*m_context);
   if (vktCase->delayedInit()) {
         vktCase->delayedCheckSupport(*m_context); // throw NotSupportedError here if needed.
   }
dneto0 commented 1 year ago

FYI: When the amber script is parsed, it produces a recipe. The recipe object has a vector of amber::ShaderInfo, and each knows what kind of shader it is. So it's short plumbing exercise to see what shader stages are exercised by the amber script.

So I think there is no work to be done in Amber's code base. Instead, it's a reasonable set of changes to vktAmberTestCase.* and vktTestPackage

dneto0 commented 1 year ago

I think there is no work to be done in Amber itself. Closing