[PATCH] drm: drm_fourcc: Add generic alias for 16_16_TILE modifier

[PATCH] drm: drm_fourcc: Add generic alias for 16_16_TILE modifier

[Brian Starkey]

In cases such as DRM_FORMAT_MOD_SAMSUNG_16_16_TILE, the modifier
describes a generic pixel re-ordering which can be applicable to
multiple vendors.

Define an alias: DRM_FORMAT_MOD_GENERIC_16_16_TILE, which can be
used to describe this layout in a vendor-neutral way, and add a
comment about the expected usage of such "generic" modifiers.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---

This is based off a suggestion from Daniel here:
https://lore.kernel.org/dri-devel/20200529115328.GC1630358@phenom.ffwll.local/

If there are future cases where a "generic" modifier is identified
before merging with a specific vendor code, perhaps we could use
`fourcc_mod_code(NONE, n)` or add a DRM_FORMAT_MOD_VENDOR_GENERIC.

However, I also think we shouldn't try and "guess" what might be generic
up-front and end up polluting the generic namespace. It seems fine to
just alias vendor-specific definitions unless it's very clear-cut.

I typed a version which was s/GENERIC/COMMON/, other suggestions
welcome.

Cheers,
-Brian

 include/uapi/drm/drm_fourcc.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 299f9ac4840b..ef78dc9c3fb0 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -345,8 +345,27 @@ extern "C" {
  * When adding a new token please document the layout with a code comment,
  * similar to the fourcc codes above. drm_fourcc.h is considered the
  * authoritative source for all of these.
+ *
+ * Generic modifier names:
+ *
+ * DRM_FORMAT_MOD_GENERIC_* definitions are used to provide vendor-neutral names
+ * for layouts which are common across multiple vendors. To preserve
+ * compatibility, in cases where a vendor-specific definition already exists and
+ * a generic name for it is desired, the common name is a purely symbolic alias
+ * and must use the same numerical value as the original definition.
+ *
+ * Note that generic names should only be used for modifiers which describe
+ * generic layouts (such as pixel re-ordering), which may have
+ * independently-developed support across multiple vendors.
+ *
+ * Generic names should not be used for cases where multiple hardware vendors
+ * have implementations of the same standardised compression scheme (such as
+ * AFBC). In those cases, all implementations should use the same format
+ * modifier(s), reflecting the vendor of the standard.
  */
 
+#define DRM_FORMAT_MOD_GENERIC_16_16_TILE DRM_FORMAT_MOD_SAMSUNG_16_16_TILE
+
 /*
  * Invalid Modifier
  *
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: drm_fourcc: Add generic alias for 16_16_TILE modifier

[Daniel Vetter]

On Fri, Jun 26, 2020 at 3:52 PM Brian Starkey <brian.starkey@arm.com> wrote:
>
> In cases such as DRM_FORMAT_MOD_SAMSUNG_16_16_TILE, the modifier
> describes a generic pixel re-ordering which can be applicable to
> multiple vendors.
>
> Define an alias: DRM_FORMAT_MOD_GENERIC_16_16_TILE, which can be
> used to describe this layout in a vendor-neutral way, and add a
> comment about the expected usage of such "generic" modifiers.
>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> ---
>
> This is based off a suggestion from Daniel here:
> https://lore.kernel.org/dri-devel/20200529115328.GC1630358@phenom.ffwll.local/
>
> If there are future cases where a "generic" modifier is identified
> before merging with a specific vendor code, perhaps we could use
> `fourcc_mod_code(NONE, n)` or add a DRM_FORMAT_MOD_VENDOR_GENERIC.
>
> However, I also think we shouldn't try and "guess" what might be generic
> up-front and end up polluting the generic namespace. It seems fine to
> just alias vendor-specific definitions unless it's very clear-cut.

I think the above two things would also be good additions to the comment.

A totally different thing: I just noticed that we don't pull in all
the comments for modifiers. I think we could convert them to
kerneldoc, and then pull them into a separate section in drm-kms.rst.
Maybe even worth to pull out into a new drm-fourcc.rst document, since
these are officially shared with gl and vk standard extensions. Then
this new modifier's doc could simply point at teh existing one (and
the generated kerneldoc would make that a hyperlink for added
awesomeness).

Aside from that makes sense to me, maybe just add it to your patch
series that will start making use of these.
-Daniel

>
> I typed a version which was s/GENERIC/COMMON/, other suggestions
> welcome.
>
> Cheers,
> -Brian
>
>  include/uapi/drm/drm_fourcc.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 299f9ac4840b..ef78dc9c3fb0 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -345,8 +345,27 @@ extern "C" {
>   * When adding a new token please document the layout with a code comment,
>   * similar to the fourcc codes above. drm_fourcc.h is considered the
>   * authoritative source for all of these.
> + *
> + * Generic modifier names:
> + *
> + * DRM_FORMAT_MOD_GENERIC_* definitions are used to provide vendor-neutral names
> + * for layouts which are common across multiple vendors. To preserve
> + * compatibility, in cases where a vendor-specific definition already exists and
> + * a generic name for it is desired, the common name is a purely symbolic alias
> + * and must use the same numerical value as the original definition.
> + *
> + * Note that generic names should only be used for modifiers which describe
> + * generic layouts (such as pixel re-ordering), which may have
> + * independently-developed support across multiple vendors.
> + *
> + * Generic names should not be used for cases where multiple hardware vendors
> + * have implementations of the same standardised compression scheme (such as
> + * AFBC). In those cases, all implementations should use the same format
> + * modifier(s), reflecting the vendor of the standard.
>   */
>
> +#define DRM_FORMAT_MOD_GENERIC_16_16_TILE DRM_FORMAT_MOD_SAMSUNG_16_16_TILE
> +
>  /*
>   * Invalid Modifier
>   *
> --
> 2.17.1
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: drm_fourcc: Add generic alias for 16_16_TILE modifier

[Brian Starkey]

Hi Daniel,

On Fri, Jun 26, 2020 at 04:07:43PM +0200, Daniel Vetter wrote:
> On Fri, Jun 26, 2020 at 3:52 PM Brian Starkey <brian.starkey@arm.com> wrote:
> >
> > In cases such as DRM_FORMAT_MOD_SAMSUNG_16_16_TILE, the modifier
> > describes a generic pixel re-ordering which can be applicable to
> > multiple vendors.
> >
> > Define an alias: DRM_FORMAT_MOD_GENERIC_16_16_TILE, which can be
> > used to describe this layout in a vendor-neutral way, and add a
> > comment about the expected usage of such "generic" modifiers.
> >
> > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> > ---
> >
> > This is based off a suggestion from Daniel here:
> > https://lore.kernel.org/dri-devel/20200529115328.GC1630358@phenom.ffwll.local/
> >
> > If there are future cases where a "generic" modifier is identified
> > before merging with a specific vendor code, perhaps we could use
> > `fourcc_mod_code(NONE, n)` or add a DRM_FORMAT_MOD_VENDOR_GENERIC.
> >
> > However, I also think we shouldn't try and "guess" what might be generic
> > up-front and end up polluting the generic namespace. It seems fine to
> > just alias vendor-specific definitions unless it's very clear-cut.
> 
> I think the above two things would also be good additions to the comment.
> 
> A totally different thing: I just noticed that we don't pull in all
> the comments for modifiers. I think we could convert them to
> kerneldoc, and then pull them into a separate section in drm-kms.rst.
> Maybe even worth to pull out into a new drm-fourcc.rst document, since
> these are officially shared with gl and vk standard extensions. Then
> this new modifier's doc could simply point at teh existing one (and
> the generated kerneldoc would make that a hyperlink for added
> awesomeness).
> 
> Aside from that makes sense to me, maybe just add it to your patch
> series that will start making use of these.

This is only supported by the GPU, so we wouldn't be using this on the
Display side.

Thanks,
-Brian

> -Daniel
> 
> >
> > I typed a version which was s/GENERIC/COMMON/, other suggestions
> > welcome.
> >
> > Cheers,
> > -Brian
> >
> >  include/uapi/drm/drm_fourcc.h | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index 299f9ac4840b..ef78dc9c3fb0 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -345,8 +345,27 @@ extern "C" {
> >   * When adding a new token please document the layout with a code comment,
> >   * similar to the fourcc codes above. drm_fourcc.h is considered the
> >   * authoritative source for all of these.
> > + *
> > + * Generic modifier names:
> > + *
> > + * DRM_FORMAT_MOD_GENERIC_* definitions are used to provide vendor-neutral names
> > + * for layouts which are common across multiple vendors. To preserve
> > + * compatibility, in cases where a vendor-specific definition already exists and
> > + * a generic name for it is desired, the common name is a purely symbolic alias
> > + * and must use the same numerical value as the original definition.
> > + *
> > + * Note that generic names should only be used for modifiers which describe
> > + * generic layouts (such as pixel re-ordering), which may have
> > + * independently-developed support across multiple vendors.
> > + *
> > + * Generic names should not be used for cases where multiple hardware vendors
> > + * have implementations of the same standardised compression scheme (such as
> > + * AFBC). In those cases, all implementations should use the same format
> > + * modifier(s), reflecting the vendor of the standard.
> >   */
> >
> > +#define DRM_FORMAT_MOD_GENERIC_16_16_TILE DRM_FORMAT_MOD_SAMSUNG_16_16_TILE
> > +
> >  /*
> >   * Invalid Modifier
> >   *
> > --
> > 2.17.1
> >
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: drm_fourcc: Add generic alias for 16_16_TILE modifier

[Daniel Vetter]

On Fri, Jun 26, 2020 at 4:16 PM Brian Starkey <brian.starkey@arm.com> wrote:
>
> Hi Daniel,
>
> On Fri, Jun 26, 2020 at 04:07:43PM +0200, Daniel Vetter wrote:
> > On Fri, Jun 26, 2020 at 3:52 PM Brian Starkey <brian.starkey@arm.com> wrote:
> > >
> > > In cases such as DRM_FORMAT_MOD_SAMSUNG_16_16_TILE, the modifier
> > > describes a generic pixel re-ordering which can be applicable to
> > > multiple vendors.
> > >
> > > Define an alias: DRM_FORMAT_MOD_GENERIC_16_16_TILE, which can be
> > > used to describe this layout in a vendor-neutral way, and add a
> > > comment about the expected usage of such "generic" modifiers.
> > >
> > > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> > > ---
> > >
> > > This is based off a suggestion from Daniel here:
> > > https://lore.kernel.org/dri-devel/20200529115328.GC1630358@phenom.ffwll.local/
> > >
> > > If there are future cases where a "generic" modifier is identified
> > > before merging with a specific vendor code, perhaps we could use
> > > `fourcc_mod_code(NONE, n)` or add a DRM_FORMAT_MOD_VENDOR_GENERIC.
> > >
> > > However, I also think we shouldn't try and "guess" what might be generic
> > > up-front and end up polluting the generic namespace. It seems fine to
> > > just alias vendor-specific definitions unless it's very clear-cut.
> >
> > I think the above two things would also be good additions to the comment.
> >
> > A totally different thing: I just noticed that we don't pull in all
> > the comments for modifiers. I think we could convert them to
> > kerneldoc, and then pull them into a separate section in drm-kms.rst.
> > Maybe even worth to pull out into a new drm-fourcc.rst document, since
> > these are officially shared with gl and vk standard extensions. Then
> > this new modifier's doc could simply point at teh existing one (and
> > the generated kerneldoc would make that a hyperlink for added
> > awesomeness).
> >
> > Aside from that makes sense to me, maybe just add it to your patch
> > series that will start making use of these.
>
> This is only supported by the GPU, so we wouldn't be using this on the
> Display side.

I mean the patch to add the NV15 fource, it mentions "suitable for 16
by 16 tile layout", would be nice to just put the generic modifier in
that comment.
-Daniel

>
> Thanks,
> -Brian
>
> > -Daniel
> >
> > >
> > > I typed a version which was s/GENERIC/COMMON/, other suggestions
> > > welcome.
> > >
> > > Cheers,
> > > -Brian
> > >
> > >  include/uapi/drm/drm_fourcc.h | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > >
> > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > > index 299f9ac4840b..ef78dc9c3fb0 100644
> > > --- a/include/uapi/drm/drm_fourcc.h
> > > +++ b/include/uapi/drm/drm_fourcc.h
> > > @@ -345,8 +345,27 @@ extern "C" {
> > >   * When adding a new token please document the layout with a code comment,
> > >   * similar to the fourcc codes above. drm_fourcc.h is considered the
> > >   * authoritative source for all of these.
> > > + *
> > > + * Generic modifier names:
> > > + *
> > > + * DRM_FORMAT_MOD_GENERIC_* definitions are used to provide vendor-neutral names
> > > + * for layouts which are common across multiple vendors. To preserve
> > > + * compatibility, in cases where a vendor-specific definition already exists and
> > > + * a generic name for it is desired, the common name is a purely symbolic alias
> > > + * and must use the same numerical value as the original definition.
> > > + *
> > > + * Note that generic names should only be used for modifiers which describe
> > > + * generic layouts (such as pixel re-ordering), which may have
> > > + * independently-developed support across multiple vendors.
> > > + *
> > > + * Generic names should not be used for cases where multiple hardware vendors
> > > + * have implementations of the same standardised compression scheme (such as
> > > + * AFBC). In those cases, all implementations should use the same format
> > > + * modifier(s), reflecting the vendor of the standard.
> > >   */
> > >
> > > +#define DRM_FORMAT_MOD_GENERIC_16_16_TILE DRM_FORMAT_MOD_SAMSUNG_16_16_TILE
> > > +
> > >  /*
> > >   * Invalid Modifier
> > >   *
> > > --
> > > 2.17.1
> > >
> >
> >
> > --
> > 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: drm_fourcc: Add generic alias for 16_16_TILE modifier

[Brian Starkey]

On Fri, Jun 26, 2020 at 05:16:53PM +0200, Daniel Vetter wrote:
> On Fri, Jun 26, 2020 at 4:16 PM Brian Starkey <brian.starkey@arm.com> wrote:
> >
> > Hi Daniel,
> >
> > On Fri, Jun 26, 2020 at 04:07:43PM +0200, Daniel Vetter wrote:
> > > On Fri, Jun 26, 2020 at 3:52 PM Brian Starkey <brian.starkey@arm.com> wrote:
> > > >
> > > > In cases such as DRM_FORMAT_MOD_SAMSUNG_16_16_TILE, the modifier
> > > > describes a generic pixel re-ordering which can be applicable to
> > > > multiple vendors.
> > > >
> > > > Define an alias: DRM_FORMAT_MOD_GENERIC_16_16_TILE, which can be
> > > > used to describe this layout in a vendor-neutral way, and add a
> > > > comment about the expected usage of such "generic" modifiers.
> > > >
> > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> > > > ---
> > > >
> > > > This is based off a suggestion from Daniel here:
> > > > https://lore.kernel.org/dri-devel/20200529115328.GC1630358@phenom.ffwll.local/
> > > >
> > > > If there are future cases where a "generic" modifier is identified
> > > > before merging with a specific vendor code, perhaps we could use
> > > > `fourcc_mod_code(NONE, n)` or add a DRM_FORMAT_MOD_VENDOR_GENERIC.
> > > >
> > > > However, I also think we shouldn't try and "guess" what might be generic
> > > > up-front and end up polluting the generic namespace. It seems fine to
> > > > just alias vendor-specific definitions unless it's very clear-cut.
> > >
> > > I think the above two things would also be good additions to the comment.
> > >
> > > A totally different thing: I just noticed that we don't pull in all
> > > the comments for modifiers. I think we could convert them to
> > > kerneldoc, and then pull them into a separate section in drm-kms.rst.
> > > Maybe even worth to pull out into a new drm-fourcc.rst document, since
> > > these are officially shared with gl and vk standard extensions. Then
> > > this new modifier's doc could simply point at teh existing one (and
> > > the generated kerneldoc would make that a hyperlink for added
> > > awesomeness).
> > >
> > > Aside from that makes sense to me, maybe just add it to your patch
> > > series that will start making use of these.
> >
> > This is only supported by the GPU, so we wouldn't be using this on the
> > Display side.
> 
> I mean the patch to add the NV15 fource, it mentions "suitable for 16
> by 16 tile layout", would be nice to just put the generic modifier in
> that comment.
> -Daniel

Ah right. I saw that one went out in Maarten's pull request this
morning.

-Brian

> 
> >
> > Thanks,
> > -Brian
> >
> > > -Daniel
> > >
> > > >
> > > > I typed a version which was s/GENERIC/COMMON/, other suggestions
> > > > welcome.
> > > >
> > > > Cheers,
> > > > -Brian
> > > >
> > > >  include/uapi/drm/drm_fourcc.h | 19 +++++++++++++++++++
> > > >  1 file changed, 19 insertions(+)
> > > >
> > > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > > > index 299f9ac4840b..ef78dc9c3fb0 100644
> > > > --- a/include/uapi/drm/drm_fourcc.h
> > > > +++ b/include/uapi/drm/drm_fourcc.h
> > > > @@ -345,8 +345,27 @@ extern "C" {
> > > >   * When adding a new token please document the layout with a code comment,
> > > >   * similar to the fourcc codes above. drm_fourcc.h is considered the
> > > >   * authoritative source for all of these.
> > > > + *
> > > > + * Generic modifier names:
> > > > + *
> > > > + * DRM_FORMAT_MOD_GENERIC_* definitions are used to provide vendor-neutral names
> > > > + * for layouts which are common across multiple vendors. To preserve
> > > > + * compatibility, in cases where a vendor-specific definition already exists and
> > > > + * a generic name for it is desired, the common name is a purely symbolic alias
> > > > + * and must use the same numerical value as the original definition.
> > > > + *
> > > > + * Note that generic names should only be used for modifiers which describe
> > > > + * generic layouts (such as pixel re-ordering), which may have
> > > > + * independently-developed support across multiple vendors.
> > > > + *
> > > > + * Generic names should not be used for cases where multiple hardware vendors
> > > > + * have implementations of the same standardised compression scheme (such as
> > > > + * AFBC). In those cases, all implementations should use the same format
> > > > + * modifier(s), reflecting the vendor of the standard.
> > > >   */
> > > >
> > > > +#define DRM_FORMAT_MOD_GENERIC_16_16_TILE DRM_FORMAT_MOD_SAMSUNG_16_16_TILE
> > > > +
> > > >  /*
> > > >   * Invalid Modifier
> > > >   *
> > > > --
> > > > 2.17.1
> > > >
> > >
> > >
> > > --
> > > 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: drm_fourcc: Add generic alias for 16_16_TILE modifier

[Daniel Vetter]

On Fri, Jun 26, 2020 at 5:21 PM Brian Starkey <brian.starkey@arm.com> wrote:
>
> On Fri, Jun 26, 2020 at 05:16:53PM +0200, Daniel Vetter wrote:
> > On Fri, Jun 26, 2020 at 4:16 PM Brian Starkey <brian.starkey@arm.com> wrote:
> > >
> > > Hi Daniel,
> > >
> > > On Fri, Jun 26, 2020 at 04:07:43PM +0200, Daniel Vetter wrote:
> > > > On Fri, Jun 26, 2020 at 3:52 PM Brian Starkey <brian.starkey@arm.com> wrote:
> > > > >
> > > > > In cases such as DRM_FORMAT_MOD_SAMSUNG_16_16_TILE, the modifier
> > > > > describes a generic pixel re-ordering which can be applicable to
> > > > > multiple vendors.
> > > > >
> > > > > Define an alias: DRM_FORMAT_MOD_GENERIC_16_16_TILE, which can be
> > > > > used to describe this layout in a vendor-neutral way, and add a
> > > > > comment about the expected usage of such "generic" modifiers.
> > > > >
> > > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> > > > > ---
> > > > >
> > > > > This is based off a suggestion from Daniel here:
> > > > > https://lore.kernel.org/dri-devel/20200529115328.GC1630358@phenom.ffwll.local/
> > > > >
> > > > > If there are future cases where a "generic" modifier is identified
> > > > > before merging with a specific vendor code, perhaps we could use
> > > > > `fourcc_mod_code(NONE, n)` or add a DRM_FORMAT_MOD_VENDOR_GENERIC.
> > > > >
> > > > > However, I also think we shouldn't try and "guess" what might be generic
> > > > > up-front and end up polluting the generic namespace. It seems fine to
> > > > > just alias vendor-specific definitions unless it's very clear-cut.
> > > >
> > > > I think the above two things would also be good additions to the comment.
> > > >
> > > > A totally different thing: I just noticed that we don't pull in all
> > > > the comments for modifiers. I think we could convert them to
> > > > kerneldoc, and then pull them into a separate section in drm-kms.rst.
> > > > Maybe even worth to pull out into a new drm-fourcc.rst document, since
> > > > these are officially shared with gl and vk standard extensions. Then
> > > > this new modifier's doc could simply point at teh existing one (and
> > > > the generated kerneldoc would make that a hyperlink for added
> > > > awesomeness).
> > > >
> > > > Aside from that makes sense to me, maybe just add it to your patch
> > > > series that will start making use of these.
> > >
> > > This is only supported by the GPU, so we wouldn't be using this on the
> > > Display side.
> >
> > I mean the patch to add the NV15 fource, it mentions "suitable for 16
> > by 16 tile layout", would be nice to just put the generic modifier in
> > that comment.
> > -Daniel
>
> Ah right. I saw that one went out in Maarten's pull request this
> morning.

Oh I missed that, then maybe just amend this patch to update the other comment?
-Daniel

>
> -Brian
>
> >
> > >
> > > Thanks,
> > > -Brian
> > >
> > > > -Daniel
> > > >
> > > > >
> > > > > I typed a version which was s/GENERIC/COMMON/, other suggestions
> > > > > welcome.
> > > > >
> > > > > Cheers,
> > > > > -Brian
> > > > >
> > > > >  include/uapi/drm/drm_fourcc.h | 19 +++++++++++++++++++
> > > > >  1 file changed, 19 insertions(+)
> > > > >
> > > > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > > > > index 299f9ac4840b..ef78dc9c3fb0 100644
> > > > > --- a/include/uapi/drm/drm_fourcc.h
> > > > > +++ b/include/uapi/drm/drm_fourcc.h
> > > > > @@ -345,8 +345,27 @@ extern "C" {
> > > > >   * When adding a new token please document the layout with a code comment,
> > > > >   * similar to the fourcc codes above. drm_fourcc.h is considered the
> > > > >   * authoritative source for all of these.
> > > > > + *
> > > > > + * Generic modifier names:
> > > > > + *
> > > > > + * DRM_FORMAT_MOD_GENERIC_* definitions are used to provide vendor-neutral names
> > > > > + * for layouts which are common across multiple vendors. To preserve
> > > > > + * compatibility, in cases where a vendor-specific definition already exists and
> > > > > + * a generic name for it is desired, the common name is a purely symbolic alias
> > > > > + * and must use the same numerical value as the original definition.
> > > > > + *
> > > > > + * Note that generic names should only be used for modifiers which describe
> > > > > + * generic layouts (such as pixel re-ordering), which may have
> > > > > + * independently-developed support across multiple vendors.
> > > > > + *
> > > > > + * Generic names should not be used for cases where multiple hardware vendors
> > > > > + * have implementations of the same standardised compression scheme (such as
> > > > > + * AFBC). In those cases, all implementations should use the same format
> > > > > + * modifier(s), reflecting the vendor of the standard.
> > > > >   */
> > > > >
> > > > > +#define DRM_FORMAT_MOD_GENERIC_16_16_TILE DRM_FORMAT_MOD_SAMSUNG_16_16_TILE
> > > > > +
> > > > >  /*
> > > > >   * Invalid Modifier
> > > > >   *
> > > > > --
> > > > > 2.17.1
> > > > >
> > > >
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> >
> >
> >
> > --
> > 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: drm_fourcc: Add generic alias for 16_16_TILE modifier

[Brian Starkey]

On Fri, Jun 26, 2020 at 06:15:36PM +0200, Daniel Vetter wrote:
> On Fri, Jun 26, 2020 at 5:21 PM Brian Starkey <brian.starkey@arm.com> wrote:
> >
> > On Fri, Jun 26, 2020 at 05:16:53PM +0200, Daniel Vetter wrote:
> > > On Fri, Jun 26, 2020 at 4:16 PM Brian Starkey <brian.starkey@arm.com> wrote:
> > > >
> > > > Hi Daniel,
> > > >
> > > > On Fri, Jun 26, 2020 at 04:07:43PM +0200, Daniel Vetter wrote:
> > > > > On Fri, Jun 26, 2020 at 3:52 PM Brian Starkey <brian.starkey@arm.com> wrote:
> > > > > >
> > > > > > In cases such as DRM_FORMAT_MOD_SAMSUNG_16_16_TILE, the modifier
> > > > > > describes a generic pixel re-ordering which can be applicable to
> > > > > > multiple vendors.
> > > > > >
> > > > > > Define an alias: DRM_FORMAT_MOD_GENERIC_16_16_TILE, which can be
> > > > > > used to describe this layout in a vendor-neutral way, and add a
> > > > > > comment about the expected usage of such "generic" modifiers.
> > > > > >
> > > > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> > > > > > ---
> > > > > >
> > > > > > This is based off a suggestion from Daniel here:
> > > > > > https://lore.kernel.org/dri-devel/20200529115328.GC1630358@phenom.ffwll.local/
> > > > > >
> > > > > > If there are future cases where a "generic" modifier is identified
> > > > > > before merging with a specific vendor code, perhaps we could use
> > > > > > `fourcc_mod_code(NONE, n)` or add a DRM_FORMAT_MOD_VENDOR_GENERIC.
> > > > > >
> > > > > > However, I also think we shouldn't try and "guess" what might be generic
> > > > > > up-front and end up polluting the generic namespace. It seems fine to
> > > > > > just alias vendor-specific definitions unless it's very clear-cut.
> > > > >
> > > > > I think the above two things would also be good additions to the comment.
> > > > >
> > > > > A totally different thing: I just noticed that we don't pull in all
> > > > > the comments for modifiers. I think we could convert them to
> > > > > kerneldoc, and then pull them into a separate section in drm-kms.rst.
> > > > > Maybe even worth to pull out into a new drm-fourcc.rst document, since
> > > > > these are officially shared with gl and vk standard extensions. Then
> > > > > this new modifier's doc could simply point at teh existing one (and
> > > > > the generated kerneldoc would make that a hyperlink for added
> > > > > awesomeness).
> > > > >
> > > > > Aside from that makes sense to me, maybe just add it to your patch
> > > > > series that will start making use of these.
> > > >
> > > > This is only supported by the GPU, so we wouldn't be using this on the
> > > > Display side.
> > >
> > > I mean the patch to add the NV15 fource, it mentions "suitable for 16
> > > by 16 tile layout", would be nice to just put the generic modifier in
> > > that comment.
> > > -Daniel
> >
> > Ah right. I saw that one went out in Maarten's pull request this
> > morning.
> 
> Oh I missed that, then maybe just amend this patch to update the other comment?

It was only mentioned in the commit message there, so I guess I can't
do anything about it now.

I'll respin to put the extra notes into the comment.

Thanks,
-Brian

> -Daniel
> 
> >
> > -Brian
> >
> > >
> > > >
> > > > Thanks,
> > > > -Brian
> > > >
> > > > > -Daniel
> > > > >
> > > > > >
> > > > > > I typed a version which was s/GENERIC/COMMON/, other suggestions
> > > > > > welcome.
> > > > > >
> > > > > > Cheers,
> > > > > > -Brian
> > > > > >
> > > > > >  include/uapi/drm/drm_fourcc.h | 19 +++++++++++++++++++
> > > > > >  1 file changed, 19 insertions(+)
> > > > > >
> > > > > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > > > > > index 299f9ac4840b..ef78dc9c3fb0 100644
> > > > > > --- a/include/uapi/drm/drm_fourcc.h
> > > > > > +++ b/include/uapi/drm/drm_fourcc.h
> > > > > > @@ -345,8 +345,27 @@ extern "C" {
> > > > > >   * When adding a new token please document the layout with a code comment,
> > > > > >   * similar to the fourcc codes above. drm_fourcc.h is considered the
> > > > > >   * authoritative source for all of these.
> > > > > > + *
> > > > > > + * Generic modifier names:
> > > > > > + *
> > > > > > + * DRM_FORMAT_MOD_GENERIC_* definitions are used to provide vendor-neutral names
> > > > > > + * for layouts which are common across multiple vendors. To preserve
> > > > > > + * compatibility, in cases where a vendor-specific definition already exists and
> > > > > > + * a generic name for it is desired, the common name is a purely symbolic alias
> > > > > > + * and must use the same numerical value as the original definition.
> > > > > > + *
> > > > > > + * Note that generic names should only be used for modifiers which describe
> > > > > > + * generic layouts (such as pixel re-ordering), which may have
> > > > > > + * independently-developed support across multiple vendors.
> > > > > > + *
> > > > > > + * Generic names should not be used for cases where multiple hardware vendors
> > > > > > + * have implementations of the same standardised compression scheme (such as
> > > > > > + * AFBC). In those cases, all implementations should use the same format
> > > > > > + * modifier(s), reflecting the vendor of the standard.
> > > > > >   */
> > > > > >
> > > > > > +#define DRM_FORMAT_MOD_GENERIC_16_16_TILE DRM_FORMAT_MOD_SAMSUNG_16_16_TILE
> > > > > > +
> > > > > >  /*
> > > > > >   * Invalid Modifier
> > > > > >   *
> > > > > > --
> > > > > > 2.17.1
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > >
> > >
> > >
> > > --
> > > 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] drm: drm_fourcc: Add generic alias for 16_16_TILE modifier

[Brian Starkey]

In cases such as DRM_FORMAT_MOD_SAMSUNG_16_16_TILE, the modifier
describes a generic pixel re-ordering which can be applicable to
multiple vendors.

Define an alias: DRM_FORMAT_MOD_GENERIC_16_16_TILE, which can be
used to describe this layout in a vendor-neutral way, and add a
comment about the expected usage of such "generic" modifiers.

Changes in v2:
 - Move note about future cases to comment (Daniel)

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 include/uapi/drm/drm_fourcc.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 299f9ac4840b..cb3db4a21012 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -345,8 +345,33 @@ extern "C" {
  * When adding a new token please document the layout with a code comment,
  * similar to the fourcc codes above. drm_fourcc.h is considered the
  * authoritative source for all of these.
+ *
+ * Generic modifier names:
+ *
+ * DRM_FORMAT_MOD_GENERIC_* definitions are used to provide vendor-neutral names
+ * for layouts which are common across multiple vendors. To preserve
+ * compatibility, in cases where a vendor-specific definition already exists and
+ * a generic name for it is desired, the common name is a purely symbolic alias
+ * and must use the same numerical value as the original definition.
+ *
+ * Note that generic names should only be used for modifiers which describe
+ * generic layouts (such as pixel re-ordering), which may have
+ * independently-developed support across multiple vendors.
+ *
+ * In future cases where a generic layout is identified before merging with a
+ * vendor-specific modifier, a new 'GENERIC' vendor or modifier using vendor
+ * 'NONE' could be considered. This should only be for obvious, exceptional
+ * cases to avoid polluting the 'GENERIC' namespace with modifiers which only
+ * apply to a single vendor.
+ *
+ * Generic names should not be used for cases where multiple hardware vendors
+ * have implementations of the same standardised compression scheme (such as
+ * AFBC). In those cases, all implementations should use the same format
+ * modifier(s), reflecting the vendor of the standard.
  */
 
+#define DRM_FORMAT_MOD_GENERIC_16_16_TILE DRM_FORMAT_MOD_SAMSUNG_16_16_TILE
+
 /*
  * Invalid Modifier
  *
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: drm_fourcc: Add generic alias for 16_16_TILE modifier

[Daniel Vetter]

On Fri, Jun 26, 2020 at 05:48:00PM +0100, Brian Starkey wrote:
> In cases such as DRM_FORMAT_MOD_SAMSUNG_16_16_TILE, the modifier
> describes a generic pixel re-ordering which can be applicable to
> multiple vendors.
> 
> Define an alias: DRM_FORMAT_MOD_GENERIC_16_16_TILE, which can be
> used to describe this layout in a vendor-neutral way, and add a
> comment about the expected usage of such "generic" modifiers.
> 
> Changes in v2:
>  - Move note about future cases to comment (Daniel)
> 
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>

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

> ---
>  include/uapi/drm/drm_fourcc.h | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 299f9ac4840b..cb3db4a21012 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -345,8 +345,33 @@ extern "C" {
>   * When adding a new token please document the layout with a code comment,
>   * similar to the fourcc codes above. drm_fourcc.h is considered the
>   * authoritative source for all of these.
> + *
> + * Generic modifier names:
> + *
> + * DRM_FORMAT_MOD_GENERIC_* definitions are used to provide vendor-neutral names
> + * for layouts which are common across multiple vendors. To preserve
> + * compatibility, in cases where a vendor-specific definition already exists and
> + * a generic name for it is desired, the common name is a purely symbolic alias
> + * and must use the same numerical value as the original definition.
> + *
> + * Note that generic names should only be used for modifiers which describe
> + * generic layouts (such as pixel re-ordering), which may have
> + * independently-developed support across multiple vendors.
> + *
> + * In future cases where a generic layout is identified before merging with a
> + * vendor-specific modifier, a new 'GENERIC' vendor or modifier using vendor
> + * 'NONE' could be considered. This should only be for obvious, exceptional
> + * cases to avoid polluting the 'GENERIC' namespace with modifiers which only
> + * apply to a single vendor.
> + *
> + * Generic names should not be used for cases where multiple hardware vendors
> + * have implementations of the same standardised compression scheme (such as
> + * AFBC). In those cases, all implementations should use the same format
> + * modifier(s), reflecting the vendor of the standard.
>   */
>  
> +#define DRM_FORMAT_MOD_GENERIC_16_16_TILE DRM_FORMAT_MOD_SAMSUNG_16_16_TILE
> +
>  /*
>   * Invalid Modifier
>   *
> -- 
> 2.17.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: drm_fourcc: Add generic alias for 16_16_TILE modifier

[Brian Starkey]

On Mon, Jun 29, 2020 at 10:32:41AM +0200, Daniel Vetter wrote:
> On Fri, Jun 26, 2020 at 05:48:00PM +0100, Brian Starkey wrote:
> > In cases such as DRM_FORMAT_MOD_SAMSUNG_16_16_TILE, the modifier
> > describes a generic pixel re-ordering which can be applicable to
> > multiple vendors.
> > 
> > Define an alias: DRM_FORMAT_MOD_GENERIC_16_16_TILE, which can be
> > used to describe this layout in a vendor-neutral way, and add a
> > comment about the expected usage of such "generic" modifiers.
> > 
> > Changes in v2:
> >  - Move note about future cases to comment (Daniel)
> > 
> > Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks. I guess this should go into drm-misc-next? @Liviu would you be
able to pick it up?

Cheers,
-Brian

> 
> > ---
> >  include/uapi/drm/drm_fourcc.h | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index 299f9ac4840b..cb3db4a21012 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -345,8 +345,33 @@ extern "C" {
> >   * When adding a new token please document the layout with a code comment,
> >   * similar to the fourcc codes above. drm_fourcc.h is considered the
> >   * authoritative source for all of these.
> > + *
> > + * Generic modifier names:
> > + *
> > + * DRM_FORMAT_MOD_GENERIC_* definitions are used to provide vendor-neutral names
> > + * for layouts which are common across multiple vendors. To preserve
> > + * compatibility, in cases where a vendor-specific definition already exists and
> > + * a generic name for it is desired, the common name is a purely symbolic alias
> > + * and must use the same numerical value as the original definition.
> > + *
> > + * Note that generic names should only be used for modifiers which describe
> > + * generic layouts (such as pixel re-ordering), which may have
> > + * independently-developed support across multiple vendors.
> > + *
> > + * In future cases where a generic layout is identified before merging with a
> > + * vendor-specific modifier, a new 'GENERIC' vendor or modifier using vendor
> > + * 'NONE' could be considered. This should only be for obvious, exceptional
> > + * cases to avoid polluting the 'GENERIC' namespace with modifiers which only
> > + * apply to a single vendor.
> > + *
> > + * Generic names should not be used for cases where multiple hardware vendors
> > + * have implementations of the same standardised compression scheme (such as
> > + * AFBC). In those cases, all implementations should use the same format
> > + * modifier(s), reflecting the vendor of the standard.
> >   */
> >  
> > +#define DRM_FORMAT_MOD_GENERIC_16_16_TILE DRM_FORMAT_MOD_SAMSUNG_16_16_TILE
> > +
> >  /*
> >   * Invalid Modifier
> >   *
> > -- 
> > 2.17.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel