[PATCH 0/3] Add Mali DP pixel formats

[PATCH 0/3] Add Mali DP pixel formats

[Alexandru Gheorghe]

Mali DP supports a bunch of pixel formats that don't have a fourcc
code defined in drm_fourcc.h, so this patchset adds the definition for
those formats and enables them in mali-dp driver.

The following new formats will be added:

Packed YCbCr444
* DRM_FORMAT_XYUV8888
* DRM_FORMAT_XVYU2101010

Two plane 10 bits format.
* DRM_FORMAT_P010

Packed YCbCr420 2x2 tiled formats
* DRM_FORMAT_Y0L0
* DRM_FORMAT_X0L0
* DRM_FORMAT_Y0L2
* DRM_FORMAT_X0L2
The difference between X0L0/X0L2 vs Y0L0/Y0L2 is that the later group
have two alpha bits per pixel.

This group is a bit special because we are dealing with a tiled format
where the first 64 bits in memory represent the pixels for a 2x2 tile,
so it needs a bit of special handling when it comes to:
 - pitch: needs to cover both rows that are in the same tile.
 - min allocation size: since a pitch cover both rows the formulas
   defined in drm_gem_fb_create don't work anymore.
 - handling of src_x and src_y offset: same as above since we are
   dealing with a tiled format drm_fb_cma_get_gem_obj doesn't return
   the correct address offset.

So, for this formats mali-dp needs a special implementation of
drm_fb_cma_get_gem_obj and drm_fb_cma_get_gem_addr which are more or
less clones of the core functions, except they take into account the
tile_size.

Other alternatives would be:
  1) Add a tile_size to drm_format_info and plumb the drm_core
     functions to proper handle this special formats.
  2) Add a driver hook that checks buffer min_size and avoid
     duplicating the code from drm_gem_fb_create_with_funcs.

If you have an opinion about how this should be handled feel free to
suggest it.

Alexandru Gheorghe (3):
  drm/fourcc: Add malidp yuv formats
  drm: Make drm_gem_fb_alloc available for drivers to use
  drm: mali-dp: Enable mali specific buffer formats

 drivers/gpu/drm/arm/malidp_drv.c             | 65 +++++++++++++++++++-
 drivers/gpu/drm/arm/malidp_hw.c              |  7 ++-
 drivers/gpu/drm/arm/malidp_planes.c          | 52 +++++++++++++---
 drivers/gpu/drm/drm_fourcc.c                 |  7 +++
 drivers/gpu/drm/drm_gem_framebuffer_helper.c |  3 +-
 include/drm/drm_gem_framebuffer_helper.h     |  5 ++
 include/uapi/drm/drm_fourcc.h                | 27 +++++++-
 7 files changed, 154 insertions(+), 12 deletions(-)

-- 
2.18.0

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

[PATCH 1/3] drm/fourcc: Add malidp yuv formats

[Alexandru Gheorghe]

Malidp implements a number of yuv buffer formats which are not
currently described in drm_fourcc.h.
This adds those definitions and describes their memory layout.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drivers/gpu/drm/drm_fourcc.c  |  7 +++++++
 include/uapi/drm/drm_fourcc.h | 27 ++++++++++++++++++++++++++-
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 35c1e2742c27..18bb960e9943 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -173,6 +173,13 @@ const struct drm_format_info *__drm_format_info(u32 format)
 		{ .format = DRM_FORMAT_UYVY,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true },
 		{ .format = DRM_FORMAT_VYUY,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1, .is_yuv = true },
 		{ .format = DRM_FORMAT_AYUV,		.depth = 0,  .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true, .is_yuv = true },
