Re: [PATCH] drm/fourcc: add LINEAR modifiers with an exact pitch alignment
On Tue, Dec 17, 2024 at 11:13:05AM +, Michel Dänzer wrote:
> On 2024-12-17 10:14, Brian Starkey wrote:
> > On Sun, Dec 15, 2024 at 03:53:14PM +, Marek Olšák wrote:
> >> The comment explains the problem with DRM_FORMAT_MOD_LINEAR.
> >>
> >> Signed-off-by: Marek Olšák
> >>
> >> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> >> index 78abd819fd62e..8ec4163429014 100644
> >> --- a/include/uapi/drm/drm_fourcc.h
> >> +++ b/include/uapi/drm/drm_fourcc.h
> >> @@ -484,9 +484,27 @@ extern "C" {
> >> * modifier (e.g. not setting DRM_MODE_FB_MODIFIERS in the DRM_ADDFB2
> >> ioctl),
> >> * which tells the driver to also take driver-internal information into
> >> account
> >> * and so might actually result in a tiled framebuffer.
> >> + *
> >> + * WARNING:
> >> + * There are drivers out there that expose DRM_FORMAT_MOD_LINEAR, but only
> >> + * support a certain pitch alignment and can't import images with this
> >> modifier
> >> + * if the pitch alignment isn't exactly the one supported. They can
> >> however
> >> + * allocate images with this modifier and other drivers can import them
> >> only
> >> + * if they support the same pitch alignment. Thus, DRM_FORMAT_MOD_LINEAR
> >> is
> >> + * fundamentically incompatible across devices and is the only modifier
> >> that
> >> + * has a chance of not working. The PITCH_ALIGN modifiers should be used
> >> + * instead.
> >> */
> >> #define DRM_FORMAT_MOD_LINEAR fourcc_mod_code(NONE, 0)
> >>
> >> +/* Linear layout modifiers with an explicit pitch alignment in bytes.
> >> + * Exposing this modifier requires that the pitch alignment is exactly
> >> + * the number in the definition.
> >> + */
> >> +#define DRM_FORMAT_MOD_LINEAR_PITCH_ALIGN_64B fourcc_mod_code(NONE, 1)
> >
> > Why do we want this to be a modifier? All (?) of the other modifiers
> > describe properties which the producer and consumer need to know in
> > order to correctly fill/interpret the data.
> >
> > Framebuffers already have a pitch property which tells the
> > producer/consumer how to do that for linear buffers.
>
> At this point, the entity which allocates a linear buffer on device
> A to be shared with another device B can't know the pitch
> restrictions of B. If it guesses incorrectly, accessing the buffer
> with B won't work, so any effort allocating the buffer and producing
> its contents will be wasted.
I do understand (and agree) the need for allocators to know about these
constraints.
>
>
> > Modifiers are meant to describe framebuffers, and this pitch alignment
> > requirement isn't really a framebuffer property - it's a device
> > constraint. It feels out of place to overload modifiers with it.
> >
> > I'm not saying we don't need a way to describe constraints to
> > allocators, but I question if modifiers the right mechanism to
> > communicate them?
> While I agree with your concern in general, AFAIK there's no other
> solution for this even on the horizon, after years of talking about
> it. The solution proposed here seems like an acceptable stop gap,
> assuming it won't result in a gazillion linear modifiers.
UAPI is baked forever, so it's worth being a little wary IMO.
This sets a precedent for describing constraints via modifiers. The
reason no other proposal is on the horizon is because describing the
plethora of constraints across devices is a hard problem; and the
answer so far has been "userspace needs to know" (à la Android's
gralloc).
If we start down the road of describing constraints with modifiers, I
fear we'll end up in a mess. The full enumeration of modifiers is
already horrendous for parameterized types, please let's not
combinatorially multiply those by constraints.
Just thinking about HW I'm familiar with...
FORMAT_MOD_AFBC_16x16_ROTATABLE_ONLY_IF_LT_2048 (x5-ish variants)
FORMAT_MOD_AFBC_16x16_ROTATABLE_ONLY_IF_LT_1088 (x5-ish variants)
FORMAT_MOD_AFBC_16x16_USABLE_ONLY_IF_1_OTHER_AFBC_LAYER
(x all AFBC modifiers, including multiply by the two ROTATABLE
constraints above)
FORMAT_MOD_LINEAR_YUV420_MAX_2048_WIDE
That last one also highlights another problem with using modifiers for
constraints. That YUV420 restriction is orthogonal to the compression
scheme. So we'd need a FORMAT_MOD_LINEAR_YUV420_MAX_2048_WIDE *and* a
FORMAT_MOD_AFBC_YUV420_MAX_2048_WIDE (multiplied by all the AFBC
variants), and any other compression scheme multiplied by all its
variants. Not very nice.
Cheers,
-Brian
P.S. "is the only modifier that has a chance of not working" is
fundamentally false. Things can not work for an infinite number of
reasons, that's why we have TEST_ONLY. Allocating with the correct
pitch alignment is not a guarantee that you can display your
framebuffer.
>
>
> --
> Earthling Michel Dänzer \GNOME / Xwayland / Mesa developer
> https://redhat.com \ Libre software enthusiast
Re: [PATCH] drm/fourcc: add LINEAR modifiers with an exact pitch alignment
Hi,
On Sun, Dec 15, 2024 at 03:53:14PM +, Marek Olšák wrote:
> The comment explains the problem with DRM_FORMAT_MOD_LINEAR.
>
> Signed-off-by: Marek Olšák
>
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 78abd819fd62e..8ec4163429014 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -484,9 +484,27 @@ extern "C" {
> * modifier (e.g. not setting DRM_MODE_FB_MODIFIERS in the DRM_ADDFB2
> ioctl),
> * which tells the driver to also take driver-internal information into
> account
> * and so might actually result in a tiled framebuffer.
> + *
> + * WARNING:
> + * There are drivers out there that expose DRM_FORMAT_MOD_LINEAR, but only
> + * support a certain pitch alignment and can't import images with this
> modifier
> + * if the pitch alignment isn't exactly the one supported. They can however
> + * allocate images with this modifier and other drivers can import them
> only
> + * if they support the same pitch alignment. Thus, DRM_FORMAT_MOD_LINEAR is
> + * fundamentically incompatible across devices and is the only modifier
> that
> + * has a chance of not working. The PITCH_ALIGN modifiers should be used
> + * instead.
> */
> #define DRM_FORMAT_MOD_LINEAR fourcc_mod_code(NONE, 0)
>
> +/* Linear layout modifiers with an explicit pitch alignment in bytes.
> + * Exposing this modifier requires that the pitch alignment is exactly
> + * the number in the definition.
> + */
> +#define DRM_FORMAT_MOD_LINEAR_PITCH_ALIGN_64B fourcc_mod_code(NONE, 1)
Why do we want this to be a modifier? All (?) of the other modifiers
describe properties which the producer and consumer need to know in
order to correctly fill/interpret the data.
Framebuffers already have a pitch property which tells the
producer/consumer how to do that for linear buffers.
Modifiers are meant to describe framebuffers, and this pitch alignment
requirement isn't really a framebuffer property - it's a device
constraint. It feels out of place to overload modifiers with it.
I'm not saying we don't need a way to describe constraints to
allocators, but I question if modifiers the right mechanism to
communicate them?
Cheers,
-Brian
Re: [PATCH] drm/fourcc: add LINEAR modifiers with an exact pitch alignment
On Wed, Dec 18, 2024 at 11:24:58AM +, 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? Cheers, -Brian
Re: [PATCH] drm/fourcc: add LINEAR modifiers with an exact pitch alignment
On Wed, Dec 18, 2024 at 09:53:56PM +, Marek Olšák wrote: > On Wed, Dec 18, 2024 at 5:32 AM Brian Starkey wrote: > > > On Wed, Dec 18, 2024 at 11:24:58AM +, 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 > alig
Re: [PATCH] drm/fourcc: add LINEAR modifiers with an exact pitch alignment
This is getting long, so tl;dr:
- Pitch alignment *by itself* is manageable.
- Adding constraints in modifiers quickly leads to combinatorial
explosion.
- drm_fourcc.h explicitly says "it's incorrect to encode pitch
alignment in a modifier", for all the reasons Daniel raised. That
needs addressing.
On Thu, Dec 19, 2024 at 07:33:07PM +, Marek Olšák wrote:
> On Thu, Dec 19, 2024 at 5:32 AM Brian Starkey wrote:
>
> > On Wed, Dec 18, 2024 at 09:53:56PM +, Marek Olšák wrote:
>
> 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}).
I'm only talking about LINEAR here.
The ALIGN modifier tells an allocator what values of pitch are
acceptable, but it doesn't add any information about the buffer layout
which isn't already communicated by {format, width, height, bpp,
pitch}.
The AMD driver doesn't need the ALIGN modifier to determine if a
buffer is valid, it can do it entirely based on {format, width,
height, bpp, pitch}.
These two buffers are identical, and a driver which accepts one should
accept both:
{
.format = DRM_FORMAT_XRGB,
.width = 64,
.height = 64,
.pitch = 256,
.modifier = DRM_FORMAT_MOD_LINEAR,
}
{
.format = DRM_FORMAT_XRGB,
.width = 64,
.height = 64,
.pitch = 256,
.modifier = DRM_FORMAT_MOD_LINEAR_PITCH_ALIGN_256B,
}
This new modifier is a clear and direct violation of the comment in
drm_fourcc.h:
* Modifiers must uniquely encode buffer layout. In other words, a buffer must
* match only a single modifier. A modifier must not be a subset of layouts of
* another modifier. For instance, it's incorrect to encode pitch alignment in
* a modifier: a buffer may match a 64-pixel aligned modifier and a 32-pixel
* aligned modifier. That said, modifiers can have implicit minimal
* requirements.
I assume the argument here is this doesn't apply in this case because
we're deprecating LINEAR / the current LINEAR definition is wrong; but
it smells bad - it implies that this isn't the right API.
>
> >
> > 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).
>
How do I know where I need interop?
>
> >
> > 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.
More modifiers makes maintenance of the list harder. That seems
entirely relevant in light of the combinatorial explosion I described
below.
>
> >
> > 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
