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@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?
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