+		{ .format = DRM_FORMAT_XYUV8888,	.depth = 0,  .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .is_yuv = true },
+		{ .format = DRM_FORMAT_XVYU2101010,	.depth = 0,  .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .is_yuv = true },
+		{ .format = DRM_FORMAT_Y0L0,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 2, .has_alpha = true, .is_yuv = true },
+		{ .format = DRM_FORMAT_X0L0,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 2, .is_yuv = true },
+		{ .format = DRM_FORMAT_Y0L2,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 2, .has_alpha = true, .is_yuv = true },
+		{ .format = DRM_FORMAT_X0L2,		.depth = 0,  .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 2, .is_yuv = true },
+		{ .format = DRM_FORMAT_P010,		.depth = 0,  .num_planes = 2, .cpp = { 2, 4, 0 }, .hsub = 2, .vsub = 2, .is_yuv  = true },
 	};
 
 	unsigned int i;
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index d43949b5bb3e..ba2fd9e9815d 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -105,13 +105,31 @@ extern "C" {
 #define DRM_FORMAT_RGBA1010102	fourcc_code('R', 'A', '3', '0') /* [31:0] R:G:B:A 10:10:10:2 little endian */
 #define DRM_FORMAT_BGRA1010102	fourcc_code('B', 'A', '3', '0') /* [31:0] B:G:R:A 10:10:10:2 little endian */
 
-/* packed YCbCr */
+/* packed YCbCr422 */
 #define DRM_FORMAT_YUYV		fourcc_code('Y', 'U', 'Y', 'V') /* [31:0] Cr0:Y1:Cb0:Y0 8:8:8:8 little endian */
 #define DRM_FORMAT_YVYU		fourcc_code('Y', 'V', 'Y', 'U') /* [31:0] Cb0:Y1:Cr0:Y0 8:8:8:8 little endian */
 #define DRM_FORMAT_UYVY		fourcc_code('U', 'Y', 'V', 'Y') /* [31:0] Y1:Cr0:Y0:Cb0 8:8:8:8 little endian */
 #define DRM_FORMAT_VYUY		fourcc_code('V', 'Y', 'U', 'Y') /* [31:0] Y1:Cb0:Y0:Cr0 8:8:8:8 little endian */
 
+/* packed YCbCr444 */
 #define DRM_FORMAT_AYUV		fourcc_code('A', 'Y', 'U', 'V') /* [31:0] A:Y:Cb:Cr 8:8:8:8 little endian */
+#define DRM_FORMAT_XYUV8888	fourcc_code('X', 'Y', 'U', 'V') /* [31:0] X:Y:Cb:Cr 8:8:8:8 little endian */
+#define DRM_FORMAT_XVYU2101010	fourcc_code('V', 'U', '3', '0') /* [31:0] X:Cr:Y:Cb 2:10:10:10 little endian */
+
+/*
+ * packed YCbCr420 2x2 tiled formats
+ * first 64 bits will contain Y,Cb,Cr components for a 2x2 tile
+ */
+
+/* [63:0]   A3:A2:Y3:0:Cr0:0:Y2:0:A1:A0:Y1:0:Cb0:0:Y0:0  1:1:8:2:8:2:8:2:1:1:8:2:8:2:8:2 little endian */
+#define DRM_FORMAT_Y0L0		fourcc_code('Y', '0', 'L', '0')
+/* [63:0]   X3:X2:Y3:0:Cr0:0:Y2:0:X1:X0:Y1:0:Cb0:0:Y0:0  1:1:8:2:8:2:8:2:1:1:8:2:8:2:8:2 little endian */
+#define DRM_FORMAT_X0L0		fourcc_code('X', '0', 'L', '0')
+
+/* [63:0]   A3:A2:Y3:Cr0:Y2:A1:A0:Y1:Cb0:Y0  1:1:10:10:10:1:1:10:10:10 little endian */
+#define DRM_FORMAT_Y0L2		fourcc_code('Y', '0', 'L', '2')
+/* [63:0]   X3:X2:Y3:Cr0:Y2:X1:X0:Y1:Cb0:Y0  1:1:10:10:10:1:1:10:10:10 little endian */
+#define DRM_FORMAT_X0L2		fourcc_code('X', '0', 'L', '2')
 
 /*
  * 2 plane RGB + A
@@ -141,6 +159,13 @@ extern "C" {
 #define DRM_FORMAT_NV24		fourcc_code('N', 'V', '2', '4') /* non-subsampled Cr:Cb plane */
 #define DRM_FORMAT_NV42		fourcc_code('N', 'V', '4', '2') /* non-subsampled Cb:Cr plane */
 
+/*
+ * Each sample packed into the top 10 bits of a 16-bit word.
+ * Y plane: [63:0] Y3:0:Y2:0:Y1:0:Y0:0, 10:6:10:6:10:6:10:6
+ * CrCb plane: [63:0] Cr2:0:Cb2:0:Cr0:0:Cb0:0, 10:6:10:6:10:6:10:6
+ */
+#define DRM_FORMAT_P010		fourcc_code('P', '0', '1', '0') /* 2x2 subsampled Cr:Cb plane */
+
 /*
  * 3 plane YCbCr
  * index 0: Y plane, [7:0] Y
-- 
2.18.0

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

[PATCH 2/3] drm: Make drm_gem_fb_alloc available for drivers to use

[Alexandru Gheorghe]

Some drivers can't use drm_gem_fb_create, so instead of copying the
logic that does the framebuffer allocation allow them to use core
drm_gem_fb_alloc.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drivers/gpu/drm/drm_gem_framebuffer_helper.c | 3 ++-
 include/drm/drm_gem_framebuffer_helper.h     | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
index 2810d4131411..64eddf5a1bd9 100644
--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
@@ -57,7 +57,7 @@ struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
 }
 EXPORT_SYMBOL_GPL(drm_gem_fb_get_obj);
 
-static struct drm_framebuffer *
+struct drm_framebuffer *
 drm_gem_fb_alloc(struct drm_device *dev,
 		 const struct drm_mode_fb_cmd2 *mode_cmd,
 		 struct drm_gem_object **obj, unsigned int num_planes,
@@ -85,6 +85,7 @@ drm_gem_fb_alloc(struct drm_device *dev,
 
 	return fb;
 }
+EXPORT_SYMBOL_GPL(drm_gem_fb_alloc);
 
 /**
  * drm_gem_fb_destroy - Free GEM backed framebuffer
diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
index a38de7eb55b4..d20c1356000a 100644
--- a/include/drm/drm_gem_framebuffer_helper.h
+++ b/include/drm/drm_gem_framebuffer_helper.h
@@ -14,6 +14,11 @@ struct drm_simple_display_pipe;
 
 struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
 					  unsigned int plane);
+struct drm_framebuffer *
+drm_gem_fb_alloc(struct drm_device *dev,
+		 const struct drm_mode_fb_cmd2 *mode_cmd,
+		 struct drm_gem_object **obj, unsigned int num_planes,
+		 const struct drm_framebuffer_funcs *funcs);
 void drm_gem_fb_destroy(struct drm_framebuffer *fb);
 int drm_gem_fb_create_handle(struct drm_framebuffer *fb, struct drm_file *file,
 			     unsigned int *handle);
-- 
2.18.0

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

[PATCH 3/3] drm: mali-dp: Enable mali specific buffer formats

[Alexandru Gheorghe]

Enable the following formats
 - DRM_FORMAT_XYUV8888
 - DRM_FORMAT_XVYU2101010
 - DRM_FORMAT_X0L0
 - DRM_FORMAT_X0L2
 - DRM_FORMAT_P010

All formats respect the rules checked by core framebuffer_check except
DRM_FORMAT_X0L0 and DRM_FORMAT_X0L2 for which we neeed to take into
consideration that it's a 2x2 tiled format, so the following things
need special handling:

1) PITCH: needs to cover two rows.
2) GEM_SIZE: the core formula (drm_gem_fb_create_with_funcs) that
checks min_object size doesn't work anymore, so I added special check
in driver for X0L0 and X0L2.
3) SOURCE_CROPPING: drm_fb_cma_get_gem_addr doesn't properly retrieves
start address, so I added the right formula for DRM_FORMAT_X0L0 and
DRM_FORMAT_X0L2 inside the driver.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drivers/gpu/drm/arm/malidp_drv.c    | 65 ++++++++++++++++++++++++++++-
 drivers/gpu/drm/arm/malidp_hw.c     |  7 +++-
 drivers/gpu/drm/arm/malidp_planes.c | 52 +++++++++++++++++++----
 3 files changed, 114 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 5b7260557391..6745c4639dd4 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -258,8 +258,71 @@ static const struct drm_mode_config_helper_funcs malidp_mode_config_helpers = {
 	.atomic_commit_tail = malidp_atomic_commit_tail,
 };
 
+static const struct drm_framebuffer_funcs malidp_gem_fb_funcs = {
+	.destroy	= drm_gem_fb_destroy,
+	.create_handle	= drm_gem_fb_create_handle,
+};
+
+struct drm_framebuffer *
+malidp_fb_create(struct drm_device *dev, struct drm_file *file,
+		 const struct drm_mode_fb_cmd2 *mode_cmd)
+{
+	if (mode_cmd->pixel_format == DRM_FORMAT_X0L2 ||
+	    mode_cmd->pixel_format == DRM_FORMAT_X0L0) {
+		const struct drm_format_info *info;
+		struct drm_gem_object *obj;
+		struct drm_framebuffer *fb = NULL;
+		const unsigned int tile_size = 2;
+		unsigned int min_size;
+
+		info = drm_format_info(mode_cmd->pixel_format &
+					~DRM_FORMAT_BIG_ENDIAN);
+		/*
+		 * Pitch needs to take into consideration that we are dealing
+		 * with a tiled 2x2 format, so the pitch/stride need to cover
+		 * both rows
+		 */
+		if (mode_cmd->pitches[0] < mode_cmd->width * info->cpp[0] *
+				tile_size) {
+			struct drm_format_name_buf format_name;
+
+			drm_get_format_name(mode_cmd->pixel_format,
+					    &format_name);
+			DRM_DEBUG_KMS("Invalid pitch for format %s",
+				      format_name.str);
+			return ERR_PTR(-EINVAL);
+		}
+		obj = drm_gem_object_lookup(file, mode_cmd->handles[0]);
+		if (!obj) {
+			DRM_DEBUG_KMS("Failed to lookup GEM object\n");
+			fb = ERR_PTR(-ENOENT);
+			goto err_gem_object_put;
+		}
+		min_size = mode_cmd->height / tile_size  * mode_cmd->pitches[0];
+		if (obj->size < min_size) {
+			drm_gem_object_put_unlocked(obj);
+			DRM_DEBUG_KMS("Object size is less than minimum"
+				      " required\n");
+			fb = ERR_PTR(-EINVAL);
+			goto err_gem_object_put;
+		}
+
+		fb = drm_gem_fb_alloc(dev, mode_cmd, &obj, 1,
+				      &malidp_gem_fb_funcs);
+		if (IS_ERR(fb))
+			goto err_gem_object_put;
+		return fb;
+
+	err_gem_object_put:
+		drm_gem_object_put_unlocked(obj);
+		return fb;
+	}
+
+	return drm_gem_fb_create(dev, file, mode_cmd);
+}
+
 static const struct drm_mode_config_funcs malidp_mode_config_funcs = {
-	.fb_create = drm_gem_fb_create,
+	.fb_create = malidp_fb_create,
 	.output_poll_changed = drm_fb_helper_output_poll_changed,
 	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = drm_atomic_helper_commit,
diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index c94a4422e0e9..472cae76e19b 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -74,10 +74,15 @@ static const struct malidp_format_id malidp500_de_formats[] = {
 	{ DRM_FORMAT_ABGR1555, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(4, 1) }, \
 	{ DRM_FORMAT_RGB565, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(4, 2) }, \
 	{ DRM_FORMAT_BGR565, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(4, 3) }, \
+	{ DRM_FORMAT_XYUV8888, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 0) }, \
 	{ DRM_FORMAT_YUYV, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 2) },	\
 	{ DRM_FORMAT_UYVY, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 3) },	\
+	{ DRM_FORMAT_X0L0, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 4)}, \
 	{ DRM_FORMAT_NV12, DE_VIDEO1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(5, 6) },	\
-	{ DRM_FORMAT_YUV420, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 7) }
+	{ DRM_FORMAT_YUV420, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 7) }, \
+	{ DRM_FORMAT_XVYU2101010, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(6, 0)}, \
+	{ DRM_FORMAT_X0L2, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(6, 6)}, \
+	{ DRM_FORMAT_P010, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(6, 7)}
 
 static const struct malidp_format_id malidp550_de_formats[] = {
 	MALIDP_COMMON_FORMATS,
diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
index 29409a65d864..11fbac3ced81 100644
--- a/drivers/gpu/drm/arm/malidp_planes.c
+++ b/drivers/gpu/drm/arm/malidp_planes.c
@@ -211,7 +211,17 @@ static int malidp_de_plane_check(struct drm_plane *plane,
 	    (state->crtc_w < mp->hwdev->min_line_size) ||
 	    (state->crtc_h < mp->hwdev->min_line_size))
 		return -EINVAL;
-
+	/*
+	 * Tiled formats DRM_FORMAT_X0L2 and DRM_FORMAT_X0L0
+	 * can be cropped only at multiple of tile dimension
+	 * which is 2.
+	 */
+	if ((fb->format->format == DRM_FORMAT_X0L2 ||
+	    fb->format->format == DRM_FORMAT_X0L0) &&
+	    ((state->src_x >> 16) % 2 || (state->src_y >> 16) % 2)) {
+		DRM_DEBUG_KMS("Invalid crop values");
+		return -EINVAL;
+	}
 	/*
 	 * DP550/650 video layers can accept 3 plane formats only if
 	 * fb->pitches[1] == fb->pitches[2] since they don't have a
@@ -321,6 +331,38 @@ static void malidp_de_set_color_encoding(struct malidp_plane *plane,
 	}
 }
 
+static void malidp_set_plane_base_addr(struct drm_framebuffer *fb,
+				       struct malidp_plane *mp,
+				       int plane_index)
+{
+	dma_addr_t paddr;
+	u16 ptr;
+	struct drm_plane *plane = &mp->base;
+
+	ptr = mp->layer->ptr + (plane_index << 4);
+
+	if (fb->format->format == DRM_FORMAT_X0L2 ||
+	    fb->format->format == DRM_FORMAT_X0L0) {
+		struct drm_gem_cma_object *obj;
+		int tile_size = 2;
+
+		obj = drm_fb_cma_get_gem_obj(fb, plane_index);
+		if (WARN_ON(!obj))
+			return;
+		paddr = obj->paddr + fb->offsets[plane_index];
+		paddr += fb->format->cpp[plane_index] *
+			 (plane->state->src_x >> 16) * tile_size;
+		paddr += (fb->pitches[plane_index] / tile_size) *
+				(plane->state->src_y >> 16);
+
+	} else
+		paddr = drm_fb_cma_get_gem_addr(fb, plane->state,
+						plane_index);
+
+	malidp_hw_write(mp->hwdev, lower_32_bits(paddr), ptr);
+	malidp_hw_write(mp->hwdev, upper_32_bits(paddr), ptr + 4);
+}
+
 static void malidp_de_plane_update(struct drm_plane *plane,
 				   struct drm_plane_state *old_state)
 {
@@ -343,13 +385,7 @@ static void malidp_de_plane_update(struct drm_plane *plane,
 	malidp_hw_write(mp->hwdev, val, mp->layer->base);
 
 	for (i = 0; i < ms->n_planes; i++) {
-		/* calculate the offset for the layer's plane registers */
-		u16 ptr = mp->layer->ptr + (i << 4);
-		dma_addr_t fb_addr = drm_fb_cma_get_gem_addr(plane->state->fb,
-							     plane->state, i);
-
-		malidp_hw_write(mp->hwdev, lower_32_bits(fb_addr), ptr);
-		malidp_hw_write(mp->hwdev, upper_32_bits(fb_addr), ptr + 4);
+		malidp_set_plane_base_addr(plane->state->fb, mp, i);
 	}
 	malidp_de_set_plane_pitches(mp, ms->n_planes,
 				    plane->state->fb->pitches);
-- 
2.18.0

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

Re: [PATCH 2/3] drm: Make drm_gem_fb_alloc available for drivers to use

[Liviu Dudau]

On Thu, Jul 26, 2018 at 03:10:04PM +0100, Alexandru Gheorghe wrote:
> Some drivers can't use drm_gem_fb_create, so instead of copying the
> logic that does the framebuffer allocation allow them to use core
> drm_gem_fb_alloc.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>

To me it looks like an useful thing to have exported, so for what is worth:

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

Sean, Maarten, Laurent, Gustavo, Daniel(s), do you have any objections?

Best regards,
Liviu

> ---
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 3 ++-
>  include/drm/drm_gem_framebuffer_helper.h     | 5 +++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> index 2810d4131411..64eddf5a1bd9 100644
> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -57,7 +57,7 @@ struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_fb_get_obj);
>  
> -static struct drm_framebuffer *
> +struct drm_framebuffer *
>  drm_gem_fb_alloc(struct drm_device *dev,
>  		 const struct drm_mode_fb_cmd2 *mode_cmd,
>  		 struct drm_gem_object **obj, unsigned int num_planes,
> @@ -85,6 +85,7 @@ drm_gem_fb_alloc(struct drm_device *dev,
>  
>  	return fb;
>  }
> +EXPORT_SYMBOL_GPL(drm_gem_fb_alloc);
>  
>  /**
>   * drm_gem_fb_destroy - Free GEM backed framebuffer
> diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
> index a38de7eb55b4..d20c1356000a 100644
> --- a/include/drm/drm_gem_framebuffer_helper.h
> +++ b/include/drm/drm_gem_framebuffer_helper.h
> @@ -14,6 +14,11 @@ struct drm_simple_display_pipe;
>  
>  struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
>  					  unsigned int plane);
> +struct drm_framebuffer *
> +drm_gem_fb_alloc(struct drm_device *dev,
> +		 const struct drm_mode_fb_cmd2 *mode_cmd,
> +		 struct drm_gem_object **obj, unsigned int num_planes,
> +		 const struct drm_framebuffer_funcs *funcs);
>  void drm_gem_fb_destroy(struct drm_framebuffer *fb);
>  int drm_gem_fb_create_handle(struct drm_framebuffer *fb, struct drm_file *file,
>  			     unsigned int *handle);
> -- 
> 2.18.0
> 

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

Re: [PATCH 2/3] drm: Make drm_gem_fb_alloc available for drivers to use

[Daniel Vetter]

On Wed, Aug 15, 2018 at 12:08:38PM +0100, Liviu Dudau wrote:
> On Thu, Jul 26, 2018 at 03:10:04PM +0100, Alexandru Gheorghe wrote:
> > Some drivers can't use drm_gem_fb_create, so instead of copying the
> > logic that does the framebuffer allocation allow them to use core
> > drm_gem_fb_alloc.
> > 
> > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> 
> To me it looks like an useful thing to have exported, so for what is worth:
> 
> Acked-by: Liviu Dudau <liviu.dudau@arm.com>
> 
> Sean, Maarten, Laurent, Gustavo, Daniel(s), do you have any objections?

In general please add meaningful kernel-doc for exported functions,
explaing what's different and how it works together.

Wrt the specific issue, I think we should teach drm core and helpers how
to allocate formats with tiled blocks. Means we need to extend
drm_format_info a bit. Your YUV formats won't be the only ones where the
format itself arranges pixels in blocks, and hence has format-based
alignment criteria for pitch and size (due to line rounding).

I think this would also fit better into the overall design where drivers
can overwrite the drm_format_info for specific (fourcc, modifier)
combinations.
-Daniel


> 
> Best regards,
> Liviu
> 
> > ---
> >  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 3 ++-
> >  include/drm/drm_gem_framebuffer_helper.h     | 5 +++++
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > index 2810d4131411..64eddf5a1bd9 100644
> > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > @@ -57,7 +57,7 @@ struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
> >  }
> >  EXPORT_SYMBOL_GPL(drm_gem_fb_get_obj);
> >  
> > -static struct drm_framebuffer *
> > +struct drm_framebuffer *
> >  drm_gem_fb_alloc(struct drm_device *dev,
> >  		 const struct drm_mode_fb_cmd2 *mode_cmd,
> >  		 struct drm_gem_object **obj, unsigned int num_planes,
> > @@ -85,6 +85,7 @@ drm_gem_fb_alloc(struct drm_device *dev,
> >  
> >  	return fb;
> >  }
> > +EXPORT_SYMBOL_GPL(drm_gem_fb_alloc);
> >  
> >  /**
> >   * drm_gem_fb_destroy - Free GEM backed framebuffer
> > diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
> > index a38de7eb55b4..d20c1356000a 100644
> > --- a/include/drm/drm_gem_framebuffer_helper.h
> > +++ b/include/drm/drm_gem_framebuffer_helper.h
> > @@ -14,6 +14,11 @@ struct drm_simple_display_pipe;
> >  
> >  struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
> >  					  unsigned int plane);
> > +struct drm_framebuffer *
> > +drm_gem_fb_alloc(struct drm_device *dev,
> > +		 const struct drm_mode_fb_cmd2 *mode_cmd,
> > +		 struct drm_gem_object **obj, unsigned int num_planes,
> > +		 const struct drm_framebuffer_funcs *funcs);
> >  void drm_gem_fb_destroy(struct drm_framebuffer *fb);
> >  int drm_gem_fb_create_handle(struct drm_framebuffer *fb, struct drm_file *file,
> >  			     unsigned int *handle);
> > -- 
> > 2.18.0
> > 
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯

-- 
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 2/3] drm: Make drm_gem_fb_alloc available for drivers to use

[Alexandru-Cosmin Gheorghe]

On Wed, Aug 15, 2018 at 01:54:12PM +0200, Daniel Vetter wrote:
> On Wed, Aug 15, 2018 at 12:08:38PM +0100, Liviu Dudau wrote:
> > On Thu, Jul 26, 2018 at 03:10:04PM +0100, Alexandru Gheorghe wrote:
> > > Some drivers can't use drm_gem_fb_create, so instead of copying the
> > > logic that does the framebuffer allocation allow them to use core
> > > drm_gem_fb_alloc.
> > > 
> > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > 
> > To me it looks like an useful thing to have exported, so for what is worth:
> > 
> > Acked-by: Liviu Dudau <liviu.dudau@arm.com>
> > 
> > Sean, Maarten, Laurent, Gustavo, Daniel(s), do you have any objections?
> 
> In general please add meaningful kernel-doc for exported functions,
> explaing what's different and how it works together.
> 
> Wrt the specific issue, I think we should teach drm core and helpers how
> to allocate formats with tiled blocks. Means we need to extend
> drm_format_info a bit. Your YUV formats won't be the only ones where the
> format itself arranges pixels in blocks, and hence has format-based
> alignment criteria for pitch and size (due to line rounding).

Something like a tile_size or do you have other ideas ? 
Any idea if there are any tile formats out there where the tile it's
not a square ?

> 
> I think this would also fit better into the overall design where drivers
> can overwrite the drm_format_info for specific (fourcc, modifier)
> combinations.
> -Daniel
> 
> 
> > 
> > Best regards,
> > Liviu
> > 
> > > ---
> > >  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 3 ++-
> > >  include/drm/drm_gem_framebuffer_helper.h     | 5 +++++
> > >  2 files changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > index 2810d4131411..64eddf5a1bd9 100644
> > > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > @@ -57,7 +57,7 @@ struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
> > >  }
> > >  EXPORT_SYMBOL_GPL(drm_gem_fb_get_obj);
> > >  
> > > -static struct drm_framebuffer *
> > > +struct drm_framebuffer *
> > >  drm_gem_fb_alloc(struct drm_device *dev,
> > >  		 const struct drm_mode_fb_cmd2 *mode_cmd,
> > >  		 struct drm_gem_object **obj, unsigned int num_planes,
> > > @@ -85,6 +85,7 @@ drm_gem_fb_alloc(struct drm_device *dev,
> > >  
> > >  	return fb;
> > >  }
> > > +EXPORT_SYMBOL_GPL(drm_gem_fb_alloc);
> > >  
> > >  /**
> > >   * drm_gem_fb_destroy - Free GEM backed framebuffer
> > > diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
> > > index a38de7eb55b4..d20c1356000a 100644
> > > --- a/include/drm/drm_gem_framebuffer_helper.h
> > > +++ b/include/drm/drm_gem_framebuffer_helper.h
> > > @@ -14,6 +14,11 @@ struct drm_simple_display_pipe;
> > >  
> > >  struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
> > >  					  unsigned int plane);
> > > +struct drm_framebuffer *
> > > +drm_gem_fb_alloc(struct drm_device *dev,
> > > +		 const struct drm_mode_fb_cmd2 *mode_cmd,
> > > +		 struct drm_gem_object **obj, unsigned int num_planes,
> > > +		 const struct drm_framebuffer_funcs *funcs);
> > >  void drm_gem_fb_destroy(struct drm_framebuffer *fb);
> > >  int drm_gem_fb_create_handle(struct drm_framebuffer *fb, struct drm_file *file,
> > >  			     unsigned int *handle);
> > > -- 
> > > 2.18.0
> > > 
> > 
> > -- 
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ¯\_(ツ)_/¯
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH 2/3] drm: Make drm_gem_fb_alloc available for drivers to use

[Daniel Vetter]

On Wed, Aug 15, 2018 at 2:27 PM, Alexandru-Cosmin Gheorghe
<Alexandru-Cosmin.Gheorghe@arm.com> wrote:
> On Wed, Aug 15, 2018 at 01:54:12PM +0200, Daniel Vetter wrote:
>> On Wed, Aug 15, 2018 at 12:08:38PM +0100, Liviu Dudau wrote:
>> > On Thu, Jul 26, 2018 at 03:10:04PM +0100, Alexandru Gheorghe wrote:
>> > > Some drivers can't use drm_gem_fb_create, so instead of copying the
>> > > logic that does the framebuffer allocation allow them to use core
>> > > drm_gem_fb_alloc.
>> > >
>> > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
>> >
>> > To me it looks like an useful thing to have exported, so for what is worth:
>> >
>> > Acked-by: Liviu Dudau <liviu.dudau@arm.com>
>> >
>> > Sean, Maarten, Laurent, Gustavo, Daniel(s), do you have any objections?
>>
>> In general please add meaningful kernel-doc for exported functions,
>> explaing what's different and how it works together.
>>
>> Wrt the specific issue, I think we should teach drm core and helpers how
>> to allocate formats with tiled blocks. Means we need to extend
>> drm_format_info a bit. Your YUV formats won't be the only ones where the
>> format itself arranges pixels in blocks, and hence has format-based
>> alignment criteria for pitch and size (due to line rounding).
>
> Something like a tile_size or do you have other ideas ?
> Any idea if there are any tile formats out there where the tile it's
> not a square ?

I think both tile_w/_h in pixels and tile_size in bytes is what you
want for full description of all options. And yes I think some of the
compressed GL formats aren't square. Not that I expect we'll ever have
to scan those out, but who knows. This would also allow drivers to let
the core check basic tiled layouts by supplying that information for a
(fourcc, modifier) pair.

The tricky part is then how you expect stride to be set, since that's
in bytes per line, which might not align. But since all the tiles I've
seen are power-of-two in height, shouldn't be an issue in practice. A
check + WARN_ON would be good though.
-Daniel

>> I think this would also fit better into the overall design where drivers
>> can overwrite the drm_format_info for specific (fourcc, modifier)
>> combinations.
>> -Daniel
>>
>>
>> >
>> > Best regards,
>> > Liviu
>> >
>> > > ---
>> > >  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 3 ++-
>> > >  include/drm/drm_gem_framebuffer_helper.h     | 5 +++++
>> > >  2 files changed, 7 insertions(+), 1 deletion(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> > > index 2810d4131411..64eddf5a1bd9 100644
>> > > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> > > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> > > @@ -57,7 +57,7 @@ struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
>> > >  }
>> > >  EXPORT_SYMBOL_GPL(drm_gem_fb_get_obj);
>> > >
>> > > -static struct drm_framebuffer *
>> > > +struct drm_framebuffer *
>> > >  drm_gem_fb_alloc(struct drm_device *dev,
>> > >            const struct drm_mode_fb_cmd2 *mode_cmd,
>> > >            struct drm_gem_object **obj, unsigned int num_planes,
>> > > @@ -85,6 +85,7 @@ drm_gem_fb_alloc(struct drm_device *dev,
>> > >
>> > >   return fb;
>> > >  }
>> > > +EXPORT_SYMBOL_GPL(drm_gem_fb_alloc);
>> > >
>> > >  /**
>> > >   * drm_gem_fb_destroy - Free GEM backed framebuffer
>> > > diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
>> > > index a38de7eb55b4..d20c1356000a 100644
>> > > --- a/include/drm/drm_gem_framebuffer_helper.h
>> > > +++ b/include/drm/drm_gem_framebuffer_helper.h
>> > > @@ -14,6 +14,11 @@ struct drm_simple_display_pipe;
>> > >
>> > >  struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
>> > >                                     unsigned int plane);
>> > > +struct drm_framebuffer *
>> > > +drm_gem_fb_alloc(struct drm_device *dev,
>> > > +          const struct drm_mode_fb_cmd2 *mode_cmd,
>> > > +          struct drm_gem_object **obj, unsigned int num_planes,
>> > > +          const struct drm_framebuffer_funcs *funcs);
>> > >  void drm_gem_fb_destroy(struct drm_framebuffer *fb);
>> > >  int drm_gem_fb_create_handle(struct drm_framebuffer *fb, struct drm_file *file,
>> > >                        unsigned int *handle);
>> > > --
>> > > 2.18.0
>> > >
>> >
>> > --
>> > ====================
>> > | I would like to |
>> > | fix the world,  |
>> > | but they're not |
>> > | giving me the   |
>> >  \ source code!  /
>> >   ---------------
>> >     ¯\_(ツ)_/¯
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>
> --
> Cheers,
> Alex G



-- 
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 2/3] drm: Make drm_gem_fb_alloc available for drivers to use

[Liviu Dudau]

On Wed, Aug 15, 2018 at 03:40:17PM +0200, Daniel Vetter wrote:
> On Wed, Aug 15, 2018 at 2:27 PM, Alexandru-Cosmin Gheorghe
> <Alexandru-Cosmin.Gheorghe@arm.com> wrote:
> > On Wed, Aug 15, 2018 at 01:54:12PM +0200, Daniel Vetter wrote:
> >> On Wed, Aug 15, 2018 at 12:08:38PM +0100, Liviu Dudau wrote:
> >> > On Thu, Jul 26, 2018 at 03:10:04PM +0100, Alexandru Gheorghe wrote:
> >> > > Some drivers can't use drm_gem_fb_create, so instead of copying the
> >> > > logic that does the framebuffer allocation allow them to use core
> >> > > drm_gem_fb_alloc.
> >> > >
> >> > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> >> >
> >> > To me it looks like an useful thing to have exported, so for what is worth:
> >> >
> >> > Acked-by: Liviu Dudau <liviu.dudau@arm.com>
> >> >
> >> > Sean, Maarten, Laurent, Gustavo, Daniel(s), do you have any objections?
> >>
> >> In general please add meaningful kernel-doc for exported functions,
> >> explaing what's different and how it works together.
> >>
> >> Wrt the specific issue, I think we should teach drm core and helpers how
> >> to allocate formats with tiled blocks. Means we need to extend
> >> drm_format_info a bit. Your YUV formats won't be the only ones where the
> >> format itself arranges pixels in blocks, and hence has format-based
> >> alignment criteria for pitch and size (due to line rounding).
> >
> > Something like a tile_size or do you have other ideas ?
> > Any idea if there are any tile formats out there where the tile it's
> > not a square ?
> 
> I think both tile_w/_h in pixels and tile_size in bytes is what you
> want for full description of all options. And yes I think some of the
> compressed GL formats aren't square. Not that I expect we'll ever have
> to scan those out, but who knows. This would also allow drivers to let
> the core check basic tiled layouts by supplying that information for a
> (fourcc, modifier) pair.
> 
> The tricky part is then how you expect stride to be set, since that's
> in bytes per line, which might not align. But since all the tiles I've
> seen are power-of-two in height, shouldn't be an issue in practice. A
> check + WARN_ON would be good though.

Another option would be to add to the drm_framebuffer_funcs structure a
hook that drivers could implement that does the validation of the fb and
could be used to bypass the more strict check in drm_gem_fb_create_fb_with_funcs()

Best regards,
Liviu

> -Daniel
> 
> >> I think this would also fit better into the overall design where drivers
> >> can overwrite the drm_format_info for specific (fourcc, modifier)
> >> combinations.
> >> -Daniel
> >>
> >>
> >> >
> >> > Best regards,
> >> > Liviu
> >> >
> >> > > ---
> >> > >  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 3 ++-
> >> > >  include/drm/drm_gem_framebuffer_helper.h     | 5 +++++
> >> > >  2 files changed, 7 insertions(+), 1 deletion(-)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> >> > > index 2810d4131411..64eddf5a1bd9 100644
> >> > > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> >> > > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> >> > > @@ -57,7 +57,7 @@ struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
> >> > >  }
> >> > >  EXPORT_SYMBOL_GPL(drm_gem_fb_get_obj);
> >> > >
> >> > > -static struct drm_framebuffer *
> >> > > +struct drm_framebuffer *
> >> > >  drm_gem_fb_alloc(struct drm_device *dev,
> >> > >            const struct drm_mode_fb_cmd2 *mode_cmd,
> >> > >            struct drm_gem_object **obj, unsigned int num_planes,
> >> > > @@ -85,6 +85,7 @@ drm_gem_fb_alloc(struct drm_device *dev,
> >> > >
> >> > >   return fb;
> >> > >  }
> >> > > +EXPORT_SYMBOL_GPL(drm_gem_fb_alloc);
> >> > >
> >> > >  /**
> >> > >   * drm_gem_fb_destroy - Free GEM backed framebuffer
> >> > > diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
> >> > > index a38de7eb55b4..d20c1356000a 100644
> >> > > --- a/include/drm/drm_gem_framebuffer_helper.h
> >> > > +++ b/include/drm/drm_gem_framebuffer_helper.h
> >> > > @@ -14,6 +14,11 @@ struct drm_simple_display_pipe;
> >> > >
> >> > >  struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
> >> > >                                     unsigned int plane);
> >> > > +struct drm_framebuffer *
> >> > > +drm_gem_fb_alloc(struct drm_device *dev,
> >> > > +          const struct drm_mode_fb_cmd2 *mode_cmd,
> >> > > +          struct drm_gem_object **obj, unsigned int num_planes,
> >> > > +          const struct drm_framebuffer_funcs *funcs);
> >> > >  void drm_gem_fb_destroy(struct drm_framebuffer *fb);
> >> > >  int drm_gem_fb_create_handle(struct drm_framebuffer *fb, struct drm_file *file,
> >> > >                        unsigned int *handle);
> >> > > --
> >> > > 2.18.0
> >> > >
> >> >
> >> > --
> >> > ====================
> >> > | I would like to |
> >> > | fix the world,  |
> >> > | but they're not |
> >> > | giving me the   |
> >> >  \ source code!  /
> >> >   ---------------
> >> >     ¯\_(ツ)_/¯
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
> >
> > --
> > Cheers,
> > Alex G
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

Re: [PATCH 2/3] drm: Make drm_gem_fb_alloc available for drivers to use

[Daniel Vetter]

On Thu, Aug 16, 2018 at 12:25 PM, Liviu Dudau <liviu.dudau@arm.com> wrote:
> On Wed, Aug 15, 2018 at 03:40:17PM +0200, Daniel Vetter wrote:
>> On Wed, Aug 15, 2018 at 2:27 PM, Alexandru-Cosmin Gheorghe
>> <Alexandru-Cosmin.Gheorghe@arm.com> wrote:
>> > On Wed, Aug 15, 2018 at 01:54:12PM +0200, Daniel Vetter wrote:
>> >> On Wed, Aug 15, 2018 at 12:08:38PM +0100, Liviu Dudau wrote:
>> >> > On Thu, Jul 26, 2018 at 03:10:04PM +0100, Alexandru Gheorghe wrote:
>> >> > > Some drivers can't use drm_gem_fb_create, so instead of copying the
>> >> > > logic that does the framebuffer allocation allow them to use core
>> >> > > drm_gem_fb_alloc.
>> >> > >
>> >> > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
>> >> >
>> >> > To me it looks like an useful thing to have exported, so for what is worth:
>> >> >
>> >> > Acked-by: Liviu Dudau <liviu.dudau@arm.com>
>> >> >
>> >> > Sean, Maarten, Laurent, Gustavo, Daniel(s), do you have any objections?
>> >>
>> >> In general please add meaningful kernel-doc for exported functions,
>> >> explaing what's different and how it works together.
>> >>
>> >> Wrt the specific issue, I think we should teach drm core and helpers how
>> >> to allocate formats with tiled blocks. Means we need to extend
>> >> drm_format_info a bit. Your YUV formats won't be the only ones where the
>> >> format itself arranges pixels in blocks, and hence has format-based
>> >> alignment criteria for pitch and size (due to line rounding).
>> >
>> > Something like a tile_size or do you have other ideas ?
>> > Any idea if there are any tile formats out there where the tile it's
>> > not a square ?
>>
>> I think both tile_w/_h in pixels and tile_size in bytes is what you
>> want for full description of all options. And yes I think some of the
>> compressed GL formats aren't square. Not that I expect we'll ever have
>> to scan those out, but who knows. This would also allow drivers to let
>> the core check basic tiled layouts by supplying that information for a
>> (fourcc, modifier) pair.
>>
>> The tricky part is then how you expect stride to be set, since that's
>> in bytes per line, which might not align. But since all the tiles I've
>> seen are power-of-two in height, shouldn't be an issue in practice. A
>> check + WARN_ON would be good though.
>
> Another option would be to add to the drm_framebuffer_funcs structure a
> hook that drivers could implement that does the validation of the fb and
> could be used to bypass the more strict check in drm_gem_fb_create_fb_with_funcs()

Can't call an fb operation if you don't yet have an fb :-)
-Daniel

>
> Best regards,
> Liviu
>
>> -Daniel
>>
>> >> I think this would also fit better into the overall design where drivers
>> >> can overwrite the drm_format_info for specific (fourcc, modifier)
>> >> combinations.
>> >> -Daniel
>> >>
>> >>
>> >> >
>> >> > Best regards,
>> >> > Liviu
>> >> >
>> >> > > ---
>> >> > >  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 3 ++-
>> >> > >  include/drm/drm_gem_framebuffer_helper.h     | 5 +++++
>> >> > >  2 files changed, 7 insertions(+), 1 deletion(-)
>> >> > >
>> >> > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> >> > > index 2810d4131411..64eddf5a1bd9 100644
>> >> > > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> >> > > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> >> > > @@ -57,7 +57,7 @@ struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
>> >> > >  }
>> >> > >  EXPORT_SYMBOL_GPL(drm_gem_fb_get_obj);
>> >> > >
>> >> > > -static struct drm_framebuffer *
>> >> > > +struct drm_framebuffer *
>> >> > >  drm_gem_fb_alloc(struct drm_device *dev,
>> >> > >            const struct drm_mode_fb_cmd2 *mode_cmd,
>> >> > >            struct drm_gem_object **obj, unsigned int num_planes,
>> >> > > @@ -85,6 +85,7 @@ drm_gem_fb_alloc(struct drm_device *dev,
>> >> > >
>> >> > >   return fb;
>> >> > >  }
>> >> > > +EXPORT_SYMBOL_GPL(drm_gem_fb_alloc);
>> >> > >
>> >> > >  /**
>> >> > >   * drm_gem_fb_destroy - Free GEM backed framebuffer
>> >> > > diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
>> >> > > index a38de7eb55b4..d20c1356000a 100644
>> >> > > --- a/include/drm/drm_gem_framebuffer_helper.h
>> >> > > +++ b/include/drm/drm_gem_framebuffer_helper.h
>> >> > > @@ -14,6 +14,11 @@ struct drm_simple_display_pipe;
>> >> > >
>> >> > >  struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
>> >> > >                                     unsigned int plane);
>> >> > > +struct drm_framebuffer *
>> >> > > +drm_gem_fb_alloc(struct drm_device *dev,
>> >> > > +          const struct drm_mode_fb_cmd2 *mode_cmd,
>> >> > > +          struct drm_gem_object **obj, unsigned int num_planes,
>> >> > > +          const struct drm_framebuffer_funcs *funcs);
>> >> > >  void drm_gem_fb_destroy(struct drm_framebuffer *fb);
>> >> > >  int drm_gem_fb_create_handle(struct drm_framebuffer *fb, struct drm_file *file,
>> >> > >                        unsigned int *handle);
>> >> > > --
>> >> > > 2.18.0
>> >> > >
>> >> >
>> >> > --
>> >> > ====================
>> >> > | I would like to |
>> >> > | fix the world,  |
>> >> > | but they're not |
>> >> > | giving me the   |
>> >> >  \ source code!  /
>> >> >   ---------------
>> >> >     ¯\_(ツ)_/¯
>> >>
>> >> --
>> >> Daniel Vetter
>> >> Software Engineer, Intel Corporation
>> >> http://blog.ffwll.ch
>> >
>> > --
>> > Cheers,
>> > Alex G
>>
>>
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯



-- 
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 2/3] drm: Make drm_gem_fb_alloc available for drivers to use

[Liviu Dudau]

On Thu, Aug 16, 2018 at 12:42:59PM +0200, Daniel Vetter wrote:
> On Thu, Aug 16, 2018 at 12:25 PM, Liviu Dudau <liviu.dudau@arm.com> wrote:
> > On Wed, Aug 15, 2018 at 03:40:17PM +0200, Daniel Vetter wrote:
> >> On Wed, Aug 15, 2018 at 2:27 PM, Alexandru-Cosmin Gheorghe
> >> <Alexandru-Cosmin.Gheorghe@arm.com> wrote:
> >> > On Wed, Aug 15, 2018 at 01:54:12PM +0200, Daniel Vetter wrote:
> >> >> On Wed, Aug 15, 2018 at 12:08:38PM +0100, Liviu Dudau wrote:
> >> >> > On Thu, Jul 26, 2018 at 03:10:04PM +0100, Alexandru Gheorghe wrote:
> >> >> > > Some drivers can't use drm_gem_fb_create, so instead of copying the
> >> >> > > logic that does the framebuffer allocation allow them to use core
> >> >> > > drm_gem_fb_alloc.
> >> >> > >
> >> >> > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> >> >> >
> >> >> > To me it looks like an useful thing to have exported, so for what is worth:
> >> >> >
> >> >> > Acked-by: Liviu Dudau <liviu.dudau@arm.com>
> >> >> >
> >> >> > Sean, Maarten, Laurent, Gustavo, Daniel(s), do you have any objections?
> >> >>
> >> >> In general please add meaningful kernel-doc for exported functions,
> >> >> explaing what's different and how it works together.
> >> >>
> >> >> Wrt the specific issue, I think we should teach drm core and helpers how
> >> >> to allocate formats with tiled blocks. Means we need to extend
> >> >> drm_format_info a bit. Your YUV formats won't be the only ones where the
> >> >> format itself arranges pixels in blocks, and hence has format-based
> >> >> alignment criteria for pitch and size (due to line rounding).
> >> >
> >> > Something like a tile_size or do you have other ideas ?
> >> > Any idea if there are any tile formats out there where the tile it's
> >> > not a square ?
> >>
> >> I think both tile_w/_h in pixels and tile_size in bytes is what you
> >> want for full description of all options. And yes I think some of the
> >> compressed GL formats aren't square. Not that I expect we'll ever have
> >> to scan those out, but who knows. This would also allow drivers to let
> >> the core check basic tiled layouts by supplying that information for a
> >> (fourcc, modifier) pair.
> >>
> >> The tricky part is then how you expect stride to be set, since that's
> >> in bytes per line, which might not align. But since all the tiles I've
> >> seen are power-of-two in height, shouldn't be an issue in practice. A
> >> check + WARN_ON would be good though.
> >
> > Another option would be to add to the drm_framebuffer_funcs structure a
> > hook that drivers could implement that does the validation of the fb and

I meant to say "does the validation of the fb creation".

> > could be used to bypass the more strict check in drm_gem_fb_create_fb_with_funcs()
> 
> Can't call an fb operation if you don't yet have an fb :-)

The funcs get passed as an argument to the _with_funcs() drm_gem_fb_create variant,
so the hook should be available. And it is a validation operation, I would say it
should be allowed even if the fb is not yet present.

Best regards,
Liviu

> -Daniel
> 
> >
> > Best regards,
> > Liviu
> >
> >> -Daniel
> >>
> >> >> I think this would also fit better into the overall design where drivers
> >> >> can overwrite the drm_format_info for specific (fourcc, modifier)
> >> >> combinations.
> >> >> -Daniel
> >> >>
> >> >>
> >> >> >
> >> >> > Best regards,
> >> >> > Liviu
> >> >> >
> >> >> > > ---
> >> >> > >  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 3 ++-
> >> >> > >  include/drm/drm_gem_framebuffer_helper.h     | 5 +++++
> >> >> > >  2 files changed, 7 insertions(+), 1 deletion(-)
> >> >> > >
> >> >> > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> >> >> > > index 2810d4131411..64eddf5a1bd9 100644
> >> >> > > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> >> >> > > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> >> >> > > @@ -57,7 +57,7 @@ struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
> >> >> > >  }
> >> >> > >  EXPORT_SYMBOL_GPL(drm_gem_fb_get_obj);
> >> >> > >
> >> >> > > -static struct drm_framebuffer *
> >> >> > > +struct drm_framebuffer *
> >> >> > >  drm_gem_fb_alloc(struct drm_device *dev,
> >> >> > >            const struct drm_mode_fb_cmd2 *mode_cmd,
> >> >> > >            struct drm_gem_object **obj, unsigned int num_planes,
> >> >> > > @@ -85,6 +85,7 @@ drm_gem_fb_alloc(struct drm_device *dev,
> >> >> > >
> >> >> > >   return fb;
> >> >> > >  }
> >> >> > > +EXPORT_SYMBOL_GPL(drm_gem_fb_alloc);
> >> >> > >
> >> >> > >  /**
> >> >> > >   * drm_gem_fb_destroy - Free GEM backed framebuffer
> >> >> > > diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
> >> >> > > index a38de7eb55b4..d20c1356000a 100644
> >> >> > > --- a/include/drm/drm_gem_framebuffer_helper.h
> >> >> > > +++ b/include/drm/drm_gem_framebuffer_helper.h
> >> >> > > @@ -14,6 +14,11 @@ struct drm_simple_display_pipe;
> >> >> > >
> >> >> > >  struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
> >> >> > >                                     unsigned int plane);
> >> >> > > +struct drm_framebuffer *
> >> >> > > +drm_gem_fb_alloc(struct drm_device *dev,
> >> >> > > +          const struct drm_mode_fb_cmd2 *mode_cmd,
> >> >> > > +          struct drm_gem_object **obj, unsigned int num_planes,
> >> >> > > +          const struct drm_framebuffer_funcs *funcs);
> >> >> > >  void drm_gem_fb_destroy(struct drm_framebuffer *fb);
> >> >> > >  int drm_gem_fb_create_handle(struct drm_framebuffer *fb, struct drm_file *file,
> >> >> > >                        unsigned int *handle);
> >> >> > > --
> >> >> > > 2.18.0
> >> >> > >
> >> >> >
> >> >> > --
> >> >> > ====================
> >> >> > | I would like to |
> >> >> > | fix the world,  |
> >> >> > | but they're not |
> >> >> > | giving me the   |
> >> >> >  \ source code!  /
> >> >> >   ---------------
> >> >> >     ¯\_(ツ)_/¯
> >> >>
> >> >> --
> >> >> Daniel Vetter
> >> >> Software Engineer, Intel Corporation
> >> >> http://blog.ffwll.ch
> >> >
> >> > --
> >> > Cheers,
> >> > Alex G
> >>
> >>
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >
> > --
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ¯\_(ツ)_/¯
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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