diff mbox series

[PATCHv5,04/34] drm/gem-fb-helper: Add generic afbc size checks

Message ID 20191217145020.14645-5-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 
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(-)

Comments

James Qian Wang Feb. 18, 2020, 5:02 a.m. UTC | #1 
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
diff mbox series

Patch

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;
+	}
+
+	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);
+
+	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;
+	/**
+	 * @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;
+};
+
+#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,