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

[Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>]

On 15/01/2025 15:45, Thomas Zimmermann wrote:
> Hi
>
>
> Am 15.01.25 um 14:33 schrieb Tomi Valkeinen:
> [...]
> > > 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.
> > I feel I still don't quite understand. Can't we define and document 
> > CREATE_DUMB like this:
> > If (bpp < 8 || is_power_of_two(bpp))
> >     bpp means bitsperpixel
> >     pitch is args->width * args->bpp / 8, aligned up to driver- 
> > specific-align
> > else
> >     bpp is a legacy parameter, and we deal with it case by case.
> >     list the cases and what they mean
> >
> > And describe that when allocating subsampled buffers, the caller must 
> > adjust the width and height accordingly. And that the bpp and width 
> > can also refer to pixel groups.
> > Or if the currently existing code prevents the above for 16 and 32 
> > bpps, how about defining that any non-RGB or not-simple buffer has to 
> > be allocated with bpp=8, and the userspace has to align the pitch 
> > correctly according to the format and platform's hw restrictions?
> What if a hardware requires certain per-format alignments? Or requires 
> certain alignments for each plane? Or only supports tile modes? Or has 
> strict limits on the maximum buffer size?
> It is not possible to encode all this in a simple 32-bit value. So user- 
> space code has to be aware of all this and tweak bpp-based allocation to 
> make it work. Obviously you can use the current UAPI for your use case. 
> It's just not optimal or future proof.
No disagreement there, we need CREATE_DUMB2.

My point is that we have the current UAPI, and we have userspace using 
it, but we don't have clear rules what the ioctl does with specific 
parameters, and we don't document how it has to be used.
Perhaps the situation is bad, and all we can really say is that 
CREATE_DUMB only works for use with simple RGB formats, and the behavior 
for all other formats is platform specific. But I think even that would 
be valuable in the UAPI docs.
Thinking about this, I wonder if this change is good for omapdrm or 
xilinx (probably other platforms too that support non-simple non-RGB 
formats via dumb buffers): without this patch, in both drivers, the 
pitch calculations just take the bpp as bit-per-pixels, align it up, and 
that's it.
With this patch we end up using drm_driver_color_mode_format(), and 
aligning buffers according to RGB formats figured out via heuristics. It 
does happen to work, for the formats I tested, but it sounds like 
something that might easily not work, as it's doing adjustments based on 
wrong format.
Should we have another version of drm_mode_size_dumb() which just 
calculates using the bpp, without the drm_driver_color_mode_format() 
path? Or does the drm_driver_color_mode_format() path provide some value 
for the drivers that do not currently do anything similar?
 Tomi