Re: [PATCHv5 04/34] drm/gem-fb-helper: Add generic afbc size checks

From: "james qian wang (Arm Technology China)" <james.qian.wang@arm.com>
To: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Cc: nd@arm.com, Ayan Halder <Ayan.Halder@arm.com>,
	kernel@collabora.com, David Airlie <airlied@linux.ie>,
	Liviu Dudau <liviu.dudau@arm.com>,
	Sandy Huang <hjc@rock-chips.com>,
	dri-devel@lists.freedesktop.org,
	Mihail Atanassov <mihail.atanassov@arm.com>,
	Sean Paul <sean@poorly.run>
Subject: Re: [PATCHv5 04/34] drm/gem-fb-helper: Add generic afbc size checks
Date: Tue, 18 Feb 2020 13:02:36 +0800	[thread overview]
Message-ID: <20200218050236.GA10649@jamwan02-TSP300> (raw)
In-Reply-To: <20191217145020.14645-5-andrzej.p@collabora.com>

Hi Andrzej:

On Tue, Dec 17, 2019 at 03:49:50PM +0100, Andrzej Pietrasiewicz wrote:
> Extend the size-checking special function to handle afbc.
> 
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> ---
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 49 +++++++++++++++++--
>  include/drm/drm_framebuffer.h                | 50 ++++++++++++++++++++
>  include/drm/drm_gem_framebuffer_helper.h     |  1 +
>  3 files changed, 96 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> index d2fce1ec8f37..5fe9032a5ee8 100644
> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -21,6 +21,11 @@
>  #include <drm/drm_modeset_helper.h>
>  #include <drm/drm_simple_kms_helper.h>
>  
> +#define AFBC_HEADER_SIZE		16
> +#define AFBC_TH_LAYOUT_ALIGNMENT	8
> +#define AFBC_SUPERBLOCK_PIXELS		256
> +#define AFBC_SUPERBLOCK_ALIGNMENT	128
> +
>  /**
>   * DOC: overview
>   *
> @@ -299,6 +304,34 @@ int drm_gem_fb_lookup(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_fb_lookup);
>  
> +static int drm_gem_afbc_min_size(struct drm_device *dev,
> +				 const struct drm_mode_fb_cmd2 *mode_cmd,
> +				 struct drm_afbc_framebuffer *afbc_fb)
> +{
> +	u32 n_blocks;
> +
> +	if (!drm_afbc_get_superblock_wh(mode_cmd->modifier[0], &afbc_fb->block_width, &afbc_fb->block_height))
> +		return -EINVAL;
> +
> +	/* tiled header afbc */
> +	if (mode_cmd->modifier[0] & AFBC_FORMAT_MOD_TILED) {
> +		afbc_fb->block_width *= AFBC_TH_LAYOUT_ALIGNMENT;
> +		afbc_fb->block_height *= AFBC_TH_LAYOUT_ALIGNMENT;
> +	}

TBH, here caculated afbc_fb->block_with/height are not
block_width/height, but fb w/h alignment.
Per my understanding, afbc only has block size: 16x16, 32x8, 64x4 ...
generally the afbc w/h alignment according the the block_size, but once the
tiled header enabled, since one tiled header describes 8x8 superblocks,
so the alignment of w/h need to mutiple 8.

So I think we'd better name the variable to width/height_alignment.

BTW: no matter block_w/h or w/h_alignmtent are only for size
calculation, seems no need to store them to afbc_fb.

> +
> +	afbc_fb->aligned_width = ALIGN(mode_cmd->width, afbc_fb->block_width);
> +	afbc_fb->aligned_height = ALIGN(mode_cmd->height, afbc_fb->block_height);
> +	afbc_fb->offset = mode_cmd->offsets[0];
> +
> +	n_blocks = (afbc_fb->aligned_width * afbc_fb->aligned_height) / AFBC_SUPERBLOCK_PIXELS;
> +	afbc_fb->offset_payload = ALIGN(n_blocks * AFBC_HEADER_SIZE, afbc_fb->alignment_header);
> +

After check the references in malidp, rockchip and komeda, seems this
afbc->alignment_header is dedicated for komeda only and a pass in
argument.

This is not true. Per afbc HW spec alignment is essential for
all afbc usage. according to the spec the requiremnt are:

  AFBC1.0/1.1: 64 byte alignment both for header and body buffer.
  AFBC1.2 (tiled header enabled): 4096 alignment.

So this alignement is not a vendor specific value, but afbc feature
requirement, can be determined by afbc modifier.
(malidp and komeda obeys this spec, not sure about Rockchip, but I
think it should be)

