diff mbox series

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

Message ID 20191217145020.14645-2-andrzej.p@collabora.com (mailing list archive)
State New, archived
Headers show
Series Add AFBC support for Rockchip | expand

Commit Message

Andrzej Pietrasiewicz Dec. 17, 2019, 2:49 p.m. UTC 
Add checking if a modifier is afbc and getting afbc block size.

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
---
 drivers/gpu/drm/drm_fourcc.c | 53 ++++++++++++++++++++++++++++++++++++
 include/drm/drm_fourcc.h     |  4 +++
 2 files changed, 57 insertions(+)

Comments

James Qian Wang Feb. 18, 2020, 3:13 a.m. UTC | #1 
Hi Andrzej:

On Tue, Dec 17, 2019 at 03:49:47PM +0100, Andrzej Pietrasiewicz wrote:
> Add checking if a modifier is afbc and getting afbc block size.
> 
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> ---
>  drivers/gpu/drm/drm_fourcc.c | 53 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_fourcc.h     |  4 +++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index b234bfaeda06..d14dd7c86020 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -29,6 +29,7 @@
>  
>  #include <drm/drm_device.h>
>  #include <drm/drm_fourcc.h>
> +#include <drm/drm_print.h>
>  
>  static char printable_char(int c)
>  {
> @@ -393,3 +394,55 @@ uint64_t drm_format_info_min_pitch(const struct drm_format_info *info,
>  			    drm_format_info_block_height(info, plane));
>  }
>  EXPORT_SYMBOL(drm_format_info_min_pitch);
> +
> +/**
> + * drm_is_afbc - test if the modifier describes an afbc buffer
> + * @modifier - modifier to be tested
> + *
> + * Returns: true if the modifier describes an afbc buffer
> + */
> +bool drm_is_afbc(u64 modifier)
> +{
> +	/* is it ARM AFBC? */
> +	if ((modifier & DRM_FORMAT_MOD_ARM_AFBC(0)) == 0)
> +		return false;
> +
> +	/* Block size must be known */
> +	if ((modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) == 0)
> +		return false;

Do we really need this block size check here ?
Since modifier with ARM AFBC modifier but have no BLOCK_SIZE which
should be an error, but this check returns such error to NONE-AFBC.

And I saw you already have such error check in func
get_superblock_wh(), so I think we can del this size check in this
func.

James.

> +
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(drm_is_afbc);
> +
> +/**
> + * 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)
> +{
> +	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 */
> +	default:
> +		DRM_DEBUG_KMS("Invalid AFBC_FORMAT_MOD_BLOCK_SIZE: %lld.\n",
> +			      modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK);
> +		return false;
> +	}
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(drm_afbc_get_superblock_wh);
> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> index 306d1efeb5e0..7eb23062bf45 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -320,4 +320,8 @@ uint64_t drm_format_info_min_pitch(const struct drm_format_info *info,
>  				   int plane, unsigned int buffer_width);
>  const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
>  
> +bool drm_is_afbc(u64 modifier);
> +
> +bool drm_afbc_get_superblock_wh(u64 modifier, u32 *w, u32 *h);
> +
>  #endif /* __DRM_FOURCC_H__ */
> -- 
> 2.17.1
Boris Brezillon Feb. 20, 2020, 9:19 a.m. UTC | #2 
On Tue, 17 Dec 2019 15:49:47 +0100
Andrzej Pietrasiewicz <andrzej.p@collabora.com> wrote:

> Add checking if a modifier is afbc and getting afbc block size.
> 
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> ---
>  drivers/gpu/drm/drm_fourcc.c | 53 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_fourcc.h     |  4 +++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index b234bfaeda06..d14dd7c86020 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -29,6 +29,7 @@
>  
>  #include <drm/drm_device.h>
>  #include <drm/drm_fourcc.h>
> +#include <drm/drm_print.h>
>  
>  static char printable_char(int c)
>  {
> @@ -393,3 +394,55 @@ uint64_t drm_format_info_min_pitch(const struct drm_format_info *info,
>  			    drm_format_info_block_height(info, plane));
>  }
>  EXPORT_SYMBOL(drm_format_info_min_pitch);
> +
> +/**
> + * drm_is_afbc - test if the modifier describes an afbc buffer
> + * @modifier - modifier to be tested
> + *
> + * Returns: true if the modifier describes an afbc buffer
> + */
> +bool drm_is_afbc(u64 modifier)
> +{
> +	/* is it ARM AFBC? */
> +	if ((modifier & DRM_FORMAT_MOD_ARM_AFBC(0)) == 0)

Hm, it's not doing what you describe. The test should be something like

#define VENDOR_AND_TYPE_MASK GENMASK_ULL(63, 52)

	if ((mod & VENDOR_AND_TYPE_MASK) == DRM_FORMAT_MOD_ARM_AFBC(0))

> +		return false;
> +
> +	/* Block size must be known */
> +	if ((modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) == 0)
> +		return false;
> +
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(drm_is_afbc);
> +
> +/**
> + * 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)
> +{
> +	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 */

Any reason for not supporting those block sizes? It probably deserves a
comment, and a mention in the commit message.

> +	default:
> +		DRM_DEBUG_KMS("Invalid AFBC_FORMAT_MOD_BLOCK_SIZE: %lld.\n",
> +			      modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK);
> +		return false;
> +	}
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(drm_afbc_get_superblock_wh);
> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> index 306d1efeb5e0..7eb23062bf45 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -320,4 +320,8 @@ uint64_t drm_format_info_min_pitch(const struct drm_format_info *info,
>  				   int plane, unsigned int buffer_width);
>  const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
>  
> +bool drm_is_afbc(u64 modifier);
> +
> +bool drm_afbc_get_superblock_wh(u64 modifier, u32 *w, u32 *h);
> +
>  #endif /* __DRM_FOURCC_H__ */
Boris Brezillon Feb. 20, 2020, 10:47 a.m. UTC | #3 
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];
}
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index b234bfaeda06..d14dd7c86020 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -29,6 +29,7 @@ 
 
 #include <drm/drm_device.h>
 #include <drm/drm_fourcc.h>
+#include <drm/drm_print.h>
 
 static char printable_char(int c)
 {
@@ -393,3 +394,55 @@  uint64_t drm_format_info_min_pitch(const struct drm_format_info *info,
 			    drm_format_info_block_height(info, plane));
 }
 EXPORT_SYMBOL(drm_format_info_min_pitch);
+
+/**
+ * drm_is_afbc - test if the modifier describes an afbc buffer
+ * @modifier - modifier to be tested
+ *
+ * Returns: true if the modifier describes an afbc buffer
+ */
+bool drm_is_afbc(u64 modifier)
+{
+	/* is it ARM AFBC? */
+	if ((modifier & DRM_FORMAT_MOD_ARM_AFBC(0)) == 0)
+		return false;
+
+	/* Block size must be known */
+	if ((modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) == 0)
+		return false;
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(drm_is_afbc);
+
+/**
+ * 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)
+{
+	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 */
+	default:
+		DRM_DEBUG_KMS("Invalid AFBC_FORMAT_MOD_BLOCK_SIZE: %lld.\n",
+			      modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK);
+		return false;
+	}
+	return true;
+}
+EXPORT_SYMBOL_GPL(drm_afbc_get_superblock_wh);
diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index 306d1efeb5e0..7eb23062bf45 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -320,4 +320,8 @@  uint64_t drm_format_info_min_pitch(const struct drm_format_info *info,
 				   int plane, unsigned int buffer_width);
 const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
 
+bool drm_is_afbc(u64 modifier);
+
+bool drm_afbc_get_superblock_wh(u64 modifier, u32 *w, u32 *h);
+
 #endif /* __DRM_FOURCC_H__ */