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@xxxxxxxxxxx>
> > > > Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > > Cc: Daniel Stone <daniel@xxxxxxxxxxxxx>
> > > > Cc: Bas Nieuwenhuizen <bas@xxxxxxxxxxxxxxxxxxx>
> > > > Cc: Dave Airlie <airlied@xxxxxxxxx>
> > > > Cc: Marek Olšák <maraeo@xxxxxxxxx>
> > > > Cc: Alex Deucher <alexdeucher@xxxxxxxxx>
> > > > Cc: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> > > > Cc: Michel Dänzer <michel@xxxxxxxxxxx>
> > > > Cc: Brian Starkey <brian.starkey@xxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
