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

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

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@xxxxxxxxxxxxxxxxxxx
> > > > 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)