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
Thomas Zimmermann
2025-Jan-15 12:34 UTC
[PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
Hi Am 15.01.25 um 13:06 schrieb Tomi Valkeinen:> 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?Drivers always did that, but it does not work correctly for (bpp < 8). As we already have helpers to deal with bpp, it makes sense to use them.? This also aligns dumb buffers with the kernel's video= parameter, which as the same odd semantics. The fallback that uses bpp directly will hopefully be the exception.> >>> 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 > > But 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".Yeah, there are constrains in the scanline and buffer alignments and orientation. And if we say that bpp==12 means NV12, it will be a problem for all other cases where bpp==12 makes sense. Best regards Thomas> >> 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 >-- -- 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)
Apparently Analagous 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()