| Message ID | CAAxE2A5BkF13bFt8_UnuiqPM8W-ZESgmKEjqqGfv=DGzSfJ7aQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/fourcc: add LINEAR modifiers with an exact pitch alignment | expand |
Missed 2 lines from the diff: +#define DRM_FORMAT_MOD_LINEAR_PITCH_ALIGN_128B fourcc_mod_code(NONE, 2) +#define DRM_FORMAT_MOD_LINEAR_PITCH_ALIGN_256B fourcc_mod_code(NONE, 3) Marek On Sun, Dec 15, 2024 at 3:53 PM Marek Olšák <maraeo@gmail.com> wrote: > The comment explains the problem with DRM_FORMAT_MOD_LINEAR. > > Signed-off-by: Marek Olšák <marek.olsak@amd.com> > > 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) >
On 2024-12-15 21:53, Marek Olšák wrote: > The comment explains the problem with DRM_FORMAT_MOD_LINEAR. > > Signed-off-by: Marek Olšák <marek.olsak@amd.com <mailto:marek.olsak@amd.com>> > > 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) It's not clear what you mean by "requires that the pitch alignment is exactly the number in the definition", since a pitch which is aligned to 256 bytes is also aligned to 128 & 64 bytes. Do you mean the pitch must be exactly the width rounded up to the next / smallest possible multiple of the specified number of bytes?
Am Montag, dem 16.12.2024 um 10:27 +0100 schrieb Michel Dänzer: > On 2024-12-15 21:53, Marek Olšák wrote: > > The comment explains the problem with DRM_FORMAT_MOD_LINEAR. > > > > Signed-off-by: Marek Olšák <marek.olsak@amd.com <mailto:marek.olsak@amd.com>> > > > > 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) > > It's not clear what you mean by "requires that the pitch alignment is exactly > the number in the definition", since a pitch which is aligned to 256 bytes is > also aligned to 128 & 64 bytes. Do you mean the pitch must be exactly the width > rounded up to the next / smallest possible multiple of the specified number of bytes? I guess that's the intention here, as some AMD GPUs apparently have this limitation that they need an exact aligned pitch. If we open the can of worms to overhaul the linear modifier, I think it would make sense to also add modifiers for the more common restriction, where the pitch needs to be aligned to a specific granule, but the engine doesn't care if things get overaligned to a multiple of the granule. Having both sets would probably make it easier for the reader to see the difference to the exact pitch modifiers proposed in this patch. Regards, Lucas
On Mon, Dec 16, 2024 at 11:46:13AM +0100, Lucas Stach wrote: > Am Montag, dem 16.12.2024 um 10:27 +0100 schrieb Michel Dänzer: > > On 2024-12-15 21:53, Marek Olšák wrote: > > > The comment explains the problem with DRM_FORMAT_MOD_LINEAR. > > > > > > Signed-off-by: Marek Olšák <marek.olsak@amd.com <mailto:marek.olsak@amd.com>> > > > > > > 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) > > > > It's not clear what you mean by "requires that the pitch alignment is exactly > > the number in the definition", since a pitch which is aligned to 256 bytes is > > also aligned to 128 & 64 bytes. Do you mean the pitch must be exactly the width > > rounded up to the next / smallest possible multiple of the specified number of bytes? > > I guess that's the intention here, as some AMD GPUs apparently have > this limitation that they need an exact aligned pitch. > > If we open the can of worms to overhaul the linear modifier, I think it > would make sense to also add modifiers for the more common restriction, > where the pitch needs to be aligned to a specific granule, but the > engine doesn't care if things get overaligned to a multiple of the > granule. Having both sets would probably make it easier for the reader > to see the difference to the exact pitch modifiers proposed in this > patch. Yeah I think with linear modifiers that sepcificy alignment limitations we need to be _extremely_ precise in what exactly is required, and what not. There's all kinds of hilarious stuff that might be incompatible, and if we don't mind those we're right back to the "everyone agrees on what linear means" falacy. So if pitch must be a multiple of 64, but cannot be a multiple of 128 (because the hw does not cope with the padding, then that's important). Other things are alignment constraints on the starting point, and any padding you need to add at the bottom (yeah some hw overscans and gets pissed if there's not memory there). So I think it's best to go overboard here with verbosity. There's also really funny stuff like power-of-two alignment and things like that, but I guess we'll get there if that's ever needed. That's also why I think we don't need to add all possible linear modifiers from the start, unless there's maybe too much confusion with stuff like "exactly 64b aligned pitch" and "at least 64b aligned pitch, but you can add lots of padding if you feel like". -""ma > > Regards, > Lucas
On Mon, Dec 16, 2024 at 4:27 AM Michel Dänzer <michel.daenzer@mailbox.org> wrote: > On 2024-12-15 21:53, Marek Olšák wrote: > > The comment explains the problem with DRM_FORMAT_MOD_LINEAR. > > > > Signed-off-by: Marek Olšák <marek.olsak@amd.com <mailto: > marek.olsak@amd.com>> > > > > 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) > > It's not clear what you mean by "requires that the pitch alignment is > exactly the number in the definition", since a pitch which is aligned to > 256 bytes is also aligned to 128 & 64 bytes. Do you mean the pitch must be > exactly the width rounded up to the next / smallest possible multiple of > the specified number of bytes? > The pitch must be width*Bpp aligned to the number of bytes in the definition. Marek
On Mon, Dec 16, 2024 at 5:46 AM Lucas Stach <l.stach@pengutronix.de> wrote: > Am Montag, dem 16.12.2024 um 10:27 +0100 schrieb Michel Dänzer: > > On 2024-12-15 21:53, Marek Olšák wrote: > > > The comment explains the problem with DRM_FORMAT_MOD_LINEAR. > > > > > > Signed-off-by: Marek Olšák <marek.olsak@amd.com <mailto: > marek.olsak@amd.com>> > > > > > > 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) > > > > It's not clear what you mean by "requires that the pitch alignment is > exactly > > the number in the definition", since a pitch which is aligned to 256 > bytes is > > also aligned to 128 & 64 bytes. Do you mean the pitch must be exactly > the width > > rounded up to the next / smallest possible multiple of the specified > number of bytes? > > I guess that's the intention here, as some AMD GPUs apparently have > this limitation that they need an exact aligned pitch. > > If we open the can of worms to overhaul the linear modifier, I think it > would make sense to also add modifiers for the more common restriction, > where the pitch needs to be aligned to a specific granule, but the > engine doesn't care if things get overaligned to a multiple of the > granule. Having both sets would probably make it easier for the reader > to see the difference to the exact pitch modifiers proposed in this > patch. > That's a good point. It could be: - LINEAR_PITCH_ALIGN_EXACT_#B - LINEAR_PITCH_ALIGN_MULTIPLE_#B Drivers that expose the MULTIPLE ones should also expose the EXACT ones that are equivalent. Other drivers will only expose the EXACT ones but not the MULTIPLE ones. Marek
On Mon, Dec 16, 2024 at 9:53 AM Simona Vetter <simona.vetter@ffwll.ch> wrote: > On Mon, Dec 16, 2024 at 11:46:13AM +0100, Lucas Stach wrote: > > Am Montag, dem 16.12.2024 um 10:27 +0100 schrieb Michel Dänzer: > > > On 2024-12-15 21:53, Marek Olšák wrote: > > > > The comment explains the problem with DRM_FORMAT_MOD_LINEAR. > > > > > > > > Signed-off-by: Marek Olšák <marek.olsak@amd.com <mailto: > marek.olsak@amd.com>> > > > > > > > > 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) > > > > > > It's not clear what you mean by "requires that the pitch alignment is > exactly > > > the number in the definition", since a pitch which is aligned to 256 > bytes is > > > also aligned to 128 & 64 bytes. Do you mean the pitch must be exactly > the width > > > rounded up to the next / smallest possible multiple of the specified > number of bytes? > > > > I guess that's the intention here, as some AMD GPUs apparently have > > this limitation that they need an exact aligned pitch. > > > > If we open the can of worms to overhaul the linear modifier, I think it > > would make sense to also add modifiers for the more common restriction, > > where the pitch needs to be aligned to a specific granule, but the > > engine doesn't care if things get overaligned to a multiple of the > > granule. Having both sets would probably make it easier for the reader > > to see the difference to the exact pitch modifiers proposed in this > > patch. > > Yeah I think with linear modifiers that sepcificy alignment limitations we > need to be _extremely_ precise in what exactly is required, and what not. > There's all kinds of hilarious stuff that might be incompatible, and if we > don't mind those we're right back to the "everyone agrees on what linear > means" falacy. > > So if pitch must be a multiple of 64, but cannot be a multiple of 128 > (because the hw does not cope with the padding, then that's important). > Other things are alignment constraints on the starting point, and any > padding you need to add at the bottom (yeah some hw overscans and gets > pissed if there's not memory there). So I think it's best to go overboard > here with verbosity. > > There's also really funny stuff like power-of-two alignment and things > like that, but I guess we'll get there if that's ever needed. That's also > why I think we don't need to add all possible linear modifiers from the > start, unless there's maybe too much confusion with stuff like "exactly > 64b aligned pitch" and "at least 64b aligned pitch, but you can add lots > of padding if you feel like". > Would it be possible and acceptable to require that the offset alignment is always 4K and the slice padding is also always 4K to simplify those constraints? Marek
On 2024-12-16 22:29, Marek Olšák wrote: > On Mon, Dec 16, 2024 at 4:27 AM Michel Dänzer <michel.daenzer@mailbox.org <mailto:michel.daenzer@mailbox.org>> wrote: > > On 2024-12-15 21:53, Marek Olšák wrote: > > The comment explains the problem with DRM_FORMAT_MOD_LINEAR. > > > > Signed-off-by: Marek Olšák <marek.olsak@amd.com <mailto:marek.olsak@amd.com> <mailto:marek.olsak@amd.com <mailto:marek.olsak@amd.com>>> > > > > 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) > > It's not clear what you mean by "requires that the pitch alignment is exactly the number in the definition", since a pitch which is aligned to 256 bytes is also aligned to 128 & 64 bytes. Do you mean the pitch must be exactly the width rounded up to the next / smallest possible multiple of the specified number of bytes? > > > The pitch must be width*Bpp aligned to the number of bytes in the definition. The comment above the modifier define should spell that out unambiguously.
Hi, On Sun, Dec 15, 2024 at 03:53:14PM +0000, Marek Olšák wrote: > The comment explains the problem with DRM_FORMAT_MOD_LINEAR. > > Signed-off-by: Marek Olšák <marek.olsak@amd.com> > > 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
On 2024-12-16 22:54, Marek Olšák wrote: > On Mon, Dec 16, 2024 at 5:46 AM Lucas Stach <l.stach@pengutronix.de <mailto:l.stach@pengutronix.de>> wrote: > > Am Montag, dem 16.12.2024 um 10:27 +0100 schrieb Michel Dänzer: > > On 2024-12-15 21:53, Marek Olšák wrote: > > > The comment explains the problem with DRM_FORMAT_MOD_LINEAR. > > > > > > Signed-off-by: Marek Olšák <marek.olsak@amd.com <mailto:marek.olsak@amd.com> <mailto:marek.olsak@amd.com <mailto:marek.olsak@amd.com>>> > > > > > > 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) > > > > It's not clear what you mean by "requires that the pitch alignment is exactly > > the number in the definition", since a pitch which is aligned to 256 bytes is > > also aligned to 128 & 64 bytes. Do you mean the pitch must be exactly the width > > rounded up to the next / smallest possible multiple of the specified number of bytes? > > I guess that's the intention here, as some AMD GPUs apparently have > this limitation that they need an exact aligned pitch. > > If we open the can of worms to overhaul the linear modifier, I think it > would make sense to also add modifiers for the more common restriction, > where the pitch needs to be aligned to a specific granule, but the > engine doesn't care if things get overaligned to a multiple of the > granule. Having both sets would probably make it easier for the reader > to see the difference to the exact pitch modifiers proposed in this > patch. > > > That's a good point. > > It could be: > - LINEAR_PITCH_ALIGN_EXACT_#B > - LINEAR_PITCH_ALIGN_MULTIPLE_#B ALIGN seems fundamentally confusing to me, since any multiple of #B is "aligned to #B". Maybe something like: LINEAR_PITCH_MIN_MULTIPLE_#B LINEAR_PITCH_ANY_MULTIPLE_#B "minimal multiple of #B" vs "any multiple of #B"
On 2024-12-17 10:14, Brian Starkey wrote: > On Sun, Dec 15, 2024 at 03:53:14PM +0000, Marek Olšák wrote: >> The comment explains the problem with DRM_FORMAT_MOD_LINEAR. >> >> Signed-off-by: Marek Olšák <marek.olsak@amd.com> >> >> 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. > 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.
On Tue, Dec 17, 2024 at 11:13:05AM +0000, Michel Dänzer wrote: > On 2024-12-17 10:14, Brian Starkey wrote: > > On Sun, Dec 15, 2024 at 03:53:14PM +0000, Marek Olšák wrote: > >> The comment explains the problem with DRM_FORMAT_MOD_LINEAR. > >> > >> Signed-off-by: Marek Olšák <marek.olsak@amd.com> > >> > >> 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
On 2024-12-17 12:03, Brian Starkey wrote: > On Tue, Dec 17, 2024 at 11:13:05AM +0000, Michel Dänzer wrote: >> On 2024-12-17 10:14, Brian Starkey wrote: >> >>> 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. FWIW, KMS framebuffers aren't the only use case for sharing buffers between devices. >>> 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. I agree there's a slippery slope. That said, linear buffers are special in that they're the only possibility which can work for sharing buffers between devices in many cases, in particular when the devices are from different vendors or even different generations from the same vendor. So as long as device vendors don't get too creative with their linear pitch alignment restrictions, it still seems like this might be workable stop-gap solution for that specific purpose alone, until a better solution for handling constraints arrives. > P.S. "is the only modifier that has a chance of not working" is > fundamentally false. My reading of that part of the comment is that pitch alignment shouldn't be an issue with non-linear modifiers, since the constraints for pitch should be part of the modifier definition. Maybe that could be clarified in the comment.
On Mon, Dec 16, 2024 at 04:58:20PM -0500, Marek Olšák wrote: > On Mon, Dec 16, 2024 at 9:53 AM Simona Vetter <simona.vetter@ffwll.ch> > wrote: > > > On Mon, Dec 16, 2024 at 11:46:13AM +0100, Lucas Stach wrote: > > > Am Montag, dem 16.12.2024 um 10:27 +0100 schrieb Michel Dänzer: > > > > On 2024-12-15 21:53, Marek Olšák wrote: > > > > > The comment explains the problem with DRM_FORMAT_MOD_LINEAR. > > > > > > > > > > Signed-off-by: Marek Olšák <marek.olsak@amd.com <mailto: > > marek.olsak@amd.com>> > > > > > > > > > > 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) > > > > > > > > It's not clear what you mean by "requires that the pitch alignment is > > exactly > > > > the number in the definition", since a pitch which is aligned to 256 > > bytes is > > > > also aligned to 128 & 64 bytes. Do you mean the pitch must be exactly > > the width > > > > rounded up to the next / smallest possible multiple of the specified > > number of bytes? > > > > > > I guess that's the intention here, as some AMD GPUs apparently have > > > this limitation that they need an exact aligned pitch. > > > > > > If we open the can of worms to overhaul the linear modifier, I think it > > > would make sense to also add modifiers for the more common restriction, > > > where the pitch needs to be aligned to a specific granule, but the > > > engine doesn't care if things get overaligned to a multiple of the > > > granule. Having both sets would probably make it easier for the reader > > > to see the difference to the exact pitch modifiers proposed in this > > > patch. > > > > Yeah I think with linear modifiers that sepcificy alignment limitations we > > need to be _extremely_ precise in what exactly is required, and what not. > > There's all kinds of hilarious stuff that might be incompatible, and if we > > don't mind those we're right back to the "everyone agrees on what linear > > means" falacy. > > > > So if pitch must be a multiple of 64, but cannot be a multiple of 128 > > (because the hw does not cope with the padding, then that's important). > > Other things are alignment constraints on the starting point, and any > > padding you need to add at the bottom (yeah some hw overscans and gets > > pissed if there's not memory there). So I think it's best to go overboard > > here with verbosity. > > > > There's also really funny stuff like power-of-two alignment and things > > like that, but I guess we'll get there if that's ever needed. That's also > > why I think we don't need to add all possible linear modifiers from the > > start, unless there's maybe too much confusion with stuff like "exactly > > 64b aligned pitch" and "at least 64b aligned pitch, but you can add lots > > of padding if you feel like". > > > > Would it be possible and acceptable to require that the offset alignment is > always 4K and the slice padding is also always 4K to simplify those > constraints? For modifiers in general my take is to try to as closely and exhaustively as possible document the actual hw requirements, and not try to be clever. I fear otherwise we'll end up in a case of "works for my case" and then someone uses modifiers in a novel way and it again falls apart. But also worst case we'll just deprecate underdefinied modifiers and roll out new ones (like here), if we've missed a corner case. It happens. If that means we'll end up with modifiers that are strict subsets you can just add more modifiers to drivers to make things work. Or if there's awkward overlaps, we can make a new linear modifier with the common constraints and roll them to all drivers that want to interop. So shouldn't be an issue if we encounter interop incompatibilites, I just feel that trying to worry about this preemptively could likely lead to more trouble and not actually solve any real ones that will pop up in the future. Because accurately predicting the future is just too hard. -Sima
On Wed, Dec 18, 2024 at 10:44:17AM +0100, Michel Dänzer wrote: > On 2024-12-17 12:03, Brian Starkey wrote: > > On Tue, Dec 17, 2024 at 11:13:05AM +0000, Michel Dänzer wrote: > >> On 2024-12-17 10:14, Brian Starkey wrote: > >> > >>> 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. > > FWIW, KMS framebuffers aren't the only use case for sharing buffers > between devices. > > > >>> 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. > > I agree there's a slippery slope. > > That said, linear buffers are special in that they're the only > possibility which can work for sharing buffers between devices in many > cases, in particular when the devices are from different vendors or even > different generations from the same vendor. > > So as long as device vendors don't get too creative with their linear > pitch alignment restrictions, it still seems like this might be workable > stop-gap solution for that specific purpose alone, until a better > solution for handling constraints arrives. > > > > P.S. "is the only modifier that has a chance of not working" is > > fundamentally false. > > My reading of that part of the comment is that pitch alignment shouldn't > be an issue with non-linear modifiers, since the constraints for pitch > should be part of the modifier definition. Maybe that could be clarified > in the comment. Yeah with all other modifiers pitch alignment or other alignment/sizing requirements are generally implied. Mostly by stuff like tile size, but there's others where the hw requirement flat out is that the buffer must have a power-of-two stride (and maybe we should document these when they pop up, but the only one I know of are the legacy i915 modifiers, which are kinda busted anyway for interop). 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 > > > -- > Earthling Michel Dänzer \ GNOME / Xwayland / Mesa developer > https://redhat.com \ Libre software enthusiast
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? Cheers, -Brian
On Wed, Dec 18, 2024 at 5:32 AM Brian Starkey <brian.starkey@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. 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. 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. Marek
On Wed, 18 Dec 2024 at 10:32, Brian Starkey <brian.starkey@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. > > 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? It's still worryingly common to see requirements for contiguous allocation, if for no other reason than we'll all be stuck with Freescale/NXP i.MX6 for a long time to come. Would that be in scope for expressing constraints via modifiers as well, and if so, should we be trying to use feature bits to express this? How this would be used in practice is also way too underdocumented. We need to document that exact-round-up 64b is more restrictive than any-multiple-of 64b is more restrictive than 'classic' linear. We need to document what people should advertise - if we were starting from scratch, the clear answer would be that anything which doesn't care should advertise all three, anything advertising any-multiple-of should also advertise exact-round-up, etc. But we're not starting from scratch, and since linear is 'special', userspace already has explicit knowledge of it. So AMD is going to have to advertise LINEAR forever, because media frameworks know about DRM_FORMAT_MOD_LINEAR and pass that around explicitly when they know that the buffer is linear. That and not breaking older userspace running in containers or as part of a bisect or whatever. There's also the question of what e.g. gbm_bo_get_modifier() should return. Again, if we were starting from scratch, most restrictive would make sense. But we're not, so I think it has to return LINEAR for maximum compatibility (because modifiers can't be morphed into other ones for fun), which further cements that we're not removing LINEAR. And how should allocators determine what to go for? Given that, I think the only sensible semantics are, when only LINEAR has been passed, to pick the most restrictive set possible; when LINEAR variants have been passed as well as LINEAR, to act as if LINEAR were not passed at all. Cheers, Daniel
On Thu, 19 Dec 2024 at 02:54, Marek Olšák <maraeo@gmail.com> wrote: > On Wed, Dec 18, 2024 at 5:32 AM Brian Starkey <brian.starkey@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. >> >> 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. > > 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. I'm afraid linear _is_ special. And we've never had a delineation between 'applications' and 'clients' for uAPI purposes; I mean, if it's OK for Mesa and AMDVLK and ROCm to know exactly how to deal with AMD tiling modes for HIC, why is it not OK for apps to know that themselves too? Mostly because it's immaterial to the kernel: if it breaks one, it's going to break the other too. > 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. Yes, but it's _not_ a misuse of the API for the app to query supported modifiers, see that LINEAR is supported, know that its input is linear (because it created it, or because the subsystem it gets it from only supports linear layouts, or because they have another flag for linear with a non-modifier API, or), and use LINEAR with that. That's exactly what happens for anyone who wants to do interop with e.g. V4L2. > 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. You're extrapolating way too far. Linear works totally fine for everyone except AMD. The special case is that it doesn't work on AMD because AMD imposes really weird constraints. LINEAR cannot be removed any time soon/ever. You'd have to change all the clients who clearly and correctly use it without breaking the rules today, wait for them to phase out, wait to make sure that no-one would have them in an old container image or want to bisect into them, and only then kill it. Cheers, Daniel
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@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
On 2024-12-19 10:02, Daniel Stone wrote: > > How this would be used in practice is also way too underdocumented. We > need to document that exact-round-up 64b is more restrictive than > any-multiple-of 64b is more restrictive than 'classic' linear. We need > to document what people should advertise - if we were starting from > scratch, the clear answer would be that anything which doesn't care > should advertise all three, anything advertising any-multiple-of > should also advertise exact-round-up, etc. > > But we're not starting from scratch, and since linear is 'special', > userspace already has explicit knowledge of it. So AMD is going to > have to advertise LINEAR forever, because media frameworks know about > DRM_FORMAT_MOD_LINEAR and pass that around explicitly when they know > that the buffer is linear. That and not breaking older userspace > running in containers or as part of a bisect or whatever. > > There's also the question of what e.g. gbm_bo_get_modifier() should > return. Again, if we were starting from scratch, most restrictive > would make sense. But we're not, so I think it has to return LINEAR > for maximum compatibility (because modifiers can't be morphed into > other ones for fun), which further cements that we're not removing > LINEAR. > > And how should allocators determine what to go for? Given that, I > think the only sensible semantics are, when only LINEAR has been > passed, to pick the most restrictive set possible; when LINEAR > variants have been passed as well as LINEAR, to act as if LINEAR were > not passed at all. These are all very good points, which call for stricter-than-usual application of the "new UAPI requires corresponding user-space patches" rule: * Patches adding support for the new modifiers in Mesa (and kernel) drivers for at least two separate vendors * Patches adding support in at least one non-Mesa user-space component, e.g. a Wayland compositor which has code using the existing linear modifier (e.g. mutter) Ideally also describe a specific multi-vendor scenario which can't work with the existing linear modifier, and validate that it works with the new ones.
On Thu, Dec 19, 2024 at 09:02:27AM +0000, Daniel Stone wrote: > On Wed, 18 Dec 2024 at 10:32, Brian Starkey <brian.starkey@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. > > > > 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? > > It's still worryingly common to see requirements for contiguous > allocation, if for no other reason than we'll all be stuck with > Freescale/NXP i.MX6 for a long time to come. Would that be in scope > for expressing constraints via modifiers as well, and if so, should we > be trying to use feature bits to express this? > > How this would be used in practice is also way too underdocumented. We > need to document that exact-round-up 64b is more restrictive than > any-multiple-of 64b is more restrictive than 'classic' linear. We need > to document what people should advertise - if we were starting from > scratch, the clear answer would be that anything which doesn't care > should advertise all three, anything advertising any-multiple-of > should also advertise exact-round-up, etc. > > But we're not starting from scratch, and since linear is 'special', > userspace already has explicit knowledge of it. So AMD is going to > have to advertise LINEAR forever, because media frameworks know about > DRM_FORMAT_MOD_LINEAR and pass that around explicitly when they know > that the buffer is linear. That and not breaking older userspace > running in containers or as part of a bisect or whatever. > > There's also the question of what e.g. gbm_bo_get_modifier() should > return. Again, if we were starting from scratch, most restrictive > would make sense. But we're not, so I think it has to return LINEAR > for maximum compatibility (because modifiers can't be morphed into > other ones for fun), which further cements that we're not removing > LINEAR. > > And how should allocators determine what to go for? Given that, I > think the only sensible semantics are, when only LINEAR has been > passed, to pick the most restrictive set possible; when LINEAR > variants have been passed as well as LINEAR, to act as if LINEAR were > not passed at all. Yeah I think this makes sense, and we'd need to add that to the kerneldoc about how drivers/apps/frameworks need to work with variants of LINEAR. Just deprecating LINEAR does indeed not work. The same way it was really hard slow crawl (and we're still not there everywhere, if you include stuff like bare metal Xorg) trying to retire the implied modifier. Maybe, in an extremely bright future were all relevant drivers advertise a full set of LINEAR variants, and all frameworks understand them, we'll get there. But if AMD is the one special case that really needs this I don't think it's realistic to plan for that, and what Daniel describe above looks like the future we're stuck to. -Sima
On Thu, Dec 19, 2024 at 5:32 AM Brian Starkey <brian.starkey@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@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
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 +0000, Marek Olšák wrote: > On Thu, Dec 19, 2024 at 5:32 AM Brian Starkey <brian.starkey@arm.com> wrote: > > > On Wed, Dec 18, 2024 at 09:53:56PM +0000, 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_XRGB8888, .width = 64, .height = 64, .pitch = 256, .modifier = DRM_FORMAT_MOD_LINEAR, } { .format = DRM_FORMAT_XRGB8888, .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 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. How does a driver developer know what they need to interop with? I want my display controller driver to work with any GPU. It needs to expose PITCH_ALIGN_16B (the actual HW capability), PITCH_ALIGN_256B (so it can interop with AMD) and any other values which are >16B and aligned to 16B for interop with any other producer. i.e. "all of them". That's manageable for PITCH_ALIGN. It's not manageable if we start adding other constraints to modifiers. > > 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? Have an allocator use some "reasonable" pitch alignment (I think we default to 64 bytes for RGB), and allocate well-aligned buffers. If "reasonable" is 256B, use that. Better is to have userspace allocator which knows the devices in the system, knows the buffer usage, and allocates accordingly. "How does it know?" --> The allocator just codes in what it needs to interop with, obviously ;-) I'd definitely rather bake that interop list in userspace than kernel drivers. I think it would be great to have a kernel interface to help allocators "know". I don't think `IN_FORMATS` should be that interface. Cheers, -Brian > > 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
> > * Modifiers must uniquely encode buffer layout. In other words, a buffer > must > * match only a single modifier. > That sentence is misleading and impossible to meet. Specifications are sometimes changed to reflect the overwhelming reality. Multiple modifiers can represent identical layouts - they already do between vendors, between generations of the same vendor, and accidentally even between chips of the same generation. Modifiers have already become 64-bit structures of bitfields with currently 2^16 possible modifiers for some vendors, and possibly exceeding 100k for all vendors combined. Encoding all linear constraints into the 64 bits is one option. It needs more thought, but encoding at least some constraints in the modifier is not totally off the table. The semi-functional LINEAR modifier needs to go. The idea of modifiers is that nobody should have to expose one that is unsupported to keep things working for a subset of apps. If the LINEAR modifier is a requirement everywhere because of apps, and even drivers that can't support it must expose it, that's a problem. It causes GBM/EGL to fail to import a DMABUF for a random reason and it can't be prevented without, say, looking at PCI IDs. If that happened for any other API, it would be considered unusable. We can either fix it (by replacing/deprecating/removing LINEAR) or abandon modifiers and replace them with something that works. Marek
On Thu, Dec 19, 2024 at 05:09:33PM +0100, Michel Dänzer wrote: > On 2024-12-19 10:02, Daniel Stone wrote: > > > > How this would be used in practice is also way too underdocumented. We > > need to document that exact-round-up 64b is more restrictive than > > any-multiple-of 64b is more restrictive than 'classic' linear. We need > > to document what people should advertise - if we were starting from > > scratch, the clear answer would be that anything which doesn't care > > should advertise all three, anything advertising any-multiple-of > > should also advertise exact-round-up, etc. > > > > But we're not starting from scratch, and since linear is 'special', > > userspace already has explicit knowledge of it. So AMD is going to > > have to advertise LINEAR forever, because media frameworks know about > > DRM_FORMAT_MOD_LINEAR and pass that around explicitly when they know > > that the buffer is linear. That and not breaking older userspace > > running in containers or as part of a bisect or whatever. > > > > There's also the question of what e.g. gbm_bo_get_modifier() should > > return. Again, if we were starting from scratch, most restrictive > > would make sense. But we're not, so I think it has to return LINEAR > > for maximum compatibility (because modifiers can't be morphed into > > other ones for fun), which further cements that we're not removing > > LINEAR. > > > > And how should allocators determine what to go for? Given that, I > > think the only sensible semantics are, when only LINEAR has been > > passed, to pick the most restrictive set possible; when LINEAR > > variants have been passed as well as LINEAR, to act as if LINEAR were > > not passed at all. > > These are all very good points, which call for stricter-than-usual > application of the "new UAPI requires corresponding user-space patches" > rule: > > * Patches adding support for the new modifiers in Mesa (and kernel) > drivers for at least two separate vendors I think this is too strict? At least I could come up with other scenarios where we'd need a new linear variant: - one driver, but two different devices that happen to have incompatible linear requirements which break and get fixed with the new linear mode. - one driver, one device, but non-driver userspace allocates the linear buffer somewhere else (e.g. from dma-buf heaps) and gets pitch constraints wrong > * Patches adding support in at least one non-Mesa user-space component, > e.g. a Wayland compositor which has code using the existing linear > modifier (e.g. mutter) This also feels a bit too strict, since I think what Daniel proposed is that drivers do the special LINEAR handling when there are variants present in the list of compatible modifiers for an alloation. Hence I don't think compositor patches are necessarily required, but we definitely need to test to make sure it actually works and there's not surprises. The exception is of course when non-mesa userspace allocates/sizes the buffer itself (maybe for an soc where the display is separate and the gpu has stricter stride constraints than the display). > Ideally also describe a specific multi-vendor scenario which can't work > with the existing linear modifier, and validate that it works with the > new ones. I think that's really the crucial part, because adding modifiers without an actual use-case that they fix is just asking for more future trouble I think. -Sima > > > -- > Earthling Michel Dänzer \ GNOME / Xwayland / Mesa developer > https://redhat.com \ Libre software enthusiast
On Fri, Dec 20, 2024 at 09:24:59AM -0500, Marek Olšák wrote: > > > > * Modifiers must uniquely encode buffer layout. In other words, a buffer > > must > > * match only a single modifier. > > > > That sentence is misleading and impossible to meet. Specifications are > sometimes changed to reflect the overwhelming reality. Multiple modifiers > can represent identical layouts - they already do between vendors, between > generations of the same vendor, and accidentally even between chips of the > same generation. Modifiers have already become 64-bit structures of > bitfields with currently 2^16 possible modifiers for some vendors, and > possibly exceeding 100k for all vendors combined. Encoding all linear > constraints into the 64 bits is one option. It needs more thought, but > encoding at least some constraints in the modifier is not totally off the > table. Yeah uniqueness is not required, we just try to avoid it since it causes some pain. Worst case all drivers that care about working sharing just need to make sure they advertise all overlapping variants they support. > The semi-functional LINEAR modifier needs to go. The idea of modifiers is > that nobody should have to expose one that is unsupported to keep things > working for a subset of apps. If the LINEAR modifier is a requirement > everywhere because of apps, and even drivers that can't support it must > expose it, that's a problem. It causes GBM/EGL to fail to import a DMABUF > for a random reason and it can't be prevented without, say, looking at PCI > IDs. If that happened for any other API, it would be considered unusable. > We can either fix it (by replacing/deprecating/removing LINEAR) or abandon > modifiers and replace them with something that works. I think Daniel's forward compatibility plan is more solid than trying to deprecate LINEAR itself, that seems like too much an uphill battle to me. -Sima
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)
The comment explains the problem with DRM_FORMAT_MOD_LINEAR. Signed-off-by: Marek Olšák <marek.olsak@amd.com>