[PATCH 0/4] Centralize format information

[PATCH 0/4] Centralize format information

[Laurent Pinchart]

Hello,

Various pieces of information about DRM formats (number of planes, color
depth, chroma subsampling, ...) are scattered across different helper
functions in the DRM core. Callers of those functions often need to access
more than a single parameter of the format, leading to inefficiencies due to
multiple lookups.

This patch series addresses this issue by centralizing all format information
in a single data structure (1/4). It reimplements the existing format helper
functions based on that structure (3/4) and converts the DRM core code to use
the new structure (4/4). Two unused format helper functions are removed in the
process (2/4).

The new API is also useful for drivers. I will shortly post a patch series for
the omapdrm driver that makes use of it.

Laurent Pinchart (4):
  drm: Centralize format information
  drm: Remove unused drm_format_plane_(width|height) helpers
  drm: Implement the drm_format_*() helpers as drm_format_info()
    wrappers
  drm: Use drm_format_info() in DRM core code

 drivers/gpu/drm/drm_crtc.c          | 391 +++++++++++-------------------------
 drivers/gpu/drm/drm_fb_cma_helper.c |  23 ++-
 include/drm/drm_crtc.h              |  23 ++-
 3 files changed, 153 insertions(+), 284 deletions(-)

-- 
Regards,

Laurent Pinchart

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

[PATCH 1/4] drm: Centralize format information

[Laurent Pinchart]

Various pieces of information about DRM formats (number of planes, color
depth, chroma subsampling, ...) are scattered across different helper
functions in the DRM core. Callers of those functions often need to
access more than a single parameter of the format, leading to
inefficiencies due to multiple lookups.

Centralize all format information in a data structure and create a
function to look up information based on the format 4CC.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/drm_crtc.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h     | 21 ++++++++++++
 2 files changed, 104 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 0e3cc66aa8b7..74b0c6dd80cd 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -5544,6 +5544,89 @@ int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
 }
 
 /**
+ * drm_format_info - information for a given format
+ * @format: pixel format (DRM_FORMAT_*)
+ *
+ * Returns:
+ * The instance of struct drm_format_info that describes the pixel format, or
+ * NULL if the format is unsupported.
+ */
+const struct drm_format_info *drm_format_info(u32 format)
+{
+	static const struct drm_format_info formats[] = {
+		{ DRM_FORMAT_C8, 8, 8, 1, { 1 }, 1, 1 },
+		{ DRM_FORMAT_RGB332, 8, 8, 1, { 1 }, 1, 1 },
+		{ DRM_FORMAT_BGR233, 8, 8, 1, { 1 }, 1, 1 },
+		{ DRM_FORMAT_XRGB4444, 12, 16, 1, { 2 }, 1, 1 },
+		{ DRM_FORMAT_XBGR4444, 12, 16, 1, { 2 }, 1, 1 },
+		{ DRM_FORMAT_RGBX4444, 12, 16, 1, { 2 }, 1, 1 },
+		{ DRM_FORMAT_BGRX4444, 12, 16, 1, { 2 }, 1, 1 },
+		{ DRM_FORMAT_ARGB4444, 12, 16, 1, { 2 }, 1, 1 },
+		{ DRM_FORMAT_ABGR4444, 12, 16, 1, { 2 }, 1, 1 },
+		{ DRM_FORMAT_RGBA4444, 12, 16, 1, { 2 }, 1, 1 },
+		{ DRM_FORMAT_BGRA4444, 12, 16, 1, { 2 }, 1, 1 },
+		{ DRM_FORMAT_XRGB1555, 15, 16, 1, { 2 }, 1, 1 },
+		{ DRM_FORMAT_XBGR1555, 15, 16, 1, { 2 }, 1, 1 },
+		{ DRM_FORMAT_RGBX5551, 15, 16, 1, { 2 }, 1, 1 },
+		{ DRM_FORMAT_BGRX5551, 15, 16, 1, { 2 }, 1, 1 },
+		{ DRM_FORMAT_ARGB1555, 15, 16, 1, { 2 }, 1, 1 },
+		{ DRM_FORMAT_ABGR1555, 15, 16, 1, { 2 }, 1, 1 },
+		{ DRM_FORMAT_RGBA5551, 15, 16, 1, { 2 }, 1, 1 },
+		{ DRM_FORMAT_BGRA5551, 15, 16, 1, { 2 }, 1, 1 },
+		{ DRM_FORMAT_RGB565, 16, 16, 1, { 2 }, 1, 1 },
+		{ DRM_FORMAT_BGR565, 16, 16, 1, { 2 }, 1, 1 },
+		{ DRM_FORMAT_RGB888, 24, 24, 1, { 3 }, 1, 1 },
+		{ DRM_FORMAT_BGR888, 24, 24, 1, { 3 }, 1, 1 },
+		{ DRM_FORMAT_XRGB8888, 24, 32, 1, { 4 }, 1, 1 },
+		{ DRM_FORMAT_XBGR8888, 24, 32, 1, { 4 }, 1, 1 },
+		{ DRM_FORMAT_RGBX8888, 24, 32, 1, { 4 }, 1, 1 },
+		{ DRM_FORMAT_BGRX8888, 24, 32, 1, { 4 }, 1, 1 },
+		{ DRM_FORMAT_XRGB2101010, 30, 32, 1, { 4 }, 1, 1 },
+		{ DRM_FORMAT_XBGR2101010, 30, 32, 1, { 4 }, 1, 1 },
+		{ DRM_FORMAT_RGBX1010102, 30, 32, 1, { 4 }, 1, 1 },
+		{ DRM_FORMAT_BGRX1010102, 30, 32, 1, { 4 }, 1, 1 },
+		{ DRM_FORMAT_ARGB2101010, 30, 32, 1, { 4 }, 1, 1 },
+		{ DRM_FORMAT_ABGR2101010, 30, 32, 1, { 4 }, 1, 1 },
+		{ DRM_FORMAT_RGBA1010102, 30, 32, 1, { 4 }, 1, 1 },
+		{ DRM_FORMAT_BGRA1010102, 30, 32, 1, { 4 }, 1, 1 },
+		{ DRM_FORMAT_ARGB8888, 32, 32, 1, { 4 }, 1, 1 },
+		{ DRM_FORMAT_ABGR8888, 32, 32, 1, { 4 }, 1, 1 },
+		{ DRM_FORMAT_RGBA8888, 32, 32, 1, { 4 }, 1, 1 },
+		{ DRM_FORMAT_BGRA8888, 32, 32, 1, { 4 }, 1, 1 },
+		{ DRM_FORMAT_YUV410, 0, 0, 3, { 1, 1, 1 }, 4, 4 },
+		{ DRM_FORMAT_YVU410, 0, 0, 3, { 1, 1, 1 }, 4, 4 },
+		{ DRM_FORMAT_YUV411, 0, 0, 3, { 1, 1, 1 }, 4, 1 },
+		{ DRM_FORMAT_YVU411, 0, 0, 3, { 1, 1, 1 }, 4, 1 },
+		{ DRM_FORMAT_YUV420, 0, 0, 3, { 1, 1, 1 }, 2, 2 },
+		{ DRM_FORMAT_YVU420, 0, 0, 3, { 1, 1, 1 }, 2, 2 },
+		{ DRM_FORMAT_YUV422, 0, 0, 3, { 1, 1, 1 }, 2, 1 },
+		{ DRM_FORMAT_YVU422, 0, 0, 3, { 1, 1, 1 }, 2, 1 },
+		{ DRM_FORMAT_YUV444, 0, 0, 3, { 1, 1, 1 }, 1, 1 },
+		{ DRM_FORMAT_YVU444, 0, 0, 3, { 1, 1, 1 }, 1, 1 },
+		{ DRM_FORMAT_NV12, 0, 0, 2, { 1, 2 }, 2, 2 },
+		{ DRM_FORMAT_NV21, 0, 0, 2, { 1, 2 }, 2, 2 },
+		{ DRM_FORMAT_NV16, 0, 0, 2, { 1, 2 }, 2, 1 },
+		{ DRM_FORMAT_NV61, 0, 0, 2, { 1, 2 }, 2, 1 },
+		{ DRM_FORMAT_NV24, 0, 0, 2, { 1, 2 }, 1, 1 },
+		{ DRM_FORMAT_NV42, 0, 0, 2, { 1, 2 }, 1, 1 },
+		{ DRM_FORMAT_YUYV, 0, 0, 1, { 2 }, 2, 1 },
+		{ DRM_FORMAT_YVYU, 0, 0, 1, { 2 }, 2, 1 },
+		{ DRM_FORMAT_UYVY, 0, 0, 1, { 2 }, 2, 1 },
+		{ DRM_FORMAT_VYUY, 0, 0, 1, { 2 }, 2, 1 },
+		{ DRM_FORMAT_AYUV, 0, 0, 1, { 4 }, 1, 1 },
+	};
+
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
+		if (formats[i].format == format)
+			return &formats[i];
+	}
+
+	return NULL;
+}
+
+/**
  * drm_fb_get_bpp_depth - get the bpp/depth values for format
  * @format: pixel format (DRM_FORMAT_*)
  * @depth: storage for the depth value
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index d1559cd04e3d..4199794cc317 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -155,6 +155,26 @@ struct drm_display_info {
 	u8 cea_rev;
 };
 
+/**
+ * struct drm_format_info - information about a DRM format
+ * @format: 4CC format identifier (DRM_FORMAT_*)
+ * @depth: color depth (number of bits per pixel excluding padding bits)
+ * @bpp: number of bits per pixel including padding
+ * @num_planes: number of color planes (1 to 3)
+ * @cpp: number of bytes per pixel (per plane)
+ * @hsub: horizontal chroma subsampling factor
+ * @vsub: vertical chroma subsampling factor
+ */
+struct drm_format_info {
+	u32 format;
+	unsigned int depth;
+	unsigned int bpp;
+	unsigned int num_planes;
+	unsigned int cpp[3];
+	unsigned int hsub;
+	unsigned int vsub;
+};
+
 /* data corresponds to displayid vend/prod/serial */
 struct drm_tile_group {
 	struct kref refcount;
@@ -2540,6 +2560,7 @@ extern int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
 extern int drm_mode_atomic_ioctl(struct drm_device *dev,
 				 void *data, struct drm_file *file_priv);
 
+extern const struct drm_format_info *drm_format_info(u32 format);
 extern void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
 				 int *bpp);
 extern int drm_format_num_planes(uint32_t format);
