Nouveau - Feb 2025 - [PATCH v3 02/25] drm/dumb-buffers: Provide helper to set pitch and size

Hi

Am 20.02.25 um 10:18 schrieb Tomi Valkeinen:
[...]
>> + * Color modes of 10, 12, 15, 30 and 64 are only supported for use by
>> + * legacy user space. Please don't use them in new code. Other
modes
>> + * are not support.
>> + *
>> + * Do not attempt to allocate anything but linear framebuffer memory
>> + * with single-plane RGB data. Allocation of other framebuffer
>> + * layouts requires dedicated ioctls in the respective DRM driver.
>
> According to this, every driver that supports, say, NV12, should 
> implement their own custom ioctl to do the exact same thing? And, of 
> course, every userspace app that uses, say, NV12, should then add code 
> for all these platforms to call the custom ioctls?
Yes, that's exactly the current status.

There has been discussion about a new dumb-create ioctl that takes a DRM 
format as parameter. I'm all for it, but it's out of the scope for this 
series.
>
> As libdrm's modetest currently supports YUV formats with dumb buffers, 
> should we remove that code, as it's not correct and I'm sure people
> use libdrm code as a reference?
Of course not.
>
> Well, I'm not serious above, but I think all my points from the 
> earlier version are still valid. I don't like this. It changes the 
> parameters of the ioctl (bpp used to be bits-per-pixel, not it's 
> "color mode"), and the behavior of the ioctl, behavior that
we've had
> for a very long time, and we have no idea how many users there are 
> that will break (could be none, of course). And the documentation 
> changes make the current behavior and uses wrong or legacy.
Before I go into details about this statement, what use case exactly are 
you referring to when you say that behavior changes?

Best regards
Thomas
>
> Clearly we need something new and better for the buffer allocation, 
> but for the time being, I'd be more comfortable just keep the current 
> behavior, at least for all the drivers I use or maintain: omapdrm, 
> tidss, renesas, xlnx, tilcdc.
>
> ?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)

Hi,

On 20/02/2025 12:05, Thomas Zimmermann wrote:
> Hi
> 
> Am 20.02.25 um 10:18 schrieb Tomi Valkeinen:
> [...]
>>> + * Color modes of 10, 12, 15, 30 and 64 are only supported for use
by
>>> + * legacy user space. Please don't use them in new code. Other
modes
>>> + * are not support.
>>> + *
>>> + * Do not attempt to allocate anything but linear framebuffer
memory
>>> + * with single-plane RGB data. Allocation of other framebuffer
>>> + * layouts requires dedicated ioctls in the respective DRM driver.
>>
>> According to this, every driver that supports, say, NV12, should 
>> implement their own custom ioctl to do the exact same thing? And, of 
>> course, every userspace app that uses, say, NV12, should then add code 
>> for all these platforms to call the custom ioctls?
> 
> Yes, that's exactly the current status.
> 
> There has been discussion about a new dumb-create ioctl that takes a DRM 
> format as parameter. I'm all for it, but it's out of the scope for
this
> series.
> 
>>
>> As libdrm's modetest currently supports YUV formats with dumb
buffers,
>> should we remove that code, as it's not correct and I'm sure
people
>> use libdrm code as a reference?
> 
> Of course not.
> 
>>
>> Well, I'm not serious above, but I think all my points from the 
>> earlier version are still valid. I don't like this. It changes the 
>> parameters of the ioctl (bpp used to be bits-per-pixel, not it's 
>> "color mode"), and the behavior of the ioctl, behavior that
we've had
>> for a very long time, and we have no idea how many users there are 
>> that will break (could be none, of course). And the documentation 
>> changes make the current behavior and uses wrong or legacy.
> 
> Before I go into details about this statement, what use case exactly are 
> you referring to when you say that behavior changes?
For every dumb_buffer allocation with bpp that is not divisible by 8, 
the result is different, i.e. instead of DIV_ROUND_UP(width * bpp, 8), 
we now have width * DIV_ROUND_UP(bpp, 8). This, of course, depends on 
the driver implementation. Some already do the latter.

This change also first calls the drm_driver_color_mode_format(), which 
could change the behavior even more, but afaics at the moment does not. 
Although, maybe some platform does width * DIV_ROUND_UP(bpp, 8) even for 
bpp < 8, and then this series changes it for 1, 2 and 4 bpps (but not 
for 3, 5, 6, 7, if I'm not mistaken).

However, as the bpp is getting rounded up, this probably won't break any 
user. But it _is_ a change in the behavior of a uapi, and every time we 
change a uapi that's been out there for a long time, I'm getting 
slightly uncomfortable.

So, as a summary, I have a feeling that nothing will break, but I can't 
say for sure. And as I'm having trouble seeing the benefit of this 
change for the user, I get even more uncomfortable.

  Tomi