[PATCH v3] drm/fourcc: add ARM GPU tile modifier

[PATCH v3] drm/fourcc: add ARM GPU tile modifier

[Qiang Yu]

v2: separate AFBC and GPU encoding

v3: rename device to type and GPU modifer name

Cc: Brian Starkey <Brian.Starkey@arm.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Cc: Ayan Halder <Ayan.Halder@arm.com>
Signed-off-by: Qiang Yu <yuq825@gmail.com>
---
 include/uapi/drm/drm_fourcc.h | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 9fa7cf7bb274..f80a675cb09a 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -601,6 +601,19 @@ extern "C" {
  */
 #define DRM_FORMAT_MOD_BROADCOM_UIF fourcc_mod_code(BROADCOM, 6)
 
+/*
+ * Arm Buffer Layout Type Code
+ *
+ * Arm has multiple types of buffer layout which are not totally shared
+ * across devices, so add a type field at the MSB of the format field
+ * to separate each type's encoding.
+ */
+#define DRM_FORMAT_MOD_ARM_TYPE_AFBC 0x00
+#define DRM_FORMAT_MOD_ARM_TYPE_AGTB 0x01
+
+#define DRM_FORMAT_MOD_ARM_CODE(type, val) \
+	fourcc_mod_code(ARM, ((__u64)type << 48) | ((val) & 0x0000ffffffffffffULL))
+
 /*
  * Arm Framebuffer Compression (AFBC) modifiers
  *
@@ -615,7 +628,8 @@ 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)
+#define DRM_FORMAT_MOD_ARM_AFBC(__afbc_mode) \
+	DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_AFBC, __afbc_mode)
 
 /*
  * AFBC superblock size
@@ -709,6 +723,21 @@ extern "C" {
  */
 #define AFBC_FORMAT_MOD_BCH     (1ULL << 11)
 
+/*
+ * Arm Graphics Tiled Buffer (AGTB) modifiers
+ */
+#define DRM_FORMAT_MOD_ARM_AGTB(mode) \
+	DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_AGTB, mode)
+
+/*
+ * AGTB mode 0 modifier
+ *
+ * This is used by ARM Mali Utgard/Midgard GPU. It divides buffer into
+ * 16x16 pixel blocks. Blocks are stored linearly in order, but pixels
+ * in the block are reordered.
+ */
+#define DRM_FORMAT_MOD_ARM_AGTB_MODE0 DRM_FORMAT_MOD_ARM_AGTB(1)
+
 /*
  * Allwinner tiled modifier
  *
-- 
2.17.1

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

Re: [PATCH v3] drm/fourcc: add ARM GPU tile modifier

[Alyssa Rosenzweig]

"AGTB" is jargon-y, but then again, so is "AFBC"... Unless Arm wants to
publish the actual name for the format, this works :)

Thank you for the clarification (in the other emails)

Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] drm/fourcc: add ARM GPU tile modifier

[Brian Starkey]

Hello Qiang,

Sorry for weighing in late on this, I've quite a backlog to work
through.

Since your first patch set, I did raise this internally. The request
has been making it's way through:

 - GPU engineering, to determine what exactly this format is, and
   what other variants there might be which are supported on different
   GPU generations, so that we can determine a sane naming convention.

 - Our legal team, to determine what detail we are happy to share from
   an IP perspective. I can't imagine there being an issue here, but
   process is process, and there's not a lot I can do to move that
   along.

There was talk on the legal side last week. I will ask for an update.
I realise this is taking a very long time, and for that I can only
apologise; I really am trying to get you an answer.

On Thu, Mar 14, 2019 at 07:13:48PM +0800, Qiang Yu wrote:
> v2: separate AFBC and GPU encoding
> 
> v3: rename device to type and GPU modifer name
> 
> Cc: Brian Starkey <Brian.Starkey@arm.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> Cc: Ayan Halder <Ayan.Halder@arm.com>
> Signed-off-by: Qiang Yu <yuq825@gmail.com>
> ---
>  include/uapi/drm/drm_fourcc.h | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 9fa7cf7bb274..f80a675cb09a 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -601,6 +601,19 @@ extern "C" {
>   */
>  #define DRM_FORMAT_MOD_BROADCOM_UIF fourcc_mod_code(BROADCOM, 6)
>  
> +/*
> + * Arm Buffer Layout Type Code
> + *
> + * Arm has multiple types of buffer layout which are not totally shared
> + * across devices, so add a type field at the MSB of the format field
> + * to separate each type's encoding.
> + */
> +#define DRM_FORMAT_MOD_ARM_TYPE_AFBC 0x00
> +#define DRM_FORMAT_MOD_ARM_TYPE_AGTB 0x01
> +

I don't think we need a whole byte here. We can just keep adding
individual bits as needed.

In any case, I'm not that keen on adding this "AGTB" category. That
effectively reserves 55 (or 47) bits just to describe a single layout.
We don't know if this is a "category" per-se, or just a single Utgard
tiling format - as discussed I'm trying to get an answer for that.

AFBC only uses bit-fields and the "__afbc_mode" helper macro because
it has many different "features" which can be combined quite freely.
That leads to combinatorial expansion, so when a new feature is
introduced, this can approximately double the list of possible AFBC
modifiers. That's rather unmanageable if we list them all
exhaustively.

For other layouts which don't have this kind of combinatorial effect,
I'd rather have #defines for the specific layouts, all individually
setting the top bit, without giving that top bit any kind of "name".
The top bit would effectively mean "not AFBC", rather than meaning
"AGTB", allowing us to use the lower bits more freely.

In the future, maybe Arm comes up with another layout with tons of
combinatorial variants (let's call it "AFBC2") - in which case I think
we'd add a new "__afbc2_mode" helper macro, which sets the top *two*
bits - but *only* if listing all of the AFBC2 variants directly wasn't
practical.

If you can hang on a while longer, I hope Arm can push a patch which
you can just use directly, and then handling the bit assignments will
be our mess rather than yours :-)

Thanks,
-Brian