But you may see, komeda uses 1024 (not 64) for none-tiled-header afbc,
that's because GPU(MALI) changed this value to 1024 for bus
performance (sorry I don't know the detail), and komeda changed to
1024 to follow.

Back to alignment_header here, I think we can just follow the spec, use 64
for none-tiled-header, 4096 for tiled-header, and no need to let the caller
to specify it

> +	afbc_fb->afbc_size = afbc_fb->offset_payload
> +			   + n_blocks * ALIGN(afbc_fb->bpp * AFBC_SUPERBLOCK_PIXELS / 8, AFBC_SUPERBLOCK_ALIGNMENT);
> +
> +	return 0;
> +}
> +
>  /**
>   * drm_gem_fb_size_check2() - Helper function for use in
>   *			      &drm_mode_config_funcs.fb_create implementations
> @@ -334,19 +367,27 @@ int drm_gem_fb_size_check2(struct drm_device *dev,
>  			    check->pitch_modulo)
>  				return -EINVAL;
>  
> -		if (check && check->use_min_size)
> +		if (check && check->use_min_size) {
>  			min_size = check->min_size[i];
> -		else
> +		} else if (check && check->data && drm_is_afbc(mode_cmd->modifier[0])) {
> +			struct drm_afbc_framebuffer *afbc_fb;
> +			int ret;
> +
> +			afbc_fb = check->data;
> +			ret = drm_gem_afbc_min_size(dev, mode_cmd, afbc_fb);
> +			if (ret < 0)
> +				return ret;
> +			min_size = ret;
> +		} else {
>  			min_size = (height - 1) * pitch
>  				 + drm_format_info_min_pitch(info, i, width)
>  				 + mode_cmd->offsets[i];
> -
> +		}
>  		if (objs[i]->size < min_size)
>  			return -EINVAL;
>  	}
>  
>  	return 0;
> -
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_fb_size_check2);
>  
> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> index c0e0256e3e98..c8a06e37585a 100644
> --- a/include/drm/drm_framebuffer.h
> +++ b/include/drm/drm_framebuffer.h
> @@ -297,4 +297,54 @@ int drm_framebuffer_plane_width(int width,
>  int drm_framebuffer_plane_height(int height,
>  				 const struct drm_framebuffer *fb, int plane);
>  
> +/**
> + * struct drm_afbc_framebuffer - a special afbc frame buffer object
> + *
> + * A derived class of struct drm_framebuffer, dedicated for afbc use cases.
> + */
> +struct drm_afbc_framebuffer {
> +	/**
> +	 * @base: base framebuffer structure.
> +	 */
> +	struct drm_framebuffer base;
> +	/**
> +	 * @block_widht: width of a single afbc block
> +	 */
> +	u32 block_width;
> +	/**
> +	 * @block_widht: height of a single afbc block
> +	 */
> +	u32 block_height;
> +	/**
> +	 * @aligned_width: aligned frame buffer width
> +	 */
> +	u32 aligned_width;
> +	/**
> +	 * @aligned_height: aligned frame buffer height
> +	 */
> +	u32 aligned_height;
> +	/**
> +	 * @offset: offset of the first afbc header
> +	 */
> +	u32 offset;

Since malidp and komeda have no requirement for none-zero offset, so I
think we can reject none zero offset as error like did in rockchip in
afbc_size_check().

> +	/**
> +	 * @alignment_header: required alignment for afbc headers
> +	 */
> +	u32 alignment_header;
> +	/**
> +	 * @afbc_size: minimum size of afbc buffer
> +	 */
> +	u32 afbc_size;
> +	/**
> +	 * @offset_payload: start of afbc body buffer
> +	 */
> +	u32 offset_payload;
> +	/**
> +	 * @bpp: bpp value for this afbc buffer
> +	 */
> +	u32 bpp;

Seems we can remove this bpp or no need to define it as a pass in argument
for size check, maybe the komeda/malidp get_afbc_bpp() function mislead
you that afbc formats may have vendor specific bpp.

But the story is:

for afbc only formats like DRM_FORMAT_YUV420_8BIT/10BIT, we have set
nothing in drm_format_info, neither cpp nor block_size, so both malidp
or komeda introduce a get_bpp(), but actually the two funcs basically
are same.

So my suggestion is we can temporary use the get_afbc_bpp() in malidp
or komeda. and eventually I think we'd better set the block size
for these formats, then we can defines a common get_bpp() like pitch

Thanks
James

> +};
> +
> +#define fb_to_afbc_fb(x) container_of(x, struct drm_afbc_framebuffer, base)
> +
>  #endif
> diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
> index 4955af96d6c3..17e3f849a0fb 100644
> --- a/include/drm/drm_gem_framebuffer_helper.h
> +++ b/include/drm/drm_gem_framebuffer_helper.h
> @@ -22,6 +22,7 @@ struct drm_size_check {
>  	u32 pitch_multiplier[4];
>  	u32 pitch_modulo;
>  	bool use_pitch_multiplier;
> +	void *data;
>  };
>  
>  struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
> -- 
> 2.17.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel