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

[Thomas Zimmermann <tzimmermann@suse.de>]

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)