-- 
Regards,

Laurent Pinchart

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

[PATCH 2/4] drm: Remove unused drm_format_plane_(width|height) helpers

[Laurent Pinchart]

The drm_format_plane_width() and drm_format_plane_height() helper
functions are not used, remove them.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/drm_crtc.c | 42 ------------------------------------------
 include/drm/drm_crtc.h     |  2 --
 2 files changed, 44 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 74b0c6dd80cd..b405d4379e47 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -5843,48 +5843,6 @@ int drm_format_vert_chroma_subsampling(uint32_t format)
 EXPORT_SYMBOL(drm_format_vert_chroma_subsampling);
 
 /**
- * drm_format_plane_width - width of the plane given the first plane
- * @width: width of the first plane
- * @format: pixel format
- * @plane: plane index
- *
- * Returns:
- * The width of @plane, given that the width of the first plane is @width.
- */
-int drm_format_plane_width(int width, uint32_t format, int plane)
-{
-	if (plane >= drm_format_num_planes(format))
-		return 0;
-
-	if (plane == 0)
-		return width;
-
-	return width / drm_format_horz_chroma_subsampling(format);
-}
-EXPORT_SYMBOL(drm_format_plane_width);
-
-/**
- * drm_format_plane_height - height of the plane given the first plane
- * @height: height of the first plane
- * @format: pixel format
- * @plane: plane index
- *
- * Returns:
- * The height of @plane, given that the height of the first plane is @height.
- */
-int drm_format_plane_height(int height, uint32_t format, int plane)
-{
-	if (plane >= drm_format_num_planes(format))
-		return 0;
-
-	if (plane == 0)
-		return height;
-
-	return height / drm_format_vert_chroma_subsampling(format);
-}
-EXPORT_SYMBOL(drm_format_plane_height);
-
-/**
  * drm_rotation_simplify() - Try to simplify the rotation
  * @rotation: Rotation to be simplified
  * @supported_rotations: Supported rotations
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 4199794cc317..a45f57f32dca 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -2567,8 +2567,6 @@ extern int drm_format_num_planes(uint32_t format);
 extern int drm_format_plane_cpp(uint32_t format, int plane);
 extern int drm_format_horz_chroma_subsampling(uint32_t format);
 extern int drm_format_vert_chroma_subsampling(uint32_t format);
-extern int drm_format_plane_width(int width, uint32_t format, int plane);
-extern int drm_format_plane_height(int height, uint32_t format, int plane);
 extern const char *drm_get_format_name(uint32_t format);
 extern struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev,
 							      unsigned int supported_rotations);
-- 
Regards,

Laurent Pinchart

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

[PATCH 3/4] drm: Implement the drm_format_*() helpers as drm_format_info() wrappers

[Laurent Pinchart]

Turn the drm_format_*() helpers into wrappers around the drm_format_info
lookup function to centralize all format information in a single place.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/drm_crtc.c | 166 +++++++--------------------------------------
 1 file changed, 24 insertions(+), 142 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index b405d4379e47..d8215fdcaeb6 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -5638,66 +5638,19 @@ const struct drm_format_info *drm_format_info(u32 format)
 void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
 			  int *bpp)
 {
-	switch (format) {
-	case DRM_FORMAT_C8:
-	case DRM_FORMAT_RGB332:
-	case DRM_FORMAT_BGR233:
-		*depth = 8;
-		*bpp = 8;
-		break;
-	case DRM_FORMAT_XRGB1555:
-	case DRM_FORMAT_XBGR1555:
-	case DRM_FORMAT_RGBX5551:
-	case DRM_FORMAT_BGRX5551:
-	case DRM_FORMAT_ARGB1555:
-	case DRM_FORMAT_ABGR1555:
-	case DRM_FORMAT_RGBA5551:
-	case DRM_FORMAT_BGRA5551:
-		*depth = 15;
-		*bpp = 16;
-		break;
-	case DRM_FORMAT_RGB565:
-	case DRM_FORMAT_BGR565:
-		*depth = 16;
-		*bpp = 16;
-		break;
-	case DRM_FORMAT_RGB888:
-	case DRM_FORMAT_BGR888:
-		*depth = 24;
-		*bpp = 24;
-		break;
-	case DRM_FORMAT_XRGB8888:
-	case DRM_FORMAT_XBGR8888:
-	case DRM_FORMAT_RGBX8888:
-	case DRM_FORMAT_BGRX8888:
-		*depth = 24;
-		*bpp = 32;
-		break;
-	case DRM_FORMAT_XRGB2101010:
-	case DRM_FORMAT_XBGR2101010:
-	case DRM_FORMAT_RGBX1010102:
-	case DRM_FORMAT_BGRX1010102:
-	case DRM_FORMAT_ARGB2101010:
-	case DRM_FORMAT_ABGR2101010:
-	case DRM_FORMAT_RGBA1010102:
-	case DRM_FORMAT_BGRA1010102:
-		*depth = 30;
-		*bpp = 32;
-		break;
-	case DRM_FORMAT_ARGB8888:
-	case DRM_FORMAT_ABGR8888:
-	case DRM_FORMAT_RGBA8888:
-	case DRM_FORMAT_BGRA8888:
-		*depth = 32;
-		*bpp = 32;
-		break;
-	default:
+	const struct drm_format_info *info;
+
+	info = drm_format_info(format);
+	if (!info || !info->depth || !info->bpp) {
 		DRM_DEBUG_KMS("unsupported pixel format %s\n",
 			      drm_get_format_name(format));
 		*depth = 0;
 		*bpp = 0;
-		break;
+		return;
 	}
+
+	*depth = info->depth;
+	*bpp = info->bpp;
 }
 EXPORT_SYMBOL(drm_fb_get_bpp_depth);
 
@@ -5710,28 +5663,10 @@ EXPORT_SYMBOL(drm_fb_get_bpp_depth);
  */
 int drm_format_num_planes(uint32_t format)
 {
-	switch (format) {
-	case DRM_FORMAT_YUV410:
-	case DRM_FORMAT_YVU410:
-	case DRM_FORMAT_YUV411:
-	case DRM_FORMAT_YVU411:
-	case DRM_FORMAT_YUV420:
-	case DRM_FORMAT_YVU420:
-	case DRM_FORMAT_YUV422:
-	case DRM_FORMAT_YVU422:
-	case DRM_FORMAT_YUV444:
-	case DRM_FORMAT_YVU444:
-		return 3;
-	case DRM_FORMAT_NV12:
-	case DRM_FORMAT_NV21:
-	case DRM_FORMAT_NV16:
-	case DRM_FORMAT_NV61:
-	case DRM_FORMAT_NV24:
-	case DRM_FORMAT_NV42:
-		return 2;
-	default:
-		return 1;
-	}
+	const struct drm_format_info *info;
+
+	info = drm_format_info(format);
+	return info ? info->num_planes : 1;
 }
 EXPORT_SYMBOL(drm_format_num_planes);
 
