Re:Re: [PATCH v2 5/8] drm/rockchip: vop2: Enforce AFBC source alignment in plane_check

Andy Yan
2026-01-20T12:48:35Z

Hi，
At 2025-12-12 17:59:34, "Chaoyi Chen" <chaoyi.chen@rock-chips.com> wrote:
>Hi Nicolas,
>
>On 12/11/2025 10:16 PM, Nicolas Frattaroli wrote:
>> On Thursday, 11 December 2025 12:06:38 Central European Standard Time Chaoyi Chen wrote:
>>> Hello Nicolas,
>>>
>>> On 12/9/2025 6:58 PM, Nicolas Frattaroli wrote:
>>>> Hi Chaoyi Chen, Andy Yan,
>>>>
>>>> On Monday, 8 December 2025 08:24:52 Central European Standard Time Nicolas Frattaroli wrote:
>>>>> On Monday, 8 December 2025 03:48:24 Central European Standard Time Chaoyi Chen wrote:
>>>>>> Hello Nicolas, Daniel,
>>>>>>
>>>>>> On 12/7/2025 4:45 AM, Nicolas Frattaroli wrote:
>>>>>>> From: Daniel Stone <daniels@collabora.com>
>>>>>>>
>>>>>>> Planes can only source AFBC framebuffers at multiples of 4px wide on
>>>>>>> RK3566/RK3568. Instead of clipping on all SoCs when the user asks for an
>>>>>>> unaligned source rectangle, reject the configuration in the plane's
>>>>>>> atomic check on RK3566/RK3568 only.
>>>>>>>
>>>>>>> Signed-off-by: Daniel Stone <daniels@collabora.com>
>>>>>>> [Make RK3566/RK3568 specific, reword message]
>>>>>>> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
>>>>>>> ---
>>>>>>> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 14 +++++++++-----
>>>>>>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>>>>> index bc1ed0ffede0..e23213337104 100644
>>>>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>>>>> @@ -1076,6 +1076,13 @@ static int vop2_plane_atomic_check(struct drm_plane *plane,
>>>>>>> return -EINVAL;
>>>>>>> }
>>>>>>>
>>>>>>> + if (vop2->version == VOP_VERSION_RK3568 && drm_is_afbc(fb->modifier) && src_w % 4) {
>>>>>>> + drm_dbg_kms(vop2->drm,
>>>>>>> + "AFBC source rectangles must be 4-byte aligned; is %d\n",
>>>>>>> + src_w);
>>>>>>> + return -EINVAL;
>>>>>>> + }
>>>>>>> +
>>>>>>> return 0;
>>>>>>> }
>>>>>>>
>>>>>>> @@ -1237,11 +1244,8 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
>>>>>>> WARN_ON(src_w < 4);
>>>>>>> WARN_ON(src_h < 4);
>>>>>>>
>>>>>>> - if (afbc_en && src_w % 4) {
>>>>>>> - drm_dbg_kms(vop2->drm, "vp%d %s src_w[%d] not 4 pixel aligned\n",
>>>>>>> - vp->id, win->data->name, src_w);
>>>>>>> - src_w = ALIGN_DOWN(src_w, 4);
>>>>>>> - }
>>>>>>> + if (vop2->version == VOP_VERSION_RK3568 && drm_is_afbc(fb->modifier))
>>>>>>> + WARN_ON(src_w % 4);
>>>>>>>
>>>>>>> act_info = (src_h - 1) << 16 | ((src_w - 1) & 0xffff);
>>>>>>> dsp_info = (dsp_h - 1) << 16 | ((dsp_w - 1) & 0xffff);
>>>>>>>
>>>>>>
>>>>>> You haven't replied to Andy's comment yet [0].
>>>>>>
>>>>>> [0] https://lore.kernel.org/dri-devel/7b4e26ec.75f3.19a77276b53.Coremail.andyshrk@163.com/
>>>>>>
>>>>>
>>>>> Hello,
>>>>>
>>>>> I addressed the follow-ups where it was clarified that the 4 pixel
>>>>> limitation was RK3566/RK3568-only. I'm not going to bring back the
>>>>> post-atomic_check modification for a fast path, but I'm open to
>>>>> suggestions on how to do this differently.
>>>>>
>>>>> One solution might be to modify the state with the ALIGN_DOWN stuff
>>>>> in atomic_check instead, where userspace is then aware of the change
>>>>> being done to its requested parameters. I'll need to double-check
>>>>> whether this is in line with atomic modesetting's design.
>>>>>
>>>>> Kind regards,
>>>>> Nicolas Frattaroli
>>>>
>>>> Okay, so I've asked internally, and atomic_check isn't allowed to
>>>> modify any of the parameters either. There's efforts [0] underway
>>>> to allow error codes to be more specific, so that userspace knows
>>>> which constraint is being violated. That would allow userspace
>>>> applications to react by either adjusting their size or turning
>>>> off AFBC in this case. Turning off AFBC seems more generally
>>>> applicable here, since it means it won't need to resize the plane
>>>> and it'll save more than enough memory bandwidth by not going
>>>> through the GPU.
>>>>
>>>> On that note: Andy, I didn't find a weston-simple-egl test in the
>>>> Weston 14.0.2 or git test suite, and weston-simple-egl itself does
>>>> not tell me whether GPU compositing is being used or not. Do you
>>>> have more information on how to test for this? I'd like to know
>>>> for when we have the necessary functionality in place to make
>>>> userspace smart enough to pick the fast path again.
>>>>
>>>
>>> I think weston-simple-egl is part of the weston client. When you build
>>> weston from source, you should obtain it. Just run `weston-simple-egl`
>>> after compile and install weston.
>>>
>>> And I guess you're using Debian... The weston package there also ships
>>> with a weston-simple-egl binary [2].
>>
>> Yeah, I know there's a tool called that, but I'm specifically curious
>> about how to determine whether it's using GPU compositing or what I
>> presume is fixed-function compositing.
>>
>> When I enable some more logging with
>>
>> weston -l log,drm-backend,gl-renderer
>>
>> and also some kms debug messages with
>>
>> echo 4 > /sys/module/drm/parameters/debug
>>
>> then I see weston outputting
>>
>> [atomic] drmModeAtomicCommit
>> [repaint] Using mixed state composition
>> [repaint] view 0xaaab18c00c10 using renderer composition
>> [repaint] view 0xaaab18b68f00 using renderer composition
>> [repaint] view 0xaaab18c00ec0 using renderer composition
>>
>> regardless of whether the size is 250x250 or fullscreen. With
>> 250x250, I know we're failing the plane check, because I see
>>
>> [ 776.160101] rockchip-drm display-subsystem: [drm:vop2_plane_atomic_check
>> [rockchipdrm]] AFBC source rectangles must be 4-pixel
>> aligned; is 250
>>
>> on the console, but with fullscreen I don't see any errors from plane-check
>> as the src_w is now divisible by 4, yet it's also "using renderer composition"
>> for all views.
>>
>> Same goes for using `weston-simple-dmabuf-egl` (which is 256x256) instead of
>> the fullscreen simple-egl.
>>
>> So basically, I need to know where a change in behaviour is actually
>> observed.
>>
>
>Hmm, I don't think weston logs make this obvious. We also need the drm kms logs.
>
>Test results may vary across different platforms. But it's certain that
>fullscreen and non-fullscreen modes are identical in the following respects:
>
>- fullscreen: Try to use only one plane (may be AFBC) .
>- non fullscreen: Try to use 2 plane (may be AFBC) .
>
>- On the RK3588, we won't get two planes without modifying the code, because
>the primary plane is already assign to AFBC background plane.
>- On the RK356x, we'll get a primary plane in linear format, while
>weston-simple-egl will use an overlay plane with AFBC format.
>- On the RK3576, we should be able to obtain an AFBC primary plane and an AFBC
>overlay plane.
>
>Try to set env `export WESTON_LIBINPUT_LOG_PRIORITY=debug` and you will see
>these log in non fullscreen mode:
>
>```
>Layer 5 (pos 0x50000000):
> View 0 (role xdg_toplevel, PID 686, surface ID 14, top-level window 'simple-egl' of org.freedesktop.weston.simple-egl, 0xaaaae61a62d0):
>
>...
>
> [view] evaluating view 0xaaaae61a62d0 for plane assignment on output HDMI-A-1 (0)
> [plane] started with zpos 18446744073709551615
> [primary] not assigning view 0xaaaae61a62d0 on primary, plane 33 (format ARGB8888 (0x34325241) with modifier 0x800000000000051) not supported
> [primary] not assigning view 0xaaaae61a62d0 on primary, plane 39 (format ARGB8888 (0x34325241) with modifier 0x800000000000051) not supported
> [overlay] not assigning view 0xaaaae61a62d0 on overlay, plane 63 (format ARGB8888 (0x34325241) with modifier 0x800000000000051) not supported
> [overlay] not assigning view 0xaaaae61a62d0 on overlay, plane 69 (format ARGB8888 (0x34325241) with modifier 0x800000000000051) not supported
> [overlay] not assigning view 0xaaaae61a62d0 on overlay, plane 75 (format ARGB8888 (0x34325241) with modifier 0x800000000000051) not supported
> [view] view 0xaaaae61a62d0 format: ARGB8888
> [plane] plane 51 picked from candidate list, type: overlay
>```
>
>When vop2_plane_atomic_check() fails, in addition to the above logs, you will
>also get the following log:
>
>```
>[view] not placing view 0xaaaad9ec8e40 on plane 51: atomic test failed
>```
>
>I don't think there's any more log information that can indicate the error here.
>The relevant code is here [3].

>

For VOP2, we have a debug node to show the plane usage:
# cat /sys/kernel/debug/dri/0/vop2/summary
Video Port0: DISABLED
Video Port1: DISABLED
Video Port2: ACTIVE
Connector: DP-1
bus_format[100a]: RGB888_1X24
output_mode[f] color_space[0]
background color (10bpc): r=0x0 g=0x0 b=0x0
Display mode: 1920x1080p60
clk[148500] real_clk[148500] type[48] flag[5]
H: 1920 2008 2052 2200
V: 1080 1084 1089 1125
Cluster2-win0: ACTIVE
win_id: 2
format: XR24 little-endian (0x34325258)[AFBC] glb_alpha[0xff]
rotate: xmirror: 0 ymirror: 0 rotate_90: 0 rotate_270: 0
zpos: 0
src: pos[0, 0] rect[1920 x 1080]
dst: pos[0, 0] rect[1920 x 1080]
buf[0]: addr: 0x0000000000000000 pitch: 7680 offset: 0
Cluster3-win0: ACTIVE
win_id: 3
format: XR24 little-endian (0x34325258)[AFBC] glb_alpha[0xff]
rotate: xmirror: 0 ymirror: 0 rotate_90: 0 rotate_270: 0
zpos: 1
src: pos[0, 0] rect[800 x 600]
dst: pos[743, 358] rect[800 x 600]
buf[0]: addr: 0x0000000001420000 pitch: 3200 offset: 0
# >[3]: https://gitlab.freedesktop.org/wayland/weston/-/blob/main/libweston/backend-drm/state-propose.c?ref_type=heads#L181
>
>>>
>>> [1]: https://gitlab.freedesktop.org/wayland/weston/-/blob/main/clients/simple-egl.c
>>> [2]: https://packages.debian.org/sid/arm64/weston/filelist
>>>
>>>> In either case, I think adhering to the atomic API to ensure
>>>> artifact-free presentation is more important here than enabling
>>>> a fast-path on RK3568. I do think in most real-world use case
>>>> scenarios, the fallback won't degrade user experience, because
>>>> almost everything performance intensive I can think of (video
>>>> playback, games) will likely already use a plane geometry
>>>> where the width is divisible by 4. 800, 1024, 1280, 1600, 1920,
>>>> 2560, 3840 are all divisible by 4, so a window or full-screen
>>>> playback of common content won't need to fall back to GPU
>>>> compositing.
>>>>
>>>> I'll send a v2 to fix another instance of "eSmart" left in a
>>>> message, but beyond that I think we should be good.
>>>>
>>>> Kind regards,
>>>> Nicolas Frattaroli
>>>>
>>>> https://lore.kernel.org/dri-devel/20251009-atomic-v6-0-d209709cc3ba@intel.com/ [0]
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>>
>>
>>
>>
>
>--
>Best,
>Chaoyi