[v5] drm/fourcc: document modifier uniqueness requirements

Commit Message

Simon Ser June 23, 2020, 3:25 p.m. UTC

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)

v5: fix AFBC example (Brian, Daniel)

Signed-off-by: Simon Ser <[e-mail]>
Reviewed-by: Daniel Vetter <[e-mail]>
Cc: Daniel Stone <[e-mail]>
Cc: Bas Nieuwenhuizen <[e-mail]>
Cc: Dave Airlie <[e-mail]>
Cc: Marek Olšák <[e-mail]>
Cc: Alex Deucher <[e-mail]>
Cc: Neil Armstrong <[e-mail]>
Cc: Michel Dänzer <[e-mail]>
Cc: Brian Starkey <[e-mail]>
---
 include/uapi/drm/drm_fourcc.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Brian Starkey June 24, 2020, 11:08 a.m. UTC | #1

Hi,

On Tue, Jun 23, 2020 at 03:25:08PM +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)
> 
> v5: fix AFBC example (Brian, Daniel)
> 
> Signed-off-by: Simon Ser <[e-mail]>
> Reviewed-by: Daniel Vetter <[e-mail]>
> Cc: Daniel Stone <[e-mail]>
> Cc: Bas Nieuwenhuizen <[e-mail]>
> Cc: Dave Airlie <[e-mail]>
> Cc: Marek Olšák <[e-mail]>
> Cc: Alex Deucher <[e-mail]>
> Cc: Neil Armstrong <[e-mail]>
> Cc: Michel Dänzer <[e-mail]>
> Cc: Brian Starkey <[e-mail]>
> ---
>  include/uapi/drm/drm_fourcc.h | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 490143500a50..8296197189bf 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -58,6 +58,34 @@ 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.

The new paragraph below looks good, but this sentence from the end of
the paragraph above still needs to be removed:

  An example is AFBC, where both ARGB and ABGR have the exact same compressed layout.

With that fixed:

Reviewed-by: Brian Starkey <[e-mail]>

Thanks,
-Brian

> + *
> + * 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 encouraged where all combinations might lead to
> + * confusion and unnecessarily reduced interoperability. An example for the
> + * latter is AFBC, where the ABGR layouts are preferred over ARGB layouts.
> + *
> + * 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 June 24, 2020, 12:58 p.m. UTC | #2

On Wed, Jun 24, 2020 at 1:08 PM Brian Starkey <[e-mail]> wrote:
>
> Hi,
>
> On Tue, Jun 23, 2020 at 03:25:08PM +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)
> >
> > v5: fix AFBC example (Brian, Daniel)
> >
> > Signed-off-by: Simon Ser <[e-mail]>
> > Reviewed-by: Daniel Vetter <[e-mail]>
> > Cc: Daniel Stone <[e-mail]>
> > Cc: Bas Nieuwenhuizen <[e-mail]>
> > Cc: Dave Airlie <[e-mail]>
> > Cc: Marek Olšák <[e-mail]>
> > Cc: Alex Deucher <[e-mail]>
> > Cc: Neil Armstrong <[e-mail]>
> > Cc: Michel Dänzer <[e-mail]>
> > Cc: Brian Starkey <[e-mail]>
> > ---
> >  include/uapi/drm/drm_fourcc.h | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index 490143500a50..8296197189bf 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -58,6 +58,34 @@ 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.
>
> The new paragraph below looks good, but this sentence from the end of
> the paragraph above still needs to be removed:
>
>   An example is AFBC, where both ARGB and ABGR have the exact same compressed layout.

I think that entire paragraph was meant to be deleted, the replacement
is the new one below. Otherwise we have 2x the exact same sentence :-)
-Daniel

>
> With that fixed:
>
> Reviewed-by: Brian Starkey <[e-mail]>
>
> Thanks,
> -Brian
>
> > + *
> > + * 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 encouraged where all combinations might lead to
> > + * confusion and unnecessarily reduced interoperability. An example for the
> > + * latter is AFBC, where the ABGR layouts are preferred over ARGB layouts.
> > + *
> > + * 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
> >
> >

Simon Ser June 24, 2020, 1 p.m. UTC | #3

> > The new paragraph below looks good, but this sentence from the end of
> > the paragraph above still needs to be removed:
> > An example is AFBC, where both ARGB and ABGR have the exact same compressed layout.
>
> I think that entire paragraph was meant to be deleted, the replacement
> is the new one below. Otherwise we have 2x the exact same sentence :-)

Err, indeed, sorry about that! Sending a fixed version.

Patch

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 490143500a50..8296197189bf 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -58,6 +58,34 @@  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.
+ *
+ * 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 encouraged where all combinations might lead to
+ * confusion and unnecessarily reduced interoperability. An example for the
+ * latter is AFBC, where the ABGR layouts are preferred over ARGB layouts.
+ *
+ * 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.