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

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

On Tue, 17 Dec 2019 15:49:47 +0100
Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxxxx> 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];
}
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel