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

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]; }