Claude review: drm/rockchip: dw_hdmi_qp: expose "overscan" property

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Issues:**

1. **`drm_mode_create_tv_properties_legacy()` is deprecated** (critical). The function docstring says:

   > NOTE: This functions registers the deprecated "mode" connector property to select the analog TV mode (ie, NTSC, PAL, etc.). New drivers must use `drm_mode_create_tv_properties()` instead.

   Using the legacy function creates ~10 unnecessary TV properties (select subconnector, subconnector, brightness, contrast, flicker reduction, saturation, hue, mode, plus the margin properties). For an HDMI connector, most of these make no sense. Either use `drm_mode_create_tv_properties()` instead, or better yet, just create the overscan property directly if that's the only one needed. Check whether `drm_mode_create_tv_properties(dev, 0)` with no supported TV modes would be cleaner.

2. **Margin symmetry assumption.** The conversion in `atomic_check`:
   ```c
   s->tv_margins.left   = adj_mode->hdisplay * overscan / 200;
   s->tv_margins.right  = s->tv_margins.left;
   s->tv_margins.top    = adj_mode->vdisplay * overscan / 200;
   s->tv_margins.bottom = s->tv_margins.top;
   ```
   Applies equal margins on both sides, which is the expected behavior for the "overscan" property (a single percentage). This is correct, but the commit message could be clearer that this is intentionally symmetric. The `/200` divisor means `overscan=100` yields margins of 50% on each side, leaving zero visible area -- while `min(conn_state->tv.overscan, 100u)` allows this. In practice the property range is 0-100 and no user would set 100%, but the edge case exists.

3. **Integer rounding.** For small display sizes or small overscan percentages, `hdisplay * overscan / 200` may round to 0 even when the user requested some overscan. For example, with `hdisplay=640` and `overscan=1`, the result is `640/200 = 3` pixels per side. That actually works fine. But for `overscan=0` the result is correctly 0. This is acceptable.

4. **Missing `drm_connector_attach_tv_margin_properties()` or equivalent.** The patch attaches `tv_overscan_property` but does not attach the individual margin properties (`left margin`, `right margin`, etc.). This is intentional since the driver uses the single "overscan" knob rather than per-side margins. However, userspace compositors like KWin may expect the individual margin properties to also exist. Verify that KWin actually uses the "overscan" property as claimed (it does appear to, based on the KWin source).

5. **Property attached after connector init.** The property is attached in `dw_hdmi_qp_rockchip_bind()` after `drm_bridge_connector_init()` returns. This should work since property attachment is just adding a property reference to the connector object, but it's worth checking that no path reads connector properties before `bind()` returns.

6. **No `atomic_check` for overscan-only changes.** If the user changes overscan but nothing else changes on the connector/CRTC, the margin values are updated in the CRTC state during `atomic_check`, but there's no explicit check that forces a CRTC update. This likely works because changing any connector property triggers a full atomic commit, but it should be verified that `vop2_post_config()` is actually called in this scenario.

**Summary:** The series is a reasonable approach but should switch from `drm_mode_create_tv_properties_legacy()` to either `drm_mode_create_tv_properties()` or a more targeted property creation to avoid registering a pile of irrelevant analog TV properties on an HDMI connector.

---
Generated by Claude Code Patch Reviewer