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

["Andy Yan" <andyshrk@163.com>]

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)