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

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

[Marek Olšák]

[-- Attachment #1: Type: text/plain, Size: 1477 bytes --]

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)

[-- Attachment #2: Type: text/html, Size: 1634 bytes --]

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

[Marek Olšák]

[-- Attachment #1: Type: text/plain, Size: 1822 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 2274 bytes --]

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

[Joshua Ashton]

You should just re-send the whole patch, probably.

On 12/15/24 8:54 PM, Marek Olšák wrote:
> 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 
> <mailto:maraeo@gmail.com>> 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)
> 

- Joshie 🐸✨

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

[Marek Olšák]

[-- Attachment #1.1: Type: text/plain, Size: 2421 bytes --]

Only for you: Attached.

Marek

On Sun, Dec 15, 2024 at 6:37 PM Joshua Ashton <joshua@froggi.es> wrote:

> You should just re-send the whole patch, probably.
>
> On 12/15/24 8:54 PM, Marek Olšák wrote:
> > 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
> > <mailto:maraeo@gmail.com>> 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)
> >
>
> - Joshie 🐸✨
>
>

[-- Attachment #1.2: Type: text/html, Size: 3408 bytes --]

[-- Attachment #2: 0001-drm-fourcc-add-LINEAR-modifiers-with-an-exact-pitch-.patch --]
[-- Type: text/x-patch, Size: 2098 bytes --]

From 50e6aa4f5182da5ce247a95c9ac69eaeb349cb9e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Ol=C5=A1=C3=A1k?= <marek.olsak@amd.com>
Date: Sun, 15 Dec 2024 15:49:30 -0500
Subject: [PATCH] drm/fourcc: add LINEAR modifiers with an exact pitch
 alignment
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The comment explains the problem with DRM_FORMAT_MOD_LINEAR.

Signed-off-by: Marek Olšák <marek.olsak@amd.com>
---
 include/uapi/drm/drm_fourcc.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

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)
+#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)
+
 /*
  * Deprecated: use DRM_FORMAT_MOD_LINEAR instead
  *
-- 
2.43.0

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

[Joshua Ashton]

Not really for my benefit, more that it's the proper thing to do if you 
want anyone to apply your patch.

You should really just be using git send-email.

On 12/15/24 11:57 PM, Marek Olšák wrote:
> Only for you: Attached.
> 
> Marek
> 
> On Sun, Dec 15, 2024 at 6:37 PM Joshua Ashton <joshua@froggi.es 
> <mailto:joshua@froggi.es>> wrote:
> 
>     You should just re-send the whole patch, probably.
> 
>     On 12/15/24 8:54 PM, Marek Olšák wrote:
>      > 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
>     <mailto:maraeo@gmail.com>
>      > <mailto:maraeo@gmail.com <mailto:maraeo@gmail.com>>> 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)
>      >
> 
>     - Joshie 🐸✨
> 

- Joshie 🐸✨

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

[Marek Olšák]

[-- Attachment #1: Type: text/plain, Size: 3807 bytes --]

git send-email (or rather the way it sends email) has been banned by gmail
due to being considered unsecure. I don't plan to find a way to make it
work and I don't plan to use a different email provider. It doesn't matter
because I'll be the committer of this patch in our amd-staging-drm-next
branch.

Let's talk about the concept and brokenness of DRM_FORMAT_MOD_LINEAR, not
send-email.

Marek

On Sun, Dec 15, 2024 at 9:08 PM Joshua Ashton <joshua@froggi.es> wrote:

> Not really for my benefit, more that it's the proper thing to do if you
> want anyone to apply your patch.
>
> You should really just be using git send-email.
>
> On 12/15/24 11:57 PM, Marek Olšák wrote:
> > Only for you: Attached.
> >
> > Marek
> >
> > On Sun, Dec 15, 2024 at 6:37 PM Joshua Ashton <joshua@froggi.es
> > <mailto:joshua@froggi.es>> wrote:
> >
> >     You should just re-send the whole patch, probably.
> >
> >     On 12/15/24 8:54 PM, Marek Olšák wrote:
> >      > 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
> >     <mailto:maraeo@gmail.com>
> >      > <mailto:maraeo@gmail.com <mailto:maraeo@gmail.com>>> 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)
> >      >
> >
> >     - Joshie 🐸✨
> >
>
> - Joshie 🐸✨
>
>

[-- Attachment #2: Type: text/html, Size: 5707 bytes --]

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

[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?


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

[Dmitry Baryshkov]

On Mon, Dec 16, 2024 at 12:40:54AM -0500, Marek Olšák wrote:
> git send-email (or rather the way it sends email) has been banned by gmail
> due to being considered unsecure. I don't plan to find a way to make it
> work and I don't plan to use a different email provider. It doesn't matter
> because I'll be the committer of this patch in our amd-staging-drm-next
> branch.

I'm sorry, but I'd second Joshua. As a community we use certain methods
and certain approaches which makes it easier for other people to review
your patches. One of those includes sending patches in plain text
without any attachments, etc (documented under Documentation/process/).
All my patches are being sent using git-send-email or b4 send via GMail.
Other developers use web-relay provided by the B4 tool.

Next, the MAINTAINERS file lists Alex, Christian and Xinhui as
maintainers of the drm/amd tree. If the file is incorrect or incomplete,
please correct it.

Last, but not least, this patch will likely go into drm-misc-next or
drm-next instead of amd-saging-drm-next. It is not AMD-specific.

> Let's talk about the concept and brokenness of DRM_FORMAT_MOD_LINEAR, not
> send-email.

The biggest problem with your approach is tht it is not clear which
modifier to use. For example, if one of the formats requires 128-byte
alignment, should the driver list just 128B or both 128B and 256B? If at
some point we add 32B alignment, will we have to update the drivers?
Which format should be used for exported buffers? Please provide
corresponding driver patches that utilize new uAPI.

Also, please don't forget the backwards compatibility issue. If we
follow this approach, the drivers have to list both LINEAR and new
PITCH_ALIGN modifiers. So the userspace still will attempt to use
LINEAR.

It is true that such requirements are platform-specific and are usually
encoded in the compostitor. I think it might make more sense to export
the pitch requirements using the extra hint-like property, which then
can be used by a smart userspace.

> Marek
> 
> On Sun, Dec 15, 2024 at 9:08 PM Joshua Ashton <joshua@froggi.es> wrote:
> 
> > Not really for my benefit, more that it's the proper thing to do if you
> > want anyone to apply your patch.
> >
> > You should really just be using git send-email.
> >
> > On 12/15/24 11:57 PM, Marek Olšák wrote:
> > > Only for you: Attached.
> > >
> > > Marek
> > >
> > > On Sun, Dec 15, 2024 at 6:37 PM Joshua Ashton <joshua@froggi.es
> > > <mailto:joshua@froggi.es>> wrote:
> > >
> > >     You should just re-send the whole patch, probably.
> > >
> > >     On 12/15/24 8:54 PM, Marek Olšák wrote:
> > >      > 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
> > >     <mailto:maraeo@gmail.com>
> > >      > <mailto:maraeo@gmail.com <mailto:maraeo@gmail.com>>> 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)
> > >      >
> > >
> > >     - Joshie 🐸✨
> > >
> >
> > - Joshie 🐸✨
> >
> >

-- 
With best wishes
Dmitry

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

[Lucas Stach]

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

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

[Simona Vetter]

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

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Marek Olšák]

[-- Attachment #1: Type: text/plain, Size: 2217 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 2927 bytes --]

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

[Marek Olšák]

[-- Attachment #1: Type: text/plain, Size: 3541 bytes --]

On Mon, Dec 16, 2024 at 4:28 AM Dmitry Baryshkov <
dmitry.baryshkov@linaro.org> wrote:

> On Mon, Dec 16, 2024 at 12:40:54AM -0500, Marek Olšák wrote:
> > git send-email (or rather the way it sends email) has been banned by
> gmail
> > due to being considered unsecure. I don't plan to find a way to make it
> > work and I don't plan to use a different email provider. It doesn't
> matter
> > because I'll be the committer of this patch in our amd-staging-drm-next
> > branch.
>
> I'm sorry, but I'd second Joshua. As a community we use certain methods
> and certain approaches which makes it easier for other people to review
> your patches. One of those includes sending patches in plain text
> without any attachments, etc (documented under Documentation/process/).
> All my patches are being sent using git-send-email or b4 send via GMail.
> Other developers use web-relay provided by the B4 tool.
>
> Next, the MAINTAINERS file lists Alex, Christian and Xinhui as
> maintainers of the drm/amd tree. If the file is incorrect or incomplete,
> please correct it.
>
> Last, but not least, this patch will likely go into drm-misc-next or
> drm-next instead of amd-saging-drm-next. It is not AMD-specific.
>

I won't comment on this because it's irrelevant. Alex will decide which
pull request it will end up in.


> > Let's talk about the concept and brokenness of DRM_FORMAT_MOD_LINEAR, not
> > send-email.
>
> The biggest problem with your approach is tht it is not clear which
> modifier to use. For example, if one of the formats requires 128-byte
> alignment, should the driver list just 128B or both 128B and 256B? If at
> some point we add 32B alignment, will we have to update the drivers?
> Which format should be used for exported buffers? Please provide
> corresponding driver patches that utilize new uAPI.
>

This is format(bpp)-independent. If some formats don't support some
alignment, only modifiers that are supported by all formats should be
exposed.

Drivers should list the alignment they support and all greater ones, e.g.:
Intel: 64B, 128B, 256B
AMD: 256B

Nobody chooses exactly which modifier to use in advance, and if some app
did that, it would be a violation of how modifiers work. Drivers only
expose modifiers sorted from best to worst, apps only compute intersections
of modifier lists and pass them to drivers, and drivers pick the first one
in the list or the best one in the list, but it doesn't matter which one at
that point. The computation of the intersection is what determines which
modifiers are allowed.


>
> Also, please don't forget the backwards compatibility issue. If we
> follow this approach, the drivers have to list both LINEAR and new
> PITCH_ALIGN modifiers. So the userspace still will attempt to use
> LINEAR.
>

Yes and no. Apps should never get an image using LINEAR if PITCH_ALIGN is
first in the list.


> It is true that such requirements are platform-specific and are usually
> encoded in the compostitor. I think it might make more sense to export
> the pitch requirements using the extra hint-like property, which then
> can be used by a smart userspace.
>

If we did that, it would be an admission that using modifiers exposed by
drivers can fail image allocation for any reason, and thus it would be an
indication that modifiers are a badly designed API because driver-exposed
modifiers don't fully describe image layouts, which is what they were
supposed to do in the first place.

Marek

[-- Attachment #2: Type: text/html, Size: 4588 bytes --]

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

[Marek Olšák]

[-- Attachment #1: Type: text/plain, Size: 3135 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 4308 bytes --]

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

[Marek Olšák]

[-- Attachment #1: Type: text/plain, Size: 4335 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 5447 bytes --]

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

[Michel Dänzer]

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.


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

[Brian Starkey]

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

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

[Michel Dänzer]

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"


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

[Michel Dänzer]

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.


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

[Brian Starkey]

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

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

[Michel Dänzer]

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.


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

[Simona Vetter]

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
-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Simona Vetter]

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

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Brian Starkey]

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

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

[Marek Olšák]

[-- Attachment #1: Type: text/plain, Size: 2031 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 2747 bytes --]

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

[Daniel Stone]

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

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

[Daniel Stone]

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

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

[Brian Starkey]

On Wed, Dec 18, 2024 at 09:53:56PM +0000, Marek Olšák wrote:
> On Wed, Dec 18, 2024 at 5:32 AM Brian Starkey <brian.starkey@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

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

[Michel Dänzer]

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.


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

[Simona Vetter]

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
-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Marek Olšák]

[-- Attachment #1: Type: text/plain, Size: 5560 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 6985 bytes --]

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

[Brian Starkey]

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

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

[Marek Olšák]

[-- Attachment #1: Type: text/plain, Size: 1428 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 1823 bytes --]

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

[Simona Vetter]

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

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Simona Vetter]

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
-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Marek Olšák]

[-- Attachment #1: Type: text/plain, Size: 4307 bytes --]

On Fri, Dec 20, 2024 at 10:24 AM Simona Vetter <simona.vetter@ffwll.ch>
wrote:

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

It won't always "work" with the new linear modifiers, but when it fails, it
will fail in a manner that is debuggable, understandable, and explainable
by non-driver developers, such as getting 0 common modifiers between 2
devices. For example, the GUI can report to a user that DRI_PRIME failed
because there are no common modifiers between the 2 devices, which is
better than failing with an unqueriable difficult-to-handle reason
(rejected pitch) or continuing with corruption (invalid pitch) or hangs
(invalid pitch causing buffer overrun and corrupting shader binaries next
to it).

Marek

[-- Attachment #2: Type: text/html, Size: 5162 bytes --]

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

[James Jones]

On 12/19/24 10:03, Simona Vetter wrote:
> 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

I spent some time thinking about this over the break, because on a venn 
diagram it does overlap a sliver of the work we've done to define the 
differences between the concepts of constraints Vs. capabilities in the 
smorgasbord of unified memory allocator talks/workshops/prototypes/etc. 
over the years. I'm not that worried about some overlap being 
introduced, because every reasonable rule deserves an exception here and 
there, but I have concerns similar to Daniel's and Brian's.

Once you start adding more than one special modifier, some things in the 
existing usage start to break down. Right now you can naively pass 
around modifiers, then somewhere either before or just after allocation 
depending on your usage, check if LINEAR is available and take your 
special "I can parse this thing" path, for whatever that means in your 
special use case. Modifying all those paths to include one variant of 
linear is probably OK-but-not-great. Modifying all those paths to 
include <N> variants of linear is probably unrealistic, and I do worry 
about slippery slopes here.

---

What got me more interested though was this led to another thought. At 
first I didn't notice that this was an exact-match constraint and 
thought it meant the usual alignment constraint of >=, and I was 
concerned about how future variants would interact poorly. It could 
still be a concern if things progress down this route, and we have 
vendor A requiring >= 32B alignment and vendor B requiring == 64B 
alignment. They're compatible, but modifiers expressing this would 
naively cancel each-other out unless vendor A proactively advertised == 
64B linear modifiers too. This isn't a huge deal at that scale, but it 
could get worse, and it got me thinking about a way to solve the problem 
of a less naive way to merge modifier lists.

As a background, the two hard problems left with implementing a 
constraint system to sit alongside the format modifier system are:

1) A way to name special heaps (E.g., local vidmem on device A) in the 
constraints in a way that spans process boundaries using some sort of 
identifier. There are various ways to solve this. Lately the thinking is 
something around dma heaps, but no one's fleshed it out yet that I'm aware.

2) A transport that doesn't require us to revise every userspace API, 
kernel API, and protocol that got revised to support DRM format 
modifiers, and every API/protocol introduced since.

I haven't seen any great ideas for the latter problem yet, but what if 
we did this:

- Introduced a new DRM format modifier vendor that was actually 
vendor-agnostic, but implied the format modifier was a constraint 
definition fragment instead of an actual modifier.

- Constraint-aware code could tack on its constraints (The ones it 
requires and/or the ones it can support allocating) as a series of 
additional modifiers using this vendor code. A given constraint might be 
fragmented into multiple modifiers, but their definition and 
serialization/deserialization mechanism could be defined in drm_fourcc.h 
as macros all the clients could use.

- Existing non-constraint-aware code in a modifier usage chain might 
filter these modifiers out using the existing strict intersection logic. 
Hence, any link in the chain not aware of constraints would likely block 
their use, but that's OK. We're muddling along without them now. It 
wouldn't make those situations any worse.

- New code would be required to use some minimal library (Header-only 
perhaps, as Simon and I proposed a few years ago) to intersect format 
modifier lists instead, and this code would parse out the constraint 
modifiers from each input list and use the envisioned per-constraint 
logic to merge them. It would result in yet another merged 
modifier+constraint list encoded as a list of modifiers that could be 
passed along through any format-modifier-aware API.

- One consideration that would be sort of tricky is that constraints are 
supposed to be advertised per-modifier, so you'd have to have a way to 
associate constraint modifiers in a given set with real modifiers in 
that set or in general. This is easily solved though. Some bits of the 
constraint modifiers would already need to be used to associate and 
order constraint fragments during deserialization, since modifier lists 
aren't strictly ordered.

This effectively allows you to use format modifiers to encode 
arbitrarily complex constraint mechanisms by piggybacking on the 
existing format modifier definition and transport mechanisms without 
breaking backwards compatibility. It's a little dirty, because modifiers 
are being abused to implement a raw bitstream, but modifiers and 
constraints are related concepts, so it's not a complete hack. It still 
requires modifying all the implementations in the system to fully make 
use of constraints, but doesn't require e.g. revising X11 DRI3 protocol 
again to tunnel them through Xwayland, and in situations where the 
constraint-aware thing sits downstream of the non-constraint-aware thing 
in the allocation pipeline, you could get some benefit even when all the 
upstream things aren't updated yet, because it could still merge in its 
local constraints before allocating or passing the modifier list down 
the chain.

Does this seem like something worth pursuing to others? I've been trying 
to decide how to best move the allocation constraints efforts forward, 
so it's potentially something I could put some time into this year.

Thanks,
-James

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

[Marek Olšák]

[-- Attachment #1: Type: text/plain, Size: 10626 bytes --]

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

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.

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.

Marek

On Fri, Jan 10, 2025 at 4:23 PM James Jones <jajones@nvidia.com> wrote:

> On 12/19/24 10:03, Simona Vetter wrote:
> > 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
>
> I spent some time thinking about this over the break, because on a venn
> diagram it does overlap a sliver of the work we've done to define the
> differences between the concepts of constraints Vs. capabilities in the
> smorgasbord of unified memory allocator talks/workshops/prototypes/etc.
> over the years. I'm not that worried about some overlap being
> introduced, because every reasonable rule deserves an exception here and
> there, but I have concerns similar to Daniel's and Brian's.
>
> Once you start adding more than one special modifier, some things in the
> existing usage start to break down. Right now you can naively pass
> around modifiers, then somewhere either before or just after allocation
> depending on your usage, check if LINEAR is available and take your
> special "I can parse this thing" path, for whatever that means in your
> special use case. Modifying all those paths to include one variant of
> linear is probably OK-but-not-great. Modifying all those paths to
> include <N> variants of linear is probably unrealistic, and I do worry
> about slippery slopes here.
>
> ---
>
> What got me more interested though was this led to another thought. At
> first I didn't notice that this was an exact-match constraint and
> thought it meant the usual alignment constraint of >=, and I was
> concerned about how future variants would interact poorly. It could
> still be a concern if things progress down this route, and we have
> vendor A requiring >= 32B alignment and vendor B requiring == 64B
> alignment. They're compatible, but modifiers expressing this would
> naively cancel each-other out unless vendor A proactively advertised ==
> 64B linear modifiers too. This isn't a huge deal at that scale, but it
> could get worse, and it got me thinking about a way to solve the problem
> of a less naive way to merge modifier lists.
>
> As a background, the two hard problems left with implementing a
> constraint system to sit alongside the format modifier system are:
>
> 1) A way to name special heaps (E.g., local vidmem on device A) in the
> constraints in a way that spans process boundaries using some sort of
> identifier. There are various ways to solve this. Lately the thinking is
> something around dma heaps, but no one's fleshed it out yet that I'm aware.
>
> 2) A transport that doesn't require us to revise every userspace API,
> kernel API, and protocol that got revised to support DRM format
> modifiers, and every API/protocol introduced since.
>
> I haven't seen any great ideas for the latter problem yet, but what if
> we did this:
>
> - Introduced a new DRM format modifier vendor that was actually
> vendor-agnostic, but implied the format modifier was a constraint
> definition fragment instead of an actual modifier.
>
> - Constraint-aware code could tack on its constraints (The ones it
> requires and/or the ones it can support allocating) as a series of
> additional modifiers using this vendor code. A given constraint might be
> fragmented into multiple modifiers, but their definition and
> serialization/deserialization mechanism could be defined in drm_fourcc.h
> as macros all the clients could use.
>
> - Existing non-constraint-aware code in a modifier usage chain might
> filter these modifiers out using the existing strict intersection logic.
> Hence, any link in the chain not aware of constraints would likely block
> their use, but that's OK. We're muddling along without them now. It
> wouldn't make those situations any worse.
>
> - New code would be required to use some minimal library (Header-only
> perhaps, as Simon and I proposed a few years ago) to intersect format
> modifier lists instead, and this code would parse out the constraint
> modifiers from each input list and use the envisioned per-constraint
> logic to merge them. It would result in yet another merged
> modifier+constraint list encoded as a list of modifiers that could be
> passed along through any format-modifier-aware API.
>
> - One consideration that would be sort of tricky is that constraints are
> supposed to be advertised per-modifier, so you'd have to have a way to
> associate constraint modifiers in a given set with real modifiers in
> that set or in general. This is easily solved though. Some bits of the
> constraint modifiers would already need to be used to associate and
> order constraint fragments during deserialization, since modifier lists
> aren't strictly ordered.
>
> This effectively allows you to use format modifiers to encode
> arbitrarily complex constraint mechanisms by piggybacking on the
> existing format modifier definition and transport mechanisms without
> breaking backwards compatibility. It's a little dirty, because modifiers
> are being abused to implement a raw bitstream, but modifiers and
> constraints are related concepts, so it's not a complete hack. It still
> requires modifying all the implementations in the system to fully make
> use of constraints, but doesn't require e.g. revising X11 DRI3 protocol
> again to tunnel them through Xwayland, and in situations where the
> constraint-aware thing sits downstream of the non-constraint-aware thing
> in the allocation pipeline, you could get some benefit even when all the
> upstream things aren't updated yet, because it could still merge in its
> local constraints before allocating or passing the modifier list down
> the chain.
>
> Does this seem like something worth pursuing to others? I've been trying
> to decide how to best move the allocation constraints efforts forward,
> so it's potentially something I could put some time into this year.
>
> Thanks,
> -James
>

[-- Attachment #2: Type: text/html, Size: 12246 bytes --]

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

[Thomas Zimmermann]

Hi


Am 15.12.24 um 21:53 schrieb Marek Olšák:
> 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)

I recently went through drivers and tried to harmonize some 
pitch-related code. There are essentially all cases from 2 to 256. See 
my series at

   https://patchwork.freedesktop.org/series/141168/

Sometimes the driver computes the pitch requirements from the 
bit-per-pixels. [1]

Hardware also has alignment requirements for framebuffer width (usually 
8 pixels).

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c#L410

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

[Thomas Zimmermann]

Hi


Am 14.01.25 um 14:46 schrieb Thomas Zimmermann:
> Hi
>
>
> Am 15.12.24 um 21:53 schrieb Marek Olšák:
>> 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)
>
> I recently went through drivers and tried to harmonize some 
> pitch-related code. There are essentially all cases from 2 to 256. See 
> my series at
>
>   https://patchwork.freedesktop.org/series/141168/
>
> Sometimes the driver computes the pitch requirements from the 
> bit-per-pixels. [1]
>
> Hardware also has alignment requirements for framebuffer width 
> (usually 8 pixels).

To clarify: given this and the overall discussion here, it seems to me 
that modifiers and descriptors really don't fit well to the problem.

Best regards
Thomas

>
> Best regards
> Thomas
>
> [1] 
> https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c#L410
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

[James Jones]

I don't see how that fits in the current modifier usage patterns. I'm 
not clear how applications are supposed to programmatically "look at the 
modifiers of other drivers to find commonalities," nor how that "keeps 
"expectations the same as today for simplicity.". I think replacing the 
existing linear modifier would be very disruptive, and I don't think 
this proposal solves a general problem. Is it common for other vendors' 
hardware to have such strict pitch/height alignment requirements? Prior 
to this discussion, I'd only ever heard of minimum alignments.

Thanks,
-James

On 1/14/25 01:38, Marek Olšák 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
> 
> 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.
> 
> 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.
> 
> Marek
> 
> On Fri, Jan 10, 2025 at 4:23 PM James Jones <jajones@nvidia.com 
> <mailto:jajones@nvidia.com>> wrote:
> 
>     On 12/19/24 10:03, Simona Vetter wrote:
>      > 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 <mailto: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
> 
>     I spent some time thinking about this over the break, because on a venn
>     diagram it does overlap a sliver of the work we've done to define the
>     differences between the concepts of constraints Vs. capabilities in the
>     smorgasbord of unified memory allocator talks/workshops/prototypes/etc.
>     over the years. I'm not that worried about some overlap being
>     introduced, because every reasonable rule deserves an exception here
>     and
>     there, but I have concerns similar to Daniel's and Brian's.
> 
>     Once you start adding more than one special modifier, some things in
>     the
>     existing usage start to break down. Right now you can naively pass
>     around modifiers, then somewhere either before or just after allocation
>     depending on your usage, check if LINEAR is available and take your
>     special "I can parse this thing" path, for whatever that means in your
>     special use case. Modifying all those paths to include one variant of
>     linear is probably OK-but-not-great. Modifying all those paths to
>     include <N> variants of linear is probably unrealistic, and I do worry
>     about slippery slopes here.
> 
>     ---
> 
>     What got me more interested though was this led to another thought. At
>     first I didn't notice that this was an exact-match constraint and
>     thought it meant the usual alignment constraint of >=, and I was
>     concerned about how future variants would interact poorly. It could
>     still be a concern if things progress down this route, and we have
>     vendor A requiring >= 32B alignment and vendor B requiring == 64B
>     alignment. They're compatible, but modifiers expressing this would
>     naively cancel each-other out unless vendor A proactively advertised ==
>     64B linear modifiers too. This isn't a huge deal at that scale, but it
>     could get worse, and it got me thinking about a way to solve the
>     problem
>     of a less naive way to merge modifier lists.
> 
>     As a background, the two hard problems left with implementing a
>     constraint system to sit alongside the format modifier system are:
> 
>     1) A way to name special heaps (E.g., local vidmem on device A) in the
>     constraints in a way that spans process boundaries using some sort of
>     identifier. There are various ways to solve this. Lately the
>     thinking is
>     something around dma heaps, but no one's fleshed it out yet that I'm
>     aware.
> 
>     2) A transport that doesn't require us to revise every userspace API,
>     kernel API, and protocol that got revised to support DRM format
>     modifiers, and every API/protocol introduced since.
> 
>     I haven't seen any great ideas for the latter problem yet, but what if
>     we did this:
> 
>     - Introduced a new DRM format modifier vendor that was actually
>     vendor-agnostic, but implied the format modifier was a constraint
>     definition fragment instead of an actual modifier.
> 
>     - Constraint-aware code could tack on its constraints (The ones it
>     requires and/or the ones it can support allocating) as a series of
>     additional modifiers using this vendor code. A given constraint
>     might be
>     fragmented into multiple modifiers, but their definition and
>     serialization/deserialization mechanism could be defined in
>     drm_fourcc.h
>     as macros all the clients could use.
> 
>     - Existing non-constraint-aware code in a modifier usage chain might
>     filter these modifiers out using the existing strict intersection
>     logic.
>     Hence, any link in the chain not aware of constraints would likely
>     block
>     their use, but that's OK. We're muddling along without them now. It
>     wouldn't make those situations any worse.
> 
>     - New code would be required to use some minimal library (Header-only
>     perhaps, as Simon and I proposed a few years ago) to intersect format
>     modifier lists instead, and this code would parse out the constraint
>     modifiers from each input list and use the envisioned per-constraint
>     logic to merge them. It would result in yet another merged
>     modifier+constraint list encoded as a list of modifiers that could be
>     passed along through any format-modifier-aware API.
> 
>     - One consideration that would be sort of tricky is that constraints
>     are
>     supposed to be advertised per-modifier, so you'd have to have a way to
>     associate constraint modifiers in a given set with real modifiers in
>     that set or in general. This is easily solved though. Some bits of the
>     constraint modifiers would already need to be used to associate and
>     order constraint fragments during deserialization, since modifier lists
>     aren't strictly ordered.
> 
>     This effectively allows you to use format modifiers to encode
>     arbitrarily complex constraint mechanisms by piggybacking on the
>     existing format modifier definition and transport mechanisms without
>     breaking backwards compatibility. It's a little dirty, because
>     modifiers
>     are being abused to implement a raw bitstream, but modifiers and
>     constraints are related concepts, so it's not a complete hack. It still
>     requires modifying all the implementations in the system to fully make
>     use of constraints, but doesn't require e.g. revising X11 DRI3 protocol
>     again to tunnel them through Xwayland, and in situations where the
>     constraint-aware thing sits downstream of the non-constraint-aware
>     thing
>     in the allocation pipeline, you could get some benefit even when all
>     the
>     upstream things aren't updated yet, because it could still merge in its
>     local constraints before allocating or passing the modifier list down
>     the chain.
> 
>     Does this seem like something worth pursuing to others? I've been
>     trying
>     to decide how to best move the allocation constraints efforts forward,
>     so it's potentially something I could put some time into this year.
> 
>     Thanks,
>     -James
> 

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

[Daniel Stone]

Hi,

On Tue, 14 Jan 2025 at 09:38, Marek Olšák <maraeo@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.

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

The design for the allocator - communicating constraints ('pitch must
be exactly aligned to the next 256-byte boundary', 'pitch must be a
multiple of 32 bytes', 'buffer must be physically contiguous', etc)
separately from the chosen layout - would seem to fit this much
better. And since there doesn't seem to be a tractable solution we can
jam into the single 'intersect multiple sets of uint64s' API, we might
as well go through typing that out.

Cheers,
Daniel

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

[Faith Ekstrand]

[-- Attachment #1: Type: text/plain, Size: 11604 bytes --]

On January 14, 2025 03:39:45 Marek Olšák <maraeo@gmail.com> 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.

~Faith


>
>
>
> 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.
>
>
> 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.
>
>
> Marek
>
>
>
> On Fri, Jan 10, 2025 at 4:23 PM James Jones <jajones@nvidia.com> wrote:
>
> On 12/19/24 10:03, Simona Vetter wrote:
>> 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
>
> I spent some time thinking about this over the break, because on a venn
> diagram it does overlap a sliver of the work we've done to define the
> differences between the concepts of constraints Vs. capabilities in the
> smorgasbord of unified memory allocator talks/workshops/prototypes/etc.
> over the years. I'm not that worried about some overlap being
> introduced, because every reasonable rule deserves an exception here and
> there, but I have concerns similar to Daniel's and Brian's.
>
> Once you start adding more than one special modifier, some things in the
> existing usage start to break down. Right now you can naively pass
> around modifiers, then somewhere either before or just after allocation
> depending on your usage, check if LINEAR is available and take your
> special "I can parse this thing" path, for whatever that means in your
> special use case. Modifying all those paths to include one variant of
> linear is probably OK-but-not-great. Modifying all those paths to
> include <N> variants of linear is probably unrealistic, and I do worry
> about slippery slopes here.
>
> ---
>
> What got me more interested though was this led to another thought. At
> first I didn't notice that this was an exact-match constraint and
> thought it meant the usual alignment constraint of >=, and I was
> concerned about how future variants would interact poorly. It could
> still be a concern if things progress down this route, and we have
> vendor A requiring >= 32B alignment and vendor B requiring == 64B
> alignment. They're compatible, but modifiers expressing this would
> naively cancel each-other out unless vendor A proactively advertised ==
> 64B linear modifiers too. This isn't a huge deal at that scale, but it
> could get worse, and it got me thinking about a way to solve the problem
> of a less naive way to merge modifier lists.
>
> As a background, the two hard problems left with implementing a
> constraint system to sit alongside the format modifier system are:
>
> 1) A way to name special heaps (E.g., local vidmem on device A) in the
> constraints in a way that spans process boundaries using some sort of
> identifier. There are various ways to solve this. Lately the thinking is
> something around dma heaps, but no one's fleshed it out yet that I'm aware.
>
> 2) A transport that doesn't require us to revise every userspace API,
> kernel API, and protocol that got revised to support DRM format
> modifiers, and every API/protocol introduced since.
>
> I haven't seen any great ideas for the latter problem yet, but what if
> we did this:
>
> - Introduced a new DRM format modifier vendor that was actually
> vendor-agnostic, but implied the format modifier was a constraint
> definition fragment instead of an actual modifier.
>
> - Constraint-aware code could tack on its constraints (The ones it
> requires and/or the ones it can support allocating) as a series of
> additional modifiers using this vendor code. A given constraint might be
> fragmented into multiple modifiers, but their definition and
> serialization/deserialization mechanism could be defined in drm_fourcc.h
> as macros all the clients could use.
>
> - Existing non-constraint-aware code in a modifier usage chain might
> filter these modifiers out using the existing strict intersection logic.
> Hence, any link in the chain not aware of constraints would likely block
> their use, but that's OK. We're muddling along without them now. It
> wouldn't make those situations any worse.
>
> - New code would be required to use some minimal library (Header-only
> perhaps, as Simon and I proposed a few years ago) to intersect format
> modifier lists instead, and this code would parse out the constraint
> modifiers from each input list and use the envisioned per-constraint
> logic to merge them. It would result in yet another merged
> modifier+constraint list encoded as a list of modifiers that could be
> passed along through any format-modifier-aware API.
>
> - One consideration that would be sort of tricky is that constraints are
> supposed to be advertised per-modifier, so you'd have to have a way to
> associate constraint modifiers in a given set with real modifiers in
> that set or in general. This is easily solved though. Some bits of the
> constraint modifiers would already need to be used to associate and
> order constraint fragments during deserialization, since modifier lists
> aren't strictly ordered.
>
> This effectively allows you to use format modifiers to encode
> arbitrarily complex constraint mechanisms by piggybacking on the
> existing format modifier definition and transport mechanisms without
> breaking backwards compatibility. It's a little dirty, because modifiers
> are being abused to implement a raw bitstream, but modifiers and
> constraints are related concepts, so it's not a complete hack. It still
> requires modifying all the implementations in the system to fully make
> use of constraints, but doesn't require e.g. revising X11 DRI3 protocol
> again to tunnel them through Xwayland, and in situations where the
> constraint-aware thing sits downstream of the non-constraint-aware thing
> in the allocation pipeline, you could get some benefit even when all the
> upstream things aren't updated yet, because it could still merge in its
> local constraints before allocating or passing the modifier list down
> the chain.
>
> Does this seem like something worth pursuing to others? I've been trying
> to decide how to best move the allocation constraints efforts forward,
> so it's potentially something I could put some time into this year.
>
> Thanks,
> -James


[-- Attachment #2: Type: text/html, Size: 14407 bytes --]

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

[Marek Olšák]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

On Tue, Jan 14, 2025 at 12:55 PM James Jones <jajones@nvidia.com> wrote:

> I don't see how that fits in the current modifier usage patterns. I'm
> not clear how applications are supposed to programmatically "look at the
> modifiers of other drivers to find commonalities," nor how that "keeps
> "expectations the same as today for simplicity.". I think replacing the
> existing linear modifier would be very disruptive, and I don't think
> this proposal solves a general problem. Is it common for other vendors'
> hardware to have such strict pitch/height alignment requirements? Prior
> to this discussion, I'd only ever heard of minimum alignments.
>

James, no changes to apps are required. Driver developers can _choose_ to
look at other drivers to find commonalities and expose them in their own
drivers.

Marek

[-- Attachment #2: Type: text/html, Size: 1238 bytes --]

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

[Marek Olšák]

[-- Attachment #1: Type: text/plain, Size: 3527 bytes --]

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

> Hi,
>
> On Tue, 14 Jan 2025 at 09:38, Marek Olšák <maraeo@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

[-- Attachment #2: Type: text/html, Size: 4447 bytes --]

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

[Marek Olšák]

[-- Attachment #1: Type: text/plain, Size: 2610 bytes --]

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

> On January 14, 2025 03:39:45 Marek Olšák <maraeo@gmail.com> 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

[-- Attachment #2: Type: text/html, Size: 3834 bytes --]

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

[Simona Vetter]

On Tue, Jan 14, 2025 at 12:33:57PM -0600, Faith Ekstrand wrote:
> On January 14, 2025 03:39:45 Marek Olšák <maraeo@gmail.com> 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.

Yeah I feel like step 1 here would be to just add LINEAR_AMD, document the
requirements, add it to drivers that can pull it in for display or
whatever or have some other interop requirement, and see where we go.

The combinatorial explosion of linear constraints is way too much as
Daniel/James both point out, but luckily there's not that many
gpu/display/camera vendors in this world, and we do have like almost
56bits of LINEAR space we can waste.

What we cannot do is drop LINEAR itself, since that would break the world.
Maybe in some glorious future, if there's enough drivers out there
supporting vendor linear modifiers, userspace could know that LINEAR is
special, and if it finds a matching vendor LINEAR_FOO modifier, it should
prefer that one. But then we could probably achieve the same by something
like "try common modifiers in order" and put LINEAR last consistently,
after all the vendor linear modifers, and have the same effect.
-Sima

> ~Faith
> 
> 
> > 
> > 
> > 
> > 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.
> > 
> > 
> > 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.
> > 
> > 
> > Marek
> > 
> > 
> > 
> > On Fri, Jan 10, 2025 at 4:23 PM James Jones <jajones@nvidia.com> wrote:
> > 
> > On 12/19/24 10:03, Simona Vetter wrote:
> > > 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
> > 
> > I spent some time thinking about this over the break, because on a venn
> > diagram it does overlap a sliver of the work we've done to define the
> > differences between the concepts of constraints Vs. capabilities in the
> > smorgasbord of unified memory allocator talks/workshops/prototypes/etc.
> > over the years. I'm not that worried about some overlap being
> > introduced, because every reasonable rule deserves an exception here and
> > there, but I have concerns similar to Daniel's and Brian's.
> > 
> > Once you start adding more than one special modifier, some things in the
> > existing usage start to break down. Right now you can naively pass
> > around modifiers, then somewhere either before or just after allocation
> > depending on your usage, check if LINEAR is available and take your
> > special "I can parse this thing" path, for whatever that means in your
> > special use case. Modifying all those paths to include one variant of
> > linear is probably OK-but-not-great. Modifying all those paths to
> > include <N> variants of linear is probably unrealistic, and I do worry
> > about slippery slopes here.
> > 
> > ---
> > 
> > What got me more interested though was this led to another thought. At
> > first I didn't notice that this was an exact-match constraint and
> > thought it meant the usual alignment constraint of >=, and I was
> > concerned about how future variants would interact poorly. It could
> > still be a concern if things progress down this route, and we have
> > vendor A requiring >= 32B alignment and vendor B requiring == 64B
> > alignment. They're compatible, but modifiers expressing this would
> > naively cancel each-other out unless vendor A proactively advertised ==
> > 64B linear modifiers too. This isn't a huge deal at that scale, but it
> > could get worse, and it got me thinking about a way to solve the problem
> > of a less naive way to merge modifier lists.
> > 
> > As a background, the two hard problems left with implementing a
> > constraint system to sit alongside the format modifier system are:
> > 
> > 1) A way to name special heaps (E.g., local vidmem on device A) in the
> > constraints in a way that spans process boundaries using some sort of
> > identifier. There are various ways to solve this. Lately the thinking is
> > something around dma heaps, but no one's fleshed it out yet that I'm aware.
> > 
> > 2) A transport that doesn't require us to revise every userspace API,
> > kernel API, and protocol that got revised to support DRM format
> > modifiers, and every API/protocol introduced since.
> > 
> > I haven't seen any great ideas for the latter problem yet, but what if
> > we did this:
> > 
> > - Introduced a new DRM format modifier vendor that was actually
> > vendor-agnostic, but implied the format modifier was a constraint
> > definition fragment instead of an actual modifier.
> > 
> > - Constraint-aware code could tack on its constraints (The ones it
> > requires and/or the ones it can support allocating) as a series of
> > additional modifiers using this vendor code. A given constraint might be
> > fragmented into multiple modifiers, but their definition and
> > serialization/deserialization mechanism could be defined in drm_fourcc.h
> > as macros all the clients could use.
> > 
> > - Existing non-constraint-aware code in a modifier usage chain might
> > filter these modifiers out using the existing strict intersection logic.
> > Hence, any link in the chain not aware of constraints would likely block
> > their use, but that's OK. We're muddling along without them now. It
> > wouldn't make those situations any worse.
> > 
> > - New code would be required to use some minimal library (Header-only
> > perhaps, as Simon and I proposed a few years ago) to intersect format
> > modifier lists instead, and this code would parse out the constraint
> > modifiers from each input list and use the envisioned per-constraint
> > logic to merge them. It would result in yet another merged
> > modifier+constraint list encoded as a list of modifiers that could be
> > passed along through any format-modifier-aware API.
> > 
> > - One consideration that would be sort of tricky is that constraints are
> > supposed to be advertised per-modifier, so you'd have to have a way to
> > associate constraint modifiers in a given set with real modifiers in
> > that set or in general. This is easily solved though. Some bits of the
> > constraint modifiers would already need to be used to associate and
> > order constraint fragments during deserialization, since modifier lists
> > aren't strictly ordered.
> > 
> > This effectively allows you to use format modifiers to encode
> > arbitrarily complex constraint mechanisms by piggybacking on the
> > existing format modifier definition and transport mechanisms without
> > breaking backwards compatibility. It's a little dirty, because modifiers
> > are being abused to implement a raw bitstream, but modifiers and
> > constraints are related concepts, so it's not a complete hack. It still
> > requires modifying all the implementations in the system to fully make
> > use of constraints, but doesn't require e.g. revising X11 DRI3 protocol
> > again to tunnel them through Xwayland, and in situations where the
> > constraint-aware thing sits downstream of the non-constraint-aware thing
> > in the allocation pipeline, you could get some benefit even when all the
> > upstream things aren't updated yet, because it could still merge in its
> > local constraints before allocating or passing the modifier list down
> > the chain.
> > 
> > Does this seem like something worth pursuing to others? I've been trying
> > to decide how to best move the allocation constraints efforts forward,
> > so it's potentially something I could put some time into this year.
> > 
> > Thanks,
> > -James
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Daniel Stone]

On Wed, 15 Jan 2025 at 04:05, Marek Olšák <maraeo@gmail.com> wrote:
> On Tue, Jan 14, 2025 at 12:58 PM Daniel Stone <daniel@fooishbar.org> wrote:
>> 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: [5 / 0 / 5 / 5 / 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.

Mali (actually two DRM drivers and sort of three Mesa drivers) can be
paired with any one of 11 KMS drivers (really 12 given that one is a
very independent subdriver), and something like 20 different codecs
(at least 12 different vendors; I didn't bother counting the actual
subdrivers which are all quite different). The VeriSilicon Hantro G2
codec driver is shipped by five (that we know of) vendors who all have
their own KMS drivers. One of those is in the Rockchip RK3588, which
(don't ask me why) ships six different codec blocks, with three
different drivers, from two different vendors - that's before you even
get to things like the ISP and NPU which really need to be sharing
buffers properly without copies.

So yeah, working widely without having to encode specific knowledge
everywhere isn't a nice-to-have, it's a hard baseline requirement.

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

It really does though!

None of these devices use TTM with placement moves, and doing that
isn't a fix either. Embedded systems have so low memory bandwidth that
the difference between choosing the wrong placement and moving it
later vs. having the right placement to begin with is the difference
between 'this does not work' and 'great, I can ship this'. Which is
great if you're a consultancy trying to get paid, but tbh I'd rather
work on more interesting things.

So yeah, userspace does very much choose the placement. On most
drivers, this is either by 'knowing' which device to allocate from, or
passing a flag to your allocation ioctl. For newer drivers though,
there's the dma-heap allocation mechanism which is now upstream and
the blessed path, for which userspace needs to explicitly know the
desired placement (and must, because fixing it up later is a
non-starter).

Given that we need to keep LINEAR ~forever for ABI reasons, and
because there's no reasonably workable alternative, let's abandon the
idea of abandoning LINEAR, and try to work with out-of-band signalling
instead.

One idea is to actually pursue the allocator idea and express this
properly through constraints. I'd be super in favour of this,
unsurprisingly, because it allows us to solve a whole pile of other
problems, rather than the extremely narrow AMD/Intel interop case.

Another idea for the out-of-band signalling would be to add
information-only modifiers, like
DRM_FORMAT_MOD_LINEAR_PITCH_ALIGN_EQ(256), or
DRM_FORMAT_MOD_LINEAR_PITCH_ALIGN_GE(32). But then that doesn't really
work at all with how people actually use modifiers: as the doc
describes, userspace takes and intersects the declared modifier lists
and passes the result through. The intersection of LINEAR+EQ256 and
LINEAR+GE32 is LINEAR, so a userspace that follows the rules will just
drop the hints on the floor and pick whatever linear allocation it
feels like.

I think I've just talked myself into the position that passing
allocator constraints together with modifiers is the only way to
actually solve this problem, at least without creating the sort of
technical debt that meant we spent years fixing up implicit/explicit
modifier interactions when it really should've just been adding a
!)@*(#$ u64 next to the u32.

Cheers,
Daniel

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

[Simona Vetter]

On Wed, Jan 15, 2025 at 12:20:07PM +0000, Daniel Stone wrote:
> On Wed, 15 Jan 2025 at 04:05, Marek Olšák <maraeo@gmail.com> wrote:
> > On Tue, Jan 14, 2025 at 12:58 PM Daniel Stone <daniel@fooishbar.org> wrote:
> >> 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: [5 / 0 / 5 / 5 / 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.
> 
> Mali (actually two DRM drivers and sort of three Mesa drivers) can be
> paired with any one of 11 KMS drivers (really 12 given that one is a
> very independent subdriver), and something like 20 different codecs
> (at least 12 different vendors; I didn't bother counting the actual
> subdrivers which are all quite different). The VeriSilicon Hantro G2
> codec driver is shipped by five (that we know of) vendors who all have
> their own KMS drivers. One of those is in the Rockchip RK3588, which
> (don't ask me why) ships six different codec blocks, with three
> different drivers, from two different vendors - that's before you even
> get to things like the ISP and NPU which really need to be sharing
> buffers properly without copies.
> 
> So yeah, working widely without having to encode specific knowledge
> everywhere isn't a nice-to-have, it's a hard baseline requirement.
> 
> >> > 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.
> 
> It really does though!
> 
> None of these devices use TTM with placement moves, and doing that
> isn't a fix either. Embedded systems have so low memory bandwidth that
> the difference between choosing the wrong placement and moving it
> later vs. having the right placement to begin with is the difference
> between 'this does not work' and 'great, I can ship this'. Which is
> great if you're a consultancy trying to get paid, but tbh I'd rather
> work on more interesting things.
> 
> So yeah, userspace does very much choose the placement. On most
> drivers, this is either by 'knowing' which device to allocate from, or
> passing a flag to your allocation ioctl. For newer drivers though,
> there's the dma-heap allocation mechanism which is now upstream and
> the blessed path, for which userspace needs to explicitly know the
> desired placement (and must, because fixing it up later is a
> non-starter).
> 
> Given that we need to keep LINEAR ~forever for ABI reasons, and
> because there's no reasonably workable alternative, let's abandon the
> idea of abandoning LINEAR, and try to work with out-of-band signalling
> instead.
> 
> One idea is to actually pursue the allocator idea and express this
> properly through constraints. I'd be super in favour of this,
> unsurprisingly, because it allows us to solve a whole pile of other
> problems, rather than the extremely narrow AMD/Intel interop case.
> 
> Another idea for the out-of-band signalling would be to add
> information-only modifiers, like
> DRM_FORMAT_MOD_LINEAR_PITCH_ALIGN_EQ(256), or
> DRM_FORMAT_MOD_LINEAR_PITCH_ALIGN_GE(32). But then that doesn't really
> work at all with how people actually use modifiers: as the doc
> describes, userspace takes and intersects the declared modifier lists
> and passes the result through. The intersection of LINEAR+EQ256 and
> LINEAR+GE32 is LINEAR, so a userspace that follows the rules will just
> drop the hints on the floor and pick whatever linear allocation it
> feels like.

Yeah I think latest when we also take into account logical image size (not
just pitch) with stuff like it needs to be aligned to 2 pixels in both
directions just using modifiers falls apart.

And the problem with linear, unlike device modifiers is that we can't just
throw up our hands and enumerate the handful of formats in actual use for
interop. There's so many produces and consumers of linera buffers
(Daniel's list above missed camera/image processors) that save assumption
is that anything really can happen.

> I think I've just talked myself into the position that passing
> allocator constraints together with modifiers is the only way to
> actually solve this problem, at least without creating the sort of
> technical debt that meant we spent years fixing up implicit/explicit
> modifier interactions when it really should've just been adding a
> !)@*(#$ u64 next to the u32.

Yeah probably.

Otoh I know inertia, so I am tempted to go with the oddball
LINEAR_VEDNOR_A_VENDOR_B_INTEROP thing and stretch the runway for a bit.
And we just assign those as we go as a very special thing, and the drivers
that support it would prefer it above just LINEAR if there's no other
common format left.

Also makes it really obvious what all userspace/kernel driver enabling
would be needed to justify such a modifier.
-Sima

> 
> Cheers,
> Daniel

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Marek Olšák]

[-- Attachment #1: Type: text/plain, Size: 7931 bytes --]

If DRM_FORMAT_MOD_LINEAR stays, then most of this discussion is irrelevant.
If you don't like the new linear modifiers, don't use them. If that's you,
bye.

For the rest, there are multiple solutions:

1) New vendor-agnostic linear modifiers. The reason why we would want them
is that they define robust functions for (x,y,w)->address, (w,h)->size, and
alignment. All 2^32 (roughly) AMD modifiers have such functions in Mesa
(really). Linear modifiers would work in the same way.

2) New cross-vendor modifier defining 1 layout for the existing case. This
is not less effort than 1). It's just a different #define, but limited to
only 1 case. Why bother. Is it really worth debating so much whether
#define should have 0 parameters instead of 5? If nobody else uses 1), so
what?

3) Implementing DRM_FORMAT_MOD_LINEAR as having 256B pitch and offset
alignment. This is what we do today. Even if Intel and some AMD chips can
do 64B or 128B alignment, they overalign to 256B. With so many AMD+NV
laptops out there, NV is probably next, unless they already do this in the
closed source driver.

Marek

On Fri, Jan 17, 2025 at 9:18 AM Simona Vetter <simona.vetter@ffwll.ch>
wrote:

> On Wed, Jan 15, 2025 at 12:20:07PM +0000, Daniel Stone wrote:
> > On Wed, 15 Jan 2025 at 04:05, Marek Olšák <maraeo@gmail.com> wrote:
> > > On Tue, Jan 14, 2025 at 12:58 PM Daniel Stone <daniel@fooishbar.org>
> wrote:
> > >> 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: [5 / 0 / 5 / 5 / 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.
> >
> > Mali (actually two DRM drivers and sort of three Mesa drivers) can be
> > paired with any one of 11 KMS drivers (really 12 given that one is a
> > very independent subdriver), and something like 20 different codecs
> > (at least 12 different vendors; I didn't bother counting the actual
> > subdrivers which are all quite different). The VeriSilicon Hantro G2
> > codec driver is shipped by five (that we know of) vendors who all have
> > their own KMS drivers. One of those is in the Rockchip RK3588, which
> > (don't ask me why) ships six different codec blocks, with three
> > different drivers, from two different vendors - that's before you even
> > get to things like the ISP and NPU which really need to be sharing
> > buffers properly without copies.
> >
> > So yeah, working widely without having to encode specific knowledge
> > everywhere isn't a nice-to-have, it's a hard baseline requirement.
> >
> > >> > 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.
> >
> > It really does though!
> >
> > None of these devices use TTM with placement moves, and doing that
> > isn't a fix either. Embedded systems have so low memory bandwidth that
> > the difference between choosing the wrong placement and moving it
> > later vs. having the right placement to begin with is the difference
> > between 'this does not work' and 'great, I can ship this'. Which is
> > great if you're a consultancy trying to get paid, but tbh I'd rather
> > work on more interesting things.
> >
> > So yeah, userspace does very much choose the placement. On most
> > drivers, this is either by 'knowing' which device to allocate from, or
> > passing a flag to your allocation ioctl. For newer drivers though,
> > there's the dma-heap allocation mechanism which is now upstream and
> > the blessed path, for which userspace needs to explicitly know the
> > desired placement (and must, because fixing it up later is a
> > non-starter).
> >
> > Given that we need to keep LINEAR ~forever for ABI reasons, and
> > because there's no reasonably workable alternative, let's abandon the
> > idea of abandoning LINEAR, and try to work with out-of-band signalling
> > instead.
> >
> > One idea is to actually pursue the allocator idea and express this
> > properly through constraints. I'd be super in favour of this,
> > unsurprisingly, because it allows us to solve a whole pile of other
> > problems, rather than the extremely narrow AMD/Intel interop case.
> >
> > Another idea for the out-of-band signalling would be to add
> > information-only modifiers, like
> > DRM_FORMAT_MOD_LINEAR_PITCH_ALIGN_EQ(256), or
> > DRM_FORMAT_MOD_LINEAR_PITCH_ALIGN_GE(32). But then that doesn't really
> > work at all with how people actually use modifiers: as the doc
> > describes, userspace takes and intersects the declared modifier lists
> > and passes the result through. The intersection of LINEAR+EQ256 and
> > LINEAR+GE32 is LINEAR, so a userspace that follows the rules will just
> > drop the hints on the floor and pick whatever linear allocation it
> > feels like.
>
> Yeah I think latest when we also take into account logical image size (not
> just pitch) with stuff like it needs to be aligned to 2 pixels in both
> directions just using modifiers falls apart.
>
> And the problem with linear, unlike device modifiers is that we can't just
> throw up our hands and enumerate the handful of formats in actual use for
> interop. There's so many produces and consumers of linera buffers
> (Daniel's list above missed camera/image processors) that save assumption
> is that anything really can happen.
>
> > I think I've just talked myself into the position that passing
> > allocator constraints together with modifiers is the only way to
> > actually solve this problem, at least without creating the sort of
> > technical debt that meant we spent years fixing up implicit/explicit
> > modifier interactions when it really should've just been adding a
> > !)@*(#$ u64 next to the u32.
>
> Yeah probably.
>
> Otoh I know inertia, so I am tempted to go with the oddball
> LINEAR_VEDNOR_A_VENDOR_B_INTEROP thing and stretch the runway for a bit.
> And we just assign those as we go as a very special thing, and the drivers
> that support it would prefer it above just LINEAR if there's no other
> common format left.
>
> Also makes it really obvious what all userspace/kernel driver enabling
> would be needed to justify such a modifier.
> -Sima
>
> >
> > Cheers,
> > Daniel
>
> --
> Simona Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>

[-- Attachment #2: Type: text/html, Size: 9875 bytes --]

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

[Thomas Zimmermann]

Hi


Am 18.01.25 um 03:37 schrieb Marek Olšák:
[...]
>
> 3) Implementing DRM_FORMAT_MOD_LINEAR as having 256B pitch and offset 
> alignment. This is what we do today. Even if Intel and some AMD chips 
> can do 64B or 128B alignment, they overalign to 256B. With so many 
> AMD+NV laptops out there, NV is probably next, unless they already do 
> this in the closed source driver.

The dumb-buffer series currently being discussed on dri-devel also 
touches handling of scanline pitches. THe actual value varies with each 
driver.  Should dumb buffers use a default pitch alignment of 256 on al 
hardware?

Best regards
Thomas

>
> Marek
>
> On Fri, Jan 17, 2025 at 9:18 AM Simona Vetter <simona.vetter@ffwll.ch> 
> wrote:
>
>     On Wed, Jan 15, 2025 at 12:20:07PM +0000, Daniel Stone wrote:
>     > On Wed, 15 Jan 2025 at 04:05, Marek Olšák <maraeo@gmail.com> wrote:
>     > > On Tue, Jan 14, 2025 at 12:58 PM Daniel Stone
>     <daniel@fooishbar.org> wrote:
>     > >> 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: [5 / 0 / 5 / 5 / 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.
>     >
>     > Mali (actually two DRM drivers and sort of three Mesa drivers)
>     can be
>     > paired with any one of 11 KMS drivers (really 12 given that one is a
>     > very independent subdriver), and something like 20 different codecs
>     > (at least 12 different vendors; I didn't bother counting the actual
>     > subdrivers which are all quite different). The VeriSilicon Hantro G2
>     > codec driver is shipped by five (that we know of) vendors who
>     all have
>     > their own KMS drivers. One of those is in the Rockchip RK3588, which
>     > (don't ask me why) ships six different codec blocks, with three
>     > different drivers, from two different vendors - that's before
>     you even
>     > get to things like the ISP and NPU which really need to be sharing
>     > buffers properly without copies.
>     >
>     > So yeah, working widely without having to encode specific knowledge
>     > everywhere isn't a nice-to-have, it's a hard baseline requirement.
>     >
>     > >> > 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.
>     >
>     > It really does though!
>     >
>     > None of these devices use TTM with placement moves, and doing that
>     > isn't a fix either. Embedded systems have so low memory
>     bandwidth that
>     > the difference between choosing the wrong placement and moving it
>     > later vs. having the right placement to begin with is the difference
>     > between 'this does not work' and 'great, I can ship this'. Which is
>     > great if you're a consultancy trying to get paid, but tbh I'd rather
>     > work on more interesting things.
>     >
>     > So yeah, userspace does very much choose the placement. On most
>     > drivers, this is either by 'knowing' which device to allocate
>     from, or
>     > passing a flag to your allocation ioctl. For newer drivers though,
>     > there's the dma-heap allocation mechanism which is now upstream and
>     > the blessed path, for which userspace needs to explicitly know the
>     > desired placement (and must, because fixing it up later is a
>     > non-starter).
>     >
>     > Given that we need to keep LINEAR ~forever for ABI reasons, and
>     > because there's no reasonably workable alternative, let's
>     abandon the
>     > idea of abandoning LINEAR, and try to work with out-of-band
>     signalling
>     > instead.
>     >
>     > One idea is to actually pursue the allocator idea and express this
>     > properly through constraints. I'd be super in favour of this,
>     > unsurprisingly, because it allows us to solve a whole pile of other
>     > problems, rather than the extremely narrow AMD/Intel interop case.
>     >
>     > Another idea for the out-of-band signalling would be to add
>     > information-only modifiers, like
>     > DRM_FORMAT_MOD_LINEAR_PITCH_ALIGN_EQ(256), or
>     > DRM_FORMAT_MOD_LINEAR_PITCH_ALIGN_GE(32). But then that doesn't
>     really
>     > work at all with how people actually use modifiers: as the doc
>     > describes, userspace takes and intersects the declared modifier
>     lists
>     > and passes the result through. The intersection of LINEAR+EQ256 and
>     > LINEAR+GE32 is LINEAR, so a userspace that follows the rules
>     will just
>     > drop the hints on the floor and pick whatever linear allocation it
>     > feels like.
>
>     Yeah I think latest when we also take into account logical image
>     size (not
>     just pitch) with stuff like it needs to be aligned to 2 pixels in both
>     directions just using modifiers falls apart.
>
>     And the problem with linear, unlike device modifiers is that we
>     can't just
>     throw up our hands and enumerate the handful of formats in actual
>     use for
>     interop. There's so many produces and consumers of linera buffers
>     (Daniel's list above missed camera/image processors) that save
>     assumption
>     is that anything really can happen.
>
>     > I think I've just talked myself into the position that passing
>     > allocator constraints together with modifiers is the only way to
>     > actually solve this problem, at least without creating the sort of
>     > technical debt that meant we spent years fixing up implicit/explicit
>     > modifier interactions when it really should've just been adding a
>     > !)@*(#$ u64 next to the u32.
>
>     Yeah probably.
>
>     Otoh I know inertia, so I am tempted to go with the oddball
>     LINEAR_VEDNOR_A_VENDOR_B_INTEROP thing and stretch the runway for
>     a bit.
>     And we just assign those as we go as a very special thing, and the
>     drivers
>     that support it would prefer it above just LINEAR if there's no other
>     common format left.
>
>     Also makes it really obvious what all userspace/kernel driver enabling
>     would be needed to justify such a modifier.
>     -Sima
>
>     >
>     > Cheers,
>     > Daniel
>
>     -- 
>     Simona Vetter
>     Software Engineer, Intel Corporation
>     http://blog.ffwll.ch
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

[Simona Vetter]

On Mon, Jan 20, 2025 at 08:58:20AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> 
> Am 18.01.25 um 03:37 schrieb Marek Olšák:
> [...]
> > 
> > 3) Implementing DRM_FORMAT_MOD_LINEAR as having 256B pitch and offset
> > alignment. This is what we do today. Even if Intel and some AMD chips
> > can do 64B or 128B alignment, they overalign to 256B. With so many
> > AMD+NV laptops out there, NV is probably next, unless they already do
> > this in the closed source driver.

I don't think this works, or at least not any better than the current
linear modifier. There's way too many users of that thing out there that I
think you can realistically redefine it.

Adding new linear modifiers and then preferring those above the old LINEAR
(if there is one left after all the wittling down to a common set) is I
think the only option that really works to fix something.

> The dumb-buffer series currently being discussed on dri-devel also touches
> handling of scanline pitches. THe actual value varies with each driver. 
> Should dumb buffers use a default pitch alignment of 256 on al hardware?

If you go with new modifiers then there could be shared code that dtrt by
just looking at the modifier list.
-Sima

> Best regards
> Thomas
> 
> > 
> > Marek
> > 
> > On Fri, Jan 17, 2025 at 9:18 AM Simona Vetter <simona.vetter@ffwll.ch>
> > wrote:
> > 
> >     On Wed, Jan 15, 2025 at 12:20:07PM +0000, Daniel Stone wrote:
> >     > On Wed, 15 Jan 2025 at 04:05, Marek Olšák <maraeo@gmail.com> wrote:
> >     > > On Tue, Jan 14, 2025 at 12:58 PM Daniel Stone
> >     <daniel@fooishbar.org> wrote:
> >     > >> 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: [5 / 0 / 5 / 5 / 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.
> >     >
> >     > Mali (actually two DRM drivers and sort of three Mesa drivers)
> >     can be
> >     > paired with any one of 11 KMS drivers (really 12 given that one is a
> >     > very independent subdriver), and something like 20 different codecs
> >     > (at least 12 different vendors; I didn't bother counting the actual
> >     > subdrivers which are all quite different). The VeriSilicon Hantro G2
> >     > codec driver is shipped by five (that we know of) vendors who
> >     all have
> >     > their own KMS drivers. One of those is in the Rockchip RK3588, which
> >     > (don't ask me why) ships six different codec blocks, with three
> >     > different drivers, from two different vendors - that's before
> >     you even
> >     > get to things like the ISP and NPU which really need to be sharing
> >     > buffers properly without copies.
> >     >
> >     > So yeah, working widely without having to encode specific knowledge
> >     > everywhere isn't a nice-to-have, it's a hard baseline requirement.
> >     >
> >     > >> > 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.
> >     >
> >     > It really does though!
> >     >
> >     > None of these devices use TTM with placement moves, and doing that
> >     > isn't a fix either. Embedded systems have so low memory
> >     bandwidth that
> >     > the difference between choosing the wrong placement and moving it
> >     > later vs. having the right placement to begin with is the difference
> >     > between 'this does not work' and 'great, I can ship this'. Which is
> >     > great if you're a consultancy trying to get paid, but tbh I'd rather
> >     > work on more interesting things.
> >     >
> >     > So yeah, userspace does very much choose the placement. On most
> >     > drivers, this is either by 'knowing' which device to allocate
> >     from, or
> >     > passing a flag to your allocation ioctl. For newer drivers though,
> >     > there's the dma-heap allocation mechanism which is now upstream and
> >     > the blessed path, for which userspace needs to explicitly know the
> >     > desired placement (and must, because fixing it up later is a
> >     > non-starter).
> >     >
> >     > Given that we need to keep LINEAR ~forever for ABI reasons, and
> >     > because there's no reasonably workable alternative, let's
> >     abandon the
> >     > idea of abandoning LINEAR, and try to work with out-of-band
> >     signalling
> >     > instead.
> >     >
> >     > One idea is to actually pursue the allocator idea and express this
> >     > properly through constraints. I'd be super in favour of this,
> >     > unsurprisingly, because it allows us to solve a whole pile of other
> >     > problems, rather than the extremely narrow AMD/Intel interop case.
> >     >
> >     > Another idea for the out-of-band signalling would be to add
> >     > information-only modifiers, like
> >     > DRM_FORMAT_MOD_LINEAR_PITCH_ALIGN_EQ(256), or
> >     > DRM_FORMAT_MOD_LINEAR_PITCH_ALIGN_GE(32). But then that doesn't
> >     really
> >     > work at all with how people actually use modifiers: as the doc
> >     > describes, userspace takes and intersects the declared modifier
> >     lists
> >     > and passes the result through. The intersection of LINEAR+EQ256 and
> >     > LINEAR+GE32 is LINEAR, so a userspace that follows the rules
> >     will just
> >     > drop the hints on the floor and pick whatever linear allocation it
> >     > feels like.
> > 
> >     Yeah I think latest when we also take into account logical image
> >     size (not
> >     just pitch) with stuff like it needs to be aligned to 2 pixels in both
> >     directions just using modifiers falls apart.
> > 
> >     And the problem with linear, unlike device modifiers is that we
> >     can't just
> >     throw up our hands and enumerate the handful of formats in actual
> >     use for
> >     interop. There's so many produces and consumers of linera buffers
> >     (Daniel's list above missed camera/image processors) that save
> >     assumption
> >     is that anything really can happen.
> > 
> >     > I think I've just talked myself into the position that passing
> >     > allocator constraints together with modifiers is the only way to
> >     > actually solve this problem, at least without creating the sort of
> >     > technical debt that meant we spent years fixing up implicit/explicit
> >     > modifier interactions when it really should've just been adding a
> >     > !)@*(#$ u64 next to the u32.
> > 
> >     Yeah probably.
> > 
> >     Otoh I know inertia, so I am tempted to go with the oddball
> >     LINEAR_VEDNOR_A_VENDOR_B_INTEROP thing and stretch the runway for
> >     a bit.
> >     And we just assign those as we go as a very special thing, and the
> >     drivers
> >     that support it would prefer it above just LINEAR if there's no other
> >     common format left.
> > 
> >     Also makes it really obvious what all userspace/kernel driver enabling
> >     would be needed to justify such a modifier.
> >     -Sima
> > 
> >     >
> >     > Cheers,
> >     > Daniel
> > 
> >     --     Simona Vetter
> >     Software Engineer, Intel Corporation
> >     http://blog.ffwll.ch
> > 
> 
> -- 
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Laurent Pinchart]

On Mon, Jan 20, 2025 at 08:58:20AM +0100, Thomas Zimmermann wrote:
> Am 18.01.25 um 03:37 schrieb Marek Olšák:
> [...]
> >
> > 3) Implementing DRM_FORMAT_MOD_LINEAR as having 256B pitch and offset 
> > alignment. This is what we do today. Even if Intel and some AMD chips 
> > can do 64B or 128B alignment, they overalign to 256B. With so many 
> > AMD+NV laptops out there, NV is probably next, unless they already do 
> > this in the closed source driver.
> 
> The dumb-buffer series currently being discussed on dri-devel also 
> touches handling of scanline pitches. THe actual value varies with each 
> driver.  Should dumb buffers use a default pitch alignment of 256 on al 
> hardware?

That may break sharing buffers with other devices (codecs, NPUs and/or
cameras) that would not support a configurable pitch. I don't expect
that to be the majority case, but I can't rule it out either. There's
also the issue that, even if the devices support configurable pitches,
the drivers may not implement it. That's fixable, but hardcoding the
pitch to 256 bytes without fixing that would be a regression.

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

On Tue, Dec 17, 2024 at 11:13:05AM +0100, 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.
> 
> > 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.

Flipping that argument, the reason why we still have no solution is
because we've constantly accepted stop-gap measures. Maybe it's time to
stop. It may feel a bit unfair to Marek that everybody until know got
away with hacks, but I don't think he would be left alone designing a
proper solution.

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

On Fri, Jan 10, 2025 at 01:23:40PM -0800, James Jones wrote:
> On 12/19/24 10:03, Simona Vetter wrote:
> > 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
> 
> I spent some time thinking about this over the break, because on a venn 
> diagram it does overlap a sliver of the work we've done to define the 
> differences between the concepts of constraints Vs. capabilities in the 
> smorgasbord of unified memory allocator talks/workshops/prototypes/etc. 
> over the years. I'm not that worried about some overlap being 
> introduced, because every reasonable rule deserves an exception here and 
> there, but I have concerns similar to Daniel's and Brian's.
> 
> Once you start adding more than one special modifier, some things in the 
> existing usage start to break down. Right now you can naively pass 
> around modifiers, then somewhere either before or just after allocation 
> depending on your usage, check if LINEAR is available and take your 
> special "I can parse this thing" path, for whatever that means in your 
> special use case. Modifying all those paths to include one variant of 
> linear is probably OK-but-not-great. Modifying all those paths to 
> include <N> variants of linear is probably unrealistic, and I do worry 
> about slippery slopes here.
> 
> ---
> 
> What got me more interested though was this led to another thought. At 
> first I didn't notice that this was an exact-match constraint and 
> thought it meant the usual alignment constraint of >=, and I was 
> concerned about how future variants would interact poorly. It could 
> still be a concern if things progress down this route, and we have 
> vendor A requiring >= 32B alignment and vendor B requiring == 64B 
> alignment. They're compatible, but modifiers expressing this would 
> naively cancel each-other out unless vendor A proactively advertised == 
> 64B linear modifiers too. This isn't a huge deal at that scale, but it 
> could get worse, and it got me thinking about a way to solve the problem 
> of a less naive way to merge modifier lists.
> 
> As a background, the two hard problems left with implementing a 
> constraint system to sit alongside the format modifier system are:
> 
> 1) A way to name special heaps (E.g., local vidmem on device A) in the 
> constraints in a way that spans process boundaries using some sort of 
> identifier. There are various ways to solve this. Lately the thinking is 
> something around dma heaps, but no one's fleshed it out yet that I'm aware.
> 
> 2) A transport that doesn't require us to revise every userspace API, 
> kernel API, and protocol that got revised to support DRM format 
> modifiers, and every API/protocol introduced since.
> 
> I haven't seen any great ideas for the latter problem yet, but what if 
> we did this:
> 
> - Introduced a new DRM format modifier vendor that was actually 
> vendor-agnostic, but implied the format modifier was a constraint 
> definition fragment instead of an actual modifier.
> 
> - Constraint-aware code could tack on its constraints (The ones it 
> requires and/or the ones it can support allocating) as a series of 
> additional modifiers using this vendor code. A given constraint might be 
> fragmented into multiple modifiers, but their definition and 
> serialization/deserialization mechanism could be defined in drm_fourcc.h 
> as macros all the clients could use.
> 
> - Existing non-constraint-aware code in a modifier usage chain might 
> filter these modifiers out using the existing strict intersection logic. 
> Hence, any link in the chain not aware of constraints would likely block 
> their use, but that's OK. We're muddling along without them now. It 
> wouldn't make those situations any worse.
> 
> - New code would be required to use some minimal library (Header-only 
> perhaps, as Simon and I proposed a few years ago) to intersect format 
> modifier lists instead, and this code would parse out the constraint 
> modifiers from each input list and use the envisioned per-constraint 
> logic to merge them. It would result in yet another merged 
> modifier+constraint list encoded as a list of modifiers that could be 
> passed along through any format-modifier-aware API.
> 
> - One consideration that would be sort of tricky is that constraints are 
> supposed to be advertised per-modifier, so you'd have to have a way to 
> associate constraint modifiers in a given set with real modifiers in 
> that set or in general. This is easily solved though. Some bits of the 
> constraint modifiers would already need to be used to associate and 
> order constraint fragments during deserialization, since modifier lists 
> aren't strictly ordered.
> 
> This effectively allows you to use format modifiers to encode 
> arbitrarily complex constraint mechanisms by piggybacking on the 
> existing format modifier definition and transport mechanisms without 
> breaking backwards compatibility. It's a little dirty, because modifiers 
> are being abused to implement a raw bitstream, but modifiers and 
> constraints are related concepts, so it's not a complete hack. It still 
> requires modifying all the implementations in the system to fully make 
> use of constraints, but doesn't require e.g. revising X11 DRI3 protocol 
> again to tunnel them through Xwayland, and in situations where the 
> constraint-aware thing sits downstream of the non-constraint-aware thing 
> in the allocation pipeline, you could get some benefit even when all the 
> upstream things aren't updated yet, because it could still merge in its 
> local constraints before allocating or passing the modifier list down 
> the chain.
> 
> Does this seem like something worth pursuing to others? I've been trying 
> to decide how to best move the allocation constraints efforts forward, 
> so it's potentially something I could put some time into this year.

It's a fairly interesting idea I hadn't thought of.

My limited experience with all the graphics protocols and their history
means I have had limited exposure to the pain that communicating
modifiers everywhere has generated. As a result, I would have (perhaps
naively) tried to design a new mechanism. Using modifiers as a transport
mechanism is clearly a hack, but it may be a clever one. It seems worth
experimenting with it at least.

After reading the whole thread, I however wonder if this will be
backward compatible. If two devices expose a constraint that ends up
being encoded in the same binary form in a modifier (let's say for
instance the same pitch alignment), isn't there a risk that an
application not aware of this new scheme will pick that common modifier
when intersecting the modifiers list as done today, without realizing
it's not a real modifier ?

-- 
Regards,

Laurent Pinchart

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

[Philipp Zabel]

Hi,

On Mo, 2025-01-20 at 08:58 +0100, Thomas Zimmermann wrote:
> Hi
> 
> 
> Am 18.01.25 um 03:37 schrieb Marek Olšák:
> [...]
> > 
> > 3) Implementing DRM_FORMAT_MOD_LINEAR as having 256B pitch and offset 
> > alignment. This is what we do today. Even if Intel and some AMD chips 
> > can do 64B or 128B alignment, they overalign to 256B. With so many 
> > AMD+NV laptops out there, NV is probably next, unless they already do 
> > this in the closed source driver.
> 
> The dumb-buffer series currently being discussed on dri-devel also 
> touches handling of scanline pitches. THe actual value varies with each 
> driver.  Should dumb buffers use a default pitch alignment of 256 on al 
> hardware?

Not all, mxsfb for example doesn't support pitch alignment.

regards
Philipp

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

[Marek Olšák]

[-- Attachment #1: Type: text/plain, Size: 1485 bytes --]

On Mon, Jan 20, 2025 at 1:41 PM Simona Vetter <simona.vetter@ffwll.ch>
wrote:

> On Mon, Jan 20, 2025 at 08:58:20AM +0100, Thomas Zimmermann wrote:
> > Hi
> >
> >
> > Am 18.01.25 um 03:37 schrieb Marek Olšák:
> > [...]
> > >
> > > 3) Implementing DRM_FORMAT_MOD_LINEAR as having 256B pitch and offset
> > > alignment. This is what we do today. Even if Intel and some AMD chips
> > > can do 64B or 128B alignment, they overalign to 256B. With so many
> > > AMD+NV laptops out there, NV is probably next, unless they already do
> > > this in the closed source driver.
>
> I don't think this works, or at least not any better than the current
> linear modifier. There's way too many users of that thing out there that I
> think you can realistically redefine it.
>

DRM_FORMAT_MOD_LINEAR was redefined on PC a long time ago to mean 256B
pitch alignment because of laptops with AMD+Intel. Drivers redefined it
because that's what happens when it's under-defined. As you say,
DRM_FORMAT_MOD_LINEAR can't be removed, but then it can't work with any
other pitch alignment on all PC hw either, so there is no other choice.

The options for PC are either a new parameterized linear modifier (with
properly defined addressing and size equations) or DRM_FORMAT_MOD_LINEAR
with 256B pitch alignment. There is no 3rd option. Even if you totally
disregard AMD, you won't get it below 128B or 64B on the rest of PC hw
anyway, and that's the same problem.

Marek

[-- Attachment #2: Type: text/html, Size: 2122 bytes --]

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

[James Jones]

On 1/20/25 14:00, Laurent Pinchart wrote:
> On Fri, Jan 10, 2025 at 01:23:40PM -0800, James Jones wrote:
>> On 12/19/24 10:03, Simona Vetter wrote:
>>> 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
>>
>> I spent some time thinking about this over the break, because on a venn
>> diagram it does overlap a sliver of the work we've done to define the
>> differences between the concepts of constraints Vs. capabilities in the
>> smorgasbord of unified memory allocator talks/workshops/prototypes/etc.
>> over the years. I'm not that worried about some overlap being
>> introduced, because every reasonable rule deserves an exception here and
>> there, but I have concerns similar to Daniel's and Brian's.
>>
>> Once you start adding more than one special modifier, some things in the
>> existing usage start to break down. Right now you can naively pass
>> around modifiers, then somewhere either before or just after allocation
>> depending on your usage, check if LINEAR is available and take your
>> special "I can parse this thing" path, for whatever that means in your
>> special use case. Modifying all those paths to include one variant of
>> linear is probably OK-but-not-great. Modifying all those paths to
>> include <N> variants of linear is probably unrealistic, and I do worry
>> about slippery slopes here.
>>
>> ---
>>
>> What got me more interested though was this led to another thought. At
>> first I didn't notice that this was an exact-match constraint and
>> thought it meant the usual alignment constraint of >=, and I was
>> concerned about how future variants would interact poorly. It could
>> still be a concern if things progress down this route, and we have
>> vendor A requiring >= 32B alignment and vendor B requiring == 64B
>> alignment. They're compatible, but modifiers expressing this would
>> naively cancel each-other out unless vendor A proactively advertised ==
>> 64B linear modifiers too. This isn't a huge deal at that scale, but it
>> could get worse, and it got me thinking about a way to solve the problem
>> of a less naive way to merge modifier lists.
>>
>> As a background, the two hard problems left with implementing a
>> constraint system to sit alongside the format modifier system are:
>>
>> 1) A way to name special heaps (E.g., local vidmem on device A) in the
>> constraints in a way that spans process boundaries using some sort of
>> identifier. There are various ways to solve this. Lately the thinking is
>> something around dma heaps, but no one's fleshed it out yet that I'm aware.
>>
>> 2) A transport that doesn't require us to revise every userspace API,
>> kernel API, and protocol that got revised to support DRM format
>> modifiers, and every API/protocol introduced since.
>>
>> I haven't seen any great ideas for the latter problem yet, but what if
>> we did this:
>>
>> - Introduced a new DRM format modifier vendor that was actually
>> vendor-agnostic, but implied the format modifier was a constraint
>> definition fragment instead of an actual modifier.
>>
>> - Constraint-aware code could tack on its constraints (The ones it
>> requires and/or the ones it can support allocating) as a series of
>> additional modifiers using this vendor code. A given constraint might be
>> fragmented into multiple modifiers, but their definition and
>> serialization/deserialization mechanism could be defined in drm_fourcc.h
>> as macros all the clients could use.
>>
>> - Existing non-constraint-aware code in a modifier usage chain might
>> filter these modifiers out using the existing strict intersection logic.
>> Hence, any link in the chain not aware of constraints would likely block
>> their use, but that's OK. We're muddling along without them now. It
>> wouldn't make those situations any worse.
>>
>> - New code would be required to use some minimal library (Header-only
>> perhaps, as Simon and I proposed a few years ago) to intersect format
>> modifier lists instead, and this code would parse out the constraint
>> modifiers from each input list and use the envisioned per-constraint
>> logic to merge them. It would result in yet another merged
>> modifier+constraint list encoded as a list of modifiers that could be
>> passed along through any format-modifier-aware API.
>>
>> - One consideration that would be sort of tricky is that constraints are
>> supposed to be advertised per-modifier, so you'd have to have a way to
>> associate constraint modifiers in a given set with real modifiers in
>> that set or in general. This is easily solved though. Some bits of the
>> constraint modifiers would already need to be used to associate and
>> order constraint fragments during deserialization, since modifier lists
>> aren't strictly ordered.
>>
>> This effectively allows you to use format modifiers to encode
>> arbitrarily complex constraint mechanisms by piggybacking on the
>> existing format modifier definition and transport mechanisms without
>> breaking backwards compatibility. It's a little dirty, because modifiers
>> are being abused to implement a raw bitstream, but modifiers and
>> constraints are related concepts, so it's not a complete hack. It still
>> requires modifying all the implementations in the system to fully make
>> use of constraints, but doesn't require e.g. revising X11 DRI3 protocol
>> again to tunnel them through Xwayland, and in situations where the
>> constraint-aware thing sits downstream of the non-constraint-aware thing
>> in the allocation pipeline, you could get some benefit even when all the
>> upstream things aren't updated yet, because it could still merge in its
>> local constraints before allocating or passing the modifier list down
>> the chain.
>>
>> Does this seem like something worth pursuing to others? I've been trying
>> to decide how to best move the allocation constraints efforts forward,
>> so it's potentially something I could put some time into this year.
> 
> It's a fairly interesting idea I hadn't thought of.
> 
> My limited experience with all the graphics protocols and their history
> means I have had limited exposure to the pain that communicating
> modifiers everywhere has generated. As a result, I would have (perhaps
> naively) tried to design a new mechanism. Using modifiers as a transport
> mechanism is clearly a hack, but it may be a clever one. It seems worth
> experimenting with it at least.
> 
> After reading the whole thread, I however wonder if this will be
> backward compatible. If two devices expose a constraint that ends up
> being encoded in the same binary form in a modifier (let's say for
> instance the same pitch alignment), isn't there a risk that an
> application not aware of this new scheme will pick that common modifier
> when intersecting the modifiers list as done today, without realizing
> it's not a real modifier ?

Yes, but they can't do anything with it besides feed it into an 
allocator in that case. In theory, the outcome would then be the same as 
before. The allocator would be:

-constraint-modifier unaware, and unaware of this modifier as a result: 
Allocation fails

-consotraint-modifier aware, and it received a modifier list with no 
actual modifiers: Allocation fails.

And without these modifiers, the intersection of modifier lists would 
have been empty, and hence allocation would trivially fail. 
Alternatively, perhaps the app passes in the empty list, and that 
triggers to allocator to use implicit modifier selection. That case may 
indeed be problematic. There could be apps misbehaving or using special 
knowledge to hand-pick modifiers that break the concept, but there are 
always bugs, and those apps are already at risk of breaking by not using 
modifiers properly.

The single explicit modifier workflow, where an app is importing an 
existing buffer, is something I haven't fully thought through though. At 
least in Vulkan, things like pitch/stride alignment wouldn't be relevant 
here because the app would also be communicating an explicit 
pitch/stride along with the explicit modifier, and things like locality 
are supposed to be discoverable via introspection on the buffer being 
imported (They're not mapping properties, but properties of the memory 
itself), but sometimes you get weird interactions (e.g., 
location-dependent alignment requirements) that might not be accounted for.

Thanks,
-James

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

[Simona Vetter]

On Tue, Jan 21, 2025 at 02:21:57PM -0500, Marek Olšák wrote:
> On Mon, Jan 20, 2025 at 1:41 PM Simona Vetter <simona.vetter@ffwll.ch>
> wrote:
> 
> > On Mon, Jan 20, 2025 at 08:58:20AM +0100, Thomas Zimmermann wrote:
> > > Hi
> > >
> > >
> > > Am 18.01.25 um 03:37 schrieb Marek Olšák:
> > > [...]
> > > >
> > > > 3) Implementing DRM_FORMAT_MOD_LINEAR as having 256B pitch and offset
> > > > alignment. This is what we do today. Even if Intel and some AMD chips
> > > > can do 64B or 128B alignment, they overalign to 256B. With so many
> > > > AMD+NV laptops out there, NV is probably next, unless they already do
> > > > this in the closed source driver.
> >
> > I don't think this works, or at least not any better than the current
> > linear modifier. There's way too many users of that thing out there that I
> > think you can realistically redefine it.
> >
> 
> DRM_FORMAT_MOD_LINEAR was redefined on PC a long time ago to mean 256B
> pitch alignment because of laptops with AMD+Intel. Drivers redefined it
> because that's what happens when it's under-defined. As you say,
> DRM_FORMAT_MOD_LINEAR can't be removed, but then it can't work with any
> other pitch alignment on all PC hw either, so there is no other choice.
> 
> The options for PC are either a new parameterized linear modifier (with
> properly defined addressing and size equations) or DRM_FORMAT_MOD_LINEAR
> with 256B pitch alignment. There is no 3rd option. Even if you totally
> disregard AMD, you won't get it below 128B or 64B on the rest of PC hw
> anyway, and that's the same problem.

Ah I missed that, but just checked in mesa, happened in 2021 apparently.
Would be really good to document this in the kernel's drm_fourcc.h
comments as the defacto rule. It's better if the docs reflect actual
reality, whatever that is.
-Sima
-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch