[PATCH v4 00/14] Centralize format information

[PATCH v4 00/14] 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 (01/14). It reimplements the existing format helper
functions based on that structure (02/14) and converts the DRM core code to
use the new structure (03/14). The DRM core now WARNs when a driver tries to
query information about an unsupported format (04/14).

The second part of the patch series removes the drm_fb_get_bpp_depth() legacy
function that shouldn't be used directly by drivers. It modifies all its users
to use the appropriate API instead (05/14 to 13/14) and finally merges the
function into its only caller in the DRM core (14/14).

The new API is also useful for drivers as shown by the "[PATCH v2 00/20] OMAP
DRM fixes and improvements" patch series previously posted (and which I have
rebased and will repost soon).

Changes since v3:

- Rebased on top of latest drm/master branch
- Collected acks
- Dropped "drm: Move format-related helpers to drm_fourcc.c" and
  "drm/msm: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()" that
  have been merged already
- Added new "drm/arm: mali-dp: Replace drm_fb_get_bpp_depth() with
  drm_format_plane_cpp()" patch
- Coding style fixes and variable renames

Changes since v2:

- Remove bpp field from drm_format_info structure
- Replace all users of drm_fb_get_bpp_depth() with the appropriate API
- Merge drm_fb_get_bpp_depth() into its only caller

Changes since v1:

- Move format-related helpers to drm_fourcc.c
- Use named initializers for the formats array
- WARN when calling drm_format_info() for an unsupported format
- Don't drop the drm_format_plane_width() and drm_format_plane_height()
  helpers

Laurent Pinchart (14):
  drm: Centralize format information
  drm: Implement the drm_format_*() helpers as drm_format_info()
    wrappers
  drm: Use drm_format_info() in DRM core code
  drm: WARN when calling drm_format_info() for an unsupported format
  drm: sti: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()
  drm: hdlcd: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()
  drm: tilcdc: Replace drm_fb_get_bpp_depth() with
    drm_format_plane_cpp()
  drm: cirrus: Replace drm_fb_get_bpp_depth() with
    drm_format_plane_cpp()
  drm: gma500: Replace drm_fb_get_bpp_depth() with drm_format_info()
  drm: amdgpu: Replace drm_fb_get_bpp_depth() with
    drm_format_plane_cpp()
  drm: radeon: Replace drm_fb_get_bpp_depth() with
    drm_format_plane_cpp()
  drm: vmwgfx: Replace drm_fb_get_bpp_depth() with drm_format_info()
  drm/arm: mali-dp: Replace drm_fb_get_bpp_depth() with
    drm_format_plane_cpp()
  drm: Don't export the drm_fb_get_bpp_depth() function

 Documentation/gpu/drm-kms.rst           |   3 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c  |  14 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c |   3 +-
 drivers/gpu/drm/arm/hdlcd_crtc.c        |   5 +-
 drivers/gpu/drm/arm/malidp_hw.c         |   7 +-
 drivers/gpu/drm/cirrus/cirrus_fbdev.c   |   6 +-
 drivers/gpu/drm/cirrus/cirrus_main.c    |   4 +-
 drivers/gpu/drm/drm_fb_cma_helper.c     |  23 +--
 drivers/gpu/drm/drm_fourcc.c            | 281 ++++++++++++++------------------
 drivers/gpu/drm/drm_framebuffer.c       | 102 ++----------
 drivers/gpu/drm/drm_modeset_helper.c    |  17 +-
 drivers/gpu/drm/gma500/framebuffer.c    |  20 +--
 drivers/gpu/drm/radeon/radeon_fb.c      |  20 +--
 drivers/gpu/drm/radeon/radeon_gem.c     |   3 +-
 drivers/gpu/drm/sti/sti_gdp.c           |   6 +-
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c    |  15 +-
 drivers/gpu/drm/tilcdc/tilcdc_plane.c   |   7 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c     |  12 +-
 include/drm/drm_fourcc.h                |  21 ++-
 19 files changed, 242 insertions(+), 327 deletions(-)

-- 
Regards,

Laurent Pinchart

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

[PATCH v4 01/14] 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>
---
 Documentation/gpu/drm-kms.rst |  3 ++
 drivers/gpu/drm/drm_fourcc.c  | 84 +++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_fourcc.h      | 19 ++++++++++
 3 files changed, 106 insertions(+)

diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index f9a991bb87d4..85c4c49f4436 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -63,6 +63,9 @@ Frame Buffer Functions Reference
 DRM Format Handling
 ===================
 
+.. kernel-doc:: include/drm/drm_fourcc.h
+   :internal:
+
 .. kernel-doc:: drivers/gpu/drm/drm_fourcc.c
    :export:
 
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 29c56b4331e0..6b91bd8a510d 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -103,6 +103,90 @@ char *drm_get_format_name(uint32_t format)
 EXPORT_SYMBOL(drm_get_format_name);
 
 /**
+ * drm_format_info - query 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[] = {
+		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGB332,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGR233,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_XRGB4444,	.depth = 12, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_XBGR4444,	.depth = 12, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGBX4444,	.depth = 12, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGRX4444,	.depth = 12, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_ARGB4444,	.depth = 12, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_ABGR4444,	.depth = 12, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGBA4444,	.depth = 12, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGRA4444,	.depth = 12, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_XRGB1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_XBGR1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGBX5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGRX5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_ARGB1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_ABGR1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGBA5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGRA5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGB565,		.depth = 16, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGR565,		.depth = 16, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGB888,		.depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGR888,		.depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_XRGB8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_XBGR8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGBX8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGRX8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_XRGB2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_XBGR2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGBX1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGRX1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_ARGB2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_ABGR2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGBA1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGRA1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_ARGB8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_ABGR8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGBA8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGRA8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_YUV410,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 },
+		{ .format = DRM_FORMAT_YVU410,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 },
+		{ .format = DRM_FORMAT_YUV411,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 1 },
+		{ .format = DRM_FORMAT_YVU411,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 1 },
+		{ .format = DRM_FORMAT_YUV420,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 },
+		{ .format = DRM_FORMAT_YVU420,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 },
+		{ .format = DRM_FORMAT_YUV422,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1 },
+		{ .format = DRM_FORMAT_YVU422,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1 },
+		{ .format = DRM_FORMAT_YUV444,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_YVU444,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_NV12,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 },
+		{ .format = DRM_FORMAT_NV21,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 },
+		{ .format = DRM_FORMAT_NV16,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 },
+		{ .format = DRM_FORMAT_NV61,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 },
+		{ .format = DRM_FORMAT_NV24,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_NV42,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_YUYV,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
+		{ .format = DRM_FORMAT_YVYU,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
+		{ .format = DRM_FORMAT_UYVY,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
+		{ .format = DRM_FORMAT_VYUY,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
+		{ .format = DRM_FORMAT_AYUV,		.depth = 0,  .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+	};
+
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
+		if (formats[i].format == format)
+			return &formats[i];
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(drm_format_info);
+
+/**
  * 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_fourcc.h b/include/drm/drm_fourcc.h
index 30c30fa87ee8..4e79d6159856 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -25,6 +25,25 @@
 #include <linux/types.h>
 #include <uapi/drm/drm_fourcc.h>
 
+/**
+ * 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)
+ * @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;
+	u8 depth;
+	u8 num_planes;
+	u8 cpp[3];
+	u8 hsub;
+	u8 vsub;
+};
+
+const struct drm_format_info *drm_format_info(u32 format);
 uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
 void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth, int *bpp);
 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 v4 02/14] 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_fourcc.c | 186 +++++++++----------------------------------
 1 file changed, 37 insertions(+), 149 deletions(-)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 6b91bd8a510d..bf91c5044d84 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -198,69 +198,22 @@ EXPORT_SYMBOL(drm_format_info);
 void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
 			  int *bpp)
 {
-	char *format_name;
-
-	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:
-		format_name = drm_get_format_name(format);
+	const struct drm_format_info *info;
+
+	info = drm_format_info(format);
+	if (!info || !info->depth) {
+		char *format_name = drm_get_format_name(format);
+
 		DRM_DEBUG_KMS("unsupported pixel format %s\n", format_name);
 		kfree(format_name);
+
 		*depth = 0;
 		*bpp = 0;
-		break;
+		return;
 	}
+
+	*depth = info->depth;
+	*bpp = info->cpp[0] << 3;
 }
 EXPORT_SYMBOL(drm_fb_get_bpp_depth);
 
@@ -273,28 +226,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);
 
@@ -308,40 +243,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);
 
@@ -355,28 +263,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);
 
@@ -390,18 +280,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);
 
@@ -416,13 +298,16 @@ EXPORT_SYMBOL(drm_format_vert_chroma_subsampling);
  */
 int drm_format_plane_width(int width, uint32_t format, int plane)
 {
-	if (plane >= drm_format_num_planes(format))
+	const struct drm_format_info *info;
+
+	info = drm_format_info(format);
+	if (!info || plane >= info->num_planes)
 		return 0;
 
 	if (plane == 0)
 		return width;
 
-	return width / drm_format_horz_chroma_subsampling(format);
+	return width / info->hsub;
 }
 EXPORT_SYMBOL(drm_format_plane_width);
 
@@ -437,12 +322,15 @@ EXPORT_SYMBOL(drm_format_plane_width);
  */
 int drm_format_plane_height(int height, uint32_t format, int plane)
 {
-	if (plane >= drm_format_num_planes(format))
+	const struct drm_format_info *info;
+
+	info = drm_format_info(format);
+	if (!info || plane >= info->num_planes)
 		return 0;
 
 	if (plane == 0)
 		return height;
 
-	return height / drm_format_vert_chroma_subsampling(format);
+	return height / info->vsub;
 }
 EXPORT_SYMBOL(drm_format_plane_height);
-- 
Regards,

Laurent Pinchart

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

[PATCH v4 03/14] 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_fb_cma_helper.c |  23 ++++----
 drivers/gpu/drm/drm_framebuffer.c   | 102 +++++-------------------------------
 2 files changed, 25 insertions(+), 100 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 1fd6eac1400c..fac4f06f8485 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -176,20 +176,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 ERR_PTR(-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]);
@@ -200,7 +200,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) {
@@ -269,12 +269,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);
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 30dc01e6eb5d..ce8045228642 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -100,111 +100,33 @@ 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;
-	char *format_name;
-
-	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:
-		format_name = drm_get_format_name(r->pixel_format);
-		DRM_DEBUG_KMS("invalid pixel format %s\n", format_name);
-		kfree(format_name);
-		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) {
 		char *format_name = drm_get_format_name(r->pixel_format);
 		DRM_DEBUG_KMS("bad framebuffer format %s\n", format_name);
 		kfree(format_name);
-		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);
@@ -247,7 +169,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;
-- 
Regards,

Laurent Pinchart

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

[PATCH v4 04/14] drm: WARN when calling drm_format_info() for an unsupported format

[Laurent Pinchart]

The format helpers have historically treated unsupported formats as part
of the default case, returning values that are likely wrong. We can't
change this behaviour now without risking breaking drivers in difficult
to detect ways, but we can WARN on unsupported formats to catch faulty
callers.

The only exception is the framebuffer_check() function that calls
drm_format_info() to validate the format passed from userspace. This is
a valid use case that shouldn't generate a warning.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/drm_fourcc.c      | 32 ++++++++++++++++++++++++--------
 drivers/gpu/drm/drm_framebuffer.c |  2 +-
 include/drm/drm_fourcc.h          |  1 +
 3 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index bf91c5044d84..0355b7ede8e4 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -102,15 +102,11 @@ char *drm_get_format_name(uint32_t format)
 }
 EXPORT_SYMBOL(drm_get_format_name);
 
-/**
- * drm_format_info - query 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.
+/*
+ * Internal function to query information for a given format. See
+ * drm_format_info() for the public API.
  */
-const struct drm_format_info *drm_format_info(u32 format)
+const struct drm_format_info *__drm_format_info(u32 format)
 {
 	static const struct drm_format_info formats[] = {
 		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
@@ -184,6 +180,26 @@ const struct drm_format_info *drm_format_info(u32 format)
 
 	return NULL;
 }
+
+/**
+ * drm_format_info - query information for a given format
+ * @format: pixel format (DRM_FORMAT_*)
+ *
+ * The caller should only pass a supported pixel format to this function.
+ * Unsupported pixel formats will generate a warning in the kernel log.
+ *
+ * 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)
+{
+	const struct drm_format_info *info;
+
+	info = __drm_format_info(format);
+	WARN_ON(!info);
+	return info;
+}
 EXPORT_SYMBOL(drm_format_info);
 
 /**
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index ce8045228642..da29bdac2009 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -105,7 +105,7 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
 	const struct drm_format_info *info;
 	int i;
 
-	info = drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
+	info = __drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
 	if (!info) {
 		char *format_name = drm_get_format_name(r->pixel_format);
 		DRM_DEBUG_KMS("bad framebuffer format %s\n", format_name);
diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index 4e79d6159856..7ffb114d2468 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -43,6 +43,7 @@ struct drm_format_info {
 	u8 vsub;
 };
 
+const struct drm_format_info *__drm_format_info(u32 format);
 const struct drm_format_info *drm_format_info(u32 format);
 uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
 void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth, int *bpp);
-- 
Regards,

Laurent Pinchart

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

[PATCH v4 05/14] drm: sti: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

[Laurent Pinchart]

The driver needs the number of bytes per pixel, not the bpp and depth
info meant for fbdev compatibility. Use the right API.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Vincent Abriou <vincent.abriou@st.com>
---
 drivers/gpu/drm/sti/sti_gdp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Cc: Vincent Abriou <vincent.abriou@st.com>

diff --git a/drivers/gpu/drm/sti/sti_gdp.c b/drivers/gpu/drm/sti/sti_gdp.c
index b8d942ca45e8..3fc62c1ea9c2 100644
--- a/drivers/gpu/drm/sti/sti_gdp.c
+++ b/drivers/gpu/drm/sti/sti_gdp.c
@@ -719,7 +719,7 @@ static void sti_gdp_atomic_update(struct drm_plane *drm_plane,
 	u32 dma_updated_top;
 	u32 dma_updated_btm;
 	int format;
-	unsigned int depth, bpp;
+	unsigned int bpp;
 	u32 ydo, xdo, yds, xds;
 
 	if (!crtc || !fb)
@@ -758,9 +758,9 @@ static void sti_gdp_atomic_update(struct drm_plane *drm_plane,
 			 (unsigned long)cma_obj->paddr);
 
 	/* pixel memory location */
-	drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
+	bpp = drm_format_plane_cpp(fb->pixel_format, 0);
 	top_field->gam_gdp_pml = (u32)cma_obj->paddr + fb->offsets[0];
-	top_field->gam_gdp_pml += src_x * (bpp >> 3);
+	top_field->gam_gdp_pml += src_x * bpp;
 	top_field->gam_gdp_pml += src_y * fb->pitches[0];
 
 	/* output parameters (clamped / cropped) */
-- 
Regards,

Laurent Pinchart

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

[PATCH v4 06/14] drm: hdlcd: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

[Laurent Pinchart]

The driver needs the number of bytes per pixel, not the bpp and depth
info meant for fbdev compatibility. Use the right API.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 drivers/gpu/drm/arm/hdlcd_crtc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Cc: Liviu Dudau <liviu.dudau@arm.com>

diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 48019ae22ddb..bbaa55add2d2 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -223,14 +223,12 @@ static void hdlcd_plane_atomic_update(struct drm_plane *plane,
 {
 	struct hdlcd_drm_private *hdlcd;
 	struct drm_gem_cma_object *gem;
-	unsigned int depth, bpp;
 	u32 src_w, src_h, dest_w, dest_h;
 	dma_addr_t scanout_start;
 
 	if (!plane->state->fb)
 		return;
 
-	drm_fb_get_bpp_depth(plane->state->fb->pixel_format, &depth, &bpp);
 	src_w = plane->state->src_w >> 16;
 	src_h = plane->state->src_h >> 16;
 	dest_w = plane->state->crtc_w;
@@ -238,7 +236,8 @@ static void hdlcd_plane_atomic_update(struct drm_plane *plane,
 	gem = drm_fb_cma_get_gem_obj(plane->state->fb, 0);
 	scanout_start = gem->paddr + plane->state->fb->offsets[0] +
 		plane->state->crtc_y * plane->state->fb->pitches[0] +
-		plane->state->crtc_x * bpp / 8;
+		plane->state->crtc_x *
+		drm_format_plane_cpp(plane->state->fb->pixel_format, 0);
 
 	hdlcd = plane->dev->dev_private;
 	hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_LENGTH, plane->state->fb->pitches[0]);
-- 
Regards,

Laurent Pinchart

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

[PATCH v4 07/14] drm: tilcdc: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

[Laurent Pinchart]

The driver needs the number of bytes per pixel, not the bpp and depth
info meant for fbdev compatibility. Use the right API.

In the tilcdc_crtc_mode_set() function compute the hardware register
value directly from the pixel format instead of computing the number of
bits per pixels first.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
Changes since v3:

- Removed DRM_FORMAT_ARGB8888 support
- Fixed coding style
- Renamed min_pitch to pitch
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c  | 15 +++++----------
 drivers/gpu/drm/tilcdc/tilcdc_plane.c |  7 ++++---
 2 files changed, 9 insertions(+), 13 deletions(-)

Cc: Jyri Sarha <jsarha@ti.com>

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 25d6b220ee8a..a64718630cdb 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -67,15 +67,13 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb)
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
 	struct drm_device *dev = crtc->dev;
 	struct drm_gem_cma_object *gem;
-	unsigned int depth, bpp;
 	dma_addr_t start, end;
 
-	drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
 	gem = drm_fb_cma_get_gem_obj(fb, 0);
 
 	start = gem->paddr + fb->offsets[0] +
 		crtc->y * fb->pitches[0] +
-		crtc->x * bpp / 8;
+		crtc->x * drm_format_plane_cpp(fb->pixel_format, 0);
 
 	end = start + (crtc->mode.vdisplay * fb->pitches[0]);
 
@@ -404,16 +402,13 @@ static void tilcdc_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	if (info->tft_alt_mode)
 		reg |= LCDC_TFT_ALT_ENABLE;
 	if (priv->rev == 2) {
-		unsigned int depth, bpp;
-
-		drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
-		switch (bpp) {
-		case 16:
+		switch (fb->pixel_format) {
+		case DRM_FORMAT_RGB565:
 			break;
-		case 32:
+		case DRM_FORMAT_XRGB8888:
 			reg |= LCDC_V2_TFT_24BPP_UNPACK;
 			/* fallthrough */
-		case 24:
+		case DRM_FORMAT_RGB888:
 			reg |= LCDC_V2_TFT_24BPP_MODE;
 			break;
 		default:
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_plane.c b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
index 41911e3110e8..604074089112 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_plane.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
@@ -43,7 +43,7 @@ static int tilcdc_plane_atomic_check(struct drm_plane *plane,
 {
 	struct drm_crtc_state *crtc_state;
 	struct drm_plane_state *old_state = plane->state;
-	unsigned int depth, bpp;
+	unsigned int pitch;
 
 	if (!state->crtc)
 		return 0;
@@ -72,8 +72,9 @@ static int tilcdc_plane_atomic_check(struct drm_plane *plane,
 		return -EINVAL;
 	}
 
-	drm_fb_get_bpp_depth(state->fb->pixel_format, &depth, &bpp);
-	if (state->fb->pitches[0] != crtc_state->mode.hdisplay * bpp / 8) {
+	pitch = crtc_state->mode.hdisplay *
+		drm_format_plane_cpp(state->fb->pixel_format, 0);
+	if (state->fb->pitches[0] != pitch) {
 		dev_err(plane->dev->dev,
 			"Invalid pitch: fb and crtc widths must be the same");
 		return -EINVAL;
-- 
Regards,

Laurent Pinchart

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

[PATCH v4 08/14] drm: cirrus: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

[Laurent Pinchart]

The driver doesn't need the color depth, only the number of bits per
pixel. Use the right API.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/cirrus/cirrus_fbdev.c | 6 +++---
 drivers/gpu/drm/cirrus/cirrus_main.c  | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

Cc: Dave Airlie <airlied@redhat.com>

diff --git a/drivers/gpu/drm/cirrus/cirrus_fbdev.c b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
index daecf1ad76a4..3a6309d7d8e4 100644
--- a/drivers/gpu/drm/cirrus/cirrus_fbdev.c
+++ b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
@@ -138,12 +138,12 @@ static int cirrusfb_create_object(struct cirrus_fbdev *afbdev,
 {
 	struct drm_device *dev = afbdev->helper.dev;
 	struct cirrus_device *cdev = dev->dev_private;
-	u32 bpp, depth;
+	u32 bpp;
 	u32 size;
 	struct drm_gem_object *gobj;
-
 	int ret = 0;
-	drm_fb_get_bpp_depth(mode_cmd->pixel_format, &depth, &bpp);
+
+	bpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0) * 8;
 
 	if (!cirrus_check_framebuffer(cdev, mode_cmd->width, mode_cmd->height,
 				      bpp, mode_cmd->pitches[0]))
diff --git a/drivers/gpu/drm/cirrus/cirrus_main.c b/drivers/gpu/drm/cirrus/cirrus_main.c
index 76bcb43e7c06..2c3c0d4072ce 100644
--- a/drivers/gpu/drm/cirrus/cirrus_main.c
+++ b/drivers/gpu/drm/cirrus/cirrus_main.c
@@ -52,10 +52,10 @@ cirrus_user_framebuffer_create(struct drm_device *dev,
 	struct cirrus_device *cdev = dev->dev_private;
 	struct drm_gem_object *obj;
 	struct cirrus_framebuffer *cirrus_fb;
+	u32 bpp;
 	int ret;
-	u32 bpp, depth;
 
-	drm_fb_get_bpp_depth(mode_cmd->pixel_format, &depth, &bpp);
+	bpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0) * 8;
 
 	if (!cirrus_check_framebuffer(cdev, mode_cmd->width, mode_cmd->height,
 				      bpp, mode_cmd->pitches[0]))
-- 
Regards,

Laurent Pinchart

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

[PATCH v4 09/14] drm: gma500: Replace drm_fb_get_bpp_depth() with drm_format_info()

[Laurent Pinchart]

The driver uses drm_fb_get_bpp_depth() to check whether it can support
the format requested by userspace when creating a framebuffer. This
isn't the right API, as it doesn't differentiate between RGB formats
other than on a depth and bpp basis.

Fixing this requires non trivial changes to the drivers internals. As a
first step, replace usage of the drm_fb_get_bpp_depth() function with an
equivalent check based on drm_format_info(). This is part of a wider
effort to remove usage of the drm_fb_get_bpp_depth() function in
drivers.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/gma500/framebuffer.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>

diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
index 3a44e705db53..6cb92cc0bef8 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -236,22 +236,20 @@ static int psb_framebuffer_init(struct drm_device *dev,
 					const struct drm_mode_fb_cmd2 *mode_cmd,
 					struct gtt_range *gt)
 {
-	u32 bpp, depth;
+	const struct drm_format_info *info;
 	int ret;
 
-	drm_fb_get_bpp_depth(mode_cmd->pixel_format, &depth, &bpp);
+	/*
+	 * Reject unknown formats, YUV formats, and formats with more than
+	 * 4 bytes per pixel.
+	 */
+	info = drm_format_info(mode_cmd->pixel_format);
+	if (!info || !info->depth || info->cpp[0] > 4)
+		return -EINVAL;
 
 	if (mode_cmd->pitches[0] & 63)
 		return -EINVAL;
-	switch (bpp) {
-	case 8:
-	case 16:
-	case 24:
-	case 32:
-		break;
-	default:
-		return -EINVAL;
-	}
+
 	drm_helper_mode_fill_fb_struct(&fb->base, mode_cmd);
 	fb->gtt = gt;
 	ret = drm_framebuffer_init(dev, &fb->base, &psb_fb_funcs);
-- 
Regards,

Laurent Pinchart

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

[PATCH v4 10/14] drm: amdgpu: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

[Laurent Pinchart]

The driver needs the number of bytes per pixel, not the bpp and depth
info meant for fbdev compatibility. Use the right API.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v3:

- Renamed bpp to cpp
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c  | 14 +++++++-------
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c |  3 ++-
 2 files changed, 9 insertions(+), 8 deletions(-)

Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Michel Dänzer <michel@daenzer.net>

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
index bf033b58056c..0727946db189 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
@@ -62,12 +62,12 @@ static struct fb_ops amdgpufb_ops = {
 };
 
 
-int amdgpu_align_pitch(struct amdgpu_device *adev, int width, int bpp, bool tiled)
+int amdgpu_align_pitch(struct amdgpu_device *adev, int width, int cpp, bool tiled)
 {
 	int aligned = width;
 	int pitch_mask = 0;
 
-	switch (bpp / 8) {
+	switch (cpp) {
 	case 1:
 		pitch_mask = 255;
 		break;
@@ -82,7 +82,7 @@ int amdgpu_align_pitch(struct amdgpu_device *adev, int width, int bpp, bool tile
 
 	aligned += pitch_mask;
 	aligned &= ~pitch_mask;
-	return aligned;
+	return aligned * cpp;
 }
 
 static void amdgpufb_destroy_pinned_object(struct drm_gem_object *gobj)
@@ -111,13 +111,13 @@ static int amdgpufb_create_pinned_object(struct amdgpu_fbdev *rfbdev,
 	int ret;
 	int aligned_size, size;
 	int height = mode_cmd->height;
-	u32 bpp, depth;
+	u32 cpp;
 
-	drm_fb_get_bpp_depth(mode_cmd->pixel_format, &depth, &bpp);
+	cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
 
 	/* need to align pitch with crtc limits */
-	mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, bpp,
-						  fb_tiled) * ((bpp + 1) / 8);
+	mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, cpp,
+						  fb_tiled);
 
 	height = ALIGN(mode_cmd->height, 8);
 	size = mode_cmd->pitches[0] * height;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 88fbed2389c0..20a4e569b245 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -704,7 +704,8 @@ int amdgpu_mode_dumb_create(struct drm_file *file_priv,
 	uint32_t handle;
 	int r;
 
-	args->pitch = amdgpu_align_pitch(adev, args->width, args->bpp, 0) * ((args->bpp + 1) / 8);
+	args->pitch = amdgpu_align_pitch(adev, args->width,
+					 DIV_ROUND_UP(args->bpp, 8), 0);
 	args->size = (u64)args->pitch * args->height;
 	args->size = ALIGN(args->size, PAGE_SIZE);
 
-- 
Regards,

Laurent Pinchart

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

[PATCH v4 11/14] drm: radeon: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

[Laurent Pinchart]

The driver needs the number of bytes per pixel, not the bpp and depth
info meant for fbdev compatibility. Use the right API.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v3:

- Renamed bpp to cpp
---
 drivers/gpu/drm/radeon/radeon_fb.c  | 20 ++++++++++----------
 drivers/gpu/drm/radeon/radeon_gem.c |  3 ++-
 2 files changed, 12 insertions(+), 11 deletions(-)

Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Michel Dänzer <michel@daenzer.net>

diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c
index 568e036d547e..f4d6899ce3bb 100644
--- a/drivers/gpu/drm/radeon/radeon_fb.c
+++ b/drivers/gpu/drm/radeon/radeon_fb.c
@@ -61,13 +61,13 @@ static struct fb_ops radeonfb_ops = {
 };
 
 
-int radeon_align_pitch(struct radeon_device *rdev, int width, int bpp, bool tiled)
+int radeon_align_pitch(struct radeon_device *rdev, int width, int cpp, bool tiled)
 {
 	int aligned = width;
 	int align_large = (ASIC_IS_AVIVO(rdev)) || tiled;
 	int pitch_mask = 0;
 
-	switch (bpp / 8) {
+	switch (cpp) {
 	case 1:
 		pitch_mask = align_large ? 255 : 127;
 		break;
@@ -82,7 +82,7 @@ int radeon_align_pitch(struct radeon_device *rdev, int width, int bpp, bool tile
 
 	aligned += pitch_mask;
 	aligned &= ~pitch_mask;
-	return aligned;
+	return aligned * cpp;
 }
 
 static void radeonfb_destroy_pinned_object(struct drm_gem_object *gobj)
@@ -111,13 +111,13 @@ static int radeonfb_create_pinned_object(struct radeon_fbdev *rfbdev,
 	int ret;
 	int aligned_size, size;
 	int height = mode_cmd->height;
-	u32 bpp, depth;
+	u32 cpp;
 
-	drm_fb_get_bpp_depth(mode_cmd->pixel_format, &depth, &bpp);
+	cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
 
 	/* need to align pitch with crtc limits */
-	mode_cmd->pitches[0] = radeon_align_pitch(rdev, mode_cmd->width, bpp,
-						  fb_tiled) * ((bpp + 1) / 8);
+	mode_cmd->pitches[0] = radeon_align_pitch(rdev, mode_cmd->width, cpp,
+						  fb_tiled);
 
 	if (rdev->family >= CHIP_R600)
 		height = ALIGN(mode_cmd->height, 8);
@@ -137,11 +137,11 @@ static int radeonfb_create_pinned_object(struct radeon_fbdev *rfbdev,
 		tiling_flags = RADEON_TILING_MACRO;
 
 #ifdef __BIG_ENDIAN
-	switch (bpp) {
-	case 32:
+	switch (cpp) {
+	case 4:
 		tiling_flags |= RADEON_TILING_SWAP_32BIT;
 		break;
-	case 16:
+	case 2:
 		tiling_flags |= RADEON_TILING_SWAP_16BIT;
 	default:
 		break;
diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c
index deb9511725c9..0bcffd8a7bd3 100644
--- a/drivers/gpu/drm/radeon/radeon_gem.c
+++ b/drivers/gpu/drm/radeon/radeon_gem.c
@@ -745,7 +745,8 @@ int radeon_mode_dumb_create(struct drm_file *file_priv,
 	uint32_t handle;
 	int r;
 
-	args->pitch = radeon_align_pitch(rdev, args->width, args->bpp, 0) * ((args->bpp + 1) / 8);
+	args->pitch = radeon_align_pitch(rdev, args->width,
+					 DIV_ROUND_UP(args->bpp, 8), 0);
 	args->size = args->pitch * args->height;
 	args->size = ALIGN(args->size, PAGE_SIZE);
 
-- 
Regards,

Laurent Pinchart

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

[PATCH v4 12/14] drm: vmwgfx: Replace drm_fb_get_bpp_depth() with drm_format_info()

[Laurent Pinchart]

The driver is the last users of the drm_fb_get_bpp_depth() function. It
should ideally be converted to use struct drm_mode_fb_cmd2 instead of
the legacy struct drm_mode_fb_cmd internally, but that will require
broad changes across the code base. As a first step, replace
drm_fb_get_bpp_depth() with drm_format_info() in order to stop exporting
the function to drivers.

The new DRM_ERROR() message comes from the vmw_create_dmabuf_proxy(),
vmw_kms_new_framebuffer_surface() and vmw_kms_new_framebuffer_dmabuf()
functions that currently print an error if the pixel format is
unsupported.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Sinclair Yeh <syeh@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Cc: VMware Graphics <linux-graphics-maintainer@vmware.com>
Cc: Sinclair Yeh <syeh@vmware.com>
Cc: Thomas Hellstrom <thellstrom@vmware.com>

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index bf28ccc150df..c965514b82be 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -980,14 +980,22 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct drm_device *dev,
 	struct vmw_dma_buffer *bo = NULL;
 	struct ttm_base_object *user_obj;
 	struct drm_mode_fb_cmd mode_cmd;
+	const struct drm_format_info *info;
 	int ret;
 
+	info = drm_format_info(mode_cmd2->pixel_format);
+	if (!info || !info->depth) {
+		DRM_ERROR("Unsupported framebuffer format %s\n",
+			  drm_get_format_name(mode_cmd2->pixel_format));
+		return ERR_PTR(-EINVAL);
+	}
+
 	mode_cmd.width = mode_cmd2->width;
 	mode_cmd.height = mode_cmd2->height;
 	mode_cmd.pitch = mode_cmd2->pitches[0];
 	mode_cmd.handle = mode_cmd2->handles[0];
-	drm_fb_get_bpp_depth(mode_cmd2->pixel_format, &mode_cmd.depth,
-				    &mode_cmd.bpp);
+	mode_cmd.depth = info->depth;
+	mode_cmd.bpp = info->cpp[0] * 8;
 
 	/**
 	 * This code should be conditioned on Screen Objects not being used.
-- 
Regards,

Laurent Pinchart

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

[PATCH v4 13/14] drm/arm: mali-dp: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

[Laurent Pinchart]

The driver doesn't need the color depth, only the number of bits per
pixel. Use the right API.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/arm/malidp_hw.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index a6132f1d58c1..be815d0cc772 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -198,9 +198,6 @@ static void malidp500_modeset(struct malidp_hw_device *hwdev, struct videomode *
 
 static int malidp500_rotmem_required(struct malidp_hw_device *hwdev, u16 w, u16 h, u32 fmt)
 {
-	unsigned int depth;
-	int bpp;
-
 	/* RGB888 or BGR888 can't be rotated */
 	if ((fmt == DRM_FORMAT_RGB888) || (fmt == DRM_FORMAT_BGR888))
 		return -EINVAL;
@@ -210,9 +207,7 @@ static int malidp500_rotmem_required(struct malidp_hw_device *hwdev, u16 w, u16
 	 * worth of pixel data. Required size is then:
 	 *    size = rotated_width * (bpp / 8) * 8;
 	 */
-	drm_fb_get_bpp_depth(fmt, &depth, &bpp);
-
-	return w * bpp;
+	return w * drm_format_plane_cpp(fmt, 0) * 8;
 }
 
 static int malidp550_query_hw(struct malidp_hw_device *hwdev)
-- 
Regards,

Laurent Pinchart

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

[PATCH v4 14/14] drm: Don't export the drm_fb_get_bpp_depth() function

[Laurent Pinchart]

The function is only used by the drm_helper_mode_fill_fb_struct() core
function to fill the drm_framebuffer bpp and depth fields, used by
drivers that haven't been converted to use pixel formats directly yet.
It should not be used by new drivers, so inline it in its only caller.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/drm_fourcc.c         | 31 -------------------------------
 drivers/gpu/drm/drm_modeset_helper.c | 17 +++++++++++++++--
 include/drm/drm_fourcc.h             |  1 -
 3 files changed, 15 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 0355b7ede8e4..239d1aab8386 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -203,37 +203,6 @@ const struct drm_format_info *drm_format_info(u32 format)
 EXPORT_SYMBOL(drm_format_info);
 
 /**
- * drm_fb_get_bpp_depth - get the bpp/depth values for format
- * @format: pixel format (DRM_FORMAT_*)
- * @depth: storage for the depth value
- * @bpp: storage for the bpp value
- *
- * This only supports RGB formats here for compat with code that doesn't use
- * pixel formats directly yet.
- */
-void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
-			  int *bpp)
-{
-	const struct drm_format_info *info;
-
-	info = drm_format_info(format);
-	if (!info || !info->depth) {
-		char *format_name = drm_get_format_name(format);
-
-		DRM_DEBUG_KMS("unsupported pixel format %s\n", format_name);
-		kfree(format_name);
-
-		*depth = 0;
-		*bpp = 0;
-		return;
-	}
-
-	*depth = info->depth;
-	*bpp = info->cpp[0] << 3;
-}
-EXPORT_SYMBOL(drm_fb_get_bpp_depth);
-
-/**
  * drm_format_num_planes - get the number of planes for format
  * @format: pixel format (DRM_FORMAT_*)
  *
diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
index 1d45738f8f98..442325f10231 100644
--- a/drivers/gpu/drm/drm_modeset_helper.c
+++ b/drivers/gpu/drm/drm_modeset_helper.c
@@ -70,8 +70,23 @@ EXPORT_SYMBOL(drm_helper_move_panel_connectors_to_head);
 void drm_helper_mode_fill_fb_struct(struct drm_framebuffer *fb,
 				    const struct drm_mode_fb_cmd2 *mode_cmd)
 {
+	const struct drm_format_info *info;
 	int i;
 
+	info = drm_format_info(mode_cmd->pixel_format);
+	if (!info || !info->depth) {
+		char *format_name = drm_get_format_name(mode_cmd->pixel_format);
+
+		DRM_DEBUG_KMS("non-RGB pixel format %s\n", format_name);
+		kfree(format_name);
+
+		fb->depth = 0;
+		fb->bits_per_pixel = 0;
+	} else {
+		fb->depth = info->depth;
+		fb->bits_per_pixel = info->cpp[0] << 3;
+	}
+
 	fb->width = mode_cmd->width;
 	fb->height = mode_cmd->height;
 	for (i = 0; i < 4; i++) {
@@ -79,8 +94,6 @@ void drm_helper_mode_fill_fb_struct(struct drm_framebuffer *fb,
 		fb->offsets[i] = mode_cmd->offsets[i];
 		fb->modifier[i] = mode_cmd->modifier[i];
 	}
-	drm_fb_get_bpp_depth(mode_cmd->pixel_format, &fb->depth,
-				    &fb->bits_per_pixel);
 	fb->pixel_format = mode_cmd->pixel_format;
 	fb->flags = mode_cmd->flags;
 }
diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index 7ffb114d2468..1fbd04924b4d 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -46,7 +46,6 @@ struct drm_format_info {
 const struct drm_format_info *__drm_format_info(u32 format);
 const struct drm_format_info *drm_format_info(u32 format);
 uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
-void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth, int *bpp);
 int drm_format_num_planes(uint32_t format);
 int drm_format_plane_cpp(uint32_t format, int plane);
 int drm_format_horz_chroma_subsampling(uint32_t format);
-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v4 05/14] drm: sti: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

[Vincent ABRIOU]

Hi Laurent,

I plan to take this patch in my pull request that will be done next week.

BR
Vincent

On 09/08/2016 04:44 PM, Laurent Pinchart wrote:
> The driver needs the number of bytes per pixel, not the bpp and depth
> info meant for fbdev compatibility. Use the right API.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Acked-by: Vincent Abriou <vincent.abriou@st.com>
> ---
>  drivers/gpu/drm/sti/sti_gdp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> Cc: Vincent Abriou <vincent.abriou@st.com>
>
> diff --git a/drivers/gpu/drm/sti/sti_gdp.c b/drivers/gpu/drm/sti/sti_gdp.c
> index b8d942ca45e8..3fc62c1ea9c2 100644
> --- a/drivers/gpu/drm/sti/sti_gdp.c
> +++ b/drivers/gpu/drm/sti/sti_gdp.c
> @@ -719,7 +719,7 @@ static void sti_gdp_atomic_update(struct drm_plane *drm_plane,
>  	u32 dma_updated_top;
>  	u32 dma_updated_btm;
>  	int format;
> -	unsigned int depth, bpp;
> +	unsigned int bpp;
>  	u32 ydo, xdo, yds, xds;
>
>  	if (!crtc || !fb)
> @@ -758,9 +758,9 @@ static void sti_gdp_atomic_update(struct drm_plane *drm_plane,
>  			 (unsigned long)cma_obj->paddr);
>
>  	/* pixel memory location */
> -	drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
> +	bpp = drm_format_plane_cpp(fb->pixel_format, 0);
>  	top_field->gam_gdp_pml = (u32)cma_obj->paddr + fb->offsets[0];
> -	top_field->gam_gdp_pml += src_x * (bpp >> 3);
> +	top_field->gam_gdp_pml += src_x * bpp;
>  	top_field->gam_gdp_pml += src_y * fb->pitches[0];
>
>  	/* output parameters (clamped / cropped) */
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 01/14] drm: Centralize format information

[Tomi Valkeinen]

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

On 08/09/16 17:44, 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>
> ---
>  Documentation/gpu/drm-kms.rst |  3 ++
>  drivers/gpu/drm/drm_fourcc.c  | 84 +++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_fourcc.h      | 19 ++++++++++
>  3 files changed, 106 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index f9a991bb87d4..85c4c49f4436 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -63,6 +63,9 @@ Frame Buffer Functions Reference
>  DRM Format Handling
>  ===================
>  
> +.. kernel-doc:: include/drm/drm_fourcc.h
> +   :internal:
> +
>  .. kernel-doc:: drivers/gpu/drm/drm_fourcc.c
>     :export:
>  
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index 29c56b4331e0..6b91bd8a510d 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -103,6 +103,90 @@ char *drm_get_format_name(uint32_t format)
>  EXPORT_SYMBOL(drm_get_format_name);
>  
>  /**
> + * drm_format_info - query 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[] = {
> +		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGB332,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGR233,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XRGB4444,	.depth = 12, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XBGR4444,	.depth = 12, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBX4444,	.depth = 12, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRX4444,	.depth = 12, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ARGB4444,	.depth = 12, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ABGR4444,	.depth = 12, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBA4444,	.depth = 12, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRA4444,	.depth = 12, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },

Aren't these 16 bit deep modes? 4+4+4+4 = 16.

> +		{ .format = DRM_FORMAT_XRGB1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XBGR1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBX5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRX5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ARGB1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ABGR1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBA5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRA5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },

Aren't these 16 bit deep modes? 5+5+5+1 = 16.

> +		{ .format = DRM_FORMAT_RGB565,		.depth = 16, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGR565,		.depth = 16, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGB888,		.depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGR888,		.depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XRGB8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XBGR8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBX8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRX8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XRGB2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XBGR2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBX1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRX1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ARGB2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ABGR2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBA1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRA1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },

Aren't these 32 bit deep modes? 10+10+10+2 = 32.

> +		{ .format = DRM_FORMAT_ARGB8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ABGR8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBA8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRA8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YUV410,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 },
> +		{ .format = DRM_FORMAT_YVU410,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 },
> +		{ .format = DRM_FORMAT_YUV411,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YVU411,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YUV420,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 },
> +		{ .format = DRM_FORMAT_YVU420,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 },
> +		{ .format = DRM_FORMAT_YUV422,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YVU422,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YUV444,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YVU444,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_NV12,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 },
> +		{ .format = DRM_FORMAT_NV21,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 },
> +		{ .format = DRM_FORMAT_NV16,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_NV61,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_NV24,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_NV42,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YUYV,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YVYU,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_UYVY,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_VYUY,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_AYUV,		.depth = 0,  .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +	};
> +
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
> +		if (formats[i].format == format)
> +			return &formats[i];
> +	}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL(drm_format_info);
> +
> +/**
>   * 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_fourcc.h b/include/drm/drm_fourcc.h
> index 30c30fa87ee8..4e79d6159856 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -25,6 +25,25 @@
>  #include <linux/types.h>
>  #include <uapi/drm/drm_fourcc.h>
>  
> +/**
> + * 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)

Depth is zero for yuv formats. Maybe that should be made clear here.

 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 v4 02/14] drm: Implement the drm_format_*() helpers as drm_format_info() wrappers

[Tomi Valkeinen]

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

On 08/09/16 17:44, Laurent Pinchart wrote:
> 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_fourcc.c | 186 +++++++++----------------------------------
>  1 file changed, 37 insertions(+), 149 deletions(-)

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 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 v4 03/14] drm: Use drm_format_info() in DRM core code

[Tomi Valkeinen]

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

On 08/09/16 17:44, Laurent Pinchart wrote:
> 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_fb_cma_helper.c |  23 ++++----
>  drivers/gpu/drm/drm_framebuffer.c   | 102 +++++-------------------------------
>  2 files changed, 25 insertions(+), 100 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index 1fd6eac1400c..fac4f06f8485 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -176,20 +176,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 ERR_PTR(-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]);
> @@ -200,7 +200,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) {
> @@ -269,12 +269,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);

This change doesn't seem to improve the function. Afaics, only the num
planes is retrieved and used.

 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 v4 01/14] drm: Centralize format information

[Laurent Pinchart]

Hi Tomi,

Thank you for the review.

On Wednesday 14 Sep 2016 14:49:26 Tomi Valkeinen wrote:
> On 08/09/16 17:44, 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>
> > ---
> > 
> >  Documentation/gpu/drm-kms.rst |  3 ++
> >  drivers/gpu/drm/drm_fourcc.c  | 84 ++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_fourcc.h      | 19 ++++++++++
> >  3 files changed, 106 insertions(+)

[snip]

> > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > index 29c56b4331e0..6b91bd8a510d 100644
> > --- a/drivers/gpu/drm/drm_fourcc.c
> > +++ b/drivers/gpu/drm/drm_fourcc.c
> > @@ -103,6 +103,90 @@ char *drm_get_format_name(uint32_t format)
> >  EXPORT_SYMBOL(drm_get_format_name);
> >  
> >  /**
> > + * drm_format_info - query 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[] = {
> > +		{ .format = DRM_FORMAT_C8,		.depth = 8, 
> > .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_RGB332,		.depth = 8, 
> > .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_BGR233,		.depth = 8,
> > .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_XRGB4444,	.depth = 12,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_XBGR4444,	.depth = 12,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_RGBX4444,	.depth = 12,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_BGRX4444,	.depth = 12,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_ARGB4444,	.depth = 12,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_ABGR4444,	.depth = 12,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_RGBA4444,	.depth = 12,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_BGRA4444,	.depth = 12,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
>
> Aren't these 16 bit deep modes? 4+4+4+4 = 16.

No, the depth value is the number of colour bits, excluding the alpha bits. 
This is used to implement the fbdev compatibility code, as fbdev (unlike kms) 
makes use of that information.

The total number of bits per pixel is not stored in the table as it can be 
computed by cpp[0]*8 + (cpp[1]+cpp[2])*8/hsub/vsub. For RGB formats, which are 
the only formats supported by the existing drm_fb_get_bpp_depth() function, 
this simplifies to just cpp[0] * 8.

Same for the other formats below.

> > +		{ .format = DRM_FORMAT_XRGB1555,	.depth = 15,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 }, 
> > +		{ .format = DRM_FORMAT_XBGR1555,	.depth = 15,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_RGBX5551,	.depth = 15,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_BGRX5551,	.depth = 15,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_ARGB1555,	.depth = 15,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_ABGR1555,	.depth = 15,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_RGBA5551,	.depth = 15,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_BGRA5551,	.depth = 15,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
>
> Aren't these 16 bit deep modes? 5+5+5+1 = 16.
> 
> > +		{ .format = DRM_FORMAT_RGB565,		.depth = 16,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_BGR565,		.depth = 16,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_RGB888,		.depth = 24,
> > .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_BGR888,		.depth = 24,
> > .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_XRGB8888,	.depth = 24,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_XBGR8888,	.depth = 24,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_RGBX8888,	.depth = 24,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_BGRX8888,	.depth = 24,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_XRGB2101010,	.depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_XBGR2101010,	.depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_RGBX1010102,	.depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_BGRX1010102,	.depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_ARGB2101010,	.depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_ABGR2101010,	.depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_RGBA1010102,	.depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_BGRA1010102,	.depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
>
> Aren't these 32 bit deep modes? 10+10+10+2 = 32.
> 
> > +		{ .format = DRM_FORMAT_ARGB8888,	.depth = 32,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_ABGR8888,	.depth = 32,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_RGBA8888,	.depth = 32,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_BGRA8888,	.depth = 32,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_YUV410,		.depth = 0, 
> > .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 },
> > +		{ .format = DRM_FORMAT_YVU410,		.depth = 0, 
> > .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 },
> > +		{ .format = DRM_FORMAT_YUV411,		.depth = 0,
> > .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_YVU411,		.depth = 0,
> > .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_YUV420,		.depth = 0, 
> > .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 },
> > +		{ .format = DRM_FORMAT_YVU420,		.depth = 0,
> > .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 },
> > +		{ .format = DRM_FORMAT_YUV422,		.depth = 0, 
> > .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_YVU422,		.depth = 0,
> > .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_YUV444,		.depth = 0, 
> > .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_YVU444,		.depth = 0,
> > .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_NV12,		.depth = 0, 
> > .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 },
> > +		{ .format = DRM_FORMAT_NV21,		.depth = 0,
> > .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 },
> > +		{ .format = DRM_FORMAT_NV16,		.depth = 0, 
> > .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_NV61,		.depth = 0,
> > .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_NV24,		.depth = 0, 
> > .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_NV42,		.depth = 0,
> > .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_YUYV,		.depth = 0, 
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_YVYU,		.depth = 0,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_UYVY,		.depth = 0, 
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_VYUY,		.depth = 0,
> > .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> > +		{ .format = DRM_FORMAT_AYUV,		.depth = 0, 
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +	};
> > +
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
> > +		if (formats[i].format == format)
> > +			return &formats[i];
> > +	}
> > +
> > +	return NULL;
> > +}
> > +EXPORT_SYMBOL(drm_format_info);
> > +
> > +/**
> >   * 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_fourcc.h b/include/drm/drm_fourcc.h
> > index 30c30fa87ee8..4e79d6159856 100644
> > --- a/include/drm/drm_fourcc.h
> > +++ b/include/drm/drm_fourcc.h
> > @@ -25,6 +25,25 @@
> >  #include <linux/types.h>
> >  #include <uapi/drm/drm_fourcc.h>
> > 
> > +/**
> > + * 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)
> 
> Depth is zero for yuv formats. Maybe that should be made clear here.

How about

"color depth (number of bits per pixel excluding padding and alpha bits), 
valid for RGB formats only" ?

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v4 03/14] drm: Use drm_format_info() in DRM core code

[Laurent Pinchart]

Hi Tomi,

Thank you for the review.

On Wednesday 14 Sep 2016 16:23:09 Tomi Valkeinen wrote:
> On 08/09/16 17:44, Laurent Pinchart wrote:
> > 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_fb_cma_helper.c |  23 ++++----
> >  drivers/gpu/drm/drm_framebuffer.c   | 102 ++++---------------------------
> >  2 files changed, 25 insertions(+), 100 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > b/drivers/gpu/drm/drm_fb_cma_helper.c index 1fd6eac1400c..fac4f06f8485
> > 100644
> > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c

[snip]

> > @@ -269,12 +269,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);
> 
> This change doesn't seem to improve the function. Afaics, only the num
> planes is retrieved and used.

You're right. I was actually trying to remove usage of all the wrappers from 
the DRM core code. If that's not desired I can drop this hunk.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v4 01/14] drm: Centralize format information

[Tomi Valkeinen]

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

On 15/09/16 01:22, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the review.
> 
> On Wednesday 14 Sep 2016 14:49:26 Tomi Valkeinen wrote:
>> On 08/09/16 17:44, 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>
>>> ---
>>>
>>>  Documentation/gpu/drm-kms.rst |  3 ++
>>>  drivers/gpu/drm/drm_fourcc.c  | 84 ++++++++++++++++++++++++++++++++++++++
>>>  include/drm/drm_fourcc.h      | 19 ++++++++++
>>>  3 files changed, 106 insertions(+)
> 
> [snip]
> 
>>> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
>>> index 29c56b4331e0..6b91bd8a510d 100644
>>> --- a/drivers/gpu/drm/drm_fourcc.c
>>> +++ b/drivers/gpu/drm/drm_fourcc.c
>>> @@ -103,6 +103,90 @@ char *drm_get_format_name(uint32_t format)
>>>  EXPORT_SYMBOL(drm_get_format_name);
>>>  
>>>  /**
>>> + * drm_format_info - query 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[] = {
>>> +		{ .format = DRM_FORMAT_C8,		.depth = 8, 
>>> .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
>>> +		{ .format = DRM_FORMAT_RGB332,		.depth = 8, 
>>> .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
>>> +		{ .format = DRM_FORMAT_BGR233,		.depth = 8,
>>> .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
>>> +		{ .format = DRM_FORMAT_XRGB4444,	.depth = 12,
>>> .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
>>> +		{ .format = DRM_FORMAT_XBGR4444,	.depth = 12,
>>> .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
>>> +		{ .format = DRM_FORMAT_RGBX4444,	.depth = 12,
>>> .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
>>> +		{ .format = DRM_FORMAT_BGRX4444,	.depth = 12,
>>> .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
>>> +		{ .format = DRM_FORMAT_ARGB4444,	.depth = 12,
>>> .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
>>> +		{ .format = DRM_FORMAT_ABGR4444,	.depth = 12,
>>> .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
>>> +		{ .format = DRM_FORMAT_RGBA4444,	.depth = 12,
>>> .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
>>> +		{ .format = DRM_FORMAT_BGRA4444,	.depth = 12,
>>> .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
>>
>> Aren't these 16 bit deep modes? 4+4+4+4 = 16.
> 
> No, the depth value is the number of colour bits, excluding the alpha bits. 
> This is used to implement the fbdev compatibility code, as fbdev (unlike kms) 
> makes use of that information.
> 
> The total number of bits per pixel is not stored in the table as it can be 
> computed by cpp[0]*8 + (cpp[1]+cpp[2])*8/hsub/vsub. For RGB formats, which are 
> the only formats supported by the existing drm_fb_get_bpp_depth() function, 
> this simplifies to just cpp[0] * 8.

Ok. Then the ARGB8888 & co. formats have depth wrong. I presumed those
were right as they're the "normal" ones =).

I'm not sure if it's worth the hassle, but if the depth is only for
fbdev compat code, maybe a separate format->depth table in fbdev code
(for the formats fbdev supports) would make this cleaner and avoid any
future mistakes with new drm drivers.

>>> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
>>> index 30c30fa87ee8..4e79d6159856 100644
>>> --- a/include/drm/drm_fourcc.h
>>> +++ b/include/drm/drm_fourcc.h
>>> @@ -25,6 +25,25 @@
>>>  #include <linux/types.h>
>>>  #include <uapi/drm/drm_fourcc.h>
>>>
>>> +/**
>>> + * 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)
>>
>> Depth is zero for yuv formats. Maybe that should be made clear here.
> 
> How about
> 
> "color depth (number of bits per pixel excluding padding and alpha bits), 
> valid for RGB formats only" ?

Yes, that makes it clear.

 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 v4 04/14] drm: WARN when calling drm_format_info() for an unsupported format

[Tomi Valkeinen]

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

On 08/09/16 17:44, Laurent Pinchart wrote:
> The format helpers have historically treated unsupported formats as part
> of the default case, returning values that are likely wrong. We can't
> change this behaviour now without risking breaking drivers in difficult
> to detect ways, but we can WARN on unsupported formats to catch faulty
> callers.
> 
> The only exception is the framebuffer_check() function that calls
> drm_format_info() to validate the format passed from userspace. This is
> a valid use case that shouldn't generate a warning.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/drm_fourcc.c      | 32 ++++++++++++++++++++++++--------
>  drivers/gpu/drm/drm_framebuffer.c |  2 +-
>  include/drm/drm_fourcc.h          |  1 +
>  3 files changed, 26 insertions(+), 9 deletions(-)

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 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 v4 07/14] drm: tilcdc: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

[Tomi Valkeinen]

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

On 08/09/16 17:44, Laurent Pinchart wrote:
> The driver needs the number of bytes per pixel, not the bpp and depth
> info meant for fbdev compatibility. Use the right API.
> 
> In the tilcdc_crtc_mode_set() function compute the hardware register
> value directly from the pixel format instead of computing the number of
> bits per pixels first.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
> Changes since v3:
> 
> - Removed DRM_FORMAT_ARGB8888 support
> - Fixed coding style
> - Renamed min_pitch to pitch
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c  | 15 +++++----------
>  drivers/gpu/drm/tilcdc/tilcdc_plane.c |  7 ++++---
>  2 files changed, 9 insertions(+), 13 deletions(-)
> 
> Cc: Jyri Sarha <jsarha@ti.com>
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 25d6b220ee8a..a64718630cdb 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -67,15 +67,13 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb)
>  	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_gem_cma_object *gem;
> -	unsigned int depth, bpp;
>  	dma_addr_t start, end;
>  
> -	drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
>  	gem = drm_fb_cma_get_gem_obj(fb, 0);
>  
>  	start = gem->paddr + fb->offsets[0] +
>  		crtc->y * fb->pitches[0] +
> -		crtc->x * bpp / 8;
> +		crtc->x * drm_format_plane_cpp(fb->pixel_format, 0);
>  
>  	end = start + (crtc->mode.vdisplay * fb->pitches[0]);
>  
> @@ -404,16 +402,13 @@ static void tilcdc_crtc_mode_set_nofb(struct drm_crtc *crtc)
>  	if (info->tft_alt_mode)
>  		reg |= LCDC_TFT_ALT_ENABLE;
>  	if (priv->rev == 2) {
> -		unsigned int depth, bpp;
> -
> -		drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
> -		switch (bpp) {
> -		case 16:
> +		switch (fb->pixel_format) {
> +		case DRM_FORMAT_RGB565:
>  			break;
> -		case 32:
> +		case DRM_FORMAT_XRGB8888:
>  			reg |= LCDC_V2_TFT_24BPP_UNPACK;
>  			/* fallthrough */
> -		case 24:
> +		case DRM_FORMAT_RGB888:
>  			reg |= LCDC_V2_TFT_24BPP_MODE;
>  			break;

Jyri, with your latest tilcdc changes, we need also BGR565, XBGR8888 and
BGR888 here, don't we?

 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 v4 14/14] drm: Don't export the drm_fb_get_bpp_depth() function

[Tomi Valkeinen]

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

On 08/09/16 17:44, Laurent Pinchart wrote:
> The function is only used by the drm_helper_mode_fill_fb_struct() core
> function to fill the drm_framebuffer bpp and depth fields, used by
> drivers that haven't been converted to use pixel formats directly yet.
> It should not be used by new drivers, so inline it in its only caller.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/drm_fourcc.c         | 31 -------------------------------
>  drivers/gpu/drm/drm_modeset_helper.c | 17 +++++++++++++++--
>  include/drm/drm_fourcc.h             |  1 -
>  3 files changed, 15 insertions(+), 34 deletions(-)

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 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 v4 07/14] drm: tilcdc: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

[Jyri Sarha]

On 09/15/16 12:36, Tomi Valkeinen wrote:
> On 08/09/16 17:44, Laurent Pinchart wrote:
>> The driver needs the number of bytes per pixel, not the bpp and depth
>> info meant for fbdev compatibility. Use the right API.
>>
>> In the tilcdc_crtc_mode_set() function compute the hardware register
>> value directly from the pixel format instead of computing the number of
>> bits per pixels first.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>> Changes since v3:
>>
>> - Removed DRM_FORMAT_ARGB8888 support
>> - Fixed coding style
>> - Renamed min_pitch to pitch
>> ---
>>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c  | 15 +++++----------
>>  drivers/gpu/drm/tilcdc/tilcdc_plane.c |  7 ++++---
>>  2 files changed, 9 insertions(+), 13 deletions(-)
>>
>> Cc: Jyri Sarha <jsarha@ti.com>
>>
>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> index 25d6b220ee8a..a64718630cdb 100644
>> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> @@ -67,15 +67,13 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb)
>>  	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
>>  	struct drm_device *dev = crtc->dev;
>>  	struct drm_gem_cma_object *gem;
>> -	unsigned int depth, bpp;
>>  	dma_addr_t start, end;
>>  
>> -	drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
>>  	gem = drm_fb_cma_get_gem_obj(fb, 0);
>>  
>>  	start = gem->paddr + fb->offsets[0] +
>>  		crtc->y * fb->pitches[0] +
>> -		crtc->x * bpp / 8;
>> +		crtc->x * drm_format_plane_cpp(fb->pixel_format, 0);
>>  
>>  	end = start + (crtc->mode.vdisplay * fb->pitches[0]);
>>  
>> @@ -404,16 +402,13 @@ static void tilcdc_crtc_mode_set_nofb(struct drm_crtc *crtc)
>>  	if (info->tft_alt_mode)
>>  		reg |= LCDC_TFT_ALT_ENABLE;
>>  	if (priv->rev == 2) {
>> -		unsigned int depth, bpp;
>> -
>> -		drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
>> -		switch (bpp) {
>> -		case 16:
>> +		switch (fb->pixel_format) {
>> +		case DRM_FORMAT_RGB565:
>>  			break;
>> -		case 32:
>> +		case DRM_FORMAT_XRGB8888:
>>  			reg |= LCDC_V2_TFT_24BPP_UNPACK;
>>  			/* fallthrough */
>> -		case 24:
>> +		case DRM_FORMAT_RGB888:
>>  			reg |= LCDC_V2_TFT_24BPP_MODE;
>>  			break;
> 
> Jyri, with your latest tilcdc changes, we need also BGR565, XBGR8888 and
> BGR888 here, don't we?
> 

Yes we do.

BR,
Jyri

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

Re: [PATCH v4 01/14] drm: Centralize format information

[Eric Engestrom]

On Thu, Sep 15, 2016 at 09:22:54AM +0300, Tomi Valkeinen wrote:
> On 15/09/16 01:22, Laurent Pinchart wrote:
> > No, the depth value is the number of colour bits, excluding the alpha bits. 
> > This is used to implement the fbdev compatibility code, as fbdev (unlike kms) 
> > makes use of that information.
> > 
> > The total number of bits per pixel is not stored in the table as it can be 
> > computed by cpp[0]*8 + (cpp[1]+cpp[2])*8/hsub/vsub. For RGB formats, which are 
> > the only formats supported by the existing drm_fb_get_bpp_depth() function, 
> > this simplifies to just cpp[0] * 8.
> 
> Ok. Then the ARGB8888 & co. formats have depth wrong. I presumed those
> were right as they're the "normal" ones =).

Good catch, these should be 24 not 32.
I must admit I kinda skipped over that table the first time, and only
checked a few random values.
I just checked the whole table, and all the C and RGB formats are all
good (with the 4 /[ARGB]{4}8888/ formats set to .depth=24), and all the
YUV formats I know (~3/4) are good, but I don't know them all :)

> 
> I'm not sure if it's worth the hassle, but if the depth is only for
> fbdev compat code, maybe a separate format->depth table in fbdev code
> (for the formats fbdev supports) would make this cleaner and avoid any
> future mistakes with new drm drivers.

I agree actually, having it here will encourage anyone to use it. If you
don't want to split it out, at least add a comment along the lines of
your reply:

> > This is used to implement the fbdev compatibility code, as fbdev (unlike kms)
> > makes use of that information.

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

[PATCH v4.1 07/14] drm: tilcdc: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

[Laurent Pinchart]

The driver needs the number of bytes per pixel, not the bpp and depth
info meant for fbdev compatibility. Use the right API.

In the tilcdc_crtc_mode_set() function compute the hardware register
value directly from the pixel format instead of computing the number of
bits per pixels first.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
Changes since v4:

- Added support for DRM_FORMAT_BGR565, DRM_FORMAT_XBGR8888 and
  DRM_FORMAT_BGR888

Changes since v3:

- Removed DRM_FORMAT_ARGB8888 support
- Fixed coding style
- Renamed min_pitch to pitch

Cc: Jyri Sarha <jsarha@ti.com>

---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c  | 18 ++++++++----------
 drivers/gpu/drm/tilcdc/tilcdc_plane.c |  7 ++++---
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 208768922030..0800545b5c5a 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -67,16 +67,14 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb)
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
 	struct drm_device *dev = crtc->dev;
 	struct drm_gem_cma_object *gem;
-	unsigned int depth, bpp;
 	dma_addr_t start, end;
 	u64 dma_base_and_ceiling;
 
-	drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
 	gem = drm_fb_cma_get_gem_obj(fb, 0);
 
 	start = gem->paddr + fb->offsets[0] +
 		crtc->y * fb->pitches[0] +
-		crtc->x * bpp / 8;
+		crtc->x * drm_format_plane_cpp(fb->pixel_format, 0);
 
 	end = start + (crtc->mode.vdisplay * fb->pitches[0]);
 
@@ -412,16 +410,16 @@ static void tilcdc_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	if (info->tft_alt_mode)
 		reg |= LCDC_TFT_ALT_ENABLE;
 	if (priv->rev == 2) {
-		unsigned int depth, bpp;
-
-		drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
-		switch (bpp) {
-		case 16:
+		switch (fb->pixel_format) {
+		case DRM_FORMAT_BGR565:
+		case DRM_FORMAT_RGB565:
 			break;
-		case 32:
+		case DRM_FORMAT_XBGR8888:
+		case DRM_FORMAT_XRGB8888:
 			reg |= LCDC_V2_TFT_24BPP_UNPACK;
 			/* fallthrough */
-		case 24:
+		case DRM_FORMAT_BGR888:
+		case DRM_FORMAT_RGB888:
 			reg |= LCDC_V2_TFT_24BPP_MODE;
 			break;
 		default:
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_plane.c b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
index 74c65fa859b2..8a6a50d74aff 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_plane.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
@@ -39,7 +39,7 @@ static int tilcdc_plane_atomic_check(struct drm_plane *plane,
 {
 	struct drm_crtc_state *crtc_state;
 	struct drm_plane_state *old_state = plane->state;
-	unsigned int depth, bpp;
+	unsigned int pitch;
 
 	if (!state->crtc)
 		return 0;
@@ -68,8 +68,9 @@ static int tilcdc_plane_atomic_check(struct drm_plane *plane,
 		return -EINVAL;
 	}
 
-	drm_fb_get_bpp_depth(state->fb->pixel_format, &depth, &bpp);
-	if (state->fb->pitches[0] != crtc_state->mode.hdisplay * bpp / 8) {
+	pitch = crtc_state->mode.hdisplay *
+		drm_format_plane_cpp(state->fb->pixel_format, 0);
+	if (state->fb->pitches[0] != pitch) {
 		dev_err(plane->dev->dev,
 			"Invalid pitch: fb and crtc widths must be the same");
 		return -EINVAL;
-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v4 01/14] drm: Centralize format information

[Laurent Pinchart]

Hello,

On Thursday 15 Sep 2016 17:12:12 Eric Engestrom wrote:
> On Thu, Sep 15, 2016 at 09:22:54AM +0300, Tomi Valkeinen wrote:
> > On 15/09/16 01:22, Laurent Pinchart wrote:
> >> No, the depth value is the number of colour bits, excluding the alpha
> >> bits. This is used to implement the fbdev compatibility code, as fbdev
> >> (unlike kms) makes use of that information.
> >> 
> >> The total number of bits per pixel is not stored in the table as it can
> >> be computed by cpp[0]*8 + (cpp[1]+cpp[2])*8/hsub/vsub. For RGB formats,
> >> which are the only formats supported by the existing
> >> drm_fb_get_bpp_depth() function, this simplifies to just cpp[0] * 8.
> > 
> > Ok. Then the ARGB8888 & co. formats have depth wrong. I presumed those
> > were right as they're the "normal" ones =).
> 
> Good catch, these should be 24 not 32.
> I must admit I kinda skipped over that table the first time, and only
> checked a few random values.
> I just checked the whole table, and all the C and RGB formats are all
> good (with the 4 /[ARGB]{4}8888/ formats set to .depth=24), and all the
> YUV formats I know (~3/4) are good, but I don't know them all :)

I've checked the existing code that this patch series is replacing, and the 
[ARGB]{4}8888 formats are currently reported as having a depth of 32. I'm not 
sure why that's the case, but I'd rather not touch it in this patch. If this 
is a bug we should fix it in a separate patch.

> > I'm not sure if it's worth the hassle, but if the depth is only for
> > fbdev compat code, maybe a separate format->depth table in fbdev code
> > (for the formats fbdev supports) would make this cleaner and avoid any
> > future mistakes with new drm drivers.
> 
> I agree actually, having it here will encourage anyone to use it. If you
> don't want to split it out, at least add a comment along the lines of
> your reply:
>
> >> This is used to implement the fbdev compatibility code, as fbdev (unlike
> >> kms) makes use of that information.

I've double-checked the existing usage of the depth value, and it turns out 
that quite a few drivers still use it for various purpose, through struct 
drm_framebuffer.depth. I thus wonder whether splitting the depth value from 
the format information table would really help, as drivers would have a way to 
access it anyway.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v4 01/14] drm: Centralize format information

[Tomi Valkeinen]

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

On 16/09/16 02:30, Laurent Pinchart wrote:

> I've checked the existing code that this patch series is replacing, and the 
> [ARGB]{4}8888 formats are currently reported as having a depth of 32. I'm not 
> sure why that's the case, but I'd rather not touch it in this patch. If this 
> is a bug we should fix it in a separate patch.

I agree, I don't want this series to be held up. But this depth is
clearly broken, in some way or another. Even more reason to move it to
fb code =).

>>> I'm not sure if it's worth the hassle, but if the depth is only for
>>> fbdev compat code, maybe a separate format->depth table in fbdev code
>>> (for the formats fbdev supports) would make this cleaner and avoid any
>>> future mistakes with new drm drivers.
>>
>> I agree actually, having it here will encourage anyone to use it. If you
>> don't want to split it out, at least add a comment along the lines of
>> your reply:
>>
>>>> This is used to implement the fbdev compatibility code, as fbdev (unlike
>>>> kms) makes use of that information.
> 
> I've double-checked the existing usage of the depth value, and it turns out 
> that quite a few drivers still use it for various purpose, through struct 
> drm_framebuffer.depth. I thus wonder whether splitting the depth value from 
> the format information table would really help, as drivers would have a way to 
> access it anyway.

Ok.

Btw, are you sure alpha is not counted into depth? With a quick glance,
also drm_mode_legacy_fb_format() seems to expect depth to include alpha.

I'm just thinking here that if "depth" is not clearly defined anywhere,
and the most common formats, 24bit RGB formats, are defined with depth
including alpha, well, maybe then that's how depth should be defined.

Then again, I had a quick glance at the fbdev code, and
fb_get_color_depth() suggests that alpha is not counted in...

 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 v4 01/14] drm: Centralize format information

[Laurent Pinchart]

Hi Tomi,

On Friday 16 Sep 2016 12:44:31 Tomi Valkeinen wrote:
> On 16/09/16 02:30, Laurent Pinchart wrote:
> > I've checked the existing code that this patch series is replacing, and
> > the [ARGB]{4}8888 formats are currently reported as having a depth of 32.
> > I'm not sure why that's the case, but I'd rather not touch it in this
> > patch. If this is a bug we should fix it in a separate patch.
> 
> I agree, I don't want this series to be held up. But this depth is
> clearly broken, in some way or another. Even more reason to move it to
> fb code =).

And to then drop fbdev ;-)

> >>> I'm not sure if it's worth the hassle, but if the depth is only for
> >>> fbdev compat code, maybe a separate format->depth table in fbdev code
> >>> (for the formats fbdev supports) would make this cleaner and avoid any
> >>> future mistakes with new drm drivers.
> >> 
> >> I agree actually, having it here will encourage anyone to use it. If you
> >> don't want to split it out, at least add a comment along the lines of
> >> your reply:
> >> 
> >>>> This is used to implement the fbdev compatibility code, as fbdev
> >>>> (unlike kms) makes use of that information.
> > 
> > I've double-checked the existing usage of the depth value, and it turns
> > out that quite a few drivers still use it for various purpose, through
> > struct drm_framebuffer.depth. I thus wonder whether splitting the depth
> > value from the format information table would really help, as drivers
> > would have a way to access it anyway.
> 
> Ok.
> 
> Btw, are you sure alpha is not counted into depth? With a quick glance,
> also drm_mode_legacy_fb_format() seems to expect depth to include alpha.
> 
> I'm just thinking here that if "depth" is not clearly defined anywhere,
> and the most common formats, 24bit RGB formats, are defined with depth
> including alpha, well, maybe then that's how depth should be defined.
> 
> Then again, I had a quick glance at the fbdev code, and
> fb_get_color_depth() suggests that alpha is not counted in...

I think depth is just ill-defined and shouldn't be used in drivers anymore, at 
least certainly not by new code.

-- 
Regards,

Laurent Pinchart

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

[PATCH v4.1 01/14] 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>
---
Changes since v4:

- Fixed depth value of DRM_FORMAT_[AXRGB]{4}4444 formats to match
  current code
- Documented the depth field as legacy
---
 Documentation/gpu/drm-kms.rst |  3 ++
 drivers/gpu/drm/drm_fourcc.c  | 84 +++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_fourcc.h      | 21 +++++++++++
 3 files changed, 108 insertions(+)

diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index f9a991bb87d4..85c4c49f4436 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -63,6 +63,9 @@ Frame Buffer Functions Reference
 DRM Format Handling
 ===================
 
+.. kernel-doc:: include/drm/drm_fourcc.h
+   :internal:
+
 .. kernel-doc:: drivers/gpu/drm/drm_fourcc.c
    :export:
 
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 29c56b4331e0..39f09c564111 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -103,6 +103,90 @@ char *drm_get_format_name(uint32_t format)
 EXPORT_SYMBOL(drm_get_format_name);
 
 /**
+ * drm_format_info - query 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[] = {
+		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGB332,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGR233,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_XRGB4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_XBGR4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGBX4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGRX4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_ARGB4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_ABGR4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGBA4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGRA4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_XRGB1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_XBGR1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGBX5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGRX5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_ARGB1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_ABGR1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGBA5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGRA5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGB565,		.depth = 16, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGR565,		.depth = 16, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGB888,		.depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGR888,		.depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_XRGB8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_XBGR8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGBX8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGRX8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_XRGB2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_XBGR2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGBX1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGRX1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_ARGB2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_ABGR2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGBA1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGRA1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_ARGB8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_ABGR8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_RGBA8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_BGRA8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_YUV410,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 },
+		{ .format = DRM_FORMAT_YVU410,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 },
+		{ .format = DRM_FORMAT_YUV411,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 1 },
+		{ .format = DRM_FORMAT_YVU411,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 1 },
+		{ .format = DRM_FORMAT_YUV420,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 },
+		{ .format = DRM_FORMAT_YVU420,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 },
+		{ .format = DRM_FORMAT_YUV422,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1 },
+		{ .format = DRM_FORMAT_YVU422,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1 },
+		{ .format = DRM_FORMAT_YUV444,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_YVU444,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_NV12,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 },
+		{ .format = DRM_FORMAT_NV21,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 },
+		{ .format = DRM_FORMAT_NV16,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 },
+		{ .format = DRM_FORMAT_NV61,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 },
+		{ .format = DRM_FORMAT_NV24,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_NV42,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_YUYV,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
+		{ .format = DRM_FORMAT_YVYU,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
+		{ .format = DRM_FORMAT_UYVY,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
+		{ .format = DRM_FORMAT_VYUY,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
+		{ .format = DRM_FORMAT_AYUV,		.depth = 0,  .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
+	};
+
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
+		if (formats[i].format == format)
+			return &formats[i];
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(drm_format_info);
+
+/**
  * 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_fourcc.h b/include/drm/drm_fourcc.h
index 30c30fa87ee8..135fef050ee6 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -25,6 +25,27 @@
 #include <linux/types.h>
 #include <uapi/drm/drm_fourcc.h>
 
+/**
+ * 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),
+ *	valid for a subset of RGB formats only. This is a legacy field, do not
+ *	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)
+ * @hsub: Horizontal chroma subsampling factor
+ * @vsub: Vertical chroma subsampling factor
+ */
+struct drm_format_info {
+	u32 format;
+	u8 depth;
+	u8 num_planes;
+	u8 cpp[3];
+	u8 hsub;
+	u8 vsub;
+};
+
+const struct drm_format_info *drm_format_info(u32 format);
 uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
 void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth, int *bpp);
 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

Re: [PATCH v4.1 01/14] drm: Centralize format information

[Daniel Vetter]

On Sun, Sep 18, 2016 at 01:17:27PM +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>
> ---
> Changes since v4:
> 
> - Fixed depth value of DRM_FORMAT_[AXRGB]{4}4444 formats to match
>   current code
> - Documented the depth field as legacy
> ---
>  Documentation/gpu/drm-kms.rst |  3 ++
>  drivers/gpu/drm/drm_fourcc.c  | 84 +++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_fourcc.h      | 21 +++++++++++
>  3 files changed, 108 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index f9a991bb87d4..85c4c49f4436 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -63,6 +63,9 @@ Frame Buffer Functions Reference
>  DRM Format Handling
>  ===================
>  
> +.. kernel-doc:: include/drm/drm_fourcc.h
> +   :internal:
> +
>  .. kernel-doc:: drivers/gpu/drm/drm_fourcc.c
>     :export:
>  
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index 29c56b4331e0..39f09c564111 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -103,6 +103,90 @@ char *drm_get_format_name(uint32_t format)
>  EXPORT_SYMBOL(drm_get_format_name);
>  
>  /**
> + * drm_format_info - query 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[] = {
> +		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGB332,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGR233,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XRGB4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XBGR4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBX4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRX4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ARGB4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ABGR4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBA4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRA4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XRGB1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XBGR1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBX5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRX5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ARGB1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ABGR1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBA5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRA5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGB565,		.depth = 16, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGR565,		.depth = 16, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGB888,		.depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGR888,		.depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XRGB8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XBGR8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBX8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRX8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XRGB2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XBGR2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBX1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRX1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ARGB2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ABGR2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBA1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRA1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ARGB8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ABGR8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBA8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRA8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YUV410,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 },
> +		{ .format = DRM_FORMAT_YVU410,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 },
> +		{ .format = DRM_FORMAT_YUV411,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YVU411,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YUV420,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 },
> +		{ .format = DRM_FORMAT_YVU420,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 },
> +		{ .format = DRM_FORMAT_YUV422,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YVU422,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YUV444,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YVU444,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_NV12,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 },
> +		{ .format = DRM_FORMAT_NV21,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 },
> +		{ .format = DRM_FORMAT_NV16,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_NV61,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_NV24,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_NV42,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YUYV,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YVYU,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_UYVY,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_VYUY,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_AYUV,		.depth = 0,  .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +	};
> +
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
> +		if (formats[i].format == format)
> +			return &formats[i];
> +	}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL(drm_format_info);
> +
> +/**
>   * 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_fourcc.h b/include/drm/drm_fourcc.h
> index 30c30fa87ee8..135fef050ee6 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -25,6 +25,27 @@
>  #include <linux/types.h>
>  #include <uapi/drm/drm_fourcc.h>
>  
> +/**
> + * 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),
> + *	valid for a subset of RGB formats only. This is a legacy field, do not
> + *	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)
> + * @hsub: Horizontal chroma subsampling factor
> + * @vsub: Vertical chroma subsampling factor
> + */

Atm we don't include the drm_fourcc.h header in
Documentation/gpu/drm-kms.rst, which means we're missing this new
kerneldoc. With that fixed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +struct drm_format_info {
> +	u32 format;
> +	u8 depth;
> +	u8 num_planes;
> +	u8 cpp[3];
> +	u8 hsub;
> +	u8 vsub;
> +};
> +
> +const struct drm_format_info *drm_format_info(u32 format);
>  uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
>  void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth, int *bpp);
>  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

-- 
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 v4 03/14] drm: Use drm_format_info() in DRM core code

[Daniel Vetter]

On Thu, Sep 15, 2016 at 01:31:23AM +0300, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the review.
> 
> On Wednesday 14 Sep 2016 16:23:09 Tomi Valkeinen wrote:
> > On 08/09/16 17:44, Laurent Pinchart wrote:
> > > 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_fb_cma_helper.c |  23 ++++----
> > >  drivers/gpu/drm/drm_framebuffer.c   | 102 ++++---------------------------
> > >  2 files changed, 25 insertions(+), 100 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > b/drivers/gpu/drm/drm_fb_cma_helper.c index 1fd6eac1400c..fac4f06f8485
> > > 100644
> > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> 
> [snip]
> 
> > > @@ -269,12 +269,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);
> > 
> > This change doesn't seem to improve the function. Afaics, only the num
> > planes is retrieved and used.
> 
> You're right. I was actually trying to remove usage of all the wrappers from 
> the DRM core code. If that's not desired I can drop this hunk.

No preference from my side, either way:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
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 v4 12/14] drm: vmwgfx: Replace drm_fb_get_bpp_depth() with drm_format_info()

[Daniel Vetter]

On Thu, Sep 08, 2016 at 05:44:26PM +0300, Laurent Pinchart wrote:
> The driver is the last users of the drm_fb_get_bpp_depth() function. It
> should ideally be converted to use struct drm_mode_fb_cmd2 instead of
> the legacy struct drm_mode_fb_cmd internally, but that will require
> broad changes across the code base. As a first step, replace
> drm_fb_get_bpp_depth() with drm_format_info() in order to stop exporting
> the function to drivers.
> 
> The new DRM_ERROR() message comes from the vmw_create_dmabuf_proxy(),
> vmw_kms_new_framebuffer_surface() and vmw_kms_new_framebuffer_dmabuf()
> functions that currently print an error if the pixel format is
> unsupported.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Sinclair Yeh <syeh@vmware.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> Cc: VMware Graphics <linux-graphics-maintainer@vmware.com>
> Cc: Sinclair Yeh <syeh@vmware.com>
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index bf28ccc150df..c965514b82be 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -980,14 +980,22 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct drm_device *dev,
>  	struct vmw_dma_buffer *bo = NULL;
>  	struct ttm_base_object *user_obj;
>  	struct drm_mode_fb_cmd mode_cmd;
> +	const struct drm_format_info *info;
>  	int ret;
>  
> +	info = drm_format_info(mode_cmd2->pixel_format);
> +	if (!info || !info->depth) {
> +		DRM_ERROR("Unsupported framebuffer format %s\n",
> +			  drm_get_format_name(mode_cmd2->pixel_format));
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>  	mode_cmd.width = mode_cmd2->width;
>  	mode_cmd.height = mode_cmd2->height;
>  	mode_cmd.pitch = mode_cmd2->pitches[0];
>  	mode_cmd.handle = mode_cmd2->handles[0];
> -	drm_fb_get_bpp_depth(mode_cmd2->pixel_format, &mode_cmd.depth,
> -				    &mode_cmd.bpp);
> +	mode_cmd.depth = info->depth;
> +	mode_cmd.bpp = info->cpp[0] * 8;

I think this should use drm_helper_mode_fill_fb_struct instead.
-Daniel

>  
>  	/**
>  	 * This code should be conditioned on Screen Objects not being used.
> -- 
> 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 v4 02/14] drm: Implement the drm_format_*() helpers as drm_format_info() wrappers

[Daniel Vetter]

On Thu, Sep 08, 2016 at 05:44:16PM +0300, Laurent Pinchart wrote:
> 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_fourcc.c | 186 +++++++++----------------------------------
>  1 file changed, 37 insertions(+), 149 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index 6b91bd8a510d..bf91c5044d84 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -198,69 +198,22 @@ EXPORT_SYMBOL(drm_format_info);
>  void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
>  			  int *bpp)
>  {
> -	char *format_name;
> -
> -	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:
> -		format_name = drm_get_format_name(format);
> +	const struct drm_format_info *info;
> +
> +	info = drm_format_info(format);
> +	if (!info || !info->depth) {
> +		char *format_name = drm_get_format_name(format);
> +
>  		DRM_DEBUG_KMS("unsupported pixel format %s\n", format_name);
>  		kfree(format_name);
> +
>  		*depth = 0;
>  		*bpp = 0;
> -		break;
> +		return;
>  	}
> +
> +	*depth = info->depth;
> +	*bpp = info->cpp[0] << 3;

Bikeshed: This is a funny way to write * 8 ... Would be nice to fix imo.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
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 v4 09/14] drm: gma500: Replace drm_fb_get_bpp_depth() with drm_format_info()

[Daniel Vetter]

On Thu, Sep 08, 2016 at 05:44:23PM +0300, Laurent Pinchart wrote:
> The driver uses drm_fb_get_bpp_depth() to check whether it can support
> the format requested by userspace when creating a framebuffer. This
> isn't the right API, as it doesn't differentiate between RGB formats
> other than on a depth and bpp basis.
> 
> Fixing this requires non trivial changes to the drivers internals. As a
> first step, replace usage of the drm_fb_get_bpp_depth() function with an
> equivalent check based on drm_format_info(). This is part of a wider
> effort to remove usage of the drm_fb_get_bpp_depth() function in
> drivers.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

The real problem is that legady set_config doesn't check the plane format
lists, because that's a more recent invention ...

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/gma500/framebuffer.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
> 
> diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
> index 3a44e705db53..6cb92cc0bef8 100644
> --- a/drivers/gpu/drm/gma500/framebuffer.c
> +++ b/drivers/gpu/drm/gma500/framebuffer.c
> @@ -236,22 +236,20 @@ static int psb_framebuffer_init(struct drm_device *dev,
>  					const struct drm_mode_fb_cmd2 *mode_cmd,
>  					struct gtt_range *gt)
>  {
> -	u32 bpp, depth;
> +	const struct drm_format_info *info;
>  	int ret;
>  
> -	drm_fb_get_bpp_depth(mode_cmd->pixel_format, &depth, &bpp);
> +	/*
> +	 * Reject unknown formats, YUV formats, and formats with more than
> +	 * 4 bytes per pixel.
> +	 */
> +	info = drm_format_info(mode_cmd->pixel_format);
> +	if (!info || !info->depth || info->cpp[0] > 4)
> +		return -EINVAL;
>  
>  	if (mode_cmd->pitches[0] & 63)
>  		return -EINVAL;
> -	switch (bpp) {
> -	case 8:
> -	case 16:
> -	case 24:
> -	case 32:
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> +
>  	drm_helper_mode_fill_fb_struct(&fb->base, mode_cmd);
>  	fb->gtt = gt;
>  	ret = drm_framebuffer_init(dev, &fb->base, &psb_fb_funcs);
> -- 
> 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 v4 08/14] drm: cirrus: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

[Daniel Vetter]

On Thu, Sep 08, 2016 at 05:44:22PM +0300, Laurent Pinchart wrote:
> The driver doesn't need the color depth, only the number of bits per
> pixel. Use the right API.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/cirrus/cirrus_fbdev.c | 6 +++---
>  drivers/gpu/drm/cirrus/cirrus_main.c  | 4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> Cc: Dave Airlie <airlied@redhat.com>
> 
> diff --git a/drivers/gpu/drm/cirrus/cirrus_fbdev.c b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
> index daecf1ad76a4..3a6309d7d8e4 100644
> --- a/drivers/gpu/drm/cirrus/cirrus_fbdev.c
> +++ b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
> @@ -138,12 +138,12 @@ static int cirrusfb_create_object(struct cirrus_fbdev *afbdev,
>  {
>  	struct drm_device *dev = afbdev->helper.dev;
>  	struct cirrus_device *cdev = dev->dev_private;
> -	u32 bpp, depth;
> +	u32 bpp;
>  	u32 size;
>  	struct drm_gem_object *gobj;
> -
>  	int ret = 0;
> -	drm_fb_get_bpp_depth(mode_cmd->pixel_format, &depth, &bpp);
> +
> +	bpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0) * 8;
>  
>  	if (!cirrus_check_framebuffer(cdev, mode_cmd->width, mode_cmd->height,
>  				      bpp, mode_cmd->pitches[0]))
> diff --git a/drivers/gpu/drm/cirrus/cirrus_main.c b/drivers/gpu/drm/cirrus/cirrus_main.c
> index 76bcb43e7c06..2c3c0d4072ce 100644
> --- a/drivers/gpu/drm/cirrus/cirrus_main.c
> +++ b/drivers/gpu/drm/cirrus/cirrus_main.c
> @@ -52,10 +52,10 @@ cirrus_user_framebuffer_create(struct drm_device *dev,
>  	struct cirrus_device *cdev = dev->dev_private;
>  	struct drm_gem_object *obj;
>  	struct cirrus_framebuffer *cirrus_fb;
> +	u32 bpp;
>  	int ret;
> -	u32 bpp, depth;
>  
> -	drm_fb_get_bpp_depth(mode_cmd->pixel_format, &depth, &bpp);
> +	bpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0) * 8;
>  
>  	if (!cirrus_check_framebuffer(cdev, mode_cmd->width, mode_cmd->height,
>  				      bpp, mode_cmd->pitches[0]))
> -- 
> 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 v4 10/14] drm: amdgpu: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

[Daniel Vetter]

On Thu, Sep 08, 2016 at 05:44:24PM +0300, Laurent Pinchart wrote:
> The driver needs the number of bytes per pixel, not the bpp and depth
> info meant for fbdev compatibility. Use the right API.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v3:
> 
> - Renamed bpp to cpp
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c  | 14 +++++++-------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c |  3 ++-
>  2 files changed, 9 insertions(+), 8 deletions(-)
> 
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Michel Dänzer <michel@daenzer.net>
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> index bf033b58056c..0727946db189 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> @@ -62,12 +62,12 @@ static struct fb_ops amdgpufb_ops = {
>  };
>  
>  
> -int amdgpu_align_pitch(struct amdgpu_device *adev, int width, int bpp, bool tiled)
> +int amdgpu_align_pitch(struct amdgpu_device *adev, int width, int cpp, bool tiled)
>  {
>  	int aligned = width;
>  	int pitch_mask = 0;
>  
> -	switch (bpp / 8) {
> +	switch (cpp) {
>  	case 1:
>  		pitch_mask = 255;
>  		break;
> @@ -82,7 +82,7 @@ int amdgpu_align_pitch(struct amdgpu_device *adev, int width, int bpp, bool tile
>  
>  	aligned += pitch_mask;
>  	aligned &= ~pitch_mask;
> -	return aligned;
> +	return aligned * cpp;

Now you multiply by cpp after the rounding. Otherwise looks reasonable.
-Daniel

>  }
>  
>  static void amdgpufb_destroy_pinned_object(struct drm_gem_object *gobj)
> @@ -111,13 +111,13 @@ static int amdgpufb_create_pinned_object(struct amdgpu_fbdev *rfbdev,
>  	int ret;
>  	int aligned_size, size;
>  	int height = mode_cmd->height;
> -	u32 bpp, depth;
> +	u32 cpp;
>  
> -	drm_fb_get_bpp_depth(mode_cmd->pixel_format, &depth, &bpp);
> +	cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
>  
>  	/* need to align pitch with crtc limits */
> -	mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, bpp,
> -						  fb_tiled) * ((bpp + 1) / 8);
> +	mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, cpp,
> +						  fb_tiled);
>  
>  	height = ALIGN(mode_cmd->height, 8);
>  	size = mode_cmd->pitches[0] * height;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 88fbed2389c0..20a4e569b245 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -704,7 +704,8 @@ int amdgpu_mode_dumb_create(struct drm_file *file_priv,
>  	uint32_t handle;
>  	int r;
>  
> -	args->pitch = amdgpu_align_pitch(adev, args->width, args->bpp, 0) * ((args->bpp + 1) / 8);
> +	args->pitch = amdgpu_align_pitch(adev, args->width,
> +					 DIV_ROUND_UP(args->bpp, 8), 0);
>  	args->size = (u64)args->pitch * args->height;
>  	args->size = ALIGN(args->size, PAGE_SIZE);
>  
> -- 
> 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 v4 11/14] drm: radeon: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

[Daniel Vetter]

On Thu, Sep 08, 2016 at 05:44:25PM +0300, Laurent Pinchart wrote:
> The driver needs the number of bytes per pixel, not the bpp and depth
> info meant for fbdev compatibility. Use the right API.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v3:
> 
> - Renamed bpp to cpp
> ---
>  drivers/gpu/drm/radeon/radeon_fb.c  | 20 ++++++++++----------
>  drivers/gpu/drm/radeon/radeon_gem.c |  3 ++-
>  2 files changed, 12 insertions(+), 11 deletions(-)
> 
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Michel Dänzer <michel@daenzer.net>
> 
> diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c
> index 568e036d547e..f4d6899ce3bb 100644
> --- a/drivers/gpu/drm/radeon/radeon_fb.c
> +++ b/drivers/gpu/drm/radeon/radeon_fb.c
> @@ -61,13 +61,13 @@ static struct fb_ops radeonfb_ops = {
>  };
>  
>  
> -int radeon_align_pitch(struct radeon_device *rdev, int width, int bpp, bool tiled)
> +int radeon_align_pitch(struct radeon_device *rdev, int width, int cpp, bool tiled)
>  {
>  	int aligned = width;
>  	int align_large = (ASIC_IS_AVIVO(rdev)) || tiled;
>  	int pitch_mask = 0;
>  
> -	switch (bpp / 8) {
> +	switch (cpp) {
>  	case 1:
>  		pitch_mask = align_large ? 255 : 127;
>  		break;
> @@ -82,7 +82,7 @@ int radeon_align_pitch(struct radeon_device *rdev, int width, int bpp, bool tile
>  
>  	aligned += pitch_mask;
>  	aligned &= ~pitch_mask;
> -	return aligned;
> +	return aligned * cpp;

Same as with amdgpu, rounding and *cpp switched. Otherwise looks
reasonable.
-Daniel

>  }
>  
>  static void radeonfb_destroy_pinned_object(struct drm_gem_object *gobj)
> @@ -111,13 +111,13 @@ static int radeonfb_create_pinned_object(struct radeon_fbdev *rfbdev,
>  	int ret;
>  	int aligned_size, size;
>  	int height = mode_cmd->height;
> -	u32 bpp, depth;
> +	u32 cpp;
>  
> -	drm_fb_get_bpp_depth(mode_cmd->pixel_format, &depth, &bpp);
> +	cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
>  
>  	/* need to align pitch with crtc limits */
> -	mode_cmd->pitches[0] = radeon_align_pitch(rdev, mode_cmd->width, bpp,
> -						  fb_tiled) * ((bpp + 1) / 8);
> +	mode_cmd->pitches[0] = radeon_align_pitch(rdev, mode_cmd->width, cpp,
> +						  fb_tiled);
>  
>  	if (rdev->family >= CHIP_R600)
>  		height = ALIGN(mode_cmd->height, 8);
> @@ -137,11 +137,11 @@ static int radeonfb_create_pinned_object(struct radeon_fbdev *rfbdev,
>  		tiling_flags = RADEON_TILING_MACRO;
>  
>  #ifdef __BIG_ENDIAN
> -	switch (bpp) {
> -	case 32:
> +	switch (cpp) {
> +	case 4:
>  		tiling_flags |= RADEON_TILING_SWAP_32BIT;
>  		break;
> -	case 16:
> +	case 2:
>  		tiling_flags |= RADEON_TILING_SWAP_16BIT;
>  	default:
>  		break;
> diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c
> index deb9511725c9..0bcffd8a7bd3 100644
> --- a/drivers/gpu/drm/radeon/radeon_gem.c
> +++ b/drivers/gpu/drm/radeon/radeon_gem.c
> @@ -745,7 +745,8 @@ int radeon_mode_dumb_create(struct drm_file *file_priv,
>  	uint32_t handle;
>  	int r;
>  
> -	args->pitch = radeon_align_pitch(rdev, args->width, args->bpp, 0) * ((args->bpp + 1) / 8);
> +	args->pitch = radeon_align_pitch(rdev, args->width,
> +					 DIV_ROUND_UP(args->bpp, 8), 0);
>  	args->size = args->pitch * args->height;
>  	args->size = ALIGN(args->size, PAGE_SIZE);
>  
> -- 
> 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 v4 13/14] drm/arm: mali-dp: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

[Daniel Vetter]

On Thu, Sep 08, 2016 at 05:44:27PM +0300, Laurent Pinchart wrote:
> The driver doesn't need the color depth, only the number of bits per
> pixel. Use the right API.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/arm/malidp_hw.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
> index a6132f1d58c1..be815d0cc772 100644
> --- a/drivers/gpu/drm/arm/malidp_hw.c
> +++ b/drivers/gpu/drm/arm/malidp_hw.c
> @@ -198,9 +198,6 @@ static void malidp500_modeset(struct malidp_hw_device *hwdev, struct videomode *
>  
>  static int malidp500_rotmem_required(struct malidp_hw_device *hwdev, u16 w, u16 h, u32 fmt)
>  {
> -	unsigned int depth;
> -	int bpp;
> -
>  	/* RGB888 or BGR888 can't be rotated */
>  	if ((fmt == DRM_FORMAT_RGB888) || (fmt == DRM_FORMAT_BGR888))
>  		return -EINVAL;
> @@ -210,9 +207,7 @@ static int malidp500_rotmem_required(struct malidp_hw_device *hwdev, u16 w, u16
>  	 * worth of pixel data. Required size is then:
>  	 *    size = rotated_width * (bpp / 8) * 8;
>  	 */
> -	drm_fb_get_bpp_depth(fmt, &depth, &bpp);
> -
> -	return w * bpp;
> +	return w * drm_format_plane_cpp(fmt, 0) * 8;
>  }
>  
>  static int malidp550_query_hw(struct malidp_hw_device *hwdev)
> -- 
> 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 v4.1 01/14] drm: Centralize format information

[Laurent Pinchart]

Hi Daniel,

Thanks for the review.

On Wednesday 21 Sep 2016 09:23:14 Daniel Vetter wrote:
> On Sun, Sep 18, 2016 at 01:17:27PM +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>
> > ---
> > Changes since v4:
> > 
> > - Fixed depth value of DRM_FORMAT_[AXRGB]{4}4444 formats to match
> >   current code
> > - Documented the depth field as legacy
> > ---
> > 
> >  Documentation/gpu/drm-kms.rst |  3 ++
> >  drivers/gpu/drm/drm_fourcc.c  | 84 ++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_fourcc.h      | 21 +++++++++++
> >  3 files changed, 108 insertions(+)
> > 
> > diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> > index f9a991bb87d4..85c4c49f4436 100644
> > --- a/Documentation/gpu/drm-kms.rst
> > +++ b/Documentation/gpu/drm-kms.rst
> > @@ -63,6 +63,9 @@ Frame Buffer Functions Reference
> >  DRM Format Handling
> >  ===================
> > 
> > +.. kernel-doc:: include/drm/drm_fourcc.h
> > +   :internal:
> > +
> >  .. kernel-doc:: drivers/gpu/drm/drm_fourcc.c
> >     :export:
> >  

Given this...

[snip]

> > diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> > index 30c30fa87ee8..135fef050ee6 100644
> > --- a/include/drm/drm_fourcc.h
> > +++ b/include/drm/drm_fourcc.h
> > @@ -25,6 +25,27 @@
> > 
> >  #include <linux/types.h>
> >  #include <uapi/drm/drm_fourcc.h>
> > 
> > +/**
> > + * 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),
> > + *	valid for a subset of RGB formats only. This is a legacy field, do not
> > + *	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)
> > + * @hsub: Horizontal chroma subsampling factor
> > + * @vsub: Vertical chroma subsampling factor
> > + */
> 
> Atm we don't include the drm_fourcc.h header in
> Documentation/gpu/drm-kms.rst, which means we're missing this new
> kerneldoc.

... How do you mean exactly ? :-)

> With that fixed:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v4 02/14] drm: Implement the drm_format_*() helpers as drm_format_info() wrappers

[Laurent Pinchart]

Hi Daniel,

Thanks for the review.

On Wednesday 21 Sep 2016 09:34:13 Daniel Vetter wrote:
> On Thu, Sep 08, 2016 at 05:44:16PM +0300, Laurent Pinchart wrote:
> > 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_fourcc.c | 186 +++++++------------------------------
> >  1 file changed, 37 insertions(+), 149 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > index 6b91bd8a510d..bf91c5044d84 100644
> > --- a/drivers/gpu/drm/drm_fourcc.c
> > +++ b/drivers/gpu/drm/drm_fourcc.c
> > @@ -198,69 +198,22 @@ EXPORT_SYMBOL(drm_format_info);
> > 
> >  void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
> >  			  int *bpp)

[snip]

> > +
> > +	*depth = info->depth;
> > +	*bpp = info->cpp[0] << 3;
> 
> Bikeshed: This is a funny way to write * 8 ... Would be nice to fix imo.

Fixed.

> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v4 10/14] drm: amdgpu: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

[Laurent Pinchart]

Hi Daniel,

Thank you for the review.

On Wednesday 21 Sep 2016 09:51:44 Daniel Vetter wrote:
> On Thu, Sep 08, 2016 at 05:44:24PM +0300, Laurent Pinchart wrote:
> > The driver needs the number of bytes per pixel, not the bpp and depth
> > info meant for fbdev compatibility. Use the right API.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v3:
> > 
> > - Renamed bpp to cpp
> > ---
> > 
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c  | 14 +++++++-------
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c |  3 ++-
> >  2 files changed, 9 insertions(+), 8 deletions(-)
> > 
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: Christian König <christian.koenig@amd.com>
> > Cc: Michel Dänzer <michel@daenzer.net>
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c index bf033b58056c..0727946db189
> > 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> > @@ -62,12 +62,12 @@ static struct fb_ops amdgpufb_ops = {
> > 
> >  };
> > 
> > -int amdgpu_align_pitch(struct amdgpu_device *adev, int width, int bpp,
> > bool tiled)
> > +int amdgpu_align_pitch(struct amdgpu_device *adev, int> width, int cpp,
> > bool tiled)
> >  {
> >  	int aligned = width;
> >  	int pitch_mask = 0;
> > 
> > -	switch (bpp / 8) {
> > +	switch (cpp) {
> >  	case 1:
> >  		pitch_mask = 255;
> >  		break;
> > @@ -82,7 +82,7 @@ int amdgpu_align_pitch(struct amdgpu_device *adev, int
> > width, int bpp, bool tile
> >  	aligned += pitch_mask;
> >  	aligned &= ~pitch_mask;
> > 
> > -	return aligned;
> > +	return aligned * cpp;
> 
> Now you multiply by cpp after the rounding.

That's right, but I don't think that's a problem, as all bpp values returned 
by drm_fb_get_bpp_depth() are multiple of 8 bits.

> Otherwise looks reasonable.
> -Daniel
> 
> >  }
> >  
> >  static void amdgpufb_destroy_pinned_object(struct drm_gem_object *gobj)
> > @@ -111,13 +111,13 @@ static int amdgpufb_create_pinned_object(struct
> > amdgpu_fbdev *rfbdev,
> >  	int ret;
> >  	int aligned_size, size;
> >  	int height = mode_cmd->height;
> > -	u32 bpp, depth;
> > +	u32 cpp;
> > 
> > -	drm_fb_get_bpp_depth(mode_cmd->pixel_format, &depth, &bpp);
> > +	cpp = drm_format_plane_cpp(mode_cmd->pixel_format, 0);
> > 
> >  	/* need to align pitch with crtc limits */
> > 
> > -	mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, bpp,
> > -						  fb_tiled) * ((bpp + 1) / 8);
> > +	mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, cpp,
> > +						  fb_tiled);
> > 
> >  	height = ALIGN(mode_cmd->height, 8);
> >  	size = mode_cmd->pitches[0] * height;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c index
> > 88fbed2389c0..20a4e569b245 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> > @@ -704,7 +704,8 @@ int amdgpu_mode_dumb_create(struct drm_file
> > *file_priv,
> >  	uint32_t handle;
> >  	int r;
> > 
> > -	args->pitch = amdgpu_align_pitch(adev, args->width, args->bpp, 0) *
> > ((args->bpp + 1) / 8);
> > +	args->pitch = amdgpu_align_pitch(adev,> args->width,
> > +					 DIV_ROUND_UP(args->bpp, 8), 0);
> > 
> >  	args->size = (u64)args->pitch * args->height;
> >  	args->size = ALIGN(args->size, PAGE_SIZE);

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v4.1 01/14] drm: Centralize format information

[Daniel Vetter]

On Wed, Sep 21, 2016 at 1:21 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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),
>> > + * valid for a subset of RGB formats only. This is a legacy field, do not
>> > + * 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)
>> > + * @hsub: Horizontal chroma subsampling factor
>> > + * @vsub: Vertical chroma subsampling factor
>> > + */
>>
>> Atm we don't include the drm_fourcc.h header in
>> Documentation/gpu/drm-kms.rst, which means we're missing this new
>> kerneldoc.
>
> ... How do you mean exactly ? :-)


I'm blind ;-) r-b holds as-is.
-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 v4 10/14] drm: amdgpu: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

[Daniel Vetter]

On Wed, Sep 21, 2016 at 2:39 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> > @@ -82,7 +82,7 @@ int amdgpu_align_pitch(struct amdgpu_device *adev, int
>> > width, int bpp, bool tile
>> >     aligned += pitch_mask;
>> >     aligned &= ~pitch_mask;
>> >
>> > -   return aligned;
>> > +   return aligned * cpp;
>>
>> Now you multiply by cpp after the rounding.
>
> That's right, but I don't think that's a problem, as all bpp values returned
> by drm_fb_get_bpp_depth() are multiple of 8 bits.

Before we have ALIGN(width * cpp, pitch_mask + 1). With your patch we
have ALIGN(width, pitch_mask + 1) * cpp. In short we overalign, and
instead of e.g. aligning to 256bytes we now align to 256*4 (for
xrgb8888).
-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 v4.1 01/14] drm: Centralize format information

[Eric Engestrom]

On Sun, Sep 18, 2016 at 01:17:27PM +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>

I just realized I forgot to give you my r-b's.
Patches #1-3 are:
Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>

(One nit-pick below, though :P)

> ---
> Changes since v4:
> 
> - Fixed depth value of DRM_FORMAT_[AXRGB]{4}4444 formats to match
>   current code
> - Documented the depth field as legacy
> ---
>  Documentation/gpu/drm-kms.rst |  3 ++
>  drivers/gpu/drm/drm_fourcc.c  | 84 +++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_fourcc.h      | 21 +++++++++++
>  3 files changed, 108 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index f9a991bb87d4..85c4c49f4436 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -63,6 +63,9 @@ Frame Buffer Functions Reference
>  DRM Format Handling
>  ===================
>  
> +.. kernel-doc:: include/drm/drm_fourcc.h
> +   :internal:
> +
>  .. kernel-doc:: drivers/gpu/drm/drm_fourcc.c
>     :export:
>  
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index 29c56b4331e0..39f09c564111 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -103,6 +103,90 @@ char *drm_get_format_name(uint32_t format)
>  EXPORT_SYMBOL(drm_get_format_name);
>  
>  /**
> + * drm_format_info - query 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[] = {
> +		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGB332,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGR233,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XRGB4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XBGR4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBX4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRX4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ARGB4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ABGR4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBA4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRA4444,	.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XRGB1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XBGR1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBX5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRX5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ARGB1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ABGR1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBA5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRA5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGB565,		.depth = 16, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGR565,		.depth = 16, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGB888,		.depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGR888,		.depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XRGB8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XBGR8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBX8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRX8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XRGB2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_XBGR2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBX1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRX1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ARGB2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ABGR2101010,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBA1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRA1010102,	.depth = 30, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ARGB8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_ABGR8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_RGBA8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_BGRA8888,	.depth = 32, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YUV410,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 },
> +		{ .format = DRM_FORMAT_YVU410,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 4 },
> +		{ .format = DRM_FORMAT_YUV411,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YVU411,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 4, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YUV420,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 },
> +		{ .format = DRM_FORMAT_YVU420,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 2 },
> +		{ .format = DRM_FORMAT_YUV422,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YVU422,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YUV444,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YVU444,		.depth = 0,  .num_planes = 3, .cpp = { 1, 1, 1 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_NV12,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 },
> +		{ .format = DRM_FORMAT_NV21,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 },
> +		{ .format = DRM_FORMAT_NV16,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_NV61,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_NV24,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_NV42,		.depth = 0,  .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YUYV,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_YVYU,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_UYVY,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_VYUY,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> +		{ .format = DRM_FORMAT_AYUV,		.depth = 0,  .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },

I just realized you're aligning the .depth column using a variable-width character
(tab), which means it's not aligned for me or anyone else that doesn't have the
same tab-width as you :)
Can you replace these with spaces as you did for the rest of the columns?

Cheers,
  Eric

> +	};
> +
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
> +		if (formats[i].format == format)
> +			return &formats[i];
> +	}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL(drm_format_info);
> +
> +/**
>   * 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_fourcc.h b/include/drm/drm_fourcc.h
> index 30c30fa87ee8..135fef050ee6 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -25,6 +25,27 @@
>  #include <linux/types.h>
>  #include <uapi/drm/drm_fourcc.h>
>  
> +/**
> + * 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),
> + *	valid for a subset of RGB formats only. This is a legacy field, do not
> + *	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)
> + * @hsub: Horizontal chroma subsampling factor
> + * @vsub: Vertical chroma subsampling factor
> + */
> +struct drm_format_info {
> +	u32 format;
> +	u8 depth;
> +	u8 num_planes;
> +	u8 cpp[3];
> +	u8 hsub;
> +	u8 vsub;
> +};
> +
> +const struct drm_format_info *drm_format_info(u32 format);
>  uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
>  void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth, int *bpp);
>  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

Re: [PATCH v4 10/14] drm: amdgpu: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

[Laurent Pinchart]

Hi Daniel,

On Wednesday 21 Sep 2016 14:46:07 Daniel Vetter wrote:
> On Wed, Sep 21, 2016 at 2:39 PM, Laurent Pinchart wrote:
> >> > @@ -82,7 +82,7 @@ int amdgpu_align_pitch(struct amdgpu_device *adev,
> >> > int width, int bpp, bool tile
> >> > 
> >> >     aligned += pitch_mask;
> >> >     aligned &= ~pitch_mask;
> >> > 
> >> > -   return aligned;
> >> > +   return aligned * cpp;
> >> 
> >> Now you multiply by cpp after the rounding.
> > 
> > That's right, but I don't think that's a problem, as all bpp values
> > returned by drm_fb_get_bpp_depth() are multiple of 8 bits.
> 
> Before we have ALIGN(width * cpp, pitch_mask + 1). With your patch we
> have ALIGN(width, pitch_mask + 1) * cpp. In short we overalign, and
> instead of e.g. aligning to 256bytes we now align to 256*4 (for
> xrgb8888).

The current code is

	mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, bpp,
						  fb_tiled) * ((bpp + 1) / 8);

As bpp is a multiple of 8, this is equivalent to

	mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, bpp,
						  fb_tiled) * (bpp / 8);

The patch replaces the code with

	mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, cpp,
						  fb_tiled);

with cpp = bpp / 8, and amdgpu_align_pitch() now returns

-	return aligned;
+	return aligned * cpp;

So the patch just moves * (bpp / 8) inside the amdgpu_align_pitch() function.

The other code path is changed as follows:

-	args->pitch = amdgpu_align_pitch(adev, args->width, args->bpp, 0) * 
((args->bpp + 1) / 8);
+	args->pitch = amdgpu_align_pitch(adev, args->width,
+					 DIV_ROUND_UP(args->bpp, 8), 0);

DIV_ROUND_UP(args->bpp, 8) is equal to ((args->bpp + 1) / 8) for all supported 
bpp values, so this also moves the multiplication inside the function, without 
changing the result as far as I can see.

Note that amdgpu_align_width() is also changed as follows

-       switch (bpp / 8) {
+       switch (cpp) {
	case 1:
		pitch_mask = 255;
		break;
	case 2:
		pitch_mask = 127;
		break;
	case 3:
	case 4:
		pitch_mask = 63;
		break;
	}

This will change the pitch mask if the bpp value isn't a multiple of 8. 
However, all the formats we support use multiples of 8 as bpp values, so I 
don't see a problem here.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v4 10/14] drm: amdgpu: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

[Daniel Vetter]

On Wed, Sep 21, 2016 at 3:59 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Daniel,
>
> On Wednesday 21 Sep 2016 14:46:07 Daniel Vetter wrote:
>> On Wed, Sep 21, 2016 at 2:39 PM, Laurent Pinchart wrote:
>> >> > @@ -82,7 +82,7 @@ int amdgpu_align_pitch(struct amdgpu_device *adev,
>> >> > int width, int bpp, bool tile
>> >> >
>> >> >     aligned += pitch_mask;
>> >> >     aligned &= ~pitch_mask;
>> >> >
>> >> > -   return aligned;
>> >> > +   return aligned * cpp;
>> >>
>> >> Now you multiply by cpp after the rounding.
>> >
>> > That's right, but I don't think that's a problem, as all bpp values
>> > returned by drm_fb_get_bpp_depth() are multiple of 8 bits.
>>
>> Before we have ALIGN(width * cpp, pitch_mask + 1). With your patch we
>> have ALIGN(width, pitch_mask + 1) * cpp. In short we overalign, and
>> instead of e.g. aligning to 256bytes we now align to 256*4 (for
>> xrgb8888).
>
> The current code is
>
>         mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, bpp,
>                                                   fb_tiled) * ((bpp + 1) / 8);
>
> As bpp is a multiple of 8, this is equivalent to
>
>         mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, bpp,
>                                                   fb_tiled) * (bpp / 8);
>
> The patch replaces the code with
>
>         mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, cpp,
>                                                   fb_tiled);
>
> with cpp = bpp / 8, and amdgpu_align_pitch() now returns
>
> -       return aligned;
> +       return aligned * cpp;
>
> So the patch just moves * (bpp / 8) inside the amdgpu_align_pitch() function.
>
> The other code path is changed as follows:
>
> -       args->pitch = amdgpu_align_pitch(adev, args->width, args->bpp, 0) *
> ((args->bpp + 1) / 8);
> +       args->pitch = amdgpu_align_pitch(adev, args->width,
> +                                        DIV_ROUND_UP(args->bpp, 8), 0);
>
> DIV_ROUND_UP(args->bpp, 8) is equal to ((args->bpp + 1) / 8) for all supported
> bpp values, so this also moves the multiplication inside the function, without
> changing the result as far as I can see.
>
> Note that amdgpu_align_width() is also changed as follows
>
> -       switch (bpp / 8) {
> +       switch (cpp) {
>         case 1:
>                 pitch_mask = 255;
>                 break;
>         case 2:
>                 pitch_mask = 127;
>                 break;
>         case 3:
>         case 4:
>                 pitch_mask = 63;
>                 break;
>         }
>
> This will change the pitch mask if the bpp value isn't a multiple of 8.
> However, all the formats we support use multiples of 8 as bpp values, so I
> don't see a problem here.

All correct. The trouble is that you move the * cpp over the following code:

aligned += pitch_mask;
aligned &= ~pitch_mask;

Multiplying by 4 (for xrgb8888) before or after aligning to pitch_mask
does make a difference.
-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 v4 10/14] drm: amdgpu: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

[Laurent Pinchart]

Hi Daniel,

On Thursday 22 Sep 2016 07:31:24 Daniel Vetter wrote:
> On Wed, Sep 21, 2016 at 3:59 PM, Laurent Pinchart wrote:
> > On Wednesday 21 Sep 2016 14:46:07 Daniel Vetter wrote:
> >> On Wed, Sep 21, 2016 at 2:39 PM, Laurent Pinchart wrote:
> >>>>> @@ -82,7 +82,7 @@ int amdgpu_align_pitch(struct amdgpu_device *adev,
> >>>>> int width, int bpp, bool tile
> >>>>> 
> >>>>>     aligned += pitch_mask;
> >>>>>     aligned &= ~pitch_mask;
> >>>>> 
> >>>>> -   return aligned;
> >>>>> +   return aligned * cpp;
> >>>> 
> >>>> Now you multiply by cpp after the rounding.
> >>> 
> >>> That's right, but I don't think that's a problem, as all bpp values
> >>> returned by drm_fb_get_bpp_depth() are multiple of 8 bits.
> >> 
> >> Before we have ALIGN(width * cpp, pitch_mask + 1). With your patch we
> >> have ALIGN(width, pitch_mask + 1) * cpp. In short we overalign, and
> >> instead of e.g. aligning to 256bytes we now align to 256*4 (for
> >> xrgb8888).
> > 
> > The current code is
> > 
> >         mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width,
> >         bpp,
> >                                                   fb_tiled) * ((bpp + 1) /
> >                                                   8);
> > 
> > As bpp is a multiple of 8, this is equivalent to
> > 
> >         mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width,
> >         bpp,
> >                                                   fb_tiled) * (bpp / 8);
> > 
> > The patch replaces the code with
> > 
> >         mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width,
> >         cpp,
> >                                                   fb_tiled);
> > 
> > with cpp = bpp / 8, and amdgpu_align_pitch() now returns
> > 
> > -       return aligned;
> > +       return aligned * cpp;
> > 
> > So the patch just moves * (bpp / 8) inside the amdgpu_align_pitch()
> > function.
> > 
> > The other code path is changed as follows:
> > 
> > -       args->pitch = amdgpu_align_pitch(adev, args->width, args->bpp, 0)
> > *
> > ((args->bpp + 1) / 8);
> > +       args->pitch = amdgpu_align_pitch(adev, args->width,
> > +                                        DIV_ROUND_UP(args->bpp, 8), 0);
> > 
> > DIV_ROUND_UP(args->bpp, 8) is equal to ((args->bpp + 1) / 8) for all
> > supported bpp values, so this also moves the multiplication inside the
> > function, without changing the result as far as I can see.
> > 
> > Note that amdgpu_align_width() is also changed as follows
> > 
> > -       switch (bpp / 8) {
> > +       switch (cpp) {
> >         case 1:
> >                 pitch_mask = 255;
> >                 break;
> >         case 2:
> >                 pitch_mask = 127;
> >                 break;
> >         case 3:
> >         case 4:
> >                 pitch_mask = 63;
> >                 break;
> >         }
> > 
> > This will change the pitch mask if the bpp value isn't a multiple of 8.
> > However, all the formats we support use multiples of 8 as bpp values, so I
> > don't see a problem here.
> 
> All correct. The trouble is that you move the * cpp over the following code:
> 
> aligned += pitch_mask;
> aligned &= ~pitch_mask;
> 
> Multiplying by 4 (for xrgb8888) before or after aligning to pitch_mask
> does make a difference.

It would, but the multiplication is done *after* that block of code, isn't it 
?

        aligned += pitch_mask;
        aligned &= ~pitch_mask;
-       return aligned;
+       return aligned * cpp

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v4.1 01/14] drm: Centralize format information

[Daniel Vetter]

On Wed, Sep 21, 2016 at 02:40:40PM +0100, Eric Engestrom wrote:
> I just realized you're aligning the .depth column using a variable-width character
> (tab), which means it's not aligned for me or anyone else that doesn't have the
> same tab-width as you :)
> Can you replace these with spaces as you did for the rest of the columns?

Kernel sources have 8 chars per tab. If you editor has a different
setting, then the resulting fallout is your problem ;-) Using tabs here is
perfectly fine.
-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 v4 12/14] drm: vmwgfx: Replace drm_fb_get_bpp_depth() with drm_format_info()

[Laurent Pinchart]

Hi Daniel,

On Wednesday 21 Sep 2016 09:31:58 Daniel Vetter wrote:
> On Thu, Sep 08, 2016 at 05:44:26PM +0300, Laurent Pinchart wrote:
> > The driver is the last users of the drm_fb_get_bpp_depth() function. It
> > should ideally be converted to use struct drm_mode_fb_cmd2 instead of
> > the legacy struct drm_mode_fb_cmd internally, but that will require
> > broad changes across the code base. As a first step, replace
> > drm_fb_get_bpp_depth() with drm_format_info() in order to stop exporting
> > the function to drivers.
> > 
> > The new DRM_ERROR() message comes from the vmw_create_dmabuf_proxy(),
> > vmw_kms_new_framebuffer_surface() and vmw_kms_new_framebuffer_dmabuf()
> > functions that currently print an error if the pixel format is
> > unsupported.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Sinclair Yeh <syeh@vmware.com>
> > ---
> > 
> >  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > Cc: VMware Graphics <linux-graphics-maintainer@vmware.com>
> > Cc: Sinclair Yeh <syeh@vmware.com>
> > Cc: Thomas Hellstrom <thellstrom@vmware.com>
> > 
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index bf28ccc150df..c965514b82be
> > 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > @@ -980,14 +980,22 @@ static struct drm_framebuffer
> > *vmw_kms_fb_create(struct drm_device *dev,> 
> >  	struct vmw_dma_buffer *bo = NULL;
> >  	struct ttm_base_object *user_obj;
> >  	struct drm_mode_fb_cmd mode_cmd;
> > 
> > +	const struct drm_format_info *info;
> > 
> >  	int ret;
> > 
> > +	info = drm_format_info(mode_cmd2->pixel_format);
> > +	if (!info || !info->depth) {
> > +		DRM_ERROR("Unsupported framebuffer format %s\n",
> > +			  drm_get_format_name(mode_cmd2->pixel_format));
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > 
> >  	mode_cmd.width = mode_cmd2->width;
> >  	mode_cmd.height = mode_cmd2->height;
> >  	mode_cmd.pitch = mode_cmd2->pitches[0];
> >  	mode_cmd.handle = mode_cmd2->handles[0];
> > 
> > -	drm_fb_get_bpp_depth(mode_cmd2->pixel_format, &mode_cmd.depth,
> > -				    &mode_cmd.bpp);
> > +	mode_cmd.depth = info->depth;
> > +	mode_cmd.bpp = info->cpp[0] * 8;
> 
> I think this should use drm_helper_mode_fill_fb_struct instead.

I would do that if there was a struct drm_framebuffer to fill, but this piece 
of code converts from struct drm_mode_fb_cmd2 to drm_mode_fb_cmd that is then 
used all over the place internally. This should be fixed, but I think that's 
out of scope for this patch series.

> >  	/**
> >  	
> >  	 * This code should be conditioned on Screen Objects not being used.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v4 12/14] drm: vmwgfx: Replace drm_fb_get_bpp_depth() with drm_format_info()

[Daniel Vetter]

On Fri, Sep 23, 2016 at 03:40:17PM +0300, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Wednesday 21 Sep 2016 09:31:58 Daniel Vetter wrote:
> > On Thu, Sep 08, 2016 at 05:44:26PM +0300, Laurent Pinchart wrote:
> > > The driver is the last users of the drm_fb_get_bpp_depth() function. It
> > > should ideally be converted to use struct drm_mode_fb_cmd2 instead of
> > > the legacy struct drm_mode_fb_cmd internally, but that will require
> > > broad changes across the code base. As a first step, replace
> > > drm_fb_get_bpp_depth() with drm_format_info() in order to stop exporting
> > > the function to drivers.
> > > 
> > > The new DRM_ERROR() message comes from the vmw_create_dmabuf_proxy(),
> > > vmw_kms_new_framebuffer_surface() and vmw_kms_new_framebuffer_dmabuf()
> > > functions that currently print an error if the pixel format is
> > > unsupported.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Reviewed-by: Sinclair Yeh <syeh@vmware.com>
> > > ---
> > > 
> > >  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 12 ++++++++++--
> > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > 
> > > Cc: VMware Graphics <linux-graphics-maintainer@vmware.com>
> > > Cc: Sinclair Yeh <syeh@vmware.com>
> > > Cc: Thomas Hellstrom <thellstrom@vmware.com>
> > > 
> > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > > b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index bf28ccc150df..c965514b82be
> > > 100644
> > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > > @@ -980,14 +980,22 @@ static struct drm_framebuffer
> > > *vmw_kms_fb_create(struct drm_device *dev,> 
> > >  	struct vmw_dma_buffer *bo = NULL;
> > >  	struct ttm_base_object *user_obj;
> > >  	struct drm_mode_fb_cmd mode_cmd;
> > > 
> > > +	const struct drm_format_info *info;
> > > 
> > >  	int ret;
> > > 
> > > +	info = drm_format_info(mode_cmd2->pixel_format);
> > > +	if (!info || !info->depth) {
> > > +		DRM_ERROR("Unsupported framebuffer format %s\n",
> > > +			  drm_get_format_name(mode_cmd2->pixel_format));
> > > +		return ERR_PTR(-EINVAL);
> > > +	}
> > > +
> > > 
> > >  	mode_cmd.width = mode_cmd2->width;
> > >  	mode_cmd.height = mode_cmd2->height;
> > >  	mode_cmd.pitch = mode_cmd2->pitches[0];
> > >  	mode_cmd.handle = mode_cmd2->handles[0];
> > > 
> > > -	drm_fb_get_bpp_depth(mode_cmd2->pixel_format, &mode_cmd.depth,
> > > -				    &mode_cmd.bpp);
> > > +	mode_cmd.depth = info->depth;
> > > +	mode_cmd.bpp = info->cpp[0] * 8;
> > 
> > I think this should use drm_helper_mode_fill_fb_struct instead.
> 
> I would do that if there was a struct drm_framebuffer to fill, but this piece 
> of code converts from struct drm_mode_fb_cmd2 to drm_mode_fb_cmd that is then 
> used all over the place internally. This should be fixed, but I think that's 
> out of scope for this patch series.

Oh right, I didn't realize that ...

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> 
> > >  	/**
> > >  	
> > >  	 * This code should be conditioned on Screen Objects not being used.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
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