Conversation
Srikanth Muppandam (smuppand)
left a comment
Srikanth Muppandam (smuppand)
left a comment
There was a problem hiding this comment.
Recommended scalable design
functestlib.sh
platform_runtime_identity()
platform_identity_matches()
lib_video.sh
video_policy_lookup(platform, mode, codec)
video_stage_control_override(config, control, value, output_dir)
run.sh
platform=$(...)
policy=$(...)
effective_config=$(...)
run repeats and retries with effective_config
With this design:
- Adding a new SoC means adding policy data.
- Platform detection remains shared across audio and video.
- JSON staging has one implementation.
- Normal runs and retries cannot diverge.
- No-policy platforms use the original config unchanged.
| # if no device-tree/platform data is available (callers should treat that | ||
| # as "no match" rather than falling back to a caller-name heuristic). | ||
| # ----------------------------------------------------------------------------- | ||
| video_get_dt_identity() { |
There was a problem hiding this comment.
video_get_dt_identity() duplicates both detect_platform() and the Shikra runtime identity logic merged in PR #544 as audio_platform_is_shikra(). A future SoC alias or DT-root correction would need to be updated independently in functestlib, audio, and video. Extract a generic runtime platform identity/match helper into functestlib.sh, including both DT roots, and make audio and video consume that shared contract.
| # video_target_is_shikra -> 0/1 | ||
| # video_apply_level_override_for_target <cfg_json_path> | ||
|
|
||
| : "${VIDEO_LEVEL_OVERRIDE:=4.0}" |
There was a problem hiding this comment.
The policy model supports only one global target/value pair:
VIDEO_LEVEL_OVERRIDE=4.0
VIDEO_LEVEL_OVERRIDE_TARGET=shikra
It cannot express cases such as Shikra H.264=4.0, Shikra HEVC=4.1, and a future SoC HEVC=5.1 without adding branches or changing global state. Replace this with a policy lookup keyed by platform, mode, codec, control, and value. Adding another SoC should require one policy row, with no run.sh changes.
For example:
shikra|encode|h264|Level|4.0
shikra|encode|hevc|Level|4.0
new-soc|encode|hevc|Level|5.1
| } | ||
|
|
||
| # Convenience wrapper for the "shikra" target specifically. | ||
| video_target_is_shikra() { |
There was a problem hiding this comment.
video_target_is_shikra() has no callers and establishes a pattern that would lead to video_target_is_() helpers for every new target. Remove this wrapper. Use the shared platform identity plus the generic policy lookup.
| if video_run_once "$cfg" "$logf" "$TIMEOUT" "$SUCCESS_RE" "$LOGLEVEL"; then | ||
| run_cfg="$cfg" | ||
| if [ "$mode" = "encode" ] && { [ "$codec" = "h264" ] || [ "$codec" = "hevc" ]; }; then | ||
| if command -v video_apply_level_override_for_target >/dev/null 2>&1; then |
There was a problem hiding this comment.
Configuration preparation is duplicated in the normal path and retry path at line 1262. Every future target policy would need to remain synchronized across both branches. Resolve and stage the effective configuration once per test case, before the repeat loop, then use the same path for normal
runs and retries. Preserve helper diagnostics instead of redirecting stderr.
| BEGIN { in_level = 0 } | ||
| { | ||
| line = $0 | ||
| if (line ~ /"Id"[ \t]*:[ \t]*"Level"/) { |
There was a problem hiding this comment.
The transformation claims to be scoped to StaticControls, but it only finds "Id": "Level" and rewrites the next "Value" line. It does not track the enclosing JSON object. Make the generic staging helper structurally validate the intended object and verify that exactly one requested control changed. A malformed or ambiguous config must fail preparation rather than run with an uncertain result.
| *"$tok_l"*) | ||
| return 0 | ||
| ;; | ||
| *) |
There was a problem hiding this comment.
Remove the trailing whitespace. git diff --check currently fails here.
bbbb19e to
38ba8f4
Compare
|
Hi Srikanth Muppandam (@smuppand) |
|
|
||
| if printf '%s' "$s" | grep -Eq "$shikra_pat"; then | ||
| printf '%s\n' "shikra" | ||
| return 0 |
There was a problem hiding this comment.
Returning shikra here introduces a platform value that none of the stack helpers recognize.
With the default VIDEO_STACK=auto, stack detection returns unknown, selects upstream, and the runner exits at the hard stack gate before applying the Shikra configuration policy.
Please define Shikra’s stack contract across video_stack_status, validation, blacklist, and switching paths, or explicitly bypass legacy stack switching with an appropriate runtime readiness check.
| return 0 | ||
| } | ||
|
|
||
| # ----------------------------------------------------------------------------- |
There was a problem hiding this comment.
WK status 1 conflates an already-correct value with a target control whose object has no valid Value. The latter is documented as malformed but is returned as success, allowing Shikra to run the original unsupported level.
Please distinguish “Value found and already correct” from “target Id found without a valid Value”, and fail configuration preparation for the malformed case.
8a7da66 to
c9ae0a4
Compare
|
Hi Srikanth Muppandam (@smuppand) Improved Malformed control configurations now fail preparation instead of being silently ignored, preventing test execution with unsupported settings. |
| [ -z "$vsc_val" ] && return 1 | ||
|
|
||
| # Quick check: does this file have the target control at all? | ||
| if ! grep -q "\"Id\"[[:space:]]*:[[:space:]]*\"${vsc_ctrl}\"" "$vsc_cfg" 2>/dev/null; then |
There was a problem hiding this comment.
The Shikra policy declares Level=4.0 as required, but a config without that control returns success here. The runner then executes the original config, silently bypassing the platform policy and potentially running an unsupported driver default.
Return a distinct “required control absent” error when a matching policy row is applied, and fail config preparation rather than treating it as a no-op.
| END { | ||
| # Id found but no Value found in the object — malformed config. | ||
| if (found_id > 0 && found_val == 0) exit 2 | ||
| # Value found but already at the desired setting — valid no-op. |
There was a problem hiding this comment.
found_id is incremented but never used to enforce uniqueness. Two Level objects with already-correct values return the valid-no-op status, and a mixed pair can produce one rewrite and still succeed. That contradicts the helper contract that exactly one control object must match.
Fail when found_id != 1 or found_val != 1, independently of the number of value changes.
| # platform vars (PLATFORM_MACHINE, PLATFORM_TARGET, PLATFORM_SOC_MACHINE, | ||
| # PLATFORM_DT_MODEL, PLATFORM_DT_COMPAT). | ||
| # | ||
| # This is the SINGLE shared source of truth for "what board am I running on", |
There was a problem hiding this comment.
This says the helper is the shared source of truth “consumed by both audio and video,” but audio_common.sh still has its own audio_platform_is_shikra() DT scan.
Rework audio to consume platform_identity_matches shikra, then remove the duplicate scan. Otherwise future DT-root or identity changes will still diverge across audio and video.
| # device-tree/platform data is available. Callers should treat an empty | ||
| # result as "no match" rather than falling back to a hostname heuristic. | ||
| # ----------------------------------------------------------------------------- | ||
| platform_runtime_identity() { |
There was a problem hiding this comment.
detect_platform() already provides platform DT fields. The new shared identity helper is a reasonable extension, but its intended reuse is incomplete because audio still maintains a parallel implementation.
c9ae0a4 to
8a81b10
Compare
|
Hi Srikanth Muppandam (@smuppand) , The awk END block now enforces
|
| return | ||
| fi | ||
|
|
||
| # Inline fallback — identical logic to platform_runtime_identity() + match. |
There was a problem hiding this comment.
All repository callers source functestlib.sh before audio_common.sh, including the child user-session paths. This fallback therefore preserves a parallel DT scanner unnecessarily.
Remove the duplicate implementation and use platform_identity_matches shikra as the single contract. If standalone sourcing is required, source the shared helper explicitly.
| if (line ~ ("\"Id\"[[:space:]]*:[[:space:]]*\"" ctrl "\"")) { | ||
| found_id++ | ||
| # Compact layout: Id and Value on the same line. | ||
| if (line ~ /"Value"[[:space:]]*:/) { |
There was a problem hiding this comment.
This accepts every Value field but only rewrites quoted string values. For "Value": 1, found_val increments, substitution changes nothing, and the helper returns successful no-op even though the override was not applied. It also never proves the matching Id belongs to StaticControls.
Parse the intended StaticControls object, support or explicitly reject non-string values, and verify the staged output contains exactly the requested value.
|
|
||
| shikra) | ||
| # Shikra is upstream-only; qcom_iris is the sole driver. | ||
| if video_has_module_loaded qcom_iris; then |
There was a problem hiding this comment.
Shikra readiness is based only on lsmod, so a built-in qcom_iris driver with bound runtime devices is incorrectly reported missing.
Combine actual driver-binding evidence with module state so built-in and modular drivers are handled equivalently.
000f2db to
6b694be
Compare
|
Hi Srikanth Muppandam (@smuppand) , |
|
sign-off is missing |
6b694be to
fc73d7a
Compare
|
Hi Srikanth Muppandam (@smuppand) , |
Srikanth Muppandam (smuppand)
left a comment
Srikanth Muppandam (smuppand)
left a comment
There was a problem hiding this comment.
The single commit mixes:
- Shared platform-identity/audio behavior.
- Shikra video support.
- Hamoa video support.
- JSON policy transformation.
Recommended structure:
- utils: add shared runtime platform identity helpers
- video: add Shikra V4L2 support
- Move Hamoa support to a separate PR unless it is part of the same independently validated rollout.
| return 1 | ||
| ;; | ||
| esac | ||
| platform_identity_matches "shikra" |
There was a problem hiding this comment.
platform_identity_matches is not defined in functestlib.sh or anywhere else at this PR head. Add the shared platform identity helpers to functestlib.sh in this PR and test both matching and non-matching platforms, or retain the previous working runtime-DT implementation.
| if (line ~ /"Value"[[:space:]]*:/) { | ||
| if (line ~ /"Value"[[:space:]]*:[[:space:]]*"[^"]*"/) { | ||
| orig = line | ||
| sub(/"Value"[[:space:]]*:[[:space:]]*"[^"]*"/, \ |
There was a problem hiding this comment.
This substitution does not safely handle compact JSON. When base_h264Encoder.json is collapsed onto one valid JSON line, the helper returns success but changes Profile's Value to 4.0 while leaving Level at 5.0, because sub() replaces the first Value on the line.
The runner then executes a silently corrupted configuration while reporting that the Level policy was applied. Use an object-scoped JSON transformation that binds Value to the matching Id, and add regression fixtures for compact and pretty-printed JSON.
|
|
||
| { | ||
| printf '%s\n' \ | ||
| "shikra|encode|h264|Level|4.0" \ |
There was a problem hiding this comment.
This policy silently overwrites Level in every user-supplied Shikra H.264 or HEVC encode configuration, not only the bundled fixtures. That changes the workload requested through --config and prevents users from validating other supported levels.
Restrict the automatic rewrite to the intended bundled fixtures, or make the override explicit and document the authoritative platform contract that requires Level 4.0.
| # shikra|encode|h264|Level|4.0 | ||
| # shikra|encode|hevc|Level|4.0 | ||
| # | ||
| # Extend at runtime via VIDEO_POLICY_EXTRA (same pipe-separated format, |
There was a problem hiding this comment.
The new Shikra/Hamoa platform behavior and VIDEO_POLICY_EXTRA interface are not reflected in run.sh --help, README_Video.md, or the YAML description.
a0036da to
2654d84
Compare
2654d84 to
0eca7da
Compare
|
hi Srikanth Muppandam (@smuppand) ,
|
Srikanth Muppandam (smuppand)
left a comment
Srikanth Muppandam (smuppand)
left a comment
There was a problem hiding this comment.
The commit remains mostly cohesive, although partial undocumented Hamoa branches are still included in several stack helpers. Those should be removed if Hamoa is intended for a separate PR.
| vpl_policy_file="$LOG_DIR/.policy_${id}.$$.txt" | ||
| vpl_cfg_base="$(basename "$cfg")" | ||
| vpl_apply_policy=0 | ||
| case "$vpl_cfg_base" in |
There was a problem hiding this comment.
“Bundled” detection checks only the basename. A user running /data/base_h264Encoder.json has their custom configuration silently rewritten despite the documented promise that user-provided configs are unchanged by default. The suite can therefore PASS a workload different from the requested one.
Fix by comparing the resolved configuration path against the actual repository fixture paths, not merely its basename.
| ;; | ||
| shikra) | ||
| if [ "$post_stack" = "upstream" ]; then | ||
| if video_has_module_loaded qcom_iris; then |
There was a problem hiding this comment.
The hard gate accepts built-in qcom_iris, but this informational check uses module-only detection and then warns that the driver is absent. Use video_qcom_iris_active() here.
| # portion of the line that follows the matched Id, so that | ||
| # an earlier Value field (e.g. Profile Value) is not | ||
| # accidentally overwritten when the entire JSON is on one line. | ||
| if (line ~ /"Value"[[:space:]]*:/) { |
There was a problem hiding this comment.
For valid compact JSON such as:
{"StaticControls":[{"Value":"5.0","Id":"Level"},{"Id":"Profile","Value":"HIGH"}]}
the code selects the first Value after Id: Level, changing Profile to 4.0 while leaving Level unchanged. The global verification at line 2387 still succeeds because some Value: 4.0 exists.
Use an object-aware transformation that binds Id and Value within the same object, or explicitly reject layouts that cannot be transformed safely.
Address all reviewer comments on the Shikra V4L2 support: utils/functestlib.sh - Add platform_runtime_identity() and platform_identity_matches() as the single source of truth for board identity shared by audio, video, and future callers. utils/lib_video.sh - Add Shikra platform detection and upstream-only stack support. - Add video_qcom_iris_active() combining lsmod and sysfs binding evidence to handle both modular and built-in qcom_iris drivers. - Fix video_stage_control_override() AWK compact-JSON scoping: use a placeholder-based split so sub() applies only to the portion of the line after the matched Id. - Add video_policy_lookup() with Level-4.0 cap for Shikra H.264/HEVC encode; restrict automatic rewrite to bundled fixtures by default. - Remove Hamoa from video_detect_platform() (separate PR). suites/Multimedia/Video/Video_V4L2_Runner/run.sh - Add Shikra post-stack module validation. - Add bundled-fixture guard before video_policy_lookup. - Document --platform shikra, VIDEO_POLICY_SCOPE, and VIDEO_POLICY_EXTRA in --help. suites/Multimedia/Video/Video_V4L2_Runner/README_Video.md - Add Shikra Platform Support section. suites/Multimedia/Video/Video_V4L2_Runner/Video_V4L2_Runner.yaml - Add VIDEO_STACK and VIDEO_POLICY_SCOPE params; wire --stack into run. Signed-off-by: Vaishnavi Madireddy <vmadired@qti.qualcomm.com>
0eca7da to
16ebd65
Compare
|
Hi Srikanth Muppandam (@smuppand) , |
This branch has not been deployed
2 participants
Changes: