[PATCH v2] drm/fourcc: Add Arm 16x16 block modifier

[PATCH v2] drm/fourcc: Add Arm 16x16 block modifier

[Ayan Halder]

Add the DRM_FORMAT_MOD_ARM_16X16_BLOCK_U_INTERLEAVED modifier to
denote the 16x16 block u-interleaved format used in Arm Utgard and
Midgard GPUs.

Changes from v1:-
1. Reserved the upper four bits (out of the 56 bits assigned to each vendor)
to denote the category of Arm specific modifiers. Currently, we have two
categories ie AFBC and MISC.

Signed-off-by: Raymond Smith <raymond.smith@arm.com>
Signed-off-by: Ayan kumar halder <ayan.halder@arm.com>
---
 include/uapi/drm/drm_fourcc.h | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 3feeaa3f987a..b1d3de961109 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -648,7 +648,21 @@ extern "C" {
  * Further information on the use of AFBC modifiers can be found in
  * Documentation/gpu/afbc.rst
  */
-#define DRM_FORMAT_MOD_ARM_AFBC(__afbc_mode)	fourcc_mod_code(ARM, __afbc_mode)
+
+/*
+ * The top 4 bits (out of the 56 bits alloted for specifying vendor specific
+ * modifiers) denote the category for modifiers. Currently we have only two
+ * categories of modifiers ie AFBC and MISC. We can have a maximum of sixteen
+ * different categories.
+ */
+#define DRM_FORMAT_MOD_ARM_CODE(type, val) \
+	fourcc_mod_code(ARM, ((__u64)type << 52) | ((val) & 0x000fffffffffffffULL))
+
+#define DRM_FORMAT_MOD_ARM_TYPE_AFBC 0x00
+#define DRM_FORMAT_MOD_ARM_TYPE_MISC 0x01
+
+#define DRM_FORMAT_MOD_ARM_AFBC(__afbc_mode) \
+	DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_AFBC, __afbc_mode)
 
 /*
  * AFBC superblock size
@@ -742,6 +756,17 @@ extern "C" {
  */
 #define AFBC_FORMAT_MOD_BCH     (1ULL << 11)
 
+/*
+ * Arm 16x16 Block U-Interleaved modifier
+ *
+ * This is used by Arm Mali Utgard and Midgard GPUs. It divides the image
+ * into 16x16 pixel blocks. Blocks are stored linearly in order, but pixels
+ * in the block are reordered.
+ */
+#define DRM_FORMAT_MOD_ARM_16X16_BLOCK_U_INTERLEAVED \
+	DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_MISC, 1ULL)
+
+
 /*
  * Allwinner tiled modifier
  *
-- 
2.23.0

Re: [PATCH v2] drm/fourcc: Add Arm 16x16 block modifier

[Brian Starkey]

Hi Ayan,

Could we preserve Ray's authorship on this patch?

On Mon, Sep 30, 2019 at 04:44:38PM +0000, Ayan Halder wrote:
> Add the DRM_FORMAT_MOD_ARM_16X16_BLOCK_U_INTERLEAVED modifier to
> denote the 16x16 block u-interleaved format used in Arm Utgard and
> Midgard GPUs.
> 
> Changes from v1:-
> 1. Reserved the upper four bits (out of the 56 bits assigned to each vendor)
> to denote the category of Arm specific modifiers. Currently, we have two
> categories ie AFBC and MISC.
> 
> Signed-off-by: Raymond Smith <raymond.smith@arm.com>
> Signed-off-by: Ayan kumar halder <ayan.halder@arm.com>
> ---
>  include/uapi/drm/drm_fourcc.h | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 3feeaa3f987a..b1d3de961109 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -648,7 +648,21 @@ extern "C" {
>   * Further information on the use of AFBC modifiers can be found in
>   * Documentation/gpu/afbc.rst
>   */
> -#define DRM_FORMAT_MOD_ARM_AFBC(__afbc_mode)	fourcc_mod_code(ARM, __afbc_mode)
> +
> +/*
> + * The top 4 bits (out of the 56 bits alloted for specifying vendor specific
> + * modifiers) denote the category for modifiers. Currently we have only two
> + * categories of modifiers ie AFBC and MISC. We can have a maximum of sixteen
> + * different categories.
> + */

Yeah, this makes more sense than getting crazy with saving bits. Sorry
Qiang/Daniel for not just going with this in the first instance when
you both suggested it.

> +#define DRM_FORMAT_MOD_ARM_CODE(type, val) \
> +	fourcc_mod_code(ARM, ((__u64)type << 52) | ((val) & 0x000fffffffffffffULL))

Not a big deal, but I might prefix type and val with "__" to reduce
the chance of name collisions with code using the macro:
DRM_FORMAT_MOD_ARM_CODE(__type, __val).

> +
> +#define DRM_FORMAT_MOD_ARM_TYPE_AFBC 0x00
> +#define DRM_FORMAT_MOD_ARM_TYPE_MISC 0x01
> +
> +#define DRM_FORMAT_MOD_ARM_AFBC(__afbc_mode) \
> +	DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_AFBC, __afbc_mode)
>  
>  /*
>   * AFBC superblock size
> @@ -742,6 +756,17 @@ extern "C" {
>   */
>  #define AFBC_FORMAT_MOD_BCH     (1ULL << 11)
>  
> +/*
> + * Arm 16x16 Block U-Interleaved modifier
> + *
> + * This is used by Arm Mali Utgard and Midgard GPUs. It divides the image
> + * into 16x16 pixel blocks. Blocks are stored linearly in order, but pixels
> + * in the block are reordered.
> + */
> +#define DRM_FORMAT_MOD_ARM_16X16_BLOCK_U_INTERLEAVED \
> +	DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_MISC, 1ULL)
> +
> +

I think you can drop this newline, there's no extra space between any
of the other definitions.

With this line dropped, and if you fix up the author:

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

Thanks for the respin,
-Brian

>  /*
>   * Allwinner tiled modifier
>   *
> -- 
> 2.23.0
> 

Re: [PATCH v2] drm/fourcc: Add Arm 16x16 block modifier

[Ayan Halder]

On Mon, Sep 30, 2019 at 05:02:37PM +0000, Brian Starkey wrote:
> Hi Ayan,
> 
> Could we preserve Ray's authorship on this patch?
Apologies for this, I will definitely preserve Ray's authorship in the
v3 patch.

> 
> On Mon, Sep 30, 2019 at 04:44:38PM +0000, Ayan Halder wrote:
> > Add the DRM_FORMAT_MOD_ARM_16X16_BLOCK_U_INTERLEAVED modifier to
> > denote the 16x16 block u-interleaved format used in Arm Utgard and
> > Midgard GPUs.
> > 
> > Changes from v1:-
> > 1. Reserved the upper four bits (out of the 56 bits assigned to each vendor)
> > to denote the category of Arm specific modifiers. Currently, we have two
> > categories ie AFBC and MISC.
> > 
> > Signed-off-by: Raymond Smith <raymond.smith@arm.com>
> > Signed-off-by: Ayan kumar halder <ayan.halder@arm.com>
> > ---
> >  include/uapi/drm/drm_fourcc.h | 27 ++++++++++++++++++++++++++-
> >  1 file changed, 26 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index 3feeaa3f987a..b1d3de961109 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -648,7 +648,21 @@ extern "C" {
> >   * Further information on the use of AFBC modifiers can be found in
> >   * Documentation/gpu/afbc.rst
> >   */
> > -#define DRM_FORMAT_MOD_ARM_AFBC(__afbc_mode)	fourcc_mod_code(ARM, __afbc_mode)
> > +
> > +/*
> > + * The top 4 bits (out of the 56 bits alloted for specifying vendor specific
> > + * modifiers) denote the category for modifiers. Currently we have only two
> > + * categories of modifiers ie AFBC and MISC. We can have a maximum of sixteen
> > + * different categories.
> > + */
> 
> Yeah, this makes more sense than getting crazy with saving bits. Sorry
> Qiang/Daniel for not just going with this in the first instance when
> you both suggested it.
> 
> > +#define DRM_FORMAT_MOD_ARM_CODE(type, val) \
> > +	fourcc_mod_code(ARM, ((__u64)type << 52) | ((val) & 0x000fffffffffffffULL))
> 
> Not a big deal, but I might prefix type and val with "__" to reduce
> the chance of name collisions with code using the macro:
> DRM_FORMAT_MOD_ARM_CODE(__type, __val).
> 
> > +
> > +#define DRM_FORMAT_MOD_ARM_TYPE_AFBC 0x00
> > +#define DRM_FORMAT_MOD_ARM_TYPE_MISC 0x01
> > +
> > +#define DRM_FORMAT_MOD_ARM_AFBC(__afbc_mode) \
> > +	DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_AFBC, __afbc_mode)
> >  
> >  /*
> >   * AFBC superblock size
> > @@ -742,6 +756,17 @@ extern "C" {
> >   */
> >  #define AFBC_FORMAT_MOD_BCH     (1ULL << 11)
> >  
> > +/*
> > + * Arm 16x16 Block U-Interleaved modifier
> > + *
> > + * This is used by Arm Mali Utgard and Midgard GPUs. It divides the image
> > + * into 16x16 pixel blocks. Blocks are stored linearly in order, but pixels
> > + * in the block are reordered.
> > + */
> > +#define DRM_FORMAT_MOD_ARM_16X16_BLOCK_U_INTERLEAVED \
> > +	DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_MISC, 1ULL)
> > +
> > +
> 
> I think you can drop this newline, there's no extra space between any
> of the other definitions.
> 
> With this line dropped, and if you fix up the author:
> 
> Reviewed-by: Brian Starkey <brian.starkey@arm.com>
> 
> Thanks for the respin,

