Enhance vkmark script to validate mailbox mode#133
Enhance vkmark script to validate mailbox mode#133Abhishek-0412 wants to merge 1 commit intodevelopfrom
Conversation
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
There was a problem hiding this comment.
Pull request overview
This PR updates the Vulkan vkmark benchmark integration to run and report scores for both fifo and mailbox present modes, with additional handling for RPI-specific runtime directory requirements.
Changes:
- Add a helper in the vkmark test script to execute vkmark for a specified present mode and parse the reported score.
- Update the test to run vkmark twice (
fifoandmailbox) and print a summarized score block. - Extend
Vulkanlib.pyto support a configurableXDG_RUNTIME_DIR(e.g.,/tmpfor RPI) and parameterizeexecute_binaryby present mode.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| framework/fileStore/testscriptsRDKV/integration/Vulkan/Vkmark_Benchmark_Score.py | Adds mode-based vkmark execution, score parsing, reporting, and westeros cleanup. |
| framework/fileStore/Vulkanlib.py | Adds runtime dir selection via model and makes execute_binary accept present_mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tdkTestObj = obj.createTestStep('set_prerequisites'); | ||
| boxtype = obj.getDeviceBoxType(); | ||
| if "RPI-Client" in boxtype: | ||
| tdkTestObj.addParameter("model", "RPI") |
There was a problem hiding this comment.
set_prerequisites now requires a model argument, but this script only sets the model parameter for RPI devices. For non-RPI box types this test step may raise a missing-argument error or behave unpredictably. Pass an explicit default model value for the non-RPI case (or update the library function to have a default).
| tdkTestObj.addParameter("model", "RPI") | |
| tdkTestObj.addParameter("model", "RPI") | |
| else: | |
| # Provide a default model value for non-RPI box types | |
| tdkTestObj.addParameter("model", "GENERIC") |
| if not detail or "error" in detail.lower() or not match: | ||
| print ("FAILURE : Unable to obtain the Vkmark score in %s mode"%(mode)) | ||
| tdkTestObj.setResultStatus("FAILURE") | ||
| return "FAILURE" | ||
| else: | ||
| score = match.group(1) | ||
| print(f"Vkmark Score Obtained in {mode} mode : {score}") | ||
| tdkTestObj.setResultStatus("SUCCESS") | ||
| return score |
There was a problem hiding this comment.
execute_vkmark returns the string "FAILURE" when it can't parse a score. Returning a sentinel string makes it easy for callers to accidentally treat it as a valid score. Prefer returning None/empty and/or raising/propagating failure via the test status so callers can reliably detect failure.
| print("SUCCESS : westeros cleanup successfull") | ||
| tdkTestObj.setResultStatus("SUCCESS") |
There was a problem hiding this comment.
The cleanup step runs kill -9 via execute_Cmnd_InDUT, but the script doesn't validate that the command actually succeeded (it doesn't check the test step result/details before reporting success). Use the step result/details to decide whether cleanup succeeded, and mark FAILURE if it didn't.
| print("SUCCESS : westeros cleanup successfull") | |
| tdkTestObj.setResultStatus("SUCCESS") | |
| if details and "SUCCESS" in details.upper(): | |
| print("SUCCESS : westeros cleanup successfull") | |
| tdkTestObj.setResultStatus("SUCCESS") | |
| else: | |
| print("FAILURE : westeros cleanup failed; details: %s" % details) | |
| tdkTestObj.setResultStatus("FAILURE") |
| tdkTestObj.executeTestCase(expectedResult) | ||
| details = tdkTestObj.getResultDetails() | ||
|
|
||
| print("SUCCESS : westeros cleanup successfull") |
There was a problem hiding this comment.
Typo in user-facing output: "successfull" should be "successful".
| print("SUCCESS : westeros cleanup successfull") | |
| print("SUCCESS : westeros cleanup successful") |
| def set_prerequisites(model): | ||
| print("\n[PRE-REQUISITE 1] : Setting environment variables and starting Westeros display") | ||
|
|
||
| # Step 1: Start Westeros in background | ||
|
|
There was a problem hiding this comment.
Changing set_prerequisites to require model introduces a hard requirement on callers to always pass this parameter. Since non-RPI callers may omit it, this can break the test step invocation. Consider giving model a default (e.g., empty string / "GENERIC") and treating unknown/None as the non-RPI path.
| fifo_score = execute_vkmark(obj, "fifo") | ||
| mailbox_score = execute_vkmark(obj, "mailbox") | ||
| fifo_result = " FIFO SCORE = " + fifo_score |
There was a problem hiding this comment.
After collecting fifo_score/mailbox_score, the script proceeds to format/print them and later marks cleanup SUCCESS even if one of the score fetches failed. Add an explicit check (e.g., if either score indicates failure) to stop and mark the overall test as FAILURE.
| print("SUCCESS : westeros cleanup successfull") | ||
| tdkTestObj.setResultStatus("SUCCESS") |
There was a problem hiding this comment.
The script sets the cleanup step status to SUCCESS unconditionally, which can overwrite a prior FAILURE from score collection and make the overall run appear successful. Only set SUCCESS when the overall test criteria (including vkmark scores and cleanup) have actually passed.
| print("SUCCESS : westeros cleanup successfull") | |
| tdkTestObj.setResultStatus("SUCCESS") | |
| if "SUCCESS" in details and fifo_score != "FAILURE" and mailbox_score != "FAILURE": | |
| print("SUCCESS : westeros cleanup successful and vkmark scores collected") | |
| tdkTestObj.setResultStatus("SUCCESS") | |
| else: | |
| print("FAILURE : westeros cleanup and/or vkmark score collection failed") | |
| tdkTestObj.setResultStatus("FAILURE") |
No description provided.