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

[Marek Olšák <maraeo@xxxxxxxxx>]

On Tue, Jan 14, 2025 at 1:34 PM Faith Ekstrand <faith@xxxxxxxxxxxxx> wrote:

On January 14, 2025 03:39:45 Marek Olšák <maraeo@xxxxxxxxx> wrote:

I would keep the existing modifier interfaces, API extensions, and expectations the same as today for simplicity.

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

I'm not strictly opposed to adding a new modifier or two but this seems massively over-designed. First off, no one uses anything but simple 2D images for WSI and BOs are allocated in units of 4k pages so 2, 4, and 5 can go. If we assume pitch alignment and offset alignment are the same (and offset is almost always 0 anyway), 3 can go.

Even with that, I'm struggling to see how useful this is. My understanding is that you're trying to solve a problem where you need an exact 64-byte alignment for some AMD scanout stuff. That's not even possible to support on Nvidia (minimum alignment is 128B) so practically you're looking at one modifier that's shared between AMD and Intel. Why can't we just add an AMD modifier, make Intel support it, and move on? 

Otherwise we're massively exploding the modifier space for... Why? Intel will have to advertise basically all of them. Nvidia will advertise most of them. AMD will advertise something. And now apps have tens of thousands of modifiers to sort through when we could have just added one and solved the problem.

I don't think I'm being understood. See my reply to Daniel. There is no exploding of anything. It's the same thing we have today for vendor modifiers - lots of fields with lots of possible values, but only a few values are used.

It's most likely under-designed, but it exactly solves the problem. The minimum requirement for every modifier is that it must exactly identify a memory layout. Saying that we just need a pitch alignment of 256B (that's the AMD one) is not enough. Height alignment and image size alignment are required to make sure that the next plane doesn't start in the padding area because it can be overwritten randomly OR it can be read and cause a page fault if the full padding isn't allocated. Offset alignment is also required for multi plane images. If you want all allocators to allocate NV12 equally, the 5 fields are required.

Marek