[PATCH v2 00/25] drm/dumb-buffers: Fix and improve buffer-size calculation

[PATCH v2 00/25] drm/dumb-buffers: Fix and improve buffer-size calculation

[Thomas Zimmermann]

Dumb-buffer pitch and size is specified by width, height, bits-per-pixel
plus various hardware-specific alignments. The calculation of these
values is inconsistent and duplicated among drivers. The results for
formats with bpp < 8 are incorrect.

This series fixes this for most drivers. Default scanline pitch and
buffer size are now calculated with the existing 4CC helpers. There is
a new helper drm_mode_size_dumb() that calculates scanline pitch and
buffer size according to driver requirements.

The series fixes the common GEM implementations for DMA, SHMEM and
VRAM. It further changes most implementations of dumb_create to use
the new helper. A small number of  drivers has more complicated
calculations and will be updated by a later patches.

v2:
- rewrite series
- convert many individual drivers besides the shared GEM helpers

Thomas Zimmermann (25):
  drm/dumb-buffers: Sanitize output on errors
  drm/dumb-buffers: Provide helper to set pitch and size
  drm/gem-dma: Compute dumb-buffer sizes with drm_mode_size_dumb()
  drm/gem-shmem: Compute dumb-buffer sizes with drm_mode_size_dumb()
  drm/gem-vram: Compute dumb-buffer sizes with drm_mode_size_dumb()
  drm/armada: Compute dumb-buffer sizes with drm_mode_size_dumb()
  drm/exynos: Compute dumb-buffer sizes with drm_mode_size_dumb()
  drm/gma500: Compute dumb-buffer sizes with drm_mode_size_dumb()
  drm/hibmc: Compute dumb-buffer sizes with drm_mode_size_dumb()
  drm/imx/ipuv3: Compute dumb-buffer sizes with drm_mode_size_dumb()
  drm/loongson: Compute dumb-buffer sizes with drm_mode_size_dumb()
  drm/mediatek: Compute dumb-buffer sizes with drm_mode_size_dumb()
  drm/msm: Compute dumb-buffer sizes with drm_mode_size_dumb()
  drm/nouveau: Compute dumb-buffer sizes with drm_mode_size_dumb()
  drm/omapdrm: Compute dumb-buffer sizes with drm_mode_size_dumb()
  drm/qxl: Compute dumb-buffer sizes with drm_mode_size_dumb()
  drm/renesas/rcar-du: Compute dumb-buffer sizes with
    drm_mode_size_dumb()
  drm/renesas/rz-du: Compute dumb-buffer sizes with drm_mode_size_dumb()
  drm/rockchip: Compute dumb-buffer sizes with drm_mode_size_dumb()
  drm/tegra: Compute dumb-buffer sizes with drm_mode_size_dumb()
  drm/virtio: Compute dumb-buffer sizes with drm_mode_size_dumb()
  drm/vmwgfx: Compute dumb-buffer sizes with drm_mode_size_dumb()
  drm/xe: Compute dumb-buffer sizes with drm_mode_size_dumb()
  drm/xen: Compute dumb-buffer sizes with drm_mode_size_dumb()
  drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

 drivers/gpu/drm/armada/armada_gem.c           |  16 +--
 drivers/gpu/drm/drm_dumb_buffers.c            | 133 ++++++++++++++++--
 drivers/gpu/drm/drm_gem_dma_helper.c          |   7 +-
 drivers/gpu/drm/drm_gem_shmem_helper.c        |  16 +--
 drivers/gpu/drm/drm_gem_vram_helper.c         |  89 +++---------
 drivers/gpu/drm/exynos/exynos_drm_gem.c       |   8 +-
 drivers/gpu/drm/gma500/gem.c                  |  21 +--
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  25 +++-
 drivers/gpu/drm/imx/ipuv3/imx-drm-core.c      |  29 +++-
 drivers/gpu/drm/loongson/lsdc_gem.c           |  29 ++--
 drivers/gpu/drm/mediatek/mtk_gem.c            |  13 +-
 drivers/gpu/drm/msm/msm_gem.c                 |  27 +++-
 drivers/gpu/drm/nouveau/nouveau_display.c     |   7 +-
 drivers/gpu/drm/omapdrm/omap_gem.c            |  15 +-
 drivers/gpu/drm/qxl/qxl_dumb.c                |  17 ++-
 drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c |   7 +-
 drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c  |   7 +-
 drivers/gpu/drm/rockchip/rockchip_drm_gem.c   |  12 +-
 drivers/gpu/drm/tegra/gem.c                   |   8 +-
 drivers/gpu/drm/virtio/virtgpu_gem.c          |  11 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c       |  21 +--
 drivers/gpu/drm/xe/xe_bo.c                    |   8 +-
 drivers/gpu/drm/xen/xen_drm_front.c           |   7 +-
 drivers/gpu/drm/xlnx/zynqmp_kms.c             |   7 +-
 include/drm/drm_dumb_buffers.h                |  14 ++
 include/drm/drm_gem_vram_helper.h             |   6 -
 26 files changed, 333 insertions(+), 227 deletions(-)
 create mode 100644 include/drm/drm_dumb_buffers.h


base-commit: f06efdfad9d0e9f5cb74404ac98e1a5b3b246567
prerequisite-patch-id: 0aa359f6144c4015c140c8a6750be19099c676fb
prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
prerequisite-patch-id: cbc453ee02fae02af22fbfdce56ab732c7a88c36
-- 
2.47.1

[PATCH v2 01/25] drm/dumb-buffers: Sanitize output on errors

[Thomas Zimmermann]

The ioctls MODE_CREATE_DUMB and MODE_MAP_DUMB return results into a
memory buffer supplied by user space. On errors, it is possible that
intermediate values are being returned. The exact semantics depends
on the DRM driver's implementation of these ioctls. Although this is
most-likely not a security problem in practice, avoid any uncertainty
by clearing the memory to 0 on errors.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_dumb_buffers.c | 40 ++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
index 70032bba1c97..9916aaf5b3f2 100644
--- a/drivers/gpu/drm/drm_dumb_buffers.c
+++ b/drivers/gpu/drm/drm_dumb_buffers.c
@@ -99,7 +99,30 @@ int drm_mode_create_dumb(struct drm_device *dev,
 int drm_mode_create_dumb_ioctl(struct drm_device *dev,
 			       void *data, struct drm_file *file_priv)
 {
-	return drm_mode_create_dumb(dev, data, file_priv);
+	struct drm_mode_create_dumb *args = data;
+	int err;
+
+	err = drm_mode_create_dumb(dev, args, file_priv);
+	if (err) {
+		args->handle = 0;
+		args->pitch = 0;
+		args->size = 0;
+	}
+	return err;
+}
+
+static int drm_mode_mmap_dumb(struct drm_device *dev, struct drm_mode_map_dumb *args,
+			      struct drm_file *file_priv)
+{
+	if (!dev->driver->dumb_create)
+		return -ENOSYS;
+
+	if (dev->driver->dumb_map_offset)
+		return dev->driver->dumb_map_offset(file_priv, dev, args->handle,
+						    &args->offset);
+	else
+		return drm_gem_dumb_map_offset(file_priv, dev, args->handle,
+					       &args->offset);
 }
 
 /**
@@ -120,17 +143,12 @@ int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
 			     void *data, struct drm_file *file_priv)
 {
 	struct drm_mode_map_dumb *args = data;
+	int err;
 
-	if (!dev->driver->dumb_create)
-		return -ENOSYS;
-
-	if (dev->driver->dumb_map_offset)
-		return dev->driver->dumb_map_offset(file_priv, dev,
-						    args->handle,
-						    &args->offset);
-	else
-		return drm_gem_dumb_map_offset(file_priv, dev, args->handle,
-					       &args->offset);
+	err = drm_mode_mmap_dumb(dev, args, file_priv);
+	if (err)
+		args->offset = 0;
+	return err;
 }
 
 int drm_mode_destroy_dumb(struct drm_device *dev, u32 handle,
-- 
2.47.1

[PATCH v2 02/25] drm/dumb-buffers: Provide helper to set pitch and size

[Thomas Zimmermann]

Add drm_modes_size_dumb(), a helper to calculate the dumb-buffer
scanline pitch and allocation size. Implementations of struct
drm_driver.dumb_create can call the new helper for their size
computations. There's currently quite a bit of code duplication
among DRM's memory managers. Each calculates scanline pitch and
buffer size from the given arguments, but the implementations are
inconsistent in how they treat alignment and format support. Later
patches will unify this code on top of drm_mode_size_dumb() as
much as possible.

drm_mode_size_dumb() uses existing 4CC format helpers to interpret the
given color mode. This makes the dumb-buffer interface behave similar
the kernel's video= parameter. Again, current per-driver implementations
likely have subtle differences or bugs in how they support color modes.

Future directions: one bug is present in the current input validation
in drm_mode_create_dumb(). The dumb-buffer overflow tests round up any
given bits-per-pixel value to a multiple of 8. So even one-bit formats,
such as DRM_FORMAT_C1, require 8 bits per pixel. While not common,
low-end displays use such formats; with a possible overcommitment of
memory. At some point, the validation logic in drm_mode_size_dumb() is
supposed to replace the erronous code.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_dumb_buffers.c | 93 ++++++++++++++++++++++++++++++
 include/drm/drm_dumb_buffers.h     | 14 +++++
 2 files changed, 107 insertions(+)
 create mode 100644 include/drm/drm_dumb_buffers.h

diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
index 9916aaf5b3f2..fd39720bd617 100644
--- a/drivers/gpu/drm/drm_dumb_buffers.c
+++ b/drivers/gpu/drm/drm_dumb_buffers.c
@@ -25,6 +25,8 @@
 
 #include <drm/drm_device.h>
 #include <drm/drm_drv.h>
+#include <drm/drm_dumb_buffers.h>
+#include <drm/drm_fourcc.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_mode.h>
 
@@ -57,6 +59,97 @@
  * a hardware-specific ioctl to allocate suitable buffer objects.
  */
 
+static int drm_mode_align_dumb(struct drm_mode_create_dumb *args,
+			       unsigned long pitch_align,
+			       unsigned long size_align)
+{
+	u32 pitch = args->pitch;
+	u32 size;
+
+	if (!pitch)
+		return -EINVAL;
+
+	if (pitch_align)
+		pitch = roundup(pitch, pitch_align);
+
+	/* overflow checks for 32bit size calculations */
+	if (args->height > U32_MAX / pitch)
+		return -EINVAL;
+
+	if (!size_align)
+		size_align = PAGE_SIZE;
+	else if (!IS_ALIGNED(size_align, PAGE_SIZE))
+		return -EINVAL;
+
+	size = ALIGN(args->height * pitch, size_align);
+	if (!size)
+		return -EINVAL;
+
+	args->pitch = pitch;
+	args->size = size;
+
+	return 0;
+}
+
+/**
+ * drm_mode_size_dumb - Calculates the scanline and buffer sizes for dumb buffers
+ * @dev: DRM device
+ * @args: Parameters for the dumb buffer
+ * @pitch_align: Scanline alignment in bytes
+ * @size_align: Buffer-size alignment in bytes
+ *
+ * The helper drm_mode_size_dumb() calculates the size of the buffer
+ * allocation and the scanline size for a dumb buffer. Callers have to
+ * set the buffers width, height and color mode in the argument @arg.
+ * The helper validates the correctness of the input and tests for
+ * possible overflows. If successful, it returns the dumb buffer's
+ * required scanline pitch and size in &args.
+ *
+ * The parameter @pitch_align allows the driver to specifies an
+ * alignment for the scanline pitch, if the hardware requires any. The
+ * calculated pitch will be a multiple of the alignment. The parameter
+ * @size_align allows to specify an alignment for buffer sizes. The
+ * returned size is always a multiple of PAGE_SIZE.
+ *
+ * Returns:
+ * Zero on success, or a negative error code otherwise.
+ */
+int drm_mode_size_dumb(struct drm_device *dev,
+		       struct drm_mode_create_dumb *args,
+		       unsigned long pitch_align,
+		       unsigned long size_align)
+{
+	u32 fourcc;
+	const struct drm_format_info *info;
+	u64 pitch;
+
+	/*
+	 * The scanline pitch depends on the buffer width and the color
+	 * format. The latter is specified as a color-mode constant for
+	 * which we first have to find the corresponding color format.
+	 *
+	 * Different color formats can have the same color-mode constant.
+	 * For example XRGB8888 and BGRX8888 both have a color mode of 32.
+	 * It is possible to use different formats for dumb-buffer allocation
+	 * and rendering as long as all involved formats share the same
+	 * color-mode constant.
+	 */
+	fourcc = drm_driver_color_mode_format(dev, args->bpp);
+	if (fourcc == DRM_FORMAT_INVALID)
+		return -EINVAL;
+	info = drm_format_info(fourcc);
+	if (!info)
+		return -EINVAL;
+	pitch = drm_format_info_min_pitch(info, 0, args->width);
+	if (!pitch || pitch > U32_MAX)
+		return -EINVAL;
+
+	args->pitch = pitch;
+
+	return drm_mode_align_dumb(args, pitch_align, size_align);
+}
+EXPORT_SYMBOL(drm_mode_size_dumb);
+
 int drm_mode_create_dumb(struct drm_device *dev,
 			 struct drm_mode_create_dumb *args,
 			 struct drm_file *file_priv)
diff --git a/include/drm/drm_dumb_buffers.h b/include/drm/drm_dumb_buffers.h
new file mode 100644
index 000000000000..6fe36004b19d
--- /dev/null
+++ b/include/drm/drm_dumb_buffers.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: MIT */
+
+#ifndef __DRM_DUMB_BUFFERS_H__
+#define __DRM_DUMB_BUFFERS_H__
+
+struct drm_device;
+struct drm_mode_create_dumb;
+
+int drm_mode_size_dumb(struct drm_device *dev,
+		       struct drm_mode_create_dumb *args,
+		       unsigned long pitch_align,
+		       unsigned long size_align);
+
+#endif
-- 
2.47.1

Re:[PATCH v2 02/25] drm/dumb-buffers: Provide helper to set pitch and size

[Andy Yan]

Hi Thomas,

At 2025-01-09 22:56:56, "Thomas Zimmermann" <tzimmermann@suse.de> wrote:
>Add drm_modes_size_dumb(), a helper to calculate the dumb-buffer
>scanline pitch and allocation size. Implementations of struct
>drm_driver.dumb_create can call the new helper for their size
>computations. There's currently quite a bit of code duplication
>among DRM's memory managers. Each calculates scanline pitch and
>buffer size from the given arguments, but the implementations are
>inconsistent in how they treat alignment and format support. Later
>patches will unify this code on top of drm_mode_size_dumb() as
>much as possible.
>
>drm_mode_size_dumb() uses existing 4CC format helpers to interpret the
>given color mode. This makes the dumb-buffer interface behave similar
>the kernel's video= parameter. Again, current per-driver implementations
>likely have subtle differences or bugs in how they support color modes.
>
>Future directions: one bug is present in the current input validation
>in drm_mode_create_dumb(). The dumb-buffer overflow tests round up any
>given bits-per-pixel value to a multiple of 8. So even one-bit formats,
>such as DRM_FORMAT_C1, require 8 bits per pixel. While not common,
>low-end displays use such formats; with a possible overcommitment of
>memory. At some point, the validation logic in drm_mode_size_dumb() is
>supposed to replace the erronous code.
>
>Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>---
> drivers/gpu/drm/drm_dumb_buffers.c | 93 ++++++++++++++++++++++++++++++
> include/drm/drm_dumb_buffers.h     | 14 +++++
> 2 files changed, 107 insertions(+)
> create mode 100644 include/drm/drm_dumb_buffers.h
>
>diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
>index 9916aaf5b3f2..fd39720bd617 100644
>--- a/drivers/gpu/drm/drm_dumb_buffers.c
>+++ b/drivers/gpu/drm/drm_dumb_buffers.c
>@@ -25,6 +25,8 @@
> 
> #include <drm/drm_device.h>
> #include <drm/drm_drv.h>
>+#include <drm/drm_dumb_buffers.h>
>+#include <drm/drm_fourcc.h>
> #include <drm/drm_gem.h>
> #include <drm/drm_mode.h>
> 
>@@ -57,6 +59,97 @@
>  * a hardware-specific ioctl to allocate suitable buffer objects.
>  */
> 
>+static int drm_mode_align_dumb(struct drm_mode_create_dumb *args,
>+			       unsigned long pitch_align,
>+			       unsigned long size_align)
>+{
>+	u32 pitch = args->pitch;
>+	u32 size;
>+
>+	if (!pitch)
>+		return -EINVAL;
>+
>+	if (pitch_align)
>+		pitch = roundup(pitch, pitch_align);
>+
>+	/* overflow checks for 32bit size calculations */
>+	if (args->height > U32_MAX / pitch)
>+		return -EINVAL;
>+
>+	if (!size_align)
>+		size_align = PAGE_SIZE;
>+	else if (!IS_ALIGNED(size_align, PAGE_SIZE))
>+		return -EINVAL;
>+
>+	size = ALIGN(args->height * pitch, size_align);
>+	if (!size)
>+		return -EINVAL;
>+
>+	args->pitch = pitch;
>+	args->size = size;
>+
>+	return 0;
>+}
>+
>+/**
>+ * drm_mode_size_dumb - Calculates the scanline and buffer sizes for dumb buffers
>+ * @dev: DRM device
>+ * @args: Parameters for the dumb buffer
>+ * @pitch_align: Scanline alignment in bytes
>+ * @size_align: Buffer-size alignment in bytes
>+ *
>+ * The helper drm_mode_size_dumb() calculates the size of the buffer
>+ * allocation and the scanline size for a dumb buffer. Callers have to
>+ * set the buffers width, height and color mode in the argument @arg.
>+ * The helper validates the correctness of the input and tests for
>+ * possible overflows. If successful, it returns the dumb buffer's
>+ * required scanline pitch and size in &args.
>+ *
>+ * The parameter @pitch_align allows the driver to specifies an
>+ * alignment for the scanline pitch, if the hardware requires any. The
>+ * calculated pitch will be a multiple of the alignment. The parameter
>+ * @size_align allows to specify an alignment for buffer sizes. The
>+ * returned size is always a multiple of PAGE_SIZE.
>+ *
>+ * Returns:
>+ * Zero on success, or a negative error code otherwise.
>+ */
>+int drm_mode_size_dumb(struct drm_device *dev,
>+		       struct drm_mode_create_dumb *args,
>+		       unsigned long pitch_align,
>+		       unsigned long size_align)
>+{
>+	u32 fourcc;
>+	const struct drm_format_info *info;
>+	u64 pitch;
>+
>+	/*
>+	 * The scanline pitch depends on the buffer width and the color
>+	 * format. The latter is specified as a color-mode constant for
>+	 * which we first have to find the corresponding color format.
>+	 *
>+	 * Different color formats can have the same color-mode constant.
>+	 * For example XRGB8888 and BGRX8888 both have a color mode of 32.
>+	 * It is possible to use different formats for dumb-buffer allocation
>+	 * and rendering as long as all involved formats share the same
>+	 * color-mode constant.
>+	 */
>+	fourcc = drm_driver_color_mode_format(dev, args->bpp);

This will return -EINVAL with bpp drm_mode_legacy_fb_format doesn't support,
such as(NV15, NV20, NV30, bpp is 10)[0]

And there are also some AFBC based format with bpp can't be handled here, see:
static __u32 drm_gem_afbc_get_bpp(struct drm_device *dev,
                                  const struct drm_mode_fb_cmd2 *mode_cmd)
{               
        const struct drm_format_info *info;
                
        info = drm_get_format_info(dev, mode_cmd);
                
        switch (info->format) {
        case DRM_FORMAT_YUV420_8BIT:       
                return 12;
        case DRM_FORMAT_YUV420_10BIT:      
                return 15;                 
        case DRM_FORMAT_VUY101010:         
                return 30;                 
        default:
                return drm_format_info_bpp(info, 0);
        }
}



[0]https://gitlab.freedesktop.org/mesa/drm/-/blob/main/tests/modetest/buffers.c?ref_type=heads#L159

This introduce a modetest failure on rockchip platform:
# modetest -M rockchip -s 70@68:1920x1080 -P 32@68:1920x1080@NV30
setting mode 1920x1080-60.00Hz on connectors 70, crtc 68
testing 1920x1080@NV30 overlay plane 32
failed to create dumb buffer: Invalid argument

I think other platform with bpp can't handler by  drm_mode_legacy_fb_format will
also see this kind of failure:



>+	if (fourcc == DRM_FORMAT_INVALID)
>+		return -EINVAL;
>+	info = drm_format_info(fourcc);
>+	if (!info)
>+		return -EINVAL;
>+	pitch = drm_format_info_min_pitch(info, 0, args->width);
>+	if (!pitch || pitch > U32_MAX)
>+		return -EINVAL;
>+
>+	args->pitch = pitch;
>+
>+	return drm_mode_align_dumb(args, pitch_align, size_align);
>+}
>+EXPORT_SYMBOL(drm_mode_size_dumb);
>+
> int drm_mode_create_dumb(struct drm_device *dev,
> 			 struct drm_mode_create_dumb *args,
> 			 struct drm_file *file_priv)
>diff --git a/include/drm/drm_dumb_buffers.h b/include/drm/drm_dumb_buffers.h
>new file mode 100644
>index 000000000000..6fe36004b19d
>--- /dev/null
>+++ b/include/drm/drm_dumb_buffers.h
>@@ -0,0 +1,14 @@
>+/* SPDX-License-Identifier: MIT */
>+
>+#ifndef __DRM_DUMB_BUFFERS_H__
>+#define __DRM_DUMB_BUFFERS_H__
>+
>+struct drm_device;
>+struct drm_mode_create_dumb;
>+
>+int drm_mode_size_dumb(struct drm_device *dev,
>+		       struct drm_mode_create_dumb *args,
>+		       unsigned long pitch_align,
>+		       unsigned long size_align);
>+
>+#endif
>-- 
>2.47.1
>
>
>_______________________________________________
>Linux-rockchip mailing list
>Linux-rockchip@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 02/25] drm/dumb-buffers: Provide helper to set pitch and size

[Thomas Zimmermann]

[-- Attachment #1: Type: text/plain, Size: 8886 bytes --]

Hi


Am 10.01.25 um 02:49 schrieb Andy Yan:
> Hi Thomas,
>
> At 2025-01-09 22:56:56, "Thomas Zimmermann" <tzimmermann@suse.de> wrote:
>> Add drm_modes_size_dumb(), a helper to calculate the dumb-buffer
>> scanline pitch and allocation size. Implementations of struct
>> drm_driver.dumb_create can call the new helper for their size
>> computations. There's currently quite a bit of code duplication
>> among DRM's memory managers. Each calculates scanline pitch and
>> buffer size from the given arguments, but the implementations are
>> inconsistent in how they treat alignment and format support. Later
>> patches will unify this code on top of drm_mode_size_dumb() as
>> much as possible.
>>
>> drm_mode_size_dumb() uses existing 4CC format helpers to interpret the
>> given color mode. This makes the dumb-buffer interface behave similar
>> the kernel's video= parameter. Again, current per-driver implementations
>> likely have subtle differences or bugs in how they support color modes.
>>
>> Future directions: one bug is present in the current input validation
>> in drm_mode_create_dumb(). The dumb-buffer overflow tests round up any
>> given bits-per-pixel value to a multiple of 8. So even one-bit formats,
>> such as DRM_FORMAT_C1, require 8 bits per pixel. While not common,
>> low-end displays use such formats; with a possible overcommitment of
>> memory. At some point, the validation logic in drm_mode_size_dumb() is
>> supposed to replace the erronous code.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>> drivers/gpu/drm/drm_dumb_buffers.c | 93 ++++++++++++++++++++++++++++++
>> include/drm/drm_dumb_buffers.h     | 14 +++++
>> 2 files changed, 107 insertions(+)
>> create mode 100644 include/drm/drm_dumb_buffers.h
>>
>> diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
>> index 9916aaf5b3f2..fd39720bd617 100644
>> --- a/drivers/gpu/drm/drm_dumb_buffers.c
>> +++ b/drivers/gpu/drm/drm_dumb_buffers.c
>> @@ -25,6 +25,8 @@
>>
>> #include <drm/drm_device.h>
>> #include <drm/drm_drv.h>
>> +#include <drm/drm_dumb_buffers.h>
>> +#include <drm/drm_fourcc.h>
>> #include <drm/drm_gem.h>
>> #include <drm/drm_mode.h>
>>
>> @@ -57,6 +59,97 @@
>>   * a hardware-specific ioctl to allocate suitable buffer objects.
>>   */
>>
>> +static int drm_mode_align_dumb(struct drm_mode_create_dumb *args,
>> +			       unsigned long pitch_align,
>> +			       unsigned long size_align)
>> +{
>> +	u32 pitch = args->pitch;
>> +	u32 size;
>> +
>> +	if (!pitch)
>> +		return -EINVAL;
>> +
>> +	if (pitch_align)
>> +		pitch = roundup(pitch, pitch_align);
>> +
>> +	/* overflow checks for 32bit size calculations */
>> +	if (args->height > U32_MAX / pitch)
>> +		return -EINVAL;
>> +
>> +	if (!size_align)
>> +		size_align = PAGE_SIZE;
>> +	else if (!IS_ALIGNED(size_align, PAGE_SIZE))
>> +		return -EINVAL;
>> +
>> +	size = ALIGN(args->height * pitch, size_align);
>> +	if (!size)
>> +		return -EINVAL;
>> +
>> +	args->pitch = pitch;
>> +	args->size = size;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * drm_mode_size_dumb - Calculates the scanline and buffer sizes for dumb buffers
>> + * @dev: DRM device
>> + * @args: Parameters for the dumb buffer
>> + * @pitch_align: Scanline alignment in bytes
>> + * @size_align: Buffer-size alignment in bytes
>> + *
>> + * The helper drm_mode_size_dumb() calculates the size of the buffer
>> + * allocation and the scanline size for a dumb buffer. Callers have to
>> + * set the buffers width, height and color mode in the argument @arg.
>> + * The helper validates the correctness of the input and tests for
>> + * possible overflows. If successful, it returns the dumb buffer's
>> + * required scanline pitch and size in &args.
>> + *
>> + * The parameter @pitch_align allows the driver to specifies an
>> + * alignment for the scanline pitch, if the hardware requires any. The
>> + * calculated pitch will be a multiple of the alignment. The parameter
>> + * @size_align allows to specify an alignment for buffer sizes. The
>> + * returned size is always a multiple of PAGE_SIZE.
>> + *
>> + * Returns:
>> + * Zero on success, or a negative error code otherwise.
>> + */
>> +int drm_mode_size_dumb(struct drm_device *dev,
>> +		       struct drm_mode_create_dumb *args,
>> +		       unsigned long pitch_align,
>> +		       unsigned long size_align)
>> +{
>> +	u32 fourcc;
>> +	const struct drm_format_info *info;
>> +	u64 pitch;
>> +
>> +	/*
>> +	 * The scanline pitch depends on the buffer width and the color
>> +	 * format. The latter is specified as a color-mode constant for
>> +	 * which we first have to find the corresponding color format.
>> +	 *
>> +	 * Different color formats can have the same color-mode constant.
>> +	 * For example XRGB8888 and BGRX8888 both have a color mode of 32.
>> +	 * It is possible to use different formats for dumb-buffer allocation
>> +	 * and rendering as long as all involved formats share the same
>> +	 * color-mode constant.
>> +	 */
>> +	fourcc = drm_driver_color_mode_format(dev, args->bpp);
> This will return -EINVAL with bpp drm_mode_legacy_fb_format doesn't support,
> such as(NV15, NV20, NV30, bpp is 10)[0]

Thanks for taking a look. That NV-related code at [0] is a 'somewhat 
non-idiomatic use' of the UAPI. The dumb-buffer interface really just 
supports a single plane. The fix would be a new ioctl that takes a DRM 
4cc constant and returns a buffer handle/pitch/size for each plane. But 
that's separate series throughout the various components.

There's also code XRGB16161616F. This is a viable format for the UAPI, 
but seems not very useful in practice.

>
> And there are also some AFBC based format with bpp can't be handled here, see:
> static __u32 drm_gem_afbc_get_bpp(struct drm_device *dev,
>                                    const struct drm_mode_fb_cmd2 *mode_cmd)
> {
>          const struct drm_format_info *info;
>                  
>          info = drm_get_format_info(dev, mode_cmd);
>                  
>          switch (info->format) {
>          case DRM_FORMAT_YUV420_8BIT:
>                  return 12;
>          case DRM_FORMAT_YUV420_10BIT:
>                  return 15;
>          case DRM_FORMAT_VUY101010:
>                  return 30;
>          default:
>                  return drm_format_info_bpp(info, 0);
>          }
> }

Same problem here. These YUV formats are multi-planar and there should 
be no dumb buffers for them.

As we still have to support these all use cases, I've modified the new 
helper to fallback to computing the pitch from the given bpp value. 
That's what drivers currently do. Could you please apply the attached 
patch on top of the series and report back the result of the test? You 
should see a kernel warning about the unknown color mode, but allocation 
should succeed.

Best regards
Thomas

>
>
> [0]https://gitlab.freedesktop.org/mesa/drm/-/blob/main/tests/modetest/buffers.c?ref_type=heads#L159
>
> This introduce a modetest failure on rockchip platform:
> # modetest -M rockchip -s 70@68:1920x1080 -P 32@68:1920x1080@NV30
> setting mode 1920x1080-60.00Hz on connectors 70, crtc 68
> testing 1920x1080@NV30 overlay plane 32
> failed to create dumb buffer: Invalid argument
>
> I think other platform with bpp can't handler by  drm_mode_legacy_fb_format will
> also see this kind of failure:
>
>
>
>> +	if (fourcc == DRM_FORMAT_INVALID)
>> +		return -EINVAL;
>> +	info = drm_format_info(fourcc);
>> +	if (!info)
>> +		return -EINVAL;
>> +	pitch = drm_format_info_min_pitch(info, 0, args->width);
>> +	if (!pitch || pitch > U32_MAX)
>> +		return -EINVAL;
>> +
>> +	args->pitch = pitch;
>> +
>> +	return drm_mode_align_dumb(args, pitch_align, size_align);
>> +}
>> +EXPORT_SYMBOL(drm_mode_size_dumb);
>> +
>> int drm_mode_create_dumb(struct drm_device *dev,
>> 			 struct drm_mode_create_dumb *args,
>> 			 struct drm_file *file_priv)
>> diff --git a/include/drm/drm_dumb_buffers.h b/include/drm/drm_dumb_buffers.h
>> new file mode 100644
>> index 000000000000..6fe36004b19d
>> --- /dev/null
>> +++ b/include/drm/drm_dumb_buffers.h
>> @@ -0,0 +1,14 @@
>> +/* SPDX-License-Identifier: MIT */
>> +
>> +#ifndef __DRM_DUMB_BUFFERS_H__
>> +#define __DRM_DUMB_BUFFERS_H__
>> +
>> +struct drm_device;
>> +struct drm_mode_create_dumb;
>> +
>> +int drm_mode_size_dumb(struct drm_device *dev,
>> +		       struct drm_mode_create_dumb *args,
>> +		       unsigned long pitch_align,
>> +		       unsigned long size_align);
>> +
>> +#endif
>> -- 
>> 2.47.1
>>
>>
>> _______________________________________________
>> Linux-rockchip mailing list
>> Linux-rockchip@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: 0001-add-fallback-for-unknown-bpp.patch --]
[-- Type: text/x-patch, Size: 1881 bytes --]

From 2e7005654d76b71f78fe07fcf98a3570022f5034 Mon Sep 17 00:00:00 2001
From: Thomas Zimmermann <tzimmermann@suse.de>
Date: Fri, 10 Jan 2025 09:35:12 +0100
Subject: [PATCH] add fallback for unknown bpp

---
 drivers/gpu/drm/drm_dumb_buffers.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
index fd39720bd617..5f2d026c764c 100644
--- a/drivers/gpu/drm/drm_dumb_buffers.c
+++ b/drivers/gpu/drm/drm_dumb_buffers.c
@@ -119,9 +119,8 @@ int drm_mode_size_dumb(struct drm_device *dev,
 		       unsigned long pitch_align,
 		       unsigned long size_align)
 {
+	u64 pitch = 0;
 	u32 fourcc;
-	const struct drm_format_info *info;
-	u64 pitch;
 
 	/*
 	 * The scanline pitch depends on the buffer width and the color
@@ -135,12 +134,25 @@ int drm_mode_size_dumb(struct drm_device *dev,
 	 * color-mode constant.
 	 */
 	fourcc = drm_driver_color_mode_format(dev, args->bpp);
-	if (fourcc == DRM_FORMAT_INVALID)
-		return -EINVAL;
-	info = drm_format_info(fourcc);
-	if (!info)
-		return -EINVAL;
-	pitch = drm_format_info_min_pitch(info, 0, args->width);
+	if (fourcc != DRM_FORMAT_INVALID) {
+		const struct drm_format_info *info = drm_format_info(fourcc);
+
+		if (!info)
+			return -EINVAL;
+		pitch = drm_format_info_min_pitch(info, 0, args->width);
+	} else if (args->bpp) {
+		/*
+		 * Some userspace throws in arbitrary values for bpp and
+		 * relies on the kernel to figure it out. In this case we
+		 * fall back to the old method of using bpp directly.
+		 */
+		drm_warn(dev, "Unknown color mode %d; guessing buffer size.\n", args->bpp);
+		if (args->bpp < 8)
+			pitch = DIV_ROUND_UP(args->width * args->bpp, SZ_8);
+		else
+			pitch = args->width * DIV_ROUND_UP(args->bpp, SZ_8);
+	}
+
 	if (!pitch || pitch > U32_MAX)
 		return -EINVAL;
 
-- 
2.47.1

Re:Re: [PATCH v2 02/25] drm/dumb-buffers: Provide helper to set pitch and size

[Andy Yan]

Hi Thomas,

At 2025-01-10 21:23:48, "Thomas Zimmermann" <tzimmermann@suse.de> wrote:
>Hi
>
>
>Am 10.01.25 um 02:49 schrieb Andy Yan:
>> Hi Thomas,
>>
>> At 2025-01-09 22:56:56, "Thomas Zimmermann" <tzimmermann@suse.de> wrote:
>>> Add drm_modes_size_dumb(), a helper to calculate the dumb-buffer
>>> scanline pitch and allocation size. Implementations of struct
>>> drm_driver.dumb_create can call the new helper for their size
>>> computations. There's currently quite a bit of code duplication
>>> among DRM's memory managers. Each calculates scanline pitch and
>>> buffer size from the given arguments, but the implementations are
>>> inconsistent in how they treat alignment and format support. Later
>>> patches will unify this code on top of drm_mode_size_dumb() as
>>> much as possible.
>>>
>>> drm_mode_size_dumb() uses existing 4CC format helpers to interpret the
>>> given color mode. This makes the dumb-buffer interface behave similar
>>> the kernel's video= parameter. Again, current per-driver implementations
>>> likely have subtle differences or bugs in how they support color modes.
>>>
>>> Future directions: one bug is present in the current input validation
>>> in drm_mode_create_dumb(). The dumb-buffer overflow tests round up any
>>> given bits-per-pixel value to a multiple of 8. So even one-bit formats,
>>> such as DRM_FORMAT_C1, require 8 bits per pixel. While not common,
>>> low-end displays use such formats; with a possible overcommitment of
>>> memory. At some point, the validation logic in drm_mode_size_dumb() is
>>> supposed to replace the erronous code.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>> drivers/gpu/drm/drm_dumb_buffers.c | 93 ++++++++++++++++++++++++++++++
>>> include/drm/drm_dumb_buffers.h     | 14 +++++
>>> 2 files changed, 107 insertions(+)
>>> create mode 100644 include/drm/drm_dumb_buffers.h
>>>
>>> diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
>>> index 9916aaf5b3f2..fd39720bd617 100644
>>> --- a/drivers/gpu/drm/drm_dumb_buffers.c
>>> +++ b/drivers/gpu/drm/drm_dumb_buffers.c
>>> @@ -25,6 +25,8 @@
>>>
>>> #include <drm/drm_device.h>
>>> #include <drm/drm_drv.h>
>>> +#include <drm/drm_dumb_buffers.h>
>>> +#include <drm/drm_fourcc.h>
>>> #include <drm/drm_gem.h>
>>> #include <drm/drm_mode.h>
>>>
>>> @@ -57,6 +59,97 @@
>>>   * a hardware-specific ioctl to allocate suitable buffer objects.
>>>   */
>>>
>>> +static int drm_mode_align_dumb(struct drm_mode_create_dumb *args,
>>> +			       unsigned long pitch_align,
>>> +			       unsigned long size_align)
>>> +{
>>> +	u32 pitch = args->pitch;
>>> +	u32 size;
>>> +
>>> +	if (!pitch)
>>> +		return -EINVAL;
>>> +
>>> +	if (pitch_align)
>>> +		pitch = roundup(pitch, pitch_align);
>>> +
>>> +	/* overflow checks for 32bit size calculations */
>>> +	if (args->height > U32_MAX / pitch)
>>> +		return -EINVAL;
>>> +
>>> +	if (!size_align)
>>> +		size_align = PAGE_SIZE;
>>> +	else if (!IS_ALIGNED(size_align, PAGE_SIZE))
>>> +		return -EINVAL;
>>> +
>>> +	size = ALIGN(args->height * pitch, size_align);
>>> +	if (!size)
>>> +		return -EINVAL;
>>> +
>>> +	args->pitch = pitch;
>>> +	args->size = size;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +/**
>>> + * drm_mode_size_dumb - Calculates the scanline and buffer sizes for dumb buffers
>>> + * @dev: DRM device
>>> + * @args: Parameters for the dumb buffer
>>> + * @pitch_align: Scanline alignment in bytes
>>> + * @size_align: Buffer-size alignment in bytes
>>> + *
>>> + * The helper drm_mode_size_dumb() calculates the size of the buffer
>>> + * allocation and the scanline size for a dumb buffer. Callers have to
>>> + * set the buffers width, height and color mode in the argument @arg.
>>> + * The helper validates the correctness of the input and tests for
>>> + * possible overflows. If successful, it returns the dumb buffer's
>>> + * required scanline pitch and size in &args.
>>> + *
>>> + * The parameter @pitch_align allows the driver to specifies an
>>> + * alignment for the scanline pitch, if the hardware requires any. The
>>> + * calculated pitch will be a multiple of the alignment. The parameter
>>> + * @size_align allows to specify an alignment for buffer sizes. The
>>> + * returned size is always a multiple of PAGE_SIZE.
>>> + *
>>> + * Returns:
>>> + * Zero on success, or a negative error code otherwise.
>>> + */
>>> +int drm_mode_size_dumb(struct drm_device *dev,
>>> +		       struct drm_mode_create_dumb *args,
>>> +		       unsigned long pitch_align,
>>> +		       unsigned long size_align)
>>> +{
>>> +	u32 fourcc;
>>> +	const struct drm_format_info *info;
>>> +	u64 pitch;
>>> +
>>> +	/*
>>> +	 * The scanline pitch depends on the buffer width and the color
>>> +	 * format. The latter is specified as a color-mode constant for
>>> +	 * which we first have to find the corresponding color format.
>>> +	 *
>>> +	 * Different color formats can have the same color-mode constant.
>>> +	 * For example XRGB8888 and BGRX8888 both have a color mode of 32.
>>> +	 * It is possible to use different formats for dumb-buffer allocation
>>> +	 * and rendering as long as all involved formats share the same
>>> +	 * color-mode constant.
>>> +	 */
>>> +	fourcc = drm_driver_color_mode_format(dev, args->bpp);
>> This will return -EINVAL with bpp drm_mode_legacy_fb_format doesn't support,
>> such as(NV15, NV20, NV30, bpp is 10)[0]
>
>Thanks for taking a look. That NV-related code at [0] is a 'somewhat 
>non-idiomatic use' of the UAPI. The dumb-buffer interface really just 
>supports a single plane. The fix would be a new ioctl that takes a DRM 
>4cc constant and returns a buffer handle/pitch/size for each plane. But 
>that's separate series throughout the various components.

So is there a standard way to create buffer for NV-related format now ?
With a quick search, I can see many user space use dumb-buffer for NV-releated
buffer alloc:

[0]https://github.com/tomba/kmsxx/blob/master/kms%2B%2B/src/pixelformats.cpp
[1]https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/lib/igt_fb.c?ref_type=heads
[2]https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-bad/sys/kms/gstkmsutils.c?ref_type=heads#L116

>
>There's also code XRGB16161616F. This is a viable format for the UAPI, 
>but seems not very useful in practice.
>
>>
>> And there are also some AFBC based format with bpp can't be handled here, see:
>> static __u32 drm_gem_afbc_get_bpp(struct drm_device *dev,
>>                                    const struct drm_mode_fb_cmd2 *mode_cmd)
>> {
>>          const struct drm_format_info *info;
>>                  
>>          info = drm_get_format_info(dev, mode_cmd);
>>                  
>>          switch (info->format) {
>>          case DRM_FORMAT_YUV420_8BIT:
>>                  return 12;
>>          case DRM_FORMAT_YUV420_10BIT:
>>                  return 15;
>>          case DRM_FORMAT_VUY101010:
>>                  return 30;
>>          default:
>>                  return drm_format_info_bpp(info, 0);
>>          }
>> }
>
>Same problem here. These YUV formats are multi-planar and there should 
>be no dumb buffers for them.

These afbc based format are one plane, see:

/*
 * 1-plane YUV 4:2:0
 * In these formats, the component ordering is specified (Y, followed by U
 * then V), but the exact Linear layout is undefined.
 * These formats can only be used with a non-Linear modifier.
 */
#define DRM_FORMAT_YUV420_8BIT  fourcc_code('Y', 'U', '0', '8')
#define DRM_FORMAT_YUV420_10BIT fourcc_code('Y', 'U', '1', '0')

>
>As we still have to support these all use cases, I've modified the new 
>helper to fallback to computing the pitch from the given bpp value. 
>That's what drivers currently do. Could you please apply the attached 
>patch on top of the series and report back the result of the test? You 
>should see a kernel warning about the unknown color mode, but allocation 
>should succeed.

Yes, the attached patch works for my test case.

>
>Best regards
>Thomas
>
>>
>>
>> [0]https://gitlab.freedesktop.org/mesa/drm/-/blob/main/tests/modetest/buffers.c?ref_type=heads#L159
>>
>> This introduce a modetest failure on rockchip platform:
>> # modetest -M rockchip -s 70@68:1920x1080 -P 32@68:1920x1080@NV30
>> setting mode 1920x1080-60.00Hz on connectors 70, crtc 68
>> testing 1920x1080@NV30 overlay plane 32
>> failed to create dumb buffer: Invalid argument
>>
>> I think other platform with bpp can't handler by  drm_mode_legacy_fb_format will
>> also see this kind of failure:
>>
>>
>>
>>> +	if (fourcc == DRM_FORMAT_INVALID)
>>> +		return -EINVAL;
>>> +	info = drm_format_info(fourcc);
>>> +	if (!info)
>>> +		return -EINVAL;
>>> +	pitch = drm_format_info_min_pitch(info, 0, args->width);
>>> +	if (!pitch || pitch > U32_MAX)
>>> +		return -EINVAL;
>>> +
>>> +	args->pitch = pitch;
>>> +
>>> +	return drm_mode_align_dumb(args, pitch_align, size_align);
>>> +}
>>> +EXPORT_SYMBOL(drm_mode_size_dumb);
>>> +
>>> int drm_mode_create_dumb(struct drm_device *dev,
>>> 			 struct drm_mode_create_dumb *args,
>>> 			 struct drm_file *file_priv)
>>> diff --git a/include/drm/drm_dumb_buffers.h b/include/drm/drm_dumb_buffers.h
>>> new file mode 100644
>>> index 000000000000..6fe36004b19d
>>> --- /dev/null
>>> +++ b/include/drm/drm_dumb_buffers.h
>>> @@ -0,0 +1,14 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +
>>> +#ifndef __DRM_DUMB_BUFFERS_H__
>>> +#define __DRM_DUMB_BUFFERS_H__
>>> +
>>> +struct drm_device;
>>> +struct drm_mode_create_dumb;
>>> +
>>> +int drm_mode_size_dumb(struct drm_device *dev,
>>> +		       struct drm_mode_create_dumb *args,
>>> +		       unsigned long pitch_align,
>>> +		       unsigned long size_align);
>>> +
>>> +#endif
>>> -- 
>>> 2.47.1
>>>
>>>
>>> _______________________________________________
>>> Linux-rockchip mailing list
>>> Linux-rockchip@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>-- 
>--
>Thomas Zimmermann
>Graphics Driver Developer
>SUSE Software Solutions Germany GmbH
>Frankenstrasse 146, 90461 Nuernberg, Germany
>GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
>HRB 36809 (AG Nuernberg)

Re: [PATCH v2 02/25] drm/dumb-buffers: Provide helper to set pitch and size

[Thomas Zimmermann]

Hi


Am 13.01.25 um 04:53 schrieb Andy Yan:
[...]
>> Thanks for taking a look. That NV-related code at [0] is a 'somewhat
>> non-idiomatic use' of the UAPI. The dumb-buffer interface really just
>> supports a single plane. The fix would be a new ioctl that takes a DRM
>> 4cc constant and returns a buffer handle/pitch/size for each plane. But
>> that's separate series throughout the various components.
> So is there a standard way to create buffer for NV-related format now ?

I don't know, but it doesn't look like there is. As I outlined, a new 
dumb-buffer interface seems required.

> With a quick search, I can see many user space use dumb-buffer for NV-releated
> buffer alloc:
>
> [0]https://github.com/tomba/kmsxx/blob/master/kms%2B%2B/src/pixelformats.cpp
> [1]https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/lib/igt_fb.c?ref_type=heads
> [2]https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-bad/sys/kms/gstkmsutils.c?ref_type=heads#L116
>
>> There's also code XRGB16161616F. This is a viable format for the UAPI,
>> but seems not very useful in practice.
>>
>>> And there are also some AFBC based format with bpp can't be handled here, see:
>>> static __u32 drm_gem_afbc_get_bpp(struct drm_device *dev,
>>>                                     const struct drm_mode_fb_cmd2 *mode_cmd)
>>> {
>>>           const struct drm_format_info *info;
>>>                   
>>>           info = drm_get_format_info(dev, mode_cmd);
>>>                   
>>>           switch (info->format) {
>>>           case DRM_FORMAT_YUV420_8BIT:
>>>                   return 12;
>>>           case DRM_FORMAT_YUV420_10BIT:
>>>                   return 15;
>>>           case DRM_FORMAT_VUY101010:
>>>                   return 30;
>>>           default:
>>>                   return drm_format_info_bpp(info, 0);
>>>           }
>>> }
>> Same problem here. These YUV formats are multi-planar and there should
>> be no dumb buffers for them.
> These afbc based format are one plane, see:

Apologies. I confused them with other YUV formats.

>
> /*
>   * 1-plane YUV 4:2:0
>   * In these formats, the component ordering is specified (Y, followed by U
>   * then V), but the exact Linear layout is undefined.
>   * These formats can only be used with a non-Linear modifier.
>   */
> #define DRM_FORMAT_YUV420_8BIT  fourcc_code('Y', 'U', '0', '8')
> #define DRM_FORMAT_YUV420_10BIT fourcc_code('Y', 'U', '1', '0')
>
>> As we still have to support these all use cases, I've modified the new
>> helper to fallback to computing the pitch from the given bpp value.
>> That's what drivers currently do. Could you please apply the attached
>> patch on top of the series and report back the result of the test? You
>> should see a kernel warning about the unknown color mode, but allocation
>> should succeed.
> Yes, the attached patch works for my test case.

Thanks for testing. I'll include the changes in the patch' next iteration.

Best regards
Thomas

>
>> Best regards
>> Thomas
>>
>>>
>>> [0]https://gitlab.freedesktop.org/mesa/drm/-/blob/main/tests/modetest/buffers.c?ref_type=heads#L159
>>>
>>> This introduce a modetest failure on rockchip platform:
>>> # modetest -M rockchip -s 70@68:1920x1080 -P 32@68:1920x1080@NV30
>>> setting mode 1920x1080-60.00Hz on connectors 70, crtc 68
>>> testing 1920x1080@NV30 overlay plane 32
>>> failed to create dumb buffer: Invalid argument
>>>
>>> I think other platform with bpp can't handler by  drm_mode_legacy_fb_format will
>>> also see this kind of failure:
>>>
>>>
>>>
>>>> +	if (fourcc == DRM_FORMAT_INVALID)
>>>> +		return -EINVAL;
>>>> +	info = drm_format_info(fourcc);
>>>> +	if (!info)
>>>> +		return -EINVAL;
>>>> +	pitch = drm_format_info_min_pitch(info, 0, args->width);
>>>> +	if (!pitch || pitch > U32_MAX)
>>>> +		return -EINVAL;
>>>> +
>>>> +	args->pitch = pitch;
>>>> +
>>>> +	return drm_mode_align_dumb(args, pitch_align, size_align);
>>>> +}
>>>> +EXPORT_SYMBOL(drm_mode_size_dumb);
>>>> +
>>>> int drm_mode_create_dumb(struct drm_device *dev,
>>>> 			 struct drm_mode_create_dumb *args,
>>>> 			 struct drm_file *file_priv)
>>>> diff --git a/include/drm/drm_dumb_buffers.h b/include/drm/drm_dumb_buffers.h
>>>> new file mode 100644
>>>> index 000000000000..6fe36004b19d
>>>> --- /dev/null
>>>> +++ b/include/drm/drm_dumb_buffers.h
>>>> @@ -0,0 +1,14 @@
>>>> +/* SPDX-License-Identifier: MIT */
>>>> +
>>>> +#ifndef __DRM_DUMB_BUFFERS_H__
>>>> +#define __DRM_DUMB_BUFFERS_H__
>>>> +
>>>> +struct drm_device;
>>>> +struct drm_mode_create_dumb;
>>>> +
>>>> +int drm_mode_size_dumb(struct drm_device *dev,
>>>> +		       struct drm_mode_create_dumb *args,
>>>> +		       unsigned long pitch_align,
>>>> +		       unsigned long size_align);
>>>> +
>>>> +#endif
>>>> -- 
>>>> 2.47.1
>>>>
>>>>
>>>> _______________________________________________
>>>> Linux-rockchip mailing list
>>>> Linux-rockchip@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>> -- 
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Frankenstrasse 146, 90461 Nuernberg, Germany
>> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
>> HRB 36809 (AG Nuernberg)

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[PATCH v2 03/25] drm/gem-dma: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. Align the pitch to a multiple of 8.

Push the current calculation into the only direct caller imx. Imx's
hardware requires the framebuffer width to be aligned to 8. The
driver's current approach is actually incorrect, as it only guarantees
this implicitly and requires bpp to be a multiple of 8 already. A
later commit will fix this problem by aligning the scanline pitch
such that an aligned width still fits into each scanline's memory.

A number of other drivers are build on top of gem-dma helpers and
implement their own dumb-buffer allocation. These drivers invoke
drm_gem_dma_dumb_create_internal(), which is not affected by this
commit.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_dma_helper.c     | 7 +++++--
 drivers/gpu/drm/imx/ipuv3/imx-drm-core.c | 2 ++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_dma_helper.c b/drivers/gpu/drm/drm_gem_dma_helper.c
index 16988d316a6d..5bca7ce3683f 100644
--- a/drivers/gpu/drm/drm_gem_dma_helper.c
+++ b/drivers/gpu/drm/drm_gem_dma_helper.c
@@ -20,6 +20,7 @@
 #include <drm/drm.h>
 #include <drm/drm_device.h>
 #include <drm/drm_drv.h>
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_gem_dma_helper.h>
 #include <drm/drm_vma_manager.h>
 
@@ -304,9 +305,11 @@ int drm_gem_dma_dumb_create(struct drm_file *file_priv,
 			    struct drm_mode_create_dumb *args)
 {
 	struct drm_gem_dma_object *dma_obj;
+	int ret;
 
-	args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
-	args->size = args->pitch * args->height;
+	ret = drm_mode_size_dumb(drm, args, SZ_8, 0);
+	if (ret)
+		return ret;
 
 	dma_obj = drm_gem_dma_create_with_handle(file_priv, drm, args->size,
 						 &args->handle);
diff --git a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
index ec5fd9a01f1e..e7025df7b978 100644
--- a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
@@ -145,6 +145,8 @@ static int imx_drm_dumb_create(struct drm_file *file_priv,
 	int ret;
 
 	args->width = ALIGN(width, 8);
+	args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
+	args->size = args->pitch * args->height;
 
 	ret = drm_gem_dma_dumb_create(file_priv, drm, args);
 	if (ret)
-- 
2.47.1

[PATCH v2 04/25] drm/gem-shmem: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. Align the pitch to a multiple of 8.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_shmem_helper.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 5ab351409312..8941b5e4eda9 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -18,6 +18,7 @@
 #include <drm/drm.h>
 #include <drm/drm_device.h>
 #include <drm/drm_drv.h>
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_prime.h>
 #include <drm/drm_print.h>
@@ -514,18 +515,11 @@ EXPORT_SYMBOL(drm_gem_shmem_purge);
 int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
 			      struct drm_mode_create_dumb *args)
 {
-	u32 min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
+	int ret;
 
-	if (!args->pitch || !args->size) {
-		args->pitch = min_pitch;
-		args->size = PAGE_ALIGN(args->pitch * args->height);
-	} else {
-		/* ensure sane minimum values */
-		if (args->pitch < min_pitch)
-			args->pitch = min_pitch;
-		if (args->size < args->pitch * args->height)
-			args->size = PAGE_ALIGN(args->pitch * args->height);
-	}
+	ret = drm_mode_size_dumb(dev, args, SZ_8, 0);
+	if (ret)
+		return ret;
 
 	return drm_gem_shmem_create_with_handle(file, dev, args->size, &args->handle);
 }
-- 
2.47.1

[PATCH v2 05/25] drm/gem-vram: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. Inline code from drm_gem_vram_fill_create_dumb() without
the existing size computation. Align the pitch to a multiple of 8.

Only hibmc and vboxvideo use gem-vram. Hibmc invokes the call to
drm_gem_vram_fill_create_dumb() directly and is therefore not affected.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_vram_helper.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 22b1fe9c03b8..15cd564cbeac 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -6,6 +6,7 @@
 #include <drm/drm_debugfs.h>
 #include <drm/drm_device.h>
 #include <drm/drm_drv.h>
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_file.h>
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_atomic_helper.h>
@@ -582,10 +583,31 @@ int drm_gem_vram_driver_dumb_create(struct drm_file *file,
 				    struct drm_device *dev,
 				    struct drm_mode_create_dumb *args)
 {
+	struct drm_gem_vram_object *gbo;
+	int ret;
+
 	if (WARN_ONCE(!dev->vram_mm, "VRAM MM not initialized"))
 		return -EINVAL;
 
-	return drm_gem_vram_fill_create_dumb(file, dev, 0, 0, args);
+	ret = drm_mode_size_dumb(dev, args, SZ_8, 0);
+	if (ret)
+		return ret;
+
+	gbo = drm_gem_vram_create(dev, args->size, 0);
+	if (IS_ERR(gbo))
+		return PTR_ERR(gbo);
+
+	ret = drm_gem_handle_create(file, &gbo->bo.base, &args->handle);
+	if (ret)
+		goto err_drm_gem_object_put;
+
+	drm_gem_object_put(&gbo->bo.base);
+
+	return 0;
+
+err_drm_gem_object_put:
+	drm_gem_object_put(&gbo->bo.base);
+	return ret;
 }
 EXPORT_SYMBOL(drm_gem_vram_driver_dumb_create);
 
-- 
2.47.1

[PATCH v2 06/25] drm/armada: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. No alignment required.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Russell King <linux@armlinux.org.uk>
---
 drivers/gpu/drm/armada/armada_gem.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
index 1a1680d71486..0f11ae06064a 100644
--- a/drivers/gpu/drm/armada/armada_gem.c
+++ b/drivers/gpu/drm/armada/armada_gem.c
@@ -9,6 +9,7 @@
 #include <linux/shmem_fs.h>
 
 #include <drm/armada_drm.h>
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_prime.h>
 
 #include "armada_drm.h"
@@ -244,14 +245,13 @@ int armada_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
 	struct drm_mode_create_dumb *args)
 {
 	struct armada_gem_object *dobj;
-	u32 handle;
-	size_t size;
 	int ret;
 
-	args->pitch = armada_pitch(args->width, args->bpp);
-	args->size = size = args->pitch * args->height;
+	ret = drm_mode_size_dumb(dev, args, 0, 0);
+	if (ret)
+		return ret;
 
-	dobj = armada_gem_alloc_private_object(dev, size);
+	dobj = armada_gem_alloc_private_object(dev, args->size);
 	if (dobj == NULL)
 		return -ENOMEM;
 
@@ -259,14 +259,12 @@ int armada_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
 	if (ret)
 		goto err;
 
-	ret = drm_gem_handle_create(file, &dobj->obj, &handle);
+	ret = drm_gem_handle_create(file, &dobj->obj, &args->handle);
 	if (ret)
 		goto err;
 
-	args->handle = handle;
-
 	/* drop reference from allocate - handle holds it now */
-	DRM_DEBUG_DRIVER("obj %p size %zu handle %#x\n", dobj, size, handle);
+	DRM_DEBUG_DRIVER("obj %p size %llu handle %#x\n", dobj, args->size, args->handle);
  err:
 	drm_gem_object_put(&dobj->obj);
 	return ret;
-- 
2.47.1

[PATCH v2 07/25] drm/exynos: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. No alignment required.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Inki Dae <inki.dae@samsung.com>
Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Alim Akhtar <alim.akhtar@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_gem.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
index 4787fee4696f..ffa1c02b4b1e 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -11,6 +11,7 @@
 #include <linux/shmem_fs.h>
 #include <linux/module.h>
 
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_prime.h>
 #include <drm/drm_vma_manager.h>
 #include <drm/exynos_drm.h>
@@ -330,15 +331,16 @@ int exynos_drm_gem_dumb_create(struct drm_file *file_priv,
 	unsigned int flags;
 	int ret;
 
+	ret = drm_mode_size_dumb(dev, args, 0, 0);
+	if (ret)
+		return ret;
+
 	/*
 	 * allocate memory to be used for framebuffer.
 	 * - this callback would be called by user application
 	 *	with DRM_IOCTL_MODE_CREATE_DUMB command.
 	 */
 
-	args->pitch = args->width * ((args->bpp + 7) / 8);
-	args->size = args->pitch * args->height;
-
 	if (is_drm_iommu_supported(dev))
 		flags = EXYNOS_BO_NONCONTIG | EXYNOS_BO_WC;
 	else
-- 
2.47.1

[PATCH v2 08/25] drm/gma500: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. Align the pitch to a multiple of 64.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
---
 drivers/gpu/drm/gma500/gem.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index 4b7627a72637..fc337db0a948 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -16,6 +16,7 @@
 #include <asm/set_memory.h>
 
 #include <drm/drm.h>
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_vma_manager.h>
 
 #include "gem.h"
@@ -199,35 +200,25 @@ psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen,
 int psb_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
 			struct drm_mode_create_dumb *args)
 {
-	size_t pitch, size;
 	struct psb_gem_object *pobj;
 	struct drm_gem_object *obj;
-	u32 handle;
 	int ret;
 
-	pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
-	pitch = ALIGN(pitch, 64);
-
-	size = pitch * args->height;
-	size = roundup(size, PAGE_SIZE);
-	if (!size)
-		return -EINVAL;
+	ret = drm_mode_size_dumb(dev, args, SZ_64, 0);
+	if (ret)
+		return ret;
 
-	pobj = psb_gem_create(dev, size, "gem", false, PAGE_SIZE);
+	pobj = psb_gem_create(dev, args->size, "gem", false, PAGE_SIZE);
 	if (IS_ERR(pobj))
 		return PTR_ERR(pobj);
 	obj = &pobj->base;
 
-	ret = drm_gem_handle_create(file, obj, &handle);
+	ret = drm_gem_handle_create(file, obj, &args->handle);
 	if (ret)
 		goto err_drm_gem_object_put;
 
 	drm_gem_object_put(obj);
 
-	args->pitch = pitch;
-	args->size = size;
-	args->handle = handle;
-
 	return 0;
 
 err_drm_gem_object_put:
-- 
2.47.1

[PATCH v2 09/25] drm/hibmc: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. Align the pitch to a multiple of 128.

The hibmc driver's new hibmc_dumb_create() is similar to the one
in GEM VRAM helpers. The driver was the only caller of
drm_gem_vram_fill_create_dumb(). Remove the now unused helper.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Xinliang Liu <xinliang.liu@linaro.org>
Cc: Tian Tao <tiantao6@hisilicon.com>
Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Yongqin Liu <yongqin.liu@linaro.org>
Cc: John Stultz <jstultz@google.com>
---
 drivers/gpu/drm/drm_gem_vram_helper.c         | 65 -------------------
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   | 25 ++++++-
 include/drm/drm_gem_vram_helper.h             |  6 --
 3 files changed, 24 insertions(+), 72 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 15cd564cbeac..b4cf8134df6d 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -444,71 +444,6 @@ void drm_gem_vram_vunmap(struct drm_gem_vram_object *gbo,
 }
 EXPORT_SYMBOL(drm_gem_vram_vunmap);
 
-/**
- * drm_gem_vram_fill_create_dumb() - Helper for implementing
- *				     &struct drm_driver.dumb_create
- *
- * @file:		the DRM file
- * @dev:		the DRM device
- * @pg_align:		the buffer's alignment in multiples of the page size
- * @pitch_align:	the scanline's alignment in powers of 2
- * @args:		the arguments as provided to
- *			&struct drm_driver.dumb_create
- *
- * This helper function fills &struct drm_mode_create_dumb, which is used
- * by &struct drm_driver.dumb_create. Implementations of this interface
- * should forwards their arguments to this helper, plus the driver-specific
- * parameters.
- *
- * Returns:
- * 0 on success, or
- * a negative error code otherwise.
- */
-int drm_gem_vram_fill_create_dumb(struct drm_file *file,
-				  struct drm_device *dev,
-				  unsigned long pg_align,
-				  unsigned long pitch_align,
-				  struct drm_mode_create_dumb *args)
-{
-	size_t pitch, size;
-	struct drm_gem_vram_object *gbo;
-	int ret;
-	u32 handle;
-
-	pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
-	if (pitch_align) {
-		if (WARN_ON_ONCE(!is_power_of_2(pitch_align)))
-			return -EINVAL;
-		pitch = ALIGN(pitch, pitch_align);
-	}
-	size = pitch * args->height;
-
-	size = roundup(size, PAGE_SIZE);
-	if (!size)
-		return -EINVAL;
-
-	gbo = drm_gem_vram_create(dev, size, pg_align);
-	if (IS_ERR(gbo))
-		return PTR_ERR(gbo);
-
-	ret = drm_gem_handle_create(file, &gbo->bo.base, &handle);
-	if (ret)
-		goto err_drm_gem_object_put;
-
-	drm_gem_object_put(&gbo->bo.base);
-
-	args->pitch = pitch;
-	args->size = size;
-	args->handle = handle;
-
-	return 0;
-
-err_drm_gem_object_put:
-	drm_gem_object_put(&gbo->bo.base);
-	return ret;
-}
-EXPORT_SYMBOL(drm_gem_vram_fill_create_dumb);
-
 /*
  * Helpers for struct ttm_device_funcs
  */
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index e6de6d5edf6b..81768577871f 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -18,10 +18,12 @@
 #include <drm/clients/drm_client_setup.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_drv.h>
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_fbdev_ttm.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_gem_vram_helper.h>
 #include <drm/drm_managed.h>
+#include <drm/drm_mode.h>
 #include <drm/drm_module.h>
 #include <drm/drm_vblank.h>
 
@@ -54,7 +56,28 @@ static irqreturn_t hibmc_interrupt(int irq, void *arg)
 static int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
 			     struct drm_mode_create_dumb *args)
 {
-	return drm_gem_vram_fill_create_dumb(file, dev, 0, 128, args);
+	struct drm_gem_vram_object *gbo;
+	int ret;
+
+	ret = drm_mode_size_dumb(dev, args, SZ_128, 0);
+	if (ret)
+		return ret;
+
+	gbo = drm_gem_vram_create(dev, args->size, 0);
+	if (IS_ERR(gbo))
+		return PTR_ERR(gbo);
+
+	ret = drm_gem_handle_create(file, &gbo->bo.base, &args->handle);
+	if (ret)
+		goto err_drm_gem_object_put;
+
+	drm_gem_object_put(&gbo->bo.base);
+
+	return 0;
+
+err_drm_gem_object_put:
+	drm_gem_object_put(&gbo->bo.base);
+	return ret;
 }
 
 static const struct drm_driver hibmc_driver = {
diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
index 00830b49a3ff..b6e821f5dd03 100644
--- a/include/drm/drm_gem_vram_helper.h
+++ b/include/drm/drm_gem_vram_helper.h
@@ -100,12 +100,6 @@ int drm_gem_vram_vmap(struct drm_gem_vram_object *gbo, struct iosys_map *map);
 void drm_gem_vram_vunmap(struct drm_gem_vram_object *gbo,
 			 struct iosys_map *map);
 
-int drm_gem_vram_fill_create_dumb(struct drm_file *file,
-				  struct drm_device *dev,
-				  unsigned long pg_align,
-				  unsigned long pitch_align,
-				  struct drm_mode_create_dumb *args);
-
 /*
  * Helpers for struct drm_driver
  */
-- 
2.47.1

[PATCH v2 10/25] drm/imx/ipuv3: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. The hardware requires the framebuffer width to be a
multiple of 8. The scanline pitch has be large enough to support
this. Therefore compute the byte size of 8 pixels in the given color
mode and align the pitch accordingly.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Fabio Estevam <festevam@gmail.com>
---
 drivers/gpu/drm/imx/ipuv3/imx-drm-core.c | 31 ++++++++++++++++++------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
index e7025df7b978..465b5a6ad5bb 100644
--- a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
@@ -17,7 +17,9 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_drv.h>
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_fbdev_dma.h>
+#include <drm/drm_fourcc.h>
 #include <drm/drm_gem_dma_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_managed.h>
@@ -141,19 +143,32 @@ static int imx_drm_dumb_create(struct drm_file *file_priv,
 			       struct drm_device *drm,
 			       struct drm_mode_create_dumb *args)
 {
-	u32 width = args->width;
+	u32 fourcc;
+	const struct drm_format_info *info;
+	u64 pitch_align;
 	int ret;
 
-	args->width = ALIGN(width, 8);
-	args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
-	args->size = args->pitch * args->height;
-
-	ret = drm_gem_dma_dumb_create(file_priv, drm, args);
+	/*
+	 * Hardware requires the framebuffer width to be aligned to
+	 * multiples of 8. The mode-setting code handles this, but
+	 * the buffer pitch has to be aligned as well. Set the pitch
+	 * alignment accordingly, so that the each scanline fits into
+	 * the allocated buffer.
+	 */
+	fourcc = drm_driver_color_mode_format(drm, args->bpp);
+	if (fourcc == DRM_FORMAT_INVALID)
+		return -EINVAL;
+	info = drm_format_info(fourcc);
+	if (!info)
+		return -EINVAL;
+	pitch_align = drm_format_info_min_pitch(info, 0, SZ_8);
+	if (!pitch_align || pitch_align > U32_MAX)
+		return -EINVAL;
+	ret = drm_mode_size_dumb(drm, args, pitch_align, 0);
 	if (ret)
 		return ret;
 
-	args->width = width;
-	return ret;
+	return drm_gem_dma_dumb_create(file_priv, drm, args);
 }
 
 static const struct drm_driver imx_drm_driver = {
-- 
2.47.1

Re: [PATCH v2 10/25] drm/imx/ipuv3: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Philipp Zabel]

On Do, 2025-01-09 at 15:57 +0100, Thomas Zimmermann wrote:
> Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
> buffer size. The hardware requires the framebuffer width to be a
> multiple of 8. The scanline pitch has be large enough to support
> this. Therefore compute the byte size of 8 pixels in the given color
> mode and align the pitch accordingly.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Fabio Estevam <festevam@gmail.com>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

[PATCH v2 11/25] drm/loongson: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. Align the pitch according to hardware requirements.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Sui Jingfeng <suijingfeng@loongson.cn>
Cc: Sui Jingfeng <sui.jingfeng@linux.dev>
---
 drivers/gpu/drm/loongson/lsdc_gem.c | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/loongson/lsdc_gem.c b/drivers/gpu/drm/loongson/lsdc_gem.c
index a720d8f53209..9f982b85301f 100644
--- a/drivers/gpu/drm/loongson/lsdc_gem.c
+++ b/drivers/gpu/drm/loongson/lsdc_gem.c
@@ -6,6 +6,7 @@
 #include <linux/dma-buf.h>
 
 #include <drm/drm_debugfs.h>
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_file.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_prime.h>
@@ -204,45 +205,31 @@ int lsdc_dumb_create(struct drm_file *file, struct drm_device *ddev,
 	const struct lsdc_desc *descp = ldev->descp;
 	u32 domain = LSDC_GEM_DOMAIN_VRAM;
 	struct drm_gem_object *gobj;
-	size_t size;
-	u32 pitch;
-	u32 handle;
 	int ret;
 
-	if (!args->width || !args->height)
-		return -EINVAL;
-
-	if (args->bpp != 32 && args->bpp != 16)
-		return -EINVAL;
-
-	pitch = args->width * args->bpp / 8;
-	pitch = ALIGN(pitch, descp->pitch_align);
-	size = pitch * args->height;
-	size = ALIGN(size, PAGE_SIZE);
+	ret = drm_mode_size_dumb(ddev, args, descp->pitch_align, 0);
+	if (ret)
+		return ret;
 
 	/* Maximum single bo size allowed is the half vram size available */
-	if (size > ldev->vram_size / 2) {
-		drm_err(ddev, "Requesting(%zuMiB) failed\n", size >> 20);
+	if (args->size > ldev->vram_size / 2) {
+		drm_err(ddev, "Requesting(%zuMiB) failed\n", (size_t)(args->size >> PAGE_SHIFT));
 		return -ENOMEM;
 	}
 
-	gobj = lsdc_gem_object_create(ddev, domain, size, false, NULL, NULL);
+	gobj = lsdc_gem_object_create(ddev, domain, args->size, false, NULL, NULL);
 	if (IS_ERR(gobj)) {
 		drm_err(ddev, "Failed to create gem object\n");
 		return PTR_ERR(gobj);
 	}
 
-	ret = drm_gem_handle_create(file, gobj, &handle);
+	ret = drm_gem_handle_create(file, gobj, &args->handle);
 
 	/* drop reference from allocate, handle holds it now */
 	drm_gem_object_put(gobj);
 	if (ret)
 		return ret;
 
-	args->pitch = pitch;
-	args->size = size;
-	args->handle = handle;
-
 	return 0;
 }
 
-- 
2.47.1

Re: [PATCH v2 11/25] drm/loongson: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Sui Jingfeng]

Hi,

On 2025/1/9 22:57, Thomas Zimmermann wrote:
> Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
> buffer size. Align the pitch according to hardware requirements.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Sui Jingfeng <suijingfeng@loongson.cn>
> Cc: Sui Jingfeng <sui.jingfeng@linux.dev>


Reviewed-by: Sui Jingfeng <sui.jingfeng@linux.dev>


> ---
>   drivers/gpu/drm/loongson/lsdc_gem.c | 29 ++++++++---------------------
>   1 file changed, 8 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/loongson/lsdc_gem.c b/drivers/gpu/drm/loongson/lsdc_gem.c
> index a720d8f53209..9f982b85301f 100644
> --- a/drivers/gpu/drm/loongson/lsdc_gem.c
> +++ b/drivers/gpu/drm/loongson/lsdc_gem.c
> @@ -6,6 +6,7 @@
>   #include <linux/dma-buf.h>
>   
>   #include <drm/drm_debugfs.h>
> +#include <drm/drm_dumb_buffers.h>
>   #include <drm/drm_file.h>
>   #include <drm/drm_gem.h>
>   #include <drm/drm_prime.h>
> @@ -204,45 +205,31 @@ int lsdc_dumb_create(struct drm_file *file, struct drm_device *ddev,
>   	const struct lsdc_desc *descp = ldev->descp;
>   	u32 domain = LSDC_GEM_DOMAIN_VRAM;
>   	struct drm_gem_object *gobj;
> -	size_t size;
> -	u32 pitch;
> -	u32 handle;
>   	int ret;
>   
> -	if (!args->width || !args->height)
> -		return -EINVAL;
> -
> -	if (args->bpp != 32 && args->bpp != 16)
> -		return -EINVAL;
> -
> -	pitch = args->width * args->bpp / 8;
> -	pitch = ALIGN(pitch, descp->pitch_align);
> -	size = pitch * args->height;
> -	size = ALIGN(size, PAGE_SIZE);
> +	ret = drm_mode_size_dumb(ddev, args, descp->pitch_align, 0);
> +	if (ret)
> +		return ret;
>   
>   	/* Maximum single bo size allowed is the half vram size available */
> -	if (size > ldev->vram_size / 2) {
> -		drm_err(ddev, "Requesting(%zuMiB) failed\n", size >> 20);
> +	if (args->size > ldev->vram_size / 2) {
> +		drm_err(ddev, "Requesting(%zuMiB) failed\n", (size_t)(args->size >> PAGE_SHIFT));
>   		return -ENOMEM;
>   	}
>   
> -	gobj = lsdc_gem_object_create(ddev, domain, size, false, NULL, NULL);
> +	gobj = lsdc_gem_object_create(ddev, domain, args->size, false, NULL, NULL);
>   	if (IS_ERR(gobj)) {
>   		drm_err(ddev, "Failed to create gem object\n");
>   		return PTR_ERR(gobj);
>   	}
>   
> -	ret = drm_gem_handle_create(file, gobj, &handle);
> +	ret = drm_gem_handle_create(file, gobj, &args->handle);
>   
>   	/* drop reference from allocate, handle holds it now */
>   	drm_gem_object_put(gobj);
>   	if (ret)
>   		return ret;
>   
> -	args->pitch = pitch;
> -	args->size = size;
> -	args->handle = handle;
> -
>   	return 0;
>   }
>   

-- 
Best regards,
Sui

[PATCH v2 12/25] drm/mediatek: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. Align the pitch to a multiple of 8.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Chun-Kuang Hu <chunkuang.hu@kernel.org>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_gem.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_gem.c b/drivers/gpu/drm/mediatek/mtk_gem.c
index a172456d1d7b..21e08fabfd7f 100644
--- a/drivers/gpu/drm/mediatek/mtk_gem.c
+++ b/drivers/gpu/drm/mediatek/mtk_gem.c
@@ -8,6 +8,7 @@
 
 #include <drm/drm.h>
 #include <drm/drm_device.h>
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_gem_dma_helper.h>
 #include <drm/drm_prime.h>
@@ -124,15 +125,9 @@ int mtk_gem_dumb_create(struct drm_file *file_priv, struct drm_device *dev,
 	struct mtk_gem_obj *mtk_gem;
 	int ret;
 
-	args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
-
-	/*
-	 * Multiply 2 variables of different types,
-	 * for example: args->size = args->spacing * args->height;
-	 * may cause coverity issue with unintentional overflow.
-	 */
-	args->size = args->pitch;
-	args->size *= args->height;
+	ret = drm_mode_size_dumb(dev, args, SZ_8, 0);
+	if (ret)
+		return ret;
 
 	mtk_gem = mtk_gem_create(dev, args->size, false);
 	if (IS_ERR(mtk_gem))
-- 
2.47.1

[PATCH v2 13/25] drm/msm: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. The hardware requires the scnaline pitch to be a multiple
of 32 pixels. Therefore compute the byte size of 32 pixels in the given
color mode and align the pitch accordingly.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Sean Paul <sean@poorly.run>
Cc: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/gpu/drm/msm/msm_gem.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index ebc9ba66efb8..a956905f1ef2 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -11,8 +11,10 @@
 #include <linux/dma-buf.h>
 #include <linux/pfn_t.h>
 
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_prime.h>
 #include <drm/drm_file.h>
+#include <drm/drm_fourcc.h>
 
 #include <trace/events/gpu_mem.h>
 
@@ -700,8 +702,29 @@ void msm_gem_unpin_iova(struct drm_gem_object *obj,
 int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
 		struct drm_mode_create_dumb *args)
 {
-	args->pitch = align_pitch(args->width, args->bpp);
-	args->size  = PAGE_ALIGN(args->pitch * args->height);
+	u32 fourcc;
+	const struct drm_format_info *info;
+	u64 pitch_align;
+	int ret;
+
+	/*
+	 * Adreno needs pitch aligned to 32 pixels. Compute the number
+	 * of bytes for a block of 32 pixels at the given color format.
+	 * Use the result as pitch alignment.
+	 */
+	fourcc = drm_driver_color_mode_format(dev, args->bpp);
+	if (fourcc == DRM_FORMAT_INVALID)
+		return -EINVAL;
+	info = drm_format_info(fourcc);
+	if (!info)
+		return -EINVAL;
+	pitch_align = drm_format_info_min_pitch(info, 0, SZ_32);
+	if (!pitch_align || pitch_align > U32_MAX)
+		return -EINVAL;
+	ret = drm_mode_size_dumb(dev, args, pitch_align, 0);
+	if (ret)
+		return ret;
+
 	return msm_gem_new_handle(dev, file, args->size,
 			MSM_BO_SCANOUT | MSM_BO_WC, &args->handle, "dumb");
 }
-- 
2.47.1

Re: [PATCH v2 13/25] drm/msm: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Dmitry Baryshkov]

On Thu, Jan 09, 2025 at 03:57:07PM +0100, Thomas Zimmermann wrote:
> Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
> buffer size. The hardware requires the scnaline pitch to be a multiple
> of 32 pixels. Therefore compute the byte size of 32 pixels in the given
> color mode and align the pitch accordingly.

- scanline, not scnaline
- the statement about 32-pixel alignment needs an explanation that it is
  being currently handled by align_pitch().

With that in mind:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>  drivers/gpu/drm/msm/msm_gem.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 

-- 
With best wishes
Dmitry

[PATCH v2 14/25] drm/nouveau: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. Align the pitch to a multiple of 256.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Karol Herbst <kherbst@redhat.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Danilo Krummrich <dakr@kernel.org>
---
 drivers/gpu/drm/nouveau/nouveau_display.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index add006fc8d81..daa2528f9c9a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -30,6 +30,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_client_event.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_probe_helper.h>
@@ -808,9 +809,9 @@ nouveau_display_dumb_create(struct drm_file *file_priv, struct drm_device *dev,
 	uint32_t domain;
 	int ret;
 
-	args->pitch = roundup(args->width * (args->bpp / 8), 256);
-	args->size = args->pitch * args->height;
-	args->size = roundup(args->size, PAGE_SIZE);
+	ret = drm_mode_size_dumb(dev, args, SZ_256, 0);
+	if (ret)
+		return ret;
 
 	/* Use VRAM if there is any ; otherwise fallback to system memory */
 	if (nouveau_drm(dev)->client.device.info.ram_size != 0)
-- 
2.47.1

[PATCH v2 15/25] drm/omapdrm: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. Align the pitch to a multiple of 8.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_gem.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index b9c67e4ca360..b8413a2dcdeb 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -11,6 +11,7 @@
 #include <linux/pfn_t.h>
 #include <linux/vmalloc.h>
 
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_prime.h>
 #include <drm/drm_vma_manager.h>
 
@@ -583,15 +584,13 @@ static int omap_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_struc
 int omap_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
 		struct drm_mode_create_dumb *args)
 {
-	union omap_gem_size gsize;
-
-	args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
-
-	args->size = PAGE_ALIGN(args->pitch * args->height);
+	union omap_gem_size gsize = { };
+	int ret;
 
-	gsize = (union omap_gem_size){
-		.bytes = args->size,
-	};
+	ret = drm_mode_size_dumb(dev, args, SZ_8, 0);
+	if (ret)
+		return ret;
+	gsize.bytes = args->size;
 
 	return omap_gem_new_handle(dev, file, gsize,
 			OMAP_BO_SCANOUT | OMAP_BO_WC, &args->handle);
-- 
2.47.1

Re: [PATCH v2 15/25] drm/omapdrm: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Tomi Valkeinen]

Hi,

On 09/01/2025 16:57, Thomas Zimmermann wrote:
> Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
> buffer size. Align the pitch to a multiple of 8.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>   drivers/gpu/drm/omapdrm/omap_gem.c | 15 +++++++--------
>   1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
> index b9c67e4ca360..b8413a2dcdeb 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -11,6 +11,7 @@
>   #include <linux/pfn_t.h>
>   #include <linux/vmalloc.h>
>   
> +#include <drm/drm_dumb_buffers.h>
>   #include <drm/drm_prime.h>
>   #include <drm/drm_vma_manager.h>
>   
> @@ -583,15 +584,13 @@ static int omap_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_struc
>   int omap_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
>   		struct drm_mode_create_dumb *args)
>   {
> -	union omap_gem_size gsize;
> -
> -	args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> -
> -	args->size = PAGE_ALIGN(args->pitch * args->height);
> +	union omap_gem_size gsize = { };
> +	int ret;
>   
> -	gsize = (union omap_gem_size){
> -		.bytes = args->size,
> -	};
> +	ret = drm_mode_size_dumb(dev, args, SZ_8, 0);
> +	if (ret)
> +		return ret;
> +	gsize.bytes = args->size;
>   
>   	return omap_gem_new_handle(dev, file, gsize,
>   			OMAP_BO_SCANOUT | OMAP_BO_WC, &args->handle);

Tested on dra76 evm.

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

[PATCH v2 16/25] drm/qxl: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch
and buffer size. No alignment required.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_dumb.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_dumb.c b/drivers/gpu/drm/qxl/qxl_dumb.c
index 17df5c7ccf69..1200946767ce 100644
--- a/drivers/gpu/drm/qxl/qxl_dumb.c
+++ b/drivers/gpu/drm/qxl/qxl_dumb.c
@@ -23,6 +23,8 @@
  *          Alon Levy
  */
 
+#include <drm/drm_dumb_buffers.h>
+
 #include "qxl_drv.h"
 #include "qxl_object.h"
 
@@ -35,14 +37,13 @@ int qxl_mode_dumb_create(struct drm_file *file_priv,
 	struct qxl_device *qdev = to_qxl(dev);
 	struct qxl_bo *qobj;
 	struct drm_gem_object *gobj;
-	uint32_t handle;
 	int r;
 	struct qxl_surface surf;
-	uint32_t pitch, format;
+	u32 format;
 
-	pitch = args->width * ((args->bpp + 1) / 8);
-	args->size = pitch * args->height;
-	args->size = ALIGN(args->size, PAGE_SIZE);
+	r = drm_mode_size_dumb(dev, args, 0, 0);
+	if (r)
+		return r;
 
 	switch (args->bpp) {
 	case 16:
@@ -57,20 +58,18 @@ int qxl_mode_dumb_create(struct drm_file *file_priv,
 
 	surf.width = args->width;
 	surf.height = args->height;
-	surf.stride = pitch;
+	surf.stride = args->pitch;
 	surf.format = format;
 	surf.data = 0;
 
 	r = qxl_gem_object_create_with_handle(qdev, file_priv,
 					      QXL_GEM_DOMAIN_CPU,
 					      args->size, &surf, &gobj,
-					      &handle);
+					      &args->handle);
 	if (r)
 		return r;
 	qobj = gem_to_qxl_bo(gobj);
 	qobj->is_dumb = true;
 	drm_gem_object_put(gobj);
-	args->pitch = pitch;
-	args->handle = handle;
 	return 0;
 }
-- 
2.47.1

[PATCH v2 17/25] drm/renesas/rcar-du: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. Align the pitch according to hardware requirements.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
index 70d8ad065bfa..32c8307da522 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
@@ -11,6 +11,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_device.h>
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_dma_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
@@ -407,8 +408,8 @@ int rcar_du_dumb_create(struct drm_file *file, struct drm_device *dev,
 			struct drm_mode_create_dumb *args)
 {
 	struct rcar_du_device *rcdu = to_rcar_du_device(dev);
-	unsigned int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
 	unsigned int align;
+	int ret;
 
 	/*
 	 * The R8A7779 DU requires a 16 pixels pitch alignment as documented,
@@ -419,7 +420,9 @@ int rcar_du_dumb_create(struct drm_file *file, struct drm_device *dev,
 	else
 		align = 16 * args->bpp / 8;
 
-	args->pitch = roundup(min_pitch, align);
+	ret = drm_mode_size_dumb(dev, args, align, 0);
+	if (ret)
+		return ret;
 
 	return drm_gem_dma_dumb_create_internal(file, dev, args);
 }
-- 
2.47.1

[PATCH v2 18/25] drm/renesas/rz-du: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. Align the pitch according to hardware requirements.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
index 90c6269ccd29..f752369cd52f 100644
--- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
+++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
@@ -75,10 +75,11 @@ const struct rzg2l_du_format_info *rzg2l_du_format_info(u32 fourcc)
 int rzg2l_du_dumb_create(struct drm_file *file, struct drm_device *dev,
 			 struct drm_mode_create_dumb *args)
 {
-	unsigned int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
-	unsigned int align = 16 * args->bpp / 8;
+	int ret;
 
-	args->pitch = roundup(min_pitch, align);
+	ret = drm_mode_size_dumb(dev, args, 16 * args->bpp / 8, 0);
+	if (ret)
+		return ret;
 
 	return drm_gem_dma_dumb_create_internal(file, dev, args);
 }
-- 
2.47.1

[PATCH v2 19/25] drm/rockchip: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. Align the pitch to a multiple of 64.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Sandy Huang <hjc@rock-chips.com>
Cc: "Heiko Stübner" <heiko@sntech.de>
Cc: Andy Yan <andy.yan@rock-chips.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
index 6330b883efc3..3bd06202e232 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
@@ -9,6 +9,7 @@
 #include <linux/vmalloc.h>
 
 #include <drm/drm.h>
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_gem_dma_helper.h>
@@ -403,13 +404,12 @@ int rockchip_gem_dumb_create(struct drm_file *file_priv,
 			     struct drm_mode_create_dumb *args)
 {
 	struct rockchip_gem_object *rk_obj;
-	int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
+	int ret;
 
-	/*
-	 * align to 64 bytes since Mali requires it.
-	 */
-	args->pitch = ALIGN(min_pitch, 64);
-	args->size = args->pitch * args->height;
+	/* 64-byte alignment required by Mali */
+	ret = drm_mode_size_dumb(dev, args, SZ_64, 0);
+	if (ret)
+		return ret;
 
 	rk_obj = rockchip_gem_create_with_handle(file_priv, dev, args->size,
 						 &args->handle);
-- 
2.47.1

Re: [PATCH v2 19/25] drm/rockchip: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Heiko Stübner]

Am Donnerstag, 9. Januar 2025, 15:57:13 CET schrieb Thomas Zimmermann:
> Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
> buffer size. Align the pitch to a multiple of 64.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Sandy Huang <hjc@rock-chips.com>
> Cc: "Heiko Stübner" <heiko@sntech.de>
> Cc: Andy Yan <andy.yan@rock-chips.com>

I've looked up the patch implementing the new functionality - patch2 of
this series [0] and that looks really nice to get proper helpers and not
having many drivers open-coding the same functionality in different ways.

So for the Rockchip adaptation:

Acked-by: Heiko Stuebner <heiko@sntech.de>

and looking forward to this getting merged :-)

Thanks a lot for working on that
Heiko

[0] https://patchwork.kernel.org/project/linux-rockchip/patch/20250109150310.219442-3-tzimmermann@suse.de/

> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> index 6330b883efc3..3bd06202e232 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> @@ -9,6 +9,7 @@
>  #include <linux/vmalloc.h>
>  
>  #include <drm/drm.h>
> +#include <drm/drm_dumb_buffers.h>
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drm_gem.h>
>  #include <drm/drm_gem_dma_helper.h>
> @@ -403,13 +404,12 @@ int rockchip_gem_dumb_create(struct drm_file *file_priv,
>  			     struct drm_mode_create_dumb *args)
>  {
>  	struct rockchip_gem_object *rk_obj;
> -	int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> +	int ret;
>  
> -	/*
> -	 * align to 64 bytes since Mali requires it.
> -	 */
> -	args->pitch = ALIGN(min_pitch, 64);
> -	args->size = args->pitch * args->height;
> +	/* 64-byte alignment required by Mali */
> +	ret = drm_mode_size_dumb(dev, args, SZ_64, 0);
> +	if (ret)
> +		return ret;
>  
>  	rk_obj = rockchip_gem_create_with_handle(file_priv, dev, args->size,
>  						 &args->handle);
> 

[PATCH v2 20/25] drm/tegra: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. Align the pitch according to hardware requirements.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/gpu/drm/tegra/gem.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
index ace3e5a805cf..4d88e71192fb 100644
--- a/drivers/gpu/drm/tegra/gem.c
+++ b/drivers/gpu/drm/tegra/gem.c
@@ -16,6 +16,7 @@
 #include <linux/vmalloc.h>
 
 #include <drm/drm_drv.h>
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_prime.h>
 #include <drm/tegra_drm.h>
 
@@ -543,12 +544,13 @@ void tegra_bo_free_object(struct drm_gem_object *gem)
 int tegra_bo_dumb_create(struct drm_file *file, struct drm_device *drm,
 			 struct drm_mode_create_dumb *args)
 {
-	unsigned int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
 	struct tegra_drm *tegra = drm->dev_private;
 	struct tegra_bo *bo;
+	int ret;
 
-	args->pitch = round_up(min_pitch, tegra->pitch_align);
-	args->size = args->pitch * args->height;
+	ret = drm_mode_size_dumb(drm, args, tegra->pitch_align, 0);
+	if (ret)
+		return ret;
 
 	bo = tegra_bo_create_with_handle(file, drm, args->size, 0,
 					 &args->handle);
-- 
2.47.1

[PATCH v2 21/25] drm/virtio: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. Align the pitch to a multiple of 4.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Gurchetan Singh <gurchetansingh@chromium.org>
Cc: Chia-I Wu <olvaffe@gmail.com>
---
 drivers/gpu/drm/virtio/virtgpu_gem.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index 5aab588fc400..22cf1cd2fdfd 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -23,6 +23,7 @@
  * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
  */
 
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_file.h>
 #include <drm/drm_fourcc.h>
 
@@ -66,15 +67,14 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
 	struct virtio_gpu_object_params params = { 0 };
 	struct virtio_gpu_device *vgdev = dev->dev_private;
 	int ret;
-	uint32_t pitch;
+
+	ret = drm_mode_size_dumb(dev, args, SZ_4, 0);
+	if (ret)
+		return ret;
 
 	if (args->bpp != 32)
 		return -EINVAL;
 
-	pitch = args->width * 4;
-	args->size = pitch * args->height;
-	args->size = ALIGN(args->size, PAGE_SIZE);
-
 	params.format = virtio_gpu_translate_format(DRM_FORMAT_HOST_XRGB8888);
 	params.width = args->width;
 	params.height = args->height;
@@ -92,7 +92,6 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
 	if (ret)
 		goto fail;
 
-	args->pitch = pitch;
 	return ret;
 
 fail:
-- 
2.47.1

Re: [PATCH v2 21/25] drm/virtio: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Dmitry Osipenko]

On 1/9/25 17:57, Thomas Zimmermann wrote:
> Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
> buffer size. Align the pitch to a multiple of 4.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Gurchetan Singh <gurchetansingh@chromium.org>
> Cc: Chia-I Wu <olvaffe@gmail.com>
> ---
>  drivers/gpu/drm/virtio/virtgpu_gem.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
> index 5aab588fc400..22cf1cd2fdfd 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_gem.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
> @@ -23,6 +23,7 @@
>   * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>   */
>  
> +#include <drm/drm_dumb_buffers.h>
>  #include <drm/drm_file.h>
>  #include <drm/drm_fourcc.h>
>  
> @@ -66,15 +67,14 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
>  	struct virtio_gpu_object_params params = { 0 };
>  	struct virtio_gpu_device *vgdev = dev->dev_private;
>  	int ret;
> -	uint32_t pitch;
> +
> +	ret = drm_mode_size_dumb(dev, args, SZ_4, 0);

Nit: I'd keep using PAGE_SIZE instead of 0 for more clarity, but that's
an optional wish.

> +	if (ret)
> +		return ret;
>  
>  	if (args->bpp != 32)
>  		return -EINVAL;
>  
> -	pitch = args->width * 4;
> -	args->size = pitch * args->height;
> -	args->size = ALIGN(args->size, PAGE_SIZE);
> -
>  	params.format = virtio_gpu_translate_format(DRM_FORMAT_HOST_XRGB8888);
>  	params.width = args->width;
>  	params.height = args->height;
> @@ -92,7 +92,6 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
>  	if (ret)
>  		goto fail;
>  
> -	args->pitch = pitch;
>  	return ret;
>  
>  fail:

Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

-- 
Best regards,
Dmitry

[PATCH v2 22/25] drm/vmwgfx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch
and buffer size. No alignment required.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Zack Rusin <zack.rusin@broadcom.com>
Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index 5721c74da3e0..a3fbd4148f73 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -34,6 +34,7 @@
 #include "vmw_surface_cache.h"
 #include "device_include/svga3d_surfacedefs.h"
 
+#include <drm/drm_dumb_buffers.h>
 #include <drm/ttm/ttm_placement.h>
 
 #define SVGA3D_FLAGS_64(upper32, lower32) (((uint64_t)upper32 << 32) | lower32)
@@ -2291,23 +2292,9 @@ int vmw_dumb_create(struct drm_file *file_priv,
 	 * contents is going to be rendered guest side.
 	 */
 	if (!dev_priv->has_mob || !vmw_supports_3d(dev_priv)) {
-		int cpp = DIV_ROUND_UP(args->bpp, 8);
-
-		switch (cpp) {
-		case 1: /* DRM_FORMAT_C8 */
-		case 2: /* DRM_FORMAT_RGB565 */
-		case 4: /* DRM_FORMAT_XRGB8888 */
-			break;
-		default:
-			/*
-			 * Dumb buffers don't allow anything else.
-			 * This is tested via IGT's dumb_buffers
-			 */
-			return -EINVAL;
-		}
-
-		args->pitch = args->width * cpp;
-		args->size = ALIGN(args->pitch * args->height, PAGE_SIZE);
+		ret = drm_mode_size_dumb(dev, args, 0, 0);
+		if (ret)
+			return ret;
 
 		ret = vmw_gem_object_create_with_handle(dev_priv, file_priv,
 							args->size, &args->handle,
-- 
2.47.1

Re: [PATCH v2 22/25] drm/vmwgfx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Zack Rusin]

[-- Attachment #1: Type: text/plain, Size: 2357 bytes --]

On Thu, Jan 9, 2025 at 10:03 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch
> and buffer size. No alignment required.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Zack Rusin <zack.rusin@broadcom.com>
> Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 21 ++++-----------------
>  1 file changed, 4 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> index 5721c74da3e0..a3fbd4148f73 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> @@ -34,6 +34,7 @@
>  #include "vmw_surface_cache.h"
>  #include "device_include/svga3d_surfacedefs.h"
>
> +#include <drm/drm_dumb_buffers.h>
>  #include <drm/ttm/ttm_placement.h>
>
>  #define SVGA3D_FLAGS_64(upper32, lower32) (((uint64_t)upper32 << 32) | lower32)
> @@ -2291,23 +2292,9 @@ int vmw_dumb_create(struct drm_file *file_priv,
>          * contents is going to be rendered guest side.
>          */
>         if (!dev_priv->has_mob || !vmw_supports_3d(dev_priv)) {
> -               int cpp = DIV_ROUND_UP(args->bpp, 8);
> -
> -               switch (cpp) {
> -               case 1: /* DRM_FORMAT_C8 */
> -               case 2: /* DRM_FORMAT_RGB565 */
> -               case 4: /* DRM_FORMAT_XRGB8888 */
> -                       break;
> -               default:
> -                       /*
> -                        * Dumb buffers don't allow anything else.
> -                        * This is tested via IGT's dumb_buffers
> -                        */
> -                       return -EINVAL;
> -               }
> -
> -               args->pitch = args->width * cpp;
> -               args->size = ALIGN(args->pitch * args->height, PAGE_SIZE);
> +               ret = drm_mode_size_dumb(dev, args, 0, 0);
> +               if (ret)
> +                       return ret;
>
>                 ret = vmw_gem_object_create_with_handle(dev_priv, file_priv,
>                                                         args->size, &args->handle,
> --
> 2.47.1
>

Ah, that's great. Thanks!

Reviewed-by: Zack Rusin <zack.rusin@broadcom.com>

z

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5427 bytes --]

[PATCH v2 23/25] drm/xe: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch
and buffer size. Align the pitch to a multiple of 8. Align the
buffer size according to hardware requirements.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/xe/xe_bo.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
index e6c896ad5602..d75e3c39ab14 100644
--- a/drivers/gpu/drm/xe/xe_bo.c
+++ b/drivers/gpu/drm/xe/xe_bo.c
@@ -8,6 +8,7 @@
 #include <linux/dma-buf.h>
 
 #include <drm/drm_drv.h>
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_gem_ttm_helper.h>
 #include <drm/drm_managed.h>
 #include <drm/ttm/ttm_device.h>
@@ -2535,14 +2536,13 @@ int xe_bo_dumb_create(struct drm_file *file_priv,
 	struct xe_device *xe = to_xe_device(dev);
 	struct xe_bo *bo;
 	uint32_t handle;
-	int cpp = DIV_ROUND_UP(args->bpp, 8);
 	int err;
 	u32 page_size = max_t(u32, PAGE_SIZE,
 		xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K ? SZ_64K : SZ_4K);
 
-	args->pitch = ALIGN(args->width * cpp, 64);
-	args->size = ALIGN(mul_u32_u32(args->pitch, args->height),
-			   page_size);
+	err = drm_mode_size_dumb(dev, args, SZ_64, page_size);
+	if (err)
+		return err;
 
 	bo = xe_bo_create_user(xe, NULL, NULL, args->size,
 			       DRM_XE_GEM_CPU_CACHING_WC,
-- 
2.47.1

Re: [PATCH v2 23/25] drm/xe: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Matthew Auld]

On 09/01/2025 14:57, Thomas Zimmermann wrote:
> Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch
> and buffer size. Align the pitch to a multiple of 8. Align the
> buffer size according to hardware requirements.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_bo.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index e6c896ad5602..d75e3c39ab14 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -8,6 +8,7 @@
>   #include <linux/dma-buf.h>
>   
>   #include <drm/drm_drv.h>
> +#include <drm/drm_dumb_buffers.h>
>   #include <drm/drm_gem_ttm_helper.h>
>   #include <drm/drm_managed.h>
>   #include <drm/ttm/ttm_device.h>
> @@ -2535,14 +2536,13 @@ int xe_bo_dumb_create(struct drm_file *file_priv,
>   	struct xe_device *xe = to_xe_device(dev);
>   	struct xe_bo *bo;
>   	uint32_t handle;
> -	int cpp = DIV_ROUND_UP(args->bpp, 8);
>   	int err;
>   	u32 page_size = max_t(u32, PAGE_SIZE,
>   		xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K ? SZ_64K : SZ_4K);
>   
> -	args->pitch = ALIGN(args->width * cpp, 64);
> -	args->size = ALIGN(mul_u32_u32(args->pitch, args->height),
> -			   page_size);
> +	err = drm_mode_size_dumb(dev, args, SZ_64, page_size);

AFAICT this looks to change the behaviour, where u64 size was 
technically possible and was allowed given that args->size is u64, but 
this helper is limiting the size to u32. Is that intentional? If so, we 
should probably make that clear in the commit message.

> +	if (err)
> +		return err;
>   
>   	bo = xe_bo_create_user(xe, NULL, NULL, args->size,
>   			       DRM_XE_GEM_CPU_CACHING_WC,

Re: [PATCH v2 23/25] drm/xe: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Hi


Am 09.01.25 um 17:05 schrieb Matthew Auld:
> On 09/01/2025 14:57, Thomas Zimmermann wrote:
>> Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch
>> and buffer size. Align the pitch to a multiple of 8. Align the
>> buffer size according to hardware requirements.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_bo.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
>> index e6c896ad5602..d75e3c39ab14 100644
>> --- a/drivers/gpu/drm/xe/xe_bo.c
>> +++ b/drivers/gpu/drm/xe/xe_bo.c
>> @@ -8,6 +8,7 @@
>>   #include <linux/dma-buf.h>
>>     #include <drm/drm_drv.h>
>> +#include <drm/drm_dumb_buffers.h>
>>   #include <drm/drm_gem_ttm_helper.h>
>>   #include <drm/drm_managed.h>
>>   #include <drm/ttm/ttm_device.h>
>> @@ -2535,14 +2536,13 @@ int xe_bo_dumb_create(struct drm_file 
>> *file_priv,
>>       struct xe_device *xe = to_xe_device(dev);
>>       struct xe_bo *bo;
>>       uint32_t handle;
>> -    int cpp = DIV_ROUND_UP(args->bpp, 8);
>>       int err;
>>       u32 page_size = max_t(u32, PAGE_SIZE,
>>           xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K ? SZ_64K : SZ_4K);
>>   -    args->pitch = ALIGN(args->width * cpp, 64);
>> -    args->size = ALIGN(mul_u32_u32(args->pitch, args->height),
>> -               page_size);
>> +    err = drm_mode_size_dumb(dev, args, SZ_64, page_size);
>
> AFAICT this looks to change the behaviour, where u64 size was 
> technically possible and was allowed given that args->size is u64, but 
> this helper is limiting the size to u32. Is that intentional? If so, 
> we should probably make that clear in the commit message.

That's an interesting observation; thanks. The ioctl's internal checks 
have always limited the size to 32 bit. [1] I think it is not supposed 
to be larger than that. We can change the helper to support 64-bit sizes 
as well.

Having said that, is there any use case? Dumb buffers are for software 
rendering only. Allocating more than a few dozen MiB seems like a 
mistake. Maybe we should rather limit the allowed allocation size instead?

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/drm_dumb_buffers.c#L82

>
>> +    if (err)
>> +        return err;
>>         bo = xe_bo_create_user(xe, NULL, NULL, args->size,
>>                      DRM_XE_GEM_CPU_CACHING_WC,
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v2 23/25] drm/xe: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Matthew Auld]

On 09/01/2025 16:26, Thomas Zimmermann wrote:
> Hi
> 
> 
> Am 09.01.25 um 17:05 schrieb Matthew Auld:
>> On 09/01/2025 14:57, Thomas Zimmermann wrote:
>>> Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch
>>> and buffer size. Align the pitch to a multiple of 8. Align the
>>> buffer size according to hardware requirements.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_bo.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
>>> index e6c896ad5602..d75e3c39ab14 100644
>>> --- a/drivers/gpu/drm/xe/xe_bo.c
>>> +++ b/drivers/gpu/drm/xe/xe_bo.c
>>> @@ -8,6 +8,7 @@
>>>   #include <linux/dma-buf.h>
>>>     #include <drm/drm_drv.h>
>>> +#include <drm/drm_dumb_buffers.h>
>>>   #include <drm/drm_gem_ttm_helper.h>
>>>   #include <drm/drm_managed.h>
>>>   #include <drm/ttm/ttm_device.h>
>>> @@ -2535,14 +2536,13 @@ int xe_bo_dumb_create(struct drm_file 
>>> *file_priv,
>>>       struct xe_device *xe = to_xe_device(dev);
>>>       struct xe_bo *bo;
>>>       uint32_t handle;
>>> -    int cpp = DIV_ROUND_UP(args->bpp, 8);
>>>       int err;
>>>       u32 page_size = max_t(u32, PAGE_SIZE,
>>>           xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K ? SZ_64K : SZ_4K);
>>>   -    args->pitch = ALIGN(args->width * cpp, 64);
>>> -    args->size = ALIGN(mul_u32_u32(args->pitch, args->height),
>>> -               page_size);
>>> +    err = drm_mode_size_dumb(dev, args, SZ_64, page_size);
>>
>> AFAICT this looks to change the behaviour, where u64 size was 
>> technically possible and was allowed given that args->size is u64, but 
>> this helper is limiting the size to u32. Is that intentional? If so, 
>> we should probably make that clear in the commit message.
> 
> That's an interesting observation; thanks. The ioctl's internal checks 
> have always limited the size to 32 bit. [1] I think it is not supposed 
> to be larger than that. We can change the helper to support 64-bit sizes 
> as well.

Ah, I missed the internal check.

> 
> Having said that, is there any use case? Dumb buffers are for software 
> rendering only. Allocating more than a few dozen MiB seems like a 
> mistake. Maybe we should rather limit the allowed allocation size instead?

Yeah, I doubt there are any real users. Given the existing internal 
check, limiting to u32 makes sense to me.

> 
> Best regards
> Thomas
> 
> [1] https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/ 
> drm_dumb_buffers.c#L82
> 
>>
>>> +    if (err)
>>> +        return err;
>>>         bo = xe_bo_create_user(xe, NULL, NULL, args->size,
>>>                      DRM_XE_GEM_CPU_CACHING_WC,
>>
> 

[PATCH v2 24/25] drm/xen: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch
and buffer size. Align the pitch to a multiple of 8.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
 drivers/gpu/drm/xen/xen_drm_front.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front.c b/drivers/gpu/drm/xen/xen_drm_front.c
index 1bda7ef606cc..fd2f250fbc33 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.c
+++ b/drivers/gpu/drm/xen/xen_drm_front.c
@@ -14,6 +14,7 @@
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_drv.h>
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_ioctl.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_file.h>
@@ -414,8 +415,10 @@ static int xen_drm_drv_dumb_create(struct drm_file *filp,
 	 * object without pages etc.
 	 * For details also see drm_gem_handle_create
 	 */
-	args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
-	args->size = args->pitch * args->height;
+
+	ret = drm_mode_size_dumb(dev, args, SZ_8, 0);
+	if (ret)
+		return ret;
 
 	obj = xen_drm_front_gem_create(dev, args->size);
 	if (IS_ERR(obj)) {
-- 
2.47.1

[PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
buffer size. Align the pitch according to hardware requirements.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/xlnx/zynqmp_kms.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c b/drivers/gpu/drm/xlnx/zynqmp_kms.c
index b47463473472..7ea0cd4f71d3 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
@@ -19,6 +19,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_device.h>
 #include <drm/drm_drv.h>
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_encoder.h>
 #include <drm/drm_fbdev_dma.h>
 #include <drm/drm_fourcc.h>
@@ -363,10 +364,12 @@ static int zynqmp_dpsub_dumb_create(struct drm_file *file_priv,
 				    struct drm_mode_create_dumb *args)
 {
 	struct zynqmp_dpsub *dpsub = to_zynqmp_dpsub(drm);
-	unsigned int pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
+	int ret;
 
 	/* Enforce the alignment constraints of the DMA engine. */
-	args->pitch = ALIGN(pitch, dpsub->dma_align);
+	ret = drm_mode_size_dumb(drm, args, dpsub->dma_align, 0);
+	if (ret)
+		return ret;
 
 	return drm_gem_dma_dumb_create_internal(file_priv, drm, args);
 }
-- 
2.47.1

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Tomi Valkeinen]

Hi!

On 09/01/2025 16:57, Thomas Zimmermann wrote:
> Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
> buffer size. Align the pitch according to hardware requirements.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>   drivers/gpu/drm/xlnx/zynqmp_kms.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> index b47463473472..7ea0cd4f71d3 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> @@ -19,6 +19,7 @@
>   #include <drm/drm_crtc.h>
>   #include <drm/drm_device.h>
>   #include <drm/drm_drv.h>
> +#include <drm/drm_dumb_buffers.h>
>   #include <drm/drm_encoder.h>
>   #include <drm/drm_fbdev_dma.h>
>   #include <drm/drm_fourcc.h>
> @@ -363,10 +364,12 @@ static int zynqmp_dpsub_dumb_create(struct drm_file *file_priv,
>   				    struct drm_mode_create_dumb *args)
>   {
>   	struct zynqmp_dpsub *dpsub = to_zynqmp_dpsub(drm);
> -	unsigned int pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> +	int ret;
>   
>   	/* Enforce the alignment constraints of the DMA engine. */
> -	args->pitch = ALIGN(pitch, dpsub->dma_align);
> +	ret = drm_mode_size_dumb(drm, args, dpsub->dma_align, 0);
> +	if (ret)
> +		return ret;
>   
>   	return drm_gem_dma_dumb_create_internal(file_priv, drm, args);
>   }

I have some trouble with this one.

I have sent a series to add some pixel formats:

https://lore.kernel.org/all/20250115-xilinx-formats-v2-0-160327ca652a@ideasonboard.com/

Let's look at XV15. It's similar to NV12, but 10 bits per component, and 
some packing and padding.

First plane: 3 pixels in a 32 bit group
Second plane: 3 pixels in a 64 bit group, 2x2 subsampled

So, on average, a pixel on the first plane takes 32 / 3 = 10.666... bits 
on a line. That's not a usable number for the DRM_IOCTL_MODE_CREATE_DUMB 
ioctl.

What I did was to use the pixel group size as "bpp" for 
DRM_IOCTL_MODE_CREATE_DUMB. So, e.g., for 720 x 576:

Stride for first plane: 720 * (32 / 3) / 8 = 960 bytes
Stride for second plane: 720 / 2 * (64 / 3) / 8 = 960 bytes

First plane: 720 / 3 = 240 pixel groups
Second plane: 720 / 2 / 3 = 120 pixel groups

So I allocated the two planes with:
240 x 576 with 32 bitspp
120 x 288 with 64 bitspp

This worked, and if I look at the DRM_IOCTL_MODE_CREATE_DUMB in the 
docs, I can't right away see anything there that says my tactic was not 
allowed.

The above doesn't work anymore with this patch, as the code calls 
drm_driver_color_mode_format(), which fails for 64 bitspp. It feels a 
bit odd that DRM_IOCTL_MODE_CREATE_DUMB will try to guess the RGB fourcc 
for a dumb buffer allocation.

So, what to do here? Am I doing something silly? What's the correct way 
to allocate the buffers for XV15? Should I just use 32 bitspp for the 
plane 2 too, and double the width (this works)?

Is DRM_IOCTL_MODE_CREATE_DUMB only meant for simple RGB formats? The 
xilinx driver can, of course, just not use drm_mode_size_dumb(). But if 
so, I guess the limitations of drm_mode_size_dumb() should be documented.

Do we need a new dumb-alloc ioctl that takes the format and plane number 
as parameters? Or alternatively a simpler dumb-alloc that doesn't have 
width and bpp, but instead takes a stride and height as parameters? I 
think those would be easier for the userspace to use, instead of trying 
to adjust the parameters to be suitable for the kernel.

  Tomi

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Hi


Am 15.01.25 um 11:13 schrieb Tomi Valkeinen:
> Hi!
>
> On 09/01/2025 16:57, Thomas Zimmermann wrote:
>> Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
>> buffer size. Align the pitch according to hardware requirements.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/xlnx/zynqmp_kms.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c 
>> b/drivers/gpu/drm/xlnx/zynqmp_kms.c
>> index b47463473472..7ea0cd4f71d3 100644
>> --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
>> +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
>> @@ -19,6 +19,7 @@
>>   #include <drm/drm_crtc.h>
>>   #include <drm/drm_device.h>
>>   #include <drm/drm_drv.h>
>> +#include <drm/drm_dumb_buffers.h>
>>   #include <drm/drm_encoder.h>
>>   #include <drm/drm_fbdev_dma.h>
>>   #include <drm/drm_fourcc.h>
>> @@ -363,10 +364,12 @@ static int zynqmp_dpsub_dumb_create(struct 
>> drm_file *file_priv,
>>                       struct drm_mode_create_dumb *args)
>>   {
>>       struct zynqmp_dpsub *dpsub = to_zynqmp_dpsub(drm);
>> -    unsigned int pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
>> +    int ret;
>>         /* Enforce the alignment constraints of the DMA engine. */
>> -    args->pitch = ALIGN(pitch, dpsub->dma_align);
>> +    ret = drm_mode_size_dumb(drm, args, dpsub->dma_align, 0);
>> +    if (ret)
>> +        return ret;
>>         return drm_gem_dma_dumb_create_internal(file_priv, drm, args);
>>   }
>
> I have some trouble with this one.
>
> I have sent a series to add some pixel formats:
>
> https://lore.kernel.org/all/20250115-xilinx-formats-v2-0-160327ca652a@ideasonboard.com/ 
>
>
> Let's look at XV15. It's similar to NV12, but 10 bits per component, 
> and some packing and padding.
>
> First plane: 3 pixels in a 32 bit group
> Second plane: 3 pixels in a 64 bit group, 2x2 subsampled
>
> So, on average, a pixel on the first plane takes 32 / 3 = 10.666... 
> bits on a line. That's not a usable number for the 
> DRM_IOCTL_MODE_CREATE_DUMB ioctl.
>
> What I did was to use the pixel group size as "bpp" for 
> DRM_IOCTL_MODE_CREATE_DUMB. So, e.g., for 720 x 576:
>
> Stride for first plane: 720 * (32 / 3) / 8 = 960 bytes
> Stride for second plane: 720 / 2 * (64 / 3) / 8 = 960 bytes
>
> First plane: 720 / 3 = 240 pixel groups
> Second plane: 720 / 2 / 3 = 120 pixel groups
>
> So I allocated the two planes with:
> 240 x 576 with 32 bitspp
> 120 x 288 with 64 bitspp
>
> This worked, and if I look at the DRM_IOCTL_MODE_CREATE_DUMB in the 
> docs, I can't right away see anything there that says my tactic was 
> not allowed.
>
> The above doesn't work anymore with this patch, as the code calls 
> drm_driver_color_mode_format(), which fails for 64 bitspp. It feels a 
> bit odd that DRM_IOCTL_MODE_CREATE_DUMB will try to guess the RGB 
> fourcc for a dumb buffer allocation.
>
> So, what to do here? Am I doing something silly? What's the correct 
> way to allocate the buffers for XV15? Should I just use 32 bitspp for 
> the plane 2 too, and double the width (this works)?
>
> Is DRM_IOCTL_MODE_CREATE_DUMB only meant for simple RGB formats? The 
> xilinx driver can, of course, just not use drm_mode_size_dumb(). But 
> if so, I guess the limitations of drm_mode_size_dumb() should be 
> documented.
>
> Do we need a new dumb-alloc ioctl that takes the format and plane 
> number as parameters? Or alternatively a simpler dumb-alloc that 
> doesn't have width and bpp, but instead takes a stride and height as 
> parameters? I think those would be easier for the userspace to use, 
> instead of trying to adjust the parameters to be suitable for the kernel.

These are all good points. Did you read my discussion with Andy on patch 
2? I think it resolves all the points you have. The current CREATE_DUMB 
ioctl is unsuited for anything but the simple RGB formats. The bpp 
parameter is not very precise. The solution would be a new ioctl call 
that receives the DRM format and returns a buffer for each individual plane.

I provided a workaround patch that uses the bpp value directly if 
drm_driver_color_mode_format() does not support the bpp value. 
User-space code has to allocate a large enough buffer via the current 
CREATE_DUMB and compute the individual planes itself. See [1] for an 
example. [1] 
https://gitlab.freedesktop.org/mesa/drm/-/blob/main/tests/modetest/buffers.c?ref_type=heads#L302 
Does this work for you? Otherwise, I guess we should be talking about a 
possible CREATE_DUMB2 that fixes these shortcomings. Best regards Thomas
>
>  Tomi
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Tomi Valkeinen]

Hi,

On 15/01/2025 12:26, Thomas Zimmermann wrote:
> Hi
> 
> 
> Am 15.01.25 um 11:13 schrieb Tomi Valkeinen:
>> Hi!
>>
>> On 09/01/2025 16:57, Thomas Zimmermann wrote:
>>> Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
>>> buffer size. Align the pitch according to hardware requirements.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>> ---
>>>   drivers/gpu/drm/xlnx/zynqmp_kms.c | 7 +++++--
>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c b/drivers/gpu/drm/ 
>>> xlnx/zynqmp_kms.c
>>> index b47463473472..7ea0cd4f71d3 100644
>>> --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
>>> +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
>>> @@ -19,6 +19,7 @@
>>>   #include <drm/drm_crtc.h>
>>>   #include <drm/drm_device.h>
>>>   #include <drm/drm_drv.h>
>>> +#include <drm/drm_dumb_buffers.h>
>>>   #include <drm/drm_encoder.h>
>>>   #include <drm/drm_fbdev_dma.h>
>>>   #include <drm/drm_fourcc.h>
>>> @@ -363,10 +364,12 @@ static int zynqmp_dpsub_dumb_create(struct 
>>> drm_file *file_priv,
>>>                       struct drm_mode_create_dumb *args)
>>>   {
>>>       struct zynqmp_dpsub *dpsub = to_zynqmp_dpsub(drm);
>>> -    unsigned int pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
>>> +    int ret;
>>>         /* Enforce the alignment constraints of the DMA engine. */
>>> -    args->pitch = ALIGN(pitch, dpsub->dma_align);
>>> +    ret = drm_mode_size_dumb(drm, args, dpsub->dma_align, 0);
>>> +    if (ret)
>>> +        return ret;
>>>         return drm_gem_dma_dumb_create_internal(file_priv, drm, args);
>>>   }
>>
>> I have some trouble with this one.
>>
>> I have sent a series to add some pixel formats:
>>
>> https://lore.kernel.org/all/20250115-xilinx-formats- 
>> v2-0-160327ca652a@ideasonboard.com/
>>
>> Let's look at XV15. It's similar to NV12, but 10 bits per component, 
>> and some packing and padding.
>>
>> First plane: 3 pixels in a 32 bit group
>> Second plane: 3 pixels in a 64 bit group, 2x2 subsampled
>>
>> So, on average, a pixel on the first plane takes 32 / 3 = 10.666... 
>> bits on a line. That's not a usable number for the 
>> DRM_IOCTL_MODE_CREATE_DUMB ioctl.
>>
>> What I did was to use the pixel group size as "bpp" for 
>> DRM_IOCTL_MODE_CREATE_DUMB. So, e.g., for 720 x 576:
>>
>> Stride for first plane: 720 * (32 / 3) / 8 = 960 bytes
>> Stride for second plane: 720 / 2 * (64 / 3) / 8 = 960 bytes
>>
>> First plane: 720 / 3 = 240 pixel groups
>> Second plane: 720 / 2 / 3 = 120 pixel groups
>>
>> So I allocated the two planes with:
>> 240 x 576 with 32 bitspp
>> 120 x 288 with 64 bitspp
>>
>> This worked, and if I look at the DRM_IOCTL_MODE_CREATE_DUMB in the 
>> docs, I can't right away see anything there that says my tactic was 
>> not allowed.
>>
>> The above doesn't work anymore with this patch, as the code calls 
>> drm_driver_color_mode_format(), which fails for 64 bitspp. It feels a 
>> bit odd that DRM_IOCTL_MODE_CREATE_DUMB will try to guess the RGB 
>> fourcc for a dumb buffer allocation.
>>
>> So, what to do here? Am I doing something silly? What's the correct 
>> way to allocate the buffers for XV15? Should I just use 32 bitspp for 
>> the plane 2 too, and double the width (this works)?
>>
>> Is DRM_IOCTL_MODE_CREATE_DUMB only meant for simple RGB formats? The 
>> xilinx driver can, of course, just not use drm_mode_size_dumb(). But 
>> if so, I guess the limitations of drm_mode_size_dumb() should be 
>> documented.
>>
>> Do we need a new dumb-alloc ioctl that takes the format and plane 
>> number as parameters? Or alternatively a simpler dumb-alloc that 
>> doesn't have width and bpp, but instead takes a stride and height as 
>> parameters? I think those would be easier for the userspace to use, 
>> instead of trying to adjust the parameters to be suitable for the kernel.
> 
> These are all good points. Did you read my discussion with Andy on patch 
> 2? I think it resolves all the points you have. The current CREATE_DUMB 

I had missed the discussion, and, indeed, the patch you attached fixes 
the problem on Xilinx.

> ioctl is unsuited for anything but the simple RGB formats. The bpp 

It's a bit difficult to use, but is it really unsuited? bitsperpixel, 
width and height do give an exact pitch and size, do they not? It does 
require the userspace to handle the subsampling and planes, though, so 
far from perfect.

So, I'm all for a new ioctl, but I don't right away see why the current 
ioctl couldn't be used. Which makes me wonder about the drm_warn() in 
your patch, and the "userspace throws in arbitrary values for bpp and 
relies on the kernel to figure it out". Maybe I'm missing something here.

> parameter is not very precise. The solution would be a new ioctl call 
> that receives the DRM format and returns a buffer for each individual 
> plane.

Yes, I think that makes sense. That's a long road, though =). So my 
question is, is CREATE_DUMB really unsuitable for other than simple RGB 
formats, or can it be suitable if we just define how the userspace 
should use it for multiplanar, subsampled formats?

  Tomi

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Hi


Am 15.01.25 um 11:58 schrieb Tomi Valkeinen:
[...]
>> These are all good points. Did you read my discussion with Andy on 
>> patch 2? I think it resolves all the points you have. The current 
>> CREATE_DUMB 
>
> I had missed the discussion, and, indeed, the patch you attached fixes 
> the problem on Xilinx.

Great. Thanks for testing.

>
>> ioctl is unsuited for anything but the simple RGB formats. The bpp 
>
> It's a bit difficult to use, but is it really unsuited? bitsperpixel, 
> width and height do give an exact pitch and size, do they not? It does 
> require the userspace to handle the subsampling and planes, though, so 
> far from perfect.

The bpp value sets the number of bits per pixel; except for bpp==15 
(XRGB1555), where it sets the color depth. OR bpp is the color depth; 
except for bpp==32 (XRGB8888), where it is the number of bits per pixel. 
It's therefore best to interpret it like a color-mode enum.

>
> So, I'm all for a new ioctl, but I don't right away see why the 
> current ioctl couldn't be used. Which makes me wonder about the 
> drm_warn() in your patch, and the "userspace throws in arbitrary 
> values for bpp and relies on the kernel to figure it out". Maybe I'm 
> missing something here.

I was unsure about the drm_warn() as well. It's not really wrong to have 
odd bpp values, but handing in an unknown bpp value might point to a 
user-space error. At least there should be a drm_dbg().

>
>> parameter is not very precise. The solution would be a new ioctl call 
>> that receives the DRM format and returns a buffer for each individual 
>> plane.
>
> Yes, I think that makes sense. That's a long road, though =). So my 
> question is, is CREATE_DUMB really unsuitable for other than simple 
> RGB formats, or can it be suitable if we just define how the userspace 
> should use it for multiplanar, subsampled formats?

That would duplicate format and hardware information in user-space. Some 
hardware might have odd per-plane limitations that only the driver knows 
about. For example, there's another discussion on dri-devel about 
pitch-alignment requirements of DRM_FORMAT_MOD_LINEAR on various 
hardware. That affects dumb buffers as well. I don't think that there's 
an immediate need for a CREATE_DUMB2, but it seems worth to keep in mind.

Best regards
Thomas

>
>  Tomi
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Tomi Valkeinen]

Hi,

On 15/01/2025 13:37, Thomas Zimmermann wrote:
> Hi
> 
> 
> Am 15.01.25 um 11:58 schrieb Tomi Valkeinen:
> [...]
>>> These are all good points. Did you read my discussion with Andy on 
>>> patch 2? I think it resolves all the points you have. The current 
>>> CREATE_DUMB 
>>
>> I had missed the discussion, and, indeed, the patch you attached fixes 
>> the problem on Xilinx.
> 
> Great. Thanks for testing.
> 
>>
>>> ioctl is unsuited for anything but the simple RGB formats. The bpp 
>>
>> It's a bit difficult to use, but is it really unsuited? bitsperpixel, 
>> width and height do give an exact pitch and size, do they not? It does 
>> require the userspace to handle the subsampling and planes, though, so 
>> far from perfect.
> 
> The bpp value sets the number of bits per pixel; except for bpp==15 
> (XRGB1555), where it sets the color depth. OR bpp is the color depth; 
> except for bpp==32 (XRGB8888), where it is the number of bits per pixel. 
> It's therefore best to interpret it like a color-mode enum.

Ah, right... That's horrible =).

And I assume it's not really possible to define the bpp to mean bits per 
pixel, except for a few special cases like 15?

Why do we even really care about color depth here? We're just allocating 
memory. Doesn't DIV_ROUND_UP(args->bpp, SZ_8) work fine for XRGB1555 too?

>> So, I'm all for a new ioctl, but I don't right away see why the 
>> current ioctl couldn't be used. Which makes me wonder about the 
>> drm_warn() in your patch, and the "userspace throws in arbitrary 
>> values for bpp and relies on the kernel to figure it out". Maybe I'm 
>> missing something here.
> 
> I was unsure about the drm_warn() as well. It's not really wrong to have 
> odd bpp values, but handing in an unknown bpp value might point to a 
> user-space error. At least there should be a drm_dbg().
> 
>>
>>> parameter is not very precise. The solution would be a new ioctl call 
>>> that receives the DRM format and returns a buffer for each individual 
>>> plane.
>>
>> Yes, I think that makes sense. That's a long road, though =). So my 
>> question is, is CREATE_DUMB really unsuitable for other than simple 
>> RGB formats, or can it be suitable if we just define how the userspace 
>> should use it for multiplanar, subsampled formats?
> 
> That would duplicate format and hardware information in user-space. Some 

But we already have that, don't we? We have drivers and userspace that 
support, say, NV12 via dumb buffers. But (correct me if I'm wrong) we 
don't document how CREATE_DUMB has to be used to allocate multiplanar 
subsampled buffers, so the userspace devs have to "guess".

> hardware might have odd per-plane limitations that only the driver knows 
> about. For example, there's another discussion on dri-devel about pitch- 
> alignment requirements of DRM_FORMAT_MOD_LINEAR on various hardware. 
> That affects dumb buffers as well. I don't think that there's an 
> immediate need for a CREATE_DUMB2, but it seems worth to keep in mind.

Yes, the current CREATE_DUMB can't cover all the hardware. We do need 
CREATE_DUMB2, sooner or later. I just hope we can define and document a 
set of rules that allows using CREATE_DUMB for the cases where it 
sensibly works (and is already being used).

  Tomi

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Hi


Am 15.01.25 um 13:06 schrieb Tomi Valkeinen:
> Hi,
>
> On 15/01/2025 13:37, Thomas Zimmermann wrote:
>> Hi
>>
>>
>> Am 15.01.25 um 11:58 schrieb Tomi Valkeinen:
>> [...]
>>>> These are all good points. Did you read my discussion with Andy on 
>>>> patch 2? I think it resolves all the points you have. The current 
>>>> CREATE_DUMB 
>>>
>>> I had missed the discussion, and, indeed, the patch you attached 
>>> fixes the problem on Xilinx.
>>
>> Great. Thanks for testing.
>>
>>>
>>>> ioctl is unsuited for anything but the simple RGB formats. The bpp 
>>>
>>> It's a bit difficult to use, but is it really unsuited? 
>>> bitsperpixel, width and height do give an exact pitch and size, do 
>>> they not? It does require the userspace to handle the subsampling 
>>> and planes, though, so far from perfect.
>>
>> The bpp value sets the number of bits per pixel; except for bpp==15 
>> (XRGB1555), where it sets the color depth. OR bpp is the color depth; 
>> except for bpp==32 (XRGB8888), where it is the number of bits per 
>> pixel. It's therefore best to interpret it like a color-mode enum.
>
> Ah, right... That's horrible =).
>
> And I assume it's not really possible to define the bpp to mean bits 
> per pixel, except for a few special cases like 15?
>
> Why do we even really care about color depth here? We're just 
> allocating memory. Doesn't DIV_ROUND_UP(args->bpp, SZ_8) work fine for 
> XRGB1555 too?

Drivers always did that, but it does not work correctly for (bpp < 8). 
As we already have helpers to deal with bpp, it makes sense to use 
them.  This also aligns dumb buffers with the kernel's video= parameter, 
which as the same odd semantics. The fallback that uses bpp directly 
will hopefully be the exception.

>
>>> So, I'm all for a new ioctl, but I don't right away see why the 
>>> current ioctl couldn't be used. Which makes me wonder about the 
>>> drm_warn() in your patch, and the "userspace throws in arbitrary 
>>> values for bpp and relies on the kernel to figure it out". Maybe I'm 
>>> missing something here.
>>
>> I was unsure about the drm_warn() as well. It's not really wrong to 
>> have odd bpp values, but handing in an unknown bpp value might point 
>> to a user-space error. At least there should be a drm_dbg().
>>
>>>
>>>> parameter is not very precise. The solution would be a new ioctl 
>>>> call that receives the DRM format and returns a buffer for each 
>>>> individual plane.
>>>
>>> Yes, I think that makes sense. That's a long road, though =). So my 
>>> question is, is CREATE_DUMB really unsuitable for other than simple 
>>> RGB formats, or can it be suitable if we just define how the 
>>> userspace should use it for multiplanar, subsampled formats?
>>
>> That would duplicate format and hardware information in user-space. Some 
>
> But we already have that, don't we? We have drivers and userspace that 
> support, say, NV12 via dumb buffers. But (correct me if I'm wrong) we 
> don't document how CREATE_DUMB has to be used to allocate multiplanar 
> subsampled buffers, so the userspace devs have to "guess".

Yeah, there are constrains in the scanline and buffer alignments and 
orientation. And if we say that bpp==12 means NV12, it will be a problem 
for all other cases where bpp==12 makes sense.

Best regards
Thomas

>
>> hardware might have odd per-plane limitations that only the driver 
>> knows about. For example, there's another discussion on dri-devel 
>> about pitch- alignment requirements of DRM_FORMAT_MOD_LINEAR on 
>> various hardware. That affects dumb buffers as well. I don't think 
>> that there's an immediate need for a CREATE_DUMB2, but it seems worth 
>> to keep in mind.
>
> Yes, the current CREATE_DUMB can't cover all the hardware. We do need 
> CREATE_DUMB2, sooner or later. I just hope we can define and document 
> a set of rules that allows using CREATE_DUMB for the cases where it 
> sensibly works (and is already being used).
>
>  Tomi
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Tomi Valkeinen]

Hi,

On 15/01/2025 14:34, Thomas Zimmermann wrote:
> Hi
> 
> 
> Am 15.01.25 um 13:06 schrieb Tomi Valkeinen:
>> Hi,
>>
>> On 15/01/2025 13:37, Thomas Zimmermann wrote:
>>> Hi
>>>
>>>
>>> Am 15.01.25 um 11:58 schrieb Tomi Valkeinen:
>>> [...]
>>>>> These are all good points. Did you read my discussion with Andy on 
>>>>> patch 2? I think it resolves all the points you have. The current 
>>>>> CREATE_DUMB 
>>>>
>>>> I had missed the discussion, and, indeed, the patch you attached 
>>>> fixes the problem on Xilinx.
>>>
>>> Great. Thanks for testing.
>>>
>>>>
>>>>> ioctl is unsuited for anything but the simple RGB formats. The bpp 
>>>>
>>>> It's a bit difficult to use, but is it really unsuited? 
>>>> bitsperpixel, width and height do give an exact pitch and size, do 
>>>> they not? It does require the userspace to handle the subsampling 
>>>> and planes, though, so far from perfect.
>>>
>>> The bpp value sets the number of bits per pixel; except for bpp==15 
>>> (XRGB1555), where it sets the color depth. OR bpp is the color depth; 
>>> except for bpp==32 (XRGB8888), where it is the number of bits per 
>>> pixel. It's therefore best to interpret it like a color-mode enum.
>>
>> Ah, right... That's horrible =).
>>
>> And I assume it's not really possible to define the bpp to mean bits 
>> per pixel, except for a few special cases like 15?
>>
>> Why do we even really care about color depth here? We're just 
>> allocating memory. Doesn't DIV_ROUND_UP(args->bpp, SZ_8) work fine for 
>> XRGB1555 too?
> 
> Drivers always did that, but it does not work correctly for (bpp < 8). 
> As we already have helpers to deal with bpp, it makes sense to use 
> them.  This also aligns dumb buffers with the kernel's video= parameter, 
> which as the same odd semantics. The fallback that uses bpp directly 
> will hopefully be the exception.

Hmm, well... If we had a 64-bit RGB format in the list of "legacy fb 
formats", I wouldn't have noticed anything. And if I would just use 32 
as the bpp, and adjust width accordingly, it would also have worked. So, 
I expect the fallback to be an exception,

And by working I mean that I can run my apps fine, but the internal 
operation would sure be odd: allocating any YUV buffer will cause 
drm_mode_size_dumb() to get an RGB format from 
drm_driver_color_mode_format(), and get a drm_format_info_min_pitch() 
for an RGB format.

>>>> So, I'm all for a new ioctl, but I don't right away see why the 
>>>> current ioctl couldn't be used. Which makes me wonder about the 
>>>> drm_warn() in your patch, and the "userspace throws in arbitrary 
>>>> values for bpp and relies on the kernel to figure it out". Maybe I'm 
>>>> missing something here.
>>>
>>> I was unsure about the drm_warn() as well. It's not really wrong to 
>>> have odd bpp values, but handing in an unknown bpp value might point 
>>> to a user-space error. At least there should be a drm_dbg().
>>>
>>>>
>>>>> parameter is not very precise. The solution would be a new ioctl 
>>>>> call that receives the DRM format and returns a buffer for each 
>>>>> individual plane.
>>>>
>>>> Yes, I think that makes sense. That's a long road, though =). So my 
>>>> question is, is CREATE_DUMB really unsuitable for other than simple 
>>>> RGB formats, or can it be suitable if we just define how the 
>>>> userspace should use it for multiplanar, subsampled formats?
>>>
>>> That would duplicate format and hardware information in user-space. Some 
>>
>> But we already have that, don't we? We have drivers and userspace that 
>> support, say, NV12 via dumb buffers. But (correct me if I'm wrong) we 
>> don't document how CREATE_DUMB has to be used to allocate multiplanar 
>> subsampled buffers, so the userspace devs have to "guess".
> 
> Yeah, there are constrains in the scanline and buffer alignments and 
> orientation. And if we say that bpp==12 means NV12, it will be a problem 
> for all other cases where bpp==12 makes sense.

I feel I still don't quite understand. Can't we define and document 
CREATE_DUMB like this:

If (bpp < 8 || is_power_of_two(bpp))
	bpp means bitsperpixel
	pitch is args->width * args->bpp / 8, aligned up to driver-specific-align
else
	bpp is a legacy parameter, and we deal with it case by case.
	list the cases and what they mean

And describe that when allocating subsampled buffers, the caller must 
adjust the width and height accordingly. And that the bpp and width can 
also refer to pixel groups.

Or if the currently existing code prevents the above for 16 and 32 bpps, 
how about defining that any non-RGB or not-simple buffer has to be 
allocated with bpp=8, and the userspace has to align the pitch correctly 
according to the format and platform's hw restrictions?

  Tomi

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Hi


Am 15.01.25 um 14:33 schrieb Tomi Valkeinen:
[...]
>> Yeah, there are constrains in the scanline and buffer alignments and 
>> orientation. And if we say that bpp==12 means NV12, it will be a 
>> problem for all other cases where bpp==12 makes sense.
>
> I feel I still don't quite understand. Can't we define and document 
> CREATE_DUMB like this:
>
> If (bpp < 8 || is_power_of_two(bpp))
>     bpp means bitsperpixel
>     pitch is args->width * args->bpp / 8, aligned up to 
> driver-specific-align
> else
>     bpp is a legacy parameter, and we deal with it case by case.
>     list the cases and what they mean
>
> And describe that when allocating subsampled buffers, the caller must 
> adjust the width and height accordingly. And that the bpp and width 
> can also refer to pixel groups.
>
> Or if the currently existing code prevents the above for 16 and 32 
> bpps, how about defining that any non-RGB or not-simple buffer has to 
> be allocated with bpp=8, and the userspace has to align the pitch 
> correctly according to the format and platform's hw restrictions?

What if a hardware requires certain per-format alignments? Or requires 
certain alignments for each plane? Or only supports tile modes? Or has 
strict limits on the maximum buffer size?

It is not possible to encode all this in a simple 32-bit value. So 
user-space code has to be aware of all this and tweak bpp-based 
allocation to make it work. Obviously you can use the current UAPI for 
your use case. It's just not optimal or future proof.

Best regards
Thomas

>
>
>  Tomi
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Tomi Valkeinen]

On 15/01/2025 15:45, Thomas Zimmermann wrote:
> Hi
> 
> 
> Am 15.01.25 um 14:33 schrieb Tomi Valkeinen:
> [...]
>>> Yeah, there are constrains in the scanline and buffer alignments and 
>>> orientation. And if we say that bpp==12 means NV12, it will be a 
>>> problem for all other cases where bpp==12 makes sense.
>>
>> I feel I still don't quite understand. Can't we define and document 
>> CREATE_DUMB like this:
>>
>> If (bpp < 8 || is_power_of_two(bpp))
>>     bpp means bitsperpixel
>>     pitch is args->width * args->bpp / 8, aligned up to driver- 
>> specific-align
>> else
>>     bpp is a legacy parameter, and we deal with it case by case.
>>     list the cases and what they mean
>>
>> And describe that when allocating subsampled buffers, the caller must 
>> adjust the width and height accordingly. And that the bpp and width 
>> can also refer to pixel groups.
>>
>> Or if the currently existing code prevents the above for 16 and 32 
>> bpps, how about defining that any non-RGB or not-simple buffer has to 
>> be allocated with bpp=8, and the userspace has to align the pitch 
>> correctly according to the format and platform's hw restrictions?
> 
> What if a hardware requires certain per-format alignments? Or requires 
> certain alignments for each plane? Or only supports tile modes? Or has 
> strict limits on the maximum buffer size?
> 
> It is not possible to encode all this in a simple 32-bit value. So user- 
> space code has to be aware of all this and tweak bpp-based allocation to 
> make it work. Obviously you can use the current UAPI for your use case. 
> It's just not optimal or future proof.

No disagreement there, we need CREATE_DUMB2.

My point is that we have the current UAPI, and we have userspace using 
it, but we don't have clear rules what the ioctl does with specific 
parameters, and we don't document how it has to be used.

Perhaps the situation is bad, and all we can really say is that 
CREATE_DUMB only works for use with simple RGB formats, and the behavior 
for all other formats is platform specific. But I think even that would 
be valuable in the UAPI docs.

Thinking about this, I wonder if this change is good for omapdrm or 
xilinx (probably other platforms too that support non-simple non-RGB 
formats via dumb buffers): without this patch, in both drivers, the 
pitch calculations just take the bpp as bit-per-pixels, align it up, and 
that's it.

With this patch we end up using drm_driver_color_mode_format(), and 
aligning buffers according to RGB formats figured out via heuristics. It 
does happen to work, for the formats I tested, but it sounds like 
something that might easily not work, as it's doing adjustments based on 
wrong format.

Should we have another version of drm_mode_size_dumb() which just 
calculates using the bpp, without the drm_driver_color_mode_format() 
path? Or does the drm_driver_color_mode_format() path provide some value 
for the drivers that do not currently do anything similar?

  Tomi

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Daniel Stone]

On Wed, 15 Jan 2025 at 14:20, Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
> No disagreement there, we need CREATE_DUMB2.
>
> My point is that we have the current UAPI, and we have userspace using
> it, but we don't have clear rules what the ioctl does with specific
> parameters, and we don't document how it has to be used.
>
> Perhaps the situation is bad, and all we can really say is that
> CREATE_DUMB only works for use with simple RGB formats, and the behavior
> for all other formats is platform specific. But I think even that would
> be valuable in the UAPI docs.

Yeah, CREATE_DUMB only works for use with simple RGB formats in a
linear layout. Not monochrome or YUV or tiled or displayed rotated or
whatever.

If it happens to accidentally work for other uses, that's fine, but
it's not generically reliable for anything other than simple linear
RGB. It's intended to let you do splash screens, consoles, recovery
password entries, and software-rendered compositors if you really
want. Anything more than that isn't 'dumb'.

Cheers,
Daniel

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Laurent Pinchart]

On Wed, Jan 15, 2025 at 02:34:26PM +0000, Daniel Stone wrote:
> On Wed, 15 Jan 2025 at 14:20, Tomi Valkeinen wrote:
> > No disagreement there, we need CREATE_DUMB2.
> >
> > My point is that we have the current UAPI, and we have userspace using
> > it, but we don't have clear rules what the ioctl does with specific
> > parameters, and we don't document how it has to be used.
> >
> > Perhaps the situation is bad, and all we can really say is that
> > CREATE_DUMB only works for use with simple RGB formats, and the behavior
> > for all other formats is platform specific. But I think even that would
> > be valuable in the UAPI docs.
> 
> Yeah, CREATE_DUMB only works for use with simple RGB formats in a
> linear layout. Not monochrome or YUV or tiled or displayed rotated or
> whatever.
> 
> If it happens to accidentally work for other uses, that's fine, but
> it's not generically reliable for anything other than simple linear
> RGB. It's intended to let you do splash screens, consoles, recovery
> password entries, and software-rendered compositors if you really
> want. Anything more than that isn't 'dumb'.

We have lots of software out there that rely on CREATE_DUMB supporting
YUV linear formats, and lots of drivers (mostly on Arm I suppose) that
implement YUV support in CREATE_DUMB. I'm fine replacing it with
something better, but I think we need a standard ioctl that can create
linear YUV buffers. I've been told many times that DRM doesn't want to
standardize buffer allocation further than what CREATE_DUMB is made for.
Can we reconsider this rule then ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Laurent Pinchart]

On Thu, Jan 16, 2025 at 10:43:40AM +0200, Laurent Pinchart wrote:
> On Wed, Jan 15, 2025 at 02:34:26PM +0000, Daniel Stone wrote:
> > On Wed, 15 Jan 2025 at 14:20, Tomi Valkeinen wrote:
> > > No disagreement there, we need CREATE_DUMB2.
> > >
> > > My point is that we have the current UAPI, and we have userspace using
> > > it, but we don't have clear rules what the ioctl does with specific
> > > parameters, and we don't document how it has to be used.
> > >
> > > Perhaps the situation is bad, and all we can really say is that
> > > CREATE_DUMB only works for use with simple RGB formats, and the behavior
> > > for all other formats is platform specific. But I think even that would
> > > be valuable in the UAPI docs.
> > 
> > Yeah, CREATE_DUMB only works for use with simple RGB formats in a
> > linear layout. Not monochrome or YUV or tiled or displayed rotated or
> > whatever.
> > 
> > If it happens to accidentally work for other uses, that's fine, but
> > it's not generically reliable for anything other than simple linear
> > RGB. It's intended to let you do splash screens, consoles, recovery
> > password entries, and software-rendered compositors if you really
> > want. Anything more than that isn't 'dumb'.
> 
> We have lots of software out there that rely on CREATE_DUMB supporting
> YUV linear formats, and lots of drivers (mostly on Arm I suppose) that
> implement YUV support in CREATE_DUMB. I'm fine replacing it with
> something better, but I think we need a standard ioctl that can create
> linear YUV buffers. I've been told many times that DRM doesn't want to
> standardize buffer allocation further than what CREATE_DUMB is made for.
> Can we reconsider this rule then ?

Actually... Instead of adding a CREATE_DUMB2, it would be best on trying
to leverage DMA heaps and deprecate allocating from the KMS device.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Tomi Valkeinen]

Hi,

On 16/01/2025 11:38, Laurent Pinchart wrote:
> On Thu, Jan 16, 2025 at 10:43:40AM +0200, Laurent Pinchart wrote:
>> On Wed, Jan 15, 2025 at 02:34:26PM +0000, Daniel Stone wrote:
>>> On Wed, 15 Jan 2025 at 14:20, Tomi Valkeinen wrote:
>>>> No disagreement there, we need CREATE_DUMB2.
>>>>
>>>> My point is that we have the current UAPI, and we have userspace using
>>>> it, but we don't have clear rules what the ioctl does with specific
>>>> parameters, and we don't document how it has to be used.
>>>>
>>>> Perhaps the situation is bad, and all we can really say is that
>>>> CREATE_DUMB only works for use with simple RGB formats, and the behavior
>>>> for all other formats is platform specific. But I think even that would
>>>> be valuable in the UAPI docs.
>>>
>>> Yeah, CREATE_DUMB only works for use with simple RGB formats in a
>>> linear layout. Not monochrome or YUV or tiled or displayed rotated or
>>> whatever.
>>>
>>> If it happens to accidentally work for other uses, that's fine, but
>>> it's not generically reliable for anything other than simple linear
>>> RGB. It's intended to let you do splash screens, consoles, recovery
>>> password entries, and software-rendered compositors if you really
>>> want. Anything more than that isn't 'dumb'.
>>
>> We have lots of software out there that rely on CREATE_DUMB supporting
>> YUV linear formats, and lots of drivers (mostly on Arm I suppose) that
>> implement YUV support in CREATE_DUMB. I'm fine replacing it with
>> something better, but I think we need a standard ioctl that can create
>> linear YUV buffers. I've been told many times that DRM doesn't want to
>> standardize buffer allocation further than what CREATE_DUMB is made for.
>> Can we reconsider this rule then ?
> 
> Actually... Instead of adding a CREATE_DUMB2, it would be best on trying
> to leverage DMA heaps and deprecate allocating from the KMS device.

If we allocate from DMA heaps, I think we then need a DRM ioctl which 
will tell you how big buffer(s) you need, based on the size and format.

  Tomi

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Laurent Pinchart]

On Thu, Jan 16, 2025 at 12:07:49PM +0200, Tomi Valkeinen wrote:
> On 16/01/2025 11:38, Laurent Pinchart wrote:
> > On Thu, Jan 16, 2025 at 10:43:40AM +0200, Laurent Pinchart wrote:
> >> On Wed, Jan 15, 2025 at 02:34:26PM +0000, Daniel Stone wrote:
> >>> On Wed, 15 Jan 2025 at 14:20, Tomi Valkeinen wrote:
> >>>> No disagreement there, we need CREATE_DUMB2.
> >>>>
> >>>> My point is that we have the current UAPI, and we have userspace using
> >>>> it, but we don't have clear rules what the ioctl does with specific
> >>>> parameters, and we don't document how it has to be used.
> >>>>
> >>>> Perhaps the situation is bad, and all we can really say is that
> >>>> CREATE_DUMB only works for use with simple RGB formats, and the behavior
> >>>> for all other formats is platform specific. But I think even that would
> >>>> be valuable in the UAPI docs.
> >>>
> >>> Yeah, CREATE_DUMB only works for use with simple RGB formats in a
> >>> linear layout. Not monochrome or YUV or tiled or displayed rotated or
> >>> whatever.
> >>>
> >>> If it happens to accidentally work for other uses, that's fine, but
> >>> it's not generically reliable for anything other than simple linear
> >>> RGB. It's intended to let you do splash screens, consoles, recovery
> >>> password entries, and software-rendered compositors if you really
> >>> want. Anything more than that isn't 'dumb'.
> >>
> >> We have lots of software out there that rely on CREATE_DUMB supporting
> >> YUV linear formats, and lots of drivers (mostly on Arm I suppose) that
> >> implement YUV support in CREATE_DUMB. I'm fine replacing it with
> >> something better, but I think we need a standard ioctl that can create
> >> linear YUV buffers. I've been told many times that DRM doesn't want to
> >> standardize buffer allocation further than what CREATE_DUMB is made for.
> >> Can we reconsider this rule then ?
> > 
> > Actually... Instead of adding a CREATE_DUMB2, it would be best on trying
> > to leverage DMA heaps and deprecate allocating from the KMS device.
> 
> If we allocate from DMA heaps, I think we then need a DRM ioctl which 
> will tell you how big buffer(s) you need, based on the size and format.

We will at least a kernel API to expose constraints to userspace.
Whether that should calculate the buffer size for a given format, or
expose information to userspace to enable that calculation, I'm not
sure. But regardless of which option we pick, I agree we likely need a
new API to enable usage of DMA heaps as allocators.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Hi


Am 15.01.25 um 15:20 schrieb Tomi Valkeinen:
[...]
>
> My point is that we have the current UAPI, and we have userspace using 
> it, but we don't have clear rules what the ioctl does with specific 
> parameters, and we don't document how it has to be used.
>
> Perhaps the situation is bad, and all we can really say is that 
> CREATE_DUMB only works for use with simple RGB formats, and the 
> behavior for all other formats is platform specific. But I think even 
> that would be valuable in the UAPI docs.

To be honest, I would not want to specify behavior for anything but the 
linear RGB formats. If anything, I'd take Daniel's reply mail for 
documentation as-is. Anyone stretching the UAPI beyond RGB is on their own.

>
> Thinking about this, I wonder if this change is good for omapdrm or 
> xilinx (probably other platforms too that support non-simple non-RGB 
> formats via dumb buffers): without this patch, in both drivers, the 
> pitch calculations just take the bpp as bit-per-pixels, align it up, 
> and that's it.
>
> With this patch we end up using drm_driver_color_mode_format(), and 
> aligning buffers according to RGB formats figured out via heuristics. 
> It does happen to work, for the formats I tested, but it sounds like 
> something that might easily not work, as it's doing adjustments based 
> on wrong format.
>
> Should we have another version of drm_mode_size_dumb() which just 
> calculates using the bpp, without the drm_driver_color_mode_format() 
> path? Or does the drm_driver_color_mode_format() path provide some 
> value for the drivers that do not currently do anything similar?

With the RGB-only rule, using drm_driver_color_mode_format() makes 
sense. It aligns dumb buffers and video=, provides error checking, and 
overall harmonizes code. The fallback is only required because of the 
existing odd cases that already bend the UAPI's rules.

Best regards
Thomas

>
>  Tomi
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Tomi Valkeinen]

Hi,

On 16/01/2025 10:09, Thomas Zimmermann wrote:
> Hi
> 
> 
> Am 15.01.25 um 15:20 schrieb Tomi Valkeinen:
> [...]
>>
>> My point is that we have the current UAPI, and we have userspace using 
>> it, but we don't have clear rules what the ioctl does with specific 
>> parameters, and we don't document how it has to be used.
>>
>> Perhaps the situation is bad, and all we can really say is that 
>> CREATE_DUMB only works for use with simple RGB formats, and the 
>> behavior for all other formats is platform specific. But I think even 
>> that would be valuable in the UAPI docs.
> 
> To be honest, I would not want to specify behavior for anything but the 
> linear RGB formats. If anything, I'd take Daniel's reply mail for 
> documentation as-is. Anyone stretching the UAPI beyond RGB is on their own.
> 
>>
>> Thinking about this, I wonder if this change is good for omapdrm or 
>> xilinx (probably other platforms too that support non-simple non-RGB 
>> formats via dumb buffers): without this patch, in both drivers, the 
>> pitch calculations just take the bpp as bit-per-pixels, align it up, 
>> and that's it.
>>
>> With this patch we end up using drm_driver_color_mode_format(), and 
>> aligning buffers according to RGB formats figured out via heuristics. 
>> It does happen to work, for the formats I tested, but it sounds like 
>> something that might easily not work, as it's doing adjustments based 
>> on wrong format.
>>
>> Should we have another version of drm_mode_size_dumb() which just 
>> calculates using the bpp, without the drm_driver_color_mode_format() 
>> path? Or does the drm_driver_color_mode_format() path provide some 
>> value for the drivers that do not currently do anything similar?
> 
> With the RGB-only rule, using drm_driver_color_mode_format() makes 
> sense. It aligns dumb buffers and video=, provides error checking, and 
> overall harmonizes code. The fallback is only required because of the 
> existing odd cases that already bend the UAPI's rules.

I have to disagree here.

On the platforms I have been using (omap, tidss, xilinx, rcar) the dumb 
buffers are the only buffers you can get from the DRM driver. The dumb 
buffers have been used to allocate linear and multiplanar YUV buffers 
for a very long time on those platforms.

I tried to look around, but I did not find any mentions that CREATE_DUMB 
should only be used for RGB buffers. Is anyone outside the core 
developers even aware of it?

If we don't use dumb buffers there, where do we get the buffers? Maybe 
from a v4l2 device or from a gpu device, but often you don't have those. 
DMA_HEAP is there, of course.

So we have the option to get DMA_HEAP buffers, specifying just the size 
of the buffer. Here we only specify the size, so the userspace has to 
understand the requirements for the format and the platform.

Or we can use CREATE_DUMB, specifying the width, height and 
bitsperpixel, and if we don't have any heuristics about figuring out the 
pixel format (as it has been), the end result is exactly the same as 
with DMA_HEAP (i.e. we essentially define the size of the buffer).

So, on these platforms (omap, tidss, xilinx, rcar), the CREATE_DUMB has 
always been just "give me X amount of memory that can be used for 
scanout". With this series, the meaning of the ioctl changes, and it's 
now "give me an memory buffer buffer that works with an RGB format with 
this width, height, bpp".

In practice I believe that doesn't cause regressions, as aligning 
buffers according to RGB pixel format rules happens to be fine for YUV 
formats too, but I'm not sure (and it already almost caused a regression 
with bpp=64). And I'm having trouble seeing the upside.

Aligning video= and dumb buffers almost sounds like going backwards. 
video= parameter is bad, so let's also make dumb buffers bad?

Harmonizing code is fine, but I think that can be done with a function 
that only does the fallback-case.

So... I can only speak for the platforms I'm using and maintaining, but 
I'd rather keep the old behavior for CREATE_DUMB that we've had for ages.

  Tomi

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Geert Uytterhoeven]

On Thu, Jan 16, 2025 at 11:03 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
> On 16/01/2025 10:09, Thomas Zimmermann wrote:
> > Am 15.01.25 um 15:20 schrieb Tomi Valkeinen:
> > [...]
> >>
> >> My point is that we have the current UAPI, and we have userspace using
> >> it, but we don't have clear rules what the ioctl does with specific
> >> parameters, and we don't document how it has to be used.
> >>
> >> Perhaps the situation is bad, and all we can really say is that
> >> CREATE_DUMB only works for use with simple RGB formats, and the
> >> behavior for all other formats is platform specific. But I think even
> >> that would be valuable in the UAPI docs.
> >
> > To be honest, I would not want to specify behavior for anything but the
> > linear RGB formats. If anything, I'd take Daniel's reply mail for
> > documentation as-is. Anyone stretching the UAPI beyond RGB is on their own.
> >
> >> Thinking about this, I wonder if this change is good for omapdrm or
> >> xilinx (probably other platforms too that support non-simple non-RGB
> >> formats via dumb buffers): without this patch, in both drivers, the
> >> pitch calculations just take the bpp as bit-per-pixels, align it up,
> >> and that's it.
> >>
> >> With this patch we end up using drm_driver_color_mode_format(), and
> >> aligning buffers according to RGB formats figured out via heuristics.
> >> It does happen to work, for the formats I tested, but it sounds like
> >> something that might easily not work, as it's doing adjustments based
> >> on wrong format.
> >>
> >> Should we have another version of drm_mode_size_dumb() which just
> >> calculates using the bpp, without the drm_driver_color_mode_format()
> >> path? Or does the drm_driver_color_mode_format() path provide some
> >> value for the drivers that do not currently do anything similar?
> >
> > With the RGB-only rule, using drm_driver_color_mode_format() makes
> > sense. It aligns dumb buffers and video=, provides error checking, and
> > overall harmonizes code. The fallback is only required because of the
> > existing odd cases that already bend the UAPI's rules.
>
> I have to disagree here.
>
> On the platforms I have been using (omap, tidss, xilinx, rcar) the dumb
> buffers are the only buffers you can get from the DRM driver. The dumb
> buffers have been used to allocate linear and multiplanar YUV buffers
> for a very long time on those platforms.
>
> I tried to look around, but I did not find any mentions that CREATE_DUMB
> should only be used for RGB buffers. Is anyone outside the core
> developers even aware of it?
>
> If we don't use dumb buffers there, where do we get the buffers? Maybe
> from a v4l2 device or from a gpu device, but often you don't have those.
> DMA_HEAP is there, of course.

Why can't there be a variant that takes a proper fourcc format instead of
an imprecise bpp value?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Tomi Valkeinen]

Hi,

On 16/01/2025 12:17, Geert Uytterhoeven wrote:
> On Thu, Jan 16, 2025 at 11:03 AM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>> On 16/01/2025 10:09, Thomas Zimmermann wrote:
>>> Am 15.01.25 um 15:20 schrieb Tomi Valkeinen:
>>> [...]
>>>>
>>>> My point is that we have the current UAPI, and we have userspace using
>>>> it, but we don't have clear rules what the ioctl does with specific
>>>> parameters, and we don't document how it has to be used.
>>>>
>>>> Perhaps the situation is bad, and all we can really say is that
>>>> CREATE_DUMB only works for use with simple RGB formats, and the
>>>> behavior for all other formats is platform specific. But I think even
>>>> that would be valuable in the UAPI docs.
>>>
>>> To be honest, I would not want to specify behavior for anything but the
>>> linear RGB formats. If anything, I'd take Daniel's reply mail for
>>> documentation as-is. Anyone stretching the UAPI beyond RGB is on their own.
>>>
>>>> Thinking about this, I wonder if this change is good for omapdrm or
>>>> xilinx (probably other platforms too that support non-simple non-RGB
>>>> formats via dumb buffers): without this patch, in both drivers, the
>>>> pitch calculations just take the bpp as bit-per-pixels, align it up,
>>>> and that's it.
>>>>
>>>> With this patch we end up using drm_driver_color_mode_format(), and
>>>> aligning buffers according to RGB formats figured out via heuristics.
>>>> It does happen to work, for the formats I tested, but it sounds like
>>>> something that might easily not work, as it's doing adjustments based
>>>> on wrong format.
>>>>
>>>> Should we have another version of drm_mode_size_dumb() which just
>>>> calculates using the bpp, without the drm_driver_color_mode_format()
>>>> path? Or does the drm_driver_color_mode_format() path provide some
>>>> value for the drivers that do not currently do anything similar?
>>>
>>> With the RGB-only rule, using drm_driver_color_mode_format() makes
>>> sense. It aligns dumb buffers and video=, provides error checking, and
>>> overall harmonizes code. The fallback is only required because of the
>>> existing odd cases that already bend the UAPI's rules.
>>
>> I have to disagree here.
>>
>> On the platforms I have been using (omap, tidss, xilinx, rcar) the dumb
>> buffers are the only buffers you can get from the DRM driver. The dumb
>> buffers have been used to allocate linear and multiplanar YUV buffers
>> for a very long time on those platforms.
>>
>> I tried to look around, but I did not find any mentions that CREATE_DUMB
>> should only be used for RGB buffers. Is anyone outside the core
>> developers even aware of it?
>>
>> If we don't use dumb buffers there, where do we get the buffers? Maybe
>> from a v4l2 device or from a gpu device, but often you don't have those.
>> DMA_HEAP is there, of course.
> 
> Why can't there be a variant that takes a proper fourcc format instead of
> an imprecise bpp value?

There can, but it's somewhat a different topic, although it's been 
covered a bit in this thread.

My specific concern here is the current CREATE_DUMB, and (not) changing 
how it behaves.

  Tomi

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Dmitry Baryshkov]

On Thu, Jan 16, 2025 at 11:17:50AM +0100, Geert Uytterhoeven wrote:
> On Thu, Jan 16, 2025 at 11:03 AM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
> > On 16/01/2025 10:09, Thomas Zimmermann wrote:
> > > Am 15.01.25 um 15:20 schrieb Tomi Valkeinen:
> > > [...]
> > >>
> > >> My point is that we have the current UAPI, and we have userspace using
> > >> it, but we don't have clear rules what the ioctl does with specific
> > >> parameters, and we don't document how it has to be used.
> > >>
> > >> Perhaps the situation is bad, and all we can really say is that
> > >> CREATE_DUMB only works for use with simple RGB formats, and the
> > >> behavior for all other formats is platform specific. But I think even
> > >> that would be valuable in the UAPI docs.
> > >
> > > To be honest, I would not want to specify behavior for anything but the
> > > linear RGB formats. If anything, I'd take Daniel's reply mail for
> > > documentation as-is. Anyone stretching the UAPI beyond RGB is on their own.
> > >
> > >> Thinking about this, I wonder if this change is good for omapdrm or
> > >> xilinx (probably other platforms too that support non-simple non-RGB
> > >> formats via dumb buffers): without this patch, in both drivers, the
> > >> pitch calculations just take the bpp as bit-per-pixels, align it up,
> > >> and that's it.
> > >>
> > >> With this patch we end up using drm_driver_color_mode_format(), and
> > >> aligning buffers according to RGB formats figured out via heuristics.
> > >> It does happen to work, for the formats I tested, but it sounds like
> > >> something that might easily not work, as it's doing adjustments based
> > >> on wrong format.
> > >>
> > >> Should we have another version of drm_mode_size_dumb() which just
> > >> calculates using the bpp, without the drm_driver_color_mode_format()
> > >> path? Or does the drm_driver_color_mode_format() path provide some
> > >> value for the drivers that do not currently do anything similar?
> > >
> > > With the RGB-only rule, using drm_driver_color_mode_format() makes
> > > sense. It aligns dumb buffers and video=, provides error checking, and
> > > overall harmonizes code. The fallback is only required because of the
> > > existing odd cases that already bend the UAPI's rules.
> >
> > I have to disagree here.
> >
> > On the platforms I have been using (omap, tidss, xilinx, rcar) the dumb
> > buffers are the only buffers you can get from the DRM driver. The dumb
> > buffers have been used to allocate linear and multiplanar YUV buffers
> > for a very long time on those platforms.
> >
> > I tried to look around, but I did not find any mentions that CREATE_DUMB
> > should only be used for RGB buffers. Is anyone outside the core
> > developers even aware of it?
> >
> > If we don't use dumb buffers there, where do we get the buffers? Maybe
> > from a v4l2 device or from a gpu device, but often you don't have those.
> > DMA_HEAP is there, of course.
> 
> Why can't there be a variant that takes a proper fourcc format instead of
> an imprecise bpp value?

Backwards compatibility. We can add an IOCTL for YUV / etc. But
userspace must be able to continue allocating YUV buffers through
CREATE_DUMB.

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

-- 
With best wishes
Dmitry

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Daniel Stone]

On Thu, 16 Jan 2025 at 10:35, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
> On Thu, Jan 16, 2025 at 11:17:50AM +0100, Geert Uytterhoeven wrote:
> > On Thu, Jan 16, 2025 at 11:03 AM Tomi Valkeinen
> > <tomi.valkeinen@ideasonboard.com> wrote:
> > > On the platforms I have been using (omap, tidss, xilinx, rcar) the dumb
> > > buffers are the only buffers you can get from the DRM driver. The dumb
> > > buffers have been used to allocate linear and multiplanar YUV buffers
> > > for a very long time on those platforms.
> > >
> > > I tried to look around, but I did not find any mentions that CREATE_DUMB
> > > should only be used for RGB buffers. Is anyone outside the core
> > > developers even aware of it?
> > >
> > > If we don't use dumb buffers there, where do we get the buffers? Maybe
> > > from a v4l2 device or from a gpu device, but often you don't have those.
> > > DMA_HEAP is there, of course.
> >
> > Why can't there be a variant that takes a proper fourcc format instead of
> > an imprecise bpp value?
>
> Backwards compatibility. We can add an IOCTL for YUV / etc. But
> userspace must be able to continue allocating YUV buffers through
> CREATE_DUMB.

Right. If allocating YUYV dumb buffers works on AM68 today, then we
need to keep that working. But it doesn't mean we should go out of our
way to make CREATE_DUMB work for every YUV format on every device.

Currently, drivers are free to implement their own ioctls for anything
specific they have. But like Laurent said, standardising on heaps and
how to communicate requirements to userspace wrt heap selection / size
/ alignment / etc is imo a better path forward for something generic.

Cheers,
Daniel

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Sui Jingfeng]

Hi,

On 2025/1/16 18:35, Dmitry Baryshkov wrote:
> On Thu, Jan 16, 2025 at 11:17:50AM +0100, Geert Uytterhoeven wrote:
>> On Thu, Jan 16, 2025 at 11:03 AM Tomi Valkeinen
>> <tomi.valkeinen@ideasonboard.com> wrote:
>>> On 16/01/2025 10:09, Thomas Zimmermann wrote:
>>>> Am 15.01.25 um 15:20 schrieb Tomi Valkeinen:
>>>> [...]
>>>>> My point is that we have the current UAPI, and we have userspace using
>>>>> it, but we don't have clear rules what the ioctl does with specific
>>>>> parameters, and we don't document how it has to be used.
>>>>>
>>>>> Perhaps the situation is bad, and all we can really say is that
>>>>> CREATE_DUMB only works for use with simple RGB formats, and the
>>>>> behavior for all other formats is platform specific. But I think even
>>>>> that would be valuable in the UAPI docs.
>>>> To be honest, I would not want to specify behavior for anything but the
>>>> linear RGB formats. If anything, I'd take Daniel's reply mail for
>>>> documentation as-is. Anyone stretching the UAPI beyond RGB is on their own.
>>>>
>>>>> Thinking about this, I wonder if this change is good for omapdrm or
>>>>> xilinx (probably other platforms too that support non-simple non-RGB
>>>>> formats via dumb buffers): without this patch, in both drivers, the
>>>>> pitch calculations just take the bpp as bit-per-pixels, align it up,
>>>>> and that's it.
>>>>>
>>>>> With this patch we end up using drm_driver_color_mode_format(), and
>>>>> aligning buffers according to RGB formats figured out via heuristics.
>>>>> It does happen to work, for the formats I tested, but it sounds like
>>>>> something that might easily not work, as it's doing adjustments based
>>>>> on wrong format.
>>>>>
>>>>> Should we have another version of drm_mode_size_dumb() which just
>>>>> calculates using the bpp, without the drm_driver_color_mode_format()
>>>>> path? Or does the drm_driver_color_mode_format() path provide some
>>>>> value for the drivers that do not currently do anything similar?
>>>> With the RGB-only rule, using drm_driver_color_mode_format() makes
>>>> sense. It aligns dumb buffers and video=, provides error checking, and
>>>> overall harmonizes code. The fallback is only required because of the
>>>> existing odd cases that already bend the UAPI's rules.
>>> I have to disagree here.
>>>
>>> On the platforms I have been using (omap, tidss, xilinx, rcar) the dumb
>>> buffers are the only buffers you can get from the DRM driver. The dumb
>>> buffers have been used to allocate linear and multiplanar YUV buffers
>>> for a very long time on those platforms.
>>>
>>> I tried to look around, but I did not find any mentions that CREATE_DUMB
>>> should only be used for RGB buffers. Is anyone outside the core
>>> developers even aware of it?
>>>
>>> If we don't use dumb buffers there, where do we get the buffers? Maybe
>>> from a v4l2 device or from a gpu device, but often you don't have those.
>>> DMA_HEAP is there, of course.
>> Why can't there be a variant that takes a proper fourcc format instead of
>> an imprecise bpp value?
> Backwards compatibility. We can add an IOCTL for YUV / etc.

[...]

> But userspace must be able to continue allocating YUV buffers through
> CREATE_DUMB.

I think, allocating YUV buffers through CREATE_DUMB interface is just
an *abuse* and *misuse* of this API for now.

Take the NV12 format as an example, NV12 is YUV420 planar format, have
two planar: the Y-planar and the UV-planar. The Y-planar appear first
in memory as an array of unsigned char values. The Y-planar is followed
immediately by the UV-planar, which is also an array of unsigned char
values that contains packed U (Cb) and V (Cr) samples.

But the 'drm_mode_create_dumb' structure is only intend to provide
descriptions for *one* planar.

struct drm_mode_create_dumb {
     __u32 height;
     __u32 width;
     __u32 bpp;
     __u32 flags;
     __u32 handle;
     __u32 pitch;
     __u64 size;
};

An width x height NV12 image takes up width*height*(1 + 1/4 + 1/4) bytes.

So we can allocate an *equivalent* sized buffer to store the NV12 raw data.

Either 'width * (height * 3/2)' where each pixel take up 8 bits,
or just 'with * height' where each pixels take up 12 bits.

However, all those math are just equivalents description to the original
NV12 format, neither are concrete correct physical description.

Therefore, allocating YUV buffers through the dumb interface is just an
abuse for that API. We certainly can abuse more by allocating two dumb
buffers, one for Y-planer, another one for the UV-planer. But again,dumb buffers can be (and must be) used for *scanout* directly. What will yield if I commit the YUV buffers you allocated to the CRTC directly?

In other words, You can allocated buffers via the dumb APIs to store anything,
but the key point is that how can we interpret it.

As Daniel puts it, the semantics of that API is well defined for simple RGB
formats. Usages on non linear RGB dumb buffers are considered as undefined
behavior.

Peoples can still abusing it at the user-space though, but the kernel don't
have to guarantee that the user-space *must* to be able to continue doing
balabala..., That's it.


Best regards,
Sui

>> Gr{oetje,eeting}s,
>>
>>                          Geert
>>
>> -- 
>> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>>
>> In personal conversations with technical people, I call myself a hacker. But
>> when I'm talking to journalists I just say "programmer" or something like that.
>>                                  -- Linus Torvalds

-- 
Best regards,
Sui

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Tomi Valkeinen]

Hi,

On 19/01/2025 13:29, Sui Jingfeng wrote:
> Hi,
> 
> On 2025/1/16 18:35, Dmitry Baryshkov wrote:
>> On Thu, Jan 16, 2025 at 11:17:50AM +0100, Geert Uytterhoeven wrote:
>>> On Thu, Jan 16, 2025 at 11:03 AM Tomi Valkeinen
>>> <tomi.valkeinen@ideasonboard.com> wrote:
>>>> On 16/01/2025 10:09, Thomas Zimmermann wrote:
>>>>> Am 15.01.25 um 15:20 schrieb Tomi Valkeinen:
>>>>> [...]
>>>>>> My point is that we have the current UAPI, and we have userspace 
>>>>>> using
>>>>>> it, but we don't have clear rules what the ioctl does with specific
>>>>>> parameters, and we don't document how it has to be used.
>>>>>>
>>>>>> Perhaps the situation is bad, and all we can really say is that
>>>>>> CREATE_DUMB only works for use with simple RGB formats, and the
>>>>>> behavior for all other formats is platform specific. But I think even
>>>>>> that would be valuable in the UAPI docs.
>>>>> To be honest, I would not want to specify behavior for anything but 
>>>>> the
>>>>> linear RGB formats. If anything, I'd take Daniel's reply mail for
>>>>> documentation as-is. Anyone stretching the UAPI beyond RGB is on 
>>>>> their own.
>>>>>
>>>>>> Thinking about this, I wonder if this change is good for omapdrm or
>>>>>> xilinx (probably other platforms too that support non-simple non-RGB
>>>>>> formats via dumb buffers): without this patch, in both drivers, the
>>>>>> pitch calculations just take the bpp as bit-per-pixels, align it up,
>>>>>> and that's it.
>>>>>>
>>>>>> With this patch we end up using drm_driver_color_mode_format(), and
>>>>>> aligning buffers according to RGB formats figured out via heuristics.
>>>>>> It does happen to work, for the formats I tested, but it sounds like
>>>>>> something that might easily not work, as it's doing adjustments based
>>>>>> on wrong format.
>>>>>>
>>>>>> Should we have another version of drm_mode_size_dumb() which just
>>>>>> calculates using the bpp, without the drm_driver_color_mode_format()
>>>>>> path? Or does the drm_driver_color_mode_format() path provide some
>>>>>> value for the drivers that do not currently do anything similar?
>>>>> With the RGB-only rule, using drm_driver_color_mode_format() makes
>>>>> sense. It aligns dumb buffers and video=, provides error checking, and
>>>>> overall harmonizes code. The fallback is only required because of the
>>>>> existing odd cases that already bend the UAPI's rules.
>>>> I have to disagree here.
>>>>
>>>> On the platforms I have been using (omap, tidss, xilinx, rcar) the dumb
>>>> buffers are the only buffers you can get from the DRM driver. The dumb
>>>> buffers have been used to allocate linear and multiplanar YUV buffers
>>>> for a very long time on those platforms.
>>>>
>>>> I tried to look around, but I did not find any mentions that 
>>>> CREATE_DUMB
>>>> should only be used for RGB buffers. Is anyone outside the core
>>>> developers even aware of it?
>>>>
>>>> If we don't use dumb buffers there, where do we get the buffers? Maybe
>>>> from a v4l2 device or from a gpu device, but often you don't have 
>>>> those.
>>>> DMA_HEAP is there, of course.
>>> Why can't there be a variant that takes a proper fourcc format 
>>> instead of
>>> an imprecise bpp value?
>> Backwards compatibility. We can add an IOCTL for YUV / etc.
> 
> [...]
> 
>> But userspace must be able to continue allocating YUV buffers through
>> CREATE_DUMB.
> 
> I think, allocating YUV buffers through CREATE_DUMB interface is just
> an *abuse* and *misuse* of this API for now.
> 
> Take the NV12 format as an example, NV12 is YUV420 planar format, have
> two planar: the Y-planar and the UV-planar. The Y-planar appear first
> in memory as an array of unsigned char values. The Y-planar is followed
> immediately by the UV-planar, which is also an array of unsigned char
> values that contains packed U (Cb) and V (Cr) samples.
> 
> But the 'drm_mode_create_dumb' structure is only intend to provide
> descriptions for *one* planar.
> 
> struct drm_mode_create_dumb {
>      __u32 height;
>      __u32 width;
>      __u32 bpp;
>      __u32 flags;
>      __u32 handle;
>      __u32 pitch;
>      __u64 size;
> };
> 
> An width x height NV12 image takes up width*height*(1 + 1/4 + 1/4) bytes.
> 
> So we can allocate an *equivalent* sized buffer to store the NV12 raw data.
> 
> Either 'width * (height * 3/2)' where each pixel take up 8 bits,
> or just 'with * height' where each pixels take up 12 bits.
> 
> However, all those math are just equivalents description to the original
> NV12 format, neither are concrete correct physical description.

I don't see the problem. Allocating dumb buffers, if we don't have any 
heuristics related to RGB behind it, is essentially just allocating a 
specific amount of memory, defined by width, height and bitsperpixel.

If I want to create an NV12 framebuffer, I allocate two dumb buffers, 
one for Y and one for UV planes, and size them accordingly. And then 
create the DRM framebuffer with those.

> Therefore, allocating YUV buffers through the dumb interface is just an
> abuse for that API. We certainly can abuse more by allocating two dumb
> buffers, one for Y-planer, another one for the UV-planer. But again,dumb 
> buffers can be (and must be) used for *scanout* directly. What will 
> yield if I commit the YUV buffers you allocated to the CRTC directly?

You'll see it on the screen? I don't understand your point here...

> In other words, You can allocated buffers via the dumb APIs to store 
> anything,
> but the key point is that how can we interpret it.
> 
> As Daniel puts it, the semantics of that API is well defined for simple RGB
> formats. Usages on non linear RGB dumb buffers are considered as undefined
> behavior.
> 
> Peoples can still abusing it at the user-space though, but the kernel don't
> have to guarantee that the user-space *must* to be able to continue doing
> balabala..., That's it.

I have hard time understanding the "abuse" argument. But in any case, 
the API has been working like this for who knows how long, and used 
widely (afaik). The question is, do we break it or not. Granted, this 
series doesn't break it as such, but it adds heuristics that wasn't 
there before, and it could affect the behavior. If we still want to do 
that, I want to understand what is the benefit, because there's a 
potential to cause regressions.

  Tomi

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Sui Jingfeng]

Hi,

On 2025/1/19 20:18, Tomi Valkeinen wrote:
> Hi,
>
> On 19/01/2025 13:29, Sui Jingfeng wrote:
>> Hi,
>>
>> On 2025/1/16 18:35, Dmitry Baryshkov wrote:
>>> On Thu, Jan 16, 2025 at 11:17:50AM +0100, Geert Uytterhoeven wrote:
>>>> On Thu, Jan 16, 2025 at 11:03 AM Tomi Valkeinen
>>>> <tomi.valkeinen@ideasonboard.com> wrote:
>>>>> On 16/01/2025 10:09, Thomas Zimmermann wrote:
>>>>>> Am 15.01.25 um 15:20 schrieb Tomi Valkeinen:
>>>>>> [...]
>>>>>>> My point is that we have the current UAPI, and we have userspace 
>>>>>>> using
>>>>>>> it, but we don't have clear rules what the ioctl does with specific
>>>>>>> parameters, and we don't document how it has to be used.
>>>>>>>
>>>>>>> Perhaps the situation is bad, and all we can really say is that
>>>>>>> CREATE_DUMB only works for use with simple RGB formats, and the
>>>>>>> behavior for all other formats is platform specific. But I think 
>>>>>>> even
>>>>>>> that would be valuable in the UAPI docs.
>>>>>> To be honest, I would not want to specify behavior for anything 
>>>>>> but the
>>>>>> linear RGB formats. If anything, I'd take Daniel's reply mail for
>>>>>> documentation as-is. Anyone stretching the UAPI beyond RGB is on 
>>>>>> their own.
>>>>>>
>>>>>>> Thinking about this, I wonder if this change is good for omapdrm or
>>>>>>> xilinx (probably other platforms too that support non-simple 
>>>>>>> non-RGB
>>>>>>> formats via dumb buffers): without this patch, in both drivers, the
>>>>>>> pitch calculations just take the bpp as bit-per-pixels, align it 
>>>>>>> up,
>>>>>>> and that's it.
>>>>>>>
>>>>>>> With this patch we end up using drm_driver_color_mode_format(), and
>>>>>>> aligning buffers according to RGB formats figured out via 
>>>>>>> heuristics.
>>>>>>> It does happen to work, for the formats I tested, but it sounds 
>>>>>>> like
>>>>>>> something that might easily not work, as it's doing adjustments 
>>>>>>> based
>>>>>>> on wrong format.
>>>>>>>
>>>>>>> Should we have another version of drm_mode_size_dumb() which just
>>>>>>> calculates using the bpp, without the 
>>>>>>> drm_driver_color_mode_format()
>>>>>>> path? Or does the drm_driver_color_mode_format() path provide some
>>>>>>> value for the drivers that do not currently do anything similar?
>>>>>> With the RGB-only rule, using drm_driver_color_mode_format() makes
>>>>>> sense. It aligns dumb buffers and video=, provides error 
>>>>>> checking, and
>>>>>> overall harmonizes code. The fallback is only required because of 
>>>>>> the
>>>>>> existing odd cases that already bend the UAPI's rules.
>>>>> I have to disagree here.
>>>>>
>>>>> On the platforms I have been using (omap, tidss, xilinx, rcar) the 
>>>>> dumb
>>>>> buffers are the only buffers you can get from the DRM driver. The 
>>>>> dumb
>>>>> buffers have been used to allocate linear and multiplanar YUV buffers
>>>>> for a very long time on those platforms.
>>>>>
>>>>> I tried to look around, but I did not find any mentions that 
>>>>> CREATE_DUMB
>>>>> should only be used for RGB buffers. Is anyone outside the core
>>>>> developers even aware of it?
>>>>>
>>>>> If we don't use dumb buffers there, where do we get the buffers? 
>>>>> Maybe
>>>>> from a v4l2 device or from a gpu device, but often you don't have 
>>>>> those.
>>>>> DMA_HEAP is there, of course.
>>>> Why can't there be a variant that takes a proper fourcc format 
>>>> instead of
>>>> an imprecise bpp value?
>>> Backwards compatibility. We can add an IOCTL for YUV / etc.
>>
>> [...]
>>
>>> But userspace must be able to continue allocating YUV buffers through
>>> CREATE_DUMB.
>>
>> I think, allocating YUV buffers through CREATE_DUMB interface is just
>> an *abuse* and *misuse* of this API for now.
>>
>> Take the NV12 format as an example, NV12 is YUV420 planar format, have
>> two planar: the Y-planar and the UV-planar. The Y-planar appear first
>> in memory as an array of unsigned char values. The Y-planar is followed
>> immediately by the UV-planar, which is also an array of unsigned char
>> values that contains packed U (Cb) and V (Cr) samples.
>>
>> But the 'drm_mode_create_dumb' structure is only intend to provide
>> descriptions for *one* planar.
>>
>> struct drm_mode_create_dumb {
>>      __u32 height;
>>      __u32 width;
>>      __u32 bpp;
>>      __u32 flags;
>>      __u32 handle;
>>      __u32 pitch;
>>      __u64 size;
>> };
>>
>> An width x height NV12 image takes up width*height*(1 + 1/4 + 1/4) 
>> bytes.
>>
>> So we can allocate an *equivalent* sized buffer to store the NV12 raw 
>> data.
>>
>> Either 'width * (height * 3/2)' where each pixel take up 8 bits,
>> or just 'with * height' where each pixels take up 12 bits.
>>
>> However, all those math are just equivalents description to the original
>> NV12 format, neither are concrete correct physical description.
>
> I don't see the problem. Allocating dumb buffers, if we don't have any 
> heuristics related to RGB behind it, is essentially just allocating a 
> specific amount of memory, defined by width, height and bitsperpixel.
>
I think, the problem will be that the 'width', 'height' and 'bpp'
are originally used to describe one plane. Those three parameters
has perfectly defined physical semantics.

But with multi planar formats, take NV12 image as an example,
for a 2×2 square of pixels, there are 4 Y samples but only 1 U
sample and 1 V sample. This format requires 4x8+1x8+1x8=48 bits
to store the 2x2 square.

So its depth is 12 bits per pixel (48 / (2 * 2)).

so my problem is that the mentioned 12bpp in this example only
make sense in mathematics, it doesn't has a good physical
interpret. Do you agree with me on this technique point?
     

> If I want to create an NV12 framebuffer, I allocate two dumb buffers, 
> one for Y and one for UV planes, and size them accordingly. And then 
> create the DRM framebuffer with those.
>
Then how you fill the value of the 'width', 'height' and 'bpp' of each dumb buffers?

Why not allocate storage for the whole on one shoot?

The modetest in libdrm can be an good example, send it[1] to you as an reference.

[1] https://gitlab.freedesktop.org/mesa/drm/-/blob/main/tests/modetest/buffers.c?ref_type=heads#L114

-- 

Best regards,
Sui

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Tomi Valkeinen]

On 19/01/2025 16:59, Sui Jingfeng wrote:

>>>> But userspace must be able to continue allocating YUV buffers through
>>>> CREATE_DUMB.
>>>
>>> I think, allocating YUV buffers through CREATE_DUMB interface is just
>>> an *abuse* and *misuse* of this API for now.
>>>
>>> Take the NV12 format as an example, NV12 is YUV420 planar format, have
>>> two planar: the Y-planar and the UV-planar. The Y-planar appear first
>>> in memory as an array of unsigned char values. The Y-planar is followed
>>> immediately by the UV-planar, which is also an array of unsigned char
>>> values that contains packed U (Cb) and V (Cr) samples.
>>>
>>> But the 'drm_mode_create_dumb' structure is only intend to provide
>>> descriptions for *one* planar.
>>>
>>> struct drm_mode_create_dumb {
>>>      __u32 height;
>>>      __u32 width;
>>>      __u32 bpp;
>>>      __u32 flags;
>>>      __u32 handle;
>>>      __u32 pitch;
>>>      __u64 size;
>>> };
>>>
>>> An width x height NV12 image takes up width*height*(1 + 1/4 + 1/4) 
>>> bytes.
>>>
>>> So we can allocate an *equivalent* sized buffer to store the NV12 raw 
>>> data.
>>>
>>> Either 'width * (height * 3/2)' where each pixel take up 8 bits,
>>> or just 'with * height' where each pixels take up 12 bits.
>>>
>>> However, all those math are just equivalents description to the original
>>> NV12 format, neither are concrete correct physical description.
>>
>> I don't see the problem. Allocating dumb buffers, if we don't have any 
>> heuristics related to RGB behind it, is essentially just allocating a 
>> specific amount of memory, defined by width, height and bitsperpixel.
>>
> I think, the problem will be that the 'width', 'height' and 'bpp'
> are originally used to describe one plane. Those three parameters
> has perfectly defined physical semantics.
> 
> But with multi planar formats, take NV12 image as an example,
> for a 2×2 square of pixels, there are 4 Y samples but only 1 U
> sample and 1 V sample. This format requires 4x8+1x8+1x8=48 bits
> to store the 2x2 square.
> 
> So its depth is 12 bits per pixel (48 / (2 * 2)).
> 
> so my problem is that the mentioned 12bpp in this example only
> make sense in mathematics, it doesn't has a good physical
> interpret. Do you agree with me on this technique point?
> 
>> If I want to create an NV12 framebuffer, I allocate two dumb buffers, 
>> one for Y and one for UV planes, and size them accordingly. And then 
>> create the DRM framebuffer with those.
>>
> Then how you fill the value of the 'width', 'height' and 'bpp' of each 
> dumb buffers?

For 640x480-NV12:
plane 0: width = 640, height = 480, bpp = 8
plane 1: width = 640 / 2, height = 480 / 2, bpp = 16

> Why not allocate storage for the whole on one shoot?

You can, if you adjust the parameters accordingly. However, if the 
strides of the planes are not equal, I guess it might cause problems on 
some platforms.

But I think it's usually simpler to allocate one buffer per plane, and 
perhaps even better as it doesn't require as large contiguous memory area.

> The modetest in libdrm can be an good example, send it[1] to you as an 
> reference.

Right, so modetest already does it successfully. So... What is the issue?

Everyone agrees that CREATE_DUMB is not the best ioctl to allocate 
buffers, and one can't consider it to work identically across the 
platforms. But it's what we have and what has been used for ages.

  Tomi

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Sui Jingfeng]

Hi,

On 2025/1/19 23:22, Tomi Valkeinen wrote:
> On 19/01/2025 16:59, Sui Jingfeng wrote:
>
>>>>> But userspace must be able to continue allocating YUV buffers through
>>>>> CREATE_DUMB.
>>>>
>>>> I think, allocating YUV buffers through CREATE_DUMB interface is just
>>>> an *abuse* and *misuse* of this API for now.
>>>>
>>>> Take the NV12 format as an example, NV12 is YUV420 planar format, have
>>>> two planar: the Y-planar and the UV-planar. The Y-planar appear first
>>>> in memory as an array of unsigned char values. The Y-planar is 
>>>> followed
>>>> immediately by the UV-planar, which is also an array of unsigned char
>>>> values that contains packed U (Cb) and V (Cr) samples.
>>>>
>>>> But the 'drm_mode_create_dumb' structure is only intend to provide
>>>> descriptions for *one* planar.
>>>>
>>>> struct drm_mode_create_dumb {
>>>>      __u32 height;
>>>>      __u32 width;
>>>>      __u32 bpp;
>>>>      __u32 flags;
>>>>      __u32 handle;
>>>>      __u32 pitch;
>>>>      __u64 size;
>>>> };
>>>>
>>>> An width x height NV12 image takes up width*height*(1 + 1/4 + 1/4) 
>>>> bytes.
>>>>
>>>> So we can allocate an *equivalent* sized buffer to store the NV12 
>>>> raw data.
>>>>
>>>> Either 'width * (height * 3/2)' where each pixel take up 8 bits,
>>>> or just 'with * height' where each pixels take up 12 bits.
>>>>
>>>> However, all those math are just equivalents description to the 
>>>> original
>>>> NV12 format, neither are concrete correct physical description.
>>>
>>> I don't see the problem. Allocating dumb buffers, if we don't have 
>>> any heuristics related to RGB behind it, is essentially just 
>>> allocating a specific amount of memory, defined by width, height and 
>>> bitsperpixel.
>>>
>> I think, the problem will be that the 'width', 'height' and 'bpp'
>> are originally used to describe one plane. Those three parameters
>> has perfectly defined physical semantics.
>>
>> But with multi planar formats, take NV12 image as an example,
>> for a 2×2 square of pixels, there are 4 Y samples but only 1 U
>> sample and 1 V sample. This format requires 4x8+1x8+1x8=48 bits
>> to store the 2x2 square.
>>
>> So its depth is 12 bits per pixel (48 / (2 * 2)).
>>
>> so my problem is that the mentioned 12bpp in this example only
>> make sense in mathematics, it doesn't has a good physical
>> interpret. Do you agree with me on this technique point?
>>
>>> If I want to create an NV12 framebuffer, I allocate two dumb 
>>> buffers, one for Y and one for UV planes, and size them accordingly. 
>>> And then create the DRM framebuffer with those.
>>>
>> Then how you fill the value of the 'width', 'height' and 'bpp' of 
>> each dumb buffers?
>
> For 640x480-NV12:
> plane 0: width = 640, height = 480, bpp = 8
> plane 1: width = 640 / 2, height = 480 / 2, bpp = 16
>
But i think this should be hardware dependent. The hardware I'm using
load NV12  raw data as a whole. I only need to feed gpuva of the backing
memory to the hardware register once.

Not familiar with your hardware, so I can't talk more on this software
design. Perhaps someone know more could have a comment on this.

>> Why not allocate storage for the whole on one shoot?
>
> You can, if you adjust the parameters accordingly. However, if the 
> strides of the planes are not equal, I guess it might cause problems 
> on some platforms.
>
> But I think it's usually simpler to allocate one buffer per plane, and 
> perhaps even better as it doesn't require as large contiguous memory 
> area.
>
>> The modetest in libdrm can be an good example, send it[1] to you as 
>> an reference.
>
> Right, so modetest already does it successfully. So... What is the issue?
>
But then, the problem will become that it override the 'height' parameter.
What's the physical interpretation of the 'height' parameter when creating
an NV12 image with the dump API then?

I guess, solving complex problems with simple APIs may see the limitation,
sooner or later. But I not very sure and might be wrong. So other peoples
can override me words.


> Everyone agrees that CREATE_DUMB is not the best ioctl to allocate 
> buffers, and one can't consider it to work identically across the 
> platforms. But it's what we have and what has been used for ages.
>
Yeah, your request are not unreasonable. It can be seen as a kind of rigid demand.
Since GEM DMA helpers doesn't export an more advanced interface to userspace so far.
As a result, drivers that employing GEM DMA has no other choice, but to abuse the
dumb buffer API to do allocation for the more complex format buffers.

The dumb buffer API doesn't support to specify buffer format, tile status and
placement etc. The more advance drivers has been exposed the xxx_create_gem()
to user-space. It seems that a few more experienced programmers hint us to
create an new ioctl at above thread, so that we can keep employing simple API
to do simple things and to suit complex needs with the more advanced APIs.


>  Tomi
>
-- 
Best regards,
Sui

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Laurent Pinchart]

