Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>]

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. 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".
> 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