On Thu, Dec 19, 2024 at 5:32 AM Brian Starkey <brian.starkey@xxxxxxx> wrote:
On Wed, Dec 18, 2024 at 09:53:56PM +0000, Marek Olšák wrote:
> On Wed, Dec 18, 2024 at 5:32 AM Brian Starkey <brian.starkey@xxxxxxx> wrote:
>
> > On Wed, Dec 18, 2024 at 11:24:58AM +0000, Simona Vetter wrote:
> > >
> > > For that reason I think linear modifiers with explicit pitch/size
> > > alignment constraints is a sound concept and fits into how modifiers work
> > > overall.
> > > -Sima
> >
> > Could we make it (more) clear that pitch alignment is a "special"
> > constraint (in that it's really a description of the buffer layout),
> > and that constraints in-general shouldn't be exposed via modifiers?
> >
>
> Modifiers uniquely identify image layouts. That's why they exist and it's
> their only purpose.
Well you've quoted me saying "it's really a description of the buffer
layout", but actually I'm still unconvinced that pitch alignment is a
layout description rather than a constraint on an allocation.
To me, the layout is described by the "pitch" field of the framebuffer
object (and yes, modifiers are not only used for DRM framebuffers, but
every API which passes around linear surfaces has a pitch/stride
parameter of some sort).
The pitch doesn't always describe the layout. In practice, the pitch has no effect on any tiled layouts (on AMD), and it also has no effect on linear layouts if the pitch must be equal to a specifically rounded up width. In that case, the only function of the pitch is to reject importing a DMABUF if it's incorrect with respect to the width. In other cases, the pitch is a parameter of the modifier (i.e. the pitch augments the layout, so the layout is described by {modifier, width, height, bpp, pitch} instead of just {modifier, width, height, bpp}).
>
> It doesn't matter how many modifiers we have. No app should ever parse the
> modifier bits. All apps must treat modifiers as opaque numbers. Likewise,
> documentation of all modifiers in drm_fourcc.h is only meant for driver
> developers. No developers of apps should ever use the documentation. There
> can be a million modifiers and a million different devices, and the whole
> system of modifiers would fall apart if every app developer had to learn
> all of them.
My concern isn't with app developers, my concern is with drivers and
their authors needing to expose ever larger and more complex sets of
modifiers.
There _is_ a problem with having a million modifiers. The opaque set
intersection means that all authors from all vendors need to expose
the correct sets. The harder that is to do, the less likely things are
to work.
No, exposing the correct set is never required. You only expose your set, and then also expose those modifiers where you need interop. Interop between every pair of devices is generally unsupported (since LINEAR between devices is practically unsupported).
Look at GENERIC_16_16_TILE. We spotted that our layout was the same as
something already defined under SAMSUNG. If there were a million
modifiers, we wouldn't be able to spot that commonality, and you'd end
up with disjoint sets even when you have layouts in common.
This is unrelated.
For this specific case of pitch alignment it seems like the consensus
is we should add a modifier, but I still strongly disagree that
modifiers are the right place in-general for trying to describe device
buffer usage constraints.
I'm worried that adding these alignment constraints without any
statement on future intention pushes us down the slippery slope, and
it's *very* slippery.
Up-thread you mentioned offset alignment. If we start putting that in
modifiers then we have:
* Pitch alignment
* Arbitrary, 1 byte
* At least 16 byte aligned, arbitrary padding (Arm needs this)
* Exactly the next 64 bytes (AMD?)
* Offset alignment
* Arbitrary, 1 byte
* You mentioned 4096 bytes (AMD?)
* Arm needs 16, 8, 4 or 2 bytes, depending on format. Oh and it's
different for the chroma plane of planar YUV too, so it's more
like 16, 8, 4, 2, 2Y_1CbCr
We would need a new modifier value for each *combination* of
constraints, so 3 (pitch) * 7 (offset) gives 21 new LINEAR modifiers
which need defining, and a device with no pitch/offset constraints
needs to expose *all* of them to make sure it can interop with an
Arm/AMD device.
No, it's not needed to expose all of them. Again, you just expose what you need to interop with.
We know that the LINEAR modifier doesn't work with 1B pitch and offset alignment pretty much everywhere. What are you going to do about it?
Perhaps the solution is what Intel has done to interop with AMD: Intel's image allocator was changed to align the linear pitch to 256B. We can demand that all drivers must align the pitch to 256B in their allocators too. If they don't want to do it, they will likely be forced to do it by their management, which is likely why Intel did it. Is that the future we want? It's already happening.
Minimum alignment requirements (for AMD):
* Offset: 256B
* Pitch: 128B or 256B (only minimum or any multiple - different chips have different limits)
* Slice size alignment: 256B
* Contiguous pages (not visible to uAPI since the kernel can reallocate to enforce this constraint when needed)
Marek