I will wait for daniel@ffwll.ch and yuq825@gmail.com comments before
respinning the patch.

> -Brian
> 
> >  /*
> >   * Allwinner tiled modifier
> >   *
> > -- 
> > 2.23.0
> > 

Re: [PATCH v2] drm/fourcc: Add Arm 16x16 block modifier

[Qiang Yu]

Hi Ayan,

Thanks for the patch. I'm OK with either the static reserved type way or
the previous dynamic way. So this patch is:
Reviewed-by: Qiang Yu <yuq825@gmail.com>

PS. for anyone would like to know the usage of this modifier, lima mesa
driver need this modifier to denote some tiled texture format and shared
between client and display server.

Regards,
Qiang

On Tue, Oct 1, 2019 at 12:45 AM Ayan Halder <Ayan.Halder@arm.com> wrote:
>
> Add the DRM_FORMAT_MOD_ARM_16X16_BLOCK_U_INTERLEAVED modifier to
> denote the 16x16 block u-interleaved format used in Arm Utgard and
> Midgard GPUs.
>
> Changes from v1:-
> 1. Reserved the upper four bits (out of the 56 bits assigned to each vendor)
> to denote the category of Arm specific modifiers. Currently, we have two
> categories ie AFBC and MISC.
>
> Signed-off-by: Raymond Smith <raymond.smith@arm.com>
> Signed-off-by: Ayan kumar halder <ayan.halder@arm.com>
> ---
>  include/uapi/drm/drm_fourcc.h | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 3feeaa3f987a..b1d3de961109 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -648,7 +648,21 @@ extern "C" {
>   * Further information on the use of AFBC modifiers can be found in
>   * Documentation/gpu/afbc.rst
>   */
> -#define DRM_FORMAT_MOD_ARM_AFBC(__afbc_mode)   fourcc_mod_code(ARM, __afbc_mode)
> +
> +/*
> + * The top 4 bits (out of the 56 bits alloted for specifying vendor specific
> + * modifiers) denote the category for modifiers. Currently we have only two
> + * categories of modifiers ie AFBC and MISC. We can have a maximum of sixteen
> + * different categories.
> + */
> +#define DRM_FORMAT_MOD_ARM_CODE(type, val) \
> +       fourcc_mod_code(ARM, ((__u64)type << 52) | ((val) & 0x000fffffffffffffULL))
> +
> +#define DRM_FORMAT_MOD_ARM_TYPE_AFBC 0x00
> +#define DRM_FORMAT_MOD_ARM_TYPE_MISC 0x01
> +
> +#define DRM_FORMAT_MOD_ARM_AFBC(__afbc_mode) \
> +       DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_AFBC, __afbc_mode)
>
>  /*
>   * AFBC superblock size
> @@ -742,6 +756,17 @@ extern "C" {
>   */
>  #define AFBC_FORMAT_MOD_BCH     (1ULL << 11)
>
> +/*
> + * Arm 16x16 Block U-Interleaved modifier
> + *
> + * This is used by Arm Mali Utgard and Midgard GPUs. It divides the image
> + * into 16x16 pixel blocks. Blocks are stored linearly in order, but pixels
> + * in the block are reordered.
> + */
> +#define DRM_FORMAT_MOD_ARM_16X16_BLOCK_U_INTERLEAVED \
> +       DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_MISC, 1ULL)
> +
> +
>  /*
>   * Allwinner tiled modifier
>   *
> --
> 2.23.0
>