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

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