Thomas Zimmermann
2025-Jan-15 11:37 UTC
[PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
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)
Tomi Valkeinen
2025-Jan-15 12:06 UTC
[PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
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. SomeBut 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
Reasonably Related Threads
- [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
- [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
- [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
- [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
- [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()