Re: [PATCHv5 01/34] drm/core: Add afbc helper functions

Boris Brezillon
2020-02-20T10:48:04Z

On Tue, 17 Dec 2019 15:49:47 +0100
Andrzej Pietrasiewicz <andrzej.p@collabora.com> wrote:

> +/**
> + * drm_afbc_get_superblock_wh - extract afbc block width/height from modifier
> + * @modifier: the modifier to be looked at
> + * @w: address of a place to store the block width
> + * @h: address of a place to store the block height
> + *
> + * Returns: true if the modifier describes a supported block size
> + */
> +bool drm_afbc_get_superblock_wh(u64 modifier, u32 *w, u32 *h)

You should probably take the multiplane case into account now.
Maybe introduce the following struct and pass a pointer to such
a struct instead of the w/h pointers:

struct afbc_block_size {
struct {
u32 w;
u32 h;
} plane[2];
};

Note that you could also directly return a
const struct afbc_block_size * and consider the NULL case as
'invalid format'.

> +{
> + switch (modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> + case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> + *w = 16;
> + *h = 16;
> + break;
> + case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
> + *w = 32;
> + *h = 8;
> + break;
> + case AFBC_FORMAT_MOD_BLOCK_SIZE_64x4:
> + /* fall through */
> + case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4:
> + /* fall through */

I guess display controllers might support a subset of what's actually
defined in the spec, so maybe it makes sense to pass a 'const u8
*supported_block_sizes' and then do something like:

block_size_id = modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK;

for (i = 0; i < num_supported_block_sizes; i++) {
if (supported_block_sizes[i] == block_size_id)
break;
}

if (i == num_supported_block_sizes)
return false;

The above switch-case can also be replaced by an array of structs
encoding the block size:

static const struct afbc_block_size block_sizes[] = {
[AFBC_FORMAT_MOD_BLOCK_SIZE_16x16] = { { 16, 16 } },
[AFBC_FORMAT_MOD_BLOCK_SIZE_32x8] = { { 32, 8 } },
[AFBC_FORMAT_MOD_BLOCK_SIZE_64x4] = { { 64, 4 } },
[AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4] = { { 32, 8 }, { 64, 4} },
};

*block_size = block_sizes[block_size_id];

return true;

> + default:
> + DRM_DEBUG_KMS("Invalid AFBC_FORMAT_MOD_BLOCK_SIZE: %lld.\n",
> + modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK);
> + return false;
> + }
> + return true;
> +}

To sum-up, this would give something like (not even compile-tested):

struct afbc_block_size {
struct {
u32 width;
u32 height;
} plane[2];
};

static const struct afbc_block_size superblock_sizes[] = {
[AFBC_FORMAT_MOD_BLOCK_SIZE_16x16] = { { 16, 16 } },
[AFBC_FORMAT_MOD_BLOCK_SIZE_32x8] = { { 32, 8 } },
[AFBC_FORMAT_MOD_BLOCK_SIZE_64x4] = { { 64, 4 } },
[AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4] = { { 32, 8 }, { 64, 4} },
};

const struct afbc_block_size *
drm_afbc_get_superblock_info(u64 modifier,
const u8 *supported_sb_sizes,
unsigned int num_supported_sb_sizes)
{
u8 block_size_id = modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK;

if (!block_size_id ||
block_size_id >= ARRAY_SIZE(superblock_sizes)) {
DRM_DEBUG_KMS("Invalid AFBC_FORMAT_MOD_BLOCK_SIZE: %lld.\n",
modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK);
return NULL;
}

for (i = 0; i < num_supported_sb_sizes; i++) {
if (supported_sb_sizes[i] == block_size_id)
break;
}

if (i == num_supported_sb_sizes) {
DRM_DEBUG_KMS("Unsupported AFBC_FORMAT_MOD_BLOCK_SIZE: %lld.\n",
modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK);
return NULL;
}

return &superblock_sizes[block_size_id];