[PATCH 0/2] AFBC for Rockchip

[PATCH 0/2] AFBC for Rockchip

[Andrzej Pietrasiewicz]

This series adds AFBC support for Rockchip.
It is inspired by:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/factory-gru-9017.B-chromeos-4.4/drivers/gpu/drm/rockchip/rockchip_drm_vop.c

The first patch factors out some afbc helper functions from malidp,
as they are useful in general. The second patch adds implementation proper
of AFBC support for Rockchip.

Andrzej Pietrasiewicz (2):
  drm/arm: Factor out generic afbc helpers
  drm/rockchip: Add support for afbc

 drivers/gpu/drm/Kconfig                     |   4 +
 drivers/gpu/drm/Makefile                    |   1 +
 drivers/gpu/drm/arm/Kconfig                 |   1 +
 drivers/gpu/drm/arm/malidp_drv.c            |  58 +-------
 drivers/gpu/drm/drm_afbc.c                  | 114 ++++++++++++++++
 drivers/gpu/drm/rockchip/Kconfig            |   1 +
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c  |  43 ++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 140 +++++++++++++++++++-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.h |  12 ++
 drivers/gpu/drm/rockchip/rockchip_vop_reg.c |  86 +++++++++++-
 include/drm/drm_afbc.h                      |  25 ++++
 11 files changed, 427 insertions(+), 58 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_afbc.c
 create mode 100644 include/drm/drm_afbc.h

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] drm/arm: Factor out generic afbc helpers

[Andrzej Pietrasiewicz]

These are useful for other users of afbc, e.g. rockchip.

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
---
 drivers/gpu/drm/Kconfig          |   4 ++
 drivers/gpu/drm/Makefile         |   1 +
 drivers/gpu/drm/arm/Kconfig      |   1 +
 drivers/gpu/drm/arm/malidp_drv.c |  58 ++--------------
 drivers/gpu/drm/drm_afbc.c       | 114 +++++++++++++++++++++++++++++++
 include/drm/drm_afbc.h           |  25 +++++++
 6 files changed, 149 insertions(+), 54 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_afbc.c
 create mode 100644 include/drm/drm_afbc.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 3c88420e3497..00e3f90557f4 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -195,6 +195,10 @@ config DRM_SCHED
 	tristate
 	depends on DRM
 
+config DRM_AFBC
+	tristate
+	depends on DRM
+
 source "drivers/gpu/drm/i2c/Kconfig"
 
 source "drivers/gpu/drm/arm/Kconfig"
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 9f0d2ee35794..55368b668355 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -31,6 +31,7 @@ drm-$(CONFIG_OF) += drm_of.o
 drm-$(CONFIG_AGP) += drm_agpsupport.o
 drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
 drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
+drm-$(CONFIG_DRM_AFBC) += drm_afbc.o
 
 drm_vram_helper-y := drm_gem_vram_helper.o \
 		     drm_vram_helper_common.o \
diff --git a/drivers/gpu/drm/arm/Kconfig b/drivers/gpu/drm/arm/Kconfig
index a204103b3efb..25c3dc408cda 100644
--- a/drivers/gpu/drm/arm/Kconfig
+++ b/drivers/gpu/drm/arm/Kconfig
@@ -29,6 +29,7 @@ config DRM_MALI_DISPLAY
 	select DRM_KMS_HELPER
 	select DRM_KMS_CMA_HELPER
 	select DRM_GEM_CMA_HELPER
+	select DRM_AFBC
 	select VIDEOMODE_HELPERS
 	help
 	  Choose this option if you want to compile the ARM Mali Display
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index f25ec4382277..a67b69e08f63 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -16,6 +16,7 @@
 #include <linux/debugfs.h>
 
 #include <drm/drmP.h>
+#include <drm/drm_afbc.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc.h>
@@ -33,8 +34,6 @@
 #include "malidp_hw.h"
 
 #define MALIDP_CONF_VALID_TIMEOUT	250
-#define AFBC_HEADER_SIZE		16
-#define AFBC_SUPERBLK_ALIGNMENT		128
 
 static void malidp_write_gamma_table(struct malidp_hw_device *hwdev,
 				     u32 data[MALIDP_COEFFTAB_NUM_COEFFS])
@@ -275,24 +274,8 @@ malidp_verify_afbc_framebuffer_caps(struct drm_device *dev,
 					mode_cmd->modifier[0]) == false)
 		return false;
 
-	if (mode_cmd->offsets[0] != 0) {
-		DRM_DEBUG_KMS("AFBC buffers' plane offset should be 0\n");
-		return false;
-	}
-
-	switch (mode_cmd->modifier[0] & AFBC_SIZE_MASK) {
-	case AFBC_SIZE_16X16:
-		if ((mode_cmd->width % 16) || (mode_cmd->height % 16)) {
-			DRM_DEBUG_KMS("AFBC buffers must be aligned to 16 pixels\n");
-			return false;
-		}
-		break;
-	default:
-		DRM_DEBUG_KMS("Unsupported AFBC block size\n");
-		return false;
-	}
-
-	return true;
+	return drm_afbc_check_offset(dev, mode_cmd) &&
+	       drm_afbc_check_size_align(dev, mode_cmd);
 }
 
 static bool
@@ -300,53 +283,20 @@ malidp_verify_afbc_framebuffer_size(struct drm_device *dev,
 				    struct drm_file *file,
 				    const struct drm_mode_fb_cmd2 *mode_cmd)
 {
-	int n_superblocks = 0;
 	const struct drm_format_info *info;
 	struct drm_gem_object *objs = NULL;
-	u32 afbc_superblock_size = 0, afbc_superblock_height = 0;
-	u32 afbc_superblock_width = 0, afbc_size = 0;
 	int bpp = 0;
 
-	switch (mode_cmd->modifier[0] & AFBC_SIZE_MASK) {
-	case AFBC_SIZE_16X16:
-		afbc_superblock_height = 16;
-		afbc_superblock_width = 16;
-		break;
-	default:
-		DRM_DEBUG_KMS("AFBC superblock size is not supported\n");
-		return false;
-	}
-
 	info = drm_get_format_info(dev, mode_cmd);
-
-	n_superblocks = (mode_cmd->width / afbc_superblock_width) *
-		(mode_cmd->height / afbc_superblock_height);
-
 	bpp = malidp_format_get_bpp(info->format);
 
-	afbc_superblock_size = (bpp * afbc_superblock_width * afbc_superblock_height)
-				/ BITS_PER_BYTE;
-
-	afbc_size = ALIGN(n_superblocks * AFBC_HEADER_SIZE, AFBC_SUPERBLK_ALIGNMENT);
-	afbc_size += n_superblocks * ALIGN(afbc_superblock_size, AFBC_SUPERBLK_ALIGNMENT);
-
-	if ((mode_cmd->width * bpp) != (mode_cmd->pitches[0] * BITS_PER_BYTE)) {
-		DRM_DEBUG_KMS("Invalid value of (pitch * BITS_PER_BYTE) (=%u) "
-			      "should be same as width (=%u) * bpp (=%u)\n",
-			      (mode_cmd->pitches[0] * BITS_PER_BYTE),
-			      mode_cmd->width, bpp);
-		return false;
-	}
-
 	objs = drm_gem_object_lookup(file, mode_cmd->handles[0]);
 	if (!objs) {
 		DRM_DEBUG_KMS("Failed to lookup GEM object\n");
 		return false;
 	}
 
-	if (objs->size < afbc_size) {
-		DRM_DEBUG_KMS("buffer size (%zu) too small for AFBC buffer size = %u\n",
-			      objs->size, afbc_size);
+	if (!drm_afbc_check_fb_size(dev, mode_cmd, objs, bpp)) {
 		drm_gem_object_put_unlocked(objs);
 		return false;
 	}
diff --git a/drivers/gpu/drm/drm_afbc.c b/drivers/gpu/drm/drm_afbc.c
new file mode 100644
index 000000000000..3e8a9225fd2e
--- /dev/null
+++ b/drivers/gpu/drm/drm_afbc.c
@@ -0,0 +1,114 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * (C) 2019 Collabora Ltd.
+ *
+ * author: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
+ *
+ */
+#include <linux/module.h>
+
+#include <drm/drm_afbc.h>
+#include <drm/drm_device.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_gem.h>
+#include <drm/drm_mode.h>
+#include <drm/drm_print.h>
+
+#define AFBC_HEADER_SIZE		16
+#define AFBC_SUPERBLK_ALIGNMENT		128
+
+bool drm_afbc_check_offset(struct drm_device *dev,
+			   const struct drm_mode_fb_cmd2 *mode_cmd)
+{
+	if (mode_cmd->offsets[0] != 0) {
+		DRM_DEBUG_KMS("AFBC buffers' plane offset should be 0\n");
+		return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(drm_afbc_check_offset);
+
+bool drm_afbc_check_size_align(struct drm_device *dev,
+			       const struct drm_mode_fb_cmd2 *mode_cmd)
+{
+
+	switch (mode_cmd->modifier[0] & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
+	case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
+		if ((mode_cmd->width % 16) || (mode_cmd->height % 16)) {
+			DRM_DEBUG_KMS(
+				"AFBC buffer must be aligned to 16 pixels\n"
+			);
+			return false;
+		}
+		break;
+	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
+		/* fall through */
+	case AFBC_FORMAT_MOD_BLOCK_SIZE_64x4:
+		/* fall through */
+	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4:
+		/* fall through */
+	default:
+		DRM_DEBUG_KMS("Unsupported AFBC block size\n");
+		return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(drm_afbc_check_size_align);
+
+bool drm_afbc_check_fb_size(struct drm_device *dev,
+			    const struct drm_mode_fb_cmd2 *mode_cmd,
+			    struct drm_gem_object *objs, int bpp)
+{
+	int n_superblocks = 0;
+	u32 afbc_superblock_size = 0, afbc_superblock_height = 0;
+	u32 afbc_superblock_width = 0, afbc_size = 0;
+
+	switch (mode_cmd->modifier[0] & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
+	case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
+		afbc_superblock_height = 16;
+		afbc_superblock_width = 16;
+		break;
+	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
+		/* fall through */
+	case AFBC_FORMAT_MOD_BLOCK_SIZE_64x4:
+		/* fall through */
+	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4:
+		/* fall through */
+	default:
+		DRM_DEBUG_KMS("AFBC superblock size is not supported\n");
+		return false;
+	}
+
+	n_superblocks = (mode_cmd->width / afbc_superblock_width) *
+		(mode_cmd->height / afbc_superblock_height);
+
+	afbc_superblock_size =
+		(bpp * afbc_superblock_width * afbc_superblock_height)
+			/ BITS_PER_BYTE;
+
+	afbc_size = ALIGN(n_superblocks * AFBC_HEADER_SIZE,
+			  AFBC_SUPERBLK_ALIGNMENT);
+	afbc_size += n_superblocks *
+		ALIGN(afbc_superblock_size, AFBC_SUPERBLK_ALIGNMENT);
+
+	if ((mode_cmd->width * bpp) != (mode_cmd->pitches[0] * BITS_PER_BYTE)) {
+		DRM_DEBUG_KMS("Invalid value of (pitch * BITS_PER_BYTE) (=%u) should be same as width (=%u) * bpp (=%u)\n",
+			mode_cmd->pitches[0] * BITS_PER_BYTE,
+			mode_cmd->width, bpp
+		);
+		return false;
+	}
+
+	if (objs->size < afbc_size) {
+		DRM_DEBUG_KMS("buffer size (%zu) too small for AFBC buffer size = %u\n",
+			objs->size, afbc_size
+		);
+
+		return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL(drm_afbc_check_fb_size);
diff --git a/include/drm/drm_afbc.h b/include/drm/drm_afbc.h
new file mode 100644
index 000000000000..ce39c850217b
--- /dev/null
+++ b/include/drm/drm_afbc.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * (C) 2019 Collabora Ltd.
+ *
+ * author: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
+ *
+ */
+#ifndef __DRM_AFBC_H__
+#define __DRM_AFBC_H__
+
+struct drm_device;
+struct drm_mode_fb_cmd2;
+struct drm_gem_object;
+
+bool drm_afbc_check_offset(struct drm_device *dev,
+			   const struct drm_mode_fb_cmd2 *mode_cmd);
+
+bool drm_afbc_check_size_align(struct drm_device *dev,
+			       const struct drm_mode_fb_cmd2 *mode_cmd);
+
+bool drm_afbc_check_fb_size(struct drm_device *dev,
+			    const struct drm_mode_fb_cmd2 *mode_cmd,
+			    struct drm_gem_object *objs, int bpp);
+
+#endif /* __DRM_AFBC_H__ */
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] drm/rockchip: Add support for afbc

[Andrzej Pietrasiewicz]

This patch adds support for afbc handling. afbc is a compressed format
which reduces the necessary memory bandwidth.

Co-developed-by: Mark Yao <mark.yao@rock-chips.com>
Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
---
 drivers/gpu/drm/rockchip/Kconfig            |   1 +
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c  |  43 ++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 140 +++++++++++++++++++-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.h |  12 ++
 drivers/gpu/drm/rockchip/rockchip_vop_reg.c |  86 +++++++++++-
 5 files changed, 278 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig
index 6f4222f8beeb..ff491efc52a5 100644
--- a/drivers/gpu/drm/rockchip/Kconfig
+++ b/drivers/gpu/drm/rockchip/Kconfig
@@ -5,6 +5,7 @@ config DRM_ROCKCHIP
 	select DRM_GEM_CMA_HELPER
 	select DRM_KMS_HELPER
 	select DRM_PANEL
+	select DRM_AFBC
 	select VIDEOMODE_HELPERS
 	select DRM_ANALOGIX_DP if ROCKCHIP_ANALOGIX_DP
 	select DRM_DW_HDMI if ROCKCHIP_DW_HDMI
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index 64ca87cf6d50..873185b3a721 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -8,6 +8,7 @@
 #include <drm/drm.h>
 #include <drm/drmP.h>
 #include <drm/drm_atomic.h>
+#include <drm/drm_afbc.h>
 #include <drm/drm_damage_helper.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
@@ -18,6 +19,8 @@
 #include "rockchip_drm_gem.h"
 #include "rockchip_drm_psr.h"
 
+#define ROCKCHIP_MAX_AFBC_WIDTH	2560
+
 static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
 	.destroy       = drm_gem_fb_destroy,
 	.create_handle = drm_gem_fb_create_handle,
@@ -32,6 +35,46 @@ rockchip_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cm
 	int ret;
 	int i;
 
+	if (mode_cmd->modifier[0]) {
+		const struct drm_format_info *info;
+		int bpp;
+
+		if (mode_cmd->modifier[0] &
+			~DRM_FORMAT_MOD_ARM_AFBC(
+				AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
+				AFBC_FORMAT_MOD_SPARSE
+			 )
+		) {
+			DRM_DEV_ERROR(dev->dev,
+				"Unsupported format modifer 0x%llx\n",
+				mode_cmd->modifier[0]
+			);
+
+			return ERR_PTR(-EINVAL);
+		}
+
+		if (!drm_afbc_check_offset(dev, mode_cmd))
+			return ERR_PTR(-EINVAL);
+
+		if (!drm_afbc_check_size_align(dev, mode_cmd))
+			return ERR_PTR(-EINVAL);
+
+		if (mode_cmd->width > ROCKCHIP_MAX_AFBC_WIDTH) {
+			DRM_DEV_ERROR(dev->dev,
+				"Unsupported width %d>%d\n",
+				mode_cmd->width,
+				ROCKCHIP_MAX_AFBC_WIDTH);
+
+			return ERR_PTR(-EINVAL);
+		}
+
+		info = drm_get_format_info(dev, mode_cmd);
+		bpp = info->cpp[0] * 8;
+
+		if (!drm_afbc_check_fb_size(dev, mode_cmd, obj[0], bpp))
+			return ERR_PTR(-EINVAL);
+	}
+
 	fb = kzalloc(sizeof(*fb), GFP_KERNEL);
 	if (!fb)
 		return ERR_PTR(-ENOMEM);
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 21b68eea46cc..0ac9ab3be3f1 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -90,9 +90,20 @@
 #define VOP_WIN_TO_INDEX(vop_win) \
 	((vop_win) - (vop_win)->vop->win)
 
+#define VOP_AFBC_SET(vop, name, v) \
+	do { \
+		if ((vop)->data->afbc) \
+			vop_reg_set((vop), &(vop)->data->afbc->name, \
+				0, ~0, v, #name); \
+	} while (0)
+
 #define to_vop(x) container_of(x, struct vop, crtc)
 #define to_vop_win(x) container_of(x, struct vop_win, base)
 
+#define AFBC_FMT_RGB565		0x0
+#define AFBC_FMT_U8U8U8U8	0x5
+#define AFBC_FMT_U8U8U8		0x4
+
 /*
  * The coefficients of the following matrix are all fixed points.
  * The format is S2.10 for the 3x3 part of the matrix, and S9.12 for the offsets.
@@ -163,6 +174,7 @@ struct vop {
 	/* optional internal rgb encoder */
 	struct rockchip_rgb *rgb;
 
+	struct vop_win *afbc_win;
 	struct vop_win win[];
 };
 
@@ -271,6 +283,27 @@ static enum vop_data_format vop_convert_format(uint32_t format)
 	}
 }
 
+static int vop_convert_afbc_format(uint32_t format)
+{
+	switch (format) {
+	case DRM_FORMAT_XRGB8888:
+	case DRM_FORMAT_ARGB8888:
+	case DRM_FORMAT_XBGR8888:
+	case DRM_FORMAT_ABGR8888:
+		return AFBC_FMT_U8U8U8U8;
+	case DRM_FORMAT_RGB888:
+	case DRM_FORMAT_BGR888:
+		return AFBC_FMT_U8U8U8;
+	case DRM_FORMAT_RGB565:
+	case DRM_FORMAT_BGR565:
+		return AFBC_FMT_RGB565;
+	default:
+		DRM_ERROR("unsupported afbc format[%08x]\n", format);
+	}
+
+	return -EINVAL;
+}
+
 static uint16_t scl_vop_cal_scale(enum scale_mode mode, uint32_t src,
 				  uint32_t dst, bool is_horizontal,
 				  int vsu_mode, int *vskiplines)
@@ -587,6 +620,15 @@ static int vop_enable(struct drm_crtc *crtc)
 
 		vop_win_disable(vop, win);
 	}
+
+	if (vop->data->afbc) {
+		/*
+		 * Disable AFBC and forget there was a vop window with AFBC
+		 */
+		VOP_AFBC_SET(vop, enable, 0);
+		vop->afbc_win = NULL;
+	}
+
 	spin_unlock(&vop->reg_lock);
 
 	vop_cfg_done(vop);
@@ -671,6 +713,36 @@ static void vop_plane_destroy(struct drm_plane *plane)
 	drm_plane_cleanup(plane);
 }
 
+static bool rockchip_mod_supported(struct drm_plane *plane,
+				   u32 format, u64 modifier)
+{
+	const struct drm_format_info *info;
+
+	if (WARN_ON(modifier == DRM_FORMAT_MOD_INVALID))
+		return false;
+
+	if (modifier == DRM_FORMAT_MOD_LINEAR)
+		return true;
+
+	if (modifier & ~DRM_FORMAT_MOD_ARM_AFBC(
+				AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
+				AFBC_FORMAT_MOD_SPARSE
+			)
+	) {
+		DRM_DEBUG_KMS("Unsupported format modifer 0x%llx\n", modifier);
+
+		return false;
+	}
+
+	info = drm_format_info(format);
+	if (info->num_planes != 1) {
+		DRM_DEBUG_KMS("AFBC buffers expect one plane\n");
+		return false;
+	}
+
+	return true;
+}
+
 static int vop_plane_atomic_check(struct drm_plane *plane,
 			   struct drm_plane_state *state)
 {
@@ -719,6 +791,33 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
 		return -EINVAL;
 	}
 
+	if (fb->modifier == DRM_FORMAT_MOD_ARM_AFBC(
+		AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | AFBC_FORMAT_MOD_SPARSE)) {
+		struct vop *vop = to_vop(crtc);
+
+		if (!vop->data->afbc) {
+			DRM_ERROR("vop does not support AFBC\n");
+			return -EINVAL;
+		}
+
+		ret = vop_convert_afbc_format(fb->format->format);
+		if (ret < 0)
+			return ret;
+
+		if (state->src.x1 || state->src.y1) {
+			DRM_ERROR("afbc does not support offset display\n");
+			DRM_ERROR("xpos=%d, ypos=%d, offset=%d\n",
+				  state->src.x1, state->src.y1, fb->offsets[0]);
+			return -EINVAL;
+		}
+
+		if (state->rotation && state->rotation != DRM_MODE_ROTATE_0) {
+			DRM_ERROR("afbc does not support rotation\n");
+			DRM_ERROR("rotation=%d\n", state->rotation);
+			return -EINVAL;
+		}
+	}
+
 	return 0;
 }
 
@@ -735,6 +834,11 @@ static void vop_plane_atomic_disable(struct drm_plane *plane,
 	spin_lock(&vop->reg_lock);
 
 	vop_win_disable(vop, win);
+	/*
+	 * Forget about the AFBC window if it is being disabled
+	 */
+	if (vop_win == vop->afbc_win)
+		vop->afbc_win = NULL;
 
 	spin_unlock(&vop->reg_lock);
 }
@@ -774,6 +878,13 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
 	if (WARN_ON(!vop->is_enabled))
 		return;
 
+	/*
+	 * If updating the AFBC window then assume that
+	 * after the update there will be no AFBC window.
+	 */
+	if (vop_win == vop->afbc_win)
+		vop->afbc_win = NULL;
+
 	if (!state->visible) {
 		vop_plane_atomic_disable(plane, old_state);
 		return;
@@ -808,6 +919,24 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
 
 	spin_lock(&vop->reg_lock);
 
+	if (fb->modifier == DRM_FORMAT_MOD_ARM_AFBC(
+		AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | AFBC_FORMAT_MOD_SPARSE)) {
+		int afbc_format = vop_convert_afbc_format(fb->format->format);
+
+		VOP_AFBC_SET(vop, format, afbc_format | 1 << 4);
+		VOP_AFBC_SET(vop, hreg_block_split, 0);
+		VOP_AFBC_SET(vop, win_sel, VOP_WIN_TO_INDEX(vop_win));
+		VOP_AFBC_SET(vop, hdr_ptr, dma_addr);
+		VOP_AFBC_SET(vop, pic_size, act_info);
+
+		/*
+		 * The window being udated becomes the AFBC window
+		 */
+		vop->afbc_win = vop_win;
+
+		pr_info("AFBC on plane %s\n", plane->name);
+	}
+
 	VOP_WIN_SET(vop, win, format, format);
 	VOP_WIN_SET(vop, win, yrgb_vir, DIV_ROUND_UP(fb->pitches[0], 4));
 	VOP_WIN_SET(vop, win, yrgb_mst, dma_addr);
@@ -964,6 +1093,7 @@ static const struct drm_plane_funcs vop_plane_funcs = {
 	.reset = drm_atomic_helper_plane_reset,
 	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+	.format_mod_supported = rockchip_mod_supported,
 };
 
 static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
@@ -1163,6 +1293,10 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
 
 	spin_lock(&vop->reg_lock);
 
+	/*
+	 * Enable AFBC if there is some AFBC window, disable otherwise
+	 */
+	VOP_AFBC_SET(vop, enable, vop->afbc_win != NULL);
 	vop_cfg_done(vop);
 
 	spin_unlock(&vop->reg_lock);
@@ -1471,7 +1605,8 @@ static int vop_create_crtc(struct vop *vop)
 					       0, &vop_plane_funcs,
 					       win_data->phy->data_formats,
 					       win_data->phy->nformats,
-					       NULL, win_data->type, NULL);
+					       win_data->phy->format_modifiers,
+					       win_data->type, NULL);
 		if (ret) {
 			DRM_DEV_ERROR(vop->dev, "failed to init plane %d\n",
 				      ret);
@@ -1511,7 +1646,8 @@ static int vop_create_crtc(struct vop *vop)
 					       &vop_plane_funcs,
 					       win_data->phy->data_formats,
 					       win_data->phy->nformats,
-					       NULL, win_data->type, NULL);
+					       win_data->phy->format_modifiers,
+					       win_data->type, NULL);
 		if (ret) {
 			DRM_DEV_ERROR(vop->dev, "failed to init overlay %d\n",
 				      ret);
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
index 2149a889c29d..371b28b933a9 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
@@ -34,6 +34,16 @@ struct vop_reg {
 	bool relaxed;
 };
 
+struct vop_afbc {
+	struct vop_reg enable;
+	struct vop_reg win_sel;
+	struct vop_reg format;
+	struct vop_reg hreg_block_split;
+	struct vop_reg pic_size;
+	struct vop_reg hdr_ptr;
+	struct vop_reg rstn;
+};
+
 struct vop_modeset {
 	struct vop_reg htotal_pw;
 	struct vop_reg hact_st_end;
@@ -128,6 +138,7 @@ struct vop_win_phy {
 	const struct vop_scl_regs *scl;
 	const uint32_t *data_formats;
 	uint32_t nformats;
+	const uint64_t *format_modifiers;
 
 	struct vop_reg enable;
 	struct vop_reg gate;
@@ -167,6 +178,7 @@ struct vop_data {
 	const struct vop_misc *misc;
 	const struct vop_modeset *modeset;
 	const struct vop_output *output;
+	const struct vop_afbc *afbc;
 	const struct vop_win_yuv2yuv_data *win_yuv2yuv;
 	const struct vop_win_data *win;
 	unsigned int win_size;
diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
index 7b9c74750f6d..a3981f037c90 100644
--- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
+++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
@@ -44,6 +44,20 @@ static const uint32_t formats_win_full[] = {
 	DRM_FORMAT_NV24,
 };
 
+static const uint64_t format_modifiers_win_full[] = {
+	DRM_FORMAT_MOD_NONE,
+	DRM_FORMAT_MOD_INVALID,
+};
+
+static const uint64_t format_modifiers_win_full_afbc[] = {
+	DRM_FORMAT_MOD_ARM_AFBC(
+		AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
+		AFBC_FORMAT_MOD_SPARSE
+	),
+	DRM_FORMAT_MOD_LINEAR,
+	DRM_FORMAT_MOD_INVALID,
+};
+
 static const uint32_t formats_win_lite[] = {
 	DRM_FORMAT_XRGB8888,
 	DRM_FORMAT_ARGB8888,
@@ -55,6 +69,11 @@ static const uint32_t formats_win_lite[] = {
 	DRM_FORMAT_BGR565,
 };
 
+static const uint64_t format_modifiers_win_lite[] = {
+	DRM_FORMAT_MOD_LINEAR,
+	DRM_FORMAT_MOD_INVALID,
+};
+
 static const struct vop_scl_regs rk3036_win_scl = {
 	.scale_yrgb_x = VOP_REG(RK3036_WIN0_SCL_FACTOR_YRGB, 0xffff, 0x0),
 	.scale_yrgb_y = VOP_REG(RK3036_WIN0_SCL_FACTOR_YRGB, 0xffff, 16),
@@ -66,6 +85,7 @@ static const struct vop_win_phy rk3036_win0_data = {
 	.scl = &rk3036_win_scl,
 	.data_formats = formats_win_full,
 	.nformats = ARRAY_SIZE(formats_win_full),
+	.format_modifiers = format_modifiers_win_full,
 	.enable = VOP_REG(RK3036_SYS_CTRL, 0x1, 0),
 	.format = VOP_REG(RK3036_SYS_CTRL, 0x7, 3),
 	.rb_swap = VOP_REG(RK3036_SYS_CTRL, 0x1, 15),
@@ -81,6 +101,7 @@ static const struct vop_win_phy rk3036_win0_data = {
 static const struct vop_win_phy rk3036_win1_data = {
 	.data_formats = formats_win_lite,
 	.nformats = ARRAY_SIZE(formats_win_lite),
+	.format_modifiers = format_modifiers_win_lite,
 	.enable = VOP_REG(RK3036_SYS_CTRL, 0x1, 1),
 	.format = VOP_REG(RK3036_SYS_CTRL, 0x7, 6),
 	.rb_swap = VOP_REG(RK3036_SYS_CTRL, 0x1, 19),
@@ -147,6 +168,7 @@ static const struct vop_data rk3036_vop = {
 static const struct vop_win_phy rk3126_win1_data = {
 	.data_formats = formats_win_lite,
 	.nformats = ARRAY_SIZE(formats_win_lite),
+	.format_modifiers = format_modifiers_win_lite,
 	.enable = VOP_REG(RK3036_SYS_CTRL, 0x1, 1),
 	.format = VOP_REG(RK3036_SYS_CTRL, 0x7, 6),
 	.rb_swap = VOP_REG(RK3036_SYS_CTRL, 0x1, 19),
@@ -226,6 +248,7 @@ static const struct vop_win_phy px30_win0_data = {
 	.scl = &px30_win_scl,
 	.data_formats = formats_win_full,
 	.nformats = ARRAY_SIZE(formats_win_full),
+	.format_modifiers = format_modifiers_win_full,
 	.enable = VOP_REG(PX30_WIN0_CTRL0, 0x1, 0),
 	.format = VOP_REG(PX30_WIN0_CTRL0, 0x7, 1),
 	.rb_swap = VOP_REG(PX30_WIN0_CTRL0, 0x1, 12),
@@ -241,6 +264,7 @@ static const struct vop_win_phy px30_win0_data = {
 static const struct vop_win_phy px30_win1_data = {
 	.data_formats = formats_win_lite,
 	.nformats = ARRAY_SIZE(formats_win_lite),
+	.format_modifiers = format_modifiers_win_lite,
 	.enable = VOP_REG(PX30_WIN1_CTRL0, 0x1, 0),
 	.format = VOP_REG(PX30_WIN1_CTRL0, 0x7, 4),
 	.rb_swap = VOP_REG(PX30_WIN1_CTRL0, 0x1, 12),
@@ -253,6 +277,7 @@ static const struct vop_win_phy px30_win1_data = {
 static const struct vop_win_phy px30_win2_data = {
 	.data_formats = formats_win_lite,
 	.nformats = ARRAY_SIZE(formats_win_lite),
+	.format_modifiers = format_modifiers_win_lite,
 	.gate = VOP_REG(PX30_WIN2_CTRL0, 0x1, 4),
 	.enable = VOP_REG(PX30_WIN2_CTRL0, 0x1, 0),
 	.format = VOP_REG(PX30_WIN2_CTRL0, 0x3, 5),
@@ -308,6 +333,7 @@ static const struct vop_win_phy rk3066_win0_data = {
 	.scl = &rk3066_win_scl,
 	.data_formats = formats_win_full,
 	.nformats = ARRAY_SIZE(formats_win_full),
+	.format_modifiers = format_modifiers_win_full,
 	.enable = VOP_REG(RK3066_SYS_CTRL1, 0x1, 0),
 	.format = VOP_REG(RK3066_SYS_CTRL0, 0x7, 4),
 	.rb_swap = VOP_REG(RK3066_SYS_CTRL0, 0x1, 19),
@@ -324,6 +350,7 @@ static const struct vop_win_phy rk3066_win1_data = {
 	.scl = &rk3066_win_scl,
 	.data_formats = formats_win_full,
 	.nformats = ARRAY_SIZE(formats_win_full),
+	.format_modifiers = format_modifiers_win_full,
 	.enable = VOP_REG(RK3066_SYS_CTRL1, 0x1, 1),
 	.format = VOP_REG(RK3066_SYS_CTRL0, 0x7, 7),
 	.rb_swap = VOP_REG(RK3066_SYS_CTRL0, 0x1, 23),
@@ -339,6 +366,7 @@ static const struct vop_win_phy rk3066_win1_data = {
 static const struct vop_win_phy rk3066_win2_data = {
 	.data_formats = formats_win_lite,
 	.nformats = ARRAY_SIZE(formats_win_lite),
+	.format_modifiers = format_modifiers_win_lite,
 	.enable = VOP_REG(RK3066_SYS_CTRL1, 0x1, 2),
 	.format = VOP_REG(RK3066_SYS_CTRL0, 0x7, 10),
 	.rb_swap = VOP_REG(RK3066_SYS_CTRL0, 0x1, 27),
@@ -418,6 +446,7 @@ static const struct vop_win_phy rk3188_win0_data = {
 	.scl = &rk3188_win_scl,
 	.data_formats = formats_win_full,
 	.nformats = ARRAY_SIZE(formats_win_full),
+	.format_modifiers = format_modifiers_win_full,
 	.enable = VOP_REG(RK3188_SYS_CTRL, 0x1, 0),
 	.format = VOP_REG(RK3188_SYS_CTRL, 0x7, 3),
 	.rb_swap = VOP_REG(RK3188_SYS_CTRL, 0x1, 15),
@@ -432,6 +461,7 @@ static const struct vop_win_phy rk3188_win0_data = {
 static const struct vop_win_phy rk3188_win1_data = {
 	.data_formats = formats_win_lite,
 	.nformats = ARRAY_SIZE(formats_win_lite),
+	.format_modifiers = format_modifiers_win_lite,
 	.enable = VOP_REG(RK3188_SYS_CTRL, 0x1, 1),
 	.format = VOP_REG(RK3188_SYS_CTRL, 0x7, 6),
 	.rb_swap = VOP_REG(RK3188_SYS_CTRL, 0x1, 19),
@@ -537,6 +567,7 @@ static const struct vop_win_phy rk3288_win01_data = {
 	.scl = &rk3288_win_full_scl,
 	.data_formats = formats_win_full,
 	.nformats = ARRAY_SIZE(formats_win_full),
+	.format_modifiers = format_modifiers_win_full,
 	.enable = VOP_REG(RK3288_WIN0_CTRL0, 0x1, 0),
 	.format = VOP_REG(RK3288_WIN0_CTRL0, 0x7, 1),
 	.rb_swap = VOP_REG(RK3288_WIN0_CTRL0, 0x1, 12),
@@ -555,6 +586,7 @@ static const struct vop_win_phy rk3288_win01_data = {
 static const struct vop_win_phy rk3288_win23_data = {
 	.data_formats = formats_win_lite,
 	.nformats = ARRAY_SIZE(formats_win_lite),
+	.format_modifiers = format_modifiers_win_lite,
 	.enable = VOP_REG(RK3288_WIN2_CTRL0, 0x1, 4),
 	.gate = VOP_REG(RK3288_WIN2_CTRL0, 0x1, 0),
 	.format = VOP_REG(RK3288_WIN2_CTRL0, 0x7, 1),
@@ -667,6 +699,7 @@ static const struct vop_win_phy rk3368_win01_data = {
 	.scl = &rk3288_win_full_scl,
 	.data_formats = formats_win_full,
 	.nformats = ARRAY_SIZE(formats_win_full),
+	.format_modifiers = format_modifiers_win_full,
 	.enable = VOP_REG(RK3368_WIN0_CTRL0, 0x1, 0),
 	.format = VOP_REG(RK3368_WIN0_CTRL0, 0x7, 1),
 	.rb_swap = VOP_REG(RK3368_WIN0_CTRL0, 0x1, 12),
@@ -687,6 +720,7 @@ static const struct vop_win_phy rk3368_win01_data = {
 static const struct vop_win_phy rk3368_win23_data = {
 	.data_formats = formats_win_lite,
 	.nformats = ARRAY_SIZE(formats_win_lite),
+	.format_modifiers = format_modifiers_win_lite,
 	.gate = VOP_REG(RK3368_WIN2_CTRL0, 0x1, 0),
 	.enable = VOP_REG(RK3368_WIN2_CTRL0, 0x1, 4),
 	.format = VOP_REG(RK3368_WIN2_CTRL0, 0x3, 5),
@@ -798,6 +832,53 @@ static const struct vop_win_yuv2yuv_data rk3399_vop_big_win_yuv2yuv_data[] = {
 	  .y2r_en = VOP_REG(RK3399_YUV2YUV_WIN, 0x1, 9) },
 	{ .base = 0xC0, .phy = &rk3399_yuv2yuv_win23_data },
 	{ .base = 0x120, .phy = &rk3399_yuv2yuv_win23_data },
+
+};
+
+static const struct vop_win_phy rk3399_win01_data = {
+	.scl = &rk3288_win_full_scl,
+	.data_formats = formats_win_full,
+	.nformats = ARRAY_SIZE(formats_win_full),
+	.format_modifiers = format_modifiers_win_full_afbc,
+	.enable = VOP_REG(RK3288_WIN0_CTRL0, 0x1, 0),
+	.format = VOP_REG(RK3288_WIN0_CTRL0, 0x7, 1),
+	.rb_swap = VOP_REG(RK3288_WIN0_CTRL0, 0x1, 12),
+	.y_mir_en = VOP_REG(RK3288_WIN0_CTRL0, 0x1, 22),
+	.act_info = VOP_REG(RK3288_WIN0_ACT_INFO, 0x1fff1fff, 0),
+	.dsp_info = VOP_REG(RK3288_WIN0_DSP_INFO, 0x0fff0fff, 0),
+	.dsp_st = VOP_REG(RK3288_WIN0_DSP_ST, 0x1fff1fff, 0),
+	.yrgb_mst = VOP_REG(RK3288_WIN0_YRGB_MST, 0xffffffff, 0),
+	.uv_mst = VOP_REG(RK3288_WIN0_CBR_MST, 0xffffffff, 0),
+	.yrgb_vir = VOP_REG(RK3288_WIN0_VIR, 0x3fff, 0),
+	.uv_vir = VOP_REG(RK3288_WIN0_VIR, 0x3fff, 16),
+	.src_alpha_ctl = VOP_REG(RK3288_WIN0_SRC_ALPHA_CTRL, 0xff, 0),
+	.dst_alpha_ctl = VOP_REG(RK3288_WIN0_DST_ALPHA_CTRL, 0xff, 0),
+};
+
+/*
+ * rk3399 vop big windows register layout is same as rk3288, but we
+ * have a separate rk3399 win data array here so that we can advertise
+ * AFBC on the primary plane.
+ */
+static const struct vop_win_data rk3399_vop_win_data[] = {
+	{ .base = 0x00, .phy = &rk3399_win01_data,
+	  .type = DRM_PLANE_TYPE_PRIMARY },
+	{ .base = 0x40, .phy = &rk3288_win01_data,
+	  .type = DRM_PLANE_TYPE_OVERLAY },
+	{ .base = 0x00, .phy = &rk3288_win23_data,
+	  .type = DRM_PLANE_TYPE_OVERLAY },
+	{ .base = 0x50, .phy = &rk3288_win23_data,
+	  .type = DRM_PLANE_TYPE_CURSOR },
+};
+
+static const struct vop_afbc rk3399_vop_afbc = {
+	.rstn = VOP_REG(RK3399_AFBCD0_CTRL, 0x1, 3),
+	.enable = VOP_REG(RK3399_AFBCD0_CTRL, 0x1, 0),
+	.win_sel = VOP_REG(RK3399_AFBCD0_CTRL, 0x3, 1),
+	.format = VOP_REG(RK3399_AFBCD0_CTRL, 0x1f, 16),
+	.hreg_block_split = VOP_REG(RK3399_AFBCD0_CTRL, 0x1, 21),
+	.hdr_ptr = VOP_REG(RK3399_AFBCD0_HDR_PTR, 0xffffffff, 0),
+	.pic_size = VOP_REG(RK3399_AFBCD0_PIC_SIZE, 0xffffffff, 0),
 };
 
 static const struct vop_data rk3399_vop_big = {
@@ -807,9 +888,10 @@ static const struct vop_data rk3399_vop_big = {
 	.common = &rk3288_common,
 	.modeset = &rk3288_modeset,
 	.output = &rk3399_output,
+	.afbc = &rk3399_vop_afbc,
 	.misc = &rk3368_misc,
-	.win = rk3368_vop_win_data,
-	.win_size = ARRAY_SIZE(rk3368_vop_win_data),
+	.win = rk3399_vop_win_data,
+	.win_size = ARRAY_SIZE(rk3399_vop_win_data),
 	.win_yuv2yuv = rk3399_vop_big_win_yuv2yuv_data,
 };
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] drm/rockchip: Add support for afbc

[Daniel Stone]

Hi Andrzej,

On Fri, 11 Oct 2019 at 12:18, Andrzej Pietrasiewicz
<andrzej.p@collabora.com> wrote:
> @@ -32,6 +35,46 @@ rockchip_fb_alloc(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cm
>         int ret;
>         int i;
>
> +       if (mode_cmd->modifier[0]) {
> +               const struct drm_format_info *info;
> +               int bpp;
> +
> +               if (mode_cmd->modifier[0] &

Use != here, not &~.

> +       case DRM_FORMAT_BGR888:
> +               return AFBC_FMT_U8U8U8;
> +       case DRM_FORMAT_RGB565:
> +       case DRM_FORMAT_BGR565:
> +               return AFBC_FMT_RGB565;
> +       default:
> +               DRM_ERROR("unsupported afbc format[%08x]\n", format);

This should not be reachable, because you shouldn't be able to create
a framebuffer with an unsupported format/modifier combination.

> +static bool rockchip_mod_supported(struct drm_plane *plane,
> +                                  u32 format, u64 modifier)
> +{
> +       const struct drm_format_info *info;
> +
> +       if (WARN_ON(modifier == DRM_FORMAT_MOD_INVALID))
> +               return false;
> +
> +       if (modifier == DRM_FORMAT_MOD_LINEAR)
> +               return true;
> +
> +       if (modifier & ~DRM_FORMAT_MOD_ARM_AFBC(

Again use != here.

> +                               AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> +                               AFBC_FORMAT_MOD_SPARSE
> +                       )
> +       ) {
> +               DRM_DEBUG_KMS("Unsupported format modifer 0x%llx\n", modifier);
> +
> +               return false;
> +       }
> +
> +       info = drm_format_info(format);
> +       if (info->num_planes != 1) {
> +               DRM_DEBUG_KMS("AFBC buffers expect one plane\n");
> +               return false;
> +       }

This is where you should reject unsupported format + AFBC
combinations. Doing it here prevents it from ever being advertised to
userspace in the first place.

> @@ -808,6 +919,24 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
>
>         spin_lock(&vop->reg_lock);
>
> +       if (fb->modifier == DRM_FORMAT_MOD_ARM_AFBC(
> +               AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | AFBC_FORMAT_MOD_SPARSE)) {
> +               int afbc_format = vop_convert_afbc_format(fb->format->format);
> +
> +               VOP_AFBC_SET(vop, format, afbc_format | 1 << 4);

I assume the (1 << 4) programs the 16x16 block size. Can we support
other block sizes?

> +               VOP_AFBC_SET(vop, hreg_block_split, 0);

Does this mean we can also support AFBC_FORMAT_MOD_SPLIT?

> +               VOP_AFBC_SET(vop, win_sel, VOP_WIN_TO_INDEX(vop_win));
> +               VOP_AFBC_SET(vop, hdr_ptr, dma_addr);
> +               VOP_AFBC_SET(vop, pic_size, act_info);
> +
> +               /*
> +                * The window being udated becomes the AFBC window
> +                */
> +               vop->afbc_win = vop_win;
> +
> +               pr_info("AFBC on plane %s\n", plane->name);

Please do not use pr_info() here. Userspace should not be able to
trigger logging, apart from DRM_DEBUG.

> +static const uint64_t format_modifiers_win_full[] = {
> +       DRM_FORMAT_MOD_NONE,

*DRM_FORMAT_MOD_LINEAR

Cheers,
Daniel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] drm/arm: Factor out generic afbc helpers

[Ayan Halder]

On Fri, Oct 11, 2019 at 01:18:10PM +0200, Andrzej Pietrasiewicz wrote:
> These are useful for other users of afbc, e.g. rockchip.
> 
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

Hi Andrzej,

Thanks a lot for doing this. Much appreciated. :)
It was on our TODO list for a long time.

I have cc-ed james.qian.wang@arm.com, Mihail.Atanassov@arm.com for
their comments as well.

> ---
>  drivers/gpu/drm/Kconfig          |   4 ++
>  drivers/gpu/drm/Makefile         |   1 +
>  drivers/gpu/drm/arm/Kconfig      |   1 +
>  drivers/gpu/drm/arm/malidp_drv.c |  58 ++--------------
>  drivers/gpu/drm/drm_afbc.c       | 114 +++++++++++++++++++++++++++++++
>  include/drm/drm_afbc.h           |  25 +++++++
>  6 files changed, 149 insertions(+), 54 deletions(-)
>  create mode 100644 drivers/gpu/drm/drm_afbc.c
>  create mode 100644 include/drm/drm_afbc.h
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 3c88420e3497..00e3f90557f4 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -195,6 +195,10 @@ config DRM_SCHED
>  	tristate
>  	depends on DRM
>  
> +config DRM_AFBC
> +	tristate
> +	depends on DRM
Adding a 'help' would be great here. Stealing the first line from
https://www.kernel.org/doc/html/latest/gpu/afbc.html

"AFBC is a proprietary lossless image compression protocol and format.
It provides fine-grained random access and minimizes the amount of
data transferred between IP blocks."

> +
>  source "drivers/gpu/drm/i2c/Kconfig"
>  
>  source "drivers/gpu/drm/arm/Kconfig"
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 9f0d2ee35794..55368b668355 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -31,6 +31,7 @@ drm-$(CONFIG_OF) += drm_of.o
>  drm-$(CONFIG_AGP) += drm_agpsupport.o
>  drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
>  drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> +drm-$(CONFIG_DRM_AFBC) += drm_afbc.o
>  
>  drm_vram_helper-y := drm_gem_vram_helper.o \
>  		     drm_vram_helper_common.o \
> diff --git a/drivers/gpu/drm/arm/Kconfig b/drivers/gpu/drm/arm/Kconfig
> index a204103b3efb..25c3dc408cda 100644
> --- a/drivers/gpu/drm/arm/Kconfig
> +++ b/drivers/gpu/drm/arm/Kconfig
> @@ -29,6 +29,7 @@ config DRM_MALI_DISPLAY
>  	select DRM_KMS_HELPER
>  	select DRM_KMS_CMA_HELPER
>  	select DRM_GEM_CMA_HELPER
> +	select DRM_AFBC
>  	select VIDEOMODE_HELPERS
>  	help
>  	  Choose this option if you want to compile the ARM Mali Display
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> index f25ec4382277..a67b69e08f63 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -16,6 +16,7 @@
>  #include <linux/debugfs.h>
>  
>  #include <drm/drmP.h>
> +#include <drm/drm_afbc.h>
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc.h>
> @@ -33,8 +34,6 @@
>  #include "malidp_hw.h"
>  
>  #define MALIDP_CONF_VALID_TIMEOUT	250
> -#define AFBC_HEADER_SIZE		16
> -#define AFBC_SUPERBLK_ALIGNMENT		128
>  
>  static void malidp_write_gamma_table(struct malidp_hw_device *hwdev,
>  				     u32 data[MALIDP_COEFFTAB_NUM_COEFFS])
> @@ -275,24 +274,8 @@ malidp_verify_afbc_framebuffer_caps(struct drm_device *dev,
>  					mode_cmd->modifier[0]) == false)
>  		return false;
>  
> -	if (mode_cmd->offsets[0] != 0) {
> -		DRM_DEBUG_KMS("AFBC buffers' plane offset should be 0\n");
> -		return false;
> -	}
> -
> -	switch (mode_cmd->modifier[0] & AFBC_SIZE_MASK) {
> -	case AFBC_SIZE_16X16:
> -		if ((mode_cmd->width % 16) || (mode_cmd->height % 16)) {
> -			DRM_DEBUG_KMS("AFBC buffers must be aligned to 16 pixels\n");
> -			return false;
> -		}
> -		break;
> -	default:
> -		DRM_DEBUG_KMS("Unsupported AFBC block size\n");
> -		return false;
> -	}
> -
> -	return true;
> +	return drm_afbc_check_offset(dev, mode_cmd) &&
> +	       drm_afbc_check_size_align(dev, mode_cmd);
>  }
>  
>  static bool
> @@ -300,53 +283,20 @@ malidp_verify_afbc_framebuffer_size(struct drm_device *dev,
>  				    struct drm_file *file,
>  				    const struct drm_mode_fb_cmd2 *mode_cmd)
>  {
> -	int n_superblocks = 0;
>  	const struct drm_format_info *info;
>  	struct drm_gem_object *objs = NULL;
> -	u32 afbc_superblock_size = 0, afbc_superblock_height = 0;
> -	u32 afbc_superblock_width = 0, afbc_size = 0;
>  	int bpp = 0;
>  
> -	switch (mode_cmd->modifier[0] & AFBC_SIZE_MASK) {
> -	case AFBC_SIZE_16X16:
> -		afbc_superblock_height = 16;
> -		afbc_superblock_width = 16;
> -		break;
> -	default:
> -		DRM_DEBUG_KMS("AFBC superblock size is not supported\n");
> -		return false;
> -	}
> -
>  	info = drm_get_format_info(dev, mode_cmd);
> -
> -	n_superblocks = (mode_cmd->width / afbc_superblock_width) *
> -		(mode_cmd->height / afbc_superblock_height);
> -
>  	bpp = malidp_format_get_bpp(info->format);
>  
> -	afbc_superblock_size = (bpp * afbc_superblock_width * afbc_superblock_height)
> -				/ BITS_PER_BYTE;
> -
> -	afbc_size = ALIGN(n_superblocks * AFBC_HEADER_SIZE, AFBC_SUPERBLK_ALIGNMENT);
> -	afbc_size += n_superblocks * ALIGN(afbc_superblock_size, AFBC_SUPERBLK_ALIGNMENT);
> -
> -	if ((mode_cmd->width * bpp) != (mode_cmd->pitches[0] * BITS_PER_BYTE)) {
> -		DRM_DEBUG_KMS("Invalid value of (pitch * BITS_PER_BYTE) (=%u) "
> -			      "should be same as width (=%u) * bpp (=%u)\n",
> -			      (mode_cmd->pitches[0] * BITS_PER_BYTE),
> -			      mode_cmd->width, bpp);
> -		return false;
> -	}
> -
>  	objs = drm_gem_object_lookup(file, mode_cmd->handles[0]);
>  	if (!objs) {
>  		DRM_DEBUG_KMS("Failed to lookup GEM object\n");
>  		return false;
>  	}
>  
> -	if (objs->size < afbc_size) {
> -		DRM_DEBUG_KMS("buffer size (%zu) too small for AFBC buffer size = %u\n",
> -			      objs->size, afbc_size);
> +	if (!drm_afbc_check_fb_size(dev, mode_cmd, objs, bpp)) {
>  		drm_gem_object_put_unlocked(objs);
>  		return false;
>  	}
Also can you do the code refactoring for komeda driver as well.
specifically komeda_fb_afbc_size_check(). I will let
james.qian.wang@arm.com and Mihail.Atanassov@arm.com have their
opinion on this.

> diff --git a/drivers/gpu/drm/drm_afbc.c b/drivers/gpu/drm/drm_afbc.c
> new file mode 100644
> index 000000000000..3e8a9225fd2e
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_afbc.c
> @@ -0,0 +1,114 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) 2019 Collabora Ltd.
> + *
> + * author: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> + *
> + */
> +#include <linux/module.h>
> +
> +#include <drm/drm_afbc.h>
> +#include <drm/drm_device.h>
> +#include <drm/drm_fourcc.h>
> +#include <drm/drm_gem.h>
> +#include <drm/drm_mode.h>
> +#include <drm/drm_print.h>
> +
> +#define AFBC_HEADER_SIZE		16
> +#define AFBC_SUPERBLK_ALIGNMENT		128
> +
> +bool drm_afbc_check_offset(struct drm_device *dev,
> +			   const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> +	if (mode_cmd->offsets[0] != 0) {
> +		DRM_DEBUG_KMS("AFBC buffers' plane offset should be 0\n");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(drm_afbc_check_offset);
> +
> +bool drm_afbc_check_size_align(struct drm_device *dev,
> +			       const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> +
> +	switch (mode_cmd->modifier[0] & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> +	case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> +		if ((mode_cmd->width % 16) || (mode_cmd->height % 16)) {
> +			DRM_DEBUG_KMS(
> +				"AFBC buffer must be aligned to 16 pixels\n"
> +			);
> +			return false;
> +		}
> +		break;
> +	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
> +		/* fall through */
> +	case AFBC_FORMAT_MOD_BLOCK_SIZE_64x4:
> +		/* fall through */
> +	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4:
> +		/* fall through */
> +	default:
> +		DRM_DEBUG_KMS("Unsupported AFBC block size\n");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(drm_afbc_check_size_align);
> +
> +bool drm_afbc_check_fb_size(struct drm_device *dev,
> +			    const struct drm_mode_fb_cmd2 *mode_cmd,
> +			    struct drm_gem_object *objs, int bpp)
> +{
> +	int n_superblocks = 0;
> +	u32 afbc_superblock_size = 0, afbc_superblock_height = 0;
> +	u32 afbc_superblock_width = 0, afbc_size = 0;
> +
> +	switch (mode_cmd->modifier[0] & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> +	case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> +		afbc_superblock_height = 16;
> +		afbc_superblock_width = 16;
> +		break;
> +	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
Copying from
https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c#n60
                afbc_superblock_width = 32;
                afbc_superblock_height = 8;
> +		/* fall through */
> +	case AFBC_FORMAT_MOD_BLOCK_SIZE_64x4:
> +		/* fall through */
> +	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4:
> +		/* fall through */
> +	default:
> +		DRM_DEBUG_KMS("AFBC superblock size is not supported\n");
> +		return false;
> +	}
Can you combine the two switch - case confitions (from this function
and the one in drm_afbc_check_size_align()) and put it in a separate
function (say drm_afbc_get_superblock_dimensions()) of its own ?
This will help to avoid code repetition.

> +
> +	n_superblocks = (mode_cmd->width / afbc_superblock_width) *
> +		(mode_cmd->height / afbc_superblock_height);
> +
> +	afbc_superblock_size =
> +		(bpp * afbc_superblock_width * afbc_superblock_height)
> +			/ BITS_PER_BYTE;
> +
> +	afbc_size = ALIGN(n_superblocks * AFBC_HEADER_SIZE,
> +			  AFBC_SUPERBLK_ALIGNMENT);
> +	afbc_size += n_superblocks *
> +		ALIGN(afbc_superblock_size, AFBC_SUPERBLK_ALIGNMENT);
> +
> +	if ((mode_cmd->width * bpp) != (mode_cmd->pitches[0] * BITS_PER_BYTE)) {
> +		DRM_DEBUG_KMS("Invalid value of (pitch * BITS_PER_BYTE) (=%u) should be same as width (=%u) * bpp (=%u)\n",
> +			mode_cmd->pitches[0] * BITS_PER_BYTE,
> +			mode_cmd->width, bpp
> +		);
> +		return false;
> +	}
> +
> +	if (objs->size < afbc_size) {
> +		DRM_DEBUG_KMS("buffer size (%zu) too small for AFBC buffer size = %u\n",
> +			objs->size, afbc_size
> +		);
> +
> +		return false;
> +	}
> +
> +	return true;
> +}
> +EXPORT_SYMBOL(drm_afbc_check_fb_size);
> diff --git a/include/drm/drm_afbc.h b/include/drm/drm_afbc.h
> new file mode 100644
> index 000000000000..ce39c850217b
> --- /dev/null
> +++ b/include/drm/drm_afbc.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * (C) 2019 Collabora Ltd.
> + *
> + * author: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> + *
> + */
> +#ifndef __DRM_AFBC_H__
> +#define __DRM_AFBC_H__
> +
> +struct drm_device;
> +struct drm_mode_fb_cmd2;
> +struct drm_gem_object;
> +
> +bool drm_afbc_check_offset(struct drm_device *dev,
> +			   const struct drm_mode_fb_cmd2 *mode_cmd);
> +
> +bool drm_afbc_check_size_align(struct drm_device *dev,
> +			       const struct drm_mode_fb_cmd2 *mode_cmd);
> +
> +bool drm_afbc_check_fb_size(struct drm_device *dev,
> +			    const struct drm_mode_fb_cmd2 *mode_cmd,
> +			    struct drm_gem_object *objs, int bpp);
> +
> +#endif /* __DRM_AFBC_H__ */
> --
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] drm/arm: Factor out generic afbc helpers

[Mihail Atanassov]

Hi Andrzej,

On Monday, 21 October 2019 14:50:14 BST Ayan Halder wrote:
> On Fri, Oct 11, 2019 at 01:18:10PM +0200, Andrzej Pietrasiewicz wrote:
> > These are useful for other users of afbc, e.g. rockchip.
> > 
> > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> 
> Hi Andrzej,
> 
> Thanks a lot for doing this. Much appreciated. :)
> It was on our TODO list for a long time.

Seconded, thanks for the patch!

> 
> I have cc-ed james.qian.wang@arm.com, Mihail.Atanassov@arm.com for
> their comments as well.
> 
> > ---
> >  drivers/gpu/drm/Kconfig          |   4 ++
> >  drivers/gpu/drm/Makefile         |   1 +
> >  drivers/gpu/drm/arm/Kconfig      |   1 +
> >  drivers/gpu/drm/arm/malidp_drv.c |  58 ++--------------
> >  drivers/gpu/drm/drm_afbc.c       | 114 +++++++++++++++++++++++++++++++
> >  include/drm/drm_afbc.h           |  25 +++++++
> >  6 files changed, 149 insertions(+), 54 deletions(-)
> >  create mode 100644 drivers/gpu/drm/drm_afbc.c
> >  create mode 100644 include/drm/drm_afbc.h
> > 
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index 3c88420e3497..00e3f90557f4 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -195,6 +195,10 @@ config DRM_SCHED
> >  	tristate
> >  	depends on DRM
> >  
> > +config DRM_AFBC
> > +	tristate
> > +	depends on DRM
> Adding a 'help' would be great here. Stealing the first line from
> https://www.kernel.org/doc/html/latest/gpu/afbc.html
> 
> "AFBC is a proprietary lossless image compression protocol and format.
> It provides fine-grained random access and minimizes the amount of
> data transferred between IP blocks."
> 
> > +
> >  source "drivers/gpu/drm/i2c/Kconfig"
> >  
> >  source "drivers/gpu/drm/arm/Kconfig"
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 9f0d2ee35794..55368b668355 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -31,6 +31,7 @@ drm-$(CONFIG_OF) += drm_of.o
> >  drm-$(CONFIG_AGP) += drm_agpsupport.o
> >  drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
> >  drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> > +drm-$(CONFIG_DRM_AFBC) += drm_afbc.o
> >  
> >  drm_vram_helper-y := drm_gem_vram_helper.o \
> >  		     drm_vram_helper_common.o \
> > diff --git a/drivers/gpu/drm/arm/Kconfig b/drivers/gpu/drm/arm/Kconfig
> > index a204103b3efb..25c3dc408cda 100644
> > --- a/drivers/gpu/drm/arm/Kconfig
> > +++ b/drivers/gpu/drm/arm/Kconfig
> > @@ -29,6 +29,7 @@ config DRM_MALI_DISPLAY
> >  	select DRM_KMS_HELPER
> >  	select DRM_KMS_CMA_HELPER
> >  	select DRM_GEM_CMA_HELPER
> > +	select DRM_AFBC
> >  	select VIDEOMODE_HELPERS
> >  	help
> >  	  Choose this option if you want to compile the ARM Mali Display
> > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> > index f25ec4382277..a67b69e08f63 100644
> > --- a/drivers/gpu/drm/arm/malidp_drv.c
> > +++ b/drivers/gpu/drm/arm/malidp_drv.c

This file is GPL-2.0-only. Shouldn't this be preserved when the
substantive bit of the code in drm_afbc.c comes from here?

> > @@ -16,6 +16,7 @@
> >  #include <linux/debugfs.h>
> >  
> >  #include <drm/drmP.h>
> > +#include <drm/drm_afbc.h>
> >  #include <drm/drm_atomic.h>
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_crtc.h>
> > @@ -33,8 +34,6 @@
> >  #include "malidp_hw.h"
> >  
> >  #define MALIDP_CONF_VALID_TIMEOUT	250
> > -#define AFBC_HEADER_SIZE		16
> > -#define AFBC_SUPERBLK_ALIGNMENT		128
> >  
> >  static void malidp_write_gamma_table(struct malidp_hw_device *hwdev,
> >  				     u32 data[MALIDP_COEFFTAB_NUM_COEFFS])
> > @@ -275,24 +274,8 @@ malidp_verify_afbc_framebuffer_caps(struct drm_device *dev,
> >  					mode_cmd->modifier[0]) == false)
> >  		return false;
> >  
> > -	if (mode_cmd->offsets[0] != 0) {
> > -		DRM_DEBUG_KMS("AFBC buffers' plane offset should be 0\n");
> > -		return false;
> > -	}
> > -
> > -	switch (mode_cmd->modifier[0] & AFBC_SIZE_MASK) {
> > -	case AFBC_SIZE_16X16:
> > -		if ((mode_cmd->width % 16) || (mode_cmd->height % 16)) {
> > -			DRM_DEBUG_KMS("AFBC buffers must be aligned to 16 pixels\n");
> > -			return false;
> > -		}
> > -		break;
> > -	default:
> > -		DRM_DEBUG_KMS("Unsupported AFBC block size\n");
> > -		return false;
> > -	}
> > -
> > -	return true;
> > +	return drm_afbc_check_offset(dev, mode_cmd) &&
> > +	       drm_afbc_check_size_align(dev, mode_cmd);
> >  }
> >  
> >  static bool
> > @@ -300,53 +283,20 @@ malidp_verify_afbc_framebuffer_size(struct drm_device *dev,
> >  				    struct drm_file *file,
> >  				    const struct drm_mode_fb_cmd2 *mode_cmd)
> >  {
> > -	int n_superblocks = 0;
> >  	const struct drm_format_info *info;
> >  	struct drm_gem_object *objs = NULL;
> > -	u32 afbc_superblock_size = 0, afbc_superblock_height = 0;
> > -	u32 afbc_superblock_width = 0, afbc_size = 0;
> >  	int bpp = 0;
> >  
> > -	switch (mode_cmd->modifier[0] & AFBC_SIZE_MASK) {
> > -	case AFBC_SIZE_16X16:
> > -		afbc_superblock_height = 16;
> > -		afbc_superblock_width = 16;
> > -		break;
> > -	default:
> > -		DRM_DEBUG_KMS("AFBC superblock size is not supported\n");
> > -		return false;
> > -	}
> > -
> >  	info = drm_get_format_info(dev, mode_cmd);
> > -
> > -	n_superblocks = (mode_cmd->width / afbc_superblock_width) *
> > -		(mode_cmd->height / afbc_superblock_height);
> > -
> >  	bpp = malidp_format_get_bpp(info->format);
> >  
> > -	afbc_superblock_size = (bpp * afbc_superblock_width * afbc_superblock_height)
> > -				/ BITS_PER_BYTE;
> > -
> > -	afbc_size = ALIGN(n_superblocks * AFBC_HEADER_SIZE, AFBC_SUPERBLK_ALIGNMENT);
> > -	afbc_size += n_superblocks * ALIGN(afbc_superblock_size, AFBC_SUPERBLK_ALIGNMENT);
> > -
> > -	if ((mode_cmd->width * bpp) != (mode_cmd->pitches[0] * BITS_PER_BYTE)) {
> > -		DRM_DEBUG_KMS("Invalid value of (pitch * BITS_PER_BYTE) (=%u) "
> > -			      "should be same as width (=%u) * bpp (=%u)\n",
> > -			      (mode_cmd->pitches[0] * BITS_PER_BYTE),
> > -			      mode_cmd->width, bpp);
> > -		return false;
> > -	}
> > -
> >  	objs = drm_gem_object_lookup(file, mode_cmd->handles[0]);
> >  	if (!objs) {
> >  		DRM_DEBUG_KMS("Failed to lookup GEM object\n");
> >  		return false;
> >  	}
> >  
> > -	if (objs->size < afbc_size) {
> > -		DRM_DEBUG_KMS("buffer size (%zu) too small for AFBC buffer size = %u\n",
> > -			      objs->size, afbc_size);
> > +	if (!drm_afbc_check_fb_size(dev, mode_cmd, objs, bpp)) {
> >  		drm_gem_object_put_unlocked(objs);
> >  		return false;
> >  	}
> Also can you do the code refactoring for komeda driver as well.
> specifically komeda_fb_afbc_size_check(). I will let
> james.qian.wang@arm.com and Mihail.Atanassov@arm.com have their
> opinion on this.

I'd say that it'd be really nice to get this work done for us ;)
but it doesn't have to be in this patch but rather in a follow-up.

> 
> > diff --git a/drivers/gpu/drm/drm_afbc.c b/drivers/gpu/drm/drm_afbc.c
> > new file mode 100644
> > index 000000000000..3e8a9225fd2e
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_afbc.c
> > @@ -0,0 +1,114 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * (C) 2019 Collabora Ltd.
> > + *
> > + * author: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> > + *
> > + */
> > +#include <linux/module.h>
> > +
> > +#include <drm/drm_afbc.h>
> > +#include <drm/drm_device.h>
> > +#include <drm/drm_fourcc.h>
> > +#include <drm/drm_gem.h>
> > +#include <drm/drm_mode.h>
> > +#include <drm/drm_print.h>
> > +
> > +#define AFBC_HEADER_SIZE		16
> > +#define AFBC_SUPERBLK_ALIGNMENT		128
> > +
> > +bool drm_afbc_check_offset(struct drm_device *dev,
> > +			   const struct drm_mode_fb_cmd2 *mode_cmd)
> > +{
> > +	if (mode_cmd->offsets[0] != 0) {
> > +		DRM_DEBUG_KMS("AFBC buffers' plane offset should be 0\n");
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +EXPORT_SYMBOL_GPL(drm_afbc_check_offset);
> > +
> > +bool drm_afbc_check_size_align(struct drm_device *dev,
> > +			       const struct drm_mode_fb_cmd2 *mode_cmd)
> > +{
> > +
> > +	switch (mode_cmd->modifier[0] & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> > +	case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> > +		if ((mode_cmd->width % 16) || (mode_cmd->height % 16)) {
> > +			DRM_DEBUG_KMS(
> > +				"AFBC buffer must be aligned to 16 pixels\n"
> > +			);
> > +			return false;
> > +		}
> > +		break;
> > +	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
> > +		/* fall through */
> > +	case AFBC_FORMAT_MOD_BLOCK_SIZE_64x4:
> > +		/* fall through */
> > +	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4:
> > +		/* fall through */
> > +	default:
> > +		DRM_DEBUG_KMS("Unsupported AFBC block size\n");
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +EXPORT_SYMBOL_GPL(drm_afbc_check_size_align);
> > +
> > +bool drm_afbc_check_fb_size(struct drm_device *dev,
> > +			    const struct drm_mode_fb_cmd2 *mode_cmd,
> > +			    struct drm_gem_object *objs, int bpp)
> > +{
> > +	int n_superblocks = 0;
> > +	u32 afbc_superblock_size = 0, afbc_superblock_height = 0;
> > +	u32 afbc_superblock_width = 0, afbc_size = 0;
> > +
> > +	switch (mode_cmd->modifier[0] & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> > +	case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> > +		afbc_superblock_height = 16;
> > +		afbc_superblock_width = 16;
> > +		break;
> > +	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
> Copying from
> https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c#n60
>                 afbc_superblock_width = 32;
>                 afbc_superblock_height = 8;
> > +		/* fall through */
> > +	case AFBC_FORMAT_MOD_BLOCK_SIZE_64x4:
> > +		/* fall through */
> > +	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4:
> > +		/* fall through */
> > +	default:
> > +		DRM_DEBUG_KMS("AFBC superblock size is not supported\n");
> > +		return false;
> > +	}
> Can you combine the two switch - case confitions (from this function
> and the one in drm_afbc_check_size_align()) and put it in a separate
> function (say drm_afbc_get_superblock_dimensions()) of its own ?
> This will help to avoid code repetition.
> 

I'm personally a bit on the fence about this - functions should ideally
do one thing. That shared helper would be both getting the dimensions
_and_ checking the mode_cmd is valid, so one place cares about one half
of the function's results, and the other - the second half. ¯\_O_/¯,
opinions, everybody has them :).

> > +
> > +	n_superblocks = (mode_cmd->width / afbc_superblock_width) *
> > +		(mode_cmd->height / afbc_superblock_height);
> > +
> > +	afbc_superblock_size =
> > +		(bpp * afbc_superblock_width * afbc_superblock_height)
> > +			/ BITS_PER_BYTE;
> > +
> > +	afbc_size = ALIGN(n_superblocks * AFBC_HEADER_SIZE,
> > +			  AFBC_SUPERBLK_ALIGNMENT);
> > +	afbc_size += n_superblocks *
> > +		ALIGN(afbc_superblock_size, AFBC_SUPERBLK_ALIGNMENT);
> > +
> > +	if ((mode_cmd->width * bpp) != (mode_cmd->pitches[0] * BITS_PER_BYTE)) {
> > +		DRM_DEBUG_KMS("Invalid value of (pitch * BITS_PER_BYTE) (=%u) should be same as width (=%u) * bpp (=%u)\n",
> > +			mode_cmd->pitches[0] * BITS_PER_BYTE,
> > +			mode_cmd->width, bpp
> > +		);
> > +		return false;
> > +	}
> > +
> > +	if (objs->size < afbc_size) {
> > +		DRM_DEBUG_KMS("buffer size (%zu) too small for AFBC buffer size = %u\n",
> > +			objs->size, afbc_size
> > +		);
> > +
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +EXPORT_SYMBOL(drm_afbc_check_fb_size);
> > diff --git a/include/drm/drm_afbc.h b/include/drm/drm_afbc.h
> > new file mode 100644
> > index 000000000000..ce39c850217b
> > --- /dev/null
> > +++ b/include/drm/drm_afbc.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * (C) 2019 Collabora Ltd.
> > + *
> > + * author: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> > + *
> > + */
> > +#ifndef __DRM_AFBC_H__
> > +#define __DRM_AFBC_H__
> > +
> > +struct drm_device;
> > +struct drm_mode_fb_cmd2;
> > +struct drm_gem_object;
> > +
> > +bool drm_afbc_check_offset(struct drm_device *dev,
> > +			   const struct drm_mode_fb_cmd2 *mode_cmd);
> > +
> > +bool drm_afbc_check_size_align(struct drm_device *dev,
> > +			       const struct drm_mode_fb_cmd2 *mode_cmd);
> > +
> > +bool drm_afbc_check_fb_size(struct drm_device *dev,
> > +			    const struct drm_mode_fb_cmd2 *mode_cmd,
> > +			    struct drm_gem_object *objs, int bpp);
> > +
> > +#endif /* __DRM_AFBC_H__ */
> > --
> > 2.17.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Overall the patch LGTM as-is, nice and straightforward
mostly-cut-n-paste, just let's sort out the SPDX identifier question
I have, then I'll be happy.

-- 
Mihail




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel