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

Chaoyi Chen
2025-12-11T11:07:05Z

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].

[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