Re: [PATCH v2] drm/fourcc: add ARM GPU tile format

From: Ayan Halder <Ayan.Halder@arm.com>
To: Qiang Yu <yuq825@gmail.com>
Cc: "lima@lists.freedesktop.org" <lima@lists.freedesktop.org>,
	Maxime Ripard <maxime.ripard@bootlin.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	David Airlie <airlied@linux.ie>, nd <nd@arm.com>,
	Sean Paul <sean@poorly.run>,
	Alyssa Rosenzweig <alyssa@rosenzweig.io>
Subject: Re: [PATCH v2] drm/fourcc: add ARM GPU tile format
Date: Mon, 11 Mar 2019 16:00:10 +0000	[thread overview]
Message-ID: <20190311160009.GA11281@arm.com> (raw)
In-Reply-To: <20190309140902.9871-1-yuq825@gmail.com>

On Sat, Mar 09, 2019 at 10:09:02PM +0800, Qiang Yu wrote:

Hi Qiang,

Apologies for jumping to the very initial patch. I wanted to have some
understanding of the newly proposed modifiers since they seem to me a
duplicate of the existing AFBC modifiers.

> v2: seperate AFBC and GPU encoding
> 
> Cc: Brian Starkey <Brian.Starkey@arm.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> 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..7af5197e7ebc 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 Device code
> + *
> + * Arm has multiple devices which do not share buffer format,
> + * so add a device field at the MSB of the format field to seperate
> + * each device's encoding.
> + */
> +#define DRM_FORMAT_MOD_ARM_DEVICE_AFBC 0x00
Why can't you reuse the existing DRM_FORMAT_MOD_VENDOR_ARM ?

> +#define DRM_FORMAT_MOD_ARM_DEVICE_GPU  0x01
What is the purpose of this? AFBC modifiers are supposed to be used
across gpus, dpus which support AFBC buffers rendering.

> +
> +#define DRM_FORMAT_MOD_ARM_CODE(device, val) \
> +	fourcc_mod_code(ARM, ((__u64)device << 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_DEVICE_AFBC, __afbc_mode)
>  
Similar question? Why can't you reuse the existing one?

>  /*
>   * AFBC superblock size
> @@ -709,6 +723,21 @@ extern "C" {
>   */
>  #define AFBC_FORMAT_MOD_BCH     (1ULL << 11)
>  
> +/*
> + * Arm GPU modifiers
> + */
> +#define DRM_FORMAT_MOD_ARM_GPU(mode) \
> +	DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_DEVICE_GPU, mode)
> +
> +/*
> + * Arm GPU tiled format
> + *
> + * 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_GPU_TILED DRM_FORMAT_MOD_ARM_GPU(1)
> +
You might want to re-use the exisiting modifier AFBC_FORMAT_MOD_BLOCK_SIZE_16x16.

I would suggest you to have a look at the exisiting AFBC modifiers
(denoted by AFBC_FORMAT_MOD_XXX ) and let us know if there is something
you cannot reuse.

>  /*
>   * Allwinner tiled modifier
>   *
> -- 
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel