[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 at emersion.fr>
> > > > Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > > Cc: Daniel Stone <daniel at fooishbar.org>
> > > > Cc: Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>
> > > > Cc: Dave Airlie <airlied at gmail.com>
> > > > Cc: Marek Olšák <maraeo at gmail.com>
> > > > Cc: Alex Deucher <alexdeucher at gmail.com>
> > > > Cc: Neil Armstrong <narmstrong at baylibre.com>
> > > > Cc: Michel Dänzer <michel at daenzer.net>
> > > > Cc: Brian Starkey <brian.starkey at 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