[PATCH v2 0/3] drm/armada: fbdev: Use client buffers

[PATCH v2 0/3] drm/armada: fbdev: Use client buffers

[Thomas Zimmermann]

A client buffer holds the DRM framebuffer for an in-kernel DRM
client. Until now, armada created an internal ad-hoc framebuffer for
its fbdev emulation, while by-passing the regular interfaces used by
user-space compositors.

Convert armada's fbdev emulation to use client buffers. Replacing the
existing code with a client buffer allows for stream-lining armada code
and later also the fbdev helpers. The new framebuffer will be registered
against the client's file and will support handles for GEM objects. It
is then just another framebuffer within the DRM ecosystem.

If all driver's fbdev-emulation helpers can be converted to use client
buffers, the emulation's framebuffer handling as a whole can possibly be
moved into shared helpers.

Patches 1 and 2 convert armada's fbdev emulation to client buffers. It
still allocates a GEM object buffer tailored towards fbdev emulation,
but size calculations now use common DRM helpers.

Patch 3 cleans up symbol visibility in armada's fb code.

v2:
- add more error checks to geometry calculations

Thomas Zimmermann (3):
  drm/armada: fbdev: Calculate buffer geometry with format helpers
  drm/armada: fbdev: Use a DRM client buffer
  drm/armada: Make armada_framebuffer_create() an internal interface

 drivers/gpu/drm/armada/armada_fb.c    |  9 +--
 drivers/gpu/drm/armada/armada_fb.h    |  3 -
 drivers/gpu/drm/armada/armada_fbdev.c | 86 +++++++++++++++++----------
 3 files changed, 58 insertions(+), 40 deletions(-)


base-commit: fc59f76558703febba8056be87d1c97d14f7485e
-- 
2.54.0

[PATCH v2 1/3] drm/armada: fbdev: Calculate buffer geometry with format helpers

[Thomas Zimmermann]

Replace the geometry and size calculation in armada's fbdev emulation
with DRM format helpers. This consists of a 4CC lookup from the fbdev
parameters, format lookup, pitch calculation and size calculation.

Then allocate the GEM buffer object for the framebuffer memory from
the calculated size. Mmap provides the allocated buffer to user space,
so align the buffer size to PAGE_SIZE.

Slightly rearrange cleaning up the GEM object and rolling back on
errors. This will simplify error handling when the code uses client
buffers.

v2:
- add more error checks to geometry calculations

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/armada/armada_fbdev.c | 65 ++++++++++++++++-----------
 1 file changed, 39 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_fbdev.c b/drivers/gpu/drm/armada/armada_fbdev.c
index 8bbae94804f8..f95658091acf 100644
--- a/drivers/gpu/drm/armada/armada_fbdev.c
+++ b/drivers/gpu/drm/armada/armada_fbdev.c
@@ -45,20 +45,30 @@ int armada_fbdev_driver_fbdev_probe(struct drm_fb_helper *fbh,
 {
 	struct drm_device *dev = fbh->dev;
 	struct fb_info *info = fbh->info;
+	u32 fourcc, pitch;
+	u64 size;
+	const struct drm_format_info *format;
 	struct drm_mode_fb_cmd2 mode;
 	struct armada_framebuffer *dfb;
 	struct armada_gem_object *obj;
-	int size, ret;
+	int ret;
 	void *ptr;
 
-	memset(&mode, 0, sizeof(mode));
-	mode.width = sizes->surface_width;
-	mode.height = sizes->surface_height;
-	mode.pitches[0] = armada_pitch(mode.width, sizes->surface_bpp);
-	mode.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
-					sizes->surface_depth);
+	fourcc = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth);
+	if (fourcc == DRM_FORMAT_INVALID)
+		return -EINVAL;
+	format = drm_get_format_info(dev, fourcc, DRM_FORMAT_MOD_LINEAR);
+	if (!format)
+		return -EINVAL;
+	pitch = armada_pitch(sizes->surface_width, drm_format_info_bpp(format, 0));
+	if (!pitch)
+		return -EINVAL;
+	if (check_mul_overflow(pitch, sizes->surface_height, &size))
+		return -EINVAL;
+	size = ALIGN(size, PAGE_SIZE);
+	if (size < PAGE_SIZE)
+		return -EINVAL;
 
-	size = mode.pitches[0] * mode.height;
 	obj = armada_gem_alloc_private_object(dev, size);
 	if (!obj) {
 		DRM_ERROR("failed to allocate fb memory\n");
@@ -66,30 +76,26 @@ int armada_fbdev_driver_fbdev_probe(struct drm_fb_helper *fbh,
 	}
 
 	ret = armada_gem_linear_back(dev, obj);
-	if (ret) {
-		drm_gem_object_put(&obj->obj);
-		return ret;
-	}
+	if (ret)
+		goto err_drm_gem_object_put;
 
 	ptr = armada_gem_map_object(dev, obj);
 	if (!ptr) {
-		drm_gem_object_put(&obj->obj);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err_drm_gem_object_put;
 	}
 
-	dfb = armada_framebuffer_create(dev,
-					drm_get_format_info(dev, mode.pixel_format,
-							    mode.modifier[0]),
-					&mode, obj);
-
-	/*
-	 * A reference is now held by the framebuffer object if
-	 * successful, otherwise this drops the ref for the error path.
-	 */
-	drm_gem_object_put(&obj->obj);
+	memset(&mode, 0, sizeof(mode));
+	mode.width = sizes->surface_width;
+	mode.height = sizes->surface_height;
+	mode.pitches[0] = pitch;
+	mode.pixel_format = fourcc;
 
-	if (IS_ERR(dfb))
-		return PTR_ERR(dfb);
+	dfb = armada_framebuffer_create(dev, format, &mode, obj);
+	if (IS_ERR(dfb)) {
+		ret = PTR_ERR(dfb);
+		goto err_drm_gem_object_put;
+	}
 
 	info->fbops = &armada_fb_ops;
 	info->fix.smem_start = obj->phys_addr;
@@ -105,5 +111,12 @@ int armada_fbdev_driver_fbdev_probe(struct drm_fb_helper *fbh,
 		dfb->fb.width, dfb->fb.height, dfb->fb.format->cpp[0] * 8,
 		(unsigned long long)obj->phys_addr);
 
+	/* The framebuffer still holds a reference on the GEM object. */
+	drm_gem_object_put(&obj->obj);
+
 	return 0;
+
+err_drm_gem_object_put:
+	drm_gem_object_put(&obj->obj);
+	return ret;
 }
-- 
2.54.0

[PATCH v2 2/3] drm/armada: fbdev: Use a DRM client buffer

[Thomas Zimmermann]

Replace the internal DRM framebuffer with a DRM client buffer. The
client buffer allocates the DRM framebuffer on a file and also uses
GEM object handles via the regular ADDFB2 interfaces.

Using client-buffer interfaces unifies framebuffer allocation for
DRM clients in user space and armada's internal fbdev emulation. It
also simplifies the clean-up side of the fbdev emulation.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/armada/armada_fbdev.c | 41 ++++++++++++++++-----------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_fbdev.c b/drivers/gpu/drm/armada/armada_fbdev.c
index f95658091acf..46b2eb84b65f 100644
--- a/drivers/gpu/drm/armada/armada_fbdev.c
+++ b/drivers/gpu/drm/armada/armada_fbdev.c
@@ -26,8 +26,7 @@ static void armada_fbdev_fb_destroy(struct fb_info *info)
 
 	drm_fb_helper_fini(fbh);
 
-	fbh->fb->funcs->destroy(fbh->fb);
-
+	drm_client_buffer_delete(fbh->buffer);
 	drm_client_release(&fbh->client);
 }
 
@@ -43,14 +42,16 @@ static const struct drm_fb_helper_funcs armada_fbdev_helper_funcs;
 int armada_fbdev_driver_fbdev_probe(struct drm_fb_helper *fbh,
 				    struct drm_fb_helper_surface_size *sizes)
 {
-	struct drm_device *dev = fbh->dev;
+	struct drm_client_dev *client = &fbh->client;
+	struct drm_device *dev = client->dev;
+	struct drm_file *file = client->file;
 	struct fb_info *info = fbh->info;
 	u32 fourcc, pitch;
 	u64 size;
 	const struct drm_format_info *format;
-	struct drm_mode_fb_cmd2 mode;
-	struct armada_framebuffer *dfb;
 	struct armada_gem_object *obj;
+	struct drm_client_buffer *buffer;
+	u32 handle;
 	int ret;
 	void *ptr;
 
@@ -85,37 +86,43 @@ int armada_fbdev_driver_fbdev_probe(struct drm_fb_helper *fbh,
 		goto err_drm_gem_object_put;
 	}
 
-	memset(&mode, 0, sizeof(mode));
-	mode.width = sizes->surface_width;
-	mode.height = sizes->surface_height;
-	mode.pitches[0] = pitch;
-	mode.pixel_format = fourcc;
-
-	dfb = armada_framebuffer_create(dev, format, &mode, obj);
-	if (IS_ERR(dfb)) {
-		ret = PTR_ERR(dfb);
+	ret = drm_gem_handle_create(file, &obj->obj, &handle);
+	if (ret)
 		goto err_drm_gem_object_put;
+
+	buffer = drm_client_buffer_create(client, sizes->surface_width, sizes->surface_height,
+					  fourcc, handle, pitch);
+	if (IS_ERR(buffer)) {
+		ret = PTR_ERR(buffer);
+		goto err_drm_gem_handle_delete;
 	}
 
+	fbh->funcs = &armada_fbdev_helper_funcs;
+	fbh->buffer = buffer;
+	fbh->fb = buffer->fb;
+
 	info->fbops = &armada_fb_ops;
 	info->fix.smem_start = obj->phys_addr;
 	info->fix.smem_len = obj->obj.size;
 	info->screen_size = obj->obj.size;
 	info->screen_base = ptr;
-	fbh->funcs = &armada_fbdev_helper_funcs;
-	fbh->fb = &dfb->fb;
 
 	drm_fb_helper_fill_info(info, fbh, sizes);
 
 	DRM_DEBUG_KMS("allocated %dx%d %dbpp fb: 0x%08llx\n",
-		dfb->fb.width, dfb->fb.height, dfb->fb.format->cpp[0] * 8,
+		buffer->fb->width, buffer->fb->height, buffer->fb->format->cpp[0] * 8,
 		(unsigned long long)obj->phys_addr);
 
+	/* The handle is only needed for creating the framebuffer. */
+	drm_gem_handle_delete(file, handle);
+
 	/* The framebuffer still holds a reference on the GEM object. */
 	drm_gem_object_put(&obj->obj);
 
 	return 0;
 
+err_drm_gem_handle_delete:
+	drm_gem_handle_delete(file, handle);
 err_drm_gem_object_put:
 	drm_gem_object_put(&obj->obj);
 	return ret;
-- 
2.54.0

[PATCH v2 3/3] drm/armada: Make armada_framebuffer_create() an internal interface

[Thomas Zimmermann]

The only caller of armada_framebuffer_create() is armada_fb_create()
from the same source file. Declare the former as static.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/armada/armada_fb.c | 9 +++++----
 drivers/gpu/drm/armada/armada_fb.h | 3 ---
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_fb.c b/drivers/gpu/drm/armada/armada_fb.c
index b828bba419bf..2f2348d543fd 100644
--- a/drivers/gpu/drm/armada/armada_fb.c
+++ b/drivers/gpu/drm/armada/armada_fb.c
@@ -18,10 +18,11 @@ static const struct drm_framebuffer_funcs armada_fb_funcs = {
 	.create_handle	= drm_gem_fb_create_handle,
 };
 
-struct armada_framebuffer *armada_framebuffer_create(struct drm_device *dev,
-						     const struct drm_format_info *info,
-						     const struct drm_mode_fb_cmd2 *mode,
-						     struct armada_gem_object *obj)
+static struct armada_framebuffer *
+armada_framebuffer_create(struct drm_device *dev,
+			  const struct drm_format_info *info,
+			  const struct drm_mode_fb_cmd2 *mode,
+			  struct armada_gem_object *obj)
 {
 	struct armada_framebuffer *dfb;
 	uint8_t format, config;
diff --git a/drivers/gpu/drm/armada/armada_fb.h b/drivers/gpu/drm/armada/armada_fb.h
index f2b990f055a2..0ba358ff9147 100644
--- a/drivers/gpu/drm/armada/armada_fb.h
+++ b/drivers/gpu/drm/armada/armada_fb.h
@@ -16,9 +16,6 @@ struct armada_framebuffer {
 	container_of(dfb, struct armada_framebuffer, fb)
 #define drm_fb_obj(fb) drm_to_armada_gem((fb)->obj[0])
 
-struct armada_framebuffer *armada_framebuffer_create(struct drm_device *,
-	const struct drm_format_info *info,
-	const struct drm_mode_fb_cmd2 *, struct armada_gem_object *);
 struct drm_framebuffer *armada_fb_create(struct drm_device *dev,
 	struct drm_file *dfile, const struct drm_format_info *info,
 	const struct drm_mode_fb_cmd2 *mode);
-- 
2.54.0

Re: [PATCH v2 3/3] drm/armada: Make armada_framebuffer_create() an internal interface

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Reference leak of GEM object on error path in `armada_fb_create`
--

commit 6143d70290da395a0c8c1c0addd0695192664072
Author: Thomas Zimmermann <tzimmermann@suse.de>

drm/armada: Make armada_framebuffer_create() an internal interface

The only caller of armada_framebuffer_create() is armada_fb_create()
from the same source file. Declare the former as static.

> diff --git a/drivers/gpu/drm/armada/armada_fb.c b/drivers/gpu/drm/armada/armada_fb.c
> index b828bba419bfb..2f2348d543fd7 100644
> --- a/drivers/gpu/drm/armada/armada_fb.c
> +++ b/drivers/gpu/drm/armada/armada_fb.c
> @@ -18,10 +18,11 @@ static const struct drm_framebuffer_funcs armada_fb_funcs = {
>  	.create_handle	= drm_gem_fb_create_handle,
>  };
>  
> -struct armada_framebuffer *armada_framebuffer_create(struct drm_device *dev,
> -						     const struct drm_format_info *info,
> -						     const struct drm_mode_fb_cmd2 *mode,
> -						     struct armada_gem_object *obj)
> +static struct armada_framebuffer *
> +armada_framebuffer_create(struct drm_device *dev,
> +			  const struct drm_format_info *info,
> +			  const struct drm_mode_fb_cmd2 *mode,
> +			  struct armada_gem_object *obj)
>  {

[Severity: High]
This is a pre-existing issue, but while reviewing the caller of this newly
static function, it appears there might be a resource leak on the error path.

In armada_fb_create(), we get a reference to the GEM object:

	obj = armada_gem_object_lookup(dfile, mode->handles[0]);

But if the call to armada_framebuffer_create() fails, the code jumps to the
'err' label instead of 'err_unref':

	dfb = armada_framebuffer_create(dev, info, mode, obj);
	if (IS_ERR(dfb)) {
		ret = PTR_ERR(dfb);
		goto err;
	}

	drm_gem_object_put(&obj->obj);

	return &dfb->fb;

 err_unref:
	drm_gem_object_put(&obj->obj);
 err:
	DRM_ERROR("failed to initialize framebuffer: %d\n", ret);

Does this bypass the drm_gem_object_put() call and leak the GEM object
reference? Since armada_fb_create() is reachable from userspace via the
DRM_IOCTL_MODE_ADDFB2 ioctl, could a user repeatedly request invalid formats
to leak references and cause memory exhaustion?

>  	struct armada_framebuffer *dfb;
>  	uint8_t format, config;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611072121.71492-1-tzimmermann@suse.de?part=3

Re: [PATCH v2 1/3] drm/armada: fbdev: Calculate buffer geometry with format helpers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The newly added `check_mul_overflow` check is completely ineffective because it stores the result in a `u64` variable. Since both operands (`pitch` and `sizes->surface_height`) are `u32`, their product can never overflow a 64-bit integer, meaning the check always passes. The `u64` size is then implicitly truncated to 32 bits when passed to `armada_gem_alloc_private_object()`, which expects a `size_t` argument. This completely bypasses the intended overflow protection.
--

commit 9304c78ca806a472129099acb5c200bab2e752a0
Author: Thomas Zimmermann <tzimmermann@suse.de>

drm/armada: fbdev: Calculate buffer geometry with format helpers

This commit replaces the geometry and size calculation in armada's fbdev
emulation with DRM format helpers. It uses a 4CC lookup, pitch calculation,
and allocates the GEM buffer object for the framebuffer memory aligned to
PAGE_SIZE.

> diff --git a/drivers/gpu/drm/armada/armada_fbdev.c b/drivers/gpu/drm/armada/armada_fbdev.c
> index 8bbae94804f8b..f95658091acf2 100644
> --- a/drivers/gpu/drm/armada/armada_fbdev.c
> +++ b/drivers/gpu/drm/armada/armada_fbdev.c
> @@ -45,20 +45,30 @@ int armada_fbdev_driver_fbdev_probe(struct drm_fb_helper *fbh,
>  {
[ ... ]
> +	if (check_mul_overflow(pitch, sizes->surface_height, &size))
> +		return -EINVAL;
> +	size = ALIGN(size, PAGE_SIZE);
> +	if (size < PAGE_SIZE)
> +		return -EINVAL;
>  
> -	size = mode.pitches[0] * mode.height;
>  	obj = armada_gem_alloc_private_object(dev, size);

[Severity: High]
This isn't a bug introduced by this patch, but does this check_mul_overflow()
actually prevent overflow here?

Since pitch and sizes->surface_height are 32-bit unsigned integers, and size
is a u64, the multiplication will never overflow the 64-bit target, so the
check will always pass.

However, when size is passed to armada_gem_alloc_private_object(), which
takes a size_t argument, couldn't this silently truncate to 32 bits since
the Armada DRM driver operates on a 32-bit architecture?

For example, if pitch * height evaluates to 0xFFFFFFFF, ALIGN() expands it
to 0x100000000. The check size < PAGE_SIZE evaluates to false on the u64,
but when passed to armada_gem_alloc_private_object(), the size truncates
to 0, resulting in an undersized buffer allocation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611072121.71492-1-tzimmermann@suse.de?part=1