Make add_planar_framebuffer() take only one modifier

[linkmauve]

The kernel enforces all four modifiers to actually be the same, so requesting four of them is a legacy artifact by now. Let’s avoid encoding such legacy into our APIs even more.

Additionally, making None default to LINEAR doesn’t seem very valid, if there was a default to pick I think it would be INVALID, but it is actually much better to ask the user which modifier they actually want.

[linkmauve]

Somewhat unrelated, but why is buffer::PlanarBuffer a trait and not just a struct?

[Drakulix]

Somewhat unrelated, but why is buffer::PlanarBuffer a trait and not just a struct?

Because different types might contain drm-handles and accompanying information. E.g. dumb-buffer implements this trait in this library, but notably gbm-rs also contains an implementation of this trait for gbm BufferObjects.

[linkmauve]

Yes and no, reading through docs again, it looks like there is a way to specific no modifiers, which is not setting DRM_MODE_FB_MODIFIERS in flags.

The old code also got that wrong, but at least didn't force downstream to specify a non-sense modifier.

Note that not using modifiers (the old API) is the same as passing INVALID with the flag set.

So this should take an Option<DrmModifier> and force flags to contain DRM_MODE_FB_MODIFIERS, if that Option is Some.

So Some(INVALID) would be the same as None, that’s pretty much what I was suggesting.

An even better solution would be to turn flags into a proper enum (and pass a &[FramebufferFlags] slice to this function. It would just contain interlaced, so technically a bool would be enough, but that would mean new flags would force us to break the api in the future, while we could mark FramebufferFlags non_exhaustive today.

I’ll have a look at that, thanks for the suggestion.

[Drakulix]

Note that not using modifiers (the old API) is the same as passing INVALID with the flag set. So Some(INVALID) would be the same as None, that’s pretty much what I was suggesting.

Given that the docs explicitly mention, that at least LINEAR isn't the same as the flag missing (though that is expected and very reasonable), I don't have high hopes, that some driver might not have different paths for INVALID+flag or no-modifier-flag.

I also don't see any IGT test specifically testing this: https://drm.pages.freedesktop.org/igt-gpu-tools/igt-kms-tests.html So until proven otherwise, I would like the API to expose all argument combinations to the IOCTL, just to be safe.

I have seen enough weird bugs during compositor development, that I am not trusting this.

[linkmauve]

Kernel devs told me the kernel doesn’t accept INVALID there actually, unlike the Wayland protocol for the same thing (zwp_linux_dmabuf_v1), so I was actually wrong.

I was wondering though, why isn’t the modifier part of the PlanarBuffer actually? It is completely state that is part of the buffer, not of the call.

[Drakulix]

I was wondering though, why isn’t the modifier part of the PlanarBuffer actually? It is completely state that is part of the buffer, not of the call.

No good reason, it just has grown this way over time. Nothing about the PlanarBuffer requires the format to have a modifier, the original idea was just to model buffers with multiple planes.

That said, we could add a modifier-function just like format and make it return an Option. But then you can also question why Buffer doesn't include bpp, that is required when creating a DumbBuffer anyway and can also be queried from a gbm-BufferObject.

[linkmauve]

I’ve changed this implementation to respond to your comments. One design decision I made was to keep the DRM_MODE_FB_MODIFIERS flag there, and assert if it was missing, but it could be better to rely on Option instead, let me know if you prefer that.

I’m not (much) interested in non-planar buffers, as this is legacy API at this point.