[PATCH] drm/fourcc: add LINEAR modifiers with an exact pitch alignment

[Marek Olšák]

On Thu, Dec 19, 2024 at 5:32 AM Brian Starkey <brian.starkey at arm.com> 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 at arm.com>
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20241219/5ebce1c5/attachment-0001.htm>