> +#define DRM_FORMAT_MOD_ARM_CODE(type, val) \
> +	fourcc_mod_code(ARM, ((__u64)type << 48) | ((val) & 0x0000ffffffffffffULL))
> +
>  /*
>   * Arm Framebuffer Compression (AFBC) modifiers
>   *
> @@ -615,7 +628,8 @@ 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)
> +#define DRM_FORMAT_MOD_ARM_AFBC(__afbc_mode) \
> +	DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_AFBC, __afbc_mode)
>  
>  /*
>   * AFBC superblock size
> @@ -709,6 +723,21 @@ extern "C" {
>   */
>  #define AFBC_FORMAT_MOD_BCH     (1ULL << 11)
>  
> +/*
> + * Arm Graphics Tiled Buffer (AGTB) modifiers
> + */
> +#define DRM_FORMAT_MOD_ARM_AGTB(mode) \
> +	DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_AGTB, mode)
> +
> +/*
> + * AGTB mode 0 modifier
> + *
> + * This is used by ARM Mali Utgard/Midgard GPU. It divides buffer into
> + * 16x16 pixel blocks. Blocks are stored linearly in order, but pixels
> + * in the block are reordered.
> + */
> +#define DRM_FORMAT_MOD_ARM_AGTB_MODE0 DRM_FORMAT_MOD_ARM_AGTB(1)
> +
>  /*
>   * Allwinner tiled modifier
>   *
> -- 
> 2.17.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] drm/fourcc: add ARM GPU tile modifier

[Qiang Yu]

Hi Brian,

> Since your first patch set, I did raise this internally. The request
> has been making it's way through:
>
>  - GPU engineering, to determine what exactly this format is, and
>    what other variants there might be which are supported on different
>    GPU generations, so that we can determine a sane naming convention.
>
>  - Our legal team, to determine what detail we are happy to share from
>    an IP perspective. I can't imagine there being an issue here, but
>    process is process, and there's not a lot I can do to move that
>    along.
>
> There was talk on the legal side last week. I will ask for an update.
> I realise this is taking a very long time, and for that I can only
> apologise; I really am trying to get you an answer.

Thanks for the update and your effort to make a generic solution on
the ARM side. It's really some time passed since last time we talked
about this, but I think it's worth to wait for more time to get a reliable
solution with more internal information known by us.

> For other layouts which don't have this kind of combinatorial effect,
> I'd rather have #defines for the specific layouts, all individually
> setting the top bit, without giving that top bit any kind of "name".
> The top bit would effectively mean "not AFBC", rather than meaning
> "AGTB", allowing us to use the lower bits more freely.
>
Seems like another kind of category, but with different meaning.

> If you can hang on a while longer, I hope Arm can push a patch which
> you can just use directly, and then handling the bit assignments will
> be our mess rather than yours :-)
>
That's better, I can wait :-)

Thanks,
Qiang

> > +#define DRM_FORMAT_MOD_ARM_CODE(type, val) \
> > +     fourcc_mod_code(ARM, ((__u64)type << 48) | ((val) & 0x0000ffffffffffffULL))
> > +
> >  /*
> >   * Arm Framebuffer Compression (AFBC) modifiers
> >   *
> > @@ -615,7 +628,8 @@ 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)
> > +#define DRM_FORMAT_MOD_ARM_AFBC(__afbc_mode) \
> > +     DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_AFBC, __afbc_mode)
> >
> >  /*
> >   * AFBC superblock size
> > @@ -709,6 +723,21 @@ extern "C" {
> >   */
> >  #define AFBC_FORMAT_MOD_BCH     (1ULL << 11)
> >
> > +/*
> > + * Arm Graphics Tiled Buffer (AGTB) modifiers
> > + */
> > +#define DRM_FORMAT_MOD_ARM_AGTB(mode) \
> > +     DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_AGTB, mode)
> > +
> > +/*
> > + * AGTB mode 0 modifier
> > + *
> > + * This is used by ARM Mali Utgard/Midgard GPU. It divides buffer into
> > + * 16x16 pixel blocks. Blocks are stored linearly in order, but pixels
> > + * in the block are reordered.
> > + */
> > +#define DRM_FORMAT_MOD_ARM_AGTB_MODE0 DRM_FORMAT_MOD_ARM_AGTB(1)
> > +
> >  /*
> >   * Allwinner tiled modifier
> >   *
> > --
> > 2.17.1
> >
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] drm/fourcc: add ARM GPU tile modifier

[Brian Starkey]

On Tue, Mar 19, 2019 at 08:05:32PM +0800, Qiang Yu wrote:
> Hi Brian,
> 
> > Since your first patch set, I did raise this internally. The request
> > has been making it's way through:
> >
> >  - GPU engineering, to determine what exactly this format is, and
> >    what other variants there might be which are supported on different
> >    GPU generations, so that we can determine a sane naming convention.
> >
> >  - Our legal team, to determine what detail we are happy to share from
> >    an IP perspective. I can't imagine there being an issue here, but
> >    process is process, and there's not a lot I can do to move that
> >    along.
> >
> > There was talk on the legal side last week. I will ask for an update.
> > I realise this is taking a very long time, and for that I can only
> > apologise; I really am trying to get you an answer.
> 
> Thanks for the update and your effort to make a generic solution on
> the ARM side. It's really some time passed since last time we talked
> about this, but I think it's worth to wait for more time to get a reliable
> solution with more internal information known by us.

Thank you for your patience :-)

> 
> > For other layouts which don't have this kind of combinatorial effect,
> > I'd rather have #defines for the specific layouts, all individually
> > setting the top bit, without giving that top bit any kind of "name".
> > The top bit would effectively mean "not AFBC", rather than meaning
> > "AGTB", allowing us to use the lower bits more freely.
> >
> Seems like another kind of category, but with different meaning.

Indeed, just a category without a name ;-)

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

Re: [PATCH v3] drm/fourcc: add ARM GPU tile modifier

[Alyssa Rosenzweig]

> We don't know if this is a "category" per-se, or just a single Utgard
> tiling format - as discussed I'm trying to get an answer for that.

FWIW, as I think I mentioned on an message, this format is used on
Midgard as well, and presumably also Bifrost.

On Midgard, when a texture is uploaded (just as a plain bitmap), the
driver tiles it in this format and uploads that, to improve cache
locality.

That said, unlike on Lima/Utgard, Panfrost does not need this exposed in
kernel space, since I don't believe the format is renderable on Midgard.
Any time Qiang would need to import/export something in this tiled
format, I would just use AFBC for the same effect + bw savings.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel