[PATCH v4] drm/fourcc: document modifier uniqueness requirements

[PATCH v4] drm/fourcc: document modifier uniqueness requirements

[Simon Ser]

There have suggestions to bake pitch alignment, address alignement,
contiguous memory or other placement (hidden VRAM, GTT/BAR, etc)
constraints into modifiers. Last time this was brought up it seemed
like the consensus was to not allow this. Document this in drm_fourcc.h.

There are several reasons for this.

- Encoding all of these constraints in the modifiers would explode the
  search space pretty quickly (we only have 64 bits to work with).
- Modifiers need to be unambiguous: a buffer can only have a single
  modifier.
- Modifier users aren't expected to parse modifiers (except drivers).

v2: add paragraph about aliases (Daniel)

v3: fix unrelated changes sent with the patch

v4: disambiguate users between driver and higher-level programs (Brian,
Daniel)

Signed-off-by: Simon Ser <contact@emersion.fr>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Daniel Stone <daniel@fooishbar.org>
Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
Cc: Dave Airlie <airlied@gmail.com>
Cc: Marek Olšák <maraeo@gmail.com>
Cc: Alex Deucher <alexdeucher@gmail.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Michel Dänzer <michel@daenzer.net>
Cc: Brian Starkey <brian.starkey@arm.com>
---
 include/uapi/drm/drm_fourcc.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 490143500a50..4d3f819dca0b 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -58,6 +58,28 @@ extern "C" {
  * may preserve meaning - such as number of planes - from the fourcc code,
  * whereas others may not.
  *
+ * 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.
+ *
+ * For modifiers where the combination of fourcc code and modifier can alias,
+ * a canonical pair needs to be defined and used by all drivers. An example
+ * is AFBC, where both ARGB and ABGR have the exact same compressed layout.
+ *
+ * There are two kinds of modifier users:
+ *
+ * - Kernel and user-space drivers: for drivers it's important that modifiers
+ *   don't alias, otherwise two drivers might support the same format but use
+ *   different aliases, preventing them from sharing buffers in an efficient
+ *   format.
+ * - Higher-level programs interfacing with KMS/GBM/EGL/Vulkan/etc: these users
+ *   see modifiers as opaque tokens they can check for equality and intersect.
+ *   These users musn't need to know to reason about the modifier value
+ *   (i.e. they are not expected to extract information out of the modifier).
+ *
  * Vendors should document their modifier usage in as much detail as
  * possible, to ensure maximum compatibility across devices, drivers and
  * applications.
-- 
2.27.0


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] drm/fourcc: document modifier uniqueness requirements

[Brian Starkey]

Hi Simon,

On Fri, Jun 19, 2020 at 11:12:25AM +0000, Simon Ser wrote:
> There have suggestions to bake pitch alignment, address alignement,
> contiguous memory or other placement (hidden VRAM, GTT/BAR, etc)
> constraints into modifiers. Last time this was brought up it seemed
> like the consensus was to not allow this. Document this in drm_fourcc.h.
> 
> There are several reasons for this.
> 
> - Encoding all of these constraints in the modifiers would explode the
>   search space pretty quickly (we only have 64 bits to work with).
> - Modifiers need to be unambiguous: a buffer can only have a single
>   modifier.
> - Modifier users aren't expected to parse modifiers (except drivers).
> 
> v2: add paragraph about aliases (Daniel)
> 
> v3: fix unrelated changes sent with the patch
> 
> v4: disambiguate users between driver and higher-level programs (Brian,
> Daniel)
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Daniel Stone <daniel@fooishbar.org>
> Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> Cc: Dave Airlie <airlied@gmail.com>
> Cc: Marek Olšák <maraeo@gmail.com>
> Cc: Alex Deucher <alexdeucher@gmail.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Michel Dänzer <michel@daenzer.net>
> Cc: Brian Starkey <brian.starkey@arm.com>
> ---
>  include/uapi/drm/drm_fourcc.h | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 490143500a50..4d3f819dca0b 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -58,6 +58,28 @@ extern "C" {
>   * may preserve meaning - such as number of planes - from the fourcc code,
>   * whereas others may not.
>   *
> + * 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.
> + *
> + * For modifiers where the combination of fourcc code and modifier can alias,
> + * a canonical pair needs to be defined and used by all drivers. An example
> + * is AFBC, where both ARGB and ABGR have the exact same compressed layout.

I still don't agree with this sentence. ARGB and ABGR have different
compressed layouts in AFBC.

Thanks,
-Brian

> + *
> + * There are two kinds of modifier users:
> + *
> + * - Kernel and user-space drivers: for drivers it's important that modifiers
> + *   don't alias, otherwise two drivers might support the same format but use
> + *   different aliases, preventing them from sharing buffers in an efficient
> + *   format.
> + * - Higher-level programs interfacing with KMS/GBM/EGL/Vulkan/etc: these users
> + *   see modifiers as opaque tokens they can check for equality and intersect.
> + *   These users musn't need to know to reason about the modifier value
> + *   (i.e. they are not expected to extract information out of the modifier).
> + *
>   * Vendors should document their modifier usage in as much detail as
>   * possible, to ensure maximum compatibility across devices, drivers and
>   * applications.
> -- 
> 2.27.0
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] drm/fourcc: document modifier uniqueness requirements

[Daniel Vetter]

On Fri, Jun 19, 2020 at 01:39:34PM +0100, Brian Starkey wrote:
> Hi Simon,
> 
> On Fri, Jun 19, 2020 at 11:12:25AM +0000, Simon Ser wrote:
> > There have suggestions to bake pitch alignment, address alignement,
> > contiguous memory or other placement (hidden VRAM, GTT/BAR, etc)
> > constraints into modifiers. Last time this was brought up it seemed
> > like the consensus was to not allow this. Document this in drm_fourcc.h.
> > 
> > There are several reasons for this.
> > 
> > - Encoding all of these constraints in the modifiers would explode the
> >   search space pretty quickly (we only have 64 bits to work with).
> > - Modifiers need to be unambiguous: a buffer can only have a single
> >   modifier.
> > - Modifier users aren't expected to parse modifiers (except drivers).
> > 
> > v2: add paragraph about aliases (Daniel)
> > 
> > v3: fix unrelated changes sent with the patch
> > 
> > v4: disambiguate users between driver and higher-level programs (Brian,
> > Daniel)
> > 
> > Signed-off-by: Simon Ser <contact@emersion.fr>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Daniel Stone <daniel@fooishbar.org>
> > Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > Cc: Dave Airlie <airlied@gmail.com>
> > Cc: Marek Olšák <maraeo@gmail.com>
> > Cc: Alex Deucher <alexdeucher@gmail.com>
> > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > Cc: Michel Dänzer <michel@daenzer.net>
> > Cc: Brian Starkey <brian.starkey@arm.com>
> > ---
> >  include/uapi/drm/drm_fourcc.h | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index 490143500a50..4d3f819dca0b 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -58,6 +58,28 @@ extern "C" {
> >   * may preserve meaning - such as number of planes - from the fourcc code,
> >   * whereas others may not.
> >   *
> > + * 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.
> > + *
> > + * For modifiers where the combination of fourcc code and modifier can alias,
> > + * a canonical pair needs to be defined and used by all drivers. An example
> > + * is AFBC, where both ARGB and ABGR have the exact same compressed layout.
> 
> I still don't agree with this sentence. ARGB and ABGR have different
> compressed layouts in AFBC.

Hm then maybe I got confused for the exact reason why afbc has defined
canonical fourcc codes in Documentation/gpu/afbc.rst? They all use the
BGR versions, no RGB anywhere to be found.

What's the reason then behind the "Formats which are not listed should be
avoided." in that doc? That's the case I wanted to refer to here.
-Daniel

> 
> Thanks,
> -Brian
> 
> > + *
> > + * There are two kinds of modifier users:
> > + *
> > + * - Kernel and user-space drivers: for drivers it's important that modifiers
> > + *   don't alias, otherwise two drivers might support the same format but use
> > + *   different aliases, preventing them from sharing buffers in an efficient
> > + *   format.
> > + * - Higher-level programs interfacing with KMS/GBM/EGL/Vulkan/etc: these users
> > + *   see modifiers as opaque tokens they can check for equality and intersect.
> > + *   These users musn't need to know to reason about the modifier value
> > + *   (i.e. they are not expected to extract information out of the modifier).
> > + *
> >   * Vendors should document their modifier usage in as much detail as
> >   * possible, to ensure maximum compatibility across devices, drivers and
> >   * applications.
> > -- 
> > 2.27.0
> > 
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] drm/fourcc: document modifier uniqueness requirements

[Brian Starkey]

On Fri, Jun 19, 2020 at 06:36:17PM +0200, Daniel Vetter wrote:
> On Fri, Jun 19, 2020 at 01:39:34PM +0100, Brian Starkey wrote:
> > Hi Simon,
> > 
> > On Fri, Jun 19, 2020 at 11:12:25AM +0000, Simon Ser wrote:
> > > There have suggestions to bake pitch alignment, address alignement,
> > > contiguous memory or other placement (hidden VRAM, GTT/BAR, etc)
> > > constraints into modifiers. Last time this was brought up it seemed
> > > like the consensus was to not allow this. Document this in drm_fourcc.h.
> > > 
> > > There are several reasons for this.
> > > 
> > > - Encoding all of these constraints in the modifiers would explode the
> > >   search space pretty quickly (we only have 64 bits to work with).
> > > - Modifiers need to be unambiguous: a buffer can only have a single
> > >   modifier.
> > > - Modifier users aren't expected to parse modifiers (except drivers).
> > > 
> > > v2: add paragraph about aliases (Daniel)
> > > 
> > > v3: fix unrelated changes sent with the patch
> > > 
> > > v4: disambiguate users between driver and higher-level programs (Brian,
> > > Daniel)
> > > 
> > > Signed-off-by: Simon Ser <contact@emersion.fr>
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Daniel Stone <daniel@fooishbar.org>
> > > Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > > Cc: Dave Airlie <airlied@gmail.com>
> > > Cc: Marek Olšák <maraeo@gmail.com>
> > > Cc: Alex Deucher <alexdeucher@gmail.com>
> > > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > > Cc: Michel Dänzer <michel@daenzer.net>
> > > Cc: Brian Starkey <brian.starkey@arm.com>
> > > ---
> > >  include/uapi/drm/drm_fourcc.h | 22 ++++++++++++++++++++++
> > >  1 file changed, 22 insertions(+)
> > > 
> > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > > index 490143500a50..4d3f819dca0b 100644
> > > --- a/include/uapi/drm/drm_fourcc.h
> > > +++ b/include/uapi/drm/drm_fourcc.h
> > > @@ -58,6 +58,28 @@ extern "C" {
> > >   * may preserve meaning - such as number of planes - from the fourcc code,
> > >   * whereas others may not.
> > >   *
> > > + * 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.
> > > + *
> > > + * For modifiers where the combination of fourcc code and modifier can alias,
> > > + * a canonical pair needs to be defined and used by all drivers. An example
> > > + * is AFBC, where both ARGB and ABGR have the exact same compressed layout.
> > 
> > I still don't agree with this sentence. ARGB and ABGR have different
> > compressed layouts in AFBC.
> 
> Hm then maybe I got confused for the exact reason why afbc has defined
> canonical fourcc codes in Documentation/gpu/afbc.rst? They all use the
> BGR versions, no RGB anywhere to be found.
> 
> What's the reason then behind the "Formats which are not listed should be
> avoided." in that doc? That's the case I wanted to refer to here.

Basically there's hardware which supports only BGR, hardware which
"ignores" swizzle (treats all as BGR), and hardware which supports
both BGR and RGB. Even amongst first-party implementations it's
inconsistent.

This leads to a potentially confusing situation where someone with
hardware which "ignores" swizzle assumes that's always the case.

To avoid that, we wanted to be explicit that ordering is important:

 | The assignment of input/output color channels must be consistent
 | between the encoder and the decoder for correct operation, otherwise
 | the consumer will interpret the decoded data incorrectly.
 | ...
 | The component ordering is communicated via the fourcc code in the
 | fourcc:modifier pair. In general, component '0' is considered to
 | reside in the least-significant bits of the corresponding linear
 | format. For example, COMP(bits):

And, to try and ensure best cross compatibility, we want BGR to be
used most often. We expect that to be supported by the most hardware:

 | For maximum compatibility across devices, the table below defines
 | canonical formats for use between AFBC-enabled devices. Formats which
 | are listed here must be used exactly as specified when using the AFBC
 | modifiers. Formats which are not listed should be avoided.

Cheers,
-Brian

> -Daniel
> 
> > 
> > Thanks,
> > -Brian
> > 
> > > + *
> > > + * There are two kinds of modifier users:
> > > + *
> > > + * - Kernel and user-space drivers: for drivers it's important that modifiers
> > > + *   don't alias, otherwise two drivers might support the same format but use
> > > + *   different aliases, preventing them from sharing buffers in an efficient
> > > + *   format.
> > > + * - Higher-level programs interfacing with KMS/GBM/EGL/Vulkan/etc: these users
> > > + *   see modifiers as opaque tokens they can check for equality and intersect.
> > > + *   These users musn't need to know to reason about the modifier value
> > > + *   (i.e. they are not expected to extract information out of the modifier).
> > > + *
> > >   * Vendors should document their modifier usage in as much detail as
> > >   * possible, to ensure maximum compatibility across devices, drivers and
> > >   * applications.
> > > -- 
> > > 2.27.0
> > > 
> > > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] drm/fourcc: document modifier uniqueness requirements

[Daniel Vetter]

On Mon, Jun 22, 2020 at 11:20:51AM +0100, Brian Starkey wrote:
> On Fri, Jun 19, 2020 at 06:36:17PM +0200, Daniel Vetter wrote:
> > On Fri, Jun 19, 2020 at 01:39:34PM +0100, Brian Starkey wrote:
> > > Hi Simon,
> > > 
> > > On Fri, Jun 19, 2020 at 11:12:25AM +0000, Simon Ser wrote:
> > > > There have suggestions to bake pitch alignment, address alignement,
> > > > contiguous memory or other placement (hidden VRAM, GTT/BAR, etc)
> > > > constraints into modifiers. Last time this was brought up it seemed
> > > > like the consensus was to not allow this. Document this in drm_fourcc.h.
> > > > 
> > > > There are several reasons for this.
> > > > 
> > > > - Encoding all of these constraints in the modifiers would explode the
> > > >   search space pretty quickly (we only have 64 bits to work with).
> > > > - Modifiers need to be unambiguous: a buffer can only have a single
> > > >   modifier.
> > > > - Modifier users aren't expected to parse modifiers (except drivers).
> > > > 
> > > > v2: add paragraph about aliases (Daniel)
> > > > 
> > > > v3: fix unrelated changes sent with the patch
> > > > 
> > > > v4: disambiguate users between driver and higher-level programs (Brian,
> > > > Daniel)
> > > > 
> > > > Signed-off-by: Simon Ser <contact@emersion.fr>
> > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Cc: Daniel Stone <daniel@fooishbar.org>
> > > > Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > > > Cc: Dave Airlie <airlied@gmail.com>
> > > > Cc: Marek Olšák <maraeo@gmail.com>
> > > > Cc: Alex Deucher <alexdeucher@gmail.com>
> > > > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > > > Cc: Michel Dänzer <michel@daenzer.net>
> > > > Cc: Brian Starkey <brian.starkey@arm.com>
> > > > ---
> > > >  include/uapi/drm/drm_fourcc.h | 22 ++++++++++++++++++++++
> > > >  1 file changed, 22 insertions(+)
> > > > 
> > > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > > > index 490143500a50..4d3f819dca0b 100644
> > > > --- a/include/uapi/drm/drm_fourcc.h
> > > > +++ b/include/uapi/drm/drm_fourcc.h
> > > > @@ -58,6 +58,28 @@ extern "C" {
> > > >   * may preserve meaning - such as number of planes - from the fourcc code,
> > > >   * whereas others may not.
> > > >   *
> > > > + * 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.
> > > > + *
> > > > + * For modifiers where the combination of fourcc code and modifier can alias,
> > > > + * a canonical pair needs to be defined and used by all drivers. An example
> > > > + * is AFBC, where both ARGB and ABGR have the exact same compressed layout.
> > > 
> > > I still don't agree with this sentence. ARGB and ABGR have different
> > > compressed layouts in AFBC.
> > 
> > Hm then maybe I got confused for the exact reason why afbc has defined
> > canonical fourcc codes in Documentation/gpu/afbc.rst? They all use the
> > BGR versions, no RGB anywhere to be found.
> > 
> > What's the reason then behind the "Formats which are not listed should be
> > avoided." in that doc? That's the case I wanted to refer to here.
> 
> Basically there's hardware which supports only BGR, hardware which
> "ignores" swizzle (treats all as BGR), and hardware which supports
> both BGR and RGB. Even amongst first-party implementations it's
> inconsistent.
> 
> This leads to a potentially confusing situation where someone with
> hardware which "ignores" swizzle assumes that's always the case.
> 
> To avoid that, we wanted to be explicit that ordering is important:
> 
>  | The assignment of input/output color channels must be consistent
>  | between the encoder and the decoder for correct operation, otherwise
>  | the consumer will interpret the decoded data incorrectly.
>  | ...
>  | The component ordering is communicated via the fourcc code in the
>  | fourcc:modifier pair. In general, component '0' is considered to
>  | reside in the least-significant bits of the corresponding linear
>  | format. For example, COMP(bits):
> 
> And, to try and ensure best cross compatibility, we want BGR to be
> used most often. We expect that to be supported by the most hardware:
> 
>  | For maximum compatibility across devices, the table below defines
>  | canonical formats for use between AFBC-enabled devices. Formats which
>  | are listed here must be used exactly as specified when using the AFBC
>  | modifiers. Formats which are not listed should be avoided.

Ok, so not such an example. Simon, maybe we could go with something like:

 * For modifiers where the combination of fourcc code and modifier can alias,
 * a canonical pair needs to be defined and used by all drivers. Preferred
 * combinations are also encourage where all combinations might
 * lead to confusion and unecessarily reduced interoperability. An example
 * for the latter is AFBC, where the ABGR layouts are preferred over ARGB
 * layouts.

Brian, would that be clear from your side too?

Thanks, Daniel

> 
> Cheers,
> -Brian
> 
> > -Daniel
> > 
> > > 
> > > Thanks,
> > > -Brian
> > > 
> > > > + *
> > > > + * There are two kinds of modifier users:
> > > > + *
> > > > + * - Kernel and user-space drivers: for drivers it's important that modifiers
> > > > + *   don't alias, otherwise two drivers might support the same format but use
> > > > + *   different aliases, preventing them from sharing buffers in an efficient
> > > > + *   format.
> > > > + * - Higher-level programs interfacing with KMS/GBM/EGL/Vulkan/etc: these users
> > > > + *   see modifiers as opaque tokens they can check for equality and intersect.
> > > > + *   These users musn't need to know to reason about the modifier value
> > > > + *   (i.e. they are not expected to extract information out of the modifier).
> > > > + *
> > > >   * Vendors should document their modifier usage in as much detail as
> > > >   * possible, to ensure maximum compatibility across devices, drivers and
> > > >   * applications.
> > > > -- 
> > > > 2.27.0
> > > > 
> > > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] drm/fourcc: document modifier uniqueness requirements

[Brian Starkey]

On Tue, Jun 23, 2020 at 10:45:51AM +0200, Daniel Vetter wrote:
> On Mon, Jun 22, 2020 at 11:20:51AM +0100, Brian Starkey wrote:
> > On Fri, Jun 19, 2020 at 06:36:17PM +0200, Daniel Vetter wrote:
> > > On Fri, Jun 19, 2020 at 01:39:34PM +0100, Brian Starkey wrote:
> > > > Hi Simon,
> > > > 
> > > > On Fri, Jun 19, 2020 at 11:12:25AM +0000, Simon Ser wrote:
> > > > > There have suggestions to bake pitch alignment, address alignement,
> > > > > contiguous memory or other placement (hidden VRAM, GTT/BAR, etc)
> > > > > constraints into modifiers. Last time this was brought up it seemed
> > > > > like the consensus was to not allow this. Document this in drm_fourcc.h.
> > > > > 
> > > > > There are several reasons for this.
> > > > > 
> > > > > - Encoding all of these constraints in the modifiers would explode the
> > > > >   search space pretty quickly (we only have 64 bits to work with).
> > > > > - Modifiers need to be unambiguous: a buffer can only have a single
> > > > >   modifier.
> > > > > - Modifier users aren't expected to parse modifiers (except drivers).
> > > > > 
> > > > > v2: add paragraph about aliases (Daniel)
> > > > > 
> > > > > v3: fix unrelated changes sent with the patch
> > > > > 
> > > > > v4: disambiguate users between driver and higher-level programs (Brian,
> > > > > Daniel)
> > > > > 
> > > > > Signed-off-by: Simon Ser <contact@emersion.fr>
> > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > Cc: Daniel Stone <daniel@fooishbar.org>
> > > > > Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> > > > > Cc: Dave Airlie <airlied@gmail.com>
> > > > > Cc: Marek Olšák <maraeo@gmail.com>
> > > > > Cc: Alex Deucher <alexdeucher@gmail.com>
> > > > > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > > > > Cc: Michel Dänzer <michel@daenzer.net>
> > > > > Cc: Brian Starkey <brian.starkey@arm.com>
> > > > > ---
> > > > >  include/uapi/drm/drm_fourcc.h | 22 ++++++++++++++++++++++
> > > > >  1 file changed, 22 insertions(+)
> > > > > 
> > > > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > > > > index 490143500a50..4d3f819dca0b 100644
> > > > > --- a/include/uapi/drm/drm_fourcc.h
> > > > > +++ b/include/uapi/drm/drm_fourcc.h
> > > > > @@ -58,6 +58,28 @@ extern "C" {
> > > > >   * may preserve meaning - such as number of planes - from the fourcc code,
> > > > >   * whereas others may not.
> > > > >   *
> > > > > + * 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.
> > > > > + *
> > > > > + * For modifiers where the combination of fourcc code and modifier can alias,
> > > > > + * a canonical pair needs to be defined and used by all drivers. An example
> > > > > + * is AFBC, where both ARGB and ABGR have the exact same compressed layout.
> > > > 
> > > > I still don't agree with this sentence. ARGB and ABGR have different
> > > > compressed layouts in AFBC.
> > > 
> > > Hm then maybe I got confused for the exact reason why afbc has defined
> > > canonical fourcc codes in Documentation/gpu/afbc.rst? They all use the
> > > BGR versions, no RGB anywhere to be found.
> > > 
> > > What's the reason then behind the "Formats which are not listed should be
> > > avoided." in that doc? That's the case I wanted to refer to here.
> > 
> > Basically there's hardware which supports only BGR, hardware which
> > "ignores" swizzle (treats all as BGR), and hardware which supports
> > both BGR and RGB. Even amongst first-party implementations it's
> > inconsistent.
> > 
> > This leads to a potentially confusing situation where someone with
> > hardware which "ignores" swizzle assumes that's always the case.
> > 
> > To avoid that, we wanted to be explicit that ordering is important:
> > 
> >  | The assignment of input/output color channels must be consistent
> >  | between the encoder and the decoder for correct operation, otherwise
> >  | the consumer will interpret the decoded data incorrectly.
> >  | ...
> >  | The component ordering is communicated via the fourcc code in the
> >  | fourcc:modifier pair. In general, component '0' is considered to
> >  | reside in the least-significant bits of the corresponding linear
> >  | format. For example, COMP(bits):
> > 
> > And, to try and ensure best cross compatibility, we want BGR to be
> > used most often. We expect that to be supported by the most hardware:
> > 
> >  | For maximum compatibility across devices, the table below defines
> >  | canonical formats for use between AFBC-enabled devices. Formats which
> >  | are listed here must be used exactly as specified when using the AFBC
> >  | modifiers. Formats which are not listed should be avoided.
> 
> Ok, so not such an example. Simon, maybe we could go with something like:
> 
>  * For modifiers where the combination of fourcc code and modifier can alias,
>  * a canonical pair needs to be defined and used by all drivers. Preferred
>  * combinations are also encourage where all combinations might
>  * lead to confusion and unecessarily reduced interoperability. An example
>  * for the latter is AFBC, where the ABGR layouts are preferred over ARGB
>  * layouts.
> 
> Brian, would that be clear from your side too?

Yep, LGTM. Thanks!

> 
> Thanks, Daniel
> 
> > 
> > Cheers,
> > -Brian
> > 
> > > -Daniel
> > > 
> > > > 
> > > > Thanks,
> > > > -Brian
> > > > 
> > > > > + *
> > > > > + * There are two kinds of modifier users:
> > > > > + *
> > > > > + * - Kernel and user-space drivers: for drivers it's important that modifiers
> > > > > + *   don't alias, otherwise two drivers might support the same format but use
> > > > > + *   different aliases, preventing them from sharing buffers in an efficient
> > > > > + *   format.
> > > > > + * - Higher-level programs interfacing with KMS/GBM/EGL/Vulkan/etc: these users
> > > > > + *   see modifiers as opaque tokens they can check for equality and intersect.
> > > > > + *   These users musn't need to know to reason about the modifier value
> > > > > + *   (i.e. they are not expected to extract information out of the modifier).
> > > > > + *
> > > > >   * Vendors should document their modifier usage in as much detail as
> > > > >   * possible, to ensure maximum compatibility across devices, drivers and
> > > > >   * applications.
> > > > > -- 
> > > > > 2.27.0
> > > > > 
> > > > > 
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel