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

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

On Wed, 15 Jan 2025 at 14:20, Tomi Valkeinen
<tomi.valkeinen at ideasonboard.com> wrote:
> 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.
Yeah, CREATE_DUMB only works for use with simple RGB formats in a
linear layout. Not monochrome or YUV or tiled or displayed rotated or
whatever.

If it happens to accidentally work for other uses, that's fine, but
it's not generically reliable for anything other than simple linear
RGB. It's intended to let you do splash screens, consoles, recovery
password entries, and software-rendered compositors if you really
want. Anything more than that isn't 'dumb'.

Cheers,
Daniel

Hi


Am 15.01.25 um 15:20 schrieb Tomi Valkeinen:
[...]
>
> 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.
To be honest, I would not want to specify behavior for anything but the 
linear RGB formats. If anything, I'd take Daniel's reply mail for 
documentation as-is. Anyone stretching the UAPI beyond RGB is on their own.
>
> 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?
With the RGB-only rule, using drm_driver_color_mode_format() makes 
sense. It aligns dumb buffers and video=, provides error checking, and 
overall harmonizes code. The fallback is only required because of the 
existing odd cases that already bend the UAPI's rules.

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)