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

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

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.

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