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

[Marek Olšák]

On Tue, Jan 14, 2025 at 12:58 PM Daniel Stone <daniel at fooishbar.org> wrote:

> Hi,
>
> On Tue, 14 Jan 2025 at 09:38, Marek Olšák <maraeo at gmail.com> wrote:
> > I would keep the existing modifier interfaces, API extensions, and
> expectations the same as today for simplicity.
>
> Well yes, not just for simplicity, but because everything stops
> working if you don't.
>
> > The new linear modifier definition (proposal) will have these fields:
> >    5 bits for log2 pitch alignment in bytes
> >    5 bits for log2 height alignment in rows
> >    5 bits for log2 offset alignment in bytes
> >    5 bits for log2 minimum pitch in bytes
> >    5 bits for log2 minimum (2D) image size in bytes
> >
> > The pitch and the image size in bytes are no longer arbitrary values.
> They are fixed values computed from {width, height, bpp, modifier} as
> follows:
> >    aligned_width = align(width * bpp / 8, 1 << log2_pitch_alignment);
> >    aligned_height = align(height, 1 << log2_height_alignment);
> >    pitch = max(1 << log2_minimum_pitch, aligned_width);
> >    image_size = max(1 << log2_minimum_image_size, pitch *
> aligned_height);
> >
> >
> > The modifier defines the layout exactly and non-ambiguously.
> Overaligning the pitch or height is not supported. Only the offset
> alignment has some freedom regarding placement. Drivers can expose whatever
> they want within that definition, even exposing only 1 linear modifier is
> OK. Then, you can look at modifiers of other drivers if you want to find
> commonalities.
>
> I don't see how this squares with the first statement.
>
> AMD hardware is the only hardware I know of which doesn't support
> overaligning. Say (not hypothetically) we have a GPU and a display
> controller which have a minimum pitch alignment of 32 bytes, no
> minimum height alignment, minimum 32-byte offset alignment, minimum
> pitch of 32 bytes, and minimum image size of 32 bytes.
>
> To be maximally compatible, we'd have to expose 28 (pitch align) * 32
> (height align) * 28 (offset align) * 28 (min pitch) * 28 (min size) ==
> 19668992 individual modifiers when queried, which is 150MB per format
> just to store the list of modifiers.
>

Maximum compatibility is not required nor expected.

In your case, only 1 linear modifier would be added for that driver, which
is:
log2 pitch alignment = 5
log2 height alignment = 0
log2 offset alignment = 5
log2 minimum pitch = 5
log2 minimum image size = 5

Then if, and only if, compatibility with other devices is desired, the
driver developer could look at drivers of those other devices and determine
which other linear modifiers to add. Ideally it would be just 1, so there
would be a total of 2.


>
> > DRM_FORMAT_MOD_LINEAR needs to go because it prevents apps from
> detecting whether 2 devices have 0 compatible memory layouts, which is a
> useful thing to know.
>
> I get the point, but again, we have the exact same problem today with
> placement, i.e. some devices require buffers to be in or not be in
> VRAM or GTT or sysram for some uses, and some devices require physical
> contiguity. Solving that problem would require an additional 4 bits,
> which brings us to 2.3GB of modifiers per format with the current
> scheme. Not super viable.
>

Userspace doesn't determine placement. The kernel memory management can
move buffers between heaps to accommodate sharing between devices as
needed. This is a problem in which userspace has no say.

Marek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20250114/7638d6e3/attachment.htm>