[Lima] [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 at arm.com>
> Cc: Rob Herring <robh at kernel.org>
> Cc: Alyssa Rosenzweig <alyssa at rosenzweig.io>
> Cc: Ayan Halder <Ayan.Halder at arm.com>
> Signed-off-by: Qiang Yu <yuq825 at 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
>