[PATCH RFT v2 0/5] drm: Add and use drm_fb_dma_get_gem_clipped_addr() helper

[PATCH RFT v2 0/5] drm: Add and use drm_fb_dma_get_gem_clipped_addr() helper

[Chen-Yu Tsai]

Hi,

This is v2 of my drm_fb_dma_get_gem_clipped_addr() series.

Changes since v1:
- Add and use new drm_framebuffer_get_block_offset() helper (Thomas)


This series adds a helper to retrieve the buffer starting address of a
"clipped" framebuffer. This contrasts with drm_fb_dma_get_gem_addr(),
which gives the address of the full buffer.

Some drivers program their hardware with clipped dimensions, so they
should be using the clipped buffer address as well, unless the hardware
can advance the scanout directly. (Side note: many drivers still use
the non-clipped dimensions.)

While at it, also pull out the offset calculation of drm_fb_dma_get_gem_addr()
into a separate helper in drm_framebuffer.[ch], thereby separating
responsibilities.

The sun4i driver was recently incorrectly converted to use the unclipped
drm_fb_dma_get_gem_addr() helper. This broke offsets into subsampled
pixel groups, but also exposed the mismatch between the dimensions used
vs the buffer address. Two other drivers were also touched.


Patch 1 adds a new helper to return the byte offset into a framebuffer
for the start of the pixel block of the given pixel coordinates.

Patch 2 adds the new helper to return the buffer address based on
clipped coordinates.

Patch 3 switches the sun4i driver to the new helper, and fixes the
luma plane buffer address offset for subsampled YUV formats.

Patch 4 converts the imx/dc driver to use the new helper. This fixes a
mismatch between the programmed coordinates and the buffer address.

Patch 5 replaces the open coded buffer address calculation in the
imx/dcss driver with the new helper. Existing behavior, which might be
wrong, is preserved.


Please help test. The series is only compile tested on my end. The sun4i
changes should revert its behavior to before the drm_fb_dma_get_gem_addr()
was adopted. The imx/dcss changes should not have any behavioral
difference.


Thanks
ChenYu

Chen-Yu Tsai (5):
  drm: Split framebuffer pixel offset calculation from
    drm_fb_dma_get_gem_addr()
  drm/fb-dma-helper: Add drm_fb_dma_get_gem_clipped_addr()
  drm/sun4i: layers: Fix VI buffer address for clipped offsets
  drm/imx/dc: plane: Switch to drm_fb_dma_get_gem_clipped_addr()
  drm/imx/dcss: plane: Switch to drm_fb_dma_get_gem_clipped_addr()

 drivers/gpu/drm/drm_fb_dma_helper.c    | 61 ++++++++++++++------------
 drivers/gpu/drm/drm_framebuffer.c      | 45 +++++++++++++++++++
 drivers/gpu/drm/imx/dc/dc-plane.c      |  4 +-
 drivers/gpu/drm/imx/dcss/dcss-plane.c  | 34 ++++++--------
 drivers/gpu/drm/sun4i/sun8i_ui_layer.c |  2 +-
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 16 ++++++-
 include/drm/drm_fb_dma_helper.h        |  4 ++
 include/drm/drm_framebuffer.h          |  3 ++
 8 files changed, 118 insertions(+), 51 deletions(-)

-- 
2.55.0.1032.g73a4cd73de-goog

[PATCH RFT v2 1/5] drm: Split framebuffer pixel offset calculation from drm_fb_dma_get_gem_addr()

[Chen-Yu Tsai]

Currently drm_fb_dma_get_gem_addr() calculates the offset into the
framebuffer memory for the framebuffer's unclipped source coordinates,
adds that to the framebuffer's backing storage, and returns the result.

We are about to add a variant that uses the clipped source coordinates,
so there is already some reuse of code. However, calculating the data
offset for a given pixel is not specific to the DMA FB helpers. The
offset is only related to the framebuffer.

Split out the offset calculation into a new framebuffer helper so that
non-DMA users can also reuse the same code.

Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: <stable@vger.kernel.org> # dependency for next patch
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
Changes since v1:
- New patch
---
 drivers/gpu/drm/drm_fb_dma_helper.c | 28 ++----------------
 drivers/gpu/drm/drm_framebuffer.c   | 45 +++++++++++++++++++++++++++++
 include/drm/drm_framebuffer.h       |  3 ++
 3 files changed, 51 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c
index fd71969d2fb1..ab0f37d8a5ff 100644
--- a/drivers/gpu/drm/drm_fb_dma_helper.c
+++ b/drivers/gpu/drm/drm_fb_dma_helper.c
@@ -75,36 +75,14 @@ dma_addr_t drm_fb_dma_get_gem_addr(struct drm_framebuffer *fb,
 				   unsigned int plane)
 {
 	struct drm_gem_dma_object *obj;
-	dma_addr_t dma_addr;
-	u8 h_div = 1, v_div = 1;
-	u32 block_w = drm_format_info_block_width(fb->format, plane);
-	u32 block_h = drm_format_info_block_height(fb->format, plane);
-	u32 block_size = fb->format->char_per_block[plane];
-	u32 sample_x;
-	u32 sample_y;
-	u32 block_start_y;
-	u32 num_hblocks;
 
 	obj = drm_fb_dma_get_gem_obj(fb, plane);
 	if (!obj)
 		return 0;
 
-	dma_addr = obj->dma_addr + fb->offsets[plane];
-
-	if (plane > 0) {
-		h_div = fb->format->hsub;
-		v_div = fb->format->vsub;
-	}
-
-	sample_x = (state->src_x >> 16) / h_div;
-	sample_y = (state->src_y >> 16) / v_div;
-	block_start_y = (sample_y / block_h) * block_h;
-	num_hblocks = sample_x / block_w;
-
-	dma_addr += fb->pitches[plane] * block_start_y;
-	dma_addr += block_size * num_hblocks;
-
-	return dma_addr;
+	return obj->dma_addr + drm_framebuffer_get_block_offset(fb, plane,
+								state->src_x >> 16,
+								state->src_y >> 16);
 }
 EXPORT_SYMBOL_GPL(drm_fb_dma_get_gem_addr);
 
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index d32aceb6ca9b..9e1231162047 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -1208,6 +1208,51 @@ void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
 	}
 }
 
+/**
+ * drm_framebuffer_get_block_offset() - Get offset to start of pixel block for
+ * the given framebuffer and coordinates.
+ * @fb: The framebuffer
+ * @plane: Which plane
+ * @x: x coordinate for pixel
+ * @y: y coordinate for pixel
+ *
+ * This function will usually be called from the PLANE callback functions,
+ * or from one of the helpers that calculates the framebuffer's DMA address.
+ *
+ * Return: offset from start of framebuffer to start of pixel block
+ */
+u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
+				     unsigned int x, unsigned int y)
+{
+	u8 h_div = 1, v_div = 1;
+	u32 block_w = drm_format_info_block_width(fb->format, plane);
+	u32 block_h = drm_format_info_block_height(fb->format, plane);
+	u32 block_size = fb->format->char_per_block[plane];
+	u32 sample_x;
+	u32 sample_y;
+	u32 block_start_y;
+	u32 num_hblocks;
+	u32 offset;
+
+	offset = fb->offsets[plane];
+
+	if (plane > 0) {
+		h_div = fb->format->hsub;
+		v_div = fb->format->vsub;
+	}
+
+	sample_x = x / h_div;
+	sample_y = y / v_div;
+	block_start_y = (sample_y / block_h) * block_h;
+	num_hblocks = sample_x / block_w;
+
+	offset += fb->pitches[plane] * block_start_y;
+	offset += block_size * num_hblocks;
+
+	return offset;
+}
+EXPORT_SYMBOL(drm_framebuffer_get_block_offset);
+
 #ifdef CONFIG_DEBUG_FS
 static int drm_framebuffer_info(struct seq_file *m, void *data)
 {
diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
index 38b24fc8978d..c07aea1cc59f 100644
--- a/include/drm/drm_framebuffer.h
+++ b/include/drm/drm_framebuffer.h
@@ -220,6 +220,9 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb);
 void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
 void drm_framebuffer_unregister_private(struct drm_framebuffer *fb);
 
+u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
+				     unsigned int x, unsigned int y);
+
 /**
  * drm_framebuffer_get - acquire a framebuffer reference
  * @fb: DRM framebuffer
-- 
2.55.0.1032.g73a4cd73de-goog

[PATCH RFT v2 2/5] drm/fb-dma-helper: Add drm_fb_dma_get_gem_clipped_addr()

[Chen-Yu Tsai]

drm_fb_dma_get_gem_addr() returns the DMA address to the "unclipped"
framebuffer. However some display drivers want the "clipped" framebuffer
instead, as they are also using the clipped coordinates to program the
hardware.

Some of these drivers are open-coding drm_fb_dma_get_gem_addr() with
the source coordinates replaced, while others have been incorrectly
converted to using drm_fb_dma_get_gem_addr(), which would end up
causing incorrect parts of the framebuffer to be displayed if it were
somehow clipped.

Add drm_fb_dma_get_gem_clipped_addr(), a "clipped" version of
drm_fb_dma_get_gem_addr() for these drivers to use.

Cc: <stable@vger.kernel.org> # dependency for next patch
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
Changes since v1:
- Use new drm_framebuffer_get_block_offset() helper
---
 drivers/gpu/drm/drm_fb_dma_helper.c | 37 +++++++++++++++++++++++++----
 include/drm/drm_fb_dma_helper.h     |  4 ++++
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c
index ab0f37d8a5ff..0aaf4926db4e 100644
--- a/drivers/gpu/drm/drm_fb_dma_helper.c
+++ b/drivers/gpu/drm/drm_fb_dma_helper.c
@@ -60,15 +60,16 @@ struct drm_gem_dma_object *drm_fb_dma_get_gem_obj(struct drm_framebuffer *fb,
 EXPORT_SYMBOL_GPL(drm_fb_dma_get_gem_obj);
 
 /**
- * drm_fb_dma_get_gem_addr() - Get DMA (bus) address for framebuffer, for pixel
- * formats where values are grouped in blocks this will get you the beginning of
- * the block
+ * drm_fb_dma_get_gem_addr() - Get DMA (bus) address for unclipped framebuffer,
+ * for pixel formats where values are grouped in blocks this will get you the
+ * beginning of the block
  * @fb: The framebuffer
  * @state: Which state of drm plane
  * @plane: Which plane
- * Return the DMA GEM address for given framebuffer.
  *
  * This function will usually be called from the PLANE callback functions.
+ *
+ * Return: GEM DMA address for given framebuffer, unclipped.
  */
 dma_addr_t drm_fb_dma_get_gem_addr(struct drm_framebuffer *fb,
 				   struct drm_plane_state *state,
@@ -86,6 +87,34 @@ dma_addr_t drm_fb_dma_get_gem_addr(struct drm_framebuffer *fb,
 }
 EXPORT_SYMBOL_GPL(drm_fb_dma_get_gem_addr);
 
+/**
+ * drm_fb_dma_get_gem_clipped_addr() - Get DMA (bus) address for clipped
+ * framebuffer, for pixel formats where values are grouped in blocks this
+ * will get you the beginning of the block
+ * @fb: The framebuffer
+ * @state: Which state of drm plane
+ * @plane: Which plane
+ *
+ * This function will usually be called from the PLANE callback functions.
+ *
+ * Return: GEM DMA address for given framebuffer, clipped.
+ */
+dma_addr_t drm_fb_dma_get_gem_clipped_addr(struct drm_framebuffer *fb,
+					   struct drm_plane_state *state,
+					   unsigned int plane)
+{
+	struct drm_gem_dma_object *obj;
+
+	obj = drm_fb_dma_get_gem_obj(fb, plane);
+	if (!obj)
+		return 0;
+
+	return obj->dma_addr + drm_framebuffer_get_block_offset(fb, plane,
+								state->src.x1 >> 16,
+								state->src.y1 >> 16);
+}
+EXPORT_SYMBOL_GPL(drm_fb_dma_get_gem_clipped_addr);
+
 /**
  * drm_fb_dma_sync_non_coherent - Sync GEM object to non-coherent backing
  *	memory
diff --git a/include/drm/drm_fb_dma_helper.h b/include/drm/drm_fb_dma_helper.h
index c950732c6d36..b2a0bd7ef9d0 100644
--- a/include/drm/drm_fb_dma_helper.h
+++ b/include/drm/drm_fb_dma_helper.h
@@ -17,6 +17,10 @@ dma_addr_t drm_fb_dma_get_gem_addr(struct drm_framebuffer *fb,
 				   struct drm_plane_state *state,
 				   unsigned int plane);
 
+dma_addr_t drm_fb_dma_get_gem_clipped_addr(struct drm_framebuffer *fb,
+					   struct drm_plane_state *state,
+					   unsigned int plane);
+
 void drm_fb_dma_sync_non_coherent(struct drm_device *drm,
 				  struct drm_plane_state *old_state,
 				  struct drm_plane_state *state);
-- 
2.55.0.1032.g73a4cd73de-goog

[PATCH RFT v2 3/5] drm/sun4i: layers: Fix VI buffer address for clipped offsets

[Chen-Yu Tsai]

Commit 79ac1c945ab8 ("drm/sun4i: layers: Use drm_fb_dma_get_gem_addr() to
get display memory") dropped the code to calculate the framebuffer's DMA
address in favor of drm_fb_dma_get_gem_addr().

This turned out to be wrong in a couple ways. The hardware is programmed
with clipped dimensions, so it needs the buffer address to start at the
clipped boundary. Moving to the helper negated the clipping. Also, when
clipping on the left, the buffer address needs to start at the first
pixel in the sub-sampling group even for the luma plane. The hardware
handles the interpolation internally.

Switch to the new drm_fb_dma_get_gem_clipped_addr(), which provides the
buffer address starting at the clipped boundary. Calculate the intra-group
offset and adjust the luma plane buffer address so that it points to the
start of the sub-sampling group.

Fixes: 79ac1c945ab8 ("drm/sun4i: layers: Use drm_fb_dma_get_gem_addr() to get display memory")
Cc: <stable@vger.kernel.org> # v7.1+, needs drm_fb_dma_get_gem_clipped_addr()
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
This is an alternative to Jernej's original revert:
https://lore.kernel.org/all/3980ea1aeb3f7fe8b4700e36560deeba3d050664.1785772659.git.jernej.skrabec@gmail.com/
---
 drivers/gpu/drm/sun4i/sun8i_ui_layer.c |  2 +-
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 16 +++++++++++++++-
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
index bad102134726..530efae7e13c 100644
--- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
@@ -130,7 +130,7 @@ static void sun8i_ui_layer_update_buffer(struct sun8i_layer *layer,
 	ch_base = sun8i_channel_base(layer);
 
 	/* Get the start of the displayed memory */
-	dma_addr = drm_fb_dma_get_gem_addr(fb, state, 0);
+	dma_addr = drm_fb_dma_get_gem_clipped_addr(fb, state, 0);
 
 	/* Set the line width */
 	DRM_DEBUG_DRIVER("Layer line width: %d bytes\n", fb->pitches[0]);
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
index 2e9cda45c04e..7a1d5f1db037 100644
--- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
@@ -205,7 +205,21 @@ static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer,
 
 	for (i = 0; i < format->num_planes; i++) {
 		/* Get the start of the displayed memory */
-		dma_addr = drm_fb_dma_get_gem_addr(fb, state, i);
+		dma_addr = drm_fb_dma_get_gem_clipped_addr(fb, state, i);
+
+		/*
+		 * The mixer can handle odd offsets into sub-sampled YUV
+		 * planes, but needs the address of the first pixel in each
+		 * sub-sampled block. Adjust the luma buffer address backwards.
+		 */
+		if (i == 0) {
+			u32 x_diff, y_diff;
+
+			x_diff = (state->src.x1 >> 16) & (format->hsub - 1);
+			y_diff = (state->src.y1 >> 16) & (format->vsub - 1);
+			dma_addr -= y_diff * fb->pitches[i];
+			dma_addr -= x_diff * format->cpp[i];
+		}
 
 		/* Set the line width */
 		DRM_DEBUG_DRIVER("Layer %d. line width: %d bytes\n",
-- 
2.55.0.1032.g73a4cd73de-goog

[PATCH RFT v2 4/5] drm/imx/dc: plane: Switch to drm_fb_dma_get_gem_clipped_addr()

[Chen-Yu Tsai]

The hardware is programmed with clipped source and destination dimensions,
but the framebuffer address is calculated using drm_fb_dma_get_gem_addr(),
which uses the full source dimensions. This will not match the source
offset if the top and/or left sides are clipped.

Switch to the new drm_fb_dma_get_gem_clipped_addr(), which provides the
buffer address starting at the clipped boundary.

Fixes: 711a3b878366 ("drm/imx: Add i.MX8qxp Display Controller KMS")
Cc: <stable@vger.kernel.org> # Needs drm_fb_dma_get_gem_clipped_addr()
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/gpu/drm/imx/dc/dc-plane.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/imx/dc/dc-plane.c b/drivers/gpu/drm/imx/dc/dc-plane.c
index 62dd0576fadc..35eb5039f5ac 100644
--- a/drivers/gpu/drm/imx/dc/dc-plane.c
+++ b/drivers/gpu/drm/imx/dc/dc-plane.c
@@ -65,7 +65,7 @@ static int dc_plane_check_max_source_resolution(struct drm_plane_state *state)
 static int dc_plane_check_fb(struct drm_plane_state *state)
 {
 	struct drm_framebuffer *fb = state->fb;
-	dma_addr_t baseaddr = drm_fb_dma_get_gem_addr(fb, state, 0);
+	dma_addr_t baseaddr = drm_fb_dma_get_gem_clipped_addr(fb, state, 0);
 
 	/* base address alignment */
 	if (baseaddr & 0x3) {
@@ -146,7 +146,7 @@ dc_plane_atomic_update(struct drm_plane *plane, struct drm_atomic_commit *state)
 	src_w = drm_rect_width(&new_state->src) >> 16;
 	src_h = drm_rect_height(&new_state->src) >> 16;
 
-	baseaddr = drm_fb_dma_get_gem_addr(fb, new_state, 0);
+	baseaddr = drm_fb_dma_get_gem_clipped_addr(fb, new_state, 0);
 
 	fu_ops = dc_fu_get_ops(dplane->fu);
 
-- 
2.55.0.1032.g73a4cd73de-goog

[PATCH RFT v2 5/5] drm/imx/dcss: plane: Switch to drm_fb_dma_get_gem_clipped_addr()

[Chen-Yu Tsai]

The i.MX DCSS driver is open coding drm_fb_dma_get_gem_clipped_addr(),
with only a slight difference of rounding down the X offset for the
first plane if the format is packed, sub-sampled YUV. This is likely
to correct the buffer address to the first pixel of the 2-pixel group.
Otherwise the hardware will start the scan-out from the second pixel,
which leads to the U/V components getting swapped around, and the
chroma component of the next pixel group being used.

Switch to drm_fb_dma_get_gem_clipped_addr(), and offset the address by
a pixel if the X offset is odd.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/gpu/drm/imx/dcss/dcss-plane.c | 34 +++++++++++----------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/imx/dcss/dcss-plane.c b/drivers/gpu/drm/imx/dcss/dcss-plane.c
index 303e93fd036c..580d9cfb4053 100644
--- a/drivers/gpu/drm/imx/dcss/dcss-plane.c
+++ b/drivers/gpu/drm/imx/dcss/dcss-plane.c
@@ -219,28 +219,22 @@ static void dcss_plane_atomic_set_base(struct dcss_plane *dcss_plane)
 	struct dcss_dev *dcss = plane->dev->dev_private;
 	struct drm_framebuffer *fb = state->fb;
 	const struct drm_format_info *format = fb->format;
-	struct drm_gem_dma_object *dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
 	unsigned long p1_ba = 0, p2_ba = 0;
 
-	if (!format->is_yuv ||
-	    format->format == DRM_FORMAT_NV12 ||
-	    format->format == DRM_FORMAT_NV21)
-		p1_ba = dma_obj->dma_addr + fb->offsets[0] +
-			fb->pitches[0] * (state->src.y1 >> 16) +
-			format->char_per_block[0] * (state->src.x1 >> 16);
-	else if (format->format == DRM_FORMAT_UYVY ||
-		 format->format == DRM_FORMAT_VYUY ||
-		 format->format == DRM_FORMAT_YUYV ||
-		 format->format == DRM_FORMAT_YVYU)
-		p1_ba = dma_obj->dma_addr + fb->offsets[0] +
-			fb->pitches[0] * (state->src.y1 >> 16) +
-			2 * format->char_per_block[0] * (state->src.x1 >> 17);
-
-	if (format->format == DRM_FORMAT_NV12 ||
-	    format->format == DRM_FORMAT_NV21)
-		p2_ba = dma_obj->dma_addr + fb->offsets[1] +
-			(((fb->pitches[1] >> 1) * (state->src.y1 >> 17) +
-			(state->src.x1 >> 17)) << 1);
+	p1_ba = drm_fb_dma_get_gem_clipped_addr(fb, state, 0);
+
+	/*
+	 * TODO fix address until helpers know packed, sub-sampled YUV format block size
+	 *
+	 * The buffer address for packed, sub-sampled YUV formats such as DRM_FORMAT_UYVY
+	 * need to be on the first pixel of each pixel group or block. Otherwise the first
+	 * pixel of the next pixel group is read and the U/V values get swapped around.
+	 */
+	if (drm_format_info_is_yuv_packed(format))
+		p1_ba -= ((state->src.x1 >> 16) & 1) * format->cpp[0];
+
+	if (format->num_planes > 1)
+		p2_ba = drm_fb_dma_get_gem_clipped_addr(fb, state, 1);
 
 	dcss_dpr_addr_set(dcss->dpr, dcss_plane->ch_num, p1_ba, p2_ba,
 			  fb->pitches[0]);
-- 
2.55.0.1032.g73a4cd73de-goog

Re: [PATCH RFT v2 0/5] drm: Add and use drm_fb_dma_get_gem_clipped_addr() helper

[Icenowy Zheng]

在 2026-09-16三的 11:33 +0800，Chen-Yu Tsai写道：
> Hi,
> 
> This is v2 of my drm_fb_dma_get_gem_clipped_addr() series.
> 
> Changes since v1:
> - Add and use new drm_framebuffer_get_block_offset() helper (Thomas)
> 
> 
> This series adds a helper to retrieve the buffer starting address of
> a
> "clipped" framebuffer. This contrasts with drm_fb_dma_get_gem_addr(),
> which gives the address of the full buffer.

Should vs_fb_get_dma_addr() in verisilicon/vs_plane.c be replaced with
this helper too?

I implemented manual framebuffer offset addition here.

Thanks,
Icenowy

> 
> Some drivers program their hardware with clipped dimensions, so they
> should be using the clipped buffer address as well, unless the
> hardware
> can advance the scanout directly. (Side note: many drivers still use
> the non-clipped dimensions.)
> 
> While at it, also pull out the offset calculation of
> drm_fb_dma_get_gem_addr()
> into a separate helper in drm_framebuffer.[ch], thereby separating
> responsibilities.
> 
> The sun4i driver was recently incorrectly converted to use the
> unclipped
> drm_fb_dma_get_gem_addr() helper. This broke offsets into subsampled
> pixel groups, but also exposed the mismatch between the dimensions
> used
> vs the buffer address. Two other drivers were also touched.
> 
> 
> Patch 1 adds a new helper to return the byte offset into a
> framebuffer
> for the start of the pixel block of the given pixel coordinates.
> 
> Patch 2 adds the new helper to return the buffer address based on
> clipped coordinates.
> 
> Patch 3 switches the sun4i driver to the new helper, and fixes the
> luma plane buffer address offset for subsampled YUV formats.
> 
> Patch 4 converts the imx/dc driver to use the new helper. This fixes
> a
> mismatch between the programmed coordinates and the buffer address.
> 
> Patch 5 replaces the open coded buffer address calculation in the
> imx/dcss driver with the new helper. Existing behavior, which might
> be
> wrong, is preserved.
> 
> 
> Please help test. The series is only compile tested on my end. The
> sun4i
> changes should revert its behavior to before the
> drm_fb_dma_get_gem_addr()
> was adopted. The imx/dcss changes should not have any behavioral
> difference.
> 
> 
> Thanks
> ChenYu
> 
> Chen-Yu Tsai (5):
>   drm: Split framebuffer pixel offset calculation from
>     drm_fb_dma_get_gem_addr()
>   drm/fb-dma-helper: Add drm_fb_dma_get_gem_clipped_addr()
>   drm/sun4i: layers: Fix VI buffer address for clipped offsets
>   drm/imx/dc: plane: Switch to drm_fb_dma_get_gem_clipped_addr()
>   drm/imx/dcss: plane: Switch to drm_fb_dma_get_gem_clipped_addr()
> 
>  drivers/gpu/drm/drm_fb_dma_helper.c    | 61 ++++++++++++++----------
> --
>  drivers/gpu/drm/drm_framebuffer.c      | 45 +++++++++++++++++++
>  drivers/gpu/drm/imx/dc/dc-plane.c      |  4 +-
>  drivers/gpu/drm/imx/dcss/dcss-plane.c  | 34 ++++++--------
>  drivers/gpu/drm/sun4i/sun8i_ui_layer.c |  2 +-
>  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 16 ++++++-
>  include/drm/drm_fb_dma_helper.h        |  4 ++
>  include/drm/drm_framebuffer.h          |  3 ++
>  8 files changed, 118 insertions(+), 51 deletions(-)

Re: [PATCH RFT v2 0/5] drm: Add and use drm_fb_dma_get_gem_clipped_addr() helper

[Chen-Yu Tsai]

On Thu, Sep 17, 2026 at 7:16 PM Icenowy Zheng <uwu@icenowy.me> wrote:
>
> 在 2026-09-16三的 11:33 +0800，Chen-Yu Tsai写道：
> > Hi,
> >
> > This is v2 of my drm_fb_dma_get_gem_clipped_addr() series.
> >
> > Changes since v1:
> > - Add and use new drm_framebuffer_get_block_offset() helper (Thomas)
> >
> >
> > This series adds a helper to retrieve the buffer starting address of
> > a
> > "clipped" framebuffer. This contrasts with drm_fb_dma_get_gem_addr(),
> > which gives the address of the full buffer.
>
> Should vs_fb_get_dma_addr() in verisilicon/vs_plane.c be replaced with
> this helper too?
>
> I implemented manual framebuffer offset addition here.

Didn't I replace vs_fb_get_dma_addr() with drm_fb_dma_get_gem_addr()
already? At the time only primary and cursor planes were supported by
the driver. The primary plane can't be clipped, and the cursor plane
had some custom clipping, but seemed to want the unclipped address.


ChenYu

> Thanks,
> Icenowy
>
> >
> > Some drivers program their hardware with clipped dimensions, so they
> > should be using the clipped buffer address as well, unless the
> > hardware
> > can advance the scanout directly. (Side note: many drivers still use
> > the non-clipped dimensions.)
> >
> > While at it, also pull out the offset calculation of
> > drm_fb_dma_get_gem_addr()
> > into a separate helper in drm_framebuffer.[ch], thereby separating
> > responsibilities.
> >
> > The sun4i driver was recently incorrectly converted to use the
> > unclipped
> > drm_fb_dma_get_gem_addr() helper. This broke offsets into subsampled
> > pixel groups, but also exposed the mismatch between the dimensions
> > used
> > vs the buffer address. Two other drivers were also touched.
> >
> >
> > Patch 1 adds a new helper to return the byte offset into a
> > framebuffer
> > for the start of the pixel block of the given pixel coordinates.
> >
> > Patch 2 adds the new helper to return the buffer address based on
> > clipped coordinates.
> >
> > Patch 3 switches the sun4i driver to the new helper, and fixes the
> > luma plane buffer address offset for subsampled YUV formats.
> >
> > Patch 4 converts the imx/dc driver to use the new helper. This fixes
> > a
> > mismatch between the programmed coordinates and the buffer address.
> >
> > Patch 5 replaces the open coded buffer address calculation in the
> > imx/dcss driver with the new helper. Existing behavior, which might
> > be
> > wrong, is preserved.
> >
> >
> > Please help test. The series is only compile tested on my end. The
> > sun4i
> > changes should revert its behavior to before the
> > drm_fb_dma_get_gem_addr()
> > was adopted. The imx/dcss changes should not have any behavioral
> > difference.
> >
> >
> > Thanks
> > ChenYu
> >
> > Chen-Yu Tsai (5):
> >   drm: Split framebuffer pixel offset calculation from
> >     drm_fb_dma_get_gem_addr()
> >   drm/fb-dma-helper: Add drm_fb_dma_get_gem_clipped_addr()
> >   drm/sun4i: layers: Fix VI buffer address for clipped offsets
> >   drm/imx/dc: plane: Switch to drm_fb_dma_get_gem_clipped_addr()
> >   drm/imx/dcss: plane: Switch to drm_fb_dma_get_gem_clipped_addr()
> >
> >  drivers/gpu/drm/drm_fb_dma_helper.c    | 61 ++++++++++++++----------
> > --
> >  drivers/gpu/drm/drm_framebuffer.c      | 45 +++++++++++++++++++
> >  drivers/gpu/drm/imx/dc/dc-plane.c      |  4 +-
> >  drivers/gpu/drm/imx/dcss/dcss-plane.c  | 34 ++++++--------
> >  drivers/gpu/drm/sun4i/sun8i_ui_layer.c |  2 +-
> >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 16 ++++++-
> >  include/drm/drm_fb_dma_helper.h        |  4 ++
> >  include/drm/drm_framebuffer.h          |  3 ++
> >  8 files changed, 118 insertions(+), 51 deletions(-)

Re: [PATCH RFT v2 0/5] drm: Add and use drm_fb_dma_get_gem_clipped_addr() helper

[Icenowy Zheng]

在 2026-09-17四的 19:42 +0800，Chen-Yu Tsai写道：
> On Thu, Sep 17, 2026 at 7:16 PM Icenowy Zheng <uwu@icenowy.me> wrote:
> > 
> > 在 2026-09-16三的 11:33 +0800，Chen-Yu Tsai写道：
> > > Hi,
> > > 
> > > This is v2 of my drm_fb_dma_get_gem_clipped_addr() series.
> > > 
> > > Changes since v1:
> > > - Add and use new drm_framebuffer_get_block_offset() helper
> > > (Thomas)
> > > 
> > > 
> > > This series adds a helper to retrieve the buffer starting address
> > > of
> > > a
> > > "clipped" framebuffer. This contrasts with
> > > drm_fb_dma_get_gem_addr(),
> > > which gives the address of the full buffer.
> > 
> > Should vs_fb_get_dma_addr() in verisilicon/vs_plane.c be replaced
> > with
> > this helper too?
> > 
> > I implemented manual framebuffer offset addition here.
> 
> Didn't I replace vs_fb_get_dma_addr() with drm_fb_dma_get_gem_addr()

Yes, it seems so. I checked newest rc, but this change is in drm-misc-
next. Sorry for the noise.

> already? At the time only primary and cursor planes were supported by
> the driver. The primary plane can't be clipped, and the cursor plane
> had some custom clipping, but seemed to want the unclipped address.

Yes it looks like thedrm_fb_dma_get_gem_addr() helper already handled
the non-clipping source offset.

Thanks,
Icenowy

> 
> 
> ChenYu
> 
> > Thanks,
> > Icenowy
> > 
> > > 
> > > Some drivers program their hardware with clipped dimensions, so
> > > they
> > > should be using the clipped buffer address as well, unless the
> > > hardware
> > > can advance the scanout directly. (Side note: many drivers still
> > > use
> > > the non-clipped dimensions.)
> > > 
> > > While at it, also pull out the offset calculation of
> > > drm_fb_dma_get_gem_addr()
> > > into a separate helper in drm_framebuffer.[ch], thereby
> > > separating
> > > responsibilities.
> > > 
> > > The sun4i driver was recently incorrectly converted to use the
> > > unclipped
> > > drm_fb_dma_get_gem_addr() helper. This broke offsets into
> > > subsampled
> > > pixel groups, but also exposed the mismatch between the
> > > dimensions
> > > used
> > > vs the buffer address. Two other drivers were also touched.
> > > 
> > > 
> > > Patch 1 adds a new helper to return the byte offset into a
> > > framebuffer
> > > for the start of the pixel block of the given pixel coordinates.
> > > 
> > > Patch 2 adds the new helper to return the buffer address based on
> > > clipped coordinates.
> > > 
> > > Patch 3 switches the sun4i driver to the new helper, and fixes
> > > the
> > > luma plane buffer address offset for subsampled YUV formats.
> > > 
> > > Patch 4 converts the imx/dc driver to use the new helper. This
> > > fixes
> > > a
> > > mismatch between the programmed coordinates and the buffer
> > > address.
> > > 
> > > Patch 5 replaces the open coded buffer address calculation in the
> > > imx/dcss driver with the new helper. Existing behavior, which
> > > might
> > > be
> > > wrong, is preserved.
> > > 
> > > 
> > > Please help test. The series is only compile tested on my end.
> > > The
> > > sun4i
> > > changes should revert its behavior to before the
> > > drm_fb_dma_get_gem_addr()
> > > was adopted. The imx/dcss changes should not have any behavioral
> > > difference.
> > > 
> > > 
> > > Thanks
> > > ChenYu
> > > 
> > > Chen-Yu Tsai (5):
> > >   drm: Split framebuffer pixel offset calculation from
> > >     drm_fb_dma_get_gem_addr()
> > >   drm/fb-dma-helper: Add drm_fb_dma_get_gem_clipped_addr()
> > >   drm/sun4i: layers: Fix VI buffer address for clipped offsets
> > >   drm/imx/dc: plane: Switch to drm_fb_dma_get_gem_clipped_addr()
> > >   drm/imx/dcss: plane: Switch to
> > > drm_fb_dma_get_gem_clipped_addr()
> > > 
> > >  drivers/gpu/drm/drm_fb_dma_helper.c    | 61 ++++++++++++++------
> > > ----
> > > --
> > >  drivers/gpu/drm/drm_framebuffer.c      | 45 +++++++++++++++++++
> > >  drivers/gpu/drm/imx/dc/dc-plane.c      |  4 +-
> > >  drivers/gpu/drm/imx/dcss/dcss-plane.c  | 34 ++++++--------
> > >  drivers/gpu/drm/sun4i/sun8i_ui_layer.c |  2 +-
> > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 16 ++++++-
> > >  include/drm/drm_fb_dma_helper.h        |  4 ++
> > >  include/drm/drm_framebuffer.h          |  3 ++
> > >  8 files changed, 118 insertions(+), 51 deletions(-)

Re: [PATCH RFT v2 1/5] drm: Split framebuffer pixel offset calculation from drm_fb_dma_get_gem_addr()

[Thomas Zimmermann]

Hi

Am 16.09.26 um 05:33 schrieb Chen-Yu Tsai:
> Currently drm_fb_dma_get_gem_addr() calculates the offset into the
> framebuffer memory for the framebuffer's unclipped source coordinates,
> adds that to the framebuffer's backing storage, and returns the result.
>
> We are about to add a variant that uses the clipped source coordinates,
> so there is already some reuse of code. However, calculating the data
> offset for a given pixel is not specific to the DMA FB helpers. The
> offset is only related to the framebuffer.
>
> Split out the offset calculation into a new framebuffer helper so that
> non-DMA users can also reuse the same code.
>
> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: <stable@vger.kernel.org> # dependency for next patch
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> Changes since v1:
> - New patch
> ---
>   drivers/gpu/drm/drm_fb_dma_helper.c | 28 ++----------------
>   drivers/gpu/drm/drm_framebuffer.c   | 45 +++++++++++++++++++++++++++++
>   include/drm/drm_framebuffer.h       |  3 ++
>   3 files changed, 51 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c
> index fd71969d2fb1..ab0f37d8a5ff 100644
> --- a/drivers/gpu/drm/drm_fb_dma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_dma_helper.c
> @@ -75,36 +75,14 @@ dma_addr_t drm_fb_dma_get_gem_addr(struct drm_framebuffer *fb,
>   				   unsigned int plane)
>   {
>   	struct drm_gem_dma_object *obj;
> -	dma_addr_t dma_addr;
> -	u8 h_div = 1, v_div = 1;
> -	u32 block_w = drm_format_info_block_width(fb->format, plane);
> -	u32 block_h = drm_format_info_block_height(fb->format, plane);
> -	u32 block_size = fb->format->char_per_block[plane];
> -	u32 sample_x;
> -	u32 sample_y;
> -	u32 block_start_y;
> -	u32 num_hblocks;
>   
>   	obj = drm_fb_dma_get_gem_obj(fb, plane);
>   	if (!obj)
>   		return 0;
>   
> -	dma_addr = obj->dma_addr + fb->offsets[plane];
> -
> -	if (plane > 0) {
> -		h_div = fb->format->hsub;
> -		v_div = fb->format->vsub;
> -	}
> -
> -	sample_x = (state->src_x >> 16) / h_div;
> -	sample_y = (state->src_y >> 16) / v_div;
> -	block_start_y = (sample_y / block_h) * block_h;
> -	num_hblocks = sample_x / block_w;
> -
> -	dma_addr += fb->pitches[plane] * block_start_y;
> -	dma_addr += block_size * num_hblocks;
> -
> -	return dma_addr;
> +	return obj->dma_addr + drm_framebuffer_get_block_offset(fb, plane,
> +								state->src_x >> 16,
> +								state->src_y >> 16);
>   }
>   EXPORT_SYMBOL_GPL(drm_fb_dma_get_gem_addr);
>   
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index d32aceb6ca9b..9e1231162047 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -1208,6 +1208,51 @@ void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
>   	}
>   }
>   
> +/**
> + * drm_framebuffer_get_block_offset() - Get offset to start of pixel block for
> + * the given framebuffer and coordinates.
> + * @fb: The framebuffer
> + * @plane: Which plane
> + * @x: x coordinate for pixel
> + * @y: y coordinate for pixel
> + *
> + * This function will usually be called from the PLANE callback functions,
> + * or from one of the helpers that calculates the framebuffer's DMA address.
> + *
> + * Return: offset from start of framebuffer to start of pixel block
> + */
> +u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
> +				     unsigned int x, unsigned int y)

Better use u64 as return type.

> +{
> +	u8 h_div = 1, v_div = 1;
> +	u32 block_w = drm_format_info_block_width(fb->format, plane);
> +	u32 block_h = drm_format_info_block_height(fb->format, plane);
> +	u32 block_size = fb->format->char_per_block[plane];
> +	u32 sample_x;
> +	u32 sample_y;
> +	u32 block_start_y;
> +	u32 num_hblocks;
> +	u32 offset;
> +
> +	offset = fb->offsets[plane];
> +
> +	if (plane > 0) {
> +		h_div = fb->format->hsub;
> +		v_div = fb->format->vsub;
> +	}
> +
> +	sample_x = x / h_div;
> +	sample_y = y / v_div;
> +	block_start_y = (sample_y / block_h) * block_h;
> +	num_hblocks = sample_x / block_w;
> +
> +	offset += fb->pitches[plane] * block_start_y;
> +	offset += block_size * num_hblocks;

User space controls the values in fb->offsets and fb->pitches.  I'm not 
sure how well they have been validated already at this point. Did you 
investigate this?

Best regards
Thomas


> +
> +	return offset;
> +}
> +EXPORT_SYMBOL(drm_framebuffer_get_block_offset);
> +
>   #ifdef CONFIG_DEBUG_FS
>   static int drm_framebuffer_info(struct seq_file *m, void *data)
>   {
> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> index 38b24fc8978d..c07aea1cc59f 100644
> --- a/include/drm/drm_framebuffer.h
> +++ b/include/drm/drm_framebuffer.h
> @@ -220,6 +220,9 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb);
>   void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
>   void drm_framebuffer_unregister_private(struct drm_framebuffer *fb);
>   
> +u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
> +				     unsigned int x, unsigned int y);
> +
>   /**
>    * drm_framebuffer_get - acquire a framebuffer reference
>    * @fb: DRM framebuffer

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)

Re: [PATCH RFT v2 1/5] drm: Split framebuffer pixel offset calculation from drm_fb_dma_get_gem_addr()

[Chen-Yu Tsai]

On Thu, Sep 17, 2026 at 11:20 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 16.09.26 um 05:33 schrieb Chen-Yu Tsai:
> > Currently drm_fb_dma_get_gem_addr() calculates the offset into the
> > framebuffer memory for the framebuffer's unclipped source coordinates,
> > adds that to the framebuffer's backing storage, and returns the result.
> >
> > We are about to add a variant that uses the clipped source coordinates,
> > so there is already some reuse of code. However, calculating the data
> > offset for a given pixel is not specific to the DMA FB helpers. The
> > offset is only related to the framebuffer.
> >
> > Split out the offset calculation into a new framebuffer helper so that
> > non-DMA users can also reuse the same code.
> >
> > Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: <stable@vger.kernel.org> # dependency for next patch
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> > Changes since v1:
> > - New patch
> > ---
> >   drivers/gpu/drm/drm_fb_dma_helper.c | 28 ++----------------
> >   drivers/gpu/drm/drm_framebuffer.c   | 45 +++++++++++++++++++++++++++++
> >   include/drm/drm_framebuffer.h       |  3 ++
> >   3 files changed, 51 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c
> > index fd71969d2fb1..ab0f37d8a5ff 100644
> > --- a/drivers/gpu/drm/drm_fb_dma_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_dma_helper.c
> > @@ -75,36 +75,14 @@ dma_addr_t drm_fb_dma_get_gem_addr(struct drm_framebuffer *fb,
> >                                  unsigned int plane)
> >   {
> >       struct drm_gem_dma_object *obj;
> > -     dma_addr_t dma_addr;
> > -     u8 h_div = 1, v_div = 1;
> > -     u32 block_w = drm_format_info_block_width(fb->format, plane);
> > -     u32 block_h = drm_format_info_block_height(fb->format, plane);
> > -     u32 block_size = fb->format->char_per_block[plane];
> > -     u32 sample_x;
> > -     u32 sample_y;
> > -     u32 block_start_y;
> > -     u32 num_hblocks;
> >
> >       obj = drm_fb_dma_get_gem_obj(fb, plane);
> >       if (!obj)
> >               return 0;
> >
> > -     dma_addr = obj->dma_addr + fb->offsets[plane];
> > -
> > -     if (plane > 0) {
> > -             h_div = fb->format->hsub;
> > -             v_div = fb->format->vsub;
> > -     }
> > -
> > -     sample_x = (state->src_x >> 16) / h_div;
> > -     sample_y = (state->src_y >> 16) / v_div;
> > -     block_start_y = (sample_y / block_h) * block_h;
> > -     num_hblocks = sample_x / block_w;
> > -
> > -     dma_addr += fb->pitches[plane] * block_start_y;
> > -     dma_addr += block_size * num_hblocks;
> > -
> > -     return dma_addr;
> > +     return obj->dma_addr + drm_framebuffer_get_block_offset(fb, plane,
> > +                                                             state->src_x >> 16,
> > +                                                             state->src_y >> 16);
> >   }
> >   EXPORT_SYMBOL_GPL(drm_fb_dma_get_gem_addr);
> >
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > index d32aceb6ca9b..9e1231162047 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -1208,6 +1208,51 @@ void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
> >       }
> >   }
> >
> > +/**
> > + * drm_framebuffer_get_block_offset() - Get offset to start of pixel block for
> > + * the given framebuffer and coordinates.
> > + * @fb: The framebuffer
> > + * @plane: Which plane
> > + * @x: x coordinate for pixel
> > + * @y: y coordinate for pixel
> > + *
> > + * This function will usually be called from the PLANE callback functions,
> > + * or from one of the helpers that calculates the framebuffer's DMA address.
> > + *
> > + * Return: offset from start of framebuffer to start of pixel block
> > + */
> > +u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
> > +                                  unsigned int x, unsigned int y)
>
> Better use u64 as return type.

To avoid overflow? Not sure who would use crazy large framebuffers, but
doesn't hurt to play it safe.

> > +{
> > +     u8 h_div = 1, v_div = 1;
> > +     u32 block_w = drm_format_info_block_width(fb->format, plane);
> > +     u32 block_h = drm_format_info_block_height(fb->format, plane);
> > +     u32 block_size = fb->format->char_per_block[plane];
> > +     u32 sample_x;
> > +     u32 sample_y;
> > +     u32 block_start_y;
> > +     u32 num_hblocks;
> > +     u32 offset;
> > +
> > +     offset = fb->offsets[plane];
> > +
> > +     if (plane > 0) {
> > +             h_div = fb->format->hsub;
> > +             v_div = fb->format->vsub;
> > +     }
> > +
> > +     sample_x = x / h_div;
> > +     sample_y = y / v_div;
> > +     block_start_y = (sample_y / block_h) * block_h;
> > +     num_hblocks = sample_x / block_w;
> > +
> > +     offset += fb->pitches[plane] * block_start_y;
> > +     offset += block_size * num_hblocks;
>
> User space controls the values in fb->offsets and fb->pitches.  I'm not
> sure how well they have been validated already at this point. Did you
> investigate this?

It wouldn't be worse than before, since this changes is purely code movement.

There are minimal sanity checks done by drm_internal_framebuffer_create()
in framebuffer_check(), such as offset overflow or pitch size too small,
but that's about it. It would be up to individual drivers to perform more
checks that match their hardware limitations.

What sort of issues are you thinking about?


ChenYu

> Best regards
> Thomas
>
>
> > +
> > +     return offset;
> > +}
> > +EXPORT_SYMBOL(drm_framebuffer_get_block_offset);
> > +
> >   #ifdef CONFIG_DEBUG_FS
> >   static int drm_framebuffer_info(struct seq_file *m, void *data)
> >   {
> > diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> > index 38b24fc8978d..c07aea1cc59f 100644
> > --- a/include/drm/drm_framebuffer.h
> > +++ b/include/drm/drm_framebuffer.h
> > @@ -220,6 +220,9 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb);
> >   void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
> >   void drm_framebuffer_unregister_private(struct drm_framebuffer *fb);
> >
> > +u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
> > +                                  unsigned int x, unsigned int y);
> > +
> >   /**
> >    * drm_framebuffer_get - acquire a framebuffer reference
> >    * @fb: DRM framebuffer
>
> --
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
> GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)
>
>

Re: [PATCH RFT v2 1/5] drm: Split framebuffer pixel offset calculation from drm_fb_dma_get_gem_addr()

[Thomas Zimmermann]

Hi

Am 18.09.26 um 06:16 schrieb Chen-Yu Tsai:
> On Thu, Sep 17, 2026 at 11:20 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 16.09.26 um 05:33 schrieb Chen-Yu Tsai:
>>> Currently drm_fb_dma_get_gem_addr() calculates the offset into the
>>> framebuffer memory for the framebuffer's unclipped source coordinates,
>>> adds that to the framebuffer's backing storage, and returns the result.
>>>
>>> We are about to add a variant that uses the clipped source coordinates,
>>> so there is already some reuse of code. However, calculating the data
>>> offset for a given pixel is not specific to the DMA FB helpers. The
>>> offset is only related to the framebuffer.
>>>
>>> Split out the offset calculation into a new framebuffer helper so that
>>> non-DMA users can also reuse the same code.
>>>
>>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Cc: <stable@vger.kernel.org> # dependency for next patch
>>> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
>>> ---
>>> Changes since v1:
>>> - New patch
>>> ---
>>>    drivers/gpu/drm/drm_fb_dma_helper.c | 28 ++----------------
>>>    drivers/gpu/drm/drm_framebuffer.c   | 45 +++++++++++++++++++++++++++++
>>>    include/drm/drm_framebuffer.h       |  3 ++
>>>    3 files changed, 51 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c
>>> index fd71969d2fb1..ab0f37d8a5ff 100644
>>> --- a/drivers/gpu/drm/drm_fb_dma_helper.c
>>> +++ b/drivers/gpu/drm/drm_fb_dma_helper.c
>>> @@ -75,36 +75,14 @@ dma_addr_t drm_fb_dma_get_gem_addr(struct drm_framebuffer *fb,
>>>                                   unsigned int plane)
>>>    {
>>>        struct drm_gem_dma_object *obj;
>>> -     dma_addr_t dma_addr;
>>> -     u8 h_div = 1, v_div = 1;
>>> -     u32 block_w = drm_format_info_block_width(fb->format, plane);
>>> -     u32 block_h = drm_format_info_block_height(fb->format, plane);
>>> -     u32 block_size = fb->format->char_per_block[plane];
>>> -     u32 sample_x;
>>> -     u32 sample_y;
>>> -     u32 block_start_y;
>>> -     u32 num_hblocks;
>>>
>>>        obj = drm_fb_dma_get_gem_obj(fb, plane);
>>>        if (!obj)
>>>                return 0;
>>>
>>> -     dma_addr = obj->dma_addr + fb->offsets[plane];
>>> -
>>> -     if (plane > 0) {
>>> -             h_div = fb->format->hsub;
>>> -             v_div = fb->format->vsub;
>>> -     }
>>> -
>>> -     sample_x = (state->src_x >> 16) / h_div;
>>> -     sample_y = (state->src_y >> 16) / v_div;
>>> -     block_start_y = (sample_y / block_h) * block_h;
>>> -     num_hblocks = sample_x / block_w;
>>> -
>>> -     dma_addr += fb->pitches[plane] * block_start_y;
>>> -     dma_addr += block_size * num_hblocks;
>>> -
>>> -     return dma_addr;
>>> +     return obj->dma_addr + drm_framebuffer_get_block_offset(fb, plane,
>>> +                                                             state->src_x >> 16,
>>> +                                                             state->src_y >> 16);
>>>    }
>>>    EXPORT_SYMBOL_GPL(drm_fb_dma_get_gem_addr);
>>>
>>> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>>> index d32aceb6ca9b..9e1231162047 100644
>>> --- a/drivers/gpu/drm/drm_framebuffer.c
>>> +++ b/drivers/gpu/drm/drm_framebuffer.c
>>> @@ -1208,6 +1208,51 @@ void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
>>>        }
>>>    }
>>>
>>> +/**
>>> + * drm_framebuffer_get_block_offset() - Get offset to start of pixel block for
>>> + * the given framebuffer and coordinates.
>>> + * @fb: The framebuffer
>>> + * @plane: Which plane
>>> + * @x: x coordinate for pixel
>>> + * @y: y coordinate for pixel
>>> + *
>>> + * This function will usually be called from the PLANE callback functions,
>>> + * or from one of the helpers that calculates the framebuffer's DMA address.
>>> + *
>>> + * Return: offset from start of framebuffer to start of pixel block
>>> + */
>>> +u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
>>> +                                  unsigned int x, unsigned int y)
>> Better use u64 as return type.
> To avoid overflow? Not sure who would use crazy large framebuffers, but
> doesn't hurt to play it safe.

I'd be worried about a malicious user space that tries to access OOB.

Apart from that, we use u64 for other framebuffer-related sizes like 
pitch calculations or dma addresses. Using u64 here would keep that 
consistent.


>
>>> +{
>>> +     u8 h_div = 1, v_div = 1;
>>> +     u32 block_w = drm_format_info_block_width(fb->format, plane);
>>> +     u32 block_h = drm_format_info_block_height(fb->format, plane);
>>> +     u32 block_size = fb->format->char_per_block[plane];
>>> +     u32 sample_x;
>>> +     u32 sample_y;
>>> +     u32 block_start_y;
>>> +     u32 num_hblocks;
>>> +     u32 offset;
>>> +
>>> +     offset = fb->offsets[plane];
>>> +
>>> +     if (plane > 0) {
>>> +             h_div = fb->format->hsub;
>>> +             v_div = fb->format->vsub;
>>> +     }
>>> +
>>> +     sample_x = x / h_div;
>>> +     sample_y = y / v_div;
>>> +     block_start_y = (sample_y / block_h) * block_h;
>>> +     num_hblocks = sample_x / block_w;
>>> +
>>> +     offset += fb->pitches[plane] * block_start_y;
>>> +     offset += block_size * num_hblocks;
>> User space controls the values in fb->offsets and fb->pitches.  I'm not
>> sure how well they have been validated already at this point. Did you
>> investigate this?
> It wouldn't be worse than before, since this changes is purely code movement.
>
> There are minimal sanity checks done by drm_internal_framebuffer_create()
> in framebuffer_check(), such as offset overflow or pitch size too small,
> but that's about it. It would be up to individual drivers to perform more
> checks that match their hardware limitations.

Right, makes sense. Looking through the framebuffer validation, a 
buffer-size check could be done in framebuffer_check().  But that's 
another patch series.


>
> What sort of issues are you thinking about?

Again, I'm thinking of malicious user space that crafts these values to 
force an OOB access.

Best regards
Thomas


>
>
> ChenYu
>
>> Best regards
>> Thomas
>>
>>
>>> +
>>> +     return offset;
>>> +}
>>> +EXPORT_SYMBOL(drm_framebuffer_get_block_offset);
>>> +
>>>    #ifdef CONFIG_DEBUG_FS
>>>    static int drm_framebuffer_info(struct seq_file *m, void *data)
>>>    {
>>> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
>>> index 38b24fc8978d..c07aea1cc59f 100644
>>> --- a/include/drm/drm_framebuffer.h
>>> +++ b/include/drm/drm_framebuffer.h
>>> @@ -220,6 +220,9 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb);
>>>    void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
>>>    void drm_framebuffer_unregister_private(struct drm_framebuffer *fb);
>>>
>>> +u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
>>> +                                  unsigned int x, unsigned int y);
>>> +
>>>    /**
>>>     * drm_framebuffer_get - acquire a framebuffer reference
>>>     * @fb: DRM framebuffer
>> --
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
>> GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)
>>
>>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)

Re: [PATCH RFT v2 1/5] drm: Split framebuffer pixel offset calculation from drm_fb_dma_get_gem_addr()

[Chen-Yu Tsai]

On Fri, Sep 18, 2026 at 2:41 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 18.09.26 um 06:16 schrieb Chen-Yu Tsai:
> > On Thu, Sep 17, 2026 at 11:20 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >> Hi
> >>
> >> Am 16.09.26 um 05:33 schrieb Chen-Yu Tsai:
> >>> Currently drm_fb_dma_get_gem_addr() calculates the offset into the
> >>> framebuffer memory for the framebuffer's unclipped source coordinates,
> >>> adds that to the framebuffer's backing storage, and returns the result.
> >>>
> >>> We are about to add a variant that uses the clipped source coordinates,
> >>> so there is already some reuse of code. However, calculating the data
> >>> offset for a given pixel is not specific to the DMA FB helpers. The
> >>> offset is only related to the framebuffer.
> >>>
> >>> Split out the offset calculation into a new framebuffer helper so that
> >>> non-DMA users can also reuse the same code.
> >>>
> >>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>> Cc: <stable@vger.kernel.org> # dependency for next patch
> >>> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> >>> ---
> >>> Changes since v1:
> >>> - New patch
> >>> ---
> >>>    drivers/gpu/drm/drm_fb_dma_helper.c | 28 ++----------------
> >>>    drivers/gpu/drm/drm_framebuffer.c   | 45 +++++++++++++++++++++++++++++
> >>>    include/drm/drm_framebuffer.h       |  3 ++
> >>>    3 files changed, 51 insertions(+), 25 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c
> >>> index fd71969d2fb1..ab0f37d8a5ff 100644
> >>> --- a/drivers/gpu/drm/drm_fb_dma_helper.c
> >>> +++ b/drivers/gpu/drm/drm_fb_dma_helper.c
> >>> @@ -75,36 +75,14 @@ dma_addr_t drm_fb_dma_get_gem_addr(struct drm_framebuffer *fb,
> >>>                                   unsigned int plane)
> >>>    {
> >>>        struct drm_gem_dma_object *obj;
> >>> -     dma_addr_t dma_addr;
> >>> -     u8 h_div = 1, v_div = 1;
> >>> -     u32 block_w = drm_format_info_block_width(fb->format, plane);
> >>> -     u32 block_h = drm_format_info_block_height(fb->format, plane);
> >>> -     u32 block_size = fb->format->char_per_block[plane];
> >>> -     u32 sample_x;
> >>> -     u32 sample_y;
> >>> -     u32 block_start_y;
> >>> -     u32 num_hblocks;
> >>>
> >>>        obj = drm_fb_dma_get_gem_obj(fb, plane);
> >>>        if (!obj)
> >>>                return 0;
> >>>
> >>> -     dma_addr = obj->dma_addr + fb->offsets[plane];
> >>> -
> >>> -     if (plane > 0) {
> >>> -             h_div = fb->format->hsub;
> >>> -             v_div = fb->format->vsub;
> >>> -     }
> >>> -
> >>> -     sample_x = (state->src_x >> 16) / h_div;
> >>> -     sample_y = (state->src_y >> 16) / v_div;
> >>> -     block_start_y = (sample_y / block_h) * block_h;
> >>> -     num_hblocks = sample_x / block_w;
> >>> -
> >>> -     dma_addr += fb->pitches[plane] * block_start_y;
> >>> -     dma_addr += block_size * num_hblocks;
> >>> -
> >>> -     return dma_addr;
> >>> +     return obj->dma_addr + drm_framebuffer_get_block_offset(fb, plane,
> >>> +                                                             state->src_x >> 16,
> >>> +                                                             state->src_y >> 16);
> >>>    }
> >>>    EXPORT_SYMBOL_GPL(drm_fb_dma_get_gem_addr);
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> >>> index d32aceb6ca9b..9e1231162047 100644
> >>> --- a/drivers/gpu/drm/drm_framebuffer.c
> >>> +++ b/drivers/gpu/drm/drm_framebuffer.c
> >>> @@ -1208,6 +1208,51 @@ void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
> >>>        }
> >>>    }
> >>>
> >>> +/**
> >>> + * drm_framebuffer_get_block_offset() - Get offset to start of pixel block for
> >>> + * the given framebuffer and coordinates.
> >>> + * @fb: The framebuffer
> >>> + * @plane: Which plane
> >>> + * @x: x coordinate for pixel
> >>> + * @y: y coordinate for pixel
> >>> + *
> >>> + * This function will usually be called from the PLANE callback functions,
> >>> + * or from one of the helpers that calculates the framebuffer's DMA address.
> >>> + *
> >>> + * Return: offset from start of framebuffer to start of pixel block
> >>> + */
> >>> +u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
> >>> +                                  unsigned int x, unsigned int y)
> >> Better use u64 as return type.
> > To avoid overflow? Not sure who would use crazy large framebuffers, but
> > doesn't hurt to play it safe.
>
> I'd be worried about a malicious user space that tries to access OOB.
>
> Apart from that, we use u64 for other framebuffer-related sizes like
> pitch calculations or dma addresses. Using u64 here would keep that
> consistent.

Indeed. It seemed weird that the helper originally used u32. Maybe it
was carried over from CMA on ARMv7, which predominantly only had
32-bit address space data busses?

>
> >
> >>> +{
> >>> +     u8 h_div = 1, v_div = 1;
> >>> +     u32 block_w = drm_format_info_block_width(fb->format, plane);
> >>> +     u32 block_h = drm_format_info_block_height(fb->format, plane);
> >>> +     u32 block_size = fb->format->char_per_block[plane];
> >>> +     u32 sample_x;
> >>> +     u32 sample_y;
> >>> +     u32 block_start_y;
> >>> +     u32 num_hblocks;
> >>> +     u32 offset;
> >>> +
> >>> +     offset = fb->offsets[plane];
> >>> +
> >>> +     if (plane > 0) {
> >>> +             h_div = fb->format->hsub;
> >>> +             v_div = fb->format->vsub;
> >>> +     }
> >>> +
> >>> +     sample_x = x / h_div;
> >>> +     sample_y = y / v_div;
> >>> +     block_start_y = (sample_y / block_h) * block_h;
> >>> +     num_hblocks = sample_x / block_w;
> >>> +
> >>> +     offset += fb->pitches[plane] * block_start_y;
> >>> +     offset += block_size * num_hblocks;
> >> User space controls the values in fb->offsets and fb->pitches.  I'm not
> >> sure how well they have been validated already at this point. Did you
> >> investigate this?
> > It wouldn't be worse than before, since this changes is purely code movement.
> >
> > There are minimal sanity checks done by drm_internal_framebuffer_create()
> > in framebuffer_check(), such as offset overflow or pitch size too small,
> > but that's about it. It would be up to individual drivers to perform more
> > checks that match their hardware limitations.
>
> Right, makes sense. Looking through the framebuffer validation, a
> buffer-size check could be done in framebuffer_check().  But that's
> another patch series.

That's further covered by drm_gem_fb_init_with_funcs(), which
drm_gem_fb_create*() goes into. I didn't check all the drivers that
implemented their own .fb_create callback though.

- rockchip uses the GEM FB helpers
- MSM reimplements the GEM FB helpers, but does have proper size checks
- nouveau has size checks
- omap has size checks

> >
> > What sort of issues are you thinking about?
>
> Again, I'm thinking of malicious user space that crafts these values to
> force an OOB access.

I think we're covered.


Thanks
ChenYu

> Best regards
> Thomas
>
>
> >
> >
> > ChenYu
> >
> >> Best regards
> >> Thomas
> >>
> >>
> >>> +
> >>> +     return offset;
> >>> +}
> >>> +EXPORT_SYMBOL(drm_framebuffer_get_block_offset);
> >>> +
> >>>    #ifdef CONFIG_DEBUG_FS
> >>>    static int drm_framebuffer_info(struct seq_file *m, void *data)
> >>>    {
> >>> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> >>> index 38b24fc8978d..c07aea1cc59f 100644
> >>> --- a/include/drm/drm_framebuffer.h
> >>> +++ b/include/drm/drm_framebuffer.h
> >>> @@ -220,6 +220,9 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb);
> >>>    void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
> >>>    void drm_framebuffer_unregister_private(struct drm_framebuffer *fb);
> >>>
> >>> +u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
> >>> +                                  unsigned int x, unsigned int y);
> >>> +
> >>>    /**
> >>>     * drm_framebuffer_get - acquire a framebuffer reference
> >>>     * @fb: DRM framebuffer
> >> --
> >> --
> >> Thomas Zimmermann
> >> Graphics Driver Developer
> >> SUSE Software Solutions Germany GmbH
> >> Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
> >> GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)
> >>
> >>
>
> --
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
> GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)
>
>

Re: [PATCH RFT v2 1/5] drm: Split framebuffer pixel offset calculation from drm_fb_dma_get_gem_addr()

[Chen-Yu Tsai]

On Fri, Sep 18, 2026 at 3:06 PM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> On Fri, Sep 18, 2026 at 2:41 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >
> > Hi
> >
> > Am 18.09.26 um 06:16 schrieb Chen-Yu Tsai:
> > > On Thu, Sep 17, 2026 at 11:20 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> > >> Hi
> > >>
> > >> Am 16.09.26 um 05:33 schrieb Chen-Yu Tsai:
> > >>> Currently drm_fb_dma_get_gem_addr() calculates the offset into the
> > >>> framebuffer memory for the framebuffer's unclipped source coordinates,
> > >>> adds that to the framebuffer's backing storage, and returns the result.
> > >>>
> > >>> We are about to add a variant that uses the clipped source coordinates,
> > >>> so there is already some reuse of code. However, calculating the data
> > >>> offset for a given pixel is not specific to the DMA FB helpers. The
> > >>> offset is only related to the framebuffer.
> > >>>
> > >>> Split out the offset calculation into a new framebuffer helper so that
> > >>> non-DMA users can also reuse the same code.
> > >>>
> > >>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> > >>> Cc: <stable@vger.kernel.org> # dependency for next patch
> > >>> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > >>> ---
> > >>> Changes since v1:
> > >>> - New patch
> > >>> ---
> > >>>    drivers/gpu/drm/drm_fb_dma_helper.c | 28 ++----------------
> > >>>    drivers/gpu/drm/drm_framebuffer.c   | 45 +++++++++++++++++++++++++++++
> > >>>    include/drm/drm_framebuffer.h       |  3 ++
> > >>>    3 files changed, 51 insertions(+), 25 deletions(-)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c
> > >>> index fd71969d2fb1..ab0f37d8a5ff 100644
> > >>> --- a/drivers/gpu/drm/drm_fb_dma_helper.c
> > >>> +++ b/drivers/gpu/drm/drm_fb_dma_helper.c
> > >>> @@ -75,36 +75,14 @@ dma_addr_t drm_fb_dma_get_gem_addr(struct drm_framebuffer *fb,
> > >>>                                   unsigned int plane)
> > >>>    {
> > >>>        struct drm_gem_dma_object *obj;
> > >>> -     dma_addr_t dma_addr;
> > >>> -     u8 h_div = 1, v_div = 1;
> > >>> -     u32 block_w = drm_format_info_block_width(fb->format, plane);
> > >>> -     u32 block_h = drm_format_info_block_height(fb->format, plane);
> > >>> -     u32 block_size = fb->format->char_per_block[plane];
> > >>> -     u32 sample_x;
> > >>> -     u32 sample_y;
> > >>> -     u32 block_start_y;
> > >>> -     u32 num_hblocks;
> > >>>
> > >>>        obj = drm_fb_dma_get_gem_obj(fb, plane);
> > >>>        if (!obj)
> > >>>                return 0;
> > >>>
> > >>> -     dma_addr = obj->dma_addr + fb->offsets[plane];
> > >>> -
> > >>> -     if (plane > 0) {
> > >>> -             h_div = fb->format->hsub;
> > >>> -             v_div = fb->format->vsub;
> > >>> -     }
> > >>> -
> > >>> -     sample_x = (state->src_x >> 16) / h_div;
> > >>> -     sample_y = (state->src_y >> 16) / v_div;
> > >>> -     block_start_y = (sample_y / block_h) * block_h;
> > >>> -     num_hblocks = sample_x / block_w;
> > >>> -
> > >>> -     dma_addr += fb->pitches[plane] * block_start_y;
> > >>> -     dma_addr += block_size * num_hblocks;
> > >>> -
> > >>> -     return dma_addr;
> > >>> +     return obj->dma_addr + drm_framebuffer_get_block_offset(fb, plane,
> > >>> +                                                             state->src_x >> 16,
> > >>> +                                                             state->src_y >> 16);
> > >>>    }
> > >>>    EXPORT_SYMBOL_GPL(drm_fb_dma_get_gem_addr);
> > >>>
> > >>> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > >>> index d32aceb6ca9b..9e1231162047 100644
> > >>> --- a/drivers/gpu/drm/drm_framebuffer.c
> > >>> +++ b/drivers/gpu/drm/drm_framebuffer.c
> > >>> @@ -1208,6 +1208,51 @@ void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
> > >>>        }
> > >>>    }
> > >>>
> > >>> +/**
> > >>> + * drm_framebuffer_get_block_offset() - Get offset to start of pixel block for
> > >>> + * the given framebuffer and coordinates.
> > >>> + * @fb: The framebuffer
> > >>> + * @plane: Which plane
> > >>> + * @x: x coordinate for pixel
> > >>> + * @y: y coordinate for pixel
> > >>> + *
> > >>> + * This function will usually be called from the PLANE callback functions,
> > >>> + * or from one of the helpers that calculates the framebuffer's DMA address.
> > >>> + *
> > >>> + * Return: offset from start of framebuffer to start of pixel block
> > >>> + */
> > >>> +u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
> > >>> +                                  unsigned int x, unsigned int y)
> > >> Better use u64 as return type.
> > > To avoid overflow? Not sure who would use crazy large framebuffers, but
> > > doesn't hurt to play it safe.
> >
> > I'd be worried about a malicious user space that tries to access OOB.
> >
> > Apart from that, we use u64 for other framebuffer-related sizes like
> > pitch calculations or dma addresses. Using u64 here would keep that
> > consistent.
>
> Indeed. It seemed weird that the helper originally used u32. Maybe it
> was carried over from CMA on ARMv7, which predominantly only had
> 32-bit address space data busses?
>
> >
> > >
> > >>> +{
> > >>> +     u8 h_div = 1, v_div = 1;
> > >>> +     u32 block_w = drm_format_info_block_width(fb->format, plane);
> > >>> +     u32 block_h = drm_format_info_block_height(fb->format, plane);
> > >>> +     u32 block_size = fb->format->char_per_block[plane];
> > >>> +     u32 sample_x;
> > >>> +     u32 sample_y;
> > >>> +     u32 block_start_y;
> > >>> +     u32 num_hblocks;
> > >>> +     u32 offset;
> > >>> +
> > >>> +     offset = fb->offsets[plane];
> > >>> +
> > >>> +     if (plane > 0) {
> > >>> +             h_div = fb->format->hsub;
> > >>> +             v_div = fb->format->vsub;
> > >>> +     }
> > >>> +
> > >>> +     sample_x = x / h_div;
> > >>> +     sample_y = y / v_div;
> > >>> +     block_start_y = (sample_y / block_h) * block_h;
> > >>> +     num_hblocks = sample_x / block_w;
> > >>> +
> > >>> +     offset += fb->pitches[plane] * block_start_y;
> > >>> +     offset += block_size * num_hblocks;
> > >> User space controls the values in fb->offsets and fb->pitches.  I'm not
> > >> sure how well they have been validated already at this point. Did you
> > >> investigate this?
> > > It wouldn't be worse than before, since this changes is purely code movement.
> > >
> > > There are minimal sanity checks done by drm_internal_framebuffer_create()
> > > in framebuffer_check(), such as offset overflow or pitch size too small,
> > > but that's about it. It would be up to individual drivers to perform more
> > > checks that match their hardware limitations.
> >
> > Right, makes sense. Looking through the framebuffer validation, a
> > buffer-size check could be done in framebuffer_check().  But that's
> > another patch series.
>
> That's further covered by drm_gem_fb_init_with_funcs(), which
> drm_gem_fb_create*() goes into. I didn't check all the drivers that
> implemented their own .fb_create callback though.
>
> - rockchip uses the GEM FB helpers
> - MSM reimplements the GEM FB helpers, but does have proper size checks
> - nouveau has size checks
> - omap has size checks

Side note: it seems that the drivers that reimplement
drm_gem_fb_init_with_funcs() do so because they need to do additional
checks on the (sub-classed) GEM objects. Perhaps exporting drm_gem_fb_init()
or deconstructing drm_gem_fb_init_with_funcs() could allow more of them
to use common helpers for things like size checks.

> > >
> > > What sort of issues are you thinking about?
> >
> > Again, I'm thinking of malicious user space that crafts these values to
> > force an OOB access.
>
> I think we're covered.
>
>
> Thanks
> ChenYu
>
> > Best regards
> > Thomas
> >
> >
> > >
> > >
> > > ChenYu
> > >
> > >> Best regards
> > >> Thomas
> > >>
> > >>
> > >>> +
> > >>> +     return offset;
> > >>> +}
> > >>> +EXPORT_SYMBOL(drm_framebuffer_get_block_offset);
> > >>> +
> > >>>    #ifdef CONFIG_DEBUG_FS
> > >>>    static int drm_framebuffer_info(struct seq_file *m, void *data)
> > >>>    {
> > >>> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> > >>> index 38b24fc8978d..c07aea1cc59f 100644
> > >>> --- a/include/drm/drm_framebuffer.h
> > >>> +++ b/include/drm/drm_framebuffer.h
> > >>> @@ -220,6 +220,9 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb);
> > >>>    void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
> > >>>    void drm_framebuffer_unregister_private(struct drm_framebuffer *fb);
> > >>>
> > >>> +u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
> > >>> +                                  unsigned int x, unsigned int y);
> > >>> +
> > >>>    /**
> > >>>     * drm_framebuffer_get - acquire a framebuffer reference
> > >>>     * @fb: DRM framebuffer
> > >> --
> > >> --
> > >> Thomas Zimmermann
> > >> Graphics Driver Developer
> > >> SUSE Software Solutions Germany GmbH
> > >> Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
> > >> GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)
> > >>
> > >>
> >
> > --
> > --
> > Thomas Zimmermann
> > Graphics Driver Developer
> > SUSE Software Solutions Germany GmbH
> > Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
> > GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)
> >
> >