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

[Thomas Zimmermann <tzimmermann@suse.de>]

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)