[RFC PATCH 0/4] Add support for Arm Framebuffer Compression(AFBC)

[RFC PATCH 0/4] Add support for Arm Framebuffer Compression(AFBC)

[Ayan Kumar Halder]

In the current series of patches, we are trying to add support for AFBC
modifiers in malidp. AFBC modifiers adds some constraints to framebuffer
size, alignment, pitch, formats, etc. Here we are trying to add support
for one combination of AFBC modifier ie AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
AFBC_FORMAT_MOD_SPARSE | AFBC_FORMAT_MOD_YTR.
In future, we intend to add support for more combination of AFBC modifiers.
Currently, we are trying to enable a basic support of AFBC in malidp.

Ayan Kumar Halder (4):
  drm/arm/malidp: Add modifier definitions for describing Arm
    Framebuffer Compression (AFBC).
  drm/arm/malidp: Implemented the size validation for AFBC framebuffers
  drm/arm/malidp: Set the AFBC register bits if the framebuffer has AFBC
    modifier
  drm/arm/malidp: Added support for AFBC modifiers for all layers except
    DE_SMART

 drivers/gpu/drm/arm/malidp_drv.c    | 129 +++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/arm/malidp_hw.c     |  27 +++++---
 drivers/gpu/drm/arm/malidp_hw.h     |   7 ++
 drivers/gpu/drm/arm/malidp_planes.c | 127 ++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/arm/malidp_regs.h   |  20 ++++++
 include/uapi/drm/drm_fourcc.h       |  83 +++++++++++++++++++++++
 6 files changed, 371 insertions(+), 22 deletions(-)

-- 
2.7.4

[RFC PATCH 1/4] drm/arm/malidp: Add modifier definitions for describing Arm Framebuffer Compression (AFBC).

[Ayan Kumar Halder]

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.
AFBC has several features which may be supported and/or used, which are
represented using bits in the modifier. Not all combinations are valid,
and different devices or use-cases may support different combinations.

Signed-off-by: Rosen Zhelev <rosen.zhelev@arm.com>
Signed-off-by: Ayan Kumar halder <ayan.halder@arm.com>
Reviewed-by: Brian Starkey <brian.starkey@arm.com>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
Reviewed-by: James (Qian) Wang <james.qian.wang@arm.com>
---
 include/uapi/drm/drm_fourcc.h | 83 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index e04613d..af7e9ab 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -183,6 +183,7 @@ extern "C" {
 #define DRM_FORMAT_MOD_VENDOR_QCOM    0x05
 #define DRM_FORMAT_MOD_VENDOR_VIVANTE 0x06
 #define DRM_FORMAT_MOD_VENDOR_BROADCOM 0x07
+#define DRM_FORMAT_MOD_VENDOR_ARM     0x08
 /* add more to the end as needed */
 
 #define DRM_FORMAT_RESERVED	      ((1ULL << 56) - 1)
@@ -405,6 +406,88 @@ extern "C" {
  */
 #define DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED fourcc_mod_code(BROADCOM, 1)
 
+/*
+ * Arm Framebuffer Compression (AFBC) modifiers
+ *
+ * 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.
+ *
+ * AFBC has several features which may be supported and/or used, which are
+ * represented using bits in the modifier. Not all combinations are valid,
+ * and different devices or use-cases may support different combinations.
+ */
+#define DRM_FORMAT_MOD_ARM_AFBC(__afbc_mode)	fourcc_mod_code(ARM, __afbc_mode)
+
+/*
+ * AFBC superblock size
+ *
+ * Indicates the superblock size(s) used for the AFBC buffer. The buffer
+ * size (in pixels) must be aligned to a multiple of the superblock size.
+ * Four lowest significant bits(LSBs) are reserved for block size.
+ */
+#define AFBC_FORMAT_MOD_BLOCK_SIZE_MASK      0xf
+#define AFBC_FORMAT_MOD_BLOCK_SIZE_16x16     (1ULL)
+#define AFBC_FORMAT_MOD_BLOCK_SIZE_32x8      (2ULL)
+
+/*
+ * AFBC lossless colorspace transform
+ *
+ * Indicates that the buffer makes use of the AFBC lossless colorspace
+ * transform.
+ */
+#define AFBC_FORMAT_MOD_YTR     (1ULL <<  4)
+
+/*
+ * AFBC block-split
+ *
+ * Indicates that the payload of each superblock is split. The second
+ * half of the payload is positioned at a predefined offset from the start
+ * of the superblock payload.
+ */
+#define AFBC_FORMAT_MOD_SPLIT   (1ULL <<  5)
+
+/*
+ * AFBC sparse layout
+ *
+ * This flag indicates that the payload of each superblock must be stored at a
+ * predefined position relative to the other superblocks in the same AFBC
+ * buffer. This order is the same order used by the header buffer. In this mode
+ * each superblock is given the same amount of space as an uncompressed
+ * superblock of the particular format would require, rounding up to the next
+ * multiple of 128 bytes in size.
+ */
+#define AFBC_FORMAT_MOD_SPARSE  (1ULL <<  6)
+
+/*
+ * AFBC copy-block restrict
+ *
+ * Buffers with this flag must obey the copy-block restriction. The restriction
+ * is such that there are no copy-blocks referring across the border of 8x8
+ * blocks. For the subsampled data the 8x8 limitation is also subsampled.
+ */
+#define AFBC_FORMAT_MOD_CBR     (1ULL <<  7)
+
+/*
+ * AFBC tiled layout
+ *
+ * The tiled layout groups superblocks in 8x8 or 4x4 tiles, where all
+ * superblocks inside a tile are stored together in memory. 8x8 tiles are used
+ * for pixel formats up to and including 32 bpp while 4x4 tiles are used for
+ * larger bpp formats. The order between the tiles is scan line.
+ * When the tiled layout is used, the buffer size (in pixels) must be aligned
+ * to the tile size.
+ */
+#define AFBC_FORMAT_MOD_TILED   (1ULL <<  8)
+
+/*
+ * AFBC solid color blocks
+ *
+ * Indicates that the buffer makes use of solid-color blocks, whereby bandwidth
+ * can be reduced if a whole superblock is a single color.
+ */
+#define AFBC_FORMAT_MOD_SC      (1ULL <<  9)
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 2/4] drm/arm/malidp: Implemented the size validation for AFBC framebuffers

[Ayan Kumar Halder]

AFBC buffers include additional metadata which increases the required
allocation size. Implement the appropriate size validation and sanity
checking for AFBC buffers.
Added malidp specific function for framebuffer creation. This checks
if the framebuffer has AFBC modifiers and if so, it verifies the
necessary constraints on the size, alignment, offsets and pitch.

Signed-off-by: Ayan Kumar halder <ayan.halder@arm.com>
Reviewed-by: Brian Starkey <brian.starkey@arm.com>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
---
 drivers/gpu/drm/arm/malidp_drv.c | 128 ++++++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/arm/malidp_hw.h  |   5 ++
 2 files changed, 132 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 8d20faa..7b6a848 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -35,6 +35,7 @@
 #include "malidp_hw.h"
 
 #define MALIDP_CONF_VALID_TIMEOUT	250
+#define AFBC_HEADER_SIZE		16
 
 static void malidp_write_gamma_table(struct malidp_hw_device *hwdev,
 				     u32 data[MALIDP_COEFFTAB_NUM_COEFFS])
@@ -245,8 +246,133 @@ static const struct drm_mode_config_helper_funcs malidp_mode_config_helpers = {
 	.atomic_commit_tail = malidp_atomic_commit_tail,
 };
 
+static bool
+malidp_verify_afbc_framebuffer_caps(struct drm_device *dev,
+				       const struct drm_mode_fb_cmd2 *mode_cmd)
+{
+	const struct drm_format_info *info;
+
+	if ((mode_cmd->modifier[0] >> 56) != DRM_FORMAT_MOD_VENDOR_ARM) {
+		DRM_ERROR("Unknown modifier (not Arm)\n");
+		return false;
+	}
+
+	if (mode_cmd->modifier[0] &
+	    ~DRM_FORMAT_MOD_ARM_AFBC(AFBC_MOD_VALID_BITS)) {
+		DRM_ERROR("Unsupported modifiers\n");
+		return false;
+	}
+
+	info = drm_get_format_info(dev, mode_cmd);
+	if (!info) {
+		DRM_ERROR("Unable to get the format information\n");
+		return false;
+	}
+
+	if (info->num_planes != 1) {
+		DRM_ERROR("AFBC buffers expect one plane\n");
+		return false;
+	}
+
+	if (mode_cmd->offsets[0] != 0) {
+		DRM_ERROR("AFBC buffers' plane offset should be 0\n");
+		return false;
+	}
+
+	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_ERROR("AFBC buffers must be aligned to 16 pixels\n");
+			return false;
+		}
+		break;
+	default:
+		DRM_ERROR("Unsupported AFBC block size\n");
+		return false;
+	}
+
+	return true;
+}
+
+static bool
+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;
+
+	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;
+	default:
+		DRM_ERROR("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);
+
+	afbc_superblock_size = info->cpp[0] * afbc_superblock_width *
+		afbc_superblock_height;
+
+	afbc_size = ALIGN(n_superblocks * AFBC_HEADER_SIZE, 128);
+
+	if (mode_cmd->width * info->cpp[0] != mode_cmd->pitches[0]) {
+		DRM_ERROR("Invalid value of pitch (=%u) should be same as width (=%u) * cpp (=%u)\n",
+			  mode_cmd->pitches[0], mode_cmd->width, info->cpp[0]);
+		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_ERROR("buffer size (%zu) too small for AFBC buffer size = %u\n",
+			  objs->size, afbc_size);
+		drm_gem_object_put_unlocked(objs);
+		return false;
+	}
+
+	drm_gem_object_put_unlocked(objs);
+
+	return true;
+}
+
+static bool
+malidp_verify_afbc_framebuffer(struct drm_device *dev, struct drm_file *file,
+				  const struct drm_mode_fb_cmd2 *mode_cmd)
+{
+	if (malidp_verify_afbc_framebuffer_caps(dev, mode_cmd))
+		return malidp_verify_afbc_framebuffer_size(dev, file, mode_cmd);
+
+	return false;
+}
+
+struct drm_framebuffer *
+malidp_fb_create(struct drm_device *dev, struct drm_file *file,
+		 const struct drm_mode_fb_cmd2 *mode_cmd)
+{
+	if (mode_cmd->modifier[0]) {
+		if (!malidp_verify_afbc_framebuffer(dev, file, mode_cmd))
+			return ERR_PTR(-EINVAL);
+	}
+
+	return drm_gem_fb_create(dev, file, mode_cmd);
+}
+
 static const struct drm_mode_config_funcs malidp_mode_config_funcs = {
-	.fb_create = drm_gem_fb_create,
+	.fb_create = malidp_fb_create,
 	.output_poll_changed = drm_fb_helper_output_poll_changed,
 	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = drm_atomic_helper_commit,
diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
index 41f4521..4390243 100644
--- a/drivers/gpu/drm/arm/malidp_hw.h
+++ b/drivers/gpu/drm/arm/malidp_hw.h
@@ -361,4 +361,9 @@ static inline void malidp_se_set_enh_coeffs(struct malidp_hw_device *hwdev)
 
 #define MALIDP_GAMMA_LUT_SIZE		4096
 
+#define AFBC_MOD_VALID_BITS (AFBC_FORMAT_MOD_BLOCK_SIZE_MASK | \
+			AFBC_FORMAT_MOD_YTR | AFBC_FORMAT_MOD_SPLIT | \
+			AFBC_FORMAT_MOD_SPARSE | AFBC_FORMAT_MOD_CBR | \
+			AFBC_FORMAT_MOD_TILED | AFBC_FORMAT_MOD_SC)
+
 #endif  /* __MALIDP_HW_H__ */
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 3/4] drm/arm/malidp: Set the AFBC register bits if the framebuffer has AFBC modifier

[Ayan Kumar Halder]

Added the AFBC decoder registers for DP500 , DP550 and DP650.
These registers control the processing of AFBC buffers. It controls various
features like AFBC decoder enable, lossless transformation and block split
as well as setting of the left, right, top and bottom cropping of AFBC buffers
(in number of pixels).
All the layers (except DE_SMART) support framebuffers with AFBC modifiers.
One needs to set the pixel values of the top, left, bottom and right cropping
for the AFBC framebuffer.
Added the functionality in malidp_de_plane_update() to set the various
registers for AFBC decoder, depending on the modifiers.

Signed-off-by: Ayan Kumar halder <ayan.halder@arm.com>
Reviewed-by: Brian Starkey <brian.starkey@arm.com>
---
 drivers/gpu/drm/arm/malidp_hw.c     | 27 ++++++++-----
 drivers/gpu/drm/arm/malidp_hw.h     |  2 +
 drivers/gpu/drm/arm/malidp_planes.c | 81 +++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/arm/malidp_regs.h   | 20 +++++++++
 4 files changed, 111 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index 4dbf39f..fd6b510 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -76,33 +76,38 @@ static const struct malidp_format_id malidp550_de_formats[] = {
 
 static const struct malidp_layer malidp500_layers[] = {
 	{ DE_VIDEO1, MALIDP500_DE_LV_BASE, MALIDP500_DE_LV_PTR_BASE,
-		MALIDP_DE_LV_STRIDE0, MALIDP500_LV_YUV2RGB, ROTATE_ANY },
+		MALIDP_DE_LV_STRIDE0, MALIDP500_LV_YUV2RGB, ROTATE_ANY,
+		MALIDP500_DE_LV_AD_CTRL },
 	{ DE_GRAPHICS1, MALIDP500_DE_LG1_BASE, MALIDP500_DE_LG1_PTR_BASE,
-		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY },
+		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY, MALIDP500_DE_LG1_AD_CTRL },
 	{ DE_GRAPHICS2, MALIDP500_DE_LG2_BASE, MALIDP500_DE_LG2_PTR_BASE,
-		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY },
+		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY, MALIDP500_DE_LG2_AD_CTRL },
 };
 
 static const struct malidp_layer malidp550_layers[] = {
 	{ DE_VIDEO1, MALIDP550_DE_LV1_BASE, MALIDP550_DE_LV1_PTR_BASE,
-		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY },
+		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY,
+		MALIDP550_DE_LV1_AD_CTRL },
 	{ DE_GRAPHICS1, MALIDP550_DE_LG_BASE, MALIDP550_DE_LG_PTR_BASE,
-		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY },
+		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY, MALIDP550_DE_LG_AD_CTRL },
 	{ DE_VIDEO2, MALIDP550_DE_LV2_BASE, MALIDP550_DE_LV2_PTR_BASE,
-		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY },
+		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY,
+		MALIDP550_DE_LV2_AD_CTRL },
 	{ DE_SMART, MALIDP550_DE_LS_BASE, MALIDP550_DE_LS_PTR_BASE,
-		MALIDP550_DE_LS_R1_STRIDE, 0, ROTATE_NONE },
+		MALIDP550_DE_LS_R1_STRIDE, 0, ROTATE_NONE, 0 },
 };
 
 static const struct malidp_layer malidp650_layers[] = {
 	{ DE_VIDEO1, MALIDP550_DE_LV1_BASE, MALIDP550_DE_LV1_PTR_BASE,
-		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY },
+		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY,
+		MALIDP550_DE_LV1_AD_CTRL },
 	{ DE_GRAPHICS1, MALIDP550_DE_LG_BASE, MALIDP550_DE_LG_PTR_BASE,
-		MALIDP_DE_LG_STRIDE, 0, ROTATE_COMPRESSED },
+		MALIDP_DE_LG_STRIDE, 0, ROTATE_COMPRESSED, MALIDP550_DE_LG_AD_CTRL },
 	{ DE_VIDEO2, MALIDP550_DE_LV2_BASE, MALIDP550_DE_LV2_PTR_BASE,
-		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY },
+		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY,
+		MALIDP550_DE_LV2_AD_CTRL },
 	{ DE_SMART, MALIDP550_DE_LS_BASE, MALIDP550_DE_LS_PTR_BASE,
-		MALIDP550_DE_LS_R1_STRIDE, 0, ROTATE_NONE },
+		MALIDP550_DE_LS_R1_STRIDE, 0, ROTATE_NONE, 0 },
 };
 
 #define SE_N_SCALING_COEFFS	96
diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
index 4390243..bbe6883 100644
--- a/drivers/gpu/drm/arm/malidp_hw.h
+++ b/drivers/gpu/drm/arm/malidp_hw.h
@@ -67,6 +67,8 @@ struct malidp_layer {
 	u16 stride_offset;	/* offset to the first stride register. */
 	s16 yuv2rgb_offset;	/* offset to the YUV->RGB matrix entries */
 	enum rotation_features rot;    /* type of rotation supported */
+	/* address offset for the AFBC decoder registers */
+	u16 afbc_decoder_offset;
 };
 
 enum malidp_scaling_coeff_set {
diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
index 533cdde..3950504 100644
--- a/drivers/gpu/drm/arm/malidp_planes.c
+++ b/drivers/gpu/drm/arm/malidp_planes.c
@@ -330,6 +330,71 @@ static void malidp_de_set_color_encoding(struct malidp_plane *plane,
 	}
 }
 
+static void malidp_set_plane_base_addr(struct drm_framebuffer *fb,
+				       struct malidp_plane *mp,
+				       int plane_index)
+{
+	dma_addr_t paddr;
+	u16 ptr;
+	struct drm_plane *plane = &mp->base;
+	bool afbc = fb->modifier ? true : false;
+
+	ptr = mp->layer->ptr + (plane_index << 4);
+
+	/*
+	 * For AFBC buffers, cropping is handled by AFBC decoder rather than
+	 * pointer manipulation.
+	 */
+	if (!afbc) {
+		paddr = drm_fb_cma_get_gem_addr(fb, plane->state,
+						plane_index);
+	} else {
+		struct drm_gem_cma_object *obj;
+
+		obj = drm_fb_cma_get_gem_obj(fb, plane_index);
+
+		if (WARN_ON(!obj))
+			return;
+		paddr = obj->paddr;
+	}
+
+	malidp_hw_write(mp->hwdev, lower_32_bits(paddr), ptr);
+	malidp_hw_write(mp->hwdev, upper_32_bits(paddr), ptr + 4);
+}
+
+static void malidp_de_set_plane_afbc(struct drm_plane *plane)
+{
+	struct malidp_plane *mp;
+	u32 src_w, src_h, val = 0, src_x, src_y;
+	struct drm_framebuffer *fb = plane->state->fb;
+
+	mp = to_malidp_plane(plane);
+
+	/* convert src values from Q16 fixed point to integer */
+	src_w = plane->state->src_w >> 16;
+	src_h = plane->state->src_h >> 16;
+	src_x = plane->state->src_x >> 16;
+	src_y = plane->state->src_y >> 16;
+
+	val = ((fb->width - (src_x + src_w)) << MALIDP_AD_CROP_RIGHT_OFFSET) |
+		   src_x;
+	malidp_hw_write(mp->hwdev, val,
+			mp->layer->afbc_decoder_offset + MALIDP_AD_CROP_H);
+
+	val = ((fb->height - (src_y + src_h)) << MALIDP_AD_CROP_BOTTOM_OFFSET) |
+		   src_y;
+	malidp_hw_write(mp->hwdev, val,
+			mp->layer->afbc_decoder_offset + MALIDP_AD_CROP_V);
+
+	val = MALIDP_AD_EN;
+	if (fb->modifier & AFBC_FORMAT_MOD_SPLIT)
+		val |= MALIDP_AD_BS;
+	if (fb->modifier & AFBC_FORMAT_MOD_YTR)
+		val |= MALIDP_AD_YTR;
+
+	malidp_hw_write(mp->hwdev, val, mp->layer->afbc_decoder_offset);
+}
+
 static void malidp_de_plane_update(struct drm_plane *plane,
 				   struct drm_plane_state *old_state)
 {
@@ -338,6 +403,7 @@ static void malidp_de_plane_update(struct drm_plane *plane,
 	u32 src_w, src_h, dest_w, dest_h, val;
 	int i;
 	bool format_has_alpha = plane->state->fb->format->has_alpha;
+	struct drm_framebuffer *fb = plane->state->fb;
 
 	mp = to_malidp_plane(plane);
 
@@ -349,15 +415,9 @@ static void malidp_de_plane_update(struct drm_plane *plane,
 
 	malidp_hw_write(mp->hwdev, ms->format, mp->layer->base);
 
-	for (i = 0; i < ms->n_planes; i++) {
-		/* calculate the offset for the layer's plane registers */
-		u16 ptr = mp->layer->ptr + (i << 4);
-		dma_addr_t fb_addr = drm_fb_cma_get_gem_addr(plane->state->fb,
-							     plane->state, i);
+	for (i = 0; i < ms->n_planes; i++)
+		malidp_set_plane_base_addr(fb, mp, i);
 
-		malidp_hw_write(mp->hwdev, lower_32_bits(fb_addr), ptr);
-		malidp_hw_write(mp->hwdev, upper_32_bits(fb_addr), ptr + 4);
-	}
 	malidp_de_set_plane_pitches(mp, ms->n_planes,
 				    plane->state->fb->pitches);
 
@@ -381,6 +441,11 @@ static void malidp_de_plane_update(struct drm_plane *plane,
 				LAYER_H_VAL(src_w) | LAYER_V_VAL(src_h),
 				mp->layer->base + MALIDP550_LS_R1_IN_SIZE);
 
+	if (fb->modifier)
+		malidp_de_set_plane_afbc(plane);
+	else
+		malidp_hw_write(mp->hwdev, 0, mp->layer->afbc_decoder_offset);
+
 	/* first clear the rotation bits */
 	val = malidp_hw_read(mp->hwdev, mp->layer->base + MALIDP_LAYER_CONTROL);
 	val &= ~LAYER_ROT_MASK;
diff --git a/drivers/gpu/drm/arm/malidp_regs.h b/drivers/gpu/drm/arm/malidp_regs.h
index 149024f..54f4ec5 100644
--- a/drivers/gpu/drm/arm/malidp_regs.h
+++ b/drivers/gpu/drm/arm/malidp_regs.h
@@ -180,10 +180,13 @@
 #define MALIDP500_LV_YUV2RGB		((s16)(-0xB8))
 #define MALIDP500_DE_LV_BASE		0x00100
 #define MALIDP500_DE_LV_PTR_BASE	0x00124
+#define MALIDP500_DE_LV_AD_CTRL		0x00400
 #define MALIDP500_DE_LG1_BASE		0x00200
 #define MALIDP500_DE_LG1_PTR_BASE	0x0021c
+#define MALIDP500_DE_LG1_AD_CTRL	0x0040c
 #define MALIDP500_DE_LG2_BASE		0x00300
 #define MALIDP500_DE_LG2_PTR_BASE	0x0031c
+#define MALIDP500_DE_LG2_AD_CTRL	0x00418
 #define MALIDP500_SE_BASE		0x00c00
 #define MALIDP500_SE_CONTROL		0x00c0c
 #define MALIDP500_SE_PTR_BASE		0x00e0c
@@ -208,10 +211,13 @@
 #define MALIDP550_LV_YUV2RGB		0x00084
 #define MALIDP550_DE_LV1_BASE		0x00100
 #define MALIDP550_DE_LV1_PTR_BASE	0x00124
+#define MALIDP550_DE_LV1_AD_CTRL	0x001B8
 #define MALIDP550_DE_LV2_BASE		0x00200
 #define MALIDP550_DE_LV2_PTR_BASE	0x00224
+#define MALIDP550_DE_LV2_AD_CTRL	0x002B8
 #define MALIDP550_DE_LG_BASE		0x00300
 #define MALIDP550_DE_LG_PTR_BASE	0x0031c
+#define MALIDP550_DE_LG_AD_CTRL		0x00330
 #define MALIDP550_DE_LS_BASE		0x00400
 #define MALIDP550_DE_LS_PTR_BASE	0x0042c
 #define MALIDP550_DE_PERF_BASE		0x00500
@@ -223,6 +229,20 @@
 #define MALIDP550_CONFIG_VALID		0x0c014
 #define MALIDP550_CONFIG_ID		0x0ffd4
 
+/* AFBC register offsets relative to MALIDPXXX_DE_LX_AD_CTRL */
+/* The following register offsets are common for DP500, DP550 and DP650 */
+#define MALIDP_AD_CROP_H                0x4
+#define MALIDP_AD_CROP_V                0x8
+#define MALIDP_AD_END_PTR_LOW           0xc
+#define MALIDP_AD_END_PTR_HIGH          0x10
+
+/* AFBC decoder Registers */
+#define MALIDP_AD_EN                    BIT(0)
+#define MALIDP_AD_YTR                   BIT(4)
+#define MALIDP_AD_BS                    BIT(8)
+#define MALIDP_AD_CROP_RIGHT_OFFSET     16
+#define MALIDP_AD_CROP_BOTTOM_OFFSET    16
+
 /*
  * Starting with DP550 the register map blocks has been standardised to the
  * following layout:
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 3/4] drm/arm/malidp: Set the AFBC register bits if the framebuffer has AFBC modifier

[Liviu Dudau]

Hi Ayan,

Thanks for the patch! I have some small comments to make:

On Fri, Jun 15, 2018 at 02:51:33PM +0100, Ayan Kumar Halder wrote:
> Added the AFBC decoder registers for DP500 , DP550 and DP650.
> These registers control the processing of AFBC buffers. It controls various
> features like AFBC decoder enable, lossless transformation and block split
> as well as setting of the left, right, top and bottom cropping of AFBC buffers
> (in number of pixels).
> All the layers (except DE_SMART) support framebuffers with AFBC modifiers.
> One needs to set the pixel values of the top, left, bottom and right cropping
> for the AFBC framebuffer.
> Added the functionality in malidp_de_plane_update() to set the various
> registers for AFBC decoder, depending on the modifiers.
> 
> Signed-off-by: Ayan Kumar halder <ayan.halder@arm.com>
> Reviewed-by: Brian Starkey <brian.starkey@arm.com>
> ---
>  drivers/gpu/drm/arm/malidp_hw.c     | 27 ++++++++-----
>  drivers/gpu/drm/arm/malidp_hw.h     |  2 +
>  drivers/gpu/drm/arm/malidp_planes.c | 81 +++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/arm/malidp_regs.h   | 20 +++++++++
>  4 files changed, 111 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
> index 4dbf39f..fd6b510 100644
> --- a/drivers/gpu/drm/arm/malidp_hw.c
> +++ b/drivers/gpu/drm/arm/malidp_hw.c
> @@ -76,33 +76,38 @@ static const struct malidp_format_id malidp550_de_formats[] = {
>  
>  static const struct malidp_layer malidp500_layers[] = {
>  	{ DE_VIDEO1, MALIDP500_DE_LV_BASE, MALIDP500_DE_LV_PTR_BASE,
> -		MALIDP_DE_LV_STRIDE0, MALIDP500_LV_YUV2RGB, ROTATE_ANY },
> +		MALIDP_DE_LV_STRIDE0, MALIDP500_LV_YUV2RGB, ROTATE_ANY,
> +		MALIDP500_DE_LV_AD_CTRL },
>  	{ DE_GRAPHICS1, MALIDP500_DE_LG1_BASE, MALIDP500_DE_LG1_PTR_BASE,
> -		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY },
> +		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY, MALIDP500_DE_LG1_AD_CTRL },
>  	{ DE_GRAPHICS2, MALIDP500_DE_LG2_BASE, MALIDP500_DE_LG2_PTR_BASE,
> -		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY },
> +		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY, MALIDP500_DE_LG2_AD_CTRL },
>  };
>  
>  static const struct malidp_layer malidp550_layers[] = {
>  	{ DE_VIDEO1, MALIDP550_DE_LV1_BASE, MALIDP550_DE_LV1_PTR_BASE,
> -		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY },
> +		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY,
> +		MALIDP550_DE_LV1_AD_CTRL },
>  	{ DE_GRAPHICS1, MALIDP550_DE_LG_BASE, MALIDP550_DE_LG_PTR_BASE,
> -		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY },
> +		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY, MALIDP550_DE_LG_AD_CTRL },
>  	{ DE_VIDEO2, MALIDP550_DE_LV2_BASE, MALIDP550_DE_LV2_PTR_BASE,
> -		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY },
> +		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY,
> +		MALIDP550_DE_LV2_AD_CTRL },
>  	{ DE_SMART, MALIDP550_DE_LS_BASE, MALIDP550_DE_LS_PTR_BASE,
> -		MALIDP550_DE_LS_R1_STRIDE, 0, ROTATE_NONE },
> +		MALIDP550_DE_LS_R1_STRIDE, 0, ROTATE_NONE, 0 },
>  };
>  
>  static const struct malidp_layer malidp650_layers[] = {
>  	{ DE_VIDEO1, MALIDP550_DE_LV1_BASE, MALIDP550_DE_LV1_PTR_BASE,
> -		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY },
> +		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY,
> +		MALIDP550_DE_LV1_AD_CTRL },
>  	{ DE_GRAPHICS1, MALIDP550_DE_LG_BASE, MALIDP550_DE_LG_PTR_BASE,
> -		MALIDP_DE_LG_STRIDE, 0, ROTATE_COMPRESSED },
> +		MALIDP_DE_LG_STRIDE, 0, ROTATE_COMPRESSED, MALIDP550_DE_LG_AD_CTRL },
>  	{ DE_VIDEO2, MALIDP550_DE_LV2_BASE, MALIDP550_DE_LV2_PTR_BASE,
> -		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY },
> +		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY,
> +		MALIDP550_DE_LV2_AD_CTRL },
>  	{ DE_SMART, MALIDP550_DE_LS_BASE, MALIDP550_DE_LS_PTR_BASE,
> -		MALIDP550_DE_LS_R1_STRIDE, 0, ROTATE_NONE },
> +		MALIDP550_DE_LS_R1_STRIDE, 0, ROTATE_NONE, 0 },
>  };
>  
>  #define SE_N_SCALING_COEFFS	96
> diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
> index 4390243..bbe6883 100644
> --- a/drivers/gpu/drm/arm/malidp_hw.h
> +++ b/drivers/gpu/drm/arm/malidp_hw.h
> @@ -67,6 +67,8 @@ struct malidp_layer {
>  	u16 stride_offset;	/* offset to the first stride register. */
>  	s16 yuv2rgb_offset;	/* offset to the YUV->RGB matrix entries */
>  	enum rotation_features rot;    /* type of rotation supported */
> +	/* address offset for the AFBC decoder registers */
> +	u16 afbc_decoder_offset;
>  };
>  
>  enum malidp_scaling_coeff_set {
> diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
> index 533cdde..3950504 100644
> --- a/drivers/gpu/drm/arm/malidp_planes.c
> +++ b/drivers/gpu/drm/arm/malidp_planes.c
> @@ -330,6 +330,71 @@ static void malidp_de_set_color_encoding(struct malidp_plane *plane,
>  	}
>  }
>  
> +static void malidp_set_plane_base_addr(struct drm_framebuffer *fb,
> +				       struct malidp_plane *mp,
> +				       int plane_index)
> +{
> +	dma_addr_t paddr;
> +	u16 ptr;
> +	struct drm_plane *plane = &mp->base;
> +	bool afbc = fb->modifier ? true : false;

The decision to set afbc based on wether the fb has a modifier or not
seems a bit weak. Should we also (at least) check that the modifier is
an ARM one?

> +
> +	ptr = mp->layer->ptr + (plane_index << 4);
> +
> +	/*
> +	 * For AFBC buffers, cropping is handled by AFBC decoder rather than
> +	 * pointer manipulation.
> +	 */
> +	if (!afbc) {
> +		paddr = drm_fb_cma_get_gem_addr(fb, plane->state,
> +						plane_index);
> +	} else {
> +		struct drm_gem_cma_object *obj;
> +
> +		obj = drm_fb_cma_get_gem_obj(fb, plane_index);
> +
> +		if (WARN_ON(!obj))
> +			return;
> +		paddr = obj->paddr;
> +	}
> +
> +	malidp_hw_write(mp->hwdev, lower_32_bits(paddr), ptr);
> +	malidp_hw_write(mp->hwdev, upper_32_bits(paddr), ptr + 4);
> +}
> +
> +static void malidp_de_set_plane_afbc(struct drm_plane *plane)
> +{
> +	struct malidp_plane *mp;
> +	u32 src_w, src_h, val = 0, src_x, src_y;
> +	struct drm_framebuffer *fb = plane->state->fb;
> +
> +	mp = to_malidp_plane(plane);
> +
> +	/* convert src values from Q16 fixed point to integer */
> +	src_w = plane->state->src_w >> 16;
> +	src_h = plane->state->src_h >> 16;
> +	src_x = plane->state->src_x >> 16;
> +	src_y = plane->state->src_y >> 16;
> +
> +	val = ((fb->width - (src_x + src_w)) << MALIDP_AD_CROP_RIGHT_OFFSET) |
> +		   src_x;
> +	malidp_hw_write(mp->hwdev, val,
> +			mp->layer->afbc_decoder_offset + MALIDP_AD_CROP_H);
> +
> +	val = ((fb->height - (src_y + src_h)) << MALIDP_AD_CROP_BOTTOM_OFFSET) |
> +		   src_y;
> +	malidp_hw_write(mp->hwdev, val,
> +			mp->layer->afbc_decoder_offset + MALIDP_AD_CROP_V);
> +
> +	val = MALIDP_AD_EN;
> +	if (fb->modifier & AFBC_FORMAT_MOD_SPLIT)
> +		val |= MALIDP_AD_BS;
> +	if (fb->modifier & AFBC_FORMAT_MOD_YTR)
> +		val |= MALIDP_AD_YTR;
> +
> +	malidp_hw_write(mp->hwdev, val, mp->layer->afbc_decoder_offset);
> +}
> +
>  static void malidp_de_plane_update(struct drm_plane *plane,
>  				   struct drm_plane_state *old_state)
>  {
> @@ -338,6 +403,7 @@ static void malidp_de_plane_update(struct drm_plane *plane,
>  	u32 src_w, src_h, dest_w, dest_h, val;
>  	int i;
>  	bool format_has_alpha = plane->state->fb->format->has_alpha;
> +	struct drm_framebuffer *fb = plane->state->fb;
>  
>  	mp = to_malidp_plane(plane);
>  
> @@ -349,15 +415,9 @@ static void malidp_de_plane_update(struct drm_plane *plane,
>  
>  	malidp_hw_write(mp->hwdev, ms->format, mp->layer->base);
>  
> -	for (i = 0; i < ms->n_planes; i++) {
> -		/* calculate the offset for the layer's plane registers */
> -		u16 ptr = mp->layer->ptr + (i << 4);
> -		dma_addr_t fb_addr = drm_fb_cma_get_gem_addr(plane->state->fb,
> -							     plane->state, i);
> +	for (i = 0; i < ms->n_planes; i++)
> +		malidp_set_plane_base_addr(fb, mp, i);
>  
> -		malidp_hw_write(mp->hwdev, lower_32_bits(fb_addr), ptr);
> -		malidp_hw_write(mp->hwdev, upper_32_bits(fb_addr), ptr + 4);
> -	}
>  	malidp_de_set_plane_pitches(mp, ms->n_planes,
>  				    plane->state->fb->pitches);
>  
> @@ -381,6 +441,11 @@ static void malidp_de_plane_update(struct drm_plane *plane,
>  				LAYER_H_VAL(src_w) | LAYER_V_VAL(src_h),
>  				mp->layer->base + MALIDP550_LS_R1_IN_SIZE);
>  
> +	if (fb->modifier)
> +		malidp_de_set_plane_afbc(plane);
> +	else
> +		malidp_hw_write(mp->hwdev, 0, mp->layer->afbc_decoder_offset);

Given that you are testing for the non-zero value of fb->modifier inside
malidp_de_set_plane_afbc() function before setting the afbc_decoder
register value, I feel that you could take the 'else' branch from here
and merge it into malidp_de_set_plane_afbc() function (possibly also
rename it to malidp_de_set_plane).

With those small changes:

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu


> +
>  	/* first clear the rotation bits */
>  	val = malidp_hw_read(mp->hwdev, mp->layer->base + MALIDP_LAYER_CONTROL);
>  	val &= ~LAYER_ROT_MASK;
> diff --git a/drivers/gpu/drm/arm/malidp_regs.h b/drivers/gpu/drm/arm/malidp_regs.h
> index 149024f..54f4ec5 100644
> --- a/drivers/gpu/drm/arm/malidp_regs.h
> +++ b/drivers/gpu/drm/arm/malidp_regs.h
> @@ -180,10 +180,13 @@
>  #define MALIDP500_LV_YUV2RGB		((s16)(-0xB8))
>  #define MALIDP500_DE_LV_BASE		0x00100
>  #define MALIDP500_DE_LV_PTR_BASE	0x00124
> +#define MALIDP500_DE_LV_AD_CTRL		0x00400
>  #define MALIDP500_DE_LG1_BASE		0x00200
>  #define MALIDP500_DE_LG1_PTR_BASE	0x0021c
> +#define MALIDP500_DE_LG1_AD_CTRL	0x0040c
>  #define MALIDP500_DE_LG2_BASE		0x00300
>  #define MALIDP500_DE_LG2_PTR_BASE	0x0031c
> +#define MALIDP500_DE_LG2_AD_CTRL	0x00418
>  #define MALIDP500_SE_BASE		0x00c00
>  #define MALIDP500_SE_CONTROL		0x00c0c
>  #define MALIDP500_SE_PTR_BASE		0x00e0c
> @@ -208,10 +211,13 @@
>  #define MALIDP550_LV_YUV2RGB		0x00084
>  #define MALIDP550_DE_LV1_BASE		0x00100
>  #define MALIDP550_DE_LV1_PTR_BASE	0x00124
> +#define MALIDP550_DE_LV1_AD_CTRL	0x001B8
>  #define MALIDP550_DE_LV2_BASE		0x00200
>  #define MALIDP550_DE_LV2_PTR_BASE	0x00224
> +#define MALIDP550_DE_LV2_AD_CTRL	0x002B8
>  #define MALIDP550_DE_LG_BASE		0x00300
>  #define MALIDP550_DE_LG_PTR_BASE	0x0031c
> +#define MALIDP550_DE_LG_AD_CTRL		0x00330
>  #define MALIDP550_DE_LS_BASE		0x00400
>  #define MALIDP550_DE_LS_PTR_BASE	0x0042c
>  #define MALIDP550_DE_PERF_BASE		0x00500
> @@ -223,6 +229,20 @@
>  #define MALIDP550_CONFIG_VALID		0x0c014
>  #define MALIDP550_CONFIG_ID		0x0ffd4
>  
> +/* AFBC register offsets relative to MALIDPXXX_DE_LX_AD_CTRL */
> +/* The following register offsets are common for DP500, DP550 and DP650 */
> +#define MALIDP_AD_CROP_H                0x4
> +#define MALIDP_AD_CROP_V                0x8
> +#define MALIDP_AD_END_PTR_LOW           0xc
> +#define MALIDP_AD_END_PTR_HIGH          0x10
> +
> +/* AFBC decoder Registers */
> +#define MALIDP_AD_EN                    BIT(0)
> +#define MALIDP_AD_YTR                   BIT(4)
> +#define MALIDP_AD_BS                    BIT(8)
> +#define MALIDP_AD_CROP_RIGHT_OFFSET     16
> +#define MALIDP_AD_CROP_BOTTOM_OFFSET    16
> +
>  /*
>   * Starting with DP550 the register map blocks has been standardised to the
>   * following layout:
> -- 
> 2.7.4
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [RFC PATCH 3/4] drm/arm/malidp: Set the AFBC register bits if the framebuffer has AFBC modifier

[Ayan Halder]

On Tue, Jun 26, 2018 at 02:17:17PM +0100, Liviu Dudau wrote:
> Hi Ayan,
> 
> Thanks for the patch! I have some small comments to make:
> 
> On Fri, Jun 15, 2018 at 02:51:33PM +0100, Ayan Kumar Halder wrote:
> > Added the AFBC decoder registers for DP500 , DP550 and DP650.
> > These registers control the processing of AFBC buffers. It controls various
> > features like AFBC decoder enable, lossless transformation and block split
> > as well as setting of the left, right, top and bottom cropping of AFBC buffers
> > (in number of pixels).
> > All the layers (except DE_SMART) support framebuffers with AFBC modifiers.
> > One needs to set the pixel values of the top, left, bottom and right cropping
> > for the AFBC framebuffer.
> > Added the functionality in malidp_de_plane_update() to set the various
> > registers for AFBC decoder, depending on the modifiers.
> > 
> > Signed-off-by: Ayan Kumar halder <ayan.halder@arm.com>
> > Reviewed-by: Brian Starkey <brian.starkey@arm.com>
> > ---
> >  drivers/gpu/drm/arm/malidp_hw.c     | 27 ++++++++-----
> >  drivers/gpu/drm/arm/malidp_hw.h     |  2 +
> >  drivers/gpu/drm/arm/malidp_planes.c | 81 +++++++++++++++++++++++++++++++++----
> >  drivers/gpu/drm/arm/malidp_regs.h   | 20 +++++++++
> >  4 files changed, 111 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
> > index 4dbf39f..fd6b510 100644
> > --- a/drivers/gpu/drm/arm/malidp_hw.c
> > +++ b/drivers/gpu/drm/arm/malidp_hw.c
> > @@ -76,33 +76,38 @@ static const struct malidp_format_id malidp550_de_formats[] = {
> >  
> >  static const struct malidp_layer malidp500_layers[] = {
> >  	{ DE_VIDEO1, MALIDP500_DE_LV_BASE, MALIDP500_DE_LV_PTR_BASE,
> > -		MALIDP_DE_LV_STRIDE0, MALIDP500_LV_YUV2RGB, ROTATE_ANY },
> > +		MALIDP_DE_LV_STRIDE0, MALIDP500_LV_YUV2RGB, ROTATE_ANY,
> > +		MALIDP500_DE_LV_AD_CTRL },
> >  	{ DE_GRAPHICS1, MALIDP500_DE_LG1_BASE, MALIDP500_DE_LG1_PTR_BASE,
> > -		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY },
> > +		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY, MALIDP500_DE_LG1_AD_CTRL },
> >  	{ DE_GRAPHICS2, MALIDP500_DE_LG2_BASE, MALIDP500_DE_LG2_PTR_BASE,
> > -		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY },
> > +		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY, MALIDP500_DE_LG2_AD_CTRL },
> >  };
> >  
> >  static const struct malidp_layer malidp550_layers[] = {
> >  	{ DE_VIDEO1, MALIDP550_DE_LV1_BASE, MALIDP550_DE_LV1_PTR_BASE,
> > -		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY },
> > +		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY,
> > +		MALIDP550_DE_LV1_AD_CTRL },
> >  	{ DE_GRAPHICS1, MALIDP550_DE_LG_BASE, MALIDP550_DE_LG_PTR_BASE,
> > -		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY },
> > +		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY, MALIDP550_DE_LG_AD_CTRL },
> >  	{ DE_VIDEO2, MALIDP550_DE_LV2_BASE, MALIDP550_DE_LV2_PTR_BASE,
> > -		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY },
> > +		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY,
> > +		MALIDP550_DE_LV2_AD_CTRL },
> >  	{ DE_SMART, MALIDP550_DE_LS_BASE, MALIDP550_DE_LS_PTR_BASE,
> > -		MALIDP550_DE_LS_R1_STRIDE, 0, ROTATE_NONE },
> > +		MALIDP550_DE_LS_R1_STRIDE, 0, ROTATE_NONE, 0 },
> >  };
> >  
> >  static const struct malidp_layer malidp650_layers[] = {
> >  	{ DE_VIDEO1, MALIDP550_DE_LV1_BASE, MALIDP550_DE_LV1_PTR_BASE,
> > -		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY },
> > +		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY,
> > +		MALIDP550_DE_LV1_AD_CTRL },
> >  	{ DE_GRAPHICS1, MALIDP550_DE_LG_BASE, MALIDP550_DE_LG_PTR_BASE,
> > -		MALIDP_DE_LG_STRIDE, 0, ROTATE_COMPRESSED },
> > +		MALIDP_DE_LG_STRIDE, 0, ROTATE_COMPRESSED, MALIDP550_DE_LG_AD_CTRL },
> >  	{ DE_VIDEO2, MALIDP550_DE_LV2_BASE, MALIDP550_DE_LV2_PTR_BASE,
> > -		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY },
> > +		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY,
> > +		MALIDP550_DE_LV2_AD_CTRL },
> >  	{ DE_SMART, MALIDP550_DE_LS_BASE, MALIDP550_DE_LS_PTR_BASE,
> > -		MALIDP550_DE_LS_R1_STRIDE, 0, ROTATE_NONE },
> > +		MALIDP550_DE_LS_R1_STRIDE, 0, ROTATE_NONE, 0 },
> >  };
> >  
> >  #define SE_N_SCALING_COEFFS	96
> > diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
> > index 4390243..bbe6883 100644
> > --- a/drivers/gpu/drm/arm/malidp_hw.h
> > +++ b/drivers/gpu/drm/arm/malidp_hw.h
> > @@ -67,6 +67,8 @@ struct malidp_layer {
> >  	u16 stride_offset;	/* offset to the first stride register. */
> >  	s16 yuv2rgb_offset;	/* offset to the YUV->RGB matrix entries */
> >  	enum rotation_features rot;    /* type of rotation supported */
> > +	/* address offset for the AFBC decoder registers */
> > +	u16 afbc_decoder_offset;
> >  };
> >  
> >  enum malidp_scaling_coeff_set {
> > diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
> > index 533cdde..3950504 100644
> > --- a/drivers/gpu/drm/arm/malidp_planes.c
> > +++ b/drivers/gpu/drm/arm/malidp_planes.c
> > @@ -330,6 +330,71 @@ static void malidp_de_set_color_encoding(struct malidp_plane *plane,
> >  	}
> >  }
> >  
> > +static void malidp_set_plane_base_addr(struct drm_framebuffer *fb,
> > +				       struct malidp_plane *mp,
> > +				       int plane_index)
> > +{
> > +	dma_addr_t paddr;
> > +	u16 ptr;
> > +	struct drm_plane *plane = &mp->base;
> > +	bool afbc = fb->modifier ? true : false;
> 
> The decision to set afbc based on wether the fb has a modifier or not
> seems a bit weak. Should we also (at least) check that the modifier is
> an ARM one?

In my next patch (of this series) ie
https://patchwork.kernel.org/patch/10466537/, we have checked for the
validity of modifier and format in malidp_format_mod_supported(). When
drm_atomic_commit() is invoked, it first calls (indirectly via some
intermediate functions)
malidp_format_mod_supported() and then invokes
malidp_set_plane_base_addr(). So there is no way that an invalid
modifier could be passed to malidp_set_plane_base_addr(). If
malidp_format_mod_supported() returns error then drm_atomic_commit()
would return error too.
> 
> > +
> > +	ptr = mp->layer->ptr + (plane_index << 4);
> > +
> > +	/*
> > +	 * For AFBC buffers, cropping is handled by AFBC decoder rather than
> > +	 * pointer manipulation.
> > +	 */
> > +	if (!afbc) {
> > +		paddr = drm_fb_cma_get_gem_addr(fb, plane->state,
> > +						plane_index);
> > +	} else {
> > +		struct drm_gem_cma_object *obj;
> > +
> > +		obj = drm_fb_cma_get_gem_obj(fb, plane_index);
> > +
> > +		if (WARN_ON(!obj))
> > +			return;
> > +		paddr = obj->paddr;
> > +	}
> > +
> > +	malidp_hw_write(mp->hwdev, lower_32_bits(paddr), ptr);
> > +	malidp_hw_write(mp->hwdev, upper_32_bits(paddr), ptr + 4);
> > +}
> > +
> > +static void malidp_de_set_plane_afbc(struct drm_plane *plane)
> > +{
> > +	struct malidp_plane *mp;
> > +	u32 src_w, src_h, val = 0, src_x, src_y;
> > +	struct drm_framebuffer *fb = plane->state->fb;
> > +
> > +	mp = to_malidp_plane(plane);
> > +
> > +	/* convert src values from Q16 fixed point to integer */
> > +	src_w = plane->state->src_w >> 16;
> > +	src_h = plane->state->src_h >> 16;
> > +	src_x = plane->state->src_x >> 16;
> > +	src_y = plane->state->src_y >> 16;
> > +
> > +	val = ((fb->width - (src_x + src_w)) << MALIDP_AD_CROP_RIGHT_OFFSET) |
> > +		   src_x;
> > +	malidp_hw_write(mp->hwdev, val,
> > +			mp->layer->afbc_decoder_offset + MALIDP_AD_CROP_H);
> > +
> > +	val = ((fb->height - (src_y + src_h)) << MALIDP_AD_CROP_BOTTOM_OFFSET) |
> > +		   src_y;
> > +	malidp_hw_write(mp->hwdev, val,
> > +			mp->layer->afbc_decoder_offset + MALIDP_AD_CROP_V);
> > +
> > +	val = MALIDP_AD_EN;
> > +	if (fb->modifier & AFBC_FORMAT_MOD_SPLIT)
> > +		val |= MALIDP_AD_BS;
> > +	if (fb->modifier & AFBC_FORMAT_MOD_YTR)
> > +		val |= MALIDP_AD_YTR;
> > +
> > +	malidp_hw_write(mp->hwdev, val, mp->layer->afbc_decoder_offset);
> > +}
> > +
> >  static void malidp_de_plane_update(struct drm_plane *plane,
> >  				   struct drm_plane_state *old_state)
> >  {
> > @@ -338,6 +403,7 @@ static void malidp_de_plane_update(struct drm_plane *plane,
> >  	u32 src_w, src_h, dest_w, dest_h, val;
> >  	int i;
> >  	bool format_has_alpha = plane->state->fb->format->has_alpha;
> > +	struct drm_framebuffer *fb = plane->state->fb;
> >  
> >  	mp = to_malidp_plane(plane);
> >  
> > @@ -349,15 +415,9 @@ static void malidp_de_plane_update(struct drm_plane *plane,
> >  
> >  	malidp_hw_write(mp->hwdev, ms->format, mp->layer->base);
> >  
> > -	for (i = 0; i < ms->n_planes; i++) {
> > -		/* calculate the offset for the layer's plane registers */
> > -		u16 ptr = mp->layer->ptr + (i << 4);
> > -		dma_addr_t fb_addr = drm_fb_cma_get_gem_addr(plane->state->fb,
> > -							     plane->state, i);
> > +	for (i = 0; i < ms->n_planes; i++)
> > +		malidp_set_plane_base_addr(fb, mp, i);
> >  
> > -		malidp_hw_write(mp->hwdev, lower_32_bits(fb_addr), ptr);
> > -		malidp_hw_write(mp->hwdev, upper_32_bits(fb_addr), ptr + 4);
> > -	}
> >  	malidp_de_set_plane_pitches(mp, ms->n_planes,
> >  				    plane->state->fb->pitches);
> >  
> > @@ -381,6 +441,11 @@ static void malidp_de_plane_update(struct drm_plane *plane,
> >  				LAYER_H_VAL(src_w) | LAYER_V_VAL(src_h),
> >  				mp->layer->base + MALIDP550_LS_R1_IN_SIZE);
> >  
> > +	if (fb->modifier)
> > +		malidp_de_set_plane_afbc(plane);
> > +	else
> > +		malidp_hw_write(mp->hwdev, 0, mp->layer->afbc_decoder_offset);
> 
> Given that you are testing for the non-zero value of fb->modifier inside
> malidp_de_set_plane_afbc() function before setting the afbc_decoder
> register value, I feel that you could take the 'else' branch from here
> and merge it into malidp_de_set_plane_afbc() function (possibly also
> rename it to malidp_de_set_plane).

I am not exactly sure what you meant here. We are testing for the non
zero value of fb->modifier in malidp_de_plane_update(). If
fb->modifier is not zero, we set the afbc specific registers in
malidp_de_set_plane_afbc(). Thus, malidp_de_plane_update()  will
invoke malidp_de_set_plane_afbc() for setting afbc stuff and will
continue with the other register settings. I do not see a clear reason
to refactor this code.

> With those small changes:
> 
> Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> 
> Best regards,
> Liviu
> 
> 
> > +
> >  	/* first clear the rotation bits */
> >  	val = malidp_hw_read(mp->hwdev, mp->layer->base + MALIDP_LAYER_CONTROL);
> >  	val &= ~LAYER_ROT_MASK;
> > diff --git a/drivers/gpu/drm/arm/malidp_regs.h b/drivers/gpu/drm/arm/malidp_regs.h
> > index 149024f..54f4ec5 100644
> > --- a/drivers/gpu/drm/arm/malidp_regs.h
> > +++ b/drivers/gpu/drm/arm/malidp_regs.h
> > @@ -180,10 +180,13 @@
> >  #define MALIDP500_LV_YUV2RGB		((s16)(-0xB8))
> >  #define MALIDP500_DE_LV_BASE		0x00100
> >  #define MALIDP500_DE_LV_PTR_BASE	0x00124
> > +#define MALIDP500_DE_LV_AD_CTRL		0x00400
> >  #define MALIDP500_DE_LG1_BASE		0x00200
> >  #define MALIDP500_DE_LG1_PTR_BASE	0x0021c
> > +#define MALIDP500_DE_LG1_AD_CTRL	0x0040c
> >  #define MALIDP500_DE_LG2_BASE		0x00300
> >  #define MALIDP500_DE_LG2_PTR_BASE	0x0031c
> > +#define MALIDP500_DE_LG2_AD_CTRL	0x00418
> >  #define MALIDP500_SE_BASE		0x00c00
> >  #define MALIDP500_SE_CONTROL		0x00c0c
> >  #define MALIDP500_SE_PTR_BASE		0x00e0c
> > @@ -208,10 +211,13 @@
> >  #define MALIDP550_LV_YUV2RGB		0x00084
> >  #define MALIDP550_DE_LV1_BASE		0x00100
> >  #define MALIDP550_DE_LV1_PTR_BASE	0x00124
> > +#define MALIDP550_DE_LV1_AD_CTRL	0x001B8
> >  #define MALIDP550_DE_LV2_BASE		0x00200
> >  #define MALIDP550_DE_LV2_PTR_BASE	0x00224
> > +#define MALIDP550_DE_LV2_AD_CTRL	0x002B8
> >  #define MALIDP550_DE_LG_BASE		0x00300
> >  #define MALIDP550_DE_LG_PTR_BASE	0x0031c
> > +#define MALIDP550_DE_LG_AD_CTRL		0x00330
> >  #define MALIDP550_DE_LS_BASE		0x00400
> >  #define MALIDP550_DE_LS_PTR_BASE	0x0042c
> >  #define MALIDP550_DE_PERF_BASE		0x00500
> > @@ -223,6 +229,20 @@
> >  #define MALIDP550_CONFIG_VALID		0x0c014
> >  #define MALIDP550_CONFIG_ID		0x0ffd4
> >  
> > +/* AFBC register offsets relative to MALIDPXXX_DE_LX_AD_CTRL */
> > +/* The following register offsets are common for DP500, DP550 and DP650 */
> > +#define MALIDP_AD_CROP_H                0x4
> > +#define MALIDP_AD_CROP_V                0x8
> > +#define MALIDP_AD_END_PTR_LOW           0xc
> > +#define MALIDP_AD_END_PTR_HIGH          0x10
> > +
> > +/* AFBC decoder Registers */
> > +#define MALIDP_AD_EN                    BIT(0)
> > +#define MALIDP_AD_YTR                   BIT(4)
> > +#define MALIDP_AD_BS                    BIT(8)
> > +#define MALIDP_AD_CROP_RIGHT_OFFSET     16
> > +#define MALIDP_AD_CROP_BOTTOM_OFFSET    16
> > +
> >  /*
> >   * Starting with DP550 the register map blocks has been standardised to the
> >   * following layout:
> > -- 
> > 2.7.4
> > 
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ??\_(???)_/??
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 3/4] drm/arm/malidp: Set the AFBC register bits if the framebuffer has AFBC modifier

[Liviu Dudau]

On Thu, Jul 05, 2018 at 02:31:47PM +0100, Ayan Halder wrote:
> On Tue, Jun 26, 2018 at 02:17:17PM +0100, Liviu Dudau wrote:
> > Hi Ayan,
> > 
> > Thanks for the patch! I have some small comments to make:
> > 
> > On Fri, Jun 15, 2018 at 02:51:33PM +0100, Ayan Kumar Halder wrote:
> > > Added the AFBC decoder registers for DP500 , DP550 and DP650.
> > > These registers control the processing of AFBC buffers. It controls various
> > > features like AFBC decoder enable, lossless transformation and block split
> > > as well as setting of the left, right, top and bottom cropping of AFBC buffers
> > > (in number of pixels).
> > > All the layers (except DE_SMART) support framebuffers with AFBC modifiers.
> > > One needs to set the pixel values of the top, left, bottom and right cropping
> > > for the AFBC framebuffer.
> > > Added the functionality in malidp_de_plane_update() to set the various
> > > registers for AFBC decoder, depending on the modifiers.
> > > 
> > > Signed-off-by: Ayan Kumar halder <ayan.halder@arm.com>
> > > Reviewed-by: Brian Starkey <brian.starkey@arm.com>
> > > ---
> > >  drivers/gpu/drm/arm/malidp_hw.c     | 27 ++++++++-----
> > >  drivers/gpu/drm/arm/malidp_hw.h     |  2 +
> > >  drivers/gpu/drm/arm/malidp_planes.c | 81 +++++++++++++++++++++++++++++++++----
> > >  drivers/gpu/drm/arm/malidp_regs.h   | 20 +++++++++
> > >  4 files changed, 111 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
> > > index 4dbf39f..fd6b510 100644
> > > --- a/drivers/gpu/drm/arm/malidp_hw.c
> > > +++ b/drivers/gpu/drm/arm/malidp_hw.c
> > > @@ -76,33 +76,38 @@ static const struct malidp_format_id malidp550_de_formats[] = {
> > >  
> > >  static const struct malidp_layer malidp500_layers[] = {
> > >  	{ DE_VIDEO1, MALIDP500_DE_LV_BASE, MALIDP500_DE_LV_PTR_BASE,
> > > -		MALIDP_DE_LV_STRIDE0, MALIDP500_LV_YUV2RGB, ROTATE_ANY },
> > > +		MALIDP_DE_LV_STRIDE0, MALIDP500_LV_YUV2RGB, ROTATE_ANY,
> > > +		MALIDP500_DE_LV_AD_CTRL },
> > >  	{ DE_GRAPHICS1, MALIDP500_DE_LG1_BASE, MALIDP500_DE_LG1_PTR_BASE,
> > > -		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY },
> > > +		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY, MALIDP500_DE_LG1_AD_CTRL },
> > >  	{ DE_GRAPHICS2, MALIDP500_DE_LG2_BASE, MALIDP500_DE_LG2_PTR_BASE,
> > > -		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY },
> > > +		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY, MALIDP500_DE_LG2_AD_CTRL },
> > >  };
> > >  
> > >  static const struct malidp_layer malidp550_layers[] = {
> > >  	{ DE_VIDEO1, MALIDP550_DE_LV1_BASE, MALIDP550_DE_LV1_PTR_BASE,
> > > -		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY },
> > > +		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY,
> > > +		MALIDP550_DE_LV1_AD_CTRL },
> > >  	{ DE_GRAPHICS1, MALIDP550_DE_LG_BASE, MALIDP550_DE_LG_PTR_BASE,
> > > -		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY },
> > > +		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY, MALIDP550_DE_LG_AD_CTRL },
> > >  	{ DE_VIDEO2, MALIDP550_DE_LV2_BASE, MALIDP550_DE_LV2_PTR_BASE,
> > > -		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY },
> > > +		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY,
> > > +		MALIDP550_DE_LV2_AD_CTRL },
> > >  	{ DE_SMART, MALIDP550_DE_LS_BASE, MALIDP550_DE_LS_PTR_BASE,
> > > -		MALIDP550_DE_LS_R1_STRIDE, 0, ROTATE_NONE },
> > > +		MALIDP550_DE_LS_R1_STRIDE, 0, ROTATE_NONE, 0 },
> > >  };
> > >  
> > >  static const struct malidp_layer malidp650_layers[] = {
> > >  	{ DE_VIDEO1, MALIDP550_DE_LV1_BASE, MALIDP550_DE_LV1_PTR_BASE,
> > > -		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY },
> > > +		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY,
> > > +		MALIDP550_DE_LV1_AD_CTRL },
> > >  	{ DE_GRAPHICS1, MALIDP550_DE_LG_BASE, MALIDP550_DE_LG_PTR_BASE,
> > > -		MALIDP_DE_LG_STRIDE, 0, ROTATE_COMPRESSED },
> > > +		MALIDP_DE_LG_STRIDE, 0, ROTATE_COMPRESSED, MALIDP550_DE_LG_AD_CTRL },
> > >  	{ DE_VIDEO2, MALIDP550_DE_LV2_BASE, MALIDP550_DE_LV2_PTR_BASE,
> > > -		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY },
> > > +		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY,
> > > +		MALIDP550_DE_LV2_AD_CTRL },
> > >  	{ DE_SMART, MALIDP550_DE_LS_BASE, MALIDP550_DE_LS_PTR_BASE,
> > > -		MALIDP550_DE_LS_R1_STRIDE, 0, ROTATE_NONE },
> > > +		MALIDP550_DE_LS_R1_STRIDE, 0, ROTATE_NONE, 0 },
> > >  };
> > >  
> > >  #define SE_N_SCALING_COEFFS	96
> > > diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
> > > index 4390243..bbe6883 100644
> > > --- a/drivers/gpu/drm/arm/malidp_hw.h
> > > +++ b/drivers/gpu/drm/arm/malidp_hw.h
> > > @@ -67,6 +67,8 @@ struct malidp_layer {
> > >  	u16 stride_offset;	/* offset to the first stride register. */
> > >  	s16 yuv2rgb_offset;	/* offset to the YUV->RGB matrix entries */
> > >  	enum rotation_features rot;    /* type of rotation supported */
> > > +	/* address offset for the AFBC decoder registers */
> > > +	u16 afbc_decoder_offset;
> > >  };
> > >  
> > >  enum malidp_scaling_coeff_set {
> > > diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
> > > index 533cdde..3950504 100644
> > > --- a/drivers/gpu/drm/arm/malidp_planes.c
> > > +++ b/drivers/gpu/drm/arm/malidp_planes.c
> > > @@ -330,6 +330,71 @@ static void malidp_de_set_color_encoding(struct malidp_plane *plane,
> > >  	}
> > >  }
> > >  
> > > +static void malidp_set_plane_base_addr(struct drm_framebuffer *fb,
> > > +				       struct malidp_plane *mp,
> > > +				       int plane_index)
> > > +{
> > > +	dma_addr_t paddr;
> > > +	u16 ptr;
> > > +	struct drm_plane *plane = &mp->base;
> > > +	bool afbc = fb->modifier ? true : false;
> > 
> > The decision to set afbc based on wether the fb has a modifier or not
> > seems a bit weak. Should we also (at least) check that the modifier is
> > an ARM one?
> 
> In my next patch (of this series) ie
> https://patchwork.kernel.org/patch/10466537/, we have checked for the
> validity of modifier and format in malidp_format_mod_supported(). When
> drm_atomic_commit() is invoked, it first calls (indirectly via some
> intermediate functions)
> malidp_format_mod_supported() and then invokes
> malidp_set_plane_base_addr(). So there is no way that an invalid
> modifier could be passed to malidp_set_plane_base_addr(). If
> malidp_format_mod_supported() returns error then drm_atomic_commit()
> would return error too.
> > 
> > > +
> > > +	ptr = mp->layer->ptr + (plane_index << 4);
> > > +
> > > +	/*
> > > +	 * For AFBC buffers, cropping is handled by AFBC decoder rather than
> > > +	 * pointer manipulation.
> > > +	 */
> > > +	if (!afbc) {
> > > +		paddr = drm_fb_cma_get_gem_addr(fb, plane->state,
> > > +						plane_index);
> > > +	} else {
> > > +		struct drm_gem_cma_object *obj;
> > > +
> > > +		obj = drm_fb_cma_get_gem_obj(fb, plane_index);
> > > +
> > > +		if (WARN_ON(!obj))
> > > +			return;
> > > +		paddr = obj->paddr;
> > > +	}
> > > +
> > > +	malidp_hw_write(mp->hwdev, lower_32_bits(paddr), ptr);
> > > +	malidp_hw_write(mp->hwdev, upper_32_bits(paddr), ptr + 4);
> > > +}
> > > +
> > > +static void malidp_de_set_plane_afbc(struct drm_plane *plane)
> > > +{
> > > +	struct malidp_plane *mp;
> > > +	u32 src_w, src_h, val = 0, src_x, src_y;
> > > +	struct drm_framebuffer *fb = plane->state->fb;
> > > +
> > > +	mp = to_malidp_plane(plane);
> > > +
> > > +	/* convert src values from Q16 fixed point to integer */
> > > +	src_w = plane->state->src_w >> 16;
> > > +	src_h = plane->state->src_h >> 16;
> > > +	src_x = plane->state->src_x >> 16;
> > > +	src_y = plane->state->src_y >> 16;
> > > +
> > > +	val = ((fb->width - (src_x + src_w)) << MALIDP_AD_CROP_RIGHT_OFFSET) |
> > > +		   src_x;
> > > +	malidp_hw_write(mp->hwdev, val,
> > > +			mp->layer->afbc_decoder_offset + MALIDP_AD_CROP_H);
> > > +
> > > +	val = ((fb->height - (src_y + src_h)) << MALIDP_AD_CROP_BOTTOM_OFFSET) |
> > > +		   src_y;
> > > +	malidp_hw_write(mp->hwdev, val,
> > > +			mp->layer->afbc_decoder_offset + MALIDP_AD_CROP_V);
> > > +
> > > +	val = MALIDP_AD_EN;
> > > +	if (fb->modifier & AFBC_FORMAT_MOD_SPLIT)
> > > +		val |= MALIDP_AD_BS;
> > > +	if (fb->modifier & AFBC_FORMAT_MOD_YTR)
> > > +		val |= MALIDP_AD_YTR;
> > > +
> > > +	malidp_hw_write(mp->hwdev, val, mp->layer->afbc_decoder_offset);
> > > +}
> > > +
> > >  static void malidp_de_plane_update(struct drm_plane *plane,
> > >  				   struct drm_plane_state *old_state)
> > >  {
> > > @@ -338,6 +403,7 @@ static void malidp_de_plane_update(struct drm_plane *plane,
> > >  	u32 src_w, src_h, dest_w, dest_h, val;
> > >  	int i;
> > >  	bool format_has_alpha = plane->state->fb->format->has_alpha;
> > > +	struct drm_framebuffer *fb = plane->state->fb;
> > >  
> > >  	mp = to_malidp_plane(plane);
> > >  
> > > @@ -349,15 +415,9 @@ static void malidp_de_plane_update(struct drm_plane *plane,
> > >  
> > >  	malidp_hw_write(mp->hwdev, ms->format, mp->layer->base);
> > >  
> > > -	for (i = 0; i < ms->n_planes; i++) {
> > > -		/* calculate the offset for the layer's plane registers */
> > > -		u16 ptr = mp->layer->ptr + (i << 4);
> > > -		dma_addr_t fb_addr = drm_fb_cma_get_gem_addr(plane->state->fb,
> > > -							     plane->state, i);
> > > +	for (i = 0; i < ms->n_planes; i++)
> > > +		malidp_set_plane_base_addr(fb, mp, i);
> > >  
> > > -		malidp_hw_write(mp->hwdev, lower_32_bits(fb_addr), ptr);
> > > -		malidp_hw_write(mp->hwdev, upper_32_bits(fb_addr), ptr + 4);
> > > -	}
> > >  	malidp_de_set_plane_pitches(mp, ms->n_planes,
> > >  				    plane->state->fb->pitches);
> > >  
> > > @@ -381,6 +441,11 @@ static void malidp_de_plane_update(struct drm_plane *plane,
> > >  				LAYER_H_VAL(src_w) | LAYER_V_VAL(src_h),
> > >  				mp->layer->base + MALIDP550_LS_R1_IN_SIZE);
> > >  
> > > +	if (fb->modifier)
> > > +		malidp_de_set_plane_afbc(plane);
> > > +	else
> > > +		malidp_hw_write(mp->hwdev, 0, mp->layer->afbc_decoder_offset);
> > 
> > Given that you are testing for the non-zero value of fb->modifier inside
> > malidp_de_set_plane_afbc() function before setting the afbc_decoder
> > register value, I feel that you could take the 'else' branch from here
> > and merge it into malidp_de_set_plane_afbc() function (possibly also
> > rename it to malidp_de_set_plane).
> 
> I am not exactly sure what you meant here. We are testing for the non
> zero value of fb->modifier in malidp_de_plane_update(). If
> fb->modifier is not zero, we set the afbc specific registers in
> malidp_de_set_plane_afbc(). Thus, malidp_de_plane_update()  will
> invoke malidp_de_set_plane_afbc() for setting afbc stuff and will
> continue with the other register settings. I do not see a clear reason
> to refactor this code.

What I'm suggesting is:

 - rename malidp_de_set_plane_afbc to malidp_de_set_plane()
 - remove the if .. else malidp_hw_write(mp->hwdev, 0, mp->layer->afbc_decoder_offset);
   and keep just the call to malidp_de_set_plane() without any other
   condition
 - inside malidp_de_set_plane(), change from:
	val = MALIDP_AD_EN;
   to
	val = fb->modifier ? MALIDP_AD_EN : 0;

   and then the 0 value will be written correctly into the
   afbc_decoder_offset if fb->modifier is zero.

Best regards,
Liviu

> 
> > With those small changes:
> > 
> > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> > 
> > Best regards,
> > Liviu
> > 
> > 
> > > +
> > >  	/* first clear the rotation bits */
> > >  	val = malidp_hw_read(mp->hwdev, mp->layer->base + MALIDP_LAYER_CONTROL);
> > >  	val &= ~LAYER_ROT_MASK;
> > > diff --git a/drivers/gpu/drm/arm/malidp_regs.h b/drivers/gpu/drm/arm/malidp_regs.h
> > > index 149024f..54f4ec5 100644
> > > --- a/drivers/gpu/drm/arm/malidp_regs.h
> > > +++ b/drivers/gpu/drm/arm/malidp_regs.h
> > > @@ -180,10 +180,13 @@
> > >  #define MALIDP500_LV_YUV2RGB		((s16)(-0xB8))
> > >  #define MALIDP500_DE_LV_BASE		0x00100
> > >  #define MALIDP500_DE_LV_PTR_BASE	0x00124
> > > +#define MALIDP500_DE_LV_AD_CTRL		0x00400
> > >  #define MALIDP500_DE_LG1_BASE		0x00200
> > >  #define MALIDP500_DE_LG1_PTR_BASE	0x0021c
> > > +#define MALIDP500_DE_LG1_AD_CTRL	0x0040c
> > >  #define MALIDP500_DE_LG2_BASE		0x00300
> > >  #define MALIDP500_DE_LG2_PTR_BASE	0x0031c
> > > +#define MALIDP500_DE_LG2_AD_CTRL	0x00418
> > >  #define MALIDP500_SE_BASE		0x00c00
> > >  #define MALIDP500_SE_CONTROL		0x00c0c
> > >  #define MALIDP500_SE_PTR_BASE		0x00e0c
> > > @@ -208,10 +211,13 @@
> > >  #define MALIDP550_LV_YUV2RGB		0x00084
> > >  #define MALIDP550_DE_LV1_BASE		0x00100
> > >  #define MALIDP550_DE_LV1_PTR_BASE	0x00124
> > > +#define MALIDP550_DE_LV1_AD_CTRL	0x001B8
> > >  #define MALIDP550_DE_LV2_BASE		0x00200
> > >  #define MALIDP550_DE_LV2_PTR_BASE	0x00224
> > > +#define MALIDP550_DE_LV2_AD_CTRL	0x002B8
> > >  #define MALIDP550_DE_LG_BASE		0x00300
> > >  #define MALIDP550_DE_LG_PTR_BASE	0x0031c
> > > +#define MALIDP550_DE_LG_AD_CTRL		0x00330
> > >  #define MALIDP550_DE_LS_BASE		0x00400
> > >  #define MALIDP550_DE_LS_PTR_BASE	0x0042c
> > >  #define MALIDP550_DE_PERF_BASE		0x00500
> > > @@ -223,6 +229,20 @@
> > >  #define MALIDP550_CONFIG_VALID		0x0c014
> > >  #define MALIDP550_CONFIG_ID		0x0ffd4
> > >  
> > > +/* AFBC register offsets relative to MALIDPXXX_DE_LX_AD_CTRL */
> > > +/* The following register offsets are common for DP500, DP550 and DP650 */
> > > +#define MALIDP_AD_CROP_H                0x4
> > > +#define MALIDP_AD_CROP_V                0x8
> > > +#define MALIDP_AD_END_PTR_LOW           0xc
> > > +#define MALIDP_AD_END_PTR_HIGH          0x10
> > > +
> > > +/* AFBC decoder Registers */
> > > +#define MALIDP_AD_EN                    BIT(0)
> > > +#define MALIDP_AD_YTR                   BIT(4)
> > > +#define MALIDP_AD_BS                    BIT(8)
> > > +#define MALIDP_AD_CROP_RIGHT_OFFSET     16
> > > +#define MALIDP_AD_CROP_BOTTOM_OFFSET    16
> > > +
> > >  /*
> > >   * Starting with DP550 the register map blocks has been standardised to the
> > >   * following layout:
> > > -- 
> > > 2.7.4
> > > 
> > 
> > -- 
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ??\_(???)_/??
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 3/4] drm/arm/malidp: Set the AFBC register bits if the framebuffer has AFBC modifier

[Ayan Halder]

Hi Liviu,
On Thu, Jul 05, 2018 at 06:38:21PM +0100, Liviu Dudau wrote:
> On Thu, Jul 05, 2018 at 02:31:47PM +0100, Ayan Halder wrote:
> > On Tue, Jun 26, 2018 at 02:17:17PM +0100, Liviu Dudau wrote:
> > > Hi Ayan,
> > > 
> > > Thanks for the patch! I have some small comments to make:
> > > 
> > > On Fri, Jun 15, 2018 at 02:51:33PM +0100, Ayan Kumar Halder wrote:
> > > > Added the AFBC decoder registers for DP500 , DP550 and DP650.
> > > > These registers control the processing of AFBC buffers. It controls various
> > > > features like AFBC decoder enable, lossless transformation and block split
> > > > as well as setting of the left, right, top and bottom cropping of AFBC buffers
> > > > (in number of pixels).
> > > > All the layers (except DE_SMART) support framebuffers with AFBC modifiers.
> > > > One needs to set the pixel values of the top, left, bottom and right cropping
> > > > for the AFBC framebuffer.
> > > > Added the functionality in malidp_de_plane_update() to set the various
> > > > registers for AFBC decoder, depending on the modifiers.
> > > > 
> > > > Signed-off-by: Ayan Kumar halder <ayan.halder@arm.com>
> > > > Reviewed-by: Brian Starkey <brian.starkey@arm.com>
> > > > ---
> > > >  drivers/gpu/drm/arm/malidp_hw.c     | 27 ++++++++-----
> > > >  drivers/gpu/drm/arm/malidp_hw.h     |  2 +
> > > >  drivers/gpu/drm/arm/malidp_planes.c | 81 +++++++++++++++++++++++++++++++++----
> > > >  drivers/gpu/drm/arm/malidp_regs.h   | 20 +++++++++
> > > >  4 files changed, 111 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
> > > > index 4dbf39f..fd6b510 100644
> > > > --- a/drivers/gpu/drm/arm/malidp_hw.c
> > > > +++ b/drivers/gpu/drm/arm/malidp_hw.c
> > > > @@ -76,33 +76,38 @@ static const struct malidp_format_id malidp550_de_formats[] = {
> > > >  
> > > >  static const struct malidp_layer malidp500_layers[] = {
> > > >  	{ DE_VIDEO1, MALIDP500_DE_LV_BASE, MALIDP500_DE_LV_PTR_BASE,
> > > > -		MALIDP_DE_LV_STRIDE0, MALIDP500_LV_YUV2RGB, ROTATE_ANY },
> > > > +		MALIDP_DE_LV_STRIDE0, MALIDP500_LV_YUV2RGB, ROTATE_ANY,
> > > > +		MALIDP500_DE_LV_AD_CTRL },
> > > >  	{ DE_GRAPHICS1, MALIDP500_DE_LG1_BASE, MALIDP500_DE_LG1_PTR_BASE,
> > > > -		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY },
> > > > +		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY, MALIDP500_DE_LG1_AD_CTRL },
> > > >  	{ DE_GRAPHICS2, MALIDP500_DE_LG2_BASE, MALIDP500_DE_LG2_PTR_BASE,
> > > > -		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY },
> > > > +		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY, MALIDP500_DE_LG2_AD_CTRL },
> > > >  };
> > > >  
> > > >  static const struct malidp_layer malidp550_layers[] = {
> > > >  	{ DE_VIDEO1, MALIDP550_DE_LV1_BASE, MALIDP550_DE_LV1_PTR_BASE,
> > > > -		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY },
> > > > +		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY,
> > > > +		MALIDP550_DE_LV1_AD_CTRL },
> > > >  	{ DE_GRAPHICS1, MALIDP550_DE_LG_BASE, MALIDP550_DE_LG_PTR_BASE,
> > > > -		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY },
> > > > +		MALIDP_DE_LG_STRIDE, 0, ROTATE_ANY, MALIDP550_DE_LG_AD_CTRL },
> > > >  	{ DE_VIDEO2, MALIDP550_DE_LV2_BASE, MALIDP550_DE_LV2_PTR_BASE,
> > > > -		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY },
> > > > +		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY,
> > > > +		MALIDP550_DE_LV2_AD_CTRL },
> > > >  	{ DE_SMART, MALIDP550_DE_LS_BASE, MALIDP550_DE_LS_PTR_BASE,
> > > > -		MALIDP550_DE_LS_R1_STRIDE, 0, ROTATE_NONE },
> > > > +		MALIDP550_DE_LS_R1_STRIDE, 0, ROTATE_NONE, 0 },
> > > >  };
> > > >  
> > > >  static const struct malidp_layer malidp650_layers[] = {
> > > >  	{ DE_VIDEO1, MALIDP550_DE_LV1_BASE, MALIDP550_DE_LV1_PTR_BASE,
> > > > -		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY },
> > > > +		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY,
> > > > +		MALIDP550_DE_LV1_AD_CTRL },
> > > >  	{ DE_GRAPHICS1, MALIDP550_DE_LG_BASE, MALIDP550_DE_LG_PTR_BASE,
> > > > -		MALIDP_DE_LG_STRIDE, 0, ROTATE_COMPRESSED },
> > > > +		MALIDP_DE_LG_STRIDE, 0, ROTATE_COMPRESSED, MALIDP550_DE_LG_AD_CTRL },
> > > >  	{ DE_VIDEO2, MALIDP550_DE_LV2_BASE, MALIDP550_DE_LV2_PTR_BASE,
> > > > -		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY },
> > > > +		MALIDP_DE_LV_STRIDE0, MALIDP550_LV_YUV2RGB, ROTATE_ANY,
> > > > +		MALIDP550_DE_LV2_AD_CTRL },
> > > >  	{ DE_SMART, MALIDP550_DE_LS_BASE, MALIDP550_DE_LS_PTR_BASE,
> > > > -		MALIDP550_DE_LS_R1_STRIDE, 0, ROTATE_NONE },
> > > > +		MALIDP550_DE_LS_R1_STRIDE, 0, ROTATE_NONE, 0 },
> > > >  };
> > > >  
> > > >  #define SE_N_SCALING_COEFFS	96
> > > > diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
> > > > index 4390243..bbe6883 100644
> > > > --- a/drivers/gpu/drm/arm/malidp_hw.h
> > > > +++ b/drivers/gpu/drm/arm/malidp_hw.h
> > > > @@ -67,6 +67,8 @@ struct malidp_layer {
> > > >  	u16 stride_offset;	/* offset to the first stride register. */
> > > >  	s16 yuv2rgb_offset;	/* offset to the YUV->RGB matrix entries */
> > > >  	enum rotation_features rot;    /* type of rotation supported */
> > > > +	/* address offset for the AFBC decoder registers */
> > > > +	u16 afbc_decoder_offset;
> > > >  };
> > > >  
> > > >  enum malidp_scaling_coeff_set {
> > > > diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
> > > > index 533cdde..3950504 100644
> > > > --- a/drivers/gpu/drm/arm/malidp_planes.c
> > > > +++ b/drivers/gpu/drm/arm/malidp_planes.c
> > > > @@ -330,6 +330,71 @@ static void malidp_de_set_color_encoding(struct malidp_plane *plane,
> > > >  	}
> > > >  }
> > > >  
> > > > +static void malidp_set_plane_base_addr(struct drm_framebuffer *fb,
> > > > +				       struct malidp_plane *mp,
> > > > +				       int plane_index)
> > > > +{
> > > > +	dma_addr_t paddr;
> > > > +	u16 ptr;
> > > > +	struct drm_plane *plane = &mp->base;
> > > > +	bool afbc = fb->modifier ? true : false;
> > > 
> > > The decision to set afbc based on wether the fb has a modifier or not
> > > seems a bit weak. Should we also (at least) check that the modifier is
> > > an ARM one?
> > 
> > In my next patch (of this series) ie
> > https://patchwork.kernel.org/patch/10466537/, we have checked for the
> > validity of modifier and format in malidp_format_mod_supported(). When
> > drm_atomic_commit() is invoked, it first calls (indirectly via some
> > intermediate functions)
> > malidp_format_mod_supported() and then invokes
> > malidp_set_plane_base_addr(). So there is no way that an invalid
> > modifier could be passed to malidp_set_plane_base_addr(). If
> > malidp_format_mod_supported() returns error then drm_atomic_commit()
> > would return error too.
> > > 
> > > > +
> > > > +	ptr = mp->layer->ptr + (plane_index << 4);
> > > > +
> > > > +	/*
> > > > +	 * For AFBC buffers, cropping is handled by AFBC decoder rather than
> > > > +	 * pointer manipulation.
> > > > +	 */
> > > > +	if (!afbc) {
> > > > +		paddr = drm_fb_cma_get_gem_addr(fb, plane->state,
> > > > +						plane_index);
> > > > +	} else {
> > > > +		struct drm_gem_cma_object *obj;
> > > > +
> > > > +		obj = drm_fb_cma_get_gem_obj(fb, plane_index);
> > > > +
> > > > +		if (WARN_ON(!obj))
> > > > +			return;
> > > > +		paddr = obj->paddr;
> > > > +	}
> > > > +
> > > > +	malidp_hw_write(mp->hwdev, lower_32_bits(paddr), ptr);
> > > > +	malidp_hw_write(mp->hwdev, upper_32_bits(paddr), ptr + 4);
> > > > +}
> > > > +
> > > > +static void malidp_de_set_plane_afbc(struct drm_plane *plane)
> > > > +{
> > > > +	struct malidp_plane *mp;
> > > > +	u32 src_w, src_h, val = 0, src_x, src_y;
> > > > +	struct drm_framebuffer *fb = plane->state->fb;
> > > > +
> > > > +	mp = to_malidp_plane(plane);
> > > > +
> > > > +	/* convert src values from Q16 fixed point to integer */
> > > > +	src_w = plane->state->src_w >> 16;
> > > > +	src_h = plane->state->src_h >> 16;
> > > > +	src_x = plane->state->src_x >> 16;
> > > > +	src_y = plane->state->src_y >> 16;
> > > > +
> > > > +	val = ((fb->width - (src_x + src_w)) << MALIDP_AD_CROP_RIGHT_OFFSET) |
> > > > +		   src_x;
> > > > +	malidp_hw_write(mp->hwdev, val,
> > > > +			mp->layer->afbc_decoder_offset + MALIDP_AD_CROP_H);
> > > > +
> > > > +	val = ((fb->height - (src_y + src_h)) << MALIDP_AD_CROP_BOTTOM_OFFSET) |
> > > > +		   src_y;
> > > > +	malidp_hw_write(mp->hwdev, val,
> > > > +			mp->layer->afbc_decoder_offset + MALIDP_AD_CROP_V);
> > > > +
> > > > +	val = MALIDP_AD_EN;
> > > > +	if (fb->modifier & AFBC_FORMAT_MOD_SPLIT)
> > > > +		val |= MALIDP_AD_BS;
> > > > +	if (fb->modifier & AFBC_FORMAT_MOD_YTR)
> > > > +		val |= MALIDP_AD_YTR;
> > > > +
> > > > +	malidp_hw_write(mp->hwdev, val, mp->layer->afbc_decoder_offset);
> > > > +}
> > > > +
> > > >  static void malidp_de_plane_update(struct drm_plane *plane,
> > > >  				   struct drm_plane_state *old_state)
> > > >  {
> > > > @@ -338,6 +403,7 @@ static void malidp_de_plane_update(struct drm_plane *plane,
> > > >  	u32 src_w, src_h, dest_w, dest_h, val;
> > > >  	int i;
> > > >  	bool format_has_alpha = plane->state->fb->format->has_alpha;
> > > > +	struct drm_framebuffer *fb = plane->state->fb;
> > > >  
> > > >  	mp = to_malidp_plane(plane);
> > > >  
> > > > @@ -349,15 +415,9 @@ static void malidp_de_plane_update(struct drm_plane *plane,
> > > >  
> > > >  	malidp_hw_write(mp->hwdev, ms->format, mp->layer->base);
> > > >  
> > > > -	for (i = 0; i < ms->n_planes; i++) {
> > > > -		/* calculate the offset for the layer's plane registers */
> > > > -		u16 ptr = mp->layer->ptr + (i << 4);
> > > > -		dma_addr_t fb_addr = drm_fb_cma_get_gem_addr(plane->state->fb,
> > > > -							     plane->state, i);
> > > > +	for (i = 0; i < ms->n_planes; i++)
> > > > +		malidp_set_plane_base_addr(fb, mp, i);
> > > >  
> > > > -		malidp_hw_write(mp->hwdev, lower_32_bits(fb_addr), ptr);
> > > > -		malidp_hw_write(mp->hwdev, upper_32_bits(fb_addr), ptr + 4);
> > > > -	}
> > > >  	malidp_de_set_plane_pitches(mp, ms->n_planes,
> > > >  				    plane->state->fb->pitches);
> > > >  
> > > > @@ -381,6 +441,11 @@ static void malidp_de_plane_update(struct drm_plane *plane,
> > > >  				LAYER_H_VAL(src_w) | LAYER_V_VAL(src_h),
> > > >  				mp->layer->base + MALIDP550_LS_R1_IN_SIZE);
> > > >  
> > > > +	if (fb->modifier)
> > > > +		malidp_de_set_plane_afbc(plane);
> > > > +	else
> > > > +		malidp_hw_write(mp->hwdev, 0, mp->layer->afbc_decoder_offset);
> > > 
> > > Given that you are testing for the non-zero value of fb->modifier inside
> > > malidp_de_set_plane_afbc() function before setting the afbc_decoder
> > > register value, I feel that you could take the 'else' branch from here
> > > and merge it into malidp_de_set_plane_afbc() function (possibly also
> > > rename it to malidp_de_set_plane).
> > 
> > I am not exactly sure what you meant here. We are testing for the non
> > zero value of fb->modifier in malidp_de_plane_update(). If
> > fb->modifier is not zero, we set the afbc specific registers in
> > malidp_de_set_plane_afbc(). Thus, malidp_de_plane_update()  will
> > invoke malidp_de_set_plane_afbc() for setting afbc stuff and will
> > continue with the other register settings. I do not see a clear reason
> > to refactor this code.
> 
> What I'm suggesting is:
> 
>  - rename malidp_de_set_plane_afbc to malidp_de_set_plane()
>  - remove the if .. else malidp_hw_write(mp->hwdev, 0, mp->layer->afbc_decoder_offset);
>    and keep just the call to malidp_de_set_plane() without any other
>    condition
>  - inside malidp_de_set_plane(), change from:
> 	val = MALIDP_AD_EN;
>    to
> 	val = fb->modifier ? MALIDP_AD_EN : 0;
> 
>    and then the 0 value will be written correctly into the
>    afbc_decoder_offset if fb->modifier is zero.
> 
As discussed, I will consolidate all the AFBC specific register
configurations in a single function ie malidp_de_set_plane_afbc().
Thus, I will remove the "if (fb->modifier)" check from
malidp_de_plane_update() and add it in malidp_de_set_plane_afbc().

With this and  changes in other patches as well as the ack received,
I will send a v2 set of patches.

> Best regards,
> Liviu
> 
> > 
> > > With those small changes:
> > > 
> > > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> > > 
> > > Best regards,
> > > Liviu
> > > 
> > > 
> > > > +
> > > >  	/* first clear the rotation bits */
> > > >  	val = malidp_hw_read(mp->hwdev, mp->layer->base + MALIDP_LAYER_CONTROL);
> > > >  	val &= ~LAYER_ROT_MASK;
> > > > diff --git a/drivers/gpu/drm/arm/malidp_regs.h b/drivers/gpu/drm/arm/malidp_regs.h
> > > > index 149024f..54f4ec5 100644
> > > > --- a/drivers/gpu/drm/arm/malidp_regs.h
> > > > +++ b/drivers/gpu/drm/arm/malidp_regs.h
> > > > @@ -180,10 +180,13 @@
> > > >  #define MALIDP500_LV_YUV2RGB		((s16)(-0xB8))
> > > >  #define MALIDP500_DE_LV_BASE		0x00100
> > > >  #define MALIDP500_DE_LV_PTR_BASE	0x00124
> > > > +#define MALIDP500_DE_LV_AD_CTRL		0x00400
> > > >  #define MALIDP500_DE_LG1_BASE		0x00200
> > > >  #define MALIDP500_DE_LG1_PTR_BASE	0x0021c
> > > > +#define MALIDP500_DE_LG1_AD_CTRL	0x0040c
> > > >  #define MALIDP500_DE_LG2_BASE		0x00300
> > > >  #define MALIDP500_DE_LG2_PTR_BASE	0x0031c
> > > > +#define MALIDP500_DE_LG2_AD_CTRL	0x00418
> > > >  #define MALIDP500_SE_BASE		0x00c00
> > > >  #define MALIDP500_SE_CONTROL		0x00c0c
> > > >  #define MALIDP500_SE_PTR_BASE		0x00e0c
> > > > @@ -208,10 +211,13 @@
> > > >  #define MALIDP550_LV_YUV2RGB		0x00084
> > > >  #define MALIDP550_DE_LV1_BASE		0x00100
> > > >  #define MALIDP550_DE_LV1_PTR_BASE	0x00124
> > > > +#define MALIDP550_DE_LV1_AD_CTRL	0x001B8
> > > >  #define MALIDP550_DE_LV2_BASE		0x00200
> > > >  #define MALIDP550_DE_LV2_PTR_BASE	0x00224
> > > > +#define MALIDP550_DE_LV2_AD_CTRL	0x002B8
> > > >  #define MALIDP550_DE_LG_BASE		0x00300
> > > >  #define MALIDP550_DE_LG_PTR_BASE	0x0031c
> > > > +#define MALIDP550_DE_LG_AD_CTRL		0x00330
> > > >  #define MALIDP550_DE_LS_BASE		0x00400
> > > >  #define MALIDP550_DE_LS_PTR_BASE	0x0042c
> > > >  #define MALIDP550_DE_PERF_BASE		0x00500
> > > > @@ -223,6 +229,20 @@
> > > >  #define MALIDP550_CONFIG_VALID		0x0c014
> > > >  #define MALIDP550_CONFIG_ID		0x0ffd4
> > > >  
> > > > +/* AFBC register offsets relative to MALIDPXXX_DE_LX_AD_CTRL */
> > > > +/* The following register offsets are common for DP500, DP550 and DP650 */
> > > > +#define MALIDP_AD_CROP_H                0x4
> > > > +#define MALIDP_AD_CROP_V                0x8
> > > > +#define MALIDP_AD_END_PTR_LOW           0xc
> > > > +#define MALIDP_AD_END_PTR_HIGH          0x10
> > > > +
> > > > +/* AFBC decoder Registers */
> > > > +#define MALIDP_AD_EN                    BIT(0)
> > > > +#define MALIDP_AD_YTR                   BIT(4)
> > > > +#define MALIDP_AD_BS                    BIT(8)
> > > > +#define MALIDP_AD_CROP_RIGHT_OFFSET     16
> > > > +#define MALIDP_AD_CROP_BOTTOM_OFFSET    16
> > > > +
> > > >  /*
> > > >   * Starting with DP550 the register map blocks has been standardised to the
> > > >   * following layout:
> > > > -- 
> > > > 2.7.4
> > > > 
> > > 
> > > -- 
> > > ====================
> > > | I would like to |
> > > | fix the world,  |
> > > | but they're not |
> > > | giving me the   |
> > >  \ source code!  /
> > >   ---------------
> > >     ??\_(???)_/??
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ??\_(???)_/??

[RFC PATCH 4/4] drm/arm/malidp: Added support for AFBC modifiers for all layers except DE_SMART

[Ayan Kumar Halder]

On planes which support AFBC, expose an AFBC modifier for use with BGR888.

Signed-off-by: Ayan Kumar halder <ayan.halder@arm.com>
Reviewed-by: Brian Starkey <brian.starkey@arm.com>

Change-Id: I4739ff55c0f6d5715b268cb3947ed133a9ee7c2e
---
 drivers/gpu/drm/arm/malidp_drv.c    |  1 +
 drivers/gpu/drm/arm/malidp_planes.c | 46 +++++++++++++++++++++++++++++++++++--
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 7b6a848..7bcd679 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -392,6 +392,7 @@ static int malidp_init(struct drm_device *drm)
 	drm->mode_config.max_height = hwdev->max_line_size;
 	drm->mode_config.funcs = &malidp_mode_config_funcs;
 	drm->mode_config.helper_private = &malidp_mode_config_helpers;
+	drm->mode_config.allow_fb_modifiers = true;
 
 	ret = malidp_crtc_init(drm);
 	if (ret) {
diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
index 3950504..55c9a1f6 100644
--- a/drivers/gpu/drm/arm/malidp_planes.c
+++ b/drivers/gpu/drm/arm/malidp_planes.c
@@ -124,6 +124,35 @@ static void malidp_plane_atomic_print_state(struct drm_printer *p,
 	drm_printf(p, "\tn_planes=%u\n", ms->n_planes);
 }
 
+static bool malidp_format_mod_supported(struct drm_plane *plane,
+					u32 format, u64 modifier)
+{
+	if (WARN_ON(modifier == DRM_FORMAT_MOD_INVALID))
+		return false;
+
+	/* All the pixel formats are supported without any modifier */
+	if (modifier == DRM_FORMAT_MOD_LINEAR)
+		return true;
+
+	if ((modifier >> 56) != DRM_FORMAT_MOD_VENDOR_ARM)
+		return false;
+
+	if (modifier &
+	    ~DRM_FORMAT_MOD_ARM_AFBC(AFBC_MOD_VALID_BITS)) {
+		DRM_ERROR("Unsupported modifiers\n");
+		return false;
+	}
+
+	switch (modifier) {
+	case DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
+				AFBC_FORMAT_MOD_YTR |
+				AFBC_FORMAT_MOD_SPARSE):
+		if (format == DRM_FORMAT_BGR888)
+			return true;
+	}
+	return false;
+}
+
 static const struct drm_plane_funcs malidp_de_plane_funcs = {
 	.update_plane = drm_atomic_helper_update_plane,
 	.disable_plane = drm_atomic_helper_disable_plane,
@@ -132,6 +161,7 @@ static const struct drm_plane_funcs malidp_de_plane_funcs = {
 	.atomic_duplicate_state = malidp_duplicate_plane_state,
 	.atomic_destroy_state = malidp_destroy_plane_state,
 	.atomic_print_state = malidp_plane_atomic_print_state,
+	.format_mod_supported = malidp_format_mod_supported,
 };
 
 static int malidp_se_check_scaling(struct malidp_plane *mp,
@@ -524,6 +554,13 @@ int malidp_de_planes_init(struct drm_device *drm)
 	u32 *formats;
 	int ret, i, j, n;
 
+	static const u64 modifiers[] = {
+		DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
+			AFBC_FORMAT_MOD_YTR | AFBC_FORMAT_MOD_SPARSE),
+		DRM_FORMAT_MOD_LINEAR,
+		DRM_FORMAT_MOD_INVALID
+	};
+
 	formats = kcalloc(map->n_pixel_formats, sizeof(*formats), GFP_KERNEL);
 	if (!formats) {
 		ret = -ENOMEM;
@@ -547,9 +584,14 @@ int malidp_de_planes_init(struct drm_device *drm)
 
 		plane_type = (i == 0) ? DRM_PLANE_TYPE_PRIMARY :
 					DRM_PLANE_TYPE_OVERLAY;
+
+		/*
+		 * All the layers except smart layer supports AFBC modifiers.
+		 */
 		ret = drm_universal_plane_init(drm, &plane->base, crtcs,
-					       &malidp_de_plane_funcs, formats,
-					       n, NULL, plane_type, NULL);
+				&malidp_de_plane_funcs, formats, n,
+				(id == DE_SMART) ? NULL : modifiers, plane_type, NULL);
+
 		if (ret < 0)
 			goto cleanup;
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 4/4] drm/arm/malidp: Added support for AFBC modifiers for all layers except DE_SMART

[Liviu Dudau]

On Fri, Jun 15, 2018 at 02:51:34PM +0100, Ayan Kumar Halder wrote:
> On planes which support AFBC, expose an AFBC modifier for use with BGR888.
> 
> Signed-off-by: Ayan Kumar halder <ayan.halder@arm.com>
> Reviewed-by: Brian Starkey <brian.starkey@arm.com>
> 
> Change-Id: I4739ff55c0f6d5715b268cb3947ed133a9ee7c2e

Please remove the Change-Id

> ---
>  drivers/gpu/drm/arm/malidp_drv.c    |  1 +
>  drivers/gpu/drm/arm/malidp_planes.c | 46 +++++++++++++++++++++++++++++++++++--
>  2 files changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> index 7b6a848..7bcd679 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -392,6 +392,7 @@ static int malidp_init(struct drm_device *drm)
>  	drm->mode_config.max_height = hwdev->max_line_size;
>  	drm->mode_config.funcs = &malidp_mode_config_funcs;
>  	drm->mode_config.helper_private = &malidp_mode_config_helpers;
> +	drm->mode_config.allow_fb_modifiers = true;
>  
>  	ret = malidp_crtc_init(drm);
>  	if (ret) {
> diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
> index 3950504..55c9a1f6 100644
> --- a/drivers/gpu/drm/arm/malidp_planes.c
> +++ b/drivers/gpu/drm/arm/malidp_planes.c
> @@ -124,6 +124,35 @@ static void malidp_plane_atomic_print_state(struct drm_printer *p,
>  	drm_printf(p, "\tn_planes=%u\n", ms->n_planes);
>  }
>  
> +static bool malidp_format_mod_supported(struct drm_plane *plane,
> +					u32 format, u64 modifier)
> +{
> +	if (WARN_ON(modifier == DRM_FORMAT_MOD_INVALID))
> +		return false;
> +
> +	/* All the pixel formats are supported without any modifier */
> +	if (modifier == DRM_FORMAT_MOD_LINEAR)
> +		return true;
> +
> +	if ((modifier >> 56) != DRM_FORMAT_MOD_VENDOR_ARM)
> +		return false;
> +
> +	if (modifier &
> +	    ~DRM_FORMAT_MOD_ARM_AFBC(AFBC_MOD_VALID_BITS)) {
> +		DRM_ERROR("Unsupported modifiers\n");

I don't think we should polute the kernel log buffers with this. Maybe a
DRM_DEBUG() if you really want to get some notification?

With those changes added:

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> +		return false;
> +	}
> +
> +	switch (modifier) {
> +	case DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> +				AFBC_FORMAT_MOD_YTR |
> +				AFBC_FORMAT_MOD_SPARSE):
> +		if (format == DRM_FORMAT_BGR888)
> +			return true;
> +	}
> +	return false;
> +}
> +
>  static const struct drm_plane_funcs malidp_de_plane_funcs = {
>  	.update_plane = drm_atomic_helper_update_plane,
>  	.disable_plane = drm_atomic_helper_disable_plane,
> @@ -132,6 +161,7 @@ static const struct drm_plane_funcs malidp_de_plane_funcs = {
>  	.atomic_duplicate_state = malidp_duplicate_plane_state,
>  	.atomic_destroy_state = malidp_destroy_plane_state,
>  	.atomic_print_state = malidp_plane_atomic_print_state,
> +	.format_mod_supported = malidp_format_mod_supported,
>  };
>  
>  static int malidp_se_check_scaling(struct malidp_plane *mp,
> @@ -524,6 +554,13 @@ int malidp_de_planes_init(struct drm_device *drm)
>  	u32 *formats;
>  	int ret, i, j, n;
>  
> +	static const u64 modifiers[] = {
> +		DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> +			AFBC_FORMAT_MOD_YTR | AFBC_FORMAT_MOD_SPARSE),
> +		DRM_FORMAT_MOD_LINEAR,
> +		DRM_FORMAT_MOD_INVALID
> +	};
> +
>  	formats = kcalloc(map->n_pixel_formats, sizeof(*formats), GFP_KERNEL);
>  	if (!formats) {
>  		ret = -ENOMEM;
> @@ -547,9 +584,14 @@ int malidp_de_planes_init(struct drm_device *drm)
>  
>  		plane_type = (i == 0) ? DRM_PLANE_TYPE_PRIMARY :
>  					DRM_PLANE_TYPE_OVERLAY;
> +
> +		/*
> +		 * All the layers except smart layer supports AFBC modifiers.
> +		 */
>  		ret = drm_universal_plane_init(drm, &plane->base, crtcs,
> -					       &malidp_de_plane_funcs, formats,
> -					       n, NULL, plane_type, NULL);
> +				&malidp_de_plane_funcs, formats, n,
> +				(id == DE_SMART) ? NULL : modifiers, plane_type, NULL);
> +
>  		if (ret < 0)
>  			goto cleanup;
>  
> -- 
> 2.7.4
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [RFC PATCH 4/4] drm/arm/malidp: Added support for AFBC modifiers for all layers except DE_SMART

[Ayan Halder]

On Tue, Jun 26, 2018 at 02:21:58PM +0100, Liviu Dudau wrote:
> On Fri, Jun 15, 2018 at 02:51:34PM +0100, Ayan Kumar Halder wrote:
> > On planes which support AFBC, expose an AFBC modifier for use with BGR888.
> > 
> > Signed-off-by: Ayan Kumar halder <ayan.halder@arm.com>
> > Reviewed-by: Brian Starkey <brian.starkey@arm.com>
> > 
> > Change-Id: I4739ff55c0f6d5715b268cb3947ed133a9ee7c2e
> 
> Please remove the Change-Id
> 
> > ---
> >  drivers/gpu/drm/arm/malidp_drv.c    |  1 +
> >  drivers/gpu/drm/arm/malidp_planes.c | 46 +++++++++++++++++++++++++++++++++++--
> >  2 files changed, 45 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> > index 7b6a848..7bcd679 100644
> > --- a/drivers/gpu/drm/arm/malidp_drv.c
> > +++ b/drivers/gpu/drm/arm/malidp_drv.c
> > @@ -392,6 +392,7 @@ static int malidp_init(struct drm_device *drm)
> >  	drm->mode_config.max_height = hwdev->max_line_size;
> >  	drm->mode_config.funcs = &malidp_mode_config_funcs;
> >  	drm->mode_config.helper_private = &malidp_mode_config_helpers;
> > +	drm->mode_config.allow_fb_modifiers = true;
> >  
> >  	ret = malidp_crtc_init(drm);
> >  	if (ret) {
> > diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
> > index 3950504..55c9a1f6 100644
> > --- a/drivers/gpu/drm/arm/malidp_planes.c
> > +++ b/drivers/gpu/drm/arm/malidp_planes.c
> > @@ -124,6 +124,35 @@ static void malidp_plane_atomic_print_state(struct drm_printer *p,
> >  	drm_printf(p, "\tn_planes=%u\n", ms->n_planes);
> >  }
> >  
> > +static bool malidp_format_mod_supported(struct drm_plane *plane,
> > +					u32 format, u64 modifier)
> > +{
> > +	if (WARN_ON(modifier == DRM_FORMAT_MOD_INVALID))
> > +		return false;
> > +
> > +	/* All the pixel formats are supported without any modifier */
> > +	if (modifier == DRM_FORMAT_MOD_LINEAR)
> > +		return true;
> > +
> > +	if ((modifier >> 56) != DRM_FORMAT_MOD_VENDOR_ARM)
> > +		return false;
> > +
> > +	if (modifier &
> > +	    ~DRM_FORMAT_MOD_ARM_AFBC(AFBC_MOD_VALID_BITS)) {
> > +		DRM_ERROR("Unsupported modifiers\n");
> 
> I don't think we should polute the kernel log buffers with this. Maybe a
> DRM_DEBUG() if you really want to get some notification?

I agree with you completely. I think we should use DRM_DEBUG_KMS()
like intel_framebuffer_init() uses to log unsupported formats and
modifiers.
I will send a v2 patch for this.
> 
> With those changes added:
> 
> Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> 
> Best regards,
> Liviu
> 
> > +		return false;
> > +	}
> > +
> > +	switch (modifier) {
> > +	case DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> > +				AFBC_FORMAT_MOD_YTR |
> > +				AFBC_FORMAT_MOD_SPARSE):
> > +		if (format == DRM_FORMAT_BGR888)
> > +			return true;
> > +	}
> > +	return false;
> > +}
> > +
> >  static const struct drm_plane_funcs malidp_de_plane_funcs = {
> >  	.update_plane = drm_atomic_helper_update_plane,
> >  	.disable_plane = drm_atomic_helper_disable_plane,
> > @@ -132,6 +161,7 @@ static const struct drm_plane_funcs malidp_de_plane_funcs = {
> >  	.atomic_duplicate_state = malidp_duplicate_plane_state,
> >  	.atomic_destroy_state = malidp_destroy_plane_state,
> >  	.atomic_print_state = malidp_plane_atomic_print_state,
> > +	.format_mod_supported = malidp_format_mod_supported,
> >  };
> >  
> >  static int malidp_se_check_scaling(struct malidp_plane *mp,
> > @@ -524,6 +554,13 @@ int malidp_de_planes_init(struct drm_device *drm)
> >  	u32 *formats;
> >  	int ret, i, j, n;
> >  
> > +	static const u64 modifiers[] = {
> > +		DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> > +			AFBC_FORMAT_MOD_YTR | AFBC_FORMAT_MOD_SPARSE),
> > +		DRM_FORMAT_MOD_LINEAR,
> > +		DRM_FORMAT_MOD_INVALID
> > +	};
> > +
> >  	formats = kcalloc(map->n_pixel_formats, sizeof(*formats), GFP_KERNEL);
> >  	if (!formats) {
> >  		ret = -ENOMEM;
> > @@ -547,9 +584,14 @@ int malidp_de_planes_init(struct drm_device *drm)
> >  
> >  		plane_type = (i == 0) ? DRM_PLANE_TYPE_PRIMARY :
> >  					DRM_PLANE_TYPE_OVERLAY;
> > +
> > +		/*
> > +		 * All the layers except smart layer supports AFBC modifiers.
> > +		 */
> >  		ret = drm_universal_plane_init(drm, &plane->base, crtcs,
> > -					       &malidp_de_plane_funcs, formats,
> > -					       n, NULL, plane_type, NULL);
> > +				&malidp_de_plane_funcs, formats, n,
> > +				(id == DE_SMART) ? NULL : modifiers, plane_type, NULL);
> > +
> >  		if (ret < 0)
> >  			goto cleanup;
> >  
> > -- 
> > 2.7.4
> > 
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ??\_(???)_/??
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel