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 at 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