Skip to content

Conversation

@amazingfate
Copy link
Contributor

@amazingfate amazingfate commented Dec 8, 2025

Description

This will fix issue: https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/4786

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also note any relevant details for your test configuration.

  • ./compile.sh kernel BOARD=armsom-sige7 BRANCH=edge KERNEL_CONFIGURE=no DEB_COMPRESS=xz KERNEL_BTF=yes KERNEL_GIT=shallow
  • Tested with armsom sige7

Checklist:

Please delete options that are not relevant.

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

Summary by CodeRabbit

  • Bug Fixes
    • Fixed AV1 video decoding on RockChip64 hardware to correctly determine when CDEF enhancement processing should be enabled.

✏️ Tip: You can customize this high-level summary in your review settings.

@github-actions github-actions bot added size/small PR with less then 50 lines 02 Milestone: First quarter release Needs review Seeking for review Hardware Hardware related like kernel, U-Boot, ... Patches Patches related to kernel, U-Boot, ... labels Dec 8, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 8, 2025

Walkthrough

A patch that corrects AV1 CDEF enablement logic in the rockchip VPU981 hardware decoder. The fix changes CDEF register activation from relying solely on a sequence flag to checking whether any CDEF-related fields are non-zero, improving decoder behavior during video playback.

Changes

Cohort / File(s) Summary
AV1 CDEF Enablement Fix
patch/kernel/archive/rockchip64-6.18/media-0004-media-verisilicon-AV1-Fix-enable-cdef-computation.patch
Introduces a local enable_cdef boolean set to true when any CDEF field (bits, damping_minus_3, y_pri_strength, y_sec_strength, uv_pri_strength, uv_sec_strength) is non-zero, replacing prior sequence-flag-only logic. Writes computed value to hardware register while maintaining existing CDEF parameter writes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify the CDEF field logic—confirm all relevant fields are checked for non-zero values
  • Validate hardware register writes and ensure no side effects from removing the old conditional assignment
  • Confirm the fix aligns with AV1 specification expectations for CDEF enablement

Suggested reviewers

  • rpardini
  • igorpecovnik
  • joekhoobyar
  • brentr
  • prahal
  • krachlatte
  • paolosabatino
  • JohnTheCoolingFan
  • catalinii
  • TheSnowfield

Poem

🐰 A filter's truth now shines so bright,
No more does sequence rule the night,
When CDEF fields dance non-zero and free,
The hardware knows—enablement shall be!
wiggles nose at fixed decoder

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding a patch to fix AV1 decoding on rk3588, which directly matches the patch content that fixes CDEF enablement in AV1 decoding.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between ffd4257 and 541fa97.

📒 Files selected for processing (1)
  • patch/kernel/archive/rockchip64-6.18/media-0004-media-verisilicon-AV1-Fix-enable-cdef-computation.patch (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub API (https://api.github.com/repos/armbian/build/pulls/{pr_number}/files) to get the complete picture of what files are being added or modified, especially for U-Boot patches that will be applied during the build process.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub or the PR API to get the complete picture of what files are being added or modified.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8968
File: patch/u-boot/u-boot-sunxi/arm64-dts-sun50i-h6-orangepi.dtsi-Rollback-r_rsb-to-r_i2c.patch:36-36
Timestamp: 2025-11-20T18:20:11.985Z
Learning: The rewrite-patches tool (REWRITE_PATCHES=yes) in the Armbian build system can inadvertently introduce semantic changes when the u-boot/kernel git base revision differs from expected state. The tool applies patches, commits them, and re-exports them using git format-patch, which can cause the re-exported patch to reflect the base revision's state rather than preserving the original patch intent. This is particularly problematic for device tree changes like interrupt specifications. The tool currently lacks validation mechanisms to detect such semantic drift, and affected patches must be manually corrected after rewriting.
🔇 Additional comments (2)
patch/kernel/archive/rockchip64-6.18/media-0004-media-verisilicon-AV1-Fix-enable-cdef-computation.patch (2)

22-37: CDEF enable computation and register write look consistent with intent

The enable_cdef computation correctly enables the hardware CDEF bit whenever any of the relevant CDEF fields is non-zero, and the write is now colocated with the other CDEF-related registers. This makes the behavior match the commit message and avoids relying solely on the sequence flag.


38-46: Good removal of redundant av1_enable_cdef programming from parameters setup

Dropping the av1_enable_cdef write from rockchip_vpu981_av1_dec_set_parameters avoids overriding the value now computed in rockchip_vpu981_av1_dec_set_cdef, keeping all CDEF-related programming in one place and reducing the chance of inconsistent state. If this patch was synced from upstream, it’s worth double-checking after any REWRITE_PATCHES runs that it still matches the upstream fix exactly, given past drift issues. Based on learnings, this extra check can prevent subtle semantic regressions.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added the Ready to merge Reviewed, tested and ready for merge label Dec 8, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2025

✅ This PR has been reviewed and approved — all set for merge!

@github-actions github-actions bot removed the Needs review Seeking for review label Dec 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

02 Milestone: First quarter release Hardware Hardware related like kernel, U-Boot, ... Patches Patches related to kernel, U-Boot, ... Ready to merge Reviewed, tested and ready for merge size/small PR with less then 50 lines

Development

Successfully merging this pull request may close these issues.

2 participants