@@ -5745,40 +5680,13 @@ EXPORT_SYMBOL(drm_format_num_planes);
  */
 int drm_format_plane_cpp(uint32_t format, int plane)
 {
-	unsigned int depth;
-	int bpp;
+	const struct drm_format_info *info;
 
-	if (plane >= drm_format_num_planes(format))
+	info = drm_format_info(format);
+	if (!info || plane >= info->num_planes)
 		return 0;
 
-	switch (format) {
-	case DRM_FORMAT_YUYV:
-	case DRM_FORMAT_YVYU:
-	case DRM_FORMAT_UYVY:
-	case DRM_FORMAT_VYUY:
-		return 2;
-	case DRM_FORMAT_NV12:
-	case DRM_FORMAT_NV21:
-	case DRM_FORMAT_NV16:
-	case DRM_FORMAT_NV61:
-	case DRM_FORMAT_NV24:
-	case DRM_FORMAT_NV42:
-		return plane ? 2 : 1;
-	case DRM_FORMAT_YUV410:
-	case DRM_FORMAT_YVU410:
-	case DRM_FORMAT_YUV411:
-	case DRM_FORMAT_YVU411:
-	case DRM_FORMAT_YUV420:
-	case DRM_FORMAT_YVU420:
-	case DRM_FORMAT_YUV422:
-	case DRM_FORMAT_YVU422:
-	case DRM_FORMAT_YUV444:
-	case DRM_FORMAT_YVU444:
-		return 1;
-	default:
-		drm_fb_get_bpp_depth(format, &depth, &bpp);
-		return bpp >> 3;
-	}
+	return info->cpp[plane];
 }
 EXPORT_SYMBOL(drm_format_plane_cpp);
 
@@ -5792,28 +5700,10 @@ EXPORT_SYMBOL(drm_format_plane_cpp);
  */
 int drm_format_horz_chroma_subsampling(uint32_t format)
 {
-	switch (format) {
-	case DRM_FORMAT_YUV411:
-	case DRM_FORMAT_YVU411:
-	case DRM_FORMAT_YUV410:
-	case DRM_FORMAT_YVU410:
-		return 4;
-	case DRM_FORMAT_YUYV:
-	case DRM_FORMAT_YVYU:
-	case DRM_FORMAT_UYVY:
-	case DRM_FORMAT_VYUY:
-	case DRM_FORMAT_NV12:
-	case DRM_FORMAT_NV21:
-	case DRM_FORMAT_NV16:
-	case DRM_FORMAT_NV61:
-	case DRM_FORMAT_YUV422:
-	case DRM_FORMAT_YVU422:
-	case DRM_FORMAT_YUV420:
-	case DRM_FORMAT_YVU420:
-		return 2;
-	default:
-		return 1;
-	}
+	const struct drm_format_info *info;
+
+	info = drm_format_info(format);
+	return info ? info->hsub : 1;
 }
 EXPORT_SYMBOL(drm_format_horz_chroma_subsampling);
 
@@ -5827,18 +5717,10 @@ EXPORT_SYMBOL(drm_format_horz_chroma_subsampling);
  */
 int drm_format_vert_chroma_subsampling(uint32_t format)
 {
-	switch (format) {
-	case DRM_FORMAT_YUV410:
-	case DRM_FORMAT_YVU410:
-		return 4;
-	case DRM_FORMAT_YUV420:
-	case DRM_FORMAT_YVU420:
-	case DRM_FORMAT_NV12:
-	case DRM_FORMAT_NV21:
-		return 2;
-	default:
-		return 1;
-	}
+	const struct drm_format_info *info;
+
+	info = drm_format_info(format);
+	return info ? info->vsub : 1;
 }
 EXPORT_SYMBOL(drm_format_vert_chroma_subsampling);
 
-- 
Regards,

Laurent Pinchart

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

[PATCH 4/4] drm: Use drm_format_info() in DRM core code

[Laurent Pinchart]

Replace calls to the drm_format_*() helper functions with direct use of
the drm_format_info structure. This improves efficiency by removing
duplicate lookups.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/drm_crtc.c          | 100 +++++-------------------------------
 drivers/gpu/drm/drm_fb_cma_helper.c |  23 +++++----
 2 files changed, 25 insertions(+), 98 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d8215fdcaeb6..2fa458d47c90 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3204,108 +3204,32 @@ int drm_mode_addfb(struct drm_device *dev,
 	return 0;
 }
 
-static int format_check(const struct drm_mode_fb_cmd2 *r)
-{
-	uint32_t format = r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN;
-
-	switch (format) {
-	case DRM_FORMAT_C8:
-	case DRM_FORMAT_RGB332:
-	case DRM_FORMAT_BGR233:
-	case DRM_FORMAT_XRGB4444:
-	case DRM_FORMAT_XBGR4444:
-	case DRM_FORMAT_RGBX4444:
-	case DRM_FORMAT_BGRX4444:
-	case DRM_FORMAT_ARGB4444:
-	case DRM_FORMAT_ABGR4444:
-	case DRM_FORMAT_RGBA4444:
-	case DRM_FORMAT_BGRA4444:
-	case DRM_FORMAT_XRGB1555:
-	case DRM_FORMAT_XBGR1555:
-	case DRM_FORMAT_RGBX5551:
-	case DRM_FORMAT_BGRX5551:
-	case DRM_FORMAT_ARGB1555:
-	case DRM_FORMAT_ABGR1555:
-	case DRM_FORMAT_RGBA5551:
-	case DRM_FORMAT_BGRA5551:
-	case DRM_FORMAT_RGB565:
-	case DRM_FORMAT_BGR565:
-	case DRM_FORMAT_RGB888:
-	case DRM_FORMAT_BGR888:
-	case DRM_FORMAT_XRGB8888:
-	case DRM_FORMAT_XBGR8888:
-	case DRM_FORMAT_RGBX8888:
-	case DRM_FORMAT_BGRX8888:
-	case DRM_FORMAT_ARGB8888:
-	case DRM_FORMAT_ABGR8888:
-	case DRM_FORMAT_RGBA8888:
-	case DRM_FORMAT_BGRA8888:
-	case DRM_FORMAT_XRGB2101010:
-	case DRM_FORMAT_XBGR2101010:
-	case DRM_FORMAT_RGBX1010102:
-	case DRM_FORMAT_BGRX1010102:
-	case DRM_FORMAT_ARGB2101010:
-	case DRM_FORMAT_ABGR2101010:
-	case DRM_FORMAT_RGBA1010102:
-	case DRM_FORMAT_BGRA1010102:
-	case DRM_FORMAT_YUYV:
-	case DRM_FORMAT_YVYU:
-	case DRM_FORMAT_UYVY:
-	case DRM_FORMAT_VYUY:
-	case DRM_FORMAT_AYUV:
-	case DRM_FORMAT_NV12:
-	case DRM_FORMAT_NV21:
-	case DRM_FORMAT_NV16:
-	case DRM_FORMAT_NV61:
-	case DRM_FORMAT_NV24:
-	case DRM_FORMAT_NV42:
-	case DRM_FORMAT_YUV410:
-	case DRM_FORMAT_YVU410:
-	case DRM_FORMAT_YUV411:
-	case DRM_FORMAT_YVU411:
-	case DRM_FORMAT_YUV420:
-	case DRM_FORMAT_YVU420:
-	case DRM_FORMAT_YUV422:
-	case DRM_FORMAT_YVU422:
-	case DRM_FORMAT_YUV444:
-	case DRM_FORMAT_YVU444:
-		return 0;
-	default:
-		DRM_DEBUG_KMS("invalid pixel format %s\n",
-			      drm_get_format_name(r->pixel_format));
-		return -EINVAL;
-	}
-}
-
 static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
 {
-	int ret, hsub, vsub, num_planes, i;
+	const struct drm_format_info *info;
+	int i;
 
-	ret = format_check(r);
-	if (ret) {
+	info = drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
+	if (!info) {
 		DRM_DEBUG_KMS("bad framebuffer format %s\n",
 			      drm_get_format_name(r->pixel_format));
-		return ret;
+		return -EINVAL;
 	}
 
-	hsub = drm_format_horz_chroma_subsampling(r->pixel_format);
-	vsub = drm_format_vert_chroma_subsampling(r->pixel_format);
-	num_planes = drm_format_num_planes(r->pixel_format);
-
-	if (r->width == 0 || r->width % hsub) {
+	if (r->width == 0 || r->width % info->hsub) {
 		DRM_DEBUG_KMS("bad framebuffer width %u\n", r->width);
 		return -EINVAL;
 	}
 
-	if (r->height == 0 || r->height % vsub) {
+	if (r->height == 0 || r->height % info->vsub) {
 		DRM_DEBUG_KMS("bad framebuffer height %u\n", r->height);
 		return -EINVAL;
 	}
 
-	for (i = 0; i < num_planes; i++) {
-		unsigned int width = r->width / (i != 0 ? hsub : 1);
-		unsigned int height = r->height / (i != 0 ? vsub : 1);
-		unsigned int cpp = drm_format_plane_cpp(r->pixel_format, i);
+	for (i = 0; i < info->num_planes; i++) {
+		unsigned int width = r->width / (i != 0 ? info->hsub : 1);
+		unsigned int height = r->height / (i != 0 ? info->vsub : 1);
+		unsigned int cpp = info->cpp[i];
 
 		if (!r->handles[i]) {
 			DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i);
@@ -3348,7 +3272,7 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
 		}
 	}
 
-	for (i = num_planes; i < 4; i++) {
+	for (i = info->num_planes; i < 4; i++) {
 		if (r->modifier[i]) {
 			DRM_DEBUG_KMS("non-zero modifier for unused plane %d\n", i);
 			return -EINVAL;
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 5075fae3c4e2..81525a23e4b7 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -171,20 +171,20 @@ struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
 	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
 	const struct drm_framebuffer_funcs *funcs)
 {
+	const struct drm_format_info *info;
 	struct drm_fb_cma *fb_cma;
 	struct drm_gem_cma_object *objs[4];
 	struct drm_gem_object *obj;
-	unsigned int hsub;
-	unsigned int vsub;
 	int ret;
 	int i;
 
-	hsub = drm_format_horz_chroma_subsampling(mode_cmd->pixel_format);
-	vsub = drm_format_vert_chroma_subsampling(mode_cmd->pixel_format);
+	info = drm_format_info(mode_cmd->pixel_format);
+	if (!info)
+		return -EINVAL;
 
-	for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); i++) {
-		unsigned int width = mode_cmd->width / (i ? hsub : 1);
-		unsigned int height = mode_cmd->height / (i ? vsub : 1);
+	for (i = 0; i < info->num_planes; i++) {
+		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
+		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
 		unsigned int min_size;
 
 		obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[i]);
@@ -195,7 +195,7 @@ struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
 		}
 
 		min_size = (height - 1) * mode_cmd->pitches[i]
-			 + width * drm_format_plane_cpp(mode_cmd->pixel_format, i)
+			 + width * info->cpp[i]
 			 + mode_cmd->offsets[i];
 
 		if (obj->size < min_size) {
@@ -265,12 +265,15 @@ EXPORT_SYMBOL_GPL(drm_fb_cma_get_gem_obj);
 static void drm_fb_cma_describe(struct drm_framebuffer *fb, struct seq_file *m)
 {
 	struct drm_fb_cma *fb_cma = to_fb_cma(fb);
-	int i, n = drm_format_num_planes(fb->pixel_format);
+	const struct drm_format_info *info;
+	int i;
 
 	seq_printf(m, "fb: %dx%d@%4.4s\n", fb->width, fb->height,
 			(char *)&fb->pixel_format);
 
-	for (i = 0; i < n; i++) {
+	info = drm_format_info(fb->pixel_format);
+
+	for (i = 0; i < info->num_planes; i++) {
 		seq_printf(m, "   %d: offset=%d pitch=%d, obj: ",
 				i, fb->offsets[i], fb->pitches[i]);
 		drm_gem_cma_describe(fb_cma->obj[i], m);
-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 1/4] drm: Centralize format information

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 819 bytes --]

On 07/06/16 02:33, Laurent Pinchart wrote:

> +/**
> + * struct drm_format_info - information about a DRM format
> + * @format: 4CC format identifier (DRM_FORMAT_*)
> + * @depth: color depth (number of bits per pixel excluding padding bits)
> + * @bpp: number of bits per pixel including padding
> + * @num_planes: number of color planes (1 to 3)
> + * @cpp: number of bytes per pixel (per plane)
> + * @hsub: horizontal chroma subsampling factor
> + * @vsub: vertical chroma subsampling factor
> + */
> +struct drm_format_info {
> +	u32 format;
> +	unsigned int depth;
> +	unsigned int bpp;
> +	unsigned int num_planes;
> +	unsigned int cpp[3];
> +	unsigned int hsub;
> +	unsigned int vsub;
> +};

Any reason not to pack this a bit? All those unsigned ints would fit
easily into u8.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 1/4] drm: Centralize format information

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 5229 bytes --]

On 07/06/16 02:33, Laurent Pinchart wrote:
> Various pieces of information about DRM formats (number of planes, color
> depth, chroma subsampling, ...) are scattered across different helper
> functions in the DRM core. Callers of those functions often need to
> access more than a single parameter of the format, leading to
> inefficiencies due to multiple lookups.
> 
> Centralize all format information in a data structure and create a
> function to look up information based on the format 4CC.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h     | 21 ++++++++++++
>  2 files changed, 104 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 0e3cc66aa8b7..74b0c6dd80cd 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -5544,6 +5544,89 @@ int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
>  }
>  
>  /**
> + * drm_format_info - information for a given format
> + * @format: pixel format (DRM_FORMAT_*)
> + *
> + * Returns:
> + * The instance of struct drm_format_info that describes the pixel format, or
> + * NULL if the format is unsupported.
> + */
> +const struct drm_format_info *drm_format_info(u32 format)
> +{
> +	static const struct drm_format_info formats[] = {
> +		{ DRM_FORMAT_C8, 8, 8, 1, { 1 }, 1, 1 },
> +		{ DRM_FORMAT_RGB332, 8, 8, 1, { 1 }, 1, 1 },
> +		{ DRM_FORMAT_BGR233, 8, 8, 1, { 1 }, 1, 1 },
> +		{ DRM_FORMAT_XRGB4444, 12, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_XBGR4444, 12, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_RGBX4444, 12, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_BGRX4444, 12, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_ARGB4444, 12, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_ABGR4444, 12, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_RGBA4444, 12, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_BGRA4444, 12, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_XRGB1555, 15, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_XBGR1555, 15, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_RGBX5551, 15, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_BGRX5551, 15, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_ARGB1555, 15, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_ABGR1555, 15, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_RGBA5551, 15, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_BGRA5551, 15, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_RGB565, 16, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_BGR565, 16, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_RGB888, 24, 24, 1, { 3 }, 1, 1 },
> +		{ DRM_FORMAT_BGR888, 24, 24, 1, { 3 }, 1, 1 },
> +		{ DRM_FORMAT_XRGB8888, 24, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_XBGR8888, 24, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_RGBX8888, 24, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_BGRX8888, 24, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_XRGB2101010, 30, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_XBGR2101010, 30, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_RGBX1010102, 30, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_BGRX1010102, 30, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_ARGB2101010, 30, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_ABGR2101010, 30, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_RGBA1010102, 30, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_BGRA1010102, 30, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_ARGB8888, 32, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_ABGR8888, 32, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_RGBA8888, 32, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_BGRA8888, 32, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_YUV410, 0, 0, 3, { 1, 1, 1 }, 4, 4 },
> +		{ DRM_FORMAT_YVU410, 0, 0, 3, { 1, 1, 1 }, 4, 4 },
> +		{ DRM_FORMAT_YUV411, 0, 0, 3, { 1, 1, 1 }, 4, 1 },
> +		{ DRM_FORMAT_YVU411, 0, 0, 3, { 1, 1, 1 }, 4, 1 },
> +		{ DRM_FORMAT_YUV420, 0, 0, 3, { 1, 1, 1 }, 2, 2 },
> +		{ DRM_FORMAT_YVU420, 0, 0, 3, { 1, 1, 1 }, 2, 2 },
> +		{ DRM_FORMAT_YUV422, 0, 0, 3, { 1, 1, 1 }, 2, 1 },
> +		{ DRM_FORMAT_YVU422, 0, 0, 3, { 1, 1, 1 }, 2, 1 },
> +		{ DRM_FORMAT_YUV444, 0, 0, 3, { 1, 1, 1 }, 1, 1 },
> +		{ DRM_FORMAT_YVU444, 0, 0, 3, { 1, 1, 1 }, 1, 1 },
> +		{ DRM_FORMAT_NV12, 0, 0, 2, { 1, 2 }, 2, 2 },
> +		{ DRM_FORMAT_NV21, 0, 0, 2, { 1, 2 }, 2, 2 },
> +		{ DRM_FORMAT_NV16, 0, 0, 2, { 1, 2 }, 2, 1 },
> +		{ DRM_FORMAT_NV61, 0, 0, 2, { 1, 2 }, 2, 1 },
> +		{ DRM_FORMAT_NV24, 0, 0, 2, { 1, 2 }, 1, 1 },
> +		{ DRM_FORMAT_NV42, 0, 0, 2, { 1, 2 }, 1, 1 },
> +		{ DRM_FORMAT_YUYV, 0, 0, 1, { 2 }, 2, 1 },
> +		{ DRM_FORMAT_YVYU, 0, 0, 1, { 2 }, 2, 1 },
> +		{ DRM_FORMAT_UYVY, 0, 0, 1, { 2 }, 2, 1 },
> +		{ DRM_FORMAT_VYUY, 0, 0, 1, { 2 }, 2, 1 },
> +		{ DRM_FORMAT_AYUV, 0, 0, 1, { 4 }, 1, 1 },
> +	};
> +
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
> +		if (formats[i].format == format)
> +			return &formats[i];
> +	}
> +
> +	return NULL;

After looking at the third patch, I wonder if it would make sense to
give a warning here if the format was not found. In the third patch many
of the helpers will quietly return a valid value for unknown modes.
Which is what they do at the moment too, but is there ever a valid
reason to do that without something being wrong?

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 1/4] drm: Centralize format information

[Laurent Pinchart]

Hi Tomi,

On Tuesday 07 Jun 2016 12:18:34 Tomi Valkeinen wrote:
> On 07/06/16 02:33, Laurent Pinchart wrote:
> > +/**
> > + * struct drm_format_info - information about a DRM format
> > + * @format: 4CC format identifier (DRM_FORMAT_*)
> > + * @depth: color depth (number of bits per pixel excluding padding bits)
> > + * @bpp: number of bits per pixel including padding
> > + * @num_planes: number of color planes (1 to 3)
> > + * @cpp: number of bytes per pixel (per plane)
> > + * @hsub: horizontal chroma subsampling factor
> > + * @vsub: vertical chroma subsampling factor
> > + */
> > +struct drm_format_info {
> > +	u32 format;
> > +	unsigned int depth;
> > +	unsigned int bpp;
> > +	unsigned int num_planes;
> > +	unsigned int cpp[3];
> > +	unsigned int hsub;
> > +	unsigned int vsub;
> > +};
> 
> Any reason not to pack this a bit? All those unsigned ints would fit
> easily into u8.

Good point, I'll do that.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 1/4] drm: Centralize format information

[Laurent Pinchart]

Hi Tomi,

On Tuesday 07 Jun 2016 12:25:08 Tomi Valkeinen wrote:
> On 07/06/16 02:33, Laurent Pinchart wrote:
> > Various pieces of information about DRM formats (number of planes, color
> > depth, chroma subsampling, ...) are scattered across different helper
> > functions in the DRM core. Callers of those functions often need to
> > access more than a single parameter of the format, leading to
> > inefficiencies due to multiple lookups.
> > 
> > Centralize all format information in a data structure and create a
> > function to look up information based on the format 4CC.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/drm_crtc.c | 83 +++++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_crtc.h     | 21 ++++++++++++
> >  2 files changed, 104 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 0e3cc66aa8b7..74b0c6dd80cd 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -5544,6 +5544,89 @@ int drm_mode_destroy_dumb_ioctl(struct drm_device
> > *dev,> 
> >  }
> >  
> >  /**
> > 
> > + * drm_format_info - information for a given format
> > + * @format: pixel format (DRM_FORMAT_*)
> > + *
> > + * Returns:
> > + * The instance of struct drm_format_info that describes the pixel
> > format, or + * NULL if the format is unsupported.
> > + */
> > +const struct drm_format_info *drm_format_info(u32 format)
> > +{
> > +	static const struct drm_format_info formats[] = {
> > +		{ DRM_FORMAT_C8, 8, 8, 1, { 1 }, 1, 1 },
> > +		{ DRM_FORMAT_RGB332, 8, 8, 1, { 1 }, 1, 1 },
> > +		{ DRM_FORMAT_BGR233, 8, 8, 1, { 1 }, 1, 1 },
> > +		{ DRM_FORMAT_XRGB4444, 12, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_XBGR4444, 12, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_RGBX4444, 12, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_BGRX4444, 12, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_ARGB4444, 12, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_ABGR4444, 12, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_RGBA4444, 12, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_BGRA4444, 12, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_XRGB1555, 15, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_XBGR1555, 15, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_RGBX5551, 15, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_BGRX5551, 15, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_ARGB1555, 15, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_ABGR1555, 15, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_RGBA5551, 15, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_BGRA5551, 15, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_RGB565, 16, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_BGR565, 16, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_RGB888, 24, 24, 1, { 3 }, 1, 1 },
> > +		{ DRM_FORMAT_BGR888, 24, 24, 1, { 3 }, 1, 1 },
> > +		{ DRM_FORMAT_XRGB8888, 24, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_XBGR8888, 24, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_RGBX8888, 24, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_BGRX8888, 24, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_XRGB2101010, 30, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_XBGR2101010, 30, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_RGBX1010102, 30, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_BGRX1010102, 30, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_ARGB2101010, 30, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_ABGR2101010, 30, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_RGBA1010102, 30, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_BGRA1010102, 30, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_ARGB8888, 32, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_ABGR8888, 32, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_RGBA8888, 32, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_BGRA8888, 32, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_YUV410, 0, 0, 3, { 1, 1, 1 }, 4, 4 },
> > +		{ DRM_FORMAT_YVU410, 0, 0, 3, { 1, 1, 1 }, 4, 4 },
> > +		{ DRM_FORMAT_YUV411, 0, 0, 3, { 1, 1, 1 }, 4, 1 },
> > +		{ DRM_FORMAT_YVU411, 0, 0, 3, { 1, 1, 1 }, 4, 1 },
> > +		{ DRM_FORMAT_YUV420, 0, 0, 3, { 1, 1, 1 }, 2, 2 },
> > +		{ DRM_FORMAT_YVU420, 0, 0, 3, { 1, 1, 1 }, 2, 2 },
> > +		{ DRM_FORMAT_YUV422, 0, 0, 3, { 1, 1, 1 }, 2, 1 },
> > +		{ DRM_FORMAT_YVU422, 0, 0, 3, { 1, 1, 1 }, 2, 1 },
> > +		{ DRM_FORMAT_YUV444, 0, 0, 3, { 1, 1, 1 }, 1, 1 },
> > +		{ DRM_FORMAT_YVU444, 0, 0, 3, { 1, 1, 1 }, 1, 1 },
> > +		{ DRM_FORMAT_NV12, 0, 0, 2, { 1, 2 }, 2, 2 },
> > +		{ DRM_FORMAT_NV21, 0, 0, 2, { 1, 2 }, 2, 2 },
> > +		{ DRM_FORMAT_NV16, 0, 0, 2, { 1, 2 }, 2, 1 },
> > +		{ DRM_FORMAT_NV61, 0, 0, 2, { 1, 2 }, 2, 1 },
> > +		{ DRM_FORMAT_NV24, 0, 0, 2, { 1, 2 }, 1, 1 },
> > +		{ DRM_FORMAT_NV42, 0, 0, 2, { 1, 2 }, 1, 1 },
> > +		{ DRM_FORMAT_YUYV, 0, 0, 1, { 2 }, 2, 1 },
> > +		{ DRM_FORMAT_YVYU, 0, 0, 1, { 2 }, 2, 1 },
> > +		{ DRM_FORMAT_UYVY, 0, 0, 1, { 2 }, 2, 1 },
> > +		{ DRM_FORMAT_VYUY, 0, 0, 1, { 2 }, 2, 1 },
> > +		{ DRM_FORMAT_AYUV, 0, 0, 1, { 4 }, 1, 1 },
> > +	};
> > +
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
> > +		if (formats[i].format == format)
> > +			return &formats[i];
> > +	}
> > +
> > +	return NULL;
> 
> After looking at the third patch, I wonder if it would make sense to
> give a warning here if the format was not found. In the third patch many
> of the helpers will quietly return a valid value for unknown modes.
> Which is what they do at the moment too, but is there ever a valid
> reason to do that without something being wrong?

We can't warn in this function due to its usage in framebuffer_check() that 
uses drm_format_info() to validate the format provided by userspace. However, 
I agree that all other call sites should not pass an unsupported format, that 
would be a bug in the caller.

I can rename the function to __drm_format_info(), call that in 
framebuffer_check(), and create a drm_format_info() wrapper with a WARN_ON if 
the format isn't found. Would you prefer that ?

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 2/4] drm: Remove unused drm_format_plane_(width|height) helpers

[Ville Syrjälä]

On Tue, Jun 07, 2016 at 02:33:12AM +0300, Laurent Pinchart wrote:
> The drm_format_plane_width() and drm_format_plane_height() helper
> functions are not used, remove them.

I have a user lined up, assuming I could get the dang thing reviewed.

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 42 ------------------------------------------
>  include/drm/drm_crtc.h     |  2 --
>  2 files changed, 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 74b0c6dd80cd..b405d4379e47 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -5843,48 +5843,6 @@ int drm_format_vert_chroma_subsampling(uint32_t format)
>  EXPORT_SYMBOL(drm_format_vert_chroma_subsampling);
>  
>  /**
> - * drm_format_plane_width - width of the plane given the first plane
> - * @width: width of the first plane
> - * @format: pixel format
> - * @plane: plane index
> - *
> - * Returns:
> - * The width of @plane, given that the width of the first plane is @width.
> - */
> -int drm_format_plane_width(int width, uint32_t format, int plane)
> -{
> -	if (plane >= drm_format_num_planes(format))
> -		return 0;
> -
> -	if (plane == 0)
> -		return width;
> -
> -	return width / drm_format_horz_chroma_subsampling(format);
> -}
> -EXPORT_SYMBOL(drm_format_plane_width);
> -
> -/**
> - * drm_format_plane_height - height of the plane given the first plane
> - * @height: height of the first plane
> - * @format: pixel format
> - * @plane: plane index
> - *
> - * Returns:
> - * The height of @plane, given that the height of the first plane is @height.
> - */
> -int drm_format_plane_height(int height, uint32_t format, int plane)
> -{
> -	if (plane >= drm_format_num_planes(format))
> -		return 0;
> -
> -	if (plane == 0)
> -		return height;
> -
> -	return height / drm_format_vert_chroma_subsampling(format);
> -}
> -EXPORT_SYMBOL(drm_format_plane_height);
> -
> -/**
>   * drm_rotation_simplify() - Try to simplify the rotation
>   * @rotation: Rotation to be simplified
>   * @supported_rotations: Supported rotations
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 4199794cc317..a45f57f32dca 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -2567,8 +2567,6 @@ extern int drm_format_num_planes(uint32_t format);
>  extern int drm_format_plane_cpp(uint32_t format, int plane);
>  extern int drm_format_horz_chroma_subsampling(uint32_t format);
>  extern int drm_format_vert_chroma_subsampling(uint32_t format);
> -extern int drm_format_plane_width(int width, uint32_t format, int plane);
> -extern int drm_format_plane_height(int height, uint32_t format, int plane);
>  extern const char *drm_get_format_name(uint32_t format);
>  extern struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev,
>  							      unsigned int supported_rotations);
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm: Centralize format information

[Ville Syrjälä]

On Tue, Jun 07, 2016 at 02:33:11AM +0300, Laurent Pinchart wrote:
> Various pieces of information about DRM formats (number of planes, color
> depth, chroma subsampling, ...) are scattered across different helper
> functions in the DRM core. Callers of those functions often need to
> access more than a single parameter of the format, leading to
> inefficiencies due to multiple lookups.
> 
> Centralize all format information in a data structure and create a
> function to look up information based on the format 4CC.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h     | 21 ++++++++++++
>  2 files changed, 104 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 0e3cc66aa8b7..74b0c6dd80cd 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -5544,6 +5544,89 @@ int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
>  }
>  
>  /**
> + * drm_format_info - information for a given format
> + * @format: pixel format (DRM_FORMAT_*)
> + *
> + * Returns:
> + * The instance of struct drm_format_info that describes the pixel format, or
> + * NULL if the format is unsupported.
> + */
> +const struct drm_format_info *drm_format_info(u32 format)
> +{
> +	static const struct drm_format_info formats[] = {
> +		{ DRM_FORMAT_C8, 8, 8, 1, { 1 }, 1, 1 },

Named initializers please.

> +		{ DRM_FORMAT_RGB332, 8, 8, 1, { 1 }, 1, 1 },
> +		{ DRM_FORMAT_BGR233, 8, 8, 1, { 1 }, 1, 1 },
> +		{ DRM_FORMAT_XRGB4444, 12, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_XBGR4444, 12, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_RGBX4444, 12, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_BGRX4444, 12, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_ARGB4444, 12, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_ABGR4444, 12, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_RGBA4444, 12, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_BGRA4444, 12, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_XRGB1555, 15, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_XBGR1555, 15, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_RGBX5551, 15, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_BGRX5551, 15, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_ARGB1555, 15, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_ABGR1555, 15, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_RGBA5551, 15, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_BGRA5551, 15, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_RGB565, 16, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_BGR565, 16, 16, 1, { 2 }, 1, 1 },
> +		{ DRM_FORMAT_RGB888, 24, 24, 1, { 3 }, 1, 1 },
> +		{ DRM_FORMAT_BGR888, 24, 24, 1, { 3 }, 1, 1 },
> +		{ DRM_FORMAT_XRGB8888, 24, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_XBGR8888, 24, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_RGBX8888, 24, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_BGRX8888, 24, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_XRGB2101010, 30, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_XBGR2101010, 30, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_RGBX1010102, 30, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_BGRX1010102, 30, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_ARGB2101010, 30, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_ABGR2101010, 30, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_RGBA1010102, 30, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_BGRA1010102, 30, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_ARGB8888, 32, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_ABGR8888, 32, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_RGBA8888, 32, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_BGRA8888, 32, 32, 1, { 4 }, 1, 1 },
> +		{ DRM_FORMAT_YUV410, 0, 0, 3, { 1, 1, 1 }, 4, 4 },
> +		{ DRM_FORMAT_YVU410, 0, 0, 3, { 1, 1, 1 }, 4, 4 },
> +		{ DRM_FORMAT_YUV411, 0, 0, 3, { 1, 1, 1 }, 4, 1 },
> +		{ DRM_FORMAT_YVU411, 0, 0, 3, { 1, 1, 1 }, 4, 1 },
> +		{ DRM_FORMAT_YUV420, 0, 0, 3, { 1, 1, 1 }, 2, 2 },
> +		{ DRM_FORMAT_YVU420, 0, 0, 3, { 1, 1, 1 }, 2, 2 },
> +		{ DRM_FORMAT_YUV422, 0, 0, 3, { 1, 1, 1 }, 2, 1 },
> +		{ DRM_FORMAT_YVU422, 0, 0, 3, { 1, 1, 1 }, 2, 1 },
> +		{ DRM_FORMAT_YUV444, 0, 0, 3, { 1, 1, 1 }, 1, 1 },
> +		{ DRM_FORMAT_YVU444, 0, 0, 3, { 1, 1, 1 }, 1, 1 },
> +		{ DRM_FORMAT_NV12, 0, 0, 2, { 1, 2 }, 2, 2 },
> +		{ DRM_FORMAT_NV21, 0, 0, 2, { 1, 2 }, 2, 2 },
> +		{ DRM_FORMAT_NV16, 0, 0, 2, { 1, 2 }, 2, 1 },
> +		{ DRM_FORMAT_NV61, 0, 0, 2, { 1, 2 }, 2, 1 },
> +		{ DRM_FORMAT_NV24, 0, 0, 2, { 1, 2 }, 1, 1 },
> +		{ DRM_FORMAT_NV42, 0, 0, 2, { 1, 2 }, 1, 1 },
> +		{ DRM_FORMAT_YUYV, 0, 0, 1, { 2 }, 2, 1 },
> +		{ DRM_FORMAT_YVYU, 0, 0, 1, { 2 }, 2, 1 },
> +		{ DRM_FORMAT_UYVY, 0, 0, 1, { 2 }, 2, 1 },
> +		{ DRM_FORMAT_VYUY, 0, 0, 1, { 2 }, 2, 1 },
> +		{ DRM_FORMAT_AYUV, 0, 0, 1, { 4 }, 1, 1 },
> +	};
> +
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
> +		if (formats[i].format == format)
> +			return &formats[i];
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
>   * drm_fb_get_bpp_depth - get the bpp/depth values for format
>   * @format: pixel format (DRM_FORMAT_*)
>   * @depth: storage for the depth value
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index d1559cd04e3d..4199794cc317 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -155,6 +155,26 @@ struct drm_display_info {
>  	u8 cea_rev;
>  };
>  
> +/**
> + * struct drm_format_info - information about a DRM format
> + * @format: 4CC format identifier (DRM_FORMAT_*)
> + * @depth: color depth (number of bits per pixel excluding padding bits)
> + * @bpp: number of bits per pixel including padding
> + * @num_planes: number of color planes (1 to 3)
> + * @cpp: number of bytes per pixel (per plane)
> + * @hsub: horizontal chroma subsampling factor
> + * @vsub: vertical chroma subsampling factor
> + */
> +struct drm_format_info {
> +	u32 format;
> +	unsigned int depth;
> +	unsigned int bpp;
> +	unsigned int num_planes;
> +	unsigned int cpp[3];
> +	unsigned int hsub;
> +	unsigned int vsub;
> +};
> +
>  /* data corresponds to displayid vend/prod/serial */
>  struct drm_tile_group {
>  	struct kref refcount;
> @@ -2540,6 +2560,7 @@ extern int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
>  extern int drm_mode_atomic_ioctl(struct drm_device *dev,
>  				 void *data, struct drm_file *file_priv);
>  
> +extern const struct drm_format_info *drm_format_info(u32 format);
>  extern void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
>  				 int *bpp);
>  extern int drm_format_num_planes(uint32_t format);
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm: Remove unused drm_format_plane_(width|height) helpers

[Laurent Pinchart]

Hi Ville,

On Tuesday 07 Jun 2016 16:19:01 Ville Syrjälä wrote:
> On Tue, Jun 07, 2016 at 02:33:12AM +0300, Laurent Pinchart wrote:
> > The drm_format_plane_width() and drm_format_plane_height() helper
> > functions are not used, remove them.
> 
> I have a user lined up, assuming I could get the dang thing reviewed.

OK, I'll keep the functions and convert them to use drm_format_info(). If your 
user needs to call more than one format helper then I'd suggest using 
drm_format_info() directly and perform the width/height computation yourself, 
it will be more efficient.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/drm_crtc.c | 42 ----------------------------------------
> >  include/drm/drm_crtc.h     |  2 --
> >  2 files changed, 44 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 74b0c6dd80cd..b405d4379e47 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -5843,48 +5843,6 @@ int drm_format_vert_chroma_subsampling(uint32_t
> > format)> 
> >  EXPORT_SYMBOL(drm_format_vert_chroma_subsampling);
> >  
> >  /**
> > 
> > - * drm_format_plane_width - width of the plane given the first plane
> > - * @width: width of the first plane
> > - * @format: pixel format
> > - * @plane: plane index
> > - *
> > - * Returns:
> > - * The width of @plane, given that the width of the first plane is
> > @width.
> > - */
> > -int drm_format_plane_width(int width, uint32_t format, int plane)
> > -{
> > -	if (plane >= drm_format_num_planes(format))
> > -		return 0;
> > -
> > -	if (plane == 0)
> > -		return width;
> > -
> > -	return width / drm_format_horz_chroma_subsampling(format);
> > -}
> > -EXPORT_SYMBOL(drm_format_plane_width);
> > -
> > -/**
> > - * drm_format_plane_height - height of the plane given the first plane
> > - * @height: height of the first plane
> > - * @format: pixel format
> > - * @plane: plane index
> > - *
> > - * Returns:
> > - * The height of @plane, given that the height of the first plane is
> > @height. - */
> > -int drm_format_plane_height(int height, uint32_t format, int plane)
> > -{
> > -	if (plane >= drm_format_num_planes(format))
> > -		return 0;
> > -
> > -	if (plane == 0)
> > -		return height;
> > -
> > -	return height / drm_format_vert_chroma_subsampling(format);
> > -}
> > -EXPORT_SYMBOL(drm_format_plane_height);
> > -
> > -/**
> > 
> >   * drm_rotation_simplify() - Try to simplify the rotation
> >   * @rotation: Rotation to be simplified
> >   * @supported_rotations: Supported rotations
> > 
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 4199794cc317..a45f57f32dca 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -2567,8 +2567,6 @@ extern int drm_format_num_planes(uint32_t format);
> > 
> >  extern int drm_format_plane_cpp(uint32_t format, int plane);
> >  extern int drm_format_horz_chroma_subsampling(uint32_t format);
> >  extern int drm_format_vert_chroma_subsampling(uint32_t format);
> > 
> > -extern int drm_format_plane_width(int width, uint32_t format, int plane);
> > -extern int drm_format_plane_height(int height, uint32_t format, int
> > plane);> 
> >  extern const char *drm_get_format_name(uint32_t format);
> >  extern struct drm_property *drm_mode_create_rotation_property(struct
> >  drm_device *dev,>  
> >  							      unsigned int supported_rotations);

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 1/4] drm: Centralize format information

[Laurent Pinchart]

Hi Ville,

On Tuesday 07 Jun 2016 16:20:17 Ville Syrjälä wrote:
> On Tue, Jun 07, 2016 at 02:33:11AM +0300, Laurent Pinchart wrote:
> > Various pieces of information about DRM formats (number of planes, color
> > depth, chroma subsampling, ...) are scattered across different helper
> > functions in the DRM core. Callers of those functions often need to
> > access more than a single parameter of the format, leading to
> > inefficiencies due to multiple lookups.
> > 
> > Centralize all format information in a data structure and create a
> > function to look up information based on the format 4CC.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/drm_crtc.c | 83 +++++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_crtc.h     | 21 ++++++++++++
> >  2 files changed, 104 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 0e3cc66aa8b7..74b0c6dd80cd 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -5544,6 +5544,89 @@ int drm_mode_destroy_dumb_ioctl(struct drm_device
> > *dev,> 
> >  }
> >  
> >  /**
> > 
> > + * drm_format_info - information for a given format
> > + * @format: pixel format (DRM_FORMAT_*)
> > + *
> > + * Returns:
> > + * The instance of struct drm_format_info that describes the pixel
> > format, or + * NULL if the format is unsupported.
> > + */
> > +const struct drm_format_info *drm_format_info(u32 format)
> > +{
> > +	static const struct drm_format_info formats[] = {
> > +		{ DRM_FORMAT_C8, 8, 8, 1, { 1 }, 1, 1 },
> 
> Named initializers please.

Do we really want to expand every entry to 7 lines ? I can do so, but it won't 
look pretty.

> > +		{ DRM_FORMAT_RGB332, 8, 8, 1, { 1 }, 1, 1 },
> > +		{ DRM_FORMAT_BGR233, 8, 8, 1, { 1 }, 1, 1 },
> > +		{ DRM_FORMAT_XRGB4444, 12, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_XBGR4444, 12, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_RGBX4444, 12, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_BGRX4444, 12, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_ARGB4444, 12, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_ABGR4444, 12, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_RGBA4444, 12, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_BGRA4444, 12, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_XRGB1555, 15, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_XBGR1555, 15, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_RGBX5551, 15, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_BGRX5551, 15, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_ARGB1555, 15, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_ABGR1555, 15, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_RGBA5551, 15, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_BGRA5551, 15, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_RGB565, 16, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_BGR565, 16, 16, 1, { 2 }, 1, 1 },
> > +		{ DRM_FORMAT_RGB888, 24, 24, 1, { 3 }, 1, 1 },
> > +		{ DRM_FORMAT_BGR888, 24, 24, 1, { 3 }, 1, 1 },
> > +		{ DRM_FORMAT_XRGB8888, 24, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_XBGR8888, 24, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_RGBX8888, 24, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_BGRX8888, 24, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_XRGB2101010, 30, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_XBGR2101010, 30, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_RGBX1010102, 30, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_BGRX1010102, 30, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_ARGB2101010, 30, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_ABGR2101010, 30, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_RGBA1010102, 30, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_BGRA1010102, 30, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_ARGB8888, 32, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_ABGR8888, 32, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_RGBA8888, 32, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_BGRA8888, 32, 32, 1, { 4 }, 1, 1 },
> > +		{ DRM_FORMAT_YUV410, 0, 0, 3, { 1, 1, 1 }, 4, 4 },
> > +		{ DRM_FORMAT_YVU410, 0, 0, 3, { 1, 1, 1 }, 4, 4 },
> > +		{ DRM_FORMAT_YUV411, 0, 0, 3, { 1, 1, 1 }, 4, 1 },
> > +		{ DRM_FORMAT_YVU411, 0, 0, 3, { 1, 1, 1 }, 4, 1 },
> > +		{ DRM_FORMAT_YUV420, 0, 0, 3, { 1, 1, 1 }, 2, 2 },
> > +		{ DRM_FORMAT_YVU420, 0, 0, 3, { 1, 1, 1 }, 2, 2 },
> > +		{ DRM_FORMAT_YUV422, 0, 0, 3, { 1, 1, 1 }, 2, 1 },
> > +		{ DRM_FORMAT_YVU422, 0, 0, 3, { 1, 1, 1 }, 2, 1 },
> > +		{ DRM_FORMAT_YUV444, 0, 0, 3, { 1, 1, 1 }, 1, 1 },
> > +		{ DRM_FORMAT_YVU444, 0, 0, 3, { 1, 1, 1 }, 1, 1 },
> > +		{ DRM_FORMAT_NV12, 0, 0, 2, { 1, 2 }, 2, 2 },
> > +		{ DRM_FORMAT_NV21, 0, 0, 2, { 1, 2 }, 2, 2 },
> > +		{ DRM_FORMAT_NV16, 0, 0, 2, { 1, 2 }, 2, 1 },
> > +		{ DRM_FORMAT_NV61, 0, 0, 2, { 1, 2 }, 2, 1 },
> > +		{ DRM_FORMAT_NV24, 0, 0, 2, { 1, 2 }, 1, 1 },
> > +		{ DRM_FORMAT_NV42, 0, 0, 2, { 1, 2 }, 1, 1 },
> > +		{ DRM_FORMAT_YUYV, 0, 0, 1, { 2 }, 2, 1 },
> > +		{ DRM_FORMAT_YVYU, 0, 0, 1, { 2 }, 2, 1 },
> > +		{ DRM_FORMAT_UYVY, 0, 0, 1, { 2 }, 2, 1 },
> > +		{ DRM_FORMAT_VYUY, 0, 0, 1, { 2 }, 2, 1 },
> > +		{ DRM_FORMAT_AYUV, 0, 0, 1, { 4 }, 1, 1 },
> > +	};
> > +
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
> > +		if (formats[i].format == format)
> > +			return &formats[i];
> > +	}
> > +
> > +	return NULL;
> > +}

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 0/4] Centralize format information

[Daniel Vetter]

On Tue, Jun 07, 2016 at 02:33:10AM +0300, Laurent Pinchart wrote:
> Hello,
> 
> Various pieces of information about DRM formats (number of planes, color
> depth, chroma subsampling, ...) are scattered across different helper
> functions in the DRM core. Callers of those functions often need to access
> more than a single parameter of the format, leading to inefficiencies due to
> multiple lookups.
> 
> This patch series addresses this issue by centralizing all format information
> in a single data structure (1/4). It reimplements the existing format helper
> functions based on that structure (3/4) and converts the DRM core code to use
> the new structure (4/4). Two unused format helper functions are removed in the
> process (2/4).
> 
> The new API is also useful for drivers. I will shortly post a patch series for
> the omapdrm driver that makes use of it.

I'm still meh on this, but you could convince me if you'd extract all the
format related stuff into drm_fourcc.c. drm_crtc is a mess, and the abi
docs are confusing since everything is in one bag. Splitting parts out
would be awesome. Other stuff I think we could split out is all the
framebuffer handling, basic property stuff, specialized properties (for
zorder, blending, whatever) and maybe even a few more sub-topics. But
let's start somewhere.
-Daniel

> 
> Laurent Pinchart (4):
>   drm: Centralize format information
>   drm: Remove unused drm_format_plane_(width|height) helpers
>   drm: Implement the drm_format_*() helpers as drm_format_info()
>     wrappers
>   drm: Use drm_format_info() in DRM core code
> 
>  drivers/gpu/drm/drm_crtc.c          | 391 +++++++++++-------------------------
>  drivers/gpu/drm/drm_fb_cma_helper.c |  23 ++-
>  include/drm/drm_crtc.h              |  23 ++-
>  3 files changed, 153 insertions(+), 284 deletions(-)
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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 0/4] Centralize format information

[Laurent Pinchart]

Hi Daniel,

On Tuesday 07 Jun 2016 15:27:09 Daniel Vetter wrote:
> On Tue, Jun 07, 2016 at 02:33:10AM +0300, Laurent Pinchart wrote:
> > Hello,
> > 
> > Various pieces of information about DRM formats (number of planes, color
> > depth, chroma subsampling, ...) are scattered across different helper
> > functions in the DRM core. Callers of those functions often need to access
> > more than a single parameter of the format, leading to inefficiencies due
> > to multiple lookups.
> > 
> > This patch series addresses this issue by centralizing all format
> > information in a single data structure (1/4). It reimplements the
> > existing format helper functions based on that structure (3/4) and
> > converts the DRM core code to use the new structure (4/4). Two unused
> > format helper functions are removed in the process (2/4).
> > 
> > The new API is also useful for drivers. I will shortly post a patch series
> > for the omapdrm driver that makes use of it.
> 
> I'm still meh on this, but you could convince me if you'd extract all the
> format related stuff into drm_fourcc.c. drm_crtc is a mess, and the abi
> docs are confusing since everything is in one bag. Splitting parts out
> would be awesome.

I think we're on the same page, I had the exact same thought today. I'll work 
on that.

> Other stuff I think we could split out is all the framebuffer handling,
> basic property stuff, specialized properties (for zorder, blending,
> whatever) and maybe even a few more sub-topics. But let's start somewhere.

Deal, I'll start here :-)

-- 
Regards,

Laurent Pinchart

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