[PATCH v3 0/6] Add method to describe tile/bit_level_packed formats

[PATCH v3 0/6] Add method to describe tile/bit_level_packed formats

[Alexandru-Cosmin Gheorghe]

There has been some discussion about extending drm core to handle
linear tile formats, in the series sent by me here [1] and how to
handle formats that are intended to be used just with
modifiers(particularly AFBC modifiers) on Brian series [2] and on IRC
here [3] and [4].

Hence, this big-merged series:

Patches 1-3: handle tiled formats both in core and in malidp driver,
this is done by extending drm_format_info with three new fields
char_per_block, block_w, block_h and consistently handle in the generic
code paths, both linear tiled formats and normal formats.
What's different from [1] is the interpretation of pitch for tile
formats which has been kept to be the same as for the other formats:
pitch = average_chars_per_pixel * width.

Patches 4-6: Introduce the YUV AFBC formats, the only thing noteworthy
here is that cpp/char_per_block are set to 0 for formats where it's
mandatory to be used together with a non-linear modifier and then that
is used to bypass pitch check in framebuffer_check for formats that
have cpp/char_per_block set to 0.

[1] https://lists.freedesktop.org/archives/dri-devel/2018-September/188245.html
[2] https://lists.freedesktop.org/archives/dri-devel/2018-September/189620.html
[3] https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2018-09-13&show_html=true
[4] https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2018-09-14&show_html=true

Alexandru Gheorghe (4):
  drm/fourcc: Add char_per_block, block_w and block_h in drm_format_info
  drm/fourcc: Add fourcc for Mali linear tiled formats
  drm: mali-dp: Enable Mali-DP tiled buffer formats
  drm: Extend framebuffer_check to handle formats with
    cpp/char_per_block 0

Brian Starkey (2):
  drm/fourcc: Add AFBC yuv fourccs for Mali
  drm/afbc: Add AFBC modifier usage documentation

 Documentation/gpu/afbc.rst                   | 233 +++++++++++++++++++
 Documentation/gpu/drivers.rst                |   1 +
 MAINTAINERS                                  |   1 +
 drivers/gpu/drm/arm/malidp_hw.c              |  14 +-
 drivers/gpu/drm/arm/malidp_planes.c          |  23 +-
 drivers/gpu/drm/drm_fb_cma_helper.c          |  21 +-
 drivers/gpu/drm/drm_fb_helper.c              |   6 +
 drivers/gpu/drm/drm_fourcc.c                 |  65 ++++++
 drivers/gpu/drm/drm_framebuffer.c            |  13 +-
 drivers/gpu/drm/drm_gem_framebuffer_helper.c |   4 +-
 include/drm/drm_fourcc.h                     |  29 ++-
 include/uapi/drm/drm_fourcc.h                |  31 +++
 12 files changed, 428 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/gpu/afbc.rst

-- 
2.18.0

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

[PATCH v3 1/6] drm/fourcc: Add char_per_block, block_w and block_h in drm_format_info

[Alexandru-Cosmin Gheorghe]

For some pixel formats .cpp structure in drm_format info it's not
enough to describe the peculiarities of the pixel layout, for example
tiled formats or packed formats at bit level.

What's implemented here is to add three new members to drm_format_info
that could describe such formats:

- char_per_block[3]
- block_w[3]
- block_h[3]

char_per_block will be put in a union alongside cpp, for transparent
compatibility  with the existing format descriptions.

Regarding, block_w and block_h they are intended to be used through
their equivalent getters drm_format_info_block_width /
drm_format_info_block_height, the reason of the getters is to abstract
the fact that for normal formats block_w and block_h will be unset/0,
but the methods will be returning 1.

Using that the following drm core functions had been updated to
generically handle both block and non-block formats:

- drm_fb_cma_get_gem_addr: for block formats it will just return the
  beginning of the block.
- framebuffer_check: Updated to compute the minimum pitch as
	DIV_ROUND_UP(width * char_per_block, drm_format_info_block_width(info, i)
	* drm_format_info_block_height(info, i))
- drm_gem_fb_create_with_funcs: Updated to compute the size of the
  last line of pixels with the same formula as for the minimum pitch.
- In places where is not expecting to handle block formats, like fbdev
  helpers I just added some warnings in case the block width/height
  are greater than 1.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
---
 drivers/gpu/drm/drm_fb_cma_helper.c          | 21 ++++++++--
 drivers/gpu/drm/drm_fb_helper.c              |  6 +++
 drivers/gpu/drm/drm_fourcc.c                 | 40 ++++++++++++++++++++
 drivers/gpu/drm/drm_framebuffer.c            |  9 +++--
 drivers/gpu/drm/drm_gem_framebuffer_helper.c |  4 +-
 include/drm/drm_fourcc.h                     | 29 +++++++++++++-
 6 files changed, 101 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 47e0e2f6642d..f8293f4d96cf 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -72,7 +72,9 @@ struct drm_gem_cma_object *drm_fb_cma_get_gem_obj(struct drm_framebuffer *fb,
 EXPORT_SYMBOL_GPL(drm_fb_cma_get_gem_obj);
 
 /**
- * drm_fb_cma_get_gem_addr() - Get physical address for framebuffer
+ * drm_fb_cma_get_gem_addr() - Get physical address for framebuffer, for pixel
+ * formats where values are grouped in blocks this will get you the beginning of
+ * the block
  * @fb: The framebuffer
  * @state: Which state of drm plane
  * @plane: Which plane
@@ -87,6 +89,14 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
 	struct drm_gem_cma_object *obj;
 	dma_addr_t paddr;
 	u8 h_div = 1, v_div = 1;
+	u32 block_w = drm_format_info_block_width(fb->format, plane);
+	u32 block_h = drm_format_info_block_height(fb->format, plane);
+	u32 block_size = fb->format->char_per_block[plane];
+	u32 sample_x;
+	u32 sample_y;
+	u32 block_start_y;
+	u32 num_hblocks;
+
 
 	obj = drm_fb_cma_get_gem_obj(fb, plane);
 	if (!obj)
@@ -99,8 +109,13 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
 		v_div = fb->format->vsub;
 	}
 
-	paddr += (fb->format->cpp[plane] * (state->src_x >> 16)) / h_div;
-	paddr += (fb->pitches[plane] * (state->src_y >> 16)) / v_div;
+	sample_x = (state->src_x >> 16) / h_div;
+	sample_y = (state->src_y >> 16) / v_div;
+	block_start_y = (sample_y / block_h) * block_h;
+	num_hblocks = sample_x / block_w;
+
+	paddr += fb->pitches[plane] * block_start_y;
+	paddr += block_size * num_hblocks;
 
 	return paddr;
 }
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index a504a5e05676..9add0d7da744 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1595,6 +1595,10 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 	if (var->pixclock != 0 || in_dbg_master())
 		return -EINVAL;
 
+	if ((drm_format_info_block_width(fb->format, 0) > 1) ||
+	    (drm_format_info_block_height(fb->format, 0) > 1))
+		return -EINVAL;
+
 	/*
 	 * Changes struct fb_var_screeninfo are currently not pushed back
 	 * to KMS, hence fail if different settings are requested.
@@ -1969,6 +1973,8 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
 {
 	struct drm_framebuffer *fb = fb_helper->fb;
 
+	WARN_ON((drm_format_info_block_width(fb->format, 0) > 1) ||
+		(drm_format_info_block_height(fb->format, 0) > 1));
 	info->pseudo_palette = fb_helper->pseudo_palette;
 	info->var.xres_virtual = fb->width;
 	info->var.yres_virtual = fb->height;
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index a154763257ae..0c9725ffd5e9 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -403,3 +403,43 @@ int drm_format_plane_height(int height, uint32_t format, int plane)
 	return height / info->vsub;
 }
 EXPORT_SYMBOL(drm_format_plane_height);
+
+/**
+ * drm_format_info_block_width - width in pixels of block.
+ * @info: pixel format info
+ * @plane: plane index
+ *
+ * Returns:
+ * The width in pixels of a block, depending on the plane index.
+ */
+unsigned int drm_format_info_block_width(const struct drm_format_info *info,
+					 int plane)
+{
+	if (!info || plane >= info->num_planes)
+		return 0;
+
+	if (!info->block_w[plane])
+		return 1;
+	return info->block_w[plane];
+}
+EXPORT_SYMBOL(drm_format_info_block_width);
+
+/**
+ * drm_format_info_block_height - height in pixels of a block
+ * @info: pixel format info
+ * @plane: plane index
+ *
+ * Returns:
+ * The height in pixels of a block, depending on the plane index.
+ */
+unsigned int drm_format_info_block_height(const struct drm_format_info *info,
+					  int plane)
+{
+	if (!info || plane >= info->num_planes)
+		return 0;
+
+	if (!info->block_h[plane])
+		return 1;
+	return info->block_h[plane];
+}
+EXPORT_SYMBOL(drm_format_info_block_height);
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 3bf729d0aae5..4e14013788cd 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -195,20 +195,23 @@ static int framebuffer_check(struct drm_device *dev,
 	for (i = 0; i < info->num_planes; i++) {
 		unsigned int width = fb_plane_width(r->width, info, i);
 		unsigned int height = fb_plane_height(r->height, info, i);
-		unsigned int cpp = info->cpp[i];
+		unsigned int block_size = info->char_per_block[i];
+		u64 min_pitch = DIV_ROUND_UP((u64)width * block_size,
+					drm_format_info_block_width(info, i) *
+					drm_format_info_block_height(info, i));
 
 		if (!r->handles[i]) {
 			DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i);
 			return -EINVAL;
 		}
 
-		if ((uint64_t) width * cpp > UINT_MAX)
+		if (min_pitch > UINT_MAX)
 			return -ERANGE;
 
 		if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
 			return -ERANGE;
 
-		if (r->pitches[i] < width * cpp) {
+		if (r->pitches[i] < min_pitch) {
 			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
 			return -EINVAL;
 		}
diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
index ded7a379ac35..e342511370e7 100644
--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
@@ -171,7 +171,9 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
 		}
 
 		min_size = (height - 1) * mode_cmd->pitches[i]
-			 + width * info->cpp[i]
+			 + DIV_ROUND_UP((u64)width * info->char_per_block[i],
+					drm_format_info_block_width(info, i) *
+					drm_format_info_block_height(info, i))
 			 + mode_cmd->offsets[i];
 
 		if (objs[i]->size < min_size) {
diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index 865ef60c17af..305e80847abc 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -58,6 +58,24 @@ struct drm_mode_fb_cmd2;
  *	use in new code and set to 0 for new formats.
  * @num_planes: Number of color planes (1 to 3)
  * @cpp: Number of bytes per pixel (per plane)
+ * @char_per_block:  Number of bytes per block (per plane), where blocks are
+ *	defined as a rectangle of pixels which are stored next to each other in
+ *	a byte aligned memory region.
+ *	Together with block_w and block_h this could be used to properly
+ *	describe tiles in tiled formats or to describe groups of pixels in
+ *	packed formats for which the memory needed for a single pixel it's not
+ *	byte aligned.
+ *      Cpp had been kept from historical reasons because there are a lot of
+ *	places in drivers where it's used. In drm core for generic code paths
+ *	the preferred way is to use char_per_block, drm_format_info_block_width
+ *	and drm_format_info_block_height which should allow handling both block
+ *	and non-block formats in the same way.
+ *	For formats that are intended to be used just with modifiers, cpp/
+ *	char_per_block must be 0.
+ * @block_w: Block width in pixels, this is intended to be accessed through
+ *	drm_format_info_block_width
+ * @block_h: Block height in pixels, this is intended to be accessed through
+ *	drm_format_info_block_height
  * @hsub: Horizontal chroma subsampling factor
  * @vsub: Vertical chroma subsampling factor
  * @has_alpha: Does the format embeds an alpha component?
@@ -67,7 +85,12 @@ struct drm_format_info {
 	u32 format;
 	u8 depth;
 	u8 num_planes;
-	u8 cpp[3];
+	union {
+		u8 cpp[3];
+		u8 char_per_block[3];
+	};
+	u8 block_w[3];
+	u8 block_h[3];
 	u8 hsub;
 	u8 vsub;
 	bool has_alpha;
@@ -96,6 +119,10 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
 int drm_format_vert_chroma_subsampling(uint32_t format);
 int drm_format_plane_width(int width, uint32_t format, int plane);
 int drm_format_plane_height(int height, uint32_t format, int plane);
+unsigned int drm_format_info_block_width(const struct drm_format_info *info,
+					 int plane);
+unsigned int drm_format_info_block_height(const struct drm_format_info *info,
+					  int plane);
 const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
 
 #endif /* __DRM_FOURCC_H__ */
-- 
2.18.0

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

[PATCH v3 2/6] drm/fourcc: Add fourcc for Mali linear tiled formats

[Alexandru-Cosmin Gheorghe]

Mali-DP implements a number of tiled yuv formats which are not
currently described in drm_fourcc.h.
This adds those definitions and describes their memory layout by
using the newly added char_per_block, block_w, block_h.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
---
 drivers/gpu/drm/drm_fourcc.c  | 12 ++++++++++++
 include/uapi/drm/drm_fourcc.h | 14 ++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 0c9725ffd5e9..13b2163e5d0b 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -228,6 +228,18 @@ const struct drm_format_info *__drm_format_info(u32 format)
 		{ .format = DRM_FORMAT_P010,		.depth = 0,  .num_planes = 2, .cpp = { 2, 4, 0 }, .hsub = 2, .vsub = 2, .is_yuv = true  },
 		{ .format = DRM_FORMAT_P012,		.depth = 0,  .num_planes = 2, .cpp = { 2, 4, 0 }, .hsub = 2, .vsub = 2, .is_yuv = true  },
 		{ .format = DRM_FORMAT_P016,		.depth = 0,  .num_planes = 2, .cpp = { 2, 4, 0 }, .hsub = 2, .vsub = 2, .is_yuv = true  },
+		{ .format = DRM_FORMAT_Y0L0,		.depth = 0,  .num_planes = 1,
+		  .char_per_block = { 8, 0, 0 }, .block_w = { 2, 0, 0 }, .block_h = { 2, 0, 0},
+		  .hsub = 2, .vsub = 2, .has_alpha = true, .is_yuv = true },
+		{ .format = DRM_FORMAT_X0L0,		.depth = 0,  .num_planes = 1,
+		  .char_per_block = { 8, 0, 0 }, .block_w = { 2, 0, 0 }, .block_h = { 2, 0, 0},
+		  .hsub = 2, .vsub = 2, .is_yuv = true },
+		{ .format = DRM_FORMAT_Y0L2,		.depth = 0,  .num_planes = 1,
+		  .char_per_block = { 8, 0, 0 }, .block_w = { 2, 0, 0 }, .block_h = { 2, 0, 0},
+		  .hsub = 2, .vsub = 2, .has_alpha = true, .is_yuv = true },
+		{ .format = DRM_FORMAT_X0L2,		.depth = 0,  .num_planes = 1,
+		  .char_per_block = { 8, 0, 0 }, .block_w = { 2, 0, 0 }, .block_h = { 2, 0, 0},
+		  .hsub = 2, .vsub = 2, .is_yuv = true },
 	};
 
 	unsigned int i;
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 600106adf91f..4de86dbf40ca 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -152,6 +152,20 @@ extern "C" {
 
 #define DRM_FORMAT_AYUV		fourcc_code('A', 'Y', 'U', 'V') /* [31:0] A:Y:Cb:Cr 8:8:8:8 little endian */
 
+/*
+ * packed YCbCr420 2x2 tiled formats
+ * first 64 bits will contain Y,Cb,Cr components for a 2x2 tile
+ */
+/* [63:0]   A3:A2:Y3:0:Cr0:0:Y2:0:A1:A0:Y1:0:Cb0:0:Y0:0  1:1:8:2:8:2:8:2:1:1:8:2:8:2:8:2 little endian */
+#define DRM_FORMAT_Y0L0		fourcc_code('Y', '0', 'L', '0')
+/* [63:0]   X3:X2:Y3:0:Cr0:0:Y2:0:X1:X0:Y1:0:Cb0:0:Y0:0  1:1:8:2:8:2:8:2:1:1:8:2:8:2:8:2 little endian */
+#define DRM_FORMAT_X0L0		fourcc_code('X', '0', 'L', '0')
+
+/* [63:0]   A3:A2:Y3:Cr0:Y2:A1:A0:Y1:Cb0:Y0  1:1:10:10:10:1:1:10:10:10 little endian */
+#define DRM_FORMAT_Y0L2		fourcc_code('Y', '0', 'L', '2')
+/* [63:0]   X3:X2:Y3:Cr0:Y2:X1:X0:Y1:Cb0:Y0  1:1:10:10:10:1:1:10:10:10 little endian */
+#define DRM_FORMAT_X0L2		fourcc_code('X', '0', 'L', '2')
+
 /*
  * 2 plane RGB + A
  * index 0 = RGB plane, same format as the corresponding non _A8 format has
-- 
2.18.0

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

[PATCH v3 3/6] drm: mali-dp: Enable Mali-DP tiled buffer formats

[Alexandru-Cosmin Gheorghe]

Enable the following formats
- DRM_FORMAT_X0L0: DP650
- DRM_FORMAT_X0L2: DP550, DP650

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
---
 drivers/gpu/drm/arm/malidp_hw.c     | 14 +++++++++++---
 drivers/gpu/drm/arm/malidp_planes.c | 23 +++++++++++++++++++++--
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index c94a4422e0e9..e01fc0e5b503 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -77,12 +77,18 @@ static const struct malidp_format_id malidp500_de_formats[] = {
 	{ DRM_FORMAT_YUYV, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 2) },	\
 	{ DRM_FORMAT_UYVY, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 3) },	\
 	{ DRM_FORMAT_NV12, DE_VIDEO1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(5, 6) },	\
-	{ DRM_FORMAT_YUV420, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 7) }
+	{ DRM_FORMAT_YUV420, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 7) }, \
+	{ DRM_FORMAT_X0L2, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(6, 6)}
 
 static const struct malidp_format_id malidp550_de_formats[] = {
 	MALIDP_COMMON_FORMATS,
 };
 
+static const struct malidp_format_id malidp650_de_formats[] = {
+	MALIDP_COMMON_FORMATS,
+	{ DRM_FORMAT_X0L0, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 4)},
+};
+
 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 },
 	{ DE_GRAPHICS1, MALIDP500_DE_LG1_BASE, MALIDP500_DE_LG1_PTR_BASE, MALIDP_DE_LG_STRIDE, 0 },
@@ -595,6 +601,8 @@ static int malidp550_rotmem_required(struct malidp_hw_device *hwdev, u16 w, u16
 	case DRM_FORMAT_BGR565:
 	case DRM_FORMAT_UYVY:
 	case DRM_FORMAT_YUYV:
+	case DRM_FORMAT_X0L0:
+	case DRM_FORMAT_X0L2:
 		bytes_per_col = 32;
 		break;
 	/* 16 lines at 1.5 bytes per pixel */
@@ -860,8 +868,8 @@ const struct malidp_hw malidp_device[MALIDP_MAX_DEVICES] = {
 					    MALIDP550_DC_IRQ_SE,
 				.vsync_irq = MALIDP550_DC_IRQ_CONF_VALID,
 			},
-			.pixel_formats = malidp550_de_formats,
-			.n_pixel_formats = ARRAY_SIZE(malidp550_de_formats),
+			.pixel_formats = malidp650_de_formats,
+			.n_pixel_formats = ARRAY_SIZE(malidp650_de_formats),
 			.bus_align_bytes = 16,
 		},
 		.query_hw = malidp650_query_hw,
diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
index 49c37f6dd63e..33bbc29da774 100644
--- a/drivers/gpu/drm/arm/malidp_planes.c
+++ b/drivers/gpu/drm/arm/malidp_planes.c
@@ -196,13 +196,26 @@ static int malidp_de_plane_check(struct drm_plane *plane,
 	ms->n_planes = fb->format->num_planes;
 	for (i = 0; i < ms->n_planes; i++) {
 		u8 alignment = malidp_hw_get_pitch_align(mp->hwdev, rotated);
-		if (fb->pitches[i] & (alignment - 1)) {
+
+		if ((fb->pitches[i] * drm_format_info_block_height(fb->format, i))
+				& (alignment - 1)) {
 			DRM_DEBUG_KMS("Invalid pitch %u for plane %d\n",
 				      fb->pitches[i], i);
 			return -EINVAL;
 		}
 	}
 
+	if (fb->width % drm_format_info_block_width(fb->format, 0) ||
+	    fb->height % drm_format_info_block_height(fb->format, 0)) {
+		DRM_DEBUG_KMS("Buffer width/height needs to be a multiple of tile sizes");
+		return -EINVAL;
+	}
+	if ((state->src_x >> 16) % drm_format_info_block_width(fb->format, 0) ||
+	    (state->src_y >> 16) % drm_format_info_block_height(fb->format, 0)) {
+		DRM_DEBUG_KMS("Plane src_x/src_y needs to be a multiple of tile sizes");
+		return -EINVAL;
+	}
+
 	if ((state->crtc_w > mp->hwdev->max_line_size) ||
 	    (state->crtc_h > mp->hwdev->max_line_size) ||
 	    (state->crtc_w < mp->hwdev->min_line_size) ||
@@ -258,8 +271,14 @@ static void malidp_de_set_plane_pitches(struct malidp_plane *mp,
 		num_strides = (mp->hwdev->hw->features &
 			       MALIDP_DEVICE_LV_HAS_3_STRIDES) ? 3 : 2;
 
+	/*
+	 * The drm convention for pitch is that it needs to cover width * cpp,
+	 * but our hardware wants the pitch/stride to cover all rows included
+	 * in a tile.
+	 */
 	for (i = 0; i < num_strides; ++i)
-		malidp_hw_write(mp->hwdev, pitches[i],
+		malidp_hw_write(mp->hwdev, pitches[i] *
+				drm_format_info_block_height(mp->base.state->fb->format, i),
 				mp->layer->base +
 				mp->layer->stride_offset + i * 4);
 }
-- 
2.18.0

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

[PATCH v3 4/6] drm: Extend framebuffer_check to handle formats with cpp/char_per_block 0

[Alexandru-Cosmin Gheorghe]

For formats that are supported only with non-linear modifiers it
doesn't make to much sense to define cpp or char_per_block, so that
will be set to 0.

This patch adds a restriction to force having a modifier attached when
cpp/char_per_block is 0, and to bypass checking the pitch restriction.

This had been discussed here.
[1] https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2018-09-13&show_html=true

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
---
 drivers/gpu/drm/drm_framebuffer.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 4e14013788cd..66ec126b7faf 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -200,6 +200,10 @@ static int framebuffer_check(struct drm_device *dev,
 					drm_format_info_block_width(info, i) *
 					drm_format_info_block_height(info, i));
 
+		if (!block_size && (r->modifier[i] == DRM_FORMAT_MOD_LINEAR)) {
+			DRM_DEBUG_KMS("Format requires non-linear modifier for plane %d\n", i);
+			return -EINVAL;
+		}
 		if (!r->handles[i]) {
 			DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i);
 			return -EINVAL;
@@ -211,7 +215,7 @@ static int framebuffer_check(struct drm_device *dev,
 		if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
 			return -ERANGE;
 
-		if (r->pitches[i] < min_pitch) {
+		if (block_size && r->pitches[i] < min_pitch) {
 			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
 			return -EINVAL;
 		}
-- 
2.18.0

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

[PATCH v3 5/6] drm/fourcc: Add AFBC yuv fourccs for Mali

[Alexandru-Cosmin Gheorghe]

From: Brian Starkey <brian.starkey@arm.com>

As we look to enable AFBC using DRM format modifiers, we run into
problems which we've historically handled via vendor-private details
(i.e. gralloc, on Android).

AFBC (as an encoding) is fully flexible, and for example YUV data can
be encoded into 1, 2 or 3 encoded "planes", much like the linear
equivalents. Component order is also meaningful, as AFBC doesn't
necessarily care about what each "channel" of the data it encodes
contains. Therefore ABGR8888 and RGBA8888 can be encoded in AFBC with
different representations. Similarly, 'X' components may be encoded
into AFBC streams in cases where a decoder expects to decode a 4th
component.

In addition, AFBC is a licensable IP, meaning that to support the
ecosystem we need to ensure that _all_ AFBC users are able to describe
the encodings that they need. This is much better achieved by
preserving meaning in the fourcc codes when they are combined with an
AFBC modifier.

In essence, we want to use the modifier to describe the parameters of
the AFBC encode/decode, and use the fourcc code to describe the data
being encoded/decoded.

To do anything different would be to introduce redundancy - we would
need to duplicate in the modifier information which is _already_
conveyed clearly and non-ambigiously by a fourcc code.

I hope that for RGB this is non-controversial.
(BGRA8888 + MODIFIER_AFBC) is a different format from
(RGBA8888 + MODIFIER_AFBC).

Possibly more controversial is that (XBGR8888 + MODIFIER_AFBC)
is different from (BGR888 + MODIFIER_AFBC). I understand that in some
schemes it is not the case - but in AFBC it is so.

Where we run into problems is where there are not already fourcc codes
which represent the data which the AFBC encoder/decoder is processing.
To that end, we want to introduce new fourcc codes to describe the
data being encoded/decoded, in the places where none of the existing
fourcc codes are applicable.

Where we don't support an equivalent non-compressed layout, or where
no "obvious" linear layout exists, we are proposing adding fourcc
codes which have no associated linear layout - because any layout we
proposed would be completely arbitrary.

Some formats are following the naming conventions from [2].

The summary of the new formats is:
 DRM_FORMAT_VUY888 - Packed 8-bit YUV 444. Y followed by U then V.
 DRM_FORMAT_VUY101010 - Packed 10-bit YUV 444. Y followed by U then
                        V. No defined linear encoding.
 DRM_FORMAT_Y210 - Packed 10-bit YUV 422. Y followed by U (then Y)
                   then V. 10-bit samples in 16-bit words.
 DRM_FORMAT_Y410 - Packed 10-bit YUV 444, with 2-bit alpha.
 DRM_FORMAT_P210 - Semi-planar 10-bit YUV 422. Y plane, followed by
                   interleaved U-then-V plane. 10-bit samples in
                   16-bit words.
 DRM_FORMAT_YUV420_8BIT - Packed 8-bit YUV 420. Y followed by U then
                          V. No defined linear encoding
 DRM_FORMAT_YUV420_10BIT - Packed 10-bit YUV 420. Y followed by U
                           then V. No defined linear encoding

Please also note that in the absence of AFBC, we would still need to
add Y410, Y210 and P210.

Full rationale follows:

YUV 444 8-bit, 1-plane
----------------------
 The currently defined AYUV format encodes a 4th alpha component,
 which makes it unsuitable for representing a 3-component YUV 444
 AFBC stream.

 The proposed[1] XYUV format which is supported by Mali-DP in linear
 layout is also unsuitable, because the component order is the
 opposite of the AFBC version, and it encodes a 4th 'X' component.

 DRM_FORMAT_VUY888 is the "obvious" format for a 3-component, packed,
 YUV 444 8-bit format, with the component order which our HW expects to
 encode/decode. It conforms to the same naming convention as the
 existing packed YUV 444 format.
 The naming here is meant to be consistent with DRM_FORMAT_AYUV and
 DRM_FORMAT_XYUV[1]

YUV 444 10-bit, 1-plane
-----------------------
 There is no currently-defined YUV 444 10-bit format in
 drm_fourcc.h, irrespective of number of planes.

 The proposed[1] XVYU2101010 format which is supported by Mali-DP in
 linear layout uses the wrong component order, and also encodes a 4th
 'X' component, which doesn't match the AFBC version of YUV 444
 10-bit which we support.

 DRM_FORMAT_Y410 is the same layout as XVYU2101010, but with 2 bits of
 alpha.  This format is supported with linear layout by Mali GPUs. The
 naming follows[2].

 There is no "obvious" linear encoding for a 3-component 10:10:10
 packed format, and so DRM_FORMAT_VUY101010 defines a component
 order, but not a bit encoding. Again, the naming is meant to be
 consistent with DRM_FORMAT_AYUV.

YUV 422 8-bit, 1-plane
----------------------
 The existing DRM_FORMAT_YUYV (and the other component orders) are
 single-planar YUV 422 8-bit formats. Following the convention of
 the component orders of the RGB formats, YUYV has the correct
 component order for our AFBC encoding (Y followed by U followed by
 V). We can use YUYV for AFBC YUV 422 8-bit.

YUV 422 10-bit, 1-plane
-----------------------
 There is no currently-defined YUV 422 10-bit format in drm_fourcc.h

 DRM_FORMAT_Y210 is analogous to YUYV, but with 10-bits per sample
 packed into the upper 10-bits of 16-bit samples. This format is
 supported in both linear and AFBC by Mali GPUs.

YUV 422 10-bit, 2-plane
-----------------------
 The recently defined DRM_FORMAT_P010 format is a 10-bit semi-planar
 YUV 420 format, which has the correct component ordering for an AFBC
 2-plane YUV 420 buffer. The linear layout contains meaningless padding
 bits, which will not be encoded in an AFBC stream.

YUV 420 8-bit, 1-plane
----------------------
 There is no currently defined single-planar YUV 420, 8-bit format
 in drm_fourcc.h. There's differing opinions on whether using the
 existing fourcc-implied n_planes where possible is a good idea or
 not when using modifiers.

 For me, it's much more "obvious" to use NV12 for 2-plane AFBC and
 YUV420 for 3-plane AFBC. This keeps the aforementioned separation
 between the AFBC codec settings (in the modifier) and the pixel data
 format (in the fourcc). With different vendors using AFBC, this helps
 to ensure that there is no confusion in interoperation. It also
 ensures that the AFBC modifiers describe AFBC itself (which is a
 licensable component), and not implementation details which are not
 defined by AFBC.

 The proposed[1] X0L0 format which Mali-DP supports with Linear layout
 is unsuitable, as it contains a 4th 'X' component, and our AFBC
 decoder expects only 3 components.

 To that end, we propose a new YUV 420 8-bit format. There is no
 "obvious" linear encoding for a 3-component 8:8:8, 420, packed format,
 and so DRM_FORMAT_YUV420_8BIT defines a component order, but not a
 bit encoding. I'm happy to hear different naming suggestions.

YUV 420 8-bit, 2-, 3-plane
--------------------------
 These already exist, we can use NV12 and YUV420.

YUV 420 10-bit, 1-plane
-----------------------
 As above, no current definition exists, and X0L2 encodes a 4th 'X'
 channel.

 Analogous to DRM_FORMAT_YUV420_8BIT, we define DRM_FORMAT_YUV420_10BIT.

[1] https://lists.freedesktop.org/archives/dri-devel/2018-July/184598.html
[2] https://docs.microsoft.com/en-us/windows/desktop/medfound/10-bit-and-16-bit-yuv-video-formats

Changes since RFC v1:
 - Fix confusing subsampling vs bit-depth X:X:X notation in
   descriptions (danvet)
 - Rename DRM_FORMAT_AVYU1101010 to DRM_FORMAT_Y410 (Lisa Wu)
 - Add drm_format_info structures for the new formats, using the
   new 'bpp' field for those with non-integer bytes-per-pixel
 - Rebase, including Juha-Pekka Heikkila's format definitions

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
---
 drivers/gpu/drm/drm_fourcc.c  | 13 +++++++++++++
 include/uapi/drm/drm_fourcc.h | 14 ++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 13b2163e5d0b..3d47817d7b8f 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -224,10 +224,17 @@ const struct drm_format_info *__drm_format_info(u32 format)
 		{ .format = DRM_FORMAT_YVYU,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true },
 		{ .format = DRM_FORMAT_UYVY,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true },
 		{ .format = DRM_FORMAT_VYUY,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true },
+		{ .format = DRM_FORMAT_Y210,		.depth = 0,  .num_planes = 1,
+		  .cpp = { 4, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true },
+		{ .format = DRM_FORMAT_VUY888,		.depth = 0,  .num_planes = 1,
+		  .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1, .is_yuv = true },
+		{ .format = DRM_FORMAT_Y410,		.depth = 0,  .num_planes = 1,
+		  .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true, .is_yuv = true },
 		{ .format = DRM_FORMAT_AYUV,		.depth = 0,  .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true, .is_yuv = true },
 		{ .format = DRM_FORMAT_P010,		.depth = 0,  .num_planes = 2, .cpp = { 2, 4, 0 }, .hsub = 2, .vsub = 2, .is_yuv = true  },
 		{ .format = DRM_FORMAT_P012,		.depth = 0,  .num_planes = 2, .cpp = { 2, 4, 0 }, .hsub = 2, .vsub = 2, .is_yuv = true  },
 		{ .format = DRM_FORMAT_P016,		.depth = 0,  .num_planes = 2, .cpp = { 2, 4, 0 }, .hsub = 2, .vsub = 2, .is_yuv = true  },
+		{ .format = DRM_FORMAT_P210,		.depth = 0,  .num_planes = 2, .cpp = { 2, 4, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true },
 		{ .format = DRM_FORMAT_Y0L0,		.depth = 0,  .num_planes = 1,
 		  .char_per_block = { 8, 0, 0 }, .block_w = { 2, 0, 0 }, .block_h = { 2, 0, 0},
 		  .hsub = 2, .vsub = 2, .has_alpha = true, .is_yuv = true },
@@ -240,6 +247,12 @@ const struct drm_format_info *__drm_format_info(u32 format)
 		{ .format = DRM_FORMAT_X0L2,		.depth = 0,  .num_planes = 1,
 		  .char_per_block = { 8, 0, 0 }, .block_w = { 2, 0, 0 }, .block_h = { 2, 0, 0},
 		  .hsub = 2, .vsub = 2, .is_yuv = true },
+		{ .format = DRM_FORMAT_VUY101010,	.depth = 0,  .num_planes = 1,
+		  .cpp = { 0, 0, 0 }, .hsub = 1, .vsub = 1, .is_yuv = true },
+		{ .format = DRM_FORMAT_YUV420_8BIT,	.depth = 0,  .num_planes = 1,
+		  .cpp = { 0, 0, 0 }, .hsub = 2, .vsub = 2, .is_yuv = true },
+		{ .format = DRM_FORMAT_YUV420_10BIT,	.depth = 0,  .num_planes = 1,
+		  .cpp = { 0, 0, 0 }, .hsub = 2, .vsub = 2, .is_yuv = true },
 	};
 
 	unsigned int i;
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 4de86dbf40ca..f8396d1b8fbd 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -149,8 +149,12 @@ extern "C" {
 #define DRM_FORMAT_YVYU		fourcc_code('Y', 'V', 'Y', 'U') /* [31:0] Cb0:Y1:Cr0:Y0 8:8:8:8 little endian */
 #define DRM_FORMAT_UYVY		fourcc_code('U', 'Y', 'V', 'Y') /* [31:0] Y1:Cr0:Y0:Cb0 8:8:8:8 little endian */
 #define DRM_FORMAT_VYUY		fourcc_code('V', 'Y', 'U', 'Y') /* [31:0] Y1:Cb0:Y0:Cr0 8:8:8:8 little endian */
+#define DRM_FORMAT_Y210		fourcc_code('Y', '2', '1', '0') /* [63:0] Cr0:0:Y1:0:Cb0:0:Y0:0 10:6:10:6:10:6:10:6 little endian */
 
 #define DRM_FORMAT_AYUV		fourcc_code('A', 'Y', 'U', 'V') /* [31:0] A:Y:Cb:Cr 8:8:8:8 little endian */
+#define DRM_FORMAT_VUY888	fourcc_code('V', 'U', '2', '4') /* [23:0] Cr:Cb:Y 8:8:8 little endian */
+#define DRM_FORMAT_Y410		fourcc_code('Y', '4', '1', '0') /* [31:0] A:Cr:Y:Cb 2:10:10:10 little endian */
+#define DRM_FORMAT_VUY101010	fourcc_code('V', 'U', '3', '0') /* Y followed by U then V, 10:10:10. Non-linear modifier only */
 
 /*
  * packed YCbCr420 2x2 tiled formats
@@ -166,6 +170,15 @@ extern "C" {
 /* [63:0]   X3:X2:Y3:Cr0:Y2:X1:X0:Y1:Cb0:Y0  1:1:10:10:10:1:1:10:10:10 little endian */
 #define DRM_FORMAT_X0L2		fourcc_code('X', '0', 'L', '2')
 
+/*
+ * 1-plane YUV 4:2:0
+ * In these formats, the component ordering is specified (Y, followed by U
+ * then V), but the exact Linear layout is undefined.
+ * These formats can only be used with a non-Linear modifier.
+ */
+#define DRM_FORMAT_YUV420_8BIT	fourcc_code('Y', 'U', '0', '8')
+#define DRM_FORMAT_YUV420_10BIT	fourcc_code('Y', 'U', '1', '0')
+
 /*
  * 2 plane RGB + A
  * index 0 = RGB plane, same format as the corresponding non _A8 format has
@@ -200,6 +213,7 @@ extern "C" {
  * component xxx msb Y [xxx:16-xxx]
  * index 1 = Cr:Cb plane, [31:0] Cr:Cb little endian [xxx:16-xxx:xxx:16-xxx]
  */
+#define DRM_FORMAT_P210		fourcc_code('P', '2', '1', '0') /* 2x1 subsampled Cr:Cb plane, 10 bit per channel */
 #define DRM_FORMAT_P010		fourcc_code('P', '0', '1', '0') /* 2x2 subsampled Cr:Cb plane, 10 bit per channel */
 #define DRM_FORMAT_P012		fourcc_code('P', '0', '1', '2') /* 2x2 subsampled Cr:Cb plane, 12 bit per channel */
 #define DRM_FORMAT_P016		fourcc_code('P', '0', '1', '6') /* 2x2 subsampled Cr:Cb plane, 16 bit per channel */
-- 
2.18.0

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

[PATCH v3 6/6] drm/afbc: Add AFBC modifier usage documentation

[Alexandru-Cosmin Gheorghe]

From: Brian Starkey <brian.starkey@arm.com>

AFBC is a flexible, proprietary, lossless compression protocol and
format, with a number of defined DRM format modifiers. To facilitate
consistency and compatibility between different AFBC producers and
consumers, document the expectations for usage of the AFBC DRM format
modifiers in a new .rst chapter.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
---
 Documentation/gpu/afbc.rst    | 233 ++++++++++++++++++++++++++++++++++
 Documentation/gpu/drivers.rst |   1 +
 MAINTAINERS                   |   1 +
 include/uapi/drm/drm_fourcc.h |   3 +
 4 files changed, 238 insertions(+)
 create mode 100644 Documentation/gpu/afbc.rst

diff --git a/Documentation/gpu/afbc.rst b/Documentation/gpu/afbc.rst
new file mode 100644
index 000000000000..922d955da192
--- /dev/null
+++ b/Documentation/gpu/afbc.rst
@@ -0,0 +1,233 @@
+===================================
+ Arm Framebuffer Compression (AFBC)
+===================================
+
+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 can be enabled on drivers which support it via use of the AFBC
+format modifiers defined in drm_fourcc.h. See DRM_FORMAT_MOD_ARM_AFBC(*).
+
+All users of the AFBC modifiers must follow the usage guidelines laid
+out in this document, to ensure compatibility across different AFBC
+producers and consumers.
+
+Components and Ordering
+=======================
+
+AFBC streams can contain several components - where a component
+corresponds to a color channel (i.e. R, G, B, X, A, Y, Cb, Cr).
+The assignment of input/output color channels must be consistent
+between the encoder and the decoder for correct operation, otherwise
+the consumer will interpret the decoded data incorrectly.
+
+Furthermore, when the lossless colorspace transform is used
+(AFBC_FORMAT_MOD_YTR, which should be enabled for RGB buffers for
+maximum compression efficiency), the component order must be:
+
+ * Component 0: R
+ * Component 1: G
+ * Component 2: B
+
+The component ordering is communicated via the fourcc code in the
+fourcc:modifier pair. In general, component '0' is considered to
+reside in the least-significant bits of the corresponding linear
+format. For example, COMP(bits):
+
+ * DRM_FORMAT_ABGR8888
+
+   * Component 0: R(8)
+   * Component 1: G(8)
+   * Component 2: B(8)
+   * Component 3: A(8)
+
+ * DRM_FORMAT_BGR888
+
+   * Component 0: R(8)
+   * Component 1: G(8)
+   * Component 2: B(8)
+
+ * DRM_FORMAT_YUYV
+
+   * Component 0: Y(8)
+   * Component 1: Cb(8, 2x1 subsampled)
+   * Component 2: Cr(8, 2x1 subsampled)
+
+In AFBC, 'X' components are not treated any differently from any other
+component. Therefore, an AFBC buffer with fourcc DRM_FORMAT_XBGR8888
+encodes with 4 components, like so:
+
+ * DRM_FORMAT_XBGR8888
+
+   * Component 0: R(8)
+   * Component 1: G(8)
+   * Component 2: B(8)
+   * Component 3: X(8)
+
+Please note, however, that the inclusion of a "wasted" 'X' channel is
+bad for compression efficiency, and so it's recommended to avoid
+formats containing 'X' bits. If a fourth component is
+required/expected by the encoder/decoder, then it is recommended to
+instead use an equivalent format with alpha, setting all alpha bits to
+'1'. If there is no requirement for a fourth component, then a format
+which doesn't include alpha can be used, e.g. DRM_FORMAT_BGR888.
+
+Number of Planes
+================
+
+Formats which are typically multi-planar in linear layouts (e.g. YUV
+420), can be encoded into one, or multiple, AFBC planes. As with
+component order, the encoder and decoder must agree about the number
+of planes in order to correctly decode the buffer. The fourcc code is
+used to determine the number of encoded planes in an AFBC buffer,
+matching the number of planes for the linear (unmodified) format.
+Within each plane, the component ordering also follows the fourcc
+code:
+
+For example:
+
+ * DRM_FORMAT_YUYV: nplanes = 1
+
+   * Plane 0:
+
+     * Component 0: Y(8)
+     * Component 1: Cb(8, 2x1 subsampled)
+     * Component 2: Cr(8, 2x1 subsampled)
+
+ * DRM_FORMAT_NV12: nplanes = 2
+
+   * Plane 0:
+
+     * Component 0: Y(8)
+
+   * Plane 1:
+
+     * Component 0: Cb(8, 2x1 subsampled)
+     * Component 1: Cr(8, 2x1 subsampled)
+
+Cross-device interoperability
+=============================
+
+For maximum compatibility across devices, the table below defines
+canonical formats for use between AFBC-enabled devices. Formats which
+are listed here must be used exactly as specified when using the AFBC
+modifiers. Formats which are not listed should be avoided.
+
+.. flat-table:: AFBC formats
+
+   * - Fourcc code
+     - Description
+     - Planes/Components
+
+   * - DRM_FORMAT_ABGR2101010
+     - 10-bit per component RGB, with 2-bit alpha
+     - Plane 0: 4 components
+              * Component 0: R(10)
+              * Component 1: G(10)
+              * Component 2: B(10)
+              * Component 3: A(2)
+
+   * - DRM_FORMAT_ABGR8888
+     - 8-bit per component RGB, with 8-bit alpha
+     - Plane 0: 4 components
+              * Component 0: R(8)
+              * Component 1: G(8)
+              * Component 2: B(8)
+              * Component 3: A(8)
+
+   * - DRM_FORMAT_BGR888
+     - 8-bit per component RGB
+     - Plane 0: 3 components
+              * Component 0: R(8)
+              * Component 1: G(8)
+              * Component 2: B(8)
+
+   * - DRM_FORMAT_BGR565
+     - 5/6-bit per component RGB
+     - Plane 0: 3 components
+              * Component 0: R(5)
+              * Component 1: G(6)
+              * Component 2: B(5)
+
+   * - DRM_FORMAT_ABGR1555
+     - 5-bit per component RGB, with 1-bit alpha
+     - Plane 0: 4 components
+              * Component 0: R(5)
+              * Component 1: G(5)
+              * Component 2: B(5)
+              * Component 3: A(1)
+
+   * - DRM_FORMAT_VUY888
+     - 8-bit per component YCbCr 444, single plane
+     - Plane 0: 3 components
+              * Component 0: Y(8)
+              * Component 1: Cb(8)
+              * Component 2: Cr(8)
+
+   * - DRM_FORMAT_VUY101010
+     - 10-bit per component YCbCr 444, single plane
+     - Plane 0: 3 components
+              * Component 0: Y(10)
+              * Component 1: Cb(10)
+              * Component 2: Cr(10)
+
+   * - DRM_FORMAT_YUYV
+     - 8-bit per component YCbCr 422, single plane
+     - Plane 0: 3 components
+              * Component 0: Y(8)
+              * Component 1: Cb(8, 2x1 subsampled)
+              * Component 2: Cr(8, 2x1 subsampled)
+
+   * - DRM_FORMAT_NV16
+     - 8-bit per component YCbCr 422, two plane
+     - Plane 0: 1 component
+              * Component 0: Y(8)
+       Plane 1: 2 components
+              * Component 0: Cb(8, 2x1 subsampled)
+              * Component 1: Cr(8, 2x1 subsampled)
+
+   * - DRM_FORMAT_Y210
+     - 10-bit per component YCbCr 422, single plane
+     - Plane 0: 3 components
+              * Component 0: Y(10)
+              * Component 1: Cb(10, 2x1 subsampled)
+              * Component 2: Cr(10, 2x1 subsampled)
+
+   * - DRM_FORMAT_P210
+     - 10-bit per component YCbCr 422, two plane
+     - Plane 0: 1 component
+              * Component 0: Y(10)
+       Plane 1: 2 components
+              * Component 0: Cb(10, 2x1 subsampled)
+              * Component 1: Cr(10, 2x1 subsampled)
+
+   * - DRM_FORMAT_YUV420_8BIT
+     - 8-bit per component YCbCr 420, single plane
+     - Plane 0: 3 components
+              * Component 0: Y(8)
+              * Component 1: Cb(8, 2x2 subsampled)
+              * Component 2: Cr(8, 2x2 subsampled)
+
+   * - DRM_FORMAT_YUV420_10BIT
+     - 10-bit per component YCbCr 420, single plane
+     - Plane 0: 3 components
+              * Component 0: Y(10)
+              * Component 1: Cb(10, 2x2 subsampled)
+              * Component 2: Cr(10, 2x2 subsampled)
+
+   * - DRM_FORMAT_NV12
+     - 8-bit per component YCbCr 420, two plane
+     - Plane 0: 1 component
+              * Component 0: Y(8)
+       Plane 1: 2 components
+              * Component 0: Cb(8, 2x2 subsampled)
+              * Component 1: Cr(8, 2x2 subsampled)
+
+   * - DRM_FORMAT_P010
+     - 10-bit per component YCbCr 420, two plane
+     - Plane 0: 1 component
+              * Component 0: Y(10)
+       Plane 1: 2 components
+              * Component 0: Cb(10, 2x2 subsampled)
+              * Component 1: Cr(10, 2x2 subsampled)
diff --git a/Documentation/gpu/drivers.rst b/Documentation/gpu/drivers.rst
index 7d2d3875ff1a..8ec755024390 100644
--- a/Documentation/gpu/drivers.rst
+++ b/Documentation/gpu/drivers.rst
@@ -16,6 +16,7 @@ GPU Driver Documentation
    vkms
    bridge/dw-hdmi
    xen-front
+   afbc
 
 .. only::  subproject and html
 
diff --git a/MAINTAINERS b/MAINTAINERS
index 39c3f6682ace..4d3a8822b4c5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1091,6 +1091,7 @@ M:	Mali DP Maintainers <malidp@foss.arm.com>
 S:	Supported
 F:	drivers/gpu/drm/arm/
 F:	Documentation/devicetree/bindings/display/arm,malidp.txt
+F:	Documentation/gpu/afbc.rst
 
 ARM MFM AND FLOPPY DRIVERS
 M:	Ian Molton <spyro@f2s.com>
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index f8396d1b8fbd..b2fb2d53fff7 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -586,6 +586,9 @@ extern "C" {
  * 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.
+ *
+ * Further information on the use of AFBC modifiers can be found in
+ * Documentation/gpu/afbc.rst
  */
 #define DRM_FORMAT_MOD_ARM_AFBC(__afbc_mode)	fourcc_mod_code(ARM, __afbc_mode)
 
-- 
2.18.0

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

Re: [PATCH v3 1/6] drm/fourcc: Add char_per_block, block_w and block_h in drm_format_info

[Daniel Vetter]

On Fri, Oct 05, 2018 at 09:26:47AM +0000, Alexandru-Cosmin Gheorghe wrote:
> For some pixel formats .cpp structure in drm_format info it's not
> enough to describe the peculiarities of the pixel layout, for example
> tiled formats or packed formats at bit level.
> 
> What's implemented here is to add three new members to drm_format_info
> that could describe such formats:
> 
> - char_per_block[3]
> - block_w[3]
> - block_h[3]
> 
> char_per_block will be put in a union alongside cpp, for transparent
> compatibility  with the existing format descriptions.
> 
> Regarding, block_w and block_h they are intended to be used through
> their equivalent getters drm_format_info_block_width /
> drm_format_info_block_height, the reason of the getters is to abstract
> the fact that for normal formats block_w and block_h will be unset/0,
> but the methods will be returning 1.
> 
> Using that the following drm core functions had been updated to
> generically handle both block and non-block formats:
> 
> - drm_fb_cma_get_gem_addr: for block formats it will just return the
>   beginning of the block.
> - framebuffer_check: Updated to compute the minimum pitch as
> 	DIV_ROUND_UP(width * char_per_block, drm_format_info_block_width(info, i)
> 	* drm_format_info_block_height(info, i))
> - drm_gem_fb_create_with_funcs: Updated to compute the size of the
>   last line of pixels with the same formula as for the minimum pitch.
> - In places where is not expecting to handle block formats, like fbdev
>   helpers I just added some warnings in case the block width/height
>   are greater than 1.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> ---
>  drivers/gpu/drm/drm_fb_cma_helper.c          | 21 ++++++++--
>  drivers/gpu/drm/drm_fb_helper.c              |  6 +++
>  drivers/gpu/drm/drm_fourcc.c                 | 40 ++++++++++++++++++++
>  drivers/gpu/drm/drm_framebuffer.c            |  9 +++--
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c |  4 +-
>  include/drm/drm_fourcc.h                     | 29 +++++++++++++-
>  6 files changed, 101 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index 47e0e2f6642d..f8293f4d96cf 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -72,7 +72,9 @@ struct drm_gem_cma_object *drm_fb_cma_get_gem_obj(struct drm_framebuffer *fb,
>  EXPORT_SYMBOL_GPL(drm_fb_cma_get_gem_obj);
>  
>  /**
> - * drm_fb_cma_get_gem_addr() - Get physical address for framebuffer
> + * drm_fb_cma_get_gem_addr() - Get physical address for framebuffer, for pixel
> + * formats where values are grouped in blocks this will get you the beginning of
> + * the block
>   * @fb: The framebuffer
>   * @state: Which state of drm plane
>   * @plane: Which plane
> @@ -87,6 +89,14 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
>  	struct drm_gem_cma_object *obj;
>  	dma_addr_t paddr;
>  	u8 h_div = 1, v_div = 1;
> +	u32 block_w = drm_format_info_block_width(fb->format, plane);
> +	u32 block_h = drm_format_info_block_height(fb->format, plane);
> +	u32 block_size = fb->format->char_per_block[plane];
> +	u32 sample_x;
> +	u32 sample_y;
> +	u32 block_start_y;
> +	u32 num_hblocks;
> +
>  
>  	obj = drm_fb_cma_get_gem_obj(fb, plane);
>  	if (!obj)
> @@ -99,8 +109,13 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
>  		v_div = fb->format->vsub;
>  	}
>  
> -	paddr += (fb->format->cpp[plane] * (state->src_x >> 16)) / h_div;
> -	paddr += (fb->pitches[plane] * (state->src_y >> 16)) / v_div;
> +	sample_x = (state->src_x >> 16) / h_div;
> +	sample_y = (state->src_y >> 16) / v_div;
> +	block_start_y = (sample_y / block_h) * block_h;
> +	num_hblocks = sample_x / block_w;
> +
> +	paddr += fb->pitches[plane] * block_start_y;
> +	paddr += block_size * num_hblocks;
>  
>  	return paddr;
>  }
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index a504a5e05676..9add0d7da744 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1595,6 +1595,10 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>  	if (var->pixclock != 0 || in_dbg_master())
>  		return -EINVAL;
>  
> +	if ((drm_format_info_block_width(fb->format, 0) > 1) ||
> +	    (drm_format_info_block_height(fb->format, 0) > 1))
> +		return -EINVAL;
> +
>  	/*
>  	 * Changes struct fb_var_screeninfo are currently not pushed back
>  	 * to KMS, hence fail if different settings are requested.
> @@ -1969,6 +1973,8 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
>  {
>  	struct drm_framebuffer *fb = fb_helper->fb;
>  
> +	WARN_ON((drm_format_info_block_width(fb->format, 0) > 1) ||
> +		(drm_format_info_block_height(fb->format, 0) > 1));
>  	info->pseudo_palette = fb_helper->pseudo_palette;
>  	info->var.xres_virtual = fb->width;
>  	info->var.yres_virtual = fb->height;
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index a154763257ae..0c9725ffd5e9 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -403,3 +403,43 @@ int drm_format_plane_height(int height, uint32_t format, int plane)
>  	return height / info->vsub;
>  }
>  EXPORT_SYMBOL(drm_format_plane_height);
> +
> +/**
> + * drm_format_info_block_width - width in pixels of block.
> + * @info: pixel format info
> + * @plane: plane index
> + *
> + * Returns:
> + * The width in pixels of a block, depending on the plane index.
> + */
> +unsigned int drm_format_info_block_width(const struct drm_format_info *info,
> +					 int plane)
> +{
> +	if (!info || plane >= info->num_planes)
> +		return 0;
> +
> +	if (!info->block_w[plane])
> +		return 1;
> +	return info->block_w[plane];
> +}
> +EXPORT_SYMBOL(drm_format_info_block_width);
> +
> +/**
> + * drm_format_info_block_height - height in pixels of a block
> + * @info: pixel format info
> + * @plane: plane index
> + *
> + * Returns:
> + * The height in pixels of a block, depending on the plane index.
> + */
> +unsigned int drm_format_info_block_height(const struct drm_format_info *info,
> +					  int plane)
> +{
> +	if (!info || plane >= info->num_planes)
> +		return 0;
> +
> +	if (!info->block_h[plane])
> +		return 1;
> +	return info->block_h[plane];
> +}
> +EXPORT_SYMBOL(drm_format_info_block_height);
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 3bf729d0aae5..4e14013788cd 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -195,20 +195,23 @@ static int framebuffer_check(struct drm_device *dev,
>  	for (i = 0; i < info->num_planes; i++) {
>  		unsigned int width = fb_plane_width(r->width, info, i);
>  		unsigned int height = fb_plane_height(r->height, info, i);
> -		unsigned int cpp = info->cpp[i];
> +		unsigned int block_size = info->char_per_block[i];
> +		u64 min_pitch = DIV_ROUND_UP((u64)width * block_size,
> +					drm_format_info_block_width(info, i) *
> +					drm_format_info_block_height(info, i));

I think a helper to compute the drm_pitch, including some kernel-doc that
explains what exactly it is (and why) would be good. For cleaner code,
less duplication and to have a good spot for docs.

>  
>  		if (!r->handles[i]) {
>  			DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i);
>  			return -EINVAL;
>  		}
>  
> -		if ((uint64_t) width * cpp > UINT_MAX)
> +		if (min_pitch > UINT_MAX)
>  			return -ERANGE;
>  
>  		if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
>  			return -ERANGE;
>  
> -		if (r->pitches[i] < width * cpp) {
> +		if (r->pitches[i] < min_pitch) {
>  			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
>  			return -EINVAL;
>  		}
> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> index ded7a379ac35..e342511370e7 100644
> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -171,7 +171,9 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
>  		}
>  
>  		min_size = (height - 1) * mode_cmd->pitches[i]
> -			 + width * info->cpp[i]
> +			 + DIV_ROUND_UP((u64)width * info->char_per_block[i],
> +					drm_format_info_block_width(info, i) *
> +					drm_format_info_block_height(info, i))
>  			 + mode_cmd->offsets[i];
>  
>  		if (objs[i]->size < min_size) {
> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> index 865ef60c17af..305e80847abc 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -58,6 +58,24 @@ struct drm_mode_fb_cmd2;
>   *	use in new code and set to 0 for new formats.
>   * @num_planes: Number of color planes (1 to 3)
>   * @cpp: Number of bytes per pixel (per plane)

Need to make it clear this aliases with @char_per_block and is deprecated.
Same for @char_per_block.

> + * @char_per_block:  Number of bytes per block (per plane), where blocks are
> + *	defined as a rectangle of pixels which are stored next to each other in
> + *	a byte aligned memory region.
> + *	Together with block_w and block_h this could be used to properly
s/could/is/

> + *	describe tiles in tiled formats or to describe groups of pixels in
> + *	packed formats for which the memory needed for a single pixel it's not
> + *	byte aligned.
> + *      Cpp had been kept from historical reasons because there are a lot of

s/Cpp/@cpp/ for proper highlighting in the output.

> + *	places in drivers where it's used. In drm core for generic code paths
> + *	the preferred way is to use char_per_block, drm_format_info_block_width
@char_per_block and drm_format_info_block_width() for hyperlinks. Similar
with all the other occurences of a function name.

> + *	and drm_format_info_block_height which should allow handling both block

s/should// - I hope your code actually works :-)

> + *	and non-block formats in the same way.
> + *	For formats that are intended to be used just with modifiers, cpp/
> + *	char_per_block must be 0.

This is a bit vague imo. What about:

"For formats that are intended to be used only with non-linear modifiers,
both @cpp and @char_per_block must be 0 in the generic format table.
Drivers should supply accurate information from their
&drm_mode_config.get_format_info hook though."

Since I'm assuming that once you know the exact modifier, you can enlist
the core format checking code to help validate it.

> + * @block_w: Block width in pixels, this is intended to be accessed through
> + *	drm_format_info_block_width

Since you alias cpp and char_per_block, pls make it really, really clear
that if this isn't set, then the code uses 1 as the default.

> + * @block_h: Block height in pixels, this is intended to be accessed through
> + *	drm_format_info_block_height

Same here.

Please use the inline kerneldoc style, it's much more readable for long
stuff like here. If the inconsistency irks you, then please just move them
all to the inline style. Inline style also allows you to have proper
paragraph breaks with empty lines.

>   * @hsub: Horizontal chroma subsampling factor
>   * @vsub: Vertical chroma subsampling factor
>   * @has_alpha: Does the format embeds an alpha component?
> @@ -67,7 +85,12 @@ struct drm_format_info {
>  	u32 format;
>  	u8 depth;
>  	u8 num_planes;
> -	u8 cpp[3];
> +	union {
> +		u8 cpp[3];
> +		u8 char_per_block[3];
> +	};
> +	u8 block_w[3];
> +	u8 block_h[3];
>  	u8 hsub;
>  	u8 vsub;
>  	bool has_alpha;
> @@ -96,6 +119,10 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
>  int drm_format_vert_chroma_subsampling(uint32_t format);
>  int drm_format_plane_width(int width, uint32_t format, int plane);
>  int drm_format_plane_height(int height, uint32_t format, int plane);
> +unsigned int drm_format_info_block_width(const struct drm_format_info *info,
> +					 int plane);
> +unsigned int drm_format_info_block_height(const struct drm_format_info *info,
> +					  int plane);
>  const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
>  
>  #endif /* __DRM_FOURCC_H__ */

With the polish addressed, this lgtm.

One question I have is validating this. I think a bunch of unit tests,
integrated into the existing kms selftests we already (so that
intel-gfx-ci and other CI can run it) would be great. Both for the small
helper functions (block width/height, but especially drm_pitch), but also
for the drm_framebuffer_create functions, so that we can exercise the
metric pile of validation corner cases.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/6] drm: Extend framebuffer_check to handle formats with cpp/char_per_block 0

[Daniel Vetter]

On Fri, Oct 05, 2018 at 09:26:57AM +0000, Alexandru-Cosmin Gheorghe wrote:
> For formats that are supported only with non-linear modifiers it
> doesn't make to much sense to define cpp or char_per_block, so that
> will be set to 0.
> 
> This patch adds a restriction to force having a modifier attached when
> cpp/char_per_block is 0, and to bypass checking the pitch restriction.
> 
> This had been discussed here.
> [1] https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2018-09-13&show_html=true
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Can we require that the driver supplies a more accurate drm_format_info
through get_format_into in these cases? I'm hoping that works, and the
afbc stuff isn't entirely random alignment :-)

Then we wouldn't need these escape hatches here I think.
-Daniel

> ---
>  drivers/gpu/drm/drm_framebuffer.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 4e14013788cd..66ec126b7faf 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -200,6 +200,10 @@ static int framebuffer_check(struct drm_device *dev,
>  					drm_format_info_block_width(info, i) *
>  					drm_format_info_block_height(info, i));
>  
> +		if (!block_size && (r->modifier[i] == DRM_FORMAT_MOD_LINEAR)) {
> +			DRM_DEBUG_KMS("Format requires non-linear modifier for plane %d\n", i);
> +			return -EINVAL;
> +		}
>  		if (!r->handles[i]) {
>  			DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i);
>  			return -EINVAL;
> @@ -211,7 +215,7 @@ static int framebuffer_check(struct drm_device *dev,
>  		if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
>  			return -ERANGE;
>  
> -		if (r->pitches[i] < min_pitch) {
> +		if (block_size && r->pitches[i] < min_pitch) {
>  			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
>  			return -EINVAL;
>  		}
> -- 
> 2.18.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 0/6] Add method to describe tile/bit_level_packed formats

[Daniel Vetter]

On Fri, Oct 05, 2018 at 09:26:43AM +0000, Alexandru-Cosmin Gheorghe wrote:
> There has been some discussion about extending drm core to handle
> linear tile formats, in the series sent by me here [1] and how to
> handle formats that are intended to be used just with
> modifiers(particularly AFBC modifiers) on Brian series [2] and on IRC
> here [3] and [4].
> 
> Hence, this big-merged series:
> 
> Patches 1-3: handle tiled formats both in core and in malidp driver,
> this is done by extending drm_format_info with three new fields
> char_per_block, block_w, block_h and consistently handle in the generic
> code paths, both linear tiled formats and normal formats.
> What's different from [1] is the interpretation of pitch for tile
> formats which has been kept to be the same as for the other formats:
> pitch = average_chars_per_pixel * width.
> 
> Patches 4-6: Introduce the YUV AFBC formats, the only thing noteworthy
> here is that cpp/char_per_block are set to 0 for formats where it's
> mandatory to be used together with a non-linear modifier and then that
> is used to bypass pitch check in framebuffer_check for formats that
> have cpp/char_per_block set to 0.
> 
> [1] https://lists.freedesktop.org/archives/dri-devel/2018-September/188245.html
> [2] https://lists.freedesktop.org/archives/dri-devel/2018-September/189620.html
> [3] https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2018-09-13&show_html=true
> [4] https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2018-09-14&show_html=true
> 
> Alexandru Gheorghe (4):
>   drm/fourcc: Add char_per_block, block_w and block_h in drm_format_info
>   drm/fourcc: Add fourcc for Mali linear tiled formats
>   drm: mali-dp: Enable Mali-DP tiled buffer formats
>   drm: Extend framebuffer_check to handle formats with
>     cpp/char_per_block 0
> 
> Brian Starkey (2):
>   drm/fourcc: Add AFBC yuv fourccs for Mali
>   drm/afbc: Add AFBC modifier usage documentation

I think at the design level we're getting there, yay! Bunch of
comments/suggestions/ideas on patch one.

It looks like the patch 7 is missing though, since the fancy formats where
you have no linear layout isn't enabled in mali-dp (afaics at least).
Depending what those look like, my suggestion to require drivers to use
->get_format_info instead might or might not work.

Cheers, Daniel

> 
>  Documentation/gpu/afbc.rst                   | 233 +++++++++++++++++++
>  Documentation/gpu/drivers.rst                |   1 +
>  MAINTAINERS                                  |   1 +
>  drivers/gpu/drm/arm/malidp_hw.c              |  14 +-
>  drivers/gpu/drm/arm/malidp_planes.c          |  23 +-
>  drivers/gpu/drm/drm_fb_cma_helper.c          |  21 +-
>  drivers/gpu/drm/drm_fb_helper.c              |   6 +
>  drivers/gpu/drm/drm_fourcc.c                 |  65 ++++++
>  drivers/gpu/drm/drm_framebuffer.c            |  13 +-
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c |   4 +-
>  include/drm/drm_fourcc.h                     |  29 ++-
>  include/uapi/drm/drm_fourcc.h                |  31 +++
>  12 files changed, 428 insertions(+), 13 deletions(-)
>  create mode 100644 Documentation/gpu/afbc.rst
> 
> -- 
> 2.18.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/6] drm/fourcc: Add char_per_block, block_w and block_h in drm_format_info

[Alexandru-Cosmin Gheorghe]

Hi Daniel,

Thanks for having a look.

On Fri, Oct 05, 2018 at 04:51:48PM +0200, Daniel Vetter wrote:
> On Fri, Oct 05, 2018 at 09:26:47AM +0000, Alexandru-Cosmin Gheorghe wrote:
> > For some pixel formats .cpp structure in drm_format info it's not
> > enough to describe the peculiarities of the pixel layout, for example
> > tiled formats or packed formats at bit level.
> > 
> > What's implemented here is to add three new members to drm_format_info
> > that could describe such formats:
> > 
> > - char_per_block[3]
> > - block_w[3]
> > - block_h[3]
> > 
> > char_per_block will be put in a union alongside cpp, for transparent
> > compatibility  with the existing format descriptions.
> > 
> > Regarding, block_w and block_h they are intended to be used through
> > their equivalent getters drm_format_info_block_width /
> > drm_format_info_block_height, the reason of the getters is to abstract
> > the fact that for normal formats block_w and block_h will be unset/0,
> > but the methods will be returning 1.
> > 
> > Using that the following drm core functions had been updated to
> > generically handle both block and non-block formats:
> > 
> > - drm_fb_cma_get_gem_addr: for block formats it will just return the
> >   beginning of the block.
> > - framebuffer_check: Updated to compute the minimum pitch as
> > 	DIV_ROUND_UP(width * char_per_block, drm_format_info_block_width(info, i)
> > 	* drm_format_info_block_height(info, i))
> > - drm_gem_fb_create_with_funcs: Updated to compute the size of the
> >   last line of pixels with the same formula as for the minimum pitch.
> > - In places where is not expecting to handle block formats, like fbdev
> >   helpers I just added some warnings in case the block width/height
> >   are greater than 1.
> > 
> > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> > ---
> >  drivers/gpu/drm/drm_fb_cma_helper.c          | 21 ++++++++--
> >  drivers/gpu/drm/drm_fb_helper.c              |  6 +++
> >  drivers/gpu/drm/drm_fourcc.c                 | 40 ++++++++++++++++++++
> >  drivers/gpu/drm/drm_framebuffer.c            |  9 +++--
> >  drivers/gpu/drm/drm_gem_framebuffer_helper.c |  4 +-
> >  include/drm/drm_fourcc.h                     | 29 +++++++++++++-
> >  6 files changed, 101 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> > index 47e0e2f6642d..f8293f4d96cf 100644
> > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > @@ -72,7 +72,9 @@ struct drm_gem_cma_object *drm_fb_cma_get_gem_obj(struct drm_framebuffer *fb,
> >  EXPORT_SYMBOL_GPL(drm_fb_cma_get_gem_obj);
> >  
> >  /**
> > - * drm_fb_cma_get_gem_addr() - Get physical address for framebuffer
> > + * drm_fb_cma_get_gem_addr() - Get physical address for framebuffer, for pixel
> > + * formats where values are grouped in blocks this will get you the beginning of
> > + * the block
> >   * @fb: The framebuffer
> >   * @state: Which state of drm plane
> >   * @plane: Which plane
> > @@ -87,6 +89,14 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
> >  	struct drm_gem_cma_object *obj;
> >  	dma_addr_t paddr;
> >  	u8 h_div = 1, v_div = 1;
> > +	u32 block_w = drm_format_info_block_width(fb->format, plane);
> > +	u32 block_h = drm_format_info_block_height(fb->format, plane);
> > +	u32 block_size = fb->format->char_per_block[plane];
> > +	u32 sample_x;
> > +	u32 sample_y;
> > +	u32 block_start_y;
> > +	u32 num_hblocks;
> > +
> >  
> >  	obj = drm_fb_cma_get_gem_obj(fb, plane);
> >  	if (!obj)
> > @@ -99,8 +109,13 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
> >  		v_div = fb->format->vsub;
> >  	}
> >  
> > -	paddr += (fb->format->cpp[plane] * (state->src_x >> 16)) / h_div;
> > -	paddr += (fb->pitches[plane] * (state->src_y >> 16)) / v_div;
> > +	sample_x = (state->src_x >> 16) / h_div;
> > +	sample_y = (state->src_y >> 16) / v_div;
> > +	block_start_y = (sample_y / block_h) * block_h;
> > +	num_hblocks = sample_x / block_w;
> > +
> > +	paddr += fb->pitches[plane] * block_start_y;
> > +	paddr += block_size * num_hblocks;
> >  
> >  	return paddr;
> >  }
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index a504a5e05676..9add0d7da744 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -1595,6 +1595,10 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> >  	if (var->pixclock != 0 || in_dbg_master())
> >  		return -EINVAL;
> >  
> > +	if ((drm_format_info_block_width(fb->format, 0) > 1) ||
> > +	    (drm_format_info_block_height(fb->format, 0) > 1))
> > +		return -EINVAL;
> > +
> >  	/*
> >  	 * Changes struct fb_var_screeninfo are currently not pushed back
> >  	 * to KMS, hence fail if different settings are requested.
> > @@ -1969,6 +1973,8 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
> >  {
> >  	struct drm_framebuffer *fb = fb_helper->fb;
> >  
> > +	WARN_ON((drm_format_info_block_width(fb->format, 0) > 1) ||
> > +		(drm_format_info_block_height(fb->format, 0) > 1));
> >  	info->pseudo_palette = fb_helper->pseudo_palette;
> >  	info->var.xres_virtual = fb->width;
> >  	info->var.yres_virtual = fb->height;
> > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > index a154763257ae..0c9725ffd5e9 100644
> > --- a/drivers/gpu/drm/drm_fourcc.c
> > +++ b/drivers/gpu/drm/drm_fourcc.c
> > @@ -403,3 +403,43 @@ int drm_format_plane_height(int height, uint32_t format, int plane)
> >  	return height / info->vsub;
> >  }
> >  EXPORT_SYMBOL(drm_format_plane_height);
> > +
> > +/**
> > + * drm_format_info_block_width - width in pixels of block.
> > + * @info: pixel format info
> > + * @plane: plane index
> > + *
> > + * Returns:
> > + * The width in pixels of a block, depending on the plane index.
> > + */
> > +unsigned int drm_format_info_block_width(const struct drm_format_info *info,
> > +					 int plane)
> > +{
> > +	if (!info || plane >= info->num_planes)
> > +		return 0;
> > +
> > +	if (!info->block_w[plane])
> > +		return 1;
> > +	return info->block_w[plane];
> > +}
> > +EXPORT_SYMBOL(drm_format_info_block_width);
> > +
> > +/**
> > + * drm_format_info_block_height - height in pixels of a block
> > + * @info: pixel format info
> > + * @plane: plane index
> > + *
> > + * Returns:
> > + * The height in pixels of a block, depending on the plane index.
> > + */
> > +unsigned int drm_format_info_block_height(const struct drm_format_info *info,
> > +					  int plane)
> > +{
> > +	if (!info || plane >= info->num_planes)
> > +		return 0;
> > +
> > +	if (!info->block_h[plane])
> > +		return 1;
> > +	return info->block_h[plane];
> > +}
> > +EXPORT_SYMBOL(drm_format_info_block_height);
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > index 3bf729d0aae5..4e14013788cd 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -195,20 +195,23 @@ static int framebuffer_check(struct drm_device *dev,
> >  	for (i = 0; i < info->num_planes; i++) {
> >  		unsigned int width = fb_plane_width(r->width, info, i);
> >  		unsigned int height = fb_plane_height(r->height, info, i);
> > -		unsigned int cpp = info->cpp[i];
> > +		unsigned int block_size = info->char_per_block[i];
> > +		u64 min_pitch = DIV_ROUND_UP((u64)width * block_size,
> > +					drm_format_info_block_width(info, i) *
> > +					drm_format_info_block_height(info, i));
> 
> I think a helper to compute the drm_pitch, including some kernel-doc that
> explains what exactly it is (and why) would be good. For cleaner code,
> less duplication and to have a good spot for docs.

Stark3y had the same suggestion during our internal review :).
Does this sounds good to you?
drm_format_info_min_pitch(info, plane, buffer_width)

> 
> >  
> >  		if (!r->handles[i]) {
> >  			DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i);
> >  			return -EINVAL;
> >  		}
> >  
> > -		if ((uint64_t) width * cpp > UINT_MAX)
> > +		if (min_pitch > UINT_MAX)
> >  			return -ERANGE;
> >  
> >  		if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
> >  			return -ERANGE;
> >  
> > -		if (r->pitches[i] < width * cpp) {
> > +		if (r->pitches[i] < min_pitch) {
> >  			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
> >  			return -EINVAL;
> >  		}
> > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > index ded7a379ac35..e342511370e7 100644
> > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > @@ -171,7 +171,9 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
> >  		}
> >  
> >  		min_size = (height - 1) * mode_cmd->pitches[i]
> > -			 + width * info->cpp[i]
> > +			 + DIV_ROUND_UP((u64)width * info->char_per_block[i],
> > +					drm_format_info_block_width(info, i) *
> > +					drm_format_info_block_height(info, i))
> >  			 + mode_cmd->offsets[i];
> >  
> >  		if (objs[i]->size < min_size) {
> > diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> > index 865ef60c17af..305e80847abc 100644
> > --- a/include/drm/drm_fourcc.h
> > +++ b/include/drm/drm_fourcc.h
> > @@ -58,6 +58,24 @@ struct drm_mode_fb_cmd2;
> >   *	use in new code and set to 0 for new formats.
> >   * @num_planes: Number of color planes (1 to 3)
> >   * @cpp: Number of bytes per pixel (per plane)
> 
> Need to make it clear this aliases with @char_per_block and is deprecated.
> Same for @char_per_block.
> 
> > + * @char_per_block:  Number of bytes per block (per plane), where blocks are
> > + *	defined as a rectangle of pixels which are stored next to each other in
> > + *	a byte aligned memory region.
> > + *	Together with block_w and block_h this could be used to properly
> s/could/is/
> 
> > + *	describe tiles in tiled formats or to describe groups of pixels in
> > + *	packed formats for which the memory needed for a single pixel it's not
> > + *	byte aligned.
> > + *      Cpp had been kept from historical reasons because there are a lot of
> 
> s/Cpp/@cpp/ for proper highlighting in the output.
> 
> > + *	places in drivers where it's used. In drm core for generic code paths
> > + *	the preferred way is to use char_per_block, drm_format_info_block_width
> @char_per_block and drm_format_info_block_width() for hyperlinks. Similar
> with all the other occurences of a function name.
> 
> > + *	and drm_format_info_block_height which should allow handling both block
> 
> s/should// - I hope your code actually works :-)
> 
> > + *	and non-block formats in the same way.
> > + *	For formats that are intended to be used just with modifiers, cpp/
> > + *	char_per_block must be 0.
> 
> This is a bit vague imo. What about:
> 
> "For formats that are intended to be used only with non-linear modifiers,
> both @cpp and @char_per_block must be 0 in the generic format table.
> Drivers should supply accurate information from their
> &drm_mode_config.get_format_info hook though."
> 
> Since I'm assuming that once you know the exact modifier, you can enlist
> the core format checking code to help validate it.
> 
> > + * @block_w: Block width in pixels, this is intended to be accessed through
> > + *	drm_format_info_block_width
> 
> Since you alias cpp and char_per_block, pls make it really, really clear
> that if this isn't set, then the code uses 1 as the default.
> 
> > + * @block_h: Block height in pixels, this is intended to be accessed through
> > + *	drm_format_info_block_height
> 
> Same here.
> 
> Please use the inline kerneldoc style, it's much more readable for long
> stuff like here. If the inconsistency irks you, then please just move them
> all to the inline style. Inline style also allows you to have proper
> paragraph breaks with empty lines.
> 
> >   * @hsub: Horizontal chroma subsampling factor
> >   * @vsub: Vertical chroma subsampling factor
> >   * @has_alpha: Does the format embeds an alpha component?
> > @@ -67,7 +85,12 @@ struct drm_format_info {
> >  	u32 format;
> >  	u8 depth;
> >  	u8 num_planes;
> > -	u8 cpp[3];
> > +	union {
> > +		u8 cpp[3];
> > +		u8 char_per_block[3];
> > +	};
> > +	u8 block_w[3];
> > +	u8 block_h[3];
> >  	u8 hsub;
> >  	u8 vsub;
> >  	bool has_alpha;
> > @@ -96,6 +119,10 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
> >  int drm_format_vert_chroma_subsampling(uint32_t format);
> >  int drm_format_plane_width(int width, uint32_t format, int plane);
> >  int drm_format_plane_height(int height, uint32_t format, int plane);
> > +unsigned int drm_format_info_block_width(const struct drm_format_info *info,
> > +					 int plane);
> > +unsigned int drm_format_info_block_height(const struct drm_format_info *info,
> > +					  int plane);
> >  const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
> >  
> >  #endif /* __DRM_FOURCC_H__ */
> 
> With the polish addressed, this lgtm.

All of them are reasonable to me, so I'll apply them in the next
version.

> 
> One question I have is validating this. I think a bunch of unit tests,
> integrated into the existing kms selftests we already (so that
> intel-gfx-ci and other CI can run it) would be great. Both for the small
> helper functions (block width/height, but especially drm_pitch), but also
> for the drm_framebuffer_create functions, so that we can exercise the
> metric pile of validation corner cases.
> -Daniel

So, you are thinking of adding tests here drivers/gpu/drm/selftests/ ?
Looking inside the folder, it seems more like a stub than proper
test suite for the drm framework, or am I missing something ?

I did run tests for all supported formats by the mali-dp, with our
internal testsuite and everything was OK for all the fourcc enabled
in mali-dp

I'm planning to run the igt on top of mali-dp, just to double check I
didn't change any of the behaviours. Any idea if there is any public
igt CI that I could point to a kernel branch, to test this change on
something else other than mali-dp?

Shouldn't that be enough for validating this patches? 


> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH v3 0/6] Add method to describe tile/bit_level_packed formats

[Alexandru-Cosmin Gheorghe]

On Fri, Oct 05, 2018 at 04:58:38PM +0200, Daniel Vetter wrote:
> On Fri, Oct 05, 2018 at 09:26:43AM +0000, Alexandru-Cosmin Gheorghe wrote:
> > There has been some discussion about extending drm core to handle
> > linear tile formats, in the series sent by me here [1] and how to
> > handle formats that are intended to be used just with
> > modifiers(particularly AFBC modifiers) on Brian series [2] and on IRC
> > here [3] and [4].
> > 
> > Hence, this big-merged series:
> > 
> > Patches 1-3: handle tiled formats both in core and in malidp driver,
> > this is done by extending drm_format_info with three new fields
> > char_per_block, block_w, block_h and consistently handle in the generic
> > code paths, both linear tiled formats and normal formats.
> > What's different from [1] is the interpretation of pitch for tile
> > formats which has been kept to be the same as for the other formats:
> > pitch = average_chars_per_pixel * width.
> > 
> > Patches 4-6: Introduce the YUV AFBC formats, the only thing noteworthy
> > here is that cpp/char_per_block are set to 0 for formats where it's
> > mandatory to be used together with a non-linear modifier and then that
> > is used to bypass pitch check in framebuffer_check for formats that
> > have cpp/char_per_block set to 0.
> > 
> > [1] https://lists.freedesktop.org/archives/dri-devel/2018-September/188245.html
> > [2] https://lists.freedesktop.org/archives/dri-devel/2018-September/189620.html
> > [3] https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2018-09-13&show_html=true
> > [4] https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2018-09-14&show_html=true
> > 
> > Alexandru Gheorghe (4):
> >   drm/fourcc: Add char_per_block, block_w and block_h in drm_format_info
> >   drm/fourcc: Add fourcc for Mali linear tiled formats
> >   drm: mali-dp: Enable Mali-DP tiled buffer formats
> >   drm: Extend framebuffer_check to handle formats with
> >     cpp/char_per_block 0
> > 
> > Brian Starkey (2):
> >   drm/fourcc: Add AFBC yuv fourccs for Mali
> >   drm/afbc: Add AFBC modifier usage documentation
> 
> I think at the design level we're getting there, yay! Bunch of
> comments/suggestions/ideas on patch one.
> 
> It looks like the patch 7 is missing though, since the fancy formats where
> you have no linear layout isn't enabled in mali-dp (afaics at least).
> Depending what those look like, my suggestion to require drivers to use
> ->get_format_info instead might or might not work.

Yes, patch 7 is missing, Ayan is working on that, I'm not sure if on
next version we should send one big series, with that as well, or
split it in two, by keeping the first three patches in a separate
series, any preferences ?

Anyway, regarding using get_format_info to trick check_framebuffer to
validate the pitch, the driver could invent some numbers to populate
a drm_format_info, but it doesn't make too much sense because we are 
more restrictive than that so the driver will need to do some sort of
validation anyway.

> 
> Cheers, Daniel
> 
> > 
> >  Documentation/gpu/afbc.rst                   | 233 +++++++++++++++++++
> >  Documentation/gpu/drivers.rst                |   1 +
> >  MAINTAINERS                                  |   1 +
> >  drivers/gpu/drm/arm/malidp_hw.c              |  14 +-
> >  drivers/gpu/drm/arm/malidp_planes.c          |  23 +-
> >  drivers/gpu/drm/drm_fb_cma_helper.c          |  21 +-
> >  drivers/gpu/drm/drm_fb_helper.c              |   6 +
> >  drivers/gpu/drm/drm_fourcc.c                 |  65 ++++++
> >  drivers/gpu/drm/drm_framebuffer.c            |  13 +-
> >  drivers/gpu/drm/drm_gem_framebuffer_helper.c |   4 +-
> >  include/drm/drm_fourcc.h                     |  29 ++-
> >  include/uapi/drm/drm_fourcc.h                |  31 +++
> >  12 files changed, 428 insertions(+), 13 deletions(-)
> >  create mode 100644 Documentation/gpu/afbc.rst
> > 
> > -- 
> > 2.18.0
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH v3 1/6] drm/fourcc: Add char_per_block, block_w and block_h in drm_format_info

[Daniel Vetter]

On Mon, Oct 08, 2018 at 09:52:04AM +0000, Alexandru-Cosmin Gheorghe wrote:
> Hi Daniel,
> 
> Thanks for having a look.
> 
> On Fri, Oct 05, 2018 at 04:51:48PM +0200, Daniel Vetter wrote:
> > On Fri, Oct 05, 2018 at 09:26:47AM +0000, Alexandru-Cosmin Gheorghe wrote:
> > > For some pixel formats .cpp structure in drm_format info it's not
> > > enough to describe the peculiarities of the pixel layout, for example
> > > tiled formats or packed formats at bit level.
> > > 
> > > What's implemented here is to add three new members to drm_format_info
> > > that could describe such formats:
> > > 
> > > - char_per_block[3]
> > > - block_w[3]
> > > - block_h[3]
> > > 
> > > char_per_block will be put in a union alongside cpp, for transparent
> > > compatibility  with the existing format descriptions.
> > > 
> > > Regarding, block_w and block_h they are intended to be used through
> > > their equivalent getters drm_format_info_block_width /
> > > drm_format_info_block_height, the reason of the getters is to abstract
> > > the fact that for normal formats block_w and block_h will be unset/0,
> > > but the methods will be returning 1.
> > > 
> > > Using that the following drm core functions had been updated to
> > > generically handle both block and non-block formats:
> > > 
> > > - drm_fb_cma_get_gem_addr: for block formats it will just return the
> > >   beginning of the block.
> > > - framebuffer_check: Updated to compute the minimum pitch as
> > > 	DIV_ROUND_UP(width * char_per_block, drm_format_info_block_width(info, i)
> > > 	* drm_format_info_block_height(info, i))
> > > - drm_gem_fb_create_with_funcs: Updated to compute the size of the
> > >   last line of pixels with the same formula as for the minimum pitch.
> > > - In places where is not expecting to handle block formats, like fbdev
> > >   helpers I just added some warnings in case the block width/height
> > >   are greater than 1.
> > > 
> > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> > > ---
> > >  drivers/gpu/drm/drm_fb_cma_helper.c          | 21 ++++++++--
> > >  drivers/gpu/drm/drm_fb_helper.c              |  6 +++
> > >  drivers/gpu/drm/drm_fourcc.c                 | 40 ++++++++++++++++++++
> > >  drivers/gpu/drm/drm_framebuffer.c            |  9 +++--
> > >  drivers/gpu/drm/drm_gem_framebuffer_helper.c |  4 +-
> > >  include/drm/drm_fourcc.h                     | 29 +++++++++++++-
> > >  6 files changed, 101 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > index 47e0e2f6642d..f8293f4d96cf 100644
> > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > @@ -72,7 +72,9 @@ struct drm_gem_cma_object *drm_fb_cma_get_gem_obj(struct drm_framebuffer *fb,
> > >  EXPORT_SYMBOL_GPL(drm_fb_cma_get_gem_obj);
> > >  
> > >  /**
> > > - * drm_fb_cma_get_gem_addr() - Get physical address for framebuffer
> > > + * drm_fb_cma_get_gem_addr() - Get physical address for framebuffer, for pixel
> > > + * formats where values are grouped in blocks this will get you the beginning of
> > > + * the block
> > >   * @fb: The framebuffer
> > >   * @state: Which state of drm plane
> > >   * @plane: Which plane
> > > @@ -87,6 +89,14 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
> > >  	struct drm_gem_cma_object *obj;
> > >  	dma_addr_t paddr;
> > >  	u8 h_div = 1, v_div = 1;
> > > +	u32 block_w = drm_format_info_block_width(fb->format, plane);
> > > +	u32 block_h = drm_format_info_block_height(fb->format, plane);
> > > +	u32 block_size = fb->format->char_per_block[plane];
> > > +	u32 sample_x;
> > > +	u32 sample_y;
> > > +	u32 block_start_y;
> > > +	u32 num_hblocks;
> > > +
> > >  
> > >  	obj = drm_fb_cma_get_gem_obj(fb, plane);
> > >  	if (!obj)
> > > @@ -99,8 +109,13 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
> > >  		v_div = fb->format->vsub;
> > >  	}
> > >  
> > > -	paddr += (fb->format->cpp[plane] * (state->src_x >> 16)) / h_div;
> > > -	paddr += (fb->pitches[plane] * (state->src_y >> 16)) / v_div;
> > > +	sample_x = (state->src_x >> 16) / h_div;
> > > +	sample_y = (state->src_y >> 16) / v_div;
> > > +	block_start_y = (sample_y / block_h) * block_h;
> > > +	num_hblocks = sample_x / block_w;
> > > +
> > > +	paddr += fb->pitches[plane] * block_start_y;
> > > +	paddr += block_size * num_hblocks;
> > >  
> > >  	return paddr;
> > >  }
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > index a504a5e05676..9add0d7da744 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -1595,6 +1595,10 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> > >  	if (var->pixclock != 0 || in_dbg_master())
> > >  		return -EINVAL;
> > >  
> > > +	if ((drm_format_info_block_width(fb->format, 0) > 1) ||
> > > +	    (drm_format_info_block_height(fb->format, 0) > 1))
> > > +		return -EINVAL;
> > > +
> > >  	/*
> > >  	 * Changes struct fb_var_screeninfo are currently not pushed back
> > >  	 * to KMS, hence fail if different settings are requested.
> > > @@ -1969,6 +1973,8 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
> > >  {
> > >  	struct drm_framebuffer *fb = fb_helper->fb;
> > >  
> > > +	WARN_ON((drm_format_info_block_width(fb->format, 0) > 1) ||
> > > +		(drm_format_info_block_height(fb->format, 0) > 1));
> > >  	info->pseudo_palette = fb_helper->pseudo_palette;
> > >  	info->var.xres_virtual = fb->width;
> > >  	info->var.yres_virtual = fb->height;
> > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > > index a154763257ae..0c9725ffd5e9 100644
> > > --- a/drivers/gpu/drm/drm_fourcc.c
> > > +++ b/drivers/gpu/drm/drm_fourcc.c
> > > @@ -403,3 +403,43 @@ int drm_format_plane_height(int height, uint32_t format, int plane)
> > >  	return height / info->vsub;
> > >  }
> > >  EXPORT_SYMBOL(drm_format_plane_height);
> > > +
> > > +/**
> > > + * drm_format_info_block_width - width in pixels of block.
> > > + * @info: pixel format info
> > > + * @plane: plane index
> > > + *
> > > + * Returns:
> > > + * The width in pixels of a block, depending on the plane index.
> > > + */
> > > +unsigned int drm_format_info_block_width(const struct drm_format_info *info,
> > > +					 int plane)
> > > +{
> > > +	if (!info || plane >= info->num_planes)
> > > +		return 0;
> > > +
> > > +	if (!info->block_w[plane])
> > > +		return 1;
> > > +	return info->block_w[plane];
> > > +}
> > > +EXPORT_SYMBOL(drm_format_info_block_width);
> > > +
> > > +/**
> > > + * drm_format_info_block_height - height in pixels of a block
> > > + * @info: pixel format info
> > > + * @plane: plane index
> > > + *
> > > + * Returns:
> > > + * The height in pixels of a block, depending on the plane index.
> > > + */
> > > +unsigned int drm_format_info_block_height(const struct drm_format_info *info,
> > > +					  int plane)
> > > +{
> > > +	if (!info || plane >= info->num_planes)
> > > +		return 0;
> > > +
> > > +	if (!info->block_h[plane])
> > > +		return 1;
> > > +	return info->block_h[plane];
> > > +}
> > > +EXPORT_SYMBOL(drm_format_info_block_height);
> > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > > index 3bf729d0aae5..4e14013788cd 100644
> > > --- a/drivers/gpu/drm/drm_framebuffer.c
> > > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > > @@ -195,20 +195,23 @@ static int framebuffer_check(struct drm_device *dev,
> > >  	for (i = 0; i < info->num_planes; i++) {
> > >  		unsigned int width = fb_plane_width(r->width, info, i);
> > >  		unsigned int height = fb_plane_height(r->height, info, i);
> > > -		unsigned int cpp = info->cpp[i];
> > > +		unsigned int block_size = info->char_per_block[i];
> > > +		u64 min_pitch = DIV_ROUND_UP((u64)width * block_size,
> > > +					drm_format_info_block_width(info, i) *
> > > +					drm_format_info_block_height(info, i));
> > 
> > I think a helper to compute the drm_pitch, including some kernel-doc that
> > explains what exactly it is (and why) would be good. For cleaner code,
> > less duplication and to have a good spot for docs.
> 
> Stark3y had the same suggestion during our internal review :).

Can we do more of that internal review in public please? Adding r-b tags
from internal stuff where there's 0 reasons for internal review (it's not
an embargoed feature) isn't great.

If the issue is that you can't find your stuff on dri-devel, then maybe
create your own private turf like amd/intel have. But I think arm is small
enough still that this doesn't make any sense.


> Does this sounds good to you?
> drm_format_info_min_pitch(info, plane, buffer_width)

Ack.

> > >  		if (!r->handles[i]) {
> > >  			DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i);
> > >  			return -EINVAL;
> > >  		}
> > >  
> > > -		if ((uint64_t) width * cpp > UINT_MAX)
> > > +		if (min_pitch > UINT_MAX)
> > >  			return -ERANGE;
> > >  
> > >  		if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
> > >  			return -ERANGE;
> > >  
> > > -		if (r->pitches[i] < width * cpp) {
> > > +		if (r->pitches[i] < min_pitch) {
> > >  			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
> > >  			return -EINVAL;
> > >  		}
> > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > index ded7a379ac35..e342511370e7 100644
> > > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > @@ -171,7 +171,9 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
> > >  		}
> > >  
> > >  		min_size = (height - 1) * mode_cmd->pitches[i]
> > > -			 + width * info->cpp[i]
> > > +			 + DIV_ROUND_UP((u64)width * info->char_per_block[i],
> > > +					drm_format_info_block_width(info, i) *
> > > +					drm_format_info_block_height(info, i))
> > >  			 + mode_cmd->offsets[i];
> > >  
> > >  		if (objs[i]->size < min_size) {
> > > diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> > > index 865ef60c17af..305e80847abc 100644
> > > --- a/include/drm/drm_fourcc.h
> > > +++ b/include/drm/drm_fourcc.h
> > > @@ -58,6 +58,24 @@ struct drm_mode_fb_cmd2;
> > >   *	use in new code and set to 0 for new formats.
> > >   * @num_planes: Number of color planes (1 to 3)
> > >   * @cpp: Number of bytes per pixel (per plane)
> > 
> > Need to make it clear this aliases with @char_per_block and is deprecated.
> > Same for @char_per_block.
> > 
> > > + * @char_per_block:  Number of bytes per block (per plane), where blocks are
> > > + *	defined as a rectangle of pixels which are stored next to each other in
> > > + *	a byte aligned memory region.
> > > + *	Together with block_w and block_h this could be used to properly
> > s/could/is/
> > 
> > > + *	describe tiles in tiled formats or to describe groups of pixels in
> > > + *	packed formats for which the memory needed for a single pixel it's not
> > > + *	byte aligned.
> > > + *      Cpp had been kept from historical reasons because there are a lot of
> > 
> > s/Cpp/@cpp/ for proper highlighting in the output.
> > 
> > > + *	places in drivers where it's used. In drm core for generic code paths
> > > + *	the preferred way is to use char_per_block, drm_format_info_block_width
> > @char_per_block and drm_format_info_block_width() for hyperlinks. Similar
> > with all the other occurences of a function name.
> > 
> > > + *	and drm_format_info_block_height which should allow handling both block
> > 
> > s/should// - I hope your code actually works :-)
> > 
> > > + *	and non-block formats in the same way.
> > > + *	For formats that are intended to be used just with modifiers, cpp/
> > > + *	char_per_block must be 0.
> > 
> > This is a bit vague imo. What about:
> > 
> > "For formats that are intended to be used only with non-linear modifiers,
> > both @cpp and @char_per_block must be 0 in the generic format table.
> > Drivers should supply accurate information from their
> > &drm_mode_config.get_format_info hook though."
> > 
> > Since I'm assuming that once you know the exact modifier, you can enlist
> > the core format checking code to help validate it.
> > 
> > > + * @block_w: Block width in pixels, this is intended to be accessed through
> > > + *	drm_format_info_block_width
> > 
> > Since you alias cpp and char_per_block, pls make it really, really clear
> > that if this isn't set, then the code uses 1 as the default.
> > 
> > > + * @block_h: Block height in pixels, this is intended to be accessed through
> > > + *	drm_format_info_block_height
> > 
> > Same here.
> > 
> > Please use the inline kerneldoc style, it's much more readable for long
> > stuff like here. If the inconsistency irks you, then please just move them
> > all to the inline style. Inline style also allows you to have proper
> > paragraph breaks with empty lines.
> > 
> > >   * @hsub: Horizontal chroma subsampling factor
> > >   * @vsub: Vertical chroma subsampling factor
> > >   * @has_alpha: Does the format embeds an alpha component?
> > > @@ -67,7 +85,12 @@ struct drm_format_info {
> > >  	u32 format;
> > >  	u8 depth;
> > >  	u8 num_planes;
> > > -	u8 cpp[3];
> > > +	union {
> > > +		u8 cpp[3];
> > > +		u8 char_per_block[3];
> > > +	};
> > > +	u8 block_w[3];
> > > +	u8 block_h[3];
> > >  	u8 hsub;
> > >  	u8 vsub;
> > >  	bool has_alpha;
> > > @@ -96,6 +119,10 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
> > >  int drm_format_vert_chroma_subsampling(uint32_t format);
> > >  int drm_format_plane_width(int width, uint32_t format, int plane);
> > >  int drm_format_plane_height(int height, uint32_t format, int plane);
> > > +unsigned int drm_format_info_block_width(const struct drm_format_info *info,
> > > +					 int plane);
> > > +unsigned int drm_format_info_block_height(const struct drm_format_info *info,
> > > +					  int plane);
> > >  const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
> > >  
> > >  #endif /* __DRM_FOURCC_H__ */
> > 
> > With the polish addressed, this lgtm.
> 
> All of them are reasonable to me, so I'll apply them in the next
> version.
> 
> > 
> > One question I have is validating this. I think a bunch of unit tests,
> > integrated into the existing kms selftests we already (so that
> > intel-gfx-ci and other CI can run it) would be great. Both for the small
> > helper functions (block width/height, but especially drm_pitch), but also
> > for the drm_framebuffer_create functions, so that we can exercise the
> > metric pile of validation corner cases.
> > -Daniel
> 
> So, you are thinking of adding tests here drivers/gpu/drm/selftests/ ?
> Looking inside the folder, it seems more like a stub than proper
> test suite for the drm framework, or am I missing something ?

It's indeed very incomplete still.

> I did run tests for all supported formats by the mali-dp, with our
> internal testsuite and everything was OK for all the fourcc enabled
> in mali-dp

Your internal test suite is to no value to the overall community :-) Can
we get those upstreamed somewhere, ideally as part of igt?

> I'm planning to run the igt on top of mali-dp, just to double check I
> didn't change any of the behaviours. Any idea if there is any public
> igt CI that I could point to a kernel branch, to test this change on
> something else other than mali-dp?

Atm only way to get it to pre-merge test stuff is Cc: intel-gfx m-l.

> Shouldn't that be enough for validating this patches? 

I don't think so, right now we don't have specific tests for e.g. the new
min_pitch function you're adding. Igt is still fairly far away from
providing full coverage. And it can't provide coverage for new features
ofc, without someone adding those tests.

Also, I'm not worried about you validating this correctly, that's kinda
your problem. I'm worried about keeping things working going forward, and
for that we need:
- public tests
- run in a public CI

Yes I know for the 2nd point intel-gfx-ci isn't there yet, but my
long-term plan is to run all the generic igt tests plus kernel self-tests
on virtual machines (on top of vkms+vgem) in gitlab CI.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/6] drm/fourcc: Add char_per_block, block_w and block_h in drm_format_info

[Alexandru-Cosmin Gheorghe]

On Thu, Oct 11, 2018 at 10:29:42AM +0200, Daniel Vetter wrote:
> On Mon, Oct 08, 2018 at 09:52:04AM +0000, Alexandru-Cosmin Gheorghe wrote:
> > Hi Daniel,
> > 
> > Thanks for having a look.
> > 
> > On Fri, Oct 05, 2018 at 04:51:48PM +0200, Daniel Vetter wrote:
> > > On Fri, Oct 05, 2018 at 09:26:47AM +0000, Alexandru-Cosmin Gheorghe wrote:
> > > > For some pixel formats .cpp structure in drm_format info it's not
> > > > enough to describe the peculiarities of the pixel layout, for example
> > > > tiled formats or packed formats at bit level.
> > > > 
> > > > What's implemented here is to add three new members to drm_format_info
> > > > that could describe such formats:
> > > > 
> > > > - char_per_block[3]
> > > > - block_w[3]
> > > > - block_h[3]
> > > > 
> > > > char_per_block will be put in a union alongside cpp, for transparent
> > > > compatibility  with the existing format descriptions.
> > > > 
> > > > Regarding, block_w and block_h they are intended to be used through
> > > > their equivalent getters drm_format_info_block_width /
> > > > drm_format_info_block_height, the reason of the getters is to abstract
> > > > the fact that for normal formats block_w and block_h will be unset/0,
> > > > but the methods will be returning 1.
> > > > 
> > > > Using that the following drm core functions had been updated to
> > > > generically handle both block and non-block formats:
> > > > 
> > > > - drm_fb_cma_get_gem_addr: for block formats it will just return the
> > > >   beginning of the block.
> > > > - framebuffer_check: Updated to compute the minimum pitch as
> > > > 	DIV_ROUND_UP(width * char_per_block, drm_format_info_block_width(info, i)
> > > > 	* drm_format_info_block_height(info, i))
> > > > - drm_gem_fb_create_with_funcs: Updated to compute the size of the
> > > >   last line of pixels with the same formula as for the minimum pitch.
> > > > - In places where is not expecting to handle block formats, like fbdev
> > > >   helpers I just added some warnings in case the block width/height
> > > >   are greater than 1.
> > > > 
> > > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > > > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_fb_cma_helper.c          | 21 ++++++++--
> > > >  drivers/gpu/drm/drm_fb_helper.c              |  6 +++
> > > >  drivers/gpu/drm/drm_fourcc.c                 | 40 ++++++++++++++++++++
> > > >  drivers/gpu/drm/drm_framebuffer.c            |  9 +++--
> > > >  drivers/gpu/drm/drm_gem_framebuffer_helper.c |  4 +-
> > > >  include/drm/drm_fourcc.h                     | 29 +++++++++++++-
> > > >  6 files changed, 101 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > > index 47e0e2f6642d..f8293f4d96cf 100644
> > > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > > @@ -72,7 +72,9 @@ struct drm_gem_cma_object *drm_fb_cma_get_gem_obj(struct drm_framebuffer *fb,
> > > >  EXPORT_SYMBOL_GPL(drm_fb_cma_get_gem_obj);
> > > >  
> > > >  /**
> > > > - * drm_fb_cma_get_gem_addr() - Get physical address for framebuffer
> > > > + * drm_fb_cma_get_gem_addr() - Get physical address for framebuffer, for pixel
> > > > + * formats where values are grouped in blocks this will get you the beginning of
> > > > + * the block
> > > >   * @fb: The framebuffer
> > > >   * @state: Which state of drm plane
> > > >   * @plane: Which plane
> > > > @@ -87,6 +89,14 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
> > > >  	struct drm_gem_cma_object *obj;
> > > >  	dma_addr_t paddr;
> > > >  	u8 h_div = 1, v_div = 1;
> > > > +	u32 block_w = drm_format_info_block_width(fb->format, plane);
> > > > +	u32 block_h = drm_format_info_block_height(fb->format, plane);
> > > > +	u32 block_size = fb->format->char_per_block[plane];
> > > > +	u32 sample_x;
> > > > +	u32 sample_y;
> > > > +	u32 block_start_y;
> > > > +	u32 num_hblocks;
> > > > +
> > > >  
> > > >  	obj = drm_fb_cma_get_gem_obj(fb, plane);
> > > >  	if (!obj)
> > > > @@ -99,8 +109,13 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
> > > >  		v_div = fb->format->vsub;
> > > >  	}
> > > >  
> > > > -	paddr += (fb->format->cpp[plane] * (state->src_x >> 16)) / h_div;
> > > > -	paddr += (fb->pitches[plane] * (state->src_y >> 16)) / v_div;
> > > > +	sample_x = (state->src_x >> 16) / h_div;
> > > > +	sample_y = (state->src_y >> 16) / v_div;
> > > > +	block_start_y = (sample_y / block_h) * block_h;
> > > > +	num_hblocks = sample_x / block_w;
> > > > +
> > > > +	paddr += fb->pitches[plane] * block_start_y;
> > > > +	paddr += block_size * num_hblocks;
> > > >  
> > > >  	return paddr;
> > > >  }
> > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > > index a504a5e05676..9add0d7da744 100644
> > > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > > @@ -1595,6 +1595,10 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> > > >  	if (var->pixclock != 0 || in_dbg_master())
> > > >  		return -EINVAL;
> > > >  
> > > > +	if ((drm_format_info_block_width(fb->format, 0) > 1) ||
> > > > +	    (drm_format_info_block_height(fb->format, 0) > 1))
> > > > +		return -EINVAL;
> > > > +
> > > >  	/*
> > > >  	 * Changes struct fb_var_screeninfo are currently not pushed back
> > > >  	 * to KMS, hence fail if different settings are requested.
> > > > @@ -1969,6 +1973,8 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
> > > >  {
> > > >  	struct drm_framebuffer *fb = fb_helper->fb;
> > > >  
> > > > +	WARN_ON((drm_format_info_block_width(fb->format, 0) > 1) ||
> > > > +		(drm_format_info_block_height(fb->format, 0) > 1));
> > > >  	info->pseudo_palette = fb_helper->pseudo_palette;
> > > >  	info->var.xres_virtual = fb->width;
> > > >  	info->var.yres_virtual = fb->height;
> > > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > > > index a154763257ae..0c9725ffd5e9 100644
> > > > --- a/drivers/gpu/drm/drm_fourcc.c
> > > > +++ b/drivers/gpu/drm/drm_fourcc.c
> > > > @@ -403,3 +403,43 @@ int drm_format_plane_height(int height, uint32_t format, int plane)
> > > >  	return height / info->vsub;
> > > >  }
> > > >  EXPORT_SYMBOL(drm_format_plane_height);
> > > > +
> > > > +/**
> > > > + * drm_format_info_block_width - width in pixels of block.
> > > > + * @info: pixel format info
> > > > + * @plane: plane index
> > > > + *
> > > > + * Returns:
> > > > + * The width in pixels of a block, depending on the plane index.
> > > > + */
> > > > +unsigned int drm_format_info_block_width(const struct drm_format_info *info,
> > > > +					 int plane)
> > > > +{
> > > > +	if (!info || plane >= info->num_planes)
> > > > +		return 0;
> > > > +
> > > > +	if (!info->block_w[plane])
> > > > +		return 1;
> > > > +	return info->block_w[plane];
> > > > +}
> > > > +EXPORT_SYMBOL(drm_format_info_block_width);
> > > > +
> > > > +/**
> > > > + * drm_format_info_block_height - height in pixels of a block
> > > > + * @info: pixel format info
> > > > + * @plane: plane index
> > > > + *
> > > > + * Returns:
> > > > + * The height in pixels of a block, depending on the plane index.
> > > > + */
> > > > +unsigned int drm_format_info_block_height(const struct drm_format_info *info,
> > > > +					  int plane)
> > > > +{
> > > > +	if (!info || plane >= info->num_planes)
> > > > +		return 0;
> > > > +
> > > > +	if (!info->block_h[plane])
> > > > +		return 1;
> > > > +	return info->block_h[plane];
> > > > +}
> > > > +EXPORT_SYMBOL(drm_format_info_block_height);
> > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > > > index 3bf729d0aae5..4e14013788cd 100644
> > > > --- a/drivers/gpu/drm/drm_framebuffer.c
> > > > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > > > @@ -195,20 +195,23 @@ static int framebuffer_check(struct drm_device *dev,
> > > >  	for (i = 0; i < info->num_planes; i++) {
> > > >  		unsigned int width = fb_plane_width(r->width, info, i);
> > > >  		unsigned int height = fb_plane_height(r->height, info, i);
> > > > -		unsigned int cpp = info->cpp[i];
> > > > +		unsigned int block_size = info->char_per_block[i];
> > > > +		u64 min_pitch = DIV_ROUND_UP((u64)width * block_size,
> > > > +					drm_format_info_block_width(info, i) *
> > > > +					drm_format_info_block_height(info, i));
> > > 
> > > I think a helper to compute the drm_pitch, including some kernel-doc that
> > > explains what exactly it is (and why) would be good. For cleaner code,
> > > less duplication and to have a good spot for docs.
> > 
> > Stark3y had the same suggestion during our internal review :).
> 
> Can we do more of that internal review in public please? Adding r-b tags
> from internal stuff where there's 0 reasons for internal review (it's not
> an embargoed feature) isn't great.

I'm in the same room with both Liviu and Stark3y, so it's hard to
avoid internal review before going public. I was in doubt if I should add
r-b tags, probably I should've ommit them and leave that come trough the
mailing list.

> 
> If the issue is that you can't find your stuff on dri-devel, then maybe
> create your own private turf like amd/intel have. But I think arm is small
> enough still that this doesn't make any sense.

I agree, there is no need for a private turf.

> 
> 
> > Does this sounds good to you?
> > drm_format_info_min_pitch(info, plane, buffer_width)
> 
> Ack.
> 
> > > >  		if (!r->handles[i]) {
> > > >  			DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i);
> > > >  			return -EINVAL;
> > > >  		}
> > > >  
> > > > -		if ((uint64_t) width * cpp > UINT_MAX)
> > > > +		if (min_pitch > UINT_MAX)
> > > >  			return -ERANGE;
> > > >  
> > > >  		if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
> > > >  			return -ERANGE;
> > > >  
> > > > -		if (r->pitches[i] < width * cpp) {
> > > > +		if (r->pitches[i] < min_pitch) {
> > > >  			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
> > > >  			return -EINVAL;
> > > >  		}
> > > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > > index ded7a379ac35..e342511370e7 100644
> > > > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > > @@ -171,7 +171,9 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
> > > >  		}
> > > >  
> > > >  		min_size = (height - 1) * mode_cmd->pitches[i]
> > > > -			 + width * info->cpp[i]
> > > > +			 + DIV_ROUND_UP((u64)width * info->char_per_block[i],
> > > > +					drm_format_info_block_width(info, i) *
> > > > +					drm_format_info_block_height(info, i))
> > > >  			 + mode_cmd->offsets[i];
> > > >  
> > > >  		if (objs[i]->size < min_size) {
> > > > diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> > > > index 865ef60c17af..305e80847abc 100644
> > > > --- a/include/drm/drm_fourcc.h
> > > > +++ b/include/drm/drm_fourcc.h
> > > > @@ -58,6 +58,24 @@ struct drm_mode_fb_cmd2;
> > > >   *	use in new code and set to 0 for new formats.
> > > >   * @num_planes: Number of color planes (1 to 3)
> > > >   * @cpp: Number of bytes per pixel (per plane)
> > > 
> > > Need to make it clear this aliases with @char_per_block and is deprecated.
> > > Same for @char_per_block.
> > > 
> > > > + * @char_per_block:  Number of bytes per block (per plane), where blocks are
> > > > + *	defined as a rectangle of pixels which are stored next to each other in
> > > > + *	a byte aligned memory region.
> > > > + *	Together with block_w and block_h this could be used to properly
> > > s/could/is/
> > > 
> > > > + *	describe tiles in tiled formats or to describe groups of pixels in
> > > > + *	packed formats for which the memory needed for a single pixel it's not
> > > > + *	byte aligned.
> > > > + *      Cpp had been kept from historical reasons because there are a lot of
> > > 
> > > s/Cpp/@cpp/ for proper highlighting in the output.
> > > 
> > > > + *	places in drivers where it's used. In drm core for generic code paths
> > > > + *	the preferred way is to use char_per_block, drm_format_info_block_width
> > > @char_per_block and drm_format_info_block_width() for hyperlinks. Similar
> > > with all the other occurences of a function name.
> > > 
> > > > + *	and drm_format_info_block_height which should allow handling both block
> > > 
> > > s/should// - I hope your code actually works :-)
> > > 
> > > > + *	and non-block formats in the same way.
> > > > + *	For formats that are intended to be used just with modifiers, cpp/
> > > > + *	char_per_block must be 0.
> > > 
> > > This is a bit vague imo. What about:
> > > 
> > > "For formats that are intended to be used only with non-linear modifiers,
> > > both @cpp and @char_per_block must be 0 in the generic format table.
> > > Drivers should supply accurate information from their
> > > &drm_mode_config.get_format_info hook though."
> > > 
> > > Since I'm assuming that once you know the exact modifier, you can enlist
> > > the core format checking code to help validate it.
> > > 
> > > > + * @block_w: Block width in pixels, this is intended to be accessed through
> > > > + *	drm_format_info_block_width
> > > 
> > > Since you alias cpp and char_per_block, pls make it really, really clear
> > > that if this isn't set, then the code uses 1 as the default.
> > > 
> > > > + * @block_h: Block height in pixels, this is intended to be accessed through
> > > > + *	drm_format_info_block_height
> > > 
> > > Same here.
> > > 
> > > Please use the inline kerneldoc style, it's much more readable for long
> > > stuff like here. If the inconsistency irks you, then please just move them
> > > all to the inline style. Inline style also allows you to have proper
> > > paragraph breaks with empty lines.
> > > 
> > > >   * @hsub: Horizontal chroma subsampling factor
> > > >   * @vsub: Vertical chroma subsampling factor
> > > >   * @has_alpha: Does the format embeds an alpha component?
> > > > @@ -67,7 +85,12 @@ struct drm_format_info {
> > > >  	u32 format;
> > > >  	u8 depth;
> > > >  	u8 num_planes;
> > > > -	u8 cpp[3];
> > > > +	union {
> > > > +		u8 cpp[3];
> > > > +		u8 char_per_block[3];
> > > > +	};
> > > > +	u8 block_w[3];
> > > > +	u8 block_h[3];
> > > >  	u8 hsub;
> > > >  	u8 vsub;
> > > >  	bool has_alpha;
> > > > @@ -96,6 +119,10 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
> > > >  int drm_format_vert_chroma_subsampling(uint32_t format);
> > > >  int drm_format_plane_width(int width, uint32_t format, int plane);
> > > >  int drm_format_plane_height(int height, uint32_t format, int plane);
> > > > +unsigned int drm_format_info_block_width(const struct drm_format_info *info,
> > > > +					 int plane);
> > > > +unsigned int drm_format_info_block_height(const struct drm_format_info *info,
> > > > +					  int plane);
> > > >  const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
> > > >  
> > > >  #endif /* __DRM_FOURCC_H__ */
> > > 
> > > With the polish addressed, this lgtm.
> > 
> > All of them are reasonable to me, so I'll apply them in the next
> > version.
> > 
> > > 
> > > One question I have is validating this. I think a bunch of unit tests,
> > > integrated into the existing kms selftests we already (so that
> > > intel-gfx-ci and other CI can run it) would be great. Both for the small
> > > helper functions (block width/height, but especially drm_pitch), but also
> > > for the drm_framebuffer_create functions, so that we can exercise the
> > > metric pile of validation corner cases.
> > > -Daniel
> > 
> > So, you are thinking of adding tests here drivers/gpu/drm/selftests/ ?
> > Looking inside the folder, it seems more like a stub than proper
> > test suite for the drm framework, or am I missing something ?
> 
> It's indeed very incomplete still.
> 
> > I did run tests for all supported formats by the mali-dp, with our
> > internal testsuite and everything was OK for all the fourcc enabled
> > in mali-dp
> 
> Your internal test suite is to no value to the overall community :-) Can
> we get those upstreamed somewhere, ideally as part of igt?

I agree with you that having one single test suite would make our life
easier. Hoewever, the way our internal test suite evolved means that
integrating it to igt would basically mean writing it from scratch, so
it's hard to justify the effort.

> 
> > I'm planning to run the igt on top of mali-dp, just to double check I
> > didn't change any of the behaviours. Any idea if there is any public
> > igt CI that I could point to a kernel branch, to test this change on
> > something else other than mali-dp?
> 
> Atm only way to get it to pre-merge test stuff is Cc: intel-gfx m-l.
> 
> > Shouldn't that be enough for validating this patches? 
> 
> I don't think so, right now we don't have specific tests for e.g. the new
> min_pitch function you're adding. Igt is still fairly far away from
> providing full coverage. And it can't provide coverage for new features
> ofc, without someone adding those tests.

Proabably drm/selftests makes more sense for the drm_format_info*
helpers I'm hoping is relatively painless to add, I'm not very sure
about check_framebuffer and drm_framebuffer_create, but I'll have a
look into it.

> 
> Also, I'm not worried about you validating this correctly, that's kinda
> your problem. I'm worried about keeping things working going forward, and
> for that we need:
> - public tests
> - run in a public CI
> 
> Yes I know for the 2nd point intel-gfx-ci isn't there yet, but my
> long-term plan is to run all the generic igt tests plus kernel self-tests
> on virtual machines (on top of vkms+vgem) in gitlab CI.
> 
> Cheers, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH v3 1/6] drm/fourcc: Add char_per_block, block_w and block_h in drm_format_info

[Daniel Vetter]

On Thu, Oct 11, 2018 at 11:58 AM Alexandru-Cosmin Gheorghe
<Alexandru-Cosmin.Gheorghe@arm.com> wrote:
>
> On Thu, Oct 11, 2018 at 10:29:42AM +0200, Daniel Vetter wrote:
> > On Mon, Oct 08, 2018 at 09:52:04AM +0000, Alexandru-Cosmin Gheorghe wrote:
> > > Shouldn't that be enough for validating this patches?
> >
> > I don't think so, right now we don't have specific tests for e.g. the new
> > min_pitch function you're adding. Igt is still fairly far away from
> > providing full coverage. And it can't provide coverage for new features
> > ofc, without someone adding those tests.
>
> Proabably drm/selftests makes more sense for the drm_format_info*
> helpers I'm hoping is relatively painless to add, I'm not very sure
> about check_framebuffer and drm_framebuffer_create, but I'll have a
> look into it.

This might help:

https://www.spinics.net/lists/dri-devel/msg191131.html

and on the igt side:

https://lists.freedesktop.org/archives/igt-dev/2018-October/006257.html

I've already suggested we rename that bucket of self-tests to drm_kms,
so that we can stuff all kinds of kms related selftests in there.
Instead of having to wire up the scaffolding for all of them
individually.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/6] drm/fourcc: Add char_per_block, block_w and block_h in drm_format_info

[Liviu Dudau]

On Thu, Oct 11, 2018 at 10:58:30AM +0100, Alexandru-Cosmin Gheorghe wrote:
> On Thu, Oct 11, 2018 at 10:29:42AM +0200, Daniel Vetter wrote:
> > On Mon, Oct 08, 2018 at 09:52:04AM +0000, Alexandru-Cosmin Gheorghe wrote:
> > > Hi Daniel,
> > > 
> > > Thanks for having a look.
> > > 
> > > On Fri, Oct 05, 2018 at 04:51:48PM +0200, Daniel Vetter wrote:
> > > > On Fri, Oct 05, 2018 at 09:26:47AM +0000, Alexandru-Cosmin Gheorghe wrote:
> > > > > For some pixel formats .cpp structure in drm_format info it's not
> > > > > enough to describe the peculiarities of the pixel layout, for example
> > > > > tiled formats or packed formats at bit level.
> > > > > 
> > > > > What's implemented here is to add three new members to drm_format_info
> > > > > that could describe such formats:
> > > > > 
> > > > > - char_per_block[3]
> > > > > - block_w[3]
> > > > > - block_h[3]
> > > > > 
> > > > > char_per_block will be put in a union alongside cpp, for transparent
> > > > > compatibility  with the existing format descriptions.
> > > > > 
> > > > > Regarding, block_w and block_h they are intended to be used through
> > > > > their equivalent getters drm_format_info_block_width /
> > > > > drm_format_info_block_height, the reason of the getters is to abstract
> > > > > the fact that for normal formats block_w and block_h will be unset/0,
> > > > > but the methods will be returning 1.
> > > > > 
> > > > > Using that the following drm core functions had been updated to
> > > > > generically handle both block and non-block formats:
> > > > > 
> > > > > - drm_fb_cma_get_gem_addr: for block formats it will just return the
> > > > >   beginning of the block.
> > > > > - framebuffer_check: Updated to compute the minimum pitch as
> > > > > 	DIV_ROUND_UP(width * char_per_block, drm_format_info_block_width(info, i)
> > > > > 	* drm_format_info_block_height(info, i))
> > > > > - drm_gem_fb_create_with_funcs: Updated to compute the size of the
> > > > >   last line of pixels with the same formula as for the minimum pitch.
> > > > > - In places where is not expecting to handle block formats, like fbdev
> > > > >   helpers I just added some warnings in case the block width/height
> > > > >   are greater than 1.
> > > > > 
> > > > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > > > > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_fb_cma_helper.c          | 21 ++++++++--
> > > > >  drivers/gpu/drm/drm_fb_helper.c              |  6 +++
> > > > >  drivers/gpu/drm/drm_fourcc.c                 | 40 ++++++++++++++++++++
> > > > >  drivers/gpu/drm/drm_framebuffer.c            |  9 +++--
> > > > >  drivers/gpu/drm/drm_gem_framebuffer_helper.c |  4 +-
> > > > >  include/drm/drm_fourcc.h                     | 29 +++++++++++++-
> > > > >  6 files changed, 101 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > > > index 47e0e2f6642d..f8293f4d96cf 100644
> > > > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > > > @@ -72,7 +72,9 @@ struct drm_gem_cma_object *drm_fb_cma_get_gem_obj(struct drm_framebuffer *fb,
> > > > >  EXPORT_SYMBOL_GPL(drm_fb_cma_get_gem_obj);
> > > > >  
> > > > >  /**
> > > > > - * drm_fb_cma_get_gem_addr() - Get physical address for framebuffer
> > > > > + * drm_fb_cma_get_gem_addr() - Get physical address for framebuffer, for pixel
> > > > > + * formats where values are grouped in blocks this will get you the beginning of
> > > > > + * the block
> > > > >   * @fb: The framebuffer
> > > > >   * @state: Which state of drm plane
> > > > >   * @plane: Which plane
> > > > > @@ -87,6 +89,14 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
> > > > >  	struct drm_gem_cma_object *obj;
> > > > >  	dma_addr_t paddr;
> > > > >  	u8 h_div = 1, v_div = 1;
> > > > > +	u32 block_w = drm_format_info_block_width(fb->format, plane);
> > > > > +	u32 block_h = drm_format_info_block_height(fb->format, plane);
> > > > > +	u32 block_size = fb->format->char_per_block[plane];
> > > > > +	u32 sample_x;
> > > > > +	u32 sample_y;
> > > > > +	u32 block_start_y;
> > > > > +	u32 num_hblocks;
> > > > > +
> > > > >  
> > > > >  	obj = drm_fb_cma_get_gem_obj(fb, plane);
> > > > >  	if (!obj)
> > > > > @@ -99,8 +109,13 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb,
> > > > >  		v_div = fb->format->vsub;
> > > > >  	}
> > > > >  
> > > > > -	paddr += (fb->format->cpp[plane] * (state->src_x >> 16)) / h_div;
> > > > > -	paddr += (fb->pitches[plane] * (state->src_y >> 16)) / v_div;
> > > > > +	sample_x = (state->src_x >> 16) / h_div;
> > > > > +	sample_y = (state->src_y >> 16) / v_div;
> > > > > +	block_start_y = (sample_y / block_h) * block_h;
> > > > > +	num_hblocks = sample_x / block_w;
> > > > > +
> > > > > +	paddr += fb->pitches[plane] * block_start_y;
> > > > > +	paddr += block_size * num_hblocks;
> > > > >  
> > > > >  	return paddr;
> > > > >  }
> > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > > > index a504a5e05676..9add0d7da744 100644
> > > > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > > > @@ -1595,6 +1595,10 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> > > > >  	if (var->pixclock != 0 || in_dbg_master())
> > > > >  		return -EINVAL;
> > > > >  
> > > > > +	if ((drm_format_info_block_width(fb->format, 0) > 1) ||
> > > > > +	    (drm_format_info_block_height(fb->format, 0) > 1))
> > > > > +		return -EINVAL;
> > > > > +
> > > > >  	/*
> > > > >  	 * Changes struct fb_var_screeninfo are currently not pushed back
> > > > >  	 * to KMS, hence fail if different settings are requested.
> > > > > @@ -1969,6 +1973,8 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
> > > > >  {
> > > > >  	struct drm_framebuffer *fb = fb_helper->fb;
> > > > >  
> > > > > +	WARN_ON((drm_format_info_block_width(fb->format, 0) > 1) ||
> > > > > +		(drm_format_info_block_height(fb->format, 0) > 1));
> > > > >  	info->pseudo_palette = fb_helper->pseudo_palette;
> > > > >  	info->var.xres_virtual = fb->width;
> > > > >  	info->var.yres_virtual = fb->height;
> > > > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > > > > index a154763257ae..0c9725ffd5e9 100644
> > > > > --- a/drivers/gpu/drm/drm_fourcc.c
> > > > > +++ b/drivers/gpu/drm/drm_fourcc.c
> > > > > @@ -403,3 +403,43 @@ int drm_format_plane_height(int height, uint32_t format, int plane)
> > > > >  	return height / info->vsub;
> > > > >  }
> > > > >  EXPORT_SYMBOL(drm_format_plane_height);
> > > > > +
> > > > > +/**
> > > > > + * drm_format_info_block_width - width in pixels of block.
> > > > > + * @info: pixel format info
> > > > > + * @plane: plane index
> > > > > + *
> > > > > + * Returns:
> > > > > + * The width in pixels of a block, depending on the plane index.
> > > > > + */
> > > > > +unsigned int drm_format_info_block_width(const struct drm_format_info *info,
> > > > > +					 int plane)
> > > > > +{
> > > > > +	if (!info || plane >= info->num_planes)
> > > > > +		return 0;
> > > > > +
> > > > > +	if (!info->block_w[plane])
> > > > > +		return 1;
> > > > > +	return info->block_w[plane];
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_format_info_block_width);
> > > > > +
> > > > > +/**
> > > > > + * drm_format_info_block_height - height in pixels of a block
> > > > > + * @info: pixel format info
> > > > > + * @plane: plane index
> > > > > + *
> > > > > + * Returns:
> > > > > + * The height in pixels of a block, depending on the plane index.
> > > > > + */
> > > > > +unsigned int drm_format_info_block_height(const struct drm_format_info *info,
> > > > > +					  int plane)
> > > > > +{
> > > > > +	if (!info || plane >= info->num_planes)
> > > > > +		return 0;
> > > > > +
> > > > > +	if (!info->block_h[plane])
> > > > > +		return 1;
> > > > > +	return info->block_h[plane];
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_format_info_block_height);
> > > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > > > > index 3bf729d0aae5..4e14013788cd 100644
> > > > > --- a/drivers/gpu/drm/drm_framebuffer.c
> > > > > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > > > > @@ -195,20 +195,23 @@ static int framebuffer_check(struct drm_device *dev,
> > > > >  	for (i = 0; i < info->num_planes; i++) {
> > > > >  		unsigned int width = fb_plane_width(r->width, info, i);
> > > > >  		unsigned int height = fb_plane_height(r->height, info, i);
> > > > > -		unsigned int cpp = info->cpp[i];
> > > > > +		unsigned int block_size = info->char_per_block[i];
> > > > > +		u64 min_pitch = DIV_ROUND_UP((u64)width * block_size,
> > > > > +					drm_format_info_block_width(info, i) *
> > > > > +					drm_format_info_block_height(info, i));
> > > > 
> > > > I think a helper to compute the drm_pitch, including some kernel-doc that
> > > > explains what exactly it is (and why) would be good. For cleaner code,
> > > > less duplication and to have a good spot for docs.
> > > 
> > > Stark3y had the same suggestion during our internal review :).
> > 
> > Can we do more of that internal review in public please? Adding r-b tags
> > from internal stuff where there's 0 reasons for internal review (it's not
> > an embargoed feature) isn't great.
> 
> I'm in the same room with both Liviu and Stark3y, so it's hard to
> avoid internal review before going public. I was in doubt if I should add
> r-b tags, probably I should've ommit them and leave that come trough the
> mailing list.

Just to add a bit of clarification here, we're usually not doing
internal reviews for public patches, this was more like Alex doing an
internal RFC series because it was more convenient with the travel and
people's calendars to do it that way, and as a result he added the R-b
(which I agree, could've been (re)collected in the public ML).

> 
> > 
> > If the issue is that you can't find your stuff on dri-devel, then maybe
> > create your own private turf like amd/intel have. But I think arm is small
> > enough still that this doesn't make any sense.
> 
> I agree, there is no need for a private turf.
> 
> > 
> > 
> > > Does this sounds good to you?
> > > drm_format_info_min_pitch(info, plane, buffer_width)
> > 
> > Ack.
> > 
> > > > >  		if (!r->handles[i]) {
> > > > >  			DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i);
> > > > >  			return -EINVAL;
> > > > >  		}
> > > > >  
> > > > > -		if ((uint64_t) width * cpp > UINT_MAX)
> > > > > +		if (min_pitch > UINT_MAX)
> > > > >  			return -ERANGE;
> > > > >  
> > > > >  		if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
> > > > >  			return -ERANGE;
> > > > >  
> > > > > -		if (r->pitches[i] < width * cpp) {
> > > > > +		if (r->pitches[i] < min_pitch) {
> > > > >  			DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
> > > > >  			return -EINVAL;
> > > > >  		}
> > > > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > > > index ded7a379ac35..e342511370e7 100644
> > > > > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > > > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > > > @@ -171,7 +171,9 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
> > > > >  		}
> > > > >  
> > > > >  		min_size = (height - 1) * mode_cmd->pitches[i]
> > > > > -			 + width * info->cpp[i]
> > > > > +			 + DIV_ROUND_UP((u64)width * info->char_per_block[i],
> > > > > +					drm_format_info_block_width(info, i) *
> > > > > +					drm_format_info_block_height(info, i))
> > > > >  			 + mode_cmd->offsets[i];
> > > > >  
> > > > >  		if (objs[i]->size < min_size) {
> > > > > diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> > > > > index 865ef60c17af..305e80847abc 100644
> > > > > --- a/include/drm/drm_fourcc.h
> > > > > +++ b/include/drm/drm_fourcc.h
> > > > > @@ -58,6 +58,24 @@ struct drm_mode_fb_cmd2;
> > > > >   *	use in new code and set to 0 for new formats.
> > > > >   * @num_planes: Number of color planes (1 to 3)
> > > > >   * @cpp: Number of bytes per pixel (per plane)
> > > > 
> > > > Need to make it clear this aliases with @char_per_block and is deprecated.
> > > > Same for @char_per_block.
> > > > 
> > > > > + * @char_per_block:  Number of bytes per block (per plane), where blocks are
> > > > > + *	defined as a rectangle of pixels which are stored next to each other in
> > > > > + *	a byte aligned memory region.
> > > > > + *	Together with block_w and block_h this could be used to properly
> > > > s/could/is/
> > > > 
> > > > > + *	describe tiles in tiled formats or to describe groups of pixels in
> > > > > + *	packed formats for which the memory needed for a single pixel it's not
> > > > > + *	byte aligned.
> > > > > + *      Cpp had been kept from historical reasons because there are a lot of
> > > > 
> > > > s/Cpp/@cpp/ for proper highlighting in the output.
> > > > 
> > > > > + *	places in drivers where it's used. In drm core for generic code paths
> > > > > + *	the preferred way is to use char_per_block, drm_format_info_block_width
> > > > @char_per_block and drm_format_info_block_width() for hyperlinks. Similar
> > > > with all the other occurences of a function name.
> > > > 
> > > > > + *	and drm_format_info_block_height which should allow handling both block
> > > > 
> > > > s/should// - I hope your code actually works :-)
> > > > 
> > > > > + *	and non-block formats in the same way.
> > > > > + *	For formats that are intended to be used just with modifiers, cpp/
> > > > > + *	char_per_block must be 0.
> > > > 
> > > > This is a bit vague imo. What about:
> > > > 
> > > > "For formats that are intended to be used only with non-linear modifiers,
> > > > both @cpp and @char_per_block must be 0 in the generic format table.
> > > > Drivers should supply accurate information from their
> > > > &drm_mode_config.get_format_info hook though."
> > > > 
> > > > Since I'm assuming that once you know the exact modifier, you can enlist
> > > > the core format checking code to help validate it.
> > > > 
> > > > > + * @block_w: Block width in pixels, this is intended to be accessed through
> > > > > + *	drm_format_info_block_width
> > > > 
> > > > Since you alias cpp and char_per_block, pls make it really, really clear
> > > > that if this isn't set, then the code uses 1 as the default.
> > > > 
> > > > > + * @block_h: Block height in pixels, this is intended to be accessed through
> > > > > + *	drm_format_info_block_height
> > > > 
> > > > Same here.
> > > > 
> > > > Please use the inline kerneldoc style, it's much more readable for long
> > > > stuff like here. If the inconsistency irks you, then please just move them
> > > > all to the inline style. Inline style also allows you to have proper
> > > > paragraph breaks with empty lines.
> > > > 
> > > > >   * @hsub: Horizontal chroma subsampling factor
> > > > >   * @vsub: Vertical chroma subsampling factor
> > > > >   * @has_alpha: Does the format embeds an alpha component?
> > > > > @@ -67,7 +85,12 @@ struct drm_format_info {
> > > > >  	u32 format;
> > > > >  	u8 depth;
> > > > >  	u8 num_planes;
> > > > > -	u8 cpp[3];
> > > > > +	union {
> > > > > +		u8 cpp[3];
> > > > > +		u8 char_per_block[3];
> > > > > +	};
> > > > > +	u8 block_w[3];
> > > > > +	u8 block_h[3];
> > > > >  	u8 hsub;
> > > > >  	u8 vsub;
> > > > >  	bool has_alpha;
> > > > > @@ -96,6 +119,10 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
> > > > >  int drm_format_vert_chroma_subsampling(uint32_t format);
> > > > >  int drm_format_plane_width(int width, uint32_t format, int plane);
> > > > >  int drm_format_plane_height(int height, uint32_t format, int plane);
> > > > > +unsigned int drm_format_info_block_width(const struct drm_format_info *info,
> > > > > +					 int plane);
> > > > > +unsigned int drm_format_info_block_height(const struct drm_format_info *info,
> > > > > +					  int plane);
> > > > >  const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
> > > > >  
> > > > >  #endif /* __DRM_FOURCC_H__ */
> > > > 
> > > > With the polish addressed, this lgtm.
> > > 
> > > All of them are reasonable to me, so I'll apply them in the next
> > > version.
> > > 
> > > > 
> > > > One question I have is validating this. I think a bunch of unit tests,
> > > > integrated into the existing kms selftests we already (so that
> > > > intel-gfx-ci and other CI can run it) would be great. Both for the small
> > > > helper functions (block width/height, but especially drm_pitch), but also
> > > > for the drm_framebuffer_create functions, so that we can exercise the
> > > > metric pile of validation corner cases.
> > > > -Daniel
> > > 
> > > So, you are thinking of adding tests here drivers/gpu/drm/selftests/ ?
> > > Looking inside the folder, it seems more like a stub than proper
> > > test suite for the drm framework, or am I missing something ?
> > 
> > It's indeed very incomplete still.
> > 
> > > I did run tests for all supported formats by the mali-dp, with our
> > > internal testsuite and everything was OK for all the fourcc enabled
> > > in mali-dp
> > 
> > Your internal test suite is to no value to the overall community :-) Can
> > we get those upstreamed somewhere, ideally as part of igt?

On balance of things, how many more drivers are we going to cover when
going from internal test suite to igt? +1 (intel)? +3 (finger in the
air)? By looking at the commit log is hard to judge the traction of igt in
the "overall community".

Best regards,
Liviu

> 
> I agree with you that having one single test suite would make our life
> easier. Hoewever, the way our internal test suite evolved means that
> integrating it to igt would basically mean writing it from scratch, so
> it's hard to justify the effort.
> 
> > 
> > > I'm planning to run the igt on top of mali-dp, just to double check I
> > > didn't change any of the behaviours. Any idea if there is any public
> > > igt CI that I could point to a kernel branch, to test this change on
> > > something else other than mali-dp?
> > 
> > Atm only way to get it to pre-merge test stuff is Cc: intel-gfx m-l.
> > 
> > > Shouldn't that be enough for validating this patches? 
> > 
> > I don't think so, right now we don't have specific tests for e.g. the new
> > min_pitch function you're adding. Igt is still fairly far away from
> > providing full coverage. And it can't provide coverage for new features
> > ofc, without someone adding those tests.
> 
> Proabably drm/selftests makes more sense for the drm_format_info*
> helpers I'm hoping is relatively painless to add, I'm not very sure
> about check_framebuffer and drm_framebuffer_create, but I'll have a
> look into it.
> 
> > 
> > Also, I'm not worried about you validating this correctly, that's kinda
> > your problem. I'm worried about keeping things working going forward, and
> > for that we need:
> > - public tests
> > - run in a public CI
> > 
> > Yes I know for the 2nd point intel-gfx-ci isn't there yet, but my
> > long-term plan is to run all the generic igt tests plus kernel self-tests
> > on virtual machines (on top of vkms+vgem) in gitlab CI.
> > 
> > Cheers, Daniel
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Cheers,
> Alex G

-- 
====================
| 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: [PATCH v3 1/6] drm/fourcc: Add char_per_block, block_w and block_h in drm_format_info

[Daniel Vetter]

On Thu, Oct 11, 2018 at 12:11 PM Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> On Thu, Oct 11, 2018 at 10:58:30AM +0100, Alexandru-Cosmin Gheorghe wrote:
> > On Thu, Oct 11, 2018 at 10:29:42AM +0200, Daniel Vetter wrote:
> > > On Mon, Oct 08, 2018 at 09:52:04AM +0000, Alexandru-Cosmin Gheorghe wrote:
> > > > > One question I have is validating this. I think a bunch of unit tests,
> > > > > integrated into the existing kms selftests we already (so that
> > > > > intel-gfx-ci and other CI can run it) would be great. Both for the small
> > > > > helper functions (block width/height, but especially drm_pitch), but also
> > > > > for the drm_framebuffer_create functions, so that we can exercise the
> > > > > metric pile of validation corner cases.
> > > > > -Daniel
> > > >
> > > > So, you are thinking of adding tests here drivers/gpu/drm/selftests/ ?
> > > > Looking inside the folder, it seems more like a stub than proper
> > > > test suite for the drm framework, or am I missing something ?
> > >
> > > It's indeed very incomplete still.
> > >
> > > > I did run tests for all supported formats by the mali-dp, with our
> > > > internal testsuite and everything was OK for all the fourcc enabled
> > > > in mali-dp
> > >
> > > Your internal test suite is to no value to the overall community :-) Can
> > > we get those upstreamed somewhere, ideally as part of igt?
>
> On balance of things, how many more drivers are we going to cover when
> going from internal test suite to igt? +1 (intel)? +3 (finger in the
> air)? By looking at the commit log is hard to judge the traction of igt in
> the "overall community".

With my intel hat on I don't particularly care how you test mali, and
I don't think you folks care how we test intel. (Ok maybe there's a
shared interest in having high quality upstream drivers so that OS
people can upgrade without fear of regressions across any of the hw
vendors, but that's a long story and somewhat an aside).

But there's piles of shared infrastructure, and that's really where I
think a common test suite, ideally run as part of drm core CI (I'm
working on that, doesn't exist yet). That's where the shared CI
efforts really pay off imo. There's also some value in trying to make
sure all drivers work the same across all drivers. Both of these
pieces need to be there to be able to refactor the drm subsystem
aggressively, which we'll need to do because hw is changing all the
time. If you don't care about any of this, then pushing your driver to
upstream doesn't make a hole lot of sense imo :-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/6] drm/fourcc: Add char_per_block, block_w and block_h in drm_format_info

[Liviu Dudau]

On Thu, Oct 11, 2018 at 02:40:10PM +0200, Daniel Vetter wrote:
> On Thu, Oct 11, 2018 at 12:11 PM Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > On Thu, Oct 11, 2018 at 10:58:30AM +0100, Alexandru-Cosmin Gheorghe wrote:
> > > On Thu, Oct 11, 2018 at 10:29:42AM +0200, Daniel Vetter wrote:
> > > > On Mon, Oct 08, 2018 at 09:52:04AM +0000, Alexandru-Cosmin Gheorghe wrote:
> > > > > > One question I have is validating this. I think a bunch of unit tests,
> > > > > > integrated into the existing kms selftests we already (so that
> > > > > > intel-gfx-ci and other CI can run it) would be great. Both for the small
> > > > > > helper functions (block width/height, but especially drm_pitch), but also
> > > > > > for the drm_framebuffer_create functions, so that we can exercise the
> > > > > > metric pile of validation corner cases.
> > > > > > -Daniel
> > > > >
> > > > > So, you are thinking of adding tests here drivers/gpu/drm/selftests/ ?
> > > > > Looking inside the folder, it seems more like a stub than proper
> > > > > test suite for the drm framework, or am I missing something ?
> > > >
> > > > It's indeed very incomplete still.
> > > >
> > > > > I did run tests for all supported formats by the mali-dp, with our
> > > > > internal testsuite and everything was OK for all the fourcc enabled
> > > > > in mali-dp
> > > >
> > > > Your internal test suite is to no value to the overall community :-) Can
> > > > we get those upstreamed somewhere, ideally as part of igt?
> >
> > On balance of things, how many more drivers are we going to cover when
> > going from internal test suite to igt? +1 (intel)? +3 (finger in the
> > air)? By looking at the commit log is hard to judge the traction of igt in
> > the "overall community".
> 
> With my intel hat on I don't particularly care how you test mali, and
> I don't think you folks care how we test intel. (Ok maybe there's a
> shared interest in having high quality upstream drivers so that OS
> people can upgrade without fear of regressions across any of the hw
> vendors, but that's a long story and somewhat an aside).
> 
> But there's piles of shared infrastructure, and that's really where I
> think a common test suite, ideally run as part of drm core CI (I'm
> working on that, doesn't exist yet). That's where the shared CI
> efforts really pay off imo. There's also some value in trying to make
> sure all drivers work the same across all drivers. Both of these
> pieces need to be there to be able to refactor the drm subsystem
> aggressively, which we'll need to do because hw is changing all the
> time. If you don't care about any of this, then pushing your driver to
> upstream doesn't make a hole lot of sense imo :-)

Nah, I wasn't looking to start a holy war, it was a genuine question. I
don't remember exactly the timeline but I do admit that I was a bit
puzzled (confused?) by the appearance of drivers/gpu/drm/selftests given
the existence of igt, so I though one of them is the "newer" thing to
support. I was on the fence on which one is worth backing, hence the
quantitative question (how many more drivers actively get tested with
igt?).

I know that I have used igt to test mali-dp before submission, we also
had some patches that need refreshing to add writeback support (sorry
for being slow on those), but I haven't seen people contributing in
earnest to igt support for their drivers (vc4 is the newest, amdgpu from
a few months ago, I thought there was some adreno stuff but I can't find
right now). So for the passer-bys (aka managers) it is hard (sometimes)
to see the benefit of investing effort in something that doesn't look
like a clear winner.

Maybe igt could use some "Supported drivers" section in the README so we
can point it out to our decision makers?

Best regards,
Liviu

> 
> Cheers, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
====================
| 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: [PATCH v3 1/6] drm/fourcc: Add char_per_block, block_w and block_h in drm_format_info

[Daniel Vetter]

On Thu, Oct 11, 2018 at 2:53 PM Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> On Thu, Oct 11, 2018 at 02:40:10PM +0200, Daniel Vetter wrote:
> > On Thu, Oct 11, 2018 at 12:11 PM Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > > On Thu, Oct 11, 2018 at 10:58:30AM +0100, Alexandru-Cosmin Gheorghe wrote:
> > > > On Thu, Oct 11, 2018 at 10:29:42AM +0200, Daniel Vetter wrote:
> > > > > On Mon, Oct 08, 2018 at 09:52:04AM +0000, Alexandru-Cosmin Gheorghe wrote:
> > > > > > > One question I have is validating this. I think a bunch of unit tests,
> > > > > > > integrated into the existing kms selftests we already (so that
> > > > > > > intel-gfx-ci and other CI can run it) would be great. Both for the small
> > > > > > > helper functions (block width/height, but especially drm_pitch), but also
> > > > > > > for the drm_framebuffer_create functions, so that we can exercise the
> > > > > > > metric pile of validation corner cases.
> > > > > > > -Daniel
> > > > > >
> > > > > > So, you are thinking of adding tests here drivers/gpu/drm/selftests/ ?
> > > > > > Looking inside the folder, it seems more like a stub than proper
> > > > > > test suite for the drm framework, or am I missing something ?
> > > > >
> > > > > It's indeed very incomplete still.
> > > > >
> > > > > > I did run tests for all supported formats by the mali-dp, with our
> > > > > > internal testsuite and everything was OK for all the fourcc enabled
> > > > > > in mali-dp
> > > > >
> > > > > Your internal test suite is to no value to the overall community :-) Can
> > > > > we get those upstreamed somewhere, ideally as part of igt?
> > >
> > > On balance of things, how many more drivers are we going to cover when
> > > going from internal test suite to igt? +1 (intel)? +3 (finger in the
> > > air)? By looking at the commit log is hard to judge the traction of igt in
> > > the "overall community".
> >
> > With my intel hat on I don't particularly care how you test mali, and
> > I don't think you folks care how we test intel. (Ok maybe there's a
> > shared interest in having high quality upstream drivers so that OS
> > people can upgrade without fear of regressions across any of the hw
> > vendors, but that's a long story and somewhat an aside).
> >
> > But there's piles of shared infrastructure, and that's really where I
> > think a common test suite, ideally run as part of drm core CI (I'm
> > working on that, doesn't exist yet). That's where the shared CI
> > efforts really pay off imo. There's also some value in trying to make
> > sure all drivers work the same across all drivers. Both of these
> > pieces need to be there to be able to refactor the drm subsystem
> > aggressively, which we'll need to do because hw is changing all the
> > time. If you don't care about any of this, then pushing your driver to
> > upstream doesn't make a hole lot of sense imo :-)
>
> Nah, I wasn't looking to start a holy war, it was a genuine question. I
> don't remember exactly the timeline but I do admit that I was a bit
> puzzled (confused?) by the appearance of drivers/gpu/drm/selftests given
> the existence of igt, so I though one of them is the "newer" thing to
> support. I was on the fence on which one is worth backing, hence the
> quantitative question (how many more drivers actively get tested with
> igt?).

They're two sides of the same coin really:
- igt does (mostly) blackbox testing from userspace, with very minimal
ties into the driver through debugfs.
- selftests run within the kernel, have deep knowledge of the
implementation (not just the uapi), and are mostly unit tests of
individual bits&pieces.

Currently we need some minimal wrappers in igt to be able to run the
kernel selftests using igt (so that it's all under one overall testing
umbrella). But the ksefltests can also be run without igt.

Doing the selftests in igt would unnecessarily tie igt to the kernel,
a massive maintenance headaches. And we want to be able to run igt
uapi tests on older/newer kernels, so that we can make sure we don't
break back/forward-compatibility. So merging igt into the kernel
(which some othe tests-suites have done to make maintenanace easier)
doesn't make much sense imo.

Depending upon what you're doing it's better to pick igt or selftests
(or both, for different aspects) as the best way to create
regression-tests for your new feature. For entirely internal work
(like the new block_* description for pixel formats) I think in-kernel
unit-tests are the most effective way to regression test them, hence
my recommendation to look into selftests.

For new uapi you generally want some igts, but perhaps augmented with
self-tests for tricky internal implementation details. That was my
recommendation for the dirty rectangle, where the validation code is
an igt testcase and a bunch of self-tests.

> I know that I have used igt to test mali-dp before submission, we also
> had some patches that need refreshing to add writeback support (sorry
> for being slow on those), but I haven't seen people contributing in
> earnest to igt support for their drivers (vc4 is the newest, amdgpu from
> a few months ago, I thought there was some adreno stuff but I can't find
> right now). So for the passer-bys (aka managers) it is hard (sometimes)
> to see the benefit of investing effort in something that doesn't look
> like a clear winner.

Igt is supposed to run on any kms driver without changes. There's no
need to add special code in there. The vc4/amdgpu/freedreno code is
for the gem side, where you do need special code to test-drive the
driver-private ioctl interfaces.

Now of course it's not perfect, and occasionally we need to remove
some intel-ism. But e.g. just recently vmwgfx people made a small
patch series to be able to run igt on vmwgfx, and it was very minimal.
And vmwgfx is probably the most "special" kms driver we have in-tree,
so I think we're doing a fairly good job overall.

> Maybe igt could use some "Supported drivers" section in the README so we
> can point it out to our decision makers?

All of them, or at least all the kms ones? Or are you more looking for
"actively supported", as in people running them in some CI regularly?
The answer for the later I frankly don't even know ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel