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