drm: align plane destination size to even pixels

[henkwiedig]

RK3566/3568 VOP2 Esmart/Smart windows cannot scale down to an odd destination width. When they are asked to, the driver shaves the pixel itself and logs

[drm] vp0 Esmart0-win0 dsp_w[1843] MODE 2 == 1 at scale down mode

on every atomic commit, so a running ground station floods dmesg at frame rate.

An odd width is easy to hit: --video-scale 0.96 on a 1920 wide mode gives 1843, and the aspect fit alone yields 1905 for a 1920x1088 source even at scale factor 1.0. Both the video and the OSD plane are affected, since modeset_atomic_prepare_commit applies the scale factor to whatever plane it is handed.

Round the destination width and height down to even in the shared helper both call sites now use. The two copies of the aspect-fit math were already meant to be identical, so this keeps them from drifting.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

DRM: round plane destination rect to even pixels for RK3566/3568 VOP2

🐞 Bug fix 🕐 10-20 Minutes

AI Description

• Add shared helper to compute centered, aspect-fit destination rectangle.
• Round destination width/height down to even pixels to avoid VOP2 scale-down warnings.
• Deduplicate geometry math between atomic commit preparation and runtime video scaling.

Diagram

graph TD
  out["modeset_output"] --> helper["video_dst_rect()"] --> props["DRM plane props (CRTC_*/SRC_*)"] --> vop2["RK3566/3568 VOP2"]
  apply["modeset_apply_video_scale()"] --> helper
  atomic["modeset_atomic_prepare_commit()"] --> helper

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Round to nearest even (or round up) instead of round down

• ➕ Reduces systematic shrinking bias (less likely to slightly undershoot intended size).
• ➕ May preserve slightly more of the requested destination area.
• ➖ Rounding up can exceed the fitted bounds and potentially violate constraints or introduce edge clipping.
• ➖ More cases to reason about (especially near CRTC bounds).

2. Apply alignment only when scaling down (or only for affected planes/hardware)

• ➕ Limits behavior changes to the problematic mode/hardware path.
• ➕ Avoids altering geometry in scale-up or pass-through scenarios.
• ➖ Requires additional detection/branching that can drift across call sites again.
• ➖ May still miss edge cases where the driver enforces alignment regardless.

Recommendation: Keep the PR’s approach: a shared helper prevents the two aspect-fit implementations from diverging, and rounding down with a simple even mask is deterministic and avoids exceeding the fitted rectangle. If visual bias becomes noticeable, consider “nearest-even” rounding with explicit clamp-to-fit bounds, but that adds complexity for a primarily log-flood mitigation.

Files changed (1) +40 / -28

Bug fix (1) +40 / -28

drm.cDeduplicate aspect-fit math and even-align destination plane size +40/-28

Deduplicate aspect-fit math and even-align destination plane size

• Introduces a shared video_dst_rect() helper to compute a centered, aspect-preserving destination rectangle from CRTC dimensions and scale factor. The helper rounds destination width/height down to even pixels to avoid RK3566/3568 VOP2 scale-down warnings and dmesg spam. Both modeset_apply_video_scale() and modeset_atomic_prepare_commit() now call the helper instead of maintaining duplicated geometry calculations.

src/drm.c

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

1. Unconditional even-size rounding 🐞 Bug ≡ Correctness

Description

video_dst_rect() always rounds the fitted destination width/height down to even pixels, even when
the fitted size equals the CRTC size and no downscaling is happening. On odd-sized modes
(out->mode.hdisplay/vdisplay), this changes the requested plane size by 1px and can leave a 1px
unused border / off-by-one centering compared to the pre-PR behavior.

Code

src/drm.c[R702-705]

+	*dst_w = (uint32_t)(fit_w * out->video_scale_factor) & ~1u;
+	*dst_h = (uint32_t)(fit_h * out->video_scale_factor) & ~1u;
+	*dst_x = (out->video_crtc_width  - (int)*dst_w) / 2;
+	*dst_y = (out->video_crtc_height - (int)*dst_h) / 2;

Evidence

The helper forces even sizes unconditionally, and the fitted area baseline comes directly from the
selected DRM mode dimensions, which may be odd; this combination introduces a new 1px shrink vs. the
previous code paths that preserved odd sizes when scale_factor==1.0 and no downscaling was required.

src/drm.c[690-705]
src/drm.c[434-446]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`video_dst_rect()` unconditionally forces `dst_w`/`dst_h` to be even via `& ~1u`. This changes output geometry on odd-sized modes even when scaling isn’t happening (e.g., scale_factor==1.0 and aspect-fit yields the full mode size), shrinking by 1px.

### Issue Context
The PR’s motivation is to avoid RK3566/3568 VOP2 warnings when *scaling down* to an odd destination width. That constraint does not necessarily apply when no downscaling occurs.

### Fix Focus Areas
- src/drm.c[690-706]

### Suggested change
1. Compute the unaligned `dst_w_unaligned` / `dst_h_unaligned` first.
2. Apply even alignment only when the operation is actually downscaling for that dimension, e.g.:
  - `if (dst_w_unaligned < src_w) dst_w_unaligned &= ~1u;`
  - `if (dst_h_unaligned < src_h) dst_h_unaligned &= ~1u;`
  (or, if the HW restriction is width-only, gate only width).
3. Keep centering based on the final aligned values.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[qodo-free-for-open-source-projects]

1. Unconditional even-size rounding 🐞 Bug ≡ Correctness

video_dst_rect() always rounds the fitted destination width/height down to even pixels, even when
the fitted size equals the CRTC size and no downscaling is happening. On odd-sized modes
(out->mode.hdisplay/vdisplay), this changes the requested plane size by 1px and can leave a 1px
unused border / off-by-one centering compared to the pre-PR behavior.

Agent Prompt

### Issue description
`video_dst_rect()` unconditionally forces `dst_w`/`dst_h` to be even via `& ~1u`. This changes output geometry on odd-sized modes even when scaling isn’t happening (e.g., scale_factor==1.0 and aspect-fit yields the full mode size), shrinking by 1px.

### Issue Context
The PR’s motivation is to avoid RK3566/3568 VOP2 warnings when *scaling down* to an odd destination width. That constraint does not necessarily apply when no downscaling occurs.

### Fix Focus Areas
- src/drm.c[690-706]

### Suggested change
1. Compute the unaligned `dst_w_unaligned` / `dst_h_unaligned` first.
2. Apply even alignment only when the operation is actually downscaling for that dimension, e.g.:
   - `if (dst_w_unaligned < src_w) dst_w_unaligned &= ~1u;`
   - `if (dst_h_unaligned < src_h) dst_h_unaligned &= ~1u;`
   (or, if the HW restriction is width-only, gate only width).
3. Keep centering based on the final aligned values.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[henkwiedig]

@qodo can you check now

[qodo-free-for-open-source-projects]

Rechecked: this is addressed. video_dst_rect() now computes the unaligned dimensions first and only rounds the width down to an even value when w < src_w, i.e. during horizontal downscaling. Pass-through and upscaled odd-sized modes retain their original geometry, while the RK VOP2 downscaling constraint is still handled.