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

[Brian Starkey]

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).

> 
> 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.

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.

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.

I'm certain that 3 * 7 is not the full gamut of pitch/offset
constraints across all devices.

Then we come up with one new constraint, let's take Daniel's example
of contiguous. So I need contiguous/non-contiguous versions of all 21+
LINEAR modifiers and I'm up to at-least 42 modifiers, just for
describing a tiny subset of device constraints on a single layout.

It's obvious that this doesn't scale.

> 
> The only thing applications are allowed to do is query modifier lists from
> all clients, compute their intersection, and pass it to one of the clients
> for allocation. All clients must allocate the exact same layout, otherwise
> the whole system of modifiers would fall apart. If the modifier dictates
> that the pitch and alignment are not variables, but fixed values derived
> from width/height/bpp, then that's what all clients must allocate.
> 
> If any app uses DRM_FORMAT_MOD_LINEAR directly instead of querying
> supported modifiers from clients, it's a misuse of the API.
> 
> DRM_FORMAT_MOD_LINEAR will be deprecated because it's the only modifier
> that is generally non-functional (it's only functional in special cases).
> After new linear modifiers land, drivers will stop
> supporting DRM_FORMAT_MOD_LINEAR if they can't support all pitches and
> alignments because we only want to have functional software.

As part of this change will you be adding some core code to
automatically add aligned versions of LINEAR to any devices which only
expose (unaligned) FORMAT_MOD_LINEAR?

I'm also curious to understand how deprecation works here. Will LINEAR
be removed from drivers which currently expose it but actually have
pitch alignment constraints? I think that risks userspace breakage.
Or userspace should start interpreting modifier lists so that it
can ignore LINEAR? Or something else?

Thanks,
-Brian

> 
> Marek