Re: [PATCH v2] drm/rockchip: vop2: Scale the AXI clock to the bandwidth the mode needs

[Igor Paunovic <royalnet026@gmail.com>]

The review bot found three things on v2 and all three are right. I am
answering rather than sending a v3 straight away, because the fix for
the two High ones is a single change that touches a file shared by
every Rockchip SoC, and I would rather ask about that than guess.

Both High findings come from the same shortcut. v2 keeps the
requirement in a global atomic state object, which I still think is the
right container, but it applies the rate from vop2_crtc_atomic_enable()
and _disable() rather than from the commit tail:

  - Out of order commits. Two non-blocking commits on different CRTCs
    share only the private object, and nothing orders them, so a commit
    that took its snapshot before another CRTC raised the rate can land
    after it and lower it again.

  - Multi-CRTC disable. atomic_disable() runs once per CRTC, and the
    first one already sees a state in which every participating CRTC is
    off, so the rate drops while the others are still scanning out and
    waiting for dsp_hold_completion.

vc4 solves both of these for its core clock, and what I did was take
half of that pattern instead of all of it:

  - vc4_atomic_commit_setup() records a pending commit per channel in
    the private state and the next commit waits on it with
    drm_crtc_commit_wait(). That is the ordering v2 has no equivalent
    of.

  - vc4_atomic_commit_tail() holds max(old, new) for the length of the
    commit and only drops to the new rate after
    drm_atomic_helper_wait_for_flip_done(). That is exactly the window
    the second finding describes.

Hence the question. Doing the same in rockchip means adding both
.atomic_commit_setup and .atomic_commit_tail to
rockchip_mode_config_helpers in rockchip_drm_fb.c, which today carries
only .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm and is
shared by every SoC this driver supports, VOP as well as VOP2. The
commit tail would be a thin wrapper around the rpm helper with the
clock work on either side of it, and both hooks would do nothing on
anything that is not RK3588.

Is that acceptable, or would you rather this stayed inside vop2 in some
other shape? I am happy to write it either way, but I would rather find
that out before than after.

The Medium finding needs no discussion: if
drm_atomic_private_obj_init() fails, the jump to err_crtcs does not
undo rockchip_rgb_init(). It is also new in this patch, since before it
nothing after rockchip_rgb_init() could fail, so it is mine and it will
be fixed in the next version whatever shape the rest takes.

Igor