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

["Andy Yan" <andyshrk@xxxxxxx>]

Hi Thomas,

At 2025-01-09 22:56:56, "Thomas Zimmermann" <tzimmermann@xxxxxxx> 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@xxxxxxx>
>---
> 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@xxxxxxxxxxxxxxxxxxx
>http://lists.infradead.org/mailman/listinfo/linux-rockchip