On Mon, Jan 20, 2025 at 12:26:30AM +0800, Sui Jingfeng wrote:
> On 2025/1/19 23:22, Tomi Valkeinen wrote:
> > On 19/01/2025 16:59, Sui Jingfeng wrote:
> >
> >>>>> But userspace must be able to continue allocating YUV buffers through
> >>>>> CREATE_DUMB.
> >>>>
> >>>> I think, allocating YUV buffers through CREATE_DUMB interface is just
> >>>> an *abuse* and *misuse* of this API for now.
> >>>>
> >>>> Take the NV12 format as an example, NV12 is YUV420 planar format, have
> >>>> two planar: the Y-planar and the UV-planar. The Y-planar appear first
> >>>> in memory as an array of unsigned char values. The Y-planar is followed
> >>>> immediately by the UV-planar, which is also an array of unsigned char
> >>>> values that contains packed U (Cb) and V (Cr) samples.
> >>>>
> >>>> But the 'drm_mode_create_dumb' structure is only intend to provide
> >>>> descriptions for *one* planar.
> >>>>
> >>>> struct drm_mode_create_dumb {
> >>>>      __u32 height;
> >>>>      __u32 width;
> >>>>      __u32 bpp;
> >>>>      __u32 flags;
> >>>>      __u32 handle;
> >>>>      __u32 pitch;
> >>>>      __u64 size;
> >>>> };
> >>>>
> >>>> An width x height NV12 image takes up width*height*(1 + 1/4 + 1/4) 
> >>>> bytes.
> >>>>
> >>>> So we can allocate an *equivalent* sized buffer to store the NV12 
> >>>> raw data.
> >>>>
> >>>> Either 'width * (height * 3/2)' where each pixel take up 8 bits,
> >>>> or just 'with * height' where each pixels take up 12 bits.
> >>>>
> >>>> However, all those math are just equivalents description to the original
> >>>> NV12 format, neither are concrete correct physical description.
> >>>
> >>> I don't see the problem. Allocating dumb buffers, if we don't have 
> >>> any heuristics related to RGB behind it, is essentially just 
> >>> allocating a specific amount of memory, defined by width, height and 
> >>> bitsperpixel.
> >>>
> >> I think, the problem will be that the 'width', 'height' and 'bpp'
> >> are originally used to describe one plane. Those three parameters
> >> has perfectly defined physical semantics.
> >>
> >> But with multi planar formats, take NV12 image as an example,
> >> for a 2×2 square of pixels, there are 4 Y samples but only 1 U
> >> sample and 1 V sample. This format requires 4x8+1x8+1x8=48 bits
> >> to store the 2x2 square.
> >>
> >> So its depth is 12 bits per pixel (48 / (2 * 2)).
> >>
> >> so my problem is that the mentioned 12bpp in this example only
> >> make sense in mathematics, it doesn't has a good physical
> >> interpret. Do you agree with me on this technique point?
> >>
> >>> If I want to create an NV12 framebuffer, I allocate two dumb 
> >>> buffers, one for Y and one for UV planes, and size them accordingly. 
> >>> And then create the DRM framebuffer with those.
> >>>
> >> Then how you fill the value of the 'width', 'height' and 'bpp' of 
> >> each dumb buffers?
> >
> > For 640x480-NV12:
> > plane 0: width = 640, height = 480, bpp = 8
> > plane 1: width = 640 / 2, height = 480 / 2, bpp = 16
>
> But i think this should be hardware dependent. The hardware I'm using
> load NV12  raw data as a whole. I only need to feed gpuva of the backing
> memory to the hardware register once.
> 
> Not familiar with your hardware, so I can't talk more on this software
> design. Perhaps someone know more could have a comment on this.

Layout of planes in memory is just one hardware constraint, the same way
we have constraints on alignment and strides. Some devices require the
planes to be contiguous (likely with some alignment constraints), some
can work with planes being in discontiguous pieces of memory, and even
require them to be discontiguous and located in separate DRAM banks.

> >> Why not allocate storage for the whole on one shoot?
> >
> > You can, if you adjust the parameters accordingly. However, if the 
> > strides of the planes are not equal, I guess it might cause problems 
> > on some platforms.
> >
> > But I think it's usually simpler to allocate one buffer per plane, and 
> > perhaps even better as it doesn't require as large contiguous memory 
> > area.
> >
> >> The modetest in libdrm can be an good example, send it[1] to you as 
> >> an reference.
> >
> > Right, so modetest already does it successfully. So... What is the issue?
>
> But then, the problem will become that it override the 'height' parameter.
> What's the physical interpretation of the 'height' parameter when creating
> an NV12 image with the dump API then?

I wouldn't be too concerned about physical interpretations. Yes, the
height, width and bpp parameters were likely designed with RGB formats
in mind. Yes, using DUMB_CREATE for YUV formats is probably something
that the original authors didn't envision. And yes, from that point of
view, it could be seen by the original authors as an abuse of the API.
But I don't think that's a problem as such.

An API is just an API. True, it would be nicer if the usage of the ioctl
parameters was more intuitive for YUV formats, but I believe we could
still standardize how the existing parameters map to linear scanout YUV
formats without causing the world to end. As has been said before, lots
of drivers are using DUMB_CREATE for this purpose, and we can't change
that.

This doesn't mean we shouldn't work on improving memory allocation, but
I see that as a separate issue.

> I guess, solving complex problems with simple APIs may see the limitation,
> sooner or later. But I not very sure and might be wrong. So other peoples
> can override me words.
> 
> > Everyone agrees that CREATE_DUMB is not the best ioctl to allocate 
> > buffers, and one can't consider it to work identically across the 
> > platforms. But it's what we have and what has been used for ages.
>
> Yeah, your request are not unreasonable. It can be seen as a kind of rigid demand.
> Since GEM DMA helpers doesn't export an more advanced interface to userspace so far.
> As a result, drivers that employing GEM DMA has no other choice, but to abuse the
> dumb buffer API to do allocation for the more complex format buffers.
> 
> The dumb buffer API doesn't support to specify buffer format, tile status and
> placement etc. The more advance drivers has been exposed the xxx_create_gem()
> to user-space. It seems that a few more experienced programmers hint us to
> create an new ioctl at above thread, so that we can keep employing simple API
> to do simple things and to suit complex needs with the more advanced APIs.

I'd really like to explore adding new ioctls to exposure memory
allocation constraints, and allocating the memory itself from DMA heaps.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Sui Jingfeng]

Hi,

On 2025/1/20 04:14, Laurent Pinchart wrote:
> On Mon, Jan 20, 2025 at 12:26:30AM +0800, Sui Jingfeng wrote:
>> On 2025/1/19 23:22, Tomi Valkeinen wrote:
>>> On 19/01/2025 16:59, Sui Jingfeng wrote:
>>>
>>>>>>> But userspace must be able to continue allocating YUV buffers through
>>>>>>> CREATE_DUMB.
>>>>>> I think, allocating YUV buffers through CREATE_DUMB interface is just
>>>>>> an *abuse* and *misuse* of this API for now.
>>>>>>
>>>>>> Take the NV12 format as an example, NV12 is YUV420 planar format, have
>>>>>> two planar: the Y-planar and the UV-planar. The Y-planar appear first
>>>>>> in memory as an array of unsigned char values. The Y-planar is followed
>>>>>> immediately by the UV-planar, which is also an array of unsigned char
>>>>>> values that contains packed U (Cb) and V (Cr) samples.
>>>>>>
>>>>>> But the 'drm_mode_create_dumb' structure is only intend to provide
>>>>>> descriptions for *one* planar.
>>>>>>
>>>>>> struct drm_mode_create_dumb {
>>>>>>       __u32 height;
>>>>>>       __u32 width;
>>>>>>       __u32 bpp;
>>>>>>       __u32 flags;
>>>>>>       __u32 handle;
>>>>>>       __u32 pitch;
>>>>>>       __u64 size;
>>>>>> };
>>>>>>
>>>>>> An width x height NV12 image takes up width*height*(1 + 1/4 + 1/4)
>>>>>> bytes.
>>>>>>
>>>>>> So we can allocate an *equivalent* sized buffer to store the NV12
>>>>>> raw data.
>>>>>>
>>>>>> Either 'width * (height * 3/2)' where each pixel take up 8 bits,
>>>>>> or just 'with * height' where each pixels take up 12 bits.
>>>>>>
>>>>>> However, all those math are just equivalents description to the original
>>>>>> NV12 format, neither are concrete correct physical description.
>>>>> I don't see the problem. Allocating dumb buffers, if we don't have
>>>>> any heuristics related to RGB behind it, is essentially just
>>>>> allocating a specific amount of memory, defined by width, height and
>>>>> bitsperpixel.
>>>>>
>>>> I think, the problem will be that the 'width', 'height' and 'bpp'
>>>> are originally used to describe one plane. Those three parameters
>>>> has perfectly defined physical semantics.
>>>>
>>>> But with multi planar formats, take NV12 image as an example,
>>>> for a 2×2 square of pixels, there are 4 Y samples but only 1 U
>>>> sample and 1 V sample. This format requires 4x8+1x8+1x8=48 bits
>>>> to store the 2x2 square.
>>>>
>>>> So its depth is 12 bits per pixel (48 / (2 * 2)).
>>>>
>>>> so my problem is that the mentioned 12bpp in this example only
>>>> make sense in mathematics, it doesn't has a good physical
>>>> interpret. Do you agree with me on this technique point?
>>>>
>>>>> If I want to create an NV12 framebuffer, I allocate two dumb
>>>>> buffers, one for Y and one for UV planes, and size them accordingly.
>>>>> And then create the DRM framebuffer with those.
>>>>>
>>>> Then how you fill the value of the 'width', 'height' and 'bpp' of
>>>> each dumb buffers?
>>> For 640x480-NV12:
>>> plane 0: width = 640, height = 480, bpp = 8
>>> plane 1: width = 640 / 2, height = 480 / 2, bpp = 16
>> But i think this should be hardware dependent. The hardware I'm using
>> load NV12  raw data as a whole. I only need to feed gpuva of the backing
>> memory to the hardware register once.
>>
>> Not familiar with your hardware, so I can't talk more on this software
>> design. Perhaps someone know more could have a comment on this.
> Layout of planes in memory is just one hardware constraint, the same way
> we have constraints on alignment and strides. Some devices require the
> planes to be contiguous (likely with some alignment constraints), some
> can work with planes being in discontiguous pieces of memory, and even
> require them to be discontiguous and located in separate DRAM banks.

Right.


>>>> Why not allocate storage for the whole on one shoot?
>>> You can, if you adjust the parameters accordingly. However, if the
>>> strides of the planes are not equal, I guess it might cause problems
>>> on some platforms.
>>>
>>> But I think it's usually simpler to allocate one buffer per plane, and
>>> perhaps even better as it doesn't require as large contiguous memory
>>> area.
>>>
>>>> The modetest in libdrm can be an good example, send it[1] to you as
>>>> an reference.
>>> Right, so modetest already does it successfully. So... What is the issue?
>> But then, the problem will become that it override the 'height' parameter.
>> What's the physical interpretation of the 'height' parameter when creating
>> an NV12 image with the dump API then?
> I wouldn't be too concerned about physical interpretations. Yes, the
> height, width and bpp parameters were likely designed with RGB formats
> in mind. Yes, using DUMB_CREATE for YUV formats is probably something
> that the original authors didn't envision. And yes, from that point of
> view, it could be seen by the original authors as an abuse of the API.
> But I don't think that's a problem as such.

Sometimes there may have 2D GPU or Image Process Unit get involved.
Setting the dimension, the clip window and pitch etc parameters to the
hardware are needed.

The value of bpp affects pitch, bigger pitch may cause the hardware load
more bytes than it should on one line. While the 'height' may affect the
size of the clip window, depend on the calculation method.

But I have no strong opinion toward with this and agree with you in overall.

> An API is just an API. True, it would be nicer if the usage of the ioctl
> parameters was more intuitive for YUV formats, but I believe we could
> still standardize how the existing parameters map to linear scanout YUV
> formats without causing the world to end. As has been said before, lots
> of drivers are using DUMB_CREATE for this purpose, and we can't change
> that.
>
> This doesn't mean we shouldn't work on improving memory allocation, but
> I see that as a separate issue.
>
>> I guess, solving complex problems with simple APIs may see the limitation,
>> sooner or later. But I not very sure and might be wrong. So other peoples
>> can override me words.
>>
>>> Everyone agrees that CREATE_DUMB is not the best ioctl to allocate
>>> buffers, and one can't consider it to work identically across the
>>> platforms. But it's what we have and what has been used for ages.
>> Yeah, your request are not unreasonable. It can be seen as a kind of rigid demand.
>> Since GEM DMA helpers doesn't export an more advanced interface to userspace so far.
>> As a result, drivers that employing GEM DMA has no other choice, but to abuse the
>> dumb buffer API to do allocation for the more complex format buffers.
>>
>> The dumb buffer API doesn't support to specify buffer format, tile status and
>> placement etc. The more advance drivers has been exposed the xxx_create_gem()
>> to user-space. It seems that a few more experienced programmers hint us to
>> create an new ioctl at above thread, so that we can keep employing simple API
>> to do simple things and to suit complex needs with the more advanced APIs.
> I'd really like to explore adding new ioctls to exposure memory
> allocation constraints, and allocating the memory itself from DMA heaps.
>
-- 
Best regards,
Sui

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Sui Jingfeng]

Hi,

On 2025/1/16 18:17, Geert Uytterhoeven wrote:
> On Thu, Jan 16, 2025 at 11:03 AM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>> On 16/01/2025 10:09, Thomas Zimmermann wrote:
>>> Am 15.01.25 um 15:20 schrieb Tomi Valkeinen:
>>> [...]
>>>> My point is that we have the current UAPI, and we have userspace using
>>>> it, but we don't have clear rules what the ioctl does with specific
>>>> parameters, and we don't document how it has to be used.
>>>>
>>>> Perhaps the situation is bad, and all we can really say is that
>>>> CREATE_DUMB only works for use with simple RGB formats, and the
>>>> behavior for all other formats is platform specific. But I think even
>>>> that would be valuable in the UAPI docs.
>>> To be honest, I would not want to specify behavior for anything but the
>>> linear RGB formats. If anything, I'd take Daniel's reply mail for
>>> documentation as-is. Anyone stretching the UAPI beyond RGB is on their own.
>>>
>>>> Thinking about this, I wonder if this change is good for omapdrm or
>>>> xilinx (probably other platforms too that support non-simple non-RGB
>>>> formats via dumb buffers): without this patch, in both drivers, the
>>>> pitch calculations just take the bpp as bit-per-pixels, align it up,
>>>> and that's it.
>>>>
>>>> With this patch we end up using drm_driver_color_mode_format(), and
>>>> aligning buffers according to RGB formats figured out via heuristics.
>>>> It does happen to work, for the formats I tested, but it sounds like
>>>> something that might easily not work, as it's doing adjustments based
>>>> on wrong format.
>>>>
>>>> Should we have another version of drm_mode_size_dumb() which just
>>>> calculates using the bpp, without the drm_driver_color_mode_format()
>>>> path? Or does the drm_driver_color_mode_format() path provide some
>>>> value for the drivers that do not currently do anything similar?
>>> With the RGB-only rule, using drm_driver_color_mode_format() makes
>>> sense. It aligns dumb buffers and video=, provides error checking, and
>>> overall harmonizes code. The fallback is only required because of the
>>> existing odd cases that already bend the UAPI's rules.
>> I have to disagree here.
>>
>> On the platforms I have been using (omap, tidss, xilinx, rcar) the dumb
>> buffers are the only buffers you can get from the DRM driver. The dumb
>> buffers have been used to allocate linear and multiplanar YUV buffers
>> for a very long time on those platforms.
>>
>> I tried to look around, but I did not find any mentions that CREATE_DUMB
>> should only be used for RGB buffers. Is anyone outside the core
>> developers even aware of it?
>>
>> If we don't use dumb buffers there, where do we get the buffers? Maybe
>> from a v4l2 device or from a gpu device, but often you don't have those.
>> DMA_HEAP is there, of course.
> Why can't there be a variant that takes a proper fourcc format instead of
> an imprecise bpp value?

The 'flags' parameter of the 'struct drm_mode_create_dumb' doesn't gets
in used so far, I guess the situation will be much better if passing a
correct fourcc code from the user-space to kernel is allowed.


> Gr{oetje,eeting}s,
>
>                          Geert
>
-- 
Best regards,
Sui

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Hi


Am 16.01.25 um 11:03 schrieb Tomi Valkeinen:
[...]
> Aligning video= and dumb buffers almost sounds like going backwards. 
> video= parameter is bad,

Who told you that? Video= is still the way to specify an initial display 
mode to the kernel and it will remain so.

Of course, it is better to auto-detect settings, but that's for a 
different use case.

Best regards
Thomas

>
> Harmonizing code is fine, but I think that can be done with a function 
> that only does the fallback-case.
>
> So... I can only speak for the platforms I'm using and maintaining, 
> but I'd rather keep the old behavior for CREATE_DUMB that we've had 
> for ages.
>
>  Tomi
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Tomi Valkeinen]

Hi,

On 20/01/2025 09:49, Thomas Zimmermann wrote:
> Hi
> 
> 
> Am 16.01.25 um 11:03 schrieb Tomi Valkeinen:
> [...]
>> Aligning video= and dumb buffers almost sounds like going backwards. 
>> video= parameter is bad,
> 
> Who told you that? Video= is still the way to specify an initial display 
> mode to the kernel and it will remain so.

You did =). "It aligns dumb buffers and video=". I understand the need 
for drm_driver_color_mode_format() for video=. But I think it's bad for 
CREATE_DUMB, at least for the platforms which have never aimed for 
"RGB-only".

So you're not in favor of a drm_mode_size_dumb() version that does not 
use drm_driver_color_mode_format(), for these platforms? I'm still at 
loss as to why we would want to change the behavior of CREATE_DUMB. I 
see no upside, but I see the chance of regressions.

  Tomi

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Hi


Am 20.01.25 um 09:51 schrieb Tomi Valkeinen:
> Hi,
>
> On 20/01/2025 09:49, Thomas Zimmermann wrote:
>> Hi
>>
>>
>> Am 16.01.25 um 11:03 schrieb Tomi Valkeinen:
>> [...]
>>> Aligning video= and dumb buffers almost sounds like going backwards. 
>>> video= parameter is bad,
>>
>> Who told you that? Video= is still the way to specify an initial 
>> display mode to the kernel and it will remain so.
>
> You did =). "It aligns dumb buffers and video=". 

I did not tell you "video= parameter is bad".

Best regards
Thomas

> I understand the need for drm_driver_color_mode_format() for video=. 
> But I think it's bad for CREATE_DUMB, at least for the platforms which 
> have never aimed for "RGB-only".
>
> So you're not in favor of a drm_mode_size_dumb() version that does not 
> use drm_driver_color_mode_format(), for these platforms? I'm still at 
> loss as to why we would want to change the behavior of CREATE_DUMB. I 
> see no upside, but I see the chance of regressions.
>
>  Tomi
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Thomas Zimmermann]

Hi


Am 16.01.25 um 11:03 schrieb Tomi Valkeinen:
[...]
>
> Harmonizing code is fine, but I think that can be done with a function 
> that only does the fallback-case.
>
> So... I can only speak for the platforms I'm using and maintaining, 
> but I'd rather keep the old behavior for CREATE_DUMB that we've had 
> for ages.

And we're not going to change that. I'll also include documentation of 
the intended behavior and semantics in the series' next update.

Whatever else is being discussed here, such as new ioctls, is a topic 
for a different series.

Best regards
Thomas

>
>  Tomi
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)