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

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

Hi,

On 15/01/2025 14:34, Thomas Zimmermann wrote:
> 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.

Hmm, well... If we had a 64-bit RGB format in the list of "legacy fb 
formats", I wouldn't have noticed anything. And if I would just use 32 
as the bpp, and adjust width accordingly, it would also have worked. So, 
I expect the fallback to be an exception,

And by working I mean that I can run my apps fine, but the internal 
operation would sure be odd: allocating any YUV buffer will cause 
drm_mode_size_dumb() to get an RGB format from 
drm_driver_color_mode_format(), and get a drm_format_info_min_pitch() 
for an RGB format.

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

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?

  Tomi