Re: [PATCHv4,05/36] drm/gem-fb-helper: Add generic afbc size checks

["james qian wang (Arm Technology China)" <james.qian.wang@arm.com>]

Hi Andrzej:

On Fri, Dec 13, 2019 at 04:58:36PM +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_fourcc.c                 | 10 +++-
>  drivers/gpu/drm/drm_framebuffer.c            |  3 +
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 60 ++++++++++++++++++--
>  include/drm/drm_gem_framebuffer_helper.h     | 16 ++++++
>  4 files changed, 82 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index d14dd7c86020..9ac2175c5bee 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -323,8 +323,14 @@ drm_get_format_info(struct drm_device *dev,
>  {
>  	const struct drm_format_info *info = NULL;
>  
> -	if (dev->mode_config.funcs->get_format_info)
> -		info = dev->mode_config.funcs->get_format_info(mode_cmd);
> +	/* bypass driver callback if afbc */
> +	if (!drm_is_afbc(mode_cmd->modifier[0]))
> +		if (dev->mode_config.funcs->get_format_info) {
> +			const struct drm_mode_config_funcs *funcs;
> +
> +			funcs = dev->mode_config.funcs;
> +			info = funcs->get_format_info(mode_cmd);
> +		}
>  
>  	if (!info)
>  		info = drm_format_info(mode_cmd->pixel_format);
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 57564318ceea..33b741cc73e8 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -204,6 +204,9 @@ static int framebuffer_check(struct drm_device *dev,
>  		unsigned int block_size = info->char_per_block[i];
>  		u64 min_pitch = drm_format_info_min_pitch(info, i, width);
>  
> +		if (drm_is_afbc(r->modifier[i]))
> +			block_size = 0;
> +
>  		if (!block_size && (r->modifier[i] == DRM_FORMAT_MOD_LINEAR)) {
>  			DRM_DEBUG_KMS("Format requires non-linear modifier for plane %d\n", i);
>  			return -EINVAL;
> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> index 4201dc1f32a5..e20f4d00b0a5 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
>   *
> @@ -200,6 +205,40 @@ 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 *afbc)
> +{
> +	u32 n_blocks;
> +
> +	if (!drm_afbc_get_superblock_wh(mode_cmd->modifier[0],
> +					&afbc->block_width,
> +					&afbc->block_height)) {
> +		return -EINVAL;
> +	}
> +
> +	/* tiled header afbc */
> +	if (mode_cmd->modifier[0] & AFBC_FORMAT_MOD_TILED) {
> +		afbc->block_width *= AFBC_TH_LAYOUT_ALIGNMENT;
> +		afbc->block_height *= AFBC_TH_LAYOUT_ALIGNMENT;
> +	}
> +
> +	afbc->aligned_width = ALIGN(mode_cmd->width, afbc->block_width);
> +	afbc->aligned_height = ALIGN(mode_cmd->height, afbc->block_height);
> +	afbc->offset = mode_cmd->offsets[0];
> +
> +	n_blocks = (afbc->aligned_width * afbc->aligned_height)
> +		 / AFBC_SUPERBLOCK_PIXELS;
> +	afbc->offset_payload = ALIGN(n_blocks * AFBC_HEADER_SIZE,
> +				     afbc->alignment_header);

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

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 display alignment, I think we can just follow the spec, use 64
for none-tiled-header, 4096 for tiled-header, but no need to caller to
pass a value.

> +
> +	afbc->afbc_size = afbc->offset_payload + n_blocks *
> +			  ALIGN(afbc->bpp * AFBC_SUPERBLOCK_PIXELS / 8,
> +				AFBC_SUPERBLOCK_ALIGNMENT);
> +
> +	return 0;
> +}
> +
>  /**
>   * drm_gem_fb_size_check_special() - Helper function for use in
>   *				     &drm_mode_config_funcs.fb_create
> @@ -218,6 +257,7 @@ int drm_gem_fb_size_check_special(struct drm_device *dev,
>  				  const struct drm_size_check *check,
>  				  struct drm_gem_object **objs)
>  {
> +#define CHECK_HAS(field) (check && check->field)
>  	const struct drm_format_info *info;
>  	int i;
>  
> @@ -231,24 +271,34 @@ int drm_gem_fb_size_check_special(struct drm_device *dev,
>  		unsigned int min_size;
>  		u32 pitch = mode_cmd->pitches[i];
>  
> -		if (check && check->use_pitch_multiplier)
> +		if (CHECK_HAS(use_pitch_multiplier))
>  			if ((pitch * check->pitch_multiplier[i]) %
>  			    check->pitch_modulo)
>  				return -EINVAL;
>  
> -		if (check && check->use_min_size)
> +		if (CHECK_HAS(use_min_size)) {
>  			min_size = check->min_size[i];
> -		else
> +		} else if (CHECK_HAS(data) &&
> +				drm_is_afbc(mode_cmd->modifier[0])) {
> +			struct drm_afbc *afbc;
> +			int ret;
> +
> +			afbc = check->data;
> +			ret = drm_gem_afbc_min_size(dev, mode_cmd, afbc);
> +			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;
> -
> +#undef CHECK_HAS
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_fb_size_check_special);
>  
> diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
> index 74304a268694..3d6015194b3c 100644
> --- a/include/drm/drm_gem_framebuffer_helper.h
> +++ b/include/drm/drm_gem_framebuffer_helper.h
> @@ -22,6 +22,22 @@ struct drm_size_check {
>  	u32 pitch_multiplier[4];
>  	u32 pitch_modulo;
>  	bool use_pitch_multiplier;
> +	void *data;
> +};
> +
> +/**
> + * struct drm_afbc - AFBC-specific data.
> + */
> +struct drm_afbc {
> +	u32 block_width;
> +	u32 block_height;
> +	u32 aligned_width;
> +	u32 aligned_height;
> +	u32 offset;
> +	u32 alignment_header;
> +	u32 afbc_size;
> +	u32 offset_payload;
> +	u32 bpp;

Seems we can remove this bpp if we set the block_size (4x4) for these afbc
only formats like DRM_FORMAT_YUV420_8BIT/10BIT in drm_format_info, and then
we can calculate the bpp like the pitch calculation, then no need
caller to specify bpp anymore, and vendor specific get_bpp() likes in malidp
and komeda also can be removed.

for this change maybe we can put it into a new series.

Thanks
James

>  };
>  
>  struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel