[PATCH 00/13] drm/rockchip: No more post-atomic_check fixups

[PATCH 00/13] drm/rockchip: No more post-atomic_check fixups

[Daniel Stone]

Hi,
This series is a pretty small and consistent one for VOP2. The atomic
uAPI very clearly specifies that drivers should either do what userspace
requested (on a successful commit), or fail atomic_check if it is not
for any reason possible to do what userspace requested.

VOP2 is unfortunately littered with a bunch of cases where it will apply
fixups after atomic_check - doing something different to what userspace
requested, e.g. clipping or aligning regions - or throw error messages
into the log when userspace does request a condition which can't be met.

Doing something different to what was requested is bad because it
results in unexpected visual output which can look like artifacts.
Throwing errors into the log is bad because generic userspace will
reasonably attempt to try any configuration it can. For example,
throwing an error message on a plane not being aligned to a 16 pixel
boundary can result in 15 frames' worth of error output in the log when
a window is being animated across a screen.

This series removes all post-check fixups - failing the check if the
configuration cannot be applied - and also demotes all messages about
unsupported configurations to DEBUG_KMS.

Cheers,
Daniel

Daniel Stone (13):
  drm/rockchip: Demote normal drm_err to debug
  drm/rockchip: Declare framebuffer width/height bounds
  drm/rockchip: Return error code for errors
  drm/rockchip: Rename variables for clarity
  drm/rockchip: Use temporary variables
  drm/rockchip: Switch impossible format conditional to WARN_ON
  drm/rockchip: Switch impossible pos conditional to WARN_ON
  drm/rockchip: Fix eSmart test condition
  drm/rockchip: Enforce scaling workaround in plane_check
  drm/rockchip: Enforce AFBC source alignment in plane_check
  drm/rockchip: Enforce AFBC transform stride align in plane_check
  drm/rockchip: Use drm_is_afbc helper function
  drm/rockchip: Simplify format_mod_supported

 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 202 +++++++++----------
 1 file changed, 99 insertions(+), 103 deletions(-)

-- 
2.51.0

[PATCH 01/13] drm/rockchip: Demote normal drm_err to debug

[Daniel Stone]

A plane check failing is a normal and expected condition, as userspace
isn't aware of the specific constraints and will try any and every
combination until one succeeds. Push this down to a debug message, so
users can see it if they want to, but make sure we don't spam the log
during normal operation.

Fixes: 604be85547ce4 ("drm/rockchip: Add VOP2 driver")
Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 4556cf7a3364..4ba5444fde4f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -1032,20 +1032,20 @@ static int vop2_plane_atomic_check(struct drm_plane *plane,
 
 	if (drm_rect_width(src) >> 16 < 4 || drm_rect_height(src) >> 16 < 4 ||
 	    drm_rect_width(dest) < 4 || drm_rect_width(dest) < 4) {
-		drm_err(vop2->drm, "Invalid size: %dx%d->%dx%d, min size is 4x4\n",
-			drm_rect_width(src) >> 16, drm_rect_height(src) >> 16,
-			drm_rect_width(dest), drm_rect_height(dest));
+		drm_dbg_kms(vop2->drm, "Invalid size: %dx%d->%dx%d, min size is 4x4\n",
+			    drm_rect_width(src) >> 16, drm_rect_height(src) >> 16,
+			    drm_rect_width(dest), drm_rect_height(dest));
 		pstate->visible = false;
 		return 0;
 	}
 
 	if (drm_rect_width(src) >> 16 > vop2_data->max_input.width ||
 	    drm_rect_height(src) >> 16 > vop2_data->max_input.height) {
-		drm_err(vop2->drm, "Invalid source: %dx%d. max input: %dx%d\n",
-			drm_rect_width(src) >> 16,
-			drm_rect_height(src) >> 16,
-			vop2_data->max_input.width,
-			vop2_data->max_input.height);
+		drm_dbg_kms(vop2->drm, "Invalid source: %dx%d. max input: %dx%d\n",
+			    drm_rect_width(src) >> 16,
+			    drm_rect_height(src) >> 16,
+			    vop2_data->max_input.width,
+			    vop2_data->max_input.height);
 		return -EINVAL;
 	}
 
@@ -1054,7 +1054,7 @@ static int vop2_plane_atomic_check(struct drm_plane *plane,
 	 * need align with 2 pixel.
 	 */
 	if (fb->format->is_yuv && ((pstate->src.x1 >> 16) % 2)) {
-		drm_err(vop2->drm, "Invalid Source: Yuv format not support odd xpos\n");
+		drm_dbg_kms(vop2->drm, "Invalid Source: Yuv format not support odd xpos\n");
 		return -EINVAL;
 	}
 
-- 
2.51.0

[PATCH 02/13] drm/rockchip: Declare framebuffer width/height bounds

[Daniel Stone]

The VOP2 has limitations on its input and output sizes. The clipped
display region must be at least 4px in each dimension for both
framebuffer source and plane destination, and the clipped source region
must be no greater than a per-version limit.

It is never valid for VOP2 to have a framebuffer which is less than four
pixels in either dimension, so declare that as our min width/height,
enforced by AddFB failing if the user tries. It can theoretically be
valid to have a single large framebuffer of which only certain clipped
regions are shown, but this is a very uncommon case. Declaring to
userspace that the framebuffer's maximum width and height is the maximum
source clip helps it make better decisions as to which mode to use,
instead of trying unsupported sizes and having to fall back.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 4ba5444fde4f..f04fb5da1295 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -2647,6 +2647,12 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
 	if (IS_ERR(vop2->map))
 		return PTR_ERR(vop2->map);
 
+	/* Set the bounds for framebuffer creation */
+	drm->mode_config.min_width = 4;
+	drm->mode_config.min_height = 4;
+	drm->mode_config.max_width = vop2_data->max_input.width;
+	drm->mode_config.max_height = vop2_data->max_input.height;
+
 	ret = vop2_win_init(vop2);
 	if (ret)
 		return ret;
-- 
2.51.0

[PATCH 03/13] drm/rockchip: Return error code for errors

[Daniel Stone]

Instead of silently disabling small planes, refuse to create them at
all.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index f04fb5da1295..659b2565dee4 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -1035,8 +1035,7 @@ static int vop2_plane_atomic_check(struct drm_plane *plane,
 		drm_dbg_kms(vop2->drm, "Invalid size: %dx%d->%dx%d, min size is 4x4\n",
 			    drm_rect_width(src) >> 16, drm_rect_height(src) >> 16,
 			    drm_rect_width(dest), drm_rect_height(dest));
-		pstate->visible = false;
-		return 0;
+		return -EINVAL;
 	}
 
 	if (drm_rect_width(src) >> 16 > vop2_data->max_input.width ||
-- 
2.51.0

[PATCH 04/13] drm/rockchip: Rename variables for clarity

[Daniel Stone]

actual_w and actual_h were the clipped source width, so rename them to
fit the use.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 38 ++++++++++----------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 659b2565dee4..e2bf2dbd882b 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -1139,7 +1139,7 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
 	struct vop2 *vop2 = win->vop2;
 	struct drm_framebuffer *fb = pstate->fb;
 	u32 bpp = vop2_get_bpp(fb->format);
-	u32 actual_w, actual_h, dsp_w, dsp_h;
+	u32 src_w, src_h, dsp_w, dsp_h;
 	u32 act_info, dsp_info;
 	u32 format;
 	u32 afbc_format;
@@ -1203,8 +1203,8 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
 		uv_mst = rk_obj->dma_addr + offset + fb->offsets[1];
 	}
 
-	actual_w = drm_rect_width(src) >> 16;
-	actual_h = drm_rect_height(src) >> 16;
+	src_w = drm_rect_width(src) >> 16;
+	src_h = drm_rect_height(src) >> 16;
 	dsp_w = drm_rect_width(dest);
 
 	if (dest->x1 + dsp_w > adjusted_mode->hdisplay) {
@@ -1214,7 +1214,7 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
 		dsp_w = adjusted_mode->hdisplay - dest->x1;
 		if (dsp_w < 4)
 			dsp_w = 4;
-		actual_w = dsp_w * actual_w / drm_rect_width(dest);
+		src_w = dsp_w * src_w / drm_rect_width(dest);
 	}
 
 	dsp_h = drm_rect_height(dest);
@@ -1226,35 +1226,35 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
 		dsp_h = adjusted_mode->vdisplay - dest->y1;
 		if (dsp_h < 4)
 			dsp_h = 4;
-		actual_h = dsp_h * actual_h / drm_rect_height(dest);
+		src_h = dsp_h * src_h / drm_rect_height(dest);
 	}
 
 	/*
 	 * This is workaround solution for IC design:
-	 * esmart can't support scale down when actual_w % 16 == 1.
+	 * esmart can't support scale down when src_w % 16 == 1.
 	 */
 	if (!(win->data->feature & WIN_FEATURE_AFBDC)) {
-		if (actual_w > dsp_w && (actual_w & 0xf) == 1) {
+		if (src_w > dsp_w && (src_w & 0xf) == 1) {
 			drm_dbg_kms(vop2->drm, "vp%d %s act_w[%d] MODE 16 == 1\n",
-				    vp->id, win->data->name, actual_w);
-			actual_w -= 1;
+				    vp->id, win->data->name, src_w);
+			src_w -= 1;
 		}
 	}
 
-	if (afbc_en && actual_w % 4) {
-		drm_dbg_kms(vop2->drm, "vp%d %s actual_w[%d] not 4 pixel aligned\n",
-			    vp->id, win->data->name, actual_w);
-		actual_w = ALIGN_DOWN(actual_w, 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);
 	}
 
-	act_info = (actual_h - 1) << 16 | ((actual_w - 1) & 0xffff);
+	act_info = (src_h - 1) << 16 | ((src_w - 1) & 0xffff);
 	dsp_info = (dsp_h - 1) << 16 | ((dsp_w - 1) & 0xffff);
 
 	format = vop2_convert_format(fb->format->format);
 	half_block_en = vop2_half_block_enable(pstate);
 
 	drm_dbg(vop2->drm, "vp%d update %s[%dx%d->%dx%d@%dx%d] fmt[%p4cc_%s] addr[%pad]\n",
-		vp->id, win->data->name, actual_w, actual_h, dsp_w, dsp_h,
+		vp->id, win->data->name, src_w, src_h, dsp_w, dsp_h,
 		dest->x1, dest->y1,
 		&fb->format->format,
 		afbc_en ? "AFBC" : "", &yrgb_mst);
@@ -1283,7 +1283,7 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
 		if (fb->modifier & AFBC_FORMAT_MOD_YTR)
 			afbc_format |= (1 << 4);
 
-		afbc_tile_num = ALIGN(actual_w, block_w) / block_w;
+		afbc_tile_num = ALIGN(src_w, block_w) / block_w;
 
 		/*
 		 * AFBC pic_vir_width is count by pixel, this is different
@@ -1361,8 +1361,8 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
 
 	if (rotate_90 || rotate_270) {
 		act_info = swahw32(act_info);
-		actual_w = drm_rect_height(src) >> 16;
-		actual_h = drm_rect_width(src) >> 16;
+		src_w = drm_rect_height(src) >> 16;
+		src_h = drm_rect_width(src) >> 16;
 	}
 
 	vop2_win_write(win, VOP2_WIN_FORMAT, format);
@@ -1378,7 +1378,7 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
 		vop2_win_write(win, VOP2_WIN_UV_MST, uv_mst);
 	}
 
-	vop2_setup_scale(vop2, win, actual_w, actual_h, dsp_w, dsp_h, fb->format->format);
+	vop2_setup_scale(vop2, win, src_w, src_h, dsp_w, dsp_h, fb->format->format);
 	if (!vop2_cluster_window(win))
 		vop2_plane_setup_color_key(plane, 0);
 	vop2_win_write(win, VOP2_WIN_ACT_INFO, act_info);
-- 
2.51.0

[PATCH 05/13] drm/rockchip: Use temporary variables

[Daniel Stone]

Brevity is good.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 24 ++++++++++++--------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index e2bf2dbd882b..284c8a048034 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -1003,6 +1003,8 @@ static int vop2_plane_atomic_check(struct drm_plane *plane,
 	struct drm_rect *src = &pstate->src;
 	int min_scale = FRAC_16_16(1, 8);
 	int max_scale = FRAC_16_16(8, 1);
+	int src_x, src_w, src_h;
+	int dest_w, dest_h;
 	int format;
 	int ret;
 
@@ -1030,19 +1032,23 @@ static int vop2_plane_atomic_check(struct drm_plane *plane,
 	if (format < 0)
 		return format;
 
-	if (drm_rect_width(src) >> 16 < 4 || drm_rect_height(src) >> 16 < 4 ||
-	    drm_rect_width(dest) < 4 || drm_rect_width(dest) < 4) {
+	/* Co-ordinates have now been clipped */
+	src_x = src->x1 >> 16;
+	src_w = drm_rect_width(src) >> 16;
+	src_h = drm_rect_height(src) >> 16;
+	dest_w = drm_rect_width(dest);
+	dest_h = drm_rect_height(dest);
+
+	if (src_w < 4 || src_h < 4 || dest_w < 4 || dest_h < 4) {
 		drm_dbg_kms(vop2->drm, "Invalid size: %dx%d->%dx%d, min size is 4x4\n",
-			    drm_rect_width(src) >> 16, drm_rect_height(src) >> 16,
-			    drm_rect_width(dest), drm_rect_height(dest));
+			    src_w, src_h, dest_w, dest_h);
 		return -EINVAL;
 	}
 
-	if (drm_rect_width(src) >> 16 > vop2_data->max_input.width ||
-	    drm_rect_height(src) >> 16 > vop2_data->max_input.height) {
+	if (src_w > vop2_data->max_input.width ||
+	    src_h > vop2_data->max_input.height) {
 		drm_dbg_kms(vop2->drm, "Invalid source: %dx%d. max input: %dx%d\n",
-			    drm_rect_width(src) >> 16,
-			    drm_rect_height(src) >> 16,
+			    src_w, src_h,
 			    vop2_data->max_input.width,
 			    vop2_data->max_input.height);
 		return -EINVAL;
@@ -1052,7 +1058,7 @@ static int vop2_plane_atomic_check(struct drm_plane *plane,
 	 * Src.x1 can be odd when do clip, but yuv plane start point
 	 * need align with 2 pixel.
 	 */
-	if (fb->format->is_yuv && ((pstate->src.x1 >> 16) % 2)) {
+	if (fb->format->is_yuv && src_x % 2) {
 		drm_dbg_kms(vop2->drm, "Invalid Source: Yuv format not support odd xpos\n");
 		return -EINVAL;
 	}
-- 
2.51.0

[PATCH 06/13] drm/rockchip: Switch impossible format conditional to WARN_ON

[Daniel Stone]

We should never be able to create a framebuffer with an unsupported
format, so throw a warning if this ever happens, instead of attempting
to stagger on.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 284c8a048034..8c5042fb2e7e 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -1029,8 +1029,8 @@ static int vop2_plane_atomic_check(struct drm_plane *plane,
 		return 0;
 
 	format = vop2_convert_format(fb->format->format);
-	if (format < 0)
-		return format;
+	/* We shouldn't be able to create a fb for an unsupported format */
+	WARN_ON(format < 0);
 
 	/* Co-ordinates have now been clipped */
 	src_x = src->x1 >> 16;
-- 
2.51.0

Re: [PATCH 06/13] drm/rockchip: Switch impossible format conditional to WARN_ON

[Heiko Stuebner]

Hi Daniel,

Am Mittwoch, 15. Oktober 2025, 13:00:35 Mitteleuropäische Sommerzeit schrieb Daniel Stone:
> We should never be able to create a framebuffer with an unsupported
> format, so throw a warning if this ever happens, instead of attempting
> to stagger on.
> 
> Signed-off-by: Daniel Stone <daniels@collabora.com>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 284c8a048034..8c5042fb2e7e 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -1029,8 +1029,8 @@ static int vop2_plane_atomic_check(struct drm_plane *plane,
>  		return 0;
>  
>  	format = vop2_convert_format(fb->format->format);
> -	if (format < 0)
> -		return format;
> +	/* We shouldn't be able to create a fb for an unsupported format */
> +	WARN_ON(format < 0);

What happened to Greg's
"But don't add new WARN() calls please, just properly clean up and handle
the error." [0]

Also, switching to WARN_ON would then continue the atomic_check function
where it currently does exit with a real error code?


Heiko


[0] https://lwn.net/ml/linux-kernel/2024041544-fester-undead-7949@gregkh/

Re: [PATCH 06/13] drm/rockchip: Switch impossible format conditional to WARN_ON

[Heiko Stuebner]

Am Montag, 20. Oktober 2025, 15:10:31 Mitteleuropäische Sommerzeit schrieb Heiko Stuebner:
> Hi Daniel,
> 
> Am Mittwoch, 15. Oktober 2025, 13:00:35 Mitteleuropäische Sommerzeit schrieb Daniel Stone:
> > We should never be able to create a framebuffer with an unsupported
> > format, so throw a warning if this ever happens, instead of attempting
> > to stagger on.
> > 
> > Signed-off-by: Daniel Stone <daniels@collabora.com>
> > ---
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > index 284c8a048034..8c5042fb2e7e 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > @@ -1029,8 +1029,8 @@ static int vop2_plane_atomic_check(struct drm_plane *plane,
> >  		return 0;
> >  
> >  	format = vop2_convert_format(fb->format->format);
> > -	if (format < 0)
> > -		return format;
> > +	/* We shouldn't be able to create a fb for an unsupported format */
> > +	WARN_ON(format < 0);
> 
> What happened to Greg's
> "But don't add new WARN() calls please, just properly clean up and handle
> the error." [0]
> 
> Also, switching to WARN_ON would then continue the atomic_check function
> where it currently does exit with a real error code?

So while I can live with WARN_ON as something that should never ever
happen, I still think atomic_check should follow its function and report
the error upwards like:

if (WARN_ON(format < 0))
  return format;


Heiko

> [0] https://lwn.net/ml/linux-kernel/2024041544-fester-undead-7949@gregkh/
> 

Re: [PATCH 06/13] drm/rockchip: Switch impossible format conditional to WARN_ON

[Daniel Stone]

Hi,

On Mon, 20 Oct 2025 at 14:25, Heiko Stuebner <heiko@sntech.de> wrote:
> Am Montag, 20. Oktober 2025, 15:10:31 Mitteleuropäische Sommerzeit schrieb Heiko Stuebner:
> > Am Mittwoch, 15. Oktober 2025, 13:00:35 Mitteleuropäische Sommerzeit schrieb Daniel Stone:
> > >     format = vop2_convert_format(fb->format->format);
> > > -   if (format < 0)
> > > -           return format;
> > > +   /* We shouldn't be able to create a fb for an unsupported format */
> > > +   WARN_ON(format < 0);
> >
> > What happened to Greg's
> > "But don't add new WARN() calls please, just properly clean up and handle
> > the error." [0]
> >
> > Also, switching to WARN_ON would then continue the atomic_check function
> > where it currently does exit with a real error code?
>
> So while I can live with WARN_ON as something that should never ever
> happen

Right, I'm following the clarifications from jgg and Kees in that
thread, namely that WARN_ON is being used for 'how on earth did this
happen, the code is somehow completely broken' situations that
userspace should never be able to trigger under any circumstances.

> I still think atomic_check should follow its function and report
> the error upwards like:
>
> if (WARN_ON(format < 0))
>   return format;

Happy to do this if you prefer.

Cheers,
Daniel

Re: [PATCH 06/13] drm/rockchip: Switch impossible format conditional to WARN_ON

[Heiko Stuebner]

Am Montag, 20. Oktober 2025, 15:47:16 Mitteleuropäische Sommerzeit schrieb Daniel Stone:
> Hi,
> 
> On Mon, 20 Oct 2025 at 14:25, Heiko Stuebner <heiko@sntech.de> wrote:
> > Am Montag, 20. Oktober 2025, 15:10:31 Mitteleuropäische Sommerzeit schrieb Heiko Stuebner:
> > > Am Mittwoch, 15. Oktober 2025, 13:00:35 Mitteleuropäische Sommerzeit schrieb Daniel Stone:
> > > >     format = vop2_convert_format(fb->format->format);
> > > > -   if (format < 0)
> > > > -           return format;
> > > > +   /* We shouldn't be able to create a fb for an unsupported format */
> > > > +   WARN_ON(format < 0);
> > >
> > > What happened to Greg's
> > > "But don't add new WARN() calls please, just properly clean up and handle
> > > the error." [0]
> > >
> > > Also, switching to WARN_ON would then continue the atomic_check function
> > > where it currently does exit with a real error code?
> >
> > So while I can live with WARN_ON as something that should never ever
> > happen
> 
> Right, I'm following the clarifications from jgg and Kees in that
> thread, namely that WARN_ON is being used for 'how on earth did this
> happen, the code is somehow completely broken' situations that
> userspace should never be able to trigger under any circumstances.
> 
> > I still think atomic_check should follow its function and report
> > the error upwards like:
> >
> > if (WARN_ON(format < 0))
> >   return format;
> 
> Happy to do this if you prefer.

I do :-) .

In the same line as the rest of your series does get rid of strange
clutches that try to work around impossible situations, if the format
thing is wrong, we should not continue, but fail the check function,
as we did before.


Heiko

[PATCH 07/13] drm/rockchip: Switch impossible pos conditional to WARN_ON

[Daniel Stone]

We already clip the plane to the display bounds in atomic_check, and
ensure that it is sufficiently sized. Instead of trying to catch this
and adjust for it in atomic_update, just assert that atomic_check has
done its job.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 28 ++++++--------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 8c5042fb2e7e..812a46032396 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -1212,28 +1212,16 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
 	src_w = drm_rect_width(src) >> 16;
 	src_h = drm_rect_height(src) >> 16;
 	dsp_w = drm_rect_width(dest);
-
-	if (dest->x1 + dsp_w > adjusted_mode->hdisplay) {
-		drm_dbg_kms(vop2->drm,
-			    "vp%d %s dest->x1[%d] + dsp_w[%d] exceed mode hdisplay[%d]\n",
-			    vp->id, win->data->name, dest->x1, dsp_w, adjusted_mode->hdisplay);
-		dsp_w = adjusted_mode->hdisplay - dest->x1;
-		if (dsp_w < 4)
-			dsp_w = 4;
-		src_w = dsp_w * src_w / drm_rect_width(dest);
-	}
-
 	dsp_h = drm_rect_height(dest);
 
-	if (dest->y1 + dsp_h > adjusted_mode->vdisplay) {
-		drm_dbg_kms(vop2->drm,
-			    "vp%d %s dest->y1[%d] + dsp_h[%d] exceed mode vdisplay[%d]\n",
-			    vp->id, win->data->name, dest->y1, dsp_h, adjusted_mode->vdisplay);
-		dsp_h = adjusted_mode->vdisplay - dest->y1;
-		if (dsp_h < 4)
-			dsp_h = 4;
-		src_h = dsp_h * src_h / drm_rect_height(dest);
-	}
+	/* drm_atomic_helper_check_plane_state calls drm_rect_clip_scaled for
+	 * us, which keeps our planes bounded within the CRTC active area */
+	WARN_ON(dest->x1 + dsp_w > adjusted_mode->hdisplay);
+	WARN_ON(dest->y1 + dsp_h > adjusted_mode->vdisplay);
+	WARN_ON(dsp_w < 4);
+	WARN_ON(dsp_h < 4);
+	WARN_ON(src_w < 4);
+	WARN_ON(src_h < 4);
 
 	/*
 	 * This is workaround solution for IC design:
-- 
2.51.0

[PATCH 08/13] drm/rockchip: Fix eSmart test condition

[Daniel Stone]

If we want to find out if a window is eSmart or not, test for not being
a cluster window, rather than AFBDC presence.

No functional effect as only cluster windows support AFBC decode.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 812a46032396..f8039dc0e829 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -1227,12 +1227,10 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
 	 * This is workaround solution for IC design:
 	 * esmart can't support scale down when src_w % 16 == 1.
 	 */
-	if (!(win->data->feature & WIN_FEATURE_AFBDC)) {
-		if (src_w > dsp_w && (src_w & 0xf) == 1) {
-			drm_dbg_kms(vop2->drm, "vp%d %s act_w[%d] MODE 16 == 1\n",
-				    vp->id, win->data->name, src_w);
-			src_w -= 1;
-		}
+	if (!vop2_cluster_window(win) && src_w > dsp_w && (src_w & 1)) {
+		drm_dbg_kms(vop2->drm, "vp%d %s act_w[%d] MODE 16 == 1\n",
+			    vp->id, win->data->name, src_w);
+		src_w -= 1;
 	}
 
 	if (afbc_en && src_w % 4) {
-- 
2.51.0

Re:[PATCH 08/13] drm/rockchip: Fix eSmart test condition

[Andy Yan]

Hello Daniel，

At 2025-10-15 19:00:37, "Daniel Stone" <daniels@collabora.com> wrote:
>If we want to find out if a window is eSmart or not, test for not being

Please s/eSmart/Esmart， keep the naming style consistent with others.

>a cluster window, rather than AFBDC presence.


>
>No functional effect as only cluster windows support AFBC decode.
>
>Signed-off-by: Daniel Stone <daniels@collabora.com>
>---
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>index 812a46032396..f8039dc0e829 100644
>--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>@@ -1227,12 +1227,10 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
> 	 * This is workaround solution for IC design:
> 	 * esmart can't support scale down when src_w % 16 == 1.
> 	 */
>-	if (!(win->data->feature & WIN_FEATURE_AFBDC)) {
>-		if (src_w > dsp_w && (src_w & 0xf) == 1) {
>-			drm_dbg_kms(vop2->drm, "vp%d %s act_w[%d] MODE 16 == 1\n",
>-				    vp->id, win->data->name, src_w);
>-			src_w -= 1;
>-		}
>+	if (!vop2_cluster_window(win) && src_w > dsp_w && (src_w & 1)) {
>+		drm_dbg_kms(vop2->drm, "vp%d %s act_w[%d] MODE 16 == 1\n",
>+			    vp->id, win->data->name, src_w);
>+		src_w -= 1;
> 	}
> 
> 	if (afbc_en && src_w % 4) {
>-- 
>2.51.0

[PATCH 09/13] drm/rockchip: Enforce scaling workaround in plane_check

[Daniel Stone]

It seems only cluster windows are capable of applying downscaling when
the source region has an odd width. Instead of applying a workaround
inside atomic_update, fail the plane check if this is requested.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 21 ++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index f8039dc0e829..65437437e3d5 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -997,6 +997,7 @@ static int vop2_plane_atomic_check(struct drm_plane *plane,
 	struct drm_crtc *crtc = pstate->crtc;
 	struct drm_crtc_state *cstate;
 	struct vop2_video_port *vp;
+	struct vop2_win *win = to_vop2_win(plane);
 	struct vop2 *vop2;
 	const struct vop2_data *vop2_data;
 	struct drm_rect *dest = &pstate->dst;
@@ -1063,6 +1064,16 @@ static int vop2_plane_atomic_check(struct drm_plane *plane,
 		return -EINVAL;
 	}
 
+	/*
+	 * This is workaround solution for IC design:
+	 * esmart can't support scale down when src_w % 16 == 1.
+	 */
+	if (!vop2_cluster_window(win) && src_w > dsp_w && (src_w & 1)) {
+		drm_dbg_kms(vop2->drm,
+			    "eSmart windows cannot downscale odd-width source regions\n");
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
@@ -1223,16 +1234,6 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
 	WARN_ON(src_w < 4);
 	WARN_ON(src_h < 4);
 
-	/*
-	 * This is workaround solution for IC design:
-	 * esmart can't support scale down when src_w % 16 == 1.
-	 */
-	if (!vop2_cluster_window(win) && src_w > dsp_w && (src_w & 1)) {
-		drm_dbg_kms(vop2->drm, "vp%d %s act_w[%d] MODE 16 == 1\n",
-			    vp->id, win->data->name, src_w);
-		src_w -= 1;
-	}
-
 	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);
-- 
2.51.0

Re:[PATCH 09/13] drm/rockchip: Enforce scaling workaround in plane_check

[Andy Yan]

Hello Daniel，

The subject should preferably be like this： "drm/rockchip: vop2:  ......."
consistent with other commits.


At 2025-10-15 19:00:38, "Daniel Stone" <daniels@collabora.com> wrote:
>It seems only cluster windows are capable of applying downscaling when
>the source region has an odd width. Instead of applying a workaround
>inside atomic_update, fail the plane check if this is requested.
>
>Signed-off-by: Daniel Stone <daniels@collabora.com>
>---
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 21 ++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>index f8039dc0e829..65437437e3d5 100644
>--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>@@ -997,6 +997,7 @@ static int vop2_plane_atomic_check(struct drm_plane *plane,
> 	struct drm_crtc *crtc = pstate->crtc;
> 	struct drm_crtc_state *cstate;
> 	struct vop2_video_port *vp;
>+	struct vop2_win *win = to_vop2_win(plane);
> 	struct vop2 *vop2;
> 	const struct vop2_data *vop2_data;
> 	struct drm_rect *dest = &pstate->dst;
>@@ -1063,6 +1064,16 @@ static int vop2_plane_atomic_check(struct drm_plane *plane,
> 		return -EINVAL;
> 	}
> 
>+	/*
>+	 * This is workaround solution for IC design:
>+	 * esmart can't support scale down when src_w % 16 == 1.
>+	 */
>+	if (!vop2_cluster_window(win) && src_w > dsp_w && (src_w & 1)) {
>+		drm_dbg_kms(vop2->drm,
>+			    "eSmart windows cannot downscale odd-width source regions\n");
>+		return -EINVAL;
>+	}
>+
> 	return 0;
> }
> 
>@@ -1223,16 +1234,6 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
> 	WARN_ON(src_w < 4);
> 	WARN_ON(src_h < 4);
> 
>-	/*
>-	 * This is workaround solution for IC design:
>-	 * esmart can't support scale down when src_w % 16 == 1.
>-	 */
>-	if (!vop2_cluster_window(win) && src_w > dsp_w && (src_w & 1)) {
>-		drm_dbg_kms(vop2->drm, "vp%d %s act_w[%d] MODE 16 == 1\n",
>-			    vp->id, win->data->name, src_w);
>-		src_w -= 1;
>-	}

There is no definition for dsp_w in this function now,  it will cause compile error for this single patch.
you can use dest_w here.

>-
> 	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);
>-- 
>2.51.0

[PATCH 10/13] drm/rockchip: Enforce AFBC source alignment in plane_check

[Daniel Stone]

Planes can only source AFBC framebuffers at multiples of 4px wide.
Instead of clipping when the user asks for an unaligned source
rectangle, reject the configuration in the plane's atomic check.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 65437437e3d5..0abaf3e0eab6 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -1068,12 +1068,19 @@ static int vop2_plane_atomic_check(struct drm_plane *plane,
 	 * This is workaround solution for IC design:
 	 * esmart can't support scale down when src_w % 16 == 1.
 	 */
-	if (!vop2_cluster_window(win) && src_w > dsp_w && (src_w & 1)) {
+	if (!vop2_cluster_window(win) && src_w > dest_w && (src_w & 1)) {
 		drm_dbg_kms(vop2->drm,
 			    "eSmart windows cannot downscale odd-width source regions\n");
 		return -EINVAL;
 	}
 
+	if (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;
 }
 
@@ -1234,11 +1241,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 (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);
-- 
2.51.0

Re:[PATCH 10/13] drm/rockchip: Enforce AFBC source alignment in plane_check

[Andy Yan]

Hello,

At 2025-10-15 19:00:39, "Daniel Stone" <daniels@collabora.com> wrote:
>Planes can only source AFBC framebuffers at multiples of 4px wide.
>Instead of clipping when the user asks for an unaligned source
>rectangle, reject the configuration in the plane's atomic check.
>
>Signed-off-by: Daniel Stone <daniels@collabora.com>
>---
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>index 65437437e3d5..0abaf3e0eab6 100644
>--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>@@ -1068,12 +1068,19 @@ static int vop2_plane_atomic_check(struct drm_plane *plane,
> 	 * This is workaround solution for IC design:
> 	 * esmart can't support scale down when src_w % 16 == 1.
> 	 */
>-	if (!vop2_cluster_window(win) && src_w > dsp_w && (src_w & 1)) {
>+	if (!vop2_cluster_window(win) && src_w > dest_w && (src_w & 1)) {
> 		drm_dbg_kms(vop2->drm,
> 			    "eSmart windows cannot downscale odd-width source regions\n");
> 		return -EINVAL;
> 	}
> 
>+	if (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;
>+	}

Just as a note here:
After applying this patch, all weston-simple-egl tests under Weston will fall back to GPU compositing mode. 
This is because the rendered boxes in weston-simple-egl have a fixed size of 250x250, which is not aligned to 4 pixels and cannot be adjusted.


>+
> 	return 0;
> }
> 
>@@ -1234,11 +1241,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 (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);
>-- 
>2.51.0

Re:Re:[PATCH 10/13] drm/rockchip: Enforce AFBC source alignment in plane_check

[Andy Yan]

Hello Daniel，

At 2025-11-12 16:21:03, "Andy Yan" <andyshrk@163.com> wrote:
>
>Hello,
>
>At 2025-10-15 19:00:39, "Daniel Stone" <daniels@collabora.com> wrote:
>>Planes can only source AFBC framebuffers at multiples of 4px wide.
>>Instead of clipping when the user asks for an unaligned source
>>rectangle, reject the configuration in the plane's atomic check.
>>
>>Signed-off-by: Daniel Stone <daniels@collabora.com>
>>---
>> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 16 ++++++++++------
>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>index 65437437e3d5..0abaf3e0eab6 100644
>>--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>@@ -1068,12 +1068,19 @@ static int vop2_plane_atomic_check(struct drm_plane *plane,
>> 	 * This is workaround solution for IC design:
>> 	 * esmart can't support scale down when src_w % 16 == 1.
>> 	 */
>>-	if (!vop2_cluster_window(win) && src_w > dsp_w && (src_w & 1)) {
>>+	if (!vop2_cluster_window(win) && src_w > dest_w && (src_w & 1)) {
>> 		drm_dbg_kms(vop2->drm,
>> 			    "eSmart windows cannot downscale odd-width source regions\n");
>> 		return -EINVAL;
>> 	}
>> 
>>+	if (drm_is_afbc(fb->modifier) && src_w % 4) {
>>+		drm_dbg_kms(vop2->drm,
>>+			    "AFBC source rectangles must be 4-byte aligned; is %d\n",
  
  s/4-byte/4 pixel/

>>+			    src_w);
>>+		return -EINVAL;
>>+	}
>

Just as a note here:
After applying this patch, all weston-simple-egl tests under Weston will fall back to GPU compositing mode. 
This is because the rendered boxes in weston-simple-egl have a fixed size of 250x250, which is not aligned to 4 pixels and cannot be adjusted.

>
>
>>+
>> 	return 0;
>> }
>> 
>>@@ -1234,11 +1241,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 (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);
>>-- 
>>2.51.0

Re:Re:Re:[PATCH 10/13] drm/rockchip: Enforce AFBC source alignment in plane_check

[Andy Yan]

Hello Daniel,

在 2025-11-12 16:34:19，"Andy Yan" <andyshrk@163.com> 写道：
>
>
>Hello Daniel，
>
>At 2025-11-12 16:21:03, "Andy Yan" <andyshrk@163.com> wrote:
>>
>>Hello,
>>
>>At 2025-10-15 19:00:39, "Daniel Stone" <daniels@collabora.com> wrote:
>>>Planes can only source AFBC framebuffers at multiples of 4px wide.
>>>Instead of clipping when the user asks for an unaligned source
>>>rectangle, reject the configuration in the plane's atomic check.
>>>
>>>Signed-off-by: Daniel Stone <daniels@collabora.com>
>>>---
>>> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 16 ++++++++++------
>>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>>
>>>diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>index 65437437e3d5..0abaf3e0eab6 100644
>>>--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>@@ -1068,12 +1068,19 @@ static int vop2_plane_atomic_check(struct drm_plane *plane,
>>> 	 * This is workaround solution for IC design:
>>> 	 * esmart can't support scale down when src_w % 16 == 1.
>>> 	 */
>>>-	if (!vop2_cluster_window(win) && src_w > dsp_w && (src_w & 1)) {
>>>+	if (!vop2_cluster_window(win) && src_w > dest_w && (src_w & 1)) {
>>> 		drm_dbg_kms(vop2->drm,
>>> 			    "eSmart windows cannot downscale odd-width source regions\n");
>>> 		return -EINVAL;
>>> 	}
>>> 
>>>+	if (drm_is_afbc(fb->modifier) && src_w % 4) {
>>>+		drm_dbg_kms(vop2->drm,
>>>+			    "AFBC source rectangles must be 4-byte aligned; is %d\n",
>  
>  s/4-byte/4 pixel/

I've checked our BSP code, and this limitation only applies to RK3568. 
Other SoCs like RK3588/RK3576 don't have this limitation.


>
>>>+			    src_w);
>>>+		return -EINVAL;
>>>+	}
>>
>
>Just as a note here:
>After applying this patch, all weston-simple-egl tests under Weston will fall back to GPU compositing mode. 
>This is because the rendered boxes in weston-simple-egl have a fixed size of 250x250, which is not aligned to 4 pixels and cannot be adjusted.
>
>>
>>
>>>+
>>> 	return 0;
>>> }
>>> 
>>>@@ -1234,11 +1241,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 (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);
>>>-- 
>>>2.51.0

[PATCH 11/13] drm/rockchip: Enforce AFBC transform stride align in plane_check

[Daniel Stone]

Make sure we can't break the hardware by requesting an unsupported
configuration.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 0abaf3e0eab6..36e5864f7e37 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -1081,6 +1081,15 @@ static int vop2_plane_atomic_check(struct drm_plane *plane,
 		return -EINVAL;
 	}
 
+	if (drm_is_afbc(fb->modifier) &&
+	    pstate->rotation &
+		(DRM_MODE_REFLECT_X | DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270) &&
+	    (fb->pitches[0] << 3) / vop2_get_bpp(fb->format) % 64) {
+		drm_dbg_kms(vop2->drm,
+			    "AFBC buffers must be 64-byte aligned for horizontal rotation or mirroring\n");
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
@@ -1287,9 +1296,6 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
 		 * with WIN_VIR_STRIDE.
 		 */
 		stride = (fb->pitches[0] << 3) / bpp;
-		if ((stride & 0x3f) && (xmirror || rotate_90 || rotate_270))
-			drm_dbg_kms(vop2->drm, "vp%d %s stride[%d] not 64 pixel aligned\n",
-				    vp->id, win->data->name, stride);
 
 		 /* It's for head stride, each head size is 16 byte */
 		stride = ALIGN(stride, block_w) / block_w * 16;
-- 
2.51.0

[PATCH 12/13] drm/rockchip: Use drm_is_afbc helper function

[Daniel Stone]

We don't need to do a long open-coded walk here; we can simply check the
modifier value.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 36e5864f7e37..e2a18c303357 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -1205,7 +1205,7 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
 		return;
 	}
 
-	afbc_en = rockchip_afbc(plane, fb->modifier);
+	afbc_en = drm_is_afbc(fb->modifier);
 
 	offset = (src->x1 >> 16) * fb->format->cpp[0];
 
-- 
2.51.0

[PATCH 13/13] drm/rockchip: Simplify format_mod_supported

[Daniel Stone]

Make it a little less convoluted, and just directly check if the
combination of plane + format + modifier is supported.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 56 ++++++++------------
 1 file changed, 22 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index e2a18c303357..fb3e10172942 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -366,59 +366,47 @@ static bool is_yuv_output(u32 bus_format)
 	}
 }
 
-static bool rockchip_afbc(struct drm_plane *plane, u64 modifier)
-{
-	int i;
-
-	if (modifier == DRM_FORMAT_MOD_LINEAR)
-		return false;
-
-	for (i = 0 ; i < plane->modifier_count; i++)
-		if (plane->modifiers[i] == modifier)
-			return true;
-
-	return false;
-}
-
 static bool rockchip_vop2_mod_supported(struct drm_plane *plane, u32 format,
 					u64 modifier)
 {
 	struct vop2_win *win = to_vop2_win(plane);
 	struct vop2 *vop2 = win->vop2;
+	int i;
 
+	/* No support for implicit modifiers */
 	if (modifier == DRM_FORMAT_MOD_INVALID)
 		return false;
 
-	if (vop2->version == VOP_VERSION_RK3568) {
-		if (vop2_cluster_window(win)) {
-			if (modifier == DRM_FORMAT_MOD_LINEAR) {
-				drm_dbg_kms(vop2->drm,
-					    "Cluster window only supports format with afbc\n");
-				return false;
-			}
-		}
+	/* The cluster window on 3568 is AFBC-only */
+	if (vop2->version == VOP_VERSION_RK3568 && vop2_cluster_window(win) &&
+	    !drm_is_afbc(modifier)) {
+		drm_dbg_kms(vop2->drm,
+			    "Cluster window only supports format with afbc\n");
+		return false;
 	}
 
-	if (format == DRM_FORMAT_XRGB2101010 || format == DRM_FORMAT_XBGR2101010) {
-		if (vop2->version == VOP_VERSION_RK3588) {
-			if (!rockchip_afbc(plane, modifier)) {
-				drm_dbg_kms(vop2->drm, "Only support 32 bpp format with afbc\n");
-				return false;
-			}
-		}
+	/* 10bpc formats on 3588 are AFBC-only */
+	if (vop2->version == VOP_VERSION_RK3588 && !drm_is_afbc(modifier) &&
+	    (format == DRM_FORMAT_XRGB2101010 || format == DRM_FORMAT_XBGR2101010)) {
+		drm_dbg_kms(vop2->drm, "Only support 10bpc format with afbc\n");
+		return false;
 	}
 
+	/* Linear is otherwise supported everywhere */
 	if (modifier == DRM_FORMAT_MOD_LINEAR)
 		return true;
 
-	if (!rockchip_afbc(plane, modifier)) {
-		drm_dbg_kms(vop2->drm, "Unsupported format modifier 0x%llx\n",
-			    modifier);
-
+	/* Not all format+modifier combinations are allowable */
+	if (vop2_convert_afbc_format(format) == VOP2_AFBC_FMT_INVALID)
 		return false;
+
+	/* Different windows have different format/modifier support */
+	for (i = 0; i < plane->modifier_count; i++) {
+		if (plane->modifiers[i] == modifier)
+			return true;
 	}
 
-	return vop2_convert_afbc_format(format) >= 0;
+	return false;
 }
 
 /*
-- 
2.51.0

Re: (subset) [PATCH 00/13] drm/rockchip: No more post-atomic_check fixups

[Heiko Stuebner]

On Wed, 15 Oct 2025 12:00:29 +0100, Daniel Stone wrote:
> This series is a pretty small and consistent one for VOP2. The atomic
> uAPI very clearly specifies that drivers should either do what userspace
> requested (on a successful commit), or fail atomic_check if it is not
> for any reason possible to do what userspace requested.
> 
> VOP2 is unfortunately littered with a bunch of cases where it will apply
> fixups after atomic_check - doing something different to what userspace
> requested, e.g. clipping or aligning regions - or throw error messages
> into the log when userspace does request a condition which can't be met.
> 
> [...]

Applied, thanks!

[01/13] drm/rockchip: Demote normal drm_err to debug
        commit: f233921d988ae6a990e76d0532b241ce3dc57c12
[02/13] drm/rockchip: Declare framebuffer width/height bounds
        commit: 4bfaa85bb5f7880ff51be1d2a2e031fe4270411a
[03/13] drm/rockchip: Return error code for errors
        commit: 70e3f77cb568e229a59c5162be717bea2e22ffd8
[04/13] drm/rockchip: Rename variables for clarity
        commit: 33cbeea62fae844574e2121e4176963e68741a4a
[05/13] drm/rockchip: Use temporary variables
        commit: 4e39740d77e9cf6c20972fde14197db7aee36f35

Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>