diff mbox series

drm/fourcc: Add DOC: overview comment

Message ID 20180821161611.10424-1-brian.starkey@arm.com (mailing list archive)
State New, archived
Headers show
Series drm/fourcc: Add DOC: overview comment | expand

Commit Message

Brian Starkey Aug. 21, 2018, 4:16 p.m. UTC
There's a number of things which haven't previously been documented
around the usage of format modifiers. Capture the current
understanding in an overview comment and add it to the rst
documentation.

Ideally, the generated documentation would also include documentation
of all of the #defines, but the kernel-doc system doesn't currently
support kernel-doc comments on #define constants.

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 Documentation/gpu/drm-kms.rst |  6 ++++++
 include/uapi/drm/drm_fourcc.h | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

Comments

Matthew Wilcox (Oracle) Aug. 21, 2018, 4:26 p.m. UTC | #1
On Tue, Aug 21, 2018 at 05:16:11PM +0100, Brian Starkey wrote:
> There's a number of things which haven't previously been documented
> around the usage of format modifiers. Capture the current
> understanding in an overview comment and add it to the rst
> documentation.
> 
> Ideally, the generated documentation would also include documentation
> of all of the #defines, but the kernel-doc system doesn't currently
> support kernel-doc comments on #define constants.

Can you turn them into enums?  This seems to work ok:

-/* color index */
-#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
-
-/* 8 bpp Red */
-#define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
+enum {
+       /* color index */
+       DRM_FORMAT_C8   = fourcc_code('C', '8', ' ', ' '), /* [7:0] C */
+       /* 8 bpp Red */
+       DRM_FORMAT_R8   = fourcc_code('R', '8', ' ', ' '), /* [7:0] R */
+};

but I appreciate this is user API and maybe there's some code out there
that does #ifndef DRM_FORMAT_C8 ...
Brian Starkey Aug. 21, 2018, 4:44 p.m. UTC | #2
Hi Matthew,

On Tue, Aug 21, 2018 at 09:26:39AM -0700, Matthew Wilcox wrote:
>On Tue, Aug 21, 2018 at 05:16:11PM +0100, Brian Starkey wrote:
>> There's a number of things which haven't previously been documented
>> around the usage of format modifiers. Capture the current
>> understanding in an overview comment and add it to the rst
>> documentation.
>>
>> Ideally, the generated documentation would also include documentation
>> of all of the #defines, but the kernel-doc system doesn't currently
>> support kernel-doc comments on #define constants.
>
>Can you turn them into enums?  This seems to work ok:
>
>-/* color index */
>-#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
>-
>-/* 8 bpp Red */
>-#define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
>+enum {
>+       /* color index */
>+       DRM_FORMAT_C8   = fourcc_code('C', '8', ' ', ' '), /* [7:0] C */
>+       /* 8 bpp Red */
>+       DRM_FORMAT_R8   = fourcc_code('R', '8', ' ', ' '), /* [7:0] R */
>+};
>
>but I appreciate this is user API and maybe there's some code out there
>that does #ifndef DRM_FORMAT_C8 ...

Thanks for the suggestion, Daniel did mention the same. However,
unfortunately I don't think we can safely change the UAPI header in
this manner.

Cheers,
-Brian
Daniel Vetter Aug. 21, 2018, 4:51 p.m. UTC | #3
On Tue, Aug 21, 2018 at 05:16:11PM +0100, Brian Starkey wrote:
> There's a number of things which haven't previously been documented
> around the usage of format modifiers. Capture the current
> understanding in an overview comment and add it to the rst
> documentation.
> 
> Ideally, the generated documentation would also include documentation
> of all of the #defines, but the kernel-doc system doesn't currently
> support kernel-doc comments on #define constants.
> 
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> ---
>  Documentation/gpu/drm-kms.rst |  6 ++++++
>  include/uapi/drm/drm_fourcc.h | 36 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 1dffd1ac4cd4..1dd9f4824d3b 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -322,6 +322,12 @@ Frame Buffer Functions Reference
>  DRM Format Handling
>  ===================
>  
> +.. kernel-doc:: include/uapi/drm/drm_fourcc.h
> +   :doc: overview
> +
> +Format Functions Reference
> +--------------------------
> +
>  .. kernel-doc:: include/drm/drm_fourcc.h
>     :internal:
>  
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 894fa2da32fd..3f6c0b499323 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -30,6 +30,42 @@
>  extern "C" {
>  #endif
>  
> +/**
> + * DOC: overview
> + *
> + * In the DRM subsystem, framebuffer pixel formats are described using the
> + * fourcc codes defined in `include/uapi/drm/drm_fourcc.h`. In addition to the
> + * fourcc code, a Format Modifier may optionally be provided, in order to
> + * further describe the buffer's format - for example tiling or compression.
> + *
> + * Format Modifiers
> + * ----------------
> + *
> + * Format modifiers are used in conjunction with a fourcc code, forming a
> + * unique fourcc:modifier pair. This format:modifier pair must fully define the
> + * format and data layout of the buffer, and should be the only way to describe
> + * that particular buffer.
> + *
> + * Having multiple fourcc:modifier pairs which describe the same layout should
> + * be avoided, as such aliases run the risk of different drivers exposing
> + * different names for the same data format, forcing userspace to understand
> + * that they are aliases.
> + *
> + * Format modifiers may change any property of the buffer, including the number
> + * of planes and/or the required allocation size. Format modifiers are
> + * vendor-namespaced, and as such the relationship between a fourcc code and a
> + * modifier is specific to the modifer being used. For example, some modifiers
> + * may preserve meaning - such as number of planes - from the fourcc code,
> + * whereas others may not.
> + *
> + * Vendors are encouraged to document their modifier usage in as much detail as

I'd go with a slightly stronger "... should document ..." but either way:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I'll leave pushing to one of the arm committers for drm-misc.
-Daniel

> + * possible, to ensure maximum compatibility across devices, drivers and
> + * applications.
> + *
> + * The authoritative list of format modifier codes is found in
> + * `include/uapi/drm/drm_fourcc.h`
> + */
> +
>  #define fourcc_code(a, b, c, d) ((__u32)(a) | ((__u32)(b) << 8) | \
>  				 ((__u32)(c) << 16) | ((__u32)(d) << 24))
>  
> -- 
> 2.16.1
>
Alexandru-Cosmin Gheorghe Aug. 21, 2018, 5:09 p.m. UTC | #4
On Tue, Aug 21, 2018 at 06:51:26PM +0200, Daniel Vetter wrote:
> On Tue, Aug 21, 2018 at 05:16:11PM +0100, Brian Starkey wrote:
> > There's a number of things which haven't previously been documented
> > around the usage of format modifiers. Capture the current
> > understanding in an overview comment and add it to the rst
> > documentation.
> > 
> > Ideally, the generated documentation would also include documentation
> > of all of the #defines, but the kernel-doc system doesn't currently
> > support kernel-doc comments on #define constants.
> > 
> > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> > ---
> >  Documentation/gpu/drm-kms.rst |  6 ++++++
> >  include/uapi/drm/drm_fourcc.h | 36 ++++++++++++++++++++++++++++++++++++
> >  2 files changed, 42 insertions(+)
> > 
> > diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> > index 1dffd1ac4cd4..1dd9f4824d3b 100644
> > --- a/Documentation/gpu/drm-kms.rst
> > +++ b/Documentation/gpu/drm-kms.rst
> > @@ -322,6 +322,12 @@ Frame Buffer Functions Reference
> >  DRM Format Handling
> >  ===================
> >  
> > +.. kernel-doc:: include/uapi/drm/drm_fourcc.h
> > +   :doc: overview
> > +
> > +Format Functions Reference
> > +--------------------------
> > +
> >  .. kernel-doc:: include/drm/drm_fourcc.h
> >     :internal:
> >  
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index 894fa2da32fd..3f6c0b499323 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -30,6 +30,42 @@
> >  extern "C" {
> >  #endif
> >  
> > +/**
> > + * DOC: overview
> > + *
> > + * In the DRM subsystem, framebuffer pixel formats are described using the
> > + * fourcc codes defined in `include/uapi/drm/drm_fourcc.h`. In addition to the
> > + * fourcc code, a Format Modifier may optionally be provided, in order to
> > + * further describe the buffer's format - for example tiling or compression.
> > + *
> > + * Format Modifiers
> > + * ----------------
> > + *
> > + * Format modifiers are used in conjunction with a fourcc code, forming a
> > + * unique fourcc:modifier pair. This format:modifier pair must fully define the
> > + * format and data layout of the buffer, and should be the only way to describe
> > + * that particular buffer.
> > + *
> > + * Having multiple fourcc:modifier pairs which describe the same layout should
> > + * be avoided, as such aliases run the risk of different drivers exposing
> > + * different names for the same data format, forcing userspace to understand
> > + * that they are aliases.
> > + *
> > + * Format modifiers may change any property of the buffer, including the number
> > + * of planes and/or the required allocation size. Format modifiers are
> > + * vendor-namespaced, and as such the relationship between a fourcc code and a
> > + * modifier is specific to the modifer being used. For example, some modifiers
> > + * may preserve meaning - such as number of planes - from the fourcc code,
> > + * whereas others may not.
> > + *
> > + * Vendors are encouraged to document their modifier usage in as much detail as
> 
> I'd go with a slightly stronger "... should document ..." but either way:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> I'll leave pushing to one of the arm committers for drm-misc.
> -Daniel

Done.

> 
> > + * possible, to ensure maximum compatibility across devices, drivers and
> > + * applications.
> > + *
> > + * The authoritative list of format modifier codes is found in
> > + * `include/uapi/drm/drm_fourcc.h`
> > + */
> > +
> >  #define fourcc_code(a, b, c, d) ((__u32)(a) | ((__u32)(b) << 8) | \
> >  				 ((__u32)(c) << 16) | ((__u32)(d) << 24))
> >  
> > -- 
> > 2.16.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Eric Engestrom Aug. 22, 2018, 2:59 p.m. UTC | #5
On Tuesday, 2018-08-21 17:44:17 +0100, Brian Starkey wrote:
> Hi Matthew,
> 
> On Tue, Aug 21, 2018 at 09:26:39AM -0700, Matthew Wilcox wrote:
> > On Tue, Aug 21, 2018 at 05:16:11PM +0100, Brian Starkey wrote:
> > > There's a number of things which haven't previously been documented
> > > around the usage of format modifiers. Capture the current
> > > understanding in an overview comment and add it to the rst
> > > documentation.
> > > 
> > > Ideally, the generated documentation would also include documentation
> > > of all of the #defines, but the kernel-doc system doesn't currently
> > > support kernel-doc comments on #define constants.
> > 
> > Can you turn them into enums?  This seems to work ok:
> > 
> > -/* color index */
> > -#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> > -
> > -/* 8 bpp Red */
> > -#define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
> > +enum {
> > +       /* color index */
> > +       DRM_FORMAT_C8   = fourcc_code('C', '8', ' ', ' '), /* [7:0] C */
> > +       /* 8 bpp Red */
> > +       DRM_FORMAT_R8   = fourcc_code('R', '8', ' ', ' '), /* [7:0] R */
> > +};
> > 
> > but I appreciate this is user API and maybe there's some code out there
> > that does #ifndef DRM_FORMAT_C8 ...
> 
> Thanks for the suggestion, Daniel did mention the same. However,
> unfortunately I don't think we can safely change the UAPI header in
> this manner.

You could get the best of both worlds by doing both:

  enum {
    foo = fourcc(...),
    bar = fourcc(...),
  }
  #define foo foo
  #define bar bar

It would mean a bit more code though, but that way these would now be
enums (with all the advantages of enums vs plain literals) and still
pass #ifdef checks :)

(BTW, on the "maybe there's some code that does #ifdef": I can tell you
there is indeed, having written this myself for an out-of-tree driver
for customer-modified kernels that may contain additional formats)

> 
> Cheers,
> -Brian
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter Aug. 22, 2018, 3:11 p.m. UTC | #6
On Wed, Aug 22, 2018 at 4:59 PM, Eric Engestrom
<eric.engestrom@intel.com> wrote:
> On Tuesday, 2018-08-21 17:44:17 +0100, Brian Starkey wrote:
>> Hi Matthew,
>>
>> On Tue, Aug 21, 2018 at 09:26:39AM -0700, Matthew Wilcox wrote:
>> > On Tue, Aug 21, 2018 at 05:16:11PM +0100, Brian Starkey wrote:
>> > > There's a number of things which haven't previously been documented
>> > > around the usage of format modifiers. Capture the current
>> > > understanding in an overview comment and add it to the rst
>> > > documentation.
>> > >
>> > > Ideally, the generated documentation would also include documentation
>> > > of all of the #defines, but the kernel-doc system doesn't currently
>> > > support kernel-doc comments on #define constants.
>> >
>> > Can you turn them into enums?  This seems to work ok:
>> >
>> > -/* color index */
>> > -#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
>> > -
>> > -/* 8 bpp Red */
>> > -#define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
>> > +enum {
>> > +       /* color index */
>> > +       DRM_FORMAT_C8   = fourcc_code('C', '8', ' ', ' '), /* [7:0] C */
>> > +       /* 8 bpp Red */
>> > +       DRM_FORMAT_R8   = fourcc_code('R', '8', ' ', ' '), /* [7:0] R */
>> > +};
>> >
>> > but I appreciate this is user API and maybe there's some code out there
>> > that does #ifndef DRM_FORMAT_C8 ...
>>
>> Thanks for the suggestion, Daniel did mention the same. However,
>> unfortunately I don't think we can safely change the UAPI header in
>> this manner.
>
> You could get the best of both worlds by doing both:
>
>   enum {
>     foo = fourcc(...),
>     bar = fourcc(...),
>   }
>   #define foo foo
>   #define bar bar
>
> It would mean a bit more code though, but that way these would now be
> enums (with all the advantages of enums vs plain literals) and still
> pass #ifdef checks :)
>
> (BTW, on the "maybe there's some code that does #ifdef": I can tell you
> there is indeed, having written this myself for an out-of-tree driver
> for customer-modified kernels that may contain additional formats)

Looks reasonable. I'd even put the #define right within each enum line
(as a reminder so people don't forget to add them. Would happily ack a
patch to mass-convert, if that ups the odds of good kerneldoc for all
this.

enum also should support the inline style of kerneldoc (otherwise I
guess we'd need to fix that first, or it just makes no sense at all).
-Daniel
Brian Starkey Aug. 22, 2018, 3:57 p.m. UTC | #7
Hi,

On Wed, Aug 22, 2018 at 05:11:55PM +0200, Daniel Vetter wrote:
>On Wed, Aug 22, 2018 at 4:59 PM, Eric Engestrom
><eric.engestrom@intel.com> wrote:
>> On Tuesday, 2018-08-21 17:44:17 +0100, Brian Starkey wrote:
>>> Hi Matthew,
>>>
>>> On Tue, Aug 21, 2018 at 09:26:39AM -0700, Matthew Wilcox wrote:
>>> > On Tue, Aug 21, 2018 at 05:16:11PM +0100, Brian Starkey wrote:
>>> > > There's a number of things which haven't previously been documented
>>> > > around the usage of format modifiers. Capture the current
>>> > > understanding in an overview comment and add it to the rst
>>> > > documentation.
>>> > >
>>> > > Ideally, the generated documentation would also include documentation
>>> > > of all of the #defines, but the kernel-doc system doesn't currently
>>> > > support kernel-doc comments on #define constants.
>>> >
>>> > Can you turn them into enums?  This seems to work ok:
>>> >
>>> > -/* color index */
>>> > -#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
>>> > -
>>> > -/* 8 bpp Red */
>>> > -#define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
>>> > +enum {
>>> > +       /* color index */
>>> > +       DRM_FORMAT_C8   = fourcc_code('C', '8', ' ', ' '), /* [7:0] C */
>>> > +       /* 8 bpp Red */
>>> > +       DRM_FORMAT_R8   = fourcc_code('R', '8', ' ', ' '), /* [7:0] R */
>>> > +};
>>> >
>>> > but I appreciate this is user API and maybe there's some code out there
>>> > that does #ifndef DRM_FORMAT_C8 ...
>>>
>>> Thanks for the suggestion, Daniel did mention the same. However,
>>> unfortunately I don't think we can safely change the UAPI header in
>>> this manner.
>>
>> You could get the best of both worlds by doing both:
>>
>>   enum {
>>     foo = fourcc(...),
>>     bar = fourcc(...),
>>   }
>>   #define foo foo
>>   #define bar bar
>>
>> It would mean a bit more code though, but that way these would now be
>> enums (with all the advantages of enums vs plain literals) and still
>> pass #ifdef checks :)
>>
>> (BTW, on the "maybe there's some code that does #ifdef": I can tell you
>> there is indeed, having written this myself for an out-of-tree driver
>> for customer-modified kernels that may contain additional formats)
>
>Looks reasonable. I'd even put the #define right within each enum line
>(as a reminder so people don't forget to add them. Would happily ack a
>patch to mass-convert, if that ups the odds of good kerneldoc for all
>this.
>
>enum also should support the inline style of kerneldoc (otherwise I
>guess we'd need to fix that first, or it just makes no sense at all).
>-Daniel

I'm not sure that swapping out explicit 32-bit unsigned integers for
enums (unspecified width, signed integers) is necessarily a good idea,
it seems like Bad Things could happen.

The C spec says:

   "the value of an enumeration constant shall be an integer constant
   expression that has a value representable as an int"

Which likely gives us 4 bytes to play with on all machines
that run Linux, but if drm_fourcc.h is ever going to be some kind of
standard reference, making it non-portable seems like a fail.

And even if you do have 4 bytes in an enum, signed integers act
differently from unsigned ones, and compilers do love to invoke the UB
clause...

Cheers,
-Brian

>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Matthew Wilcox (Oracle) Aug. 23, 2018, 2:34 p.m. UTC | #8
On Wed, Aug 22, 2018 at 04:57:33PM +0100, Brian Starkey wrote:
> On Wed, Aug 22, 2018 at 05:11:55PM +0200, Daniel Vetter wrote:
> > On Wed, Aug 22, 2018 at 4:59 PM, Eric Engestrom
> > <eric.engestrom@intel.com> wrote:
> > > On Tuesday, 2018-08-21 17:44:17 +0100, Brian Starkey wrote:
> > > > On Tue, Aug 21, 2018 at 09:26:39AM -0700, Matthew Wilcox wrote:
> > > > > Can you turn them into enums?  This seems to work ok:
> 
> I'm not sure that swapping out explicit 32-bit unsigned integers for
> enums (unspecified width, signed integers) is necessarily a good idea,
> it seems like Bad Things could happen.
> 
> The C spec says:
> 
>   "the value of an enumeration constant shall be an integer constant
>   expression that has a value representable as an int"
> 
> Which likely gives us 4 bytes to play with on all machines
> that run Linux, but if drm_fourcc.h is ever going to be some kind of
> standard reference, making it non-portable seems like a fail.
>
> And even if you do have 4 bytes in an enum, signed integers act
> differently from unsigned ones, and compilers do love to invoke the UB
> clause...

I think you're exaggerating how much latitude C compilers have here.
Further down in 6.7.2.2, it says:

  Each enumerated type shall be compatible with char, a signed
  integer type, or an unsigned integer type.  The choice of type is
  implementation-defined, but shall be capable of representing the values
  of all the members of the enumeration.

So if we include an integer which isn't representable in a plain int,
then the compiler _must_ choose a larger type.  It could choose a
signed-64-bit type rather than an unsigned-32-bit type, but I can't
imagine any compiler being quite so insane.
Brian Starkey Aug. 23, 2018, 3:40 p.m. UTC | #9
Hi Matthew,

On Thu, Aug 23, 2018 at 07:34:45AM -0700, Matthew Wilcox wrote:
>On Wed, Aug 22, 2018 at 04:57:33PM +0100, Brian Starkey wrote:
>> On Wed, Aug 22, 2018 at 05:11:55PM +0200, Daniel Vetter wrote:
>> > On Wed, Aug 22, 2018 at 4:59 PM, Eric Engestrom
>> > <eric.engestrom@intel.com> wrote:
>> > > On Tuesday, 2018-08-21 17:44:17 +0100, Brian Starkey wrote:
>> > > > On Tue, Aug 21, 2018 at 09:26:39AM -0700, Matthew Wilcox wrote:
>> > > > > Can you turn them into enums?  This seems to work ok:
>>
>> I'm not sure that swapping out explicit 32-bit unsigned integers for
>> enums (unspecified width, signed integers) is necessarily a good idea,
>> it seems like Bad Things could happen.
>>
>> The C spec says:
>>
>>   "the value of an enumeration constant shall be an integer constant
>>   expression that has a value representable as an int"
>>
>> Which likely gives us 4 bytes to play with on all machines
>> that run Linux, but if drm_fourcc.h is ever going to be some kind of
>> standard reference, making it non-portable seems like a fail.
>>
>> And even if you do have 4 bytes in an enum, signed integers act
>> differently from unsigned ones, and compilers do love to invoke the UB
>> clause...
>
>I think you're exaggerating how much latitude C compilers have here.
>Further down in 6.7.2.2, it says:
>
>  Each enumerated type shall be compatible with char, a signed
>  integer type, or an unsigned integer type.  The choice of type is
>  implementation-defined, but shall be capable of representing the values
>  of all the members of the enumeration.
>
>So if we include an integer which isn't representable in a plain int,
>then the compiler _must_ choose a larger type.

I don't think so... the sentence I pasted says that including a value
which isn't representable in a plain int would be illegal, and so the
compiler doesn't _have_ to do anything (nasal demons, right?).

>It could choose a
>signed-64-bit type rather than an unsigned-32-bit type, but I can't
>imagine any compiler being quite so insane.

The paragraph about the implementation choosing a representation is
separate from the valid range of values - the compiler can pick
whatever storage it likes (smaller or even larger than an int), so
long as that storage can fit all the defined values. However,
providing a value in an enum definition which is not representable as
an int would still be invalid (irrespective of how large the storage
is) - it's a separate restriction.

Anyhow, I'm not dying to replace all the current definitions with
enums, so if someone else wants to pick that up, be my guest.

Cheers,
-Brian
diff mbox series

Patch

diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index 1dffd1ac4cd4..1dd9f4824d3b 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -322,6 +322,12 @@  Frame Buffer Functions Reference
 DRM Format Handling
 ===================
 
+.. kernel-doc:: include/uapi/drm/drm_fourcc.h
+   :doc: overview
+
+Format Functions Reference
+--------------------------
+
 .. kernel-doc:: include/drm/drm_fourcc.h
    :internal:
 
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 894fa2da32fd..3f6c0b499323 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -30,6 +30,42 @@ 
 extern "C" {
 #endif
 
+/**
+ * DOC: overview
+ *
+ * In the DRM subsystem, framebuffer pixel formats are described using the
+ * fourcc codes defined in `include/uapi/drm/drm_fourcc.h`. In addition to the
+ * fourcc code, a Format Modifier may optionally be provided, in order to
+ * further describe the buffer's format - for example tiling or compression.
+ *
+ * Format Modifiers
+ * ----------------
+ *
+ * Format modifiers are used in conjunction with a fourcc code, forming a
+ * unique fourcc:modifier pair. This format:modifier pair must fully define the
+ * format and data layout of the buffer, and should be the only way to describe
+ * that particular buffer.
+ *
+ * Having multiple fourcc:modifier pairs which describe the same layout should
+ * be avoided, as such aliases run the risk of different drivers exposing
+ * different names for the same data format, forcing userspace to understand
+ * that they are aliases.
+ *
+ * Format modifiers may change any property of the buffer, including the number
+ * of planes and/or the required allocation size. Format modifiers are
+ * vendor-namespaced, and as such the relationship between a fourcc code and a
+ * modifier is specific to the modifer being used. For example, some modifiers
+ * may preserve meaning - such as number of planes - from the fourcc code,
+ * whereas others may not.
+ *
+ * Vendors are encouraged to document their modifier usage in as much detail as
+ * possible, to ensure maximum compatibility across devices, drivers and
+ * applications.
+ *
+ * The authoritative list of format modifier codes is found in
+ * `include/uapi/drm/drm_fourcc.h`
+ */
+
 #define fourcc_code(a, b, c, d) ((__u32)(a) | ((__u32)(b) << 8) | \
 				 ((__u32)(c) << 16) | ((__u32)(d) << 24))