* [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3.
@ 2019-04-05 12:52 Maarten Lankhorst
2019-04-05 12:52 ` [igt-dev] [PATCH i-g-t 2/2] tests/kms_chamelium: Fix *-cmp-random and *-crc-random tests, v3 Maarten Lankhorst
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Maarten Lankhorst @ 2019-04-05 12:52 UTC (permalink / raw)
To: igt-dev
Ever since commit 3fa65f4b532bd9a5b ("fb: Add support for conversions
through pixman") we can generate a valid cairo surface for any plane,
use this to avoid having to implement our own conversion routine.
Instead of duplicating this functionality in igt_fb_convert_with_stride,
we can simply convert this to a few cairo calls, because we now support
cairo calls to any of the supported framebuffer formats.
This is required to make this function more generic, and convert from any
format/modifier to any other format/modifier.
Changes since v1:
- Return fb_id in the cairo case.
Changes since v2:
- Remove the manual conversion fallback.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
lib/igt_fb.c | 59 +++++++++++-----------------------------------------
1 file changed, 12 insertions(+), 47 deletions(-)
diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 6adf422228e8..b7b78c46b40f 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -2971,58 +2971,23 @@ unsigned int igt_fb_convert_with_stride(struct igt_fb *dst, struct igt_fb *src,
uint64_t dst_modifier,
unsigned int dst_stride)
{
- struct fb_convert cvt = { };
- struct igt_fb linear;
- void *dst_ptr, *src_ptr;
- uint64_t base_modifier;
+ /* Use the cairo api to convert */
+ cairo_surface_t *surf = igt_get_cairo_surface(src->fd, src);
+ cairo_t *cr;
int fb_id;
- if (is_vc4_device(src->fd))
- base_modifier = fourcc_mod_broadcom_mod(dst_modifier);
- else
- base_modifier = dst_modifier;
-
- fb_id = igt_create_fb_with_bo_size(src->fd, src->width, src->height,
- dst_fourcc,
- LOCAL_DRM_FORMAT_MOD_NONE, &linear,
- 0, dst_stride);
+ fb_id = igt_create_fb_with_bo_size(src->fd, src->width,
+ src->height, dst_fourcc,
+ dst_modifier, dst, 0,
+ dst_stride);
igt_assert(fb_id > 0);
- src_ptr = igt_fb_map_buffer(src->fd, src);
- igt_assert(src_ptr);
-
- dst_ptr = igt_fb_map_buffer(linear.fd, &linear);
- igt_assert(dst_ptr);
-
- cvt.dst.ptr = dst_ptr;
- cvt.dst.fb = &linear;
- cvt.src.ptr = src_ptr;
- cvt.src.fb = src;
- fb_convert(&cvt);
-
- igt_fb_unmap_buffer(dst, dst_ptr);
- igt_fb_unmap_buffer(src, src_ptr);
-
- switch (base_modifier) {
- case DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED:
- fb_id = igt_vc4_fb_t_tiled_convert(dst, &linear);
- break;
- case DRM_FORMAT_MOD_BROADCOM_SAND32:
- case DRM_FORMAT_MOD_BROADCOM_SAND64:
- case DRM_FORMAT_MOD_BROADCOM_SAND128:
- case DRM_FORMAT_MOD_BROADCOM_SAND256:
- fb_id = vc4_fb_sand_tiled_convert(dst, &linear, dst_modifier);
- break;
- default:
- igt_assert(dst_modifier == LOCAL_DRM_FORMAT_MOD_NONE);
- }
-
- igt_assert(fb_id > 0);
+ cr = igt_get_cairo_ctx(dst->fd, dst);
+ cairo_set_source_surface(cr, surf, 0, 0);
+ cairo_paint(cr);
+ igt_put_cairo_ctx(dst->fd, dst, cr);
- if (dst_modifier == LOCAL_DRM_FORMAT_MOD_NONE)
- *dst = linear;
- else
- igt_remove_fb(linear.fd, &linear);
+ cairo_surface_destroy(surf);
return fb_id;
}
--
2.20.1
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [igt-dev] [PATCH i-g-t 2/2] tests/kms_chamelium: Fix *-cmp-random and *-crc-random tests, v3.
2019-04-05 12:52 [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3 Maarten Lankhorst
@ 2019-04-05 12:52 ` Maarten Lankhorst
2019-05-07 10:42 ` [igt-dev] [i-g-t, " Ser, Simon
2019-04-05 13:54 ` [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3 Paul Kocialkowski
` (2 subsequent siblings)
3 siblings, 1 reply; 17+ messages in thread
From: Maarten Lankhorst @ 2019-04-05 12:52 UTC (permalink / raw)
To: igt-dev
The random tests allowed potentially invalid things:
- 1x1 fb's to be created. Force a minimum of 32 on YUV so a scaled
planar format plane will be at least 16x16. This will fix
scaled/planar format support in i915 and avoid a div by zero when
calculating a value modulo h/2 and w/2.
- Downscaling to any amount, restrict it to 2x to make the test pass.
- Some hw may not allow scaling, in those cases we should fallback
to no scaling at all.
- Attempting to configure a minimum of 4 planes, instead of a maximum.
This fails with a null pointer deref if the hw doesn't have 4
configurable overlay planes.
Changes since v1:
- Enforce a minimum displayed size of 16x16 for intel only,
otherwise it's 1x1.
- Fix comments.
Changes since v2:
- Fix comments harder.
- Pick a random format and its modifier from the plane itself,
instead of using igt_format_array_fill(). This will cause the test
to use all valid combinations of modifiers and formats.
- Set minimum dimension to 8 for !yuv, and 16 for YUV.
- Generate format / modifier before
Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
tests/kms_chamelium.c | 318 +++++++++++++++++++++++++-----------------
1 file changed, 193 insertions(+), 125 deletions(-)
diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
index 2dc1049d2dda..a55bd86c77f5 100644
--- a/tests/kms_chamelium.c
+++ b/tests/kms_chamelium.c
@@ -710,17 +710,48 @@ test_display_frame_dump(data_t *data, struct chamelium_port *port)
drmModeFreeConnector(connector);
}
-static void select_tiled_modifier(igt_plane_t *plane, uint32_t width,
+
+static void randomize_plane_stride(data_t *data,
+ uint32_t width, uint32_t height,
+ uint32_t format, uint64_t modifier,
+ size_t *stride)
+{
+ size_t stride_min;
+ uint32_t max_tile_w = 4, tile_w, tile_h;
+ int i;
+ struct igt_fb dummy;
+
+ stride_min = width * igt_format_plane_bpp(format, 0) / 8;
+
+ /* Randomize the stride to less than twice the minimum. */
+ *stride = (rand() % stride_min) + stride_min;
+
+ /*
+ * Create a dummy FB to determine bpp for each plane, and calculate
+ * the maximum tile width from that.
+ */
+ igt_create_fb(data->drm_fd, 64, 64, format, modifier, &dummy);
+ for (i = 0; i < dummy.num_planes; i++) {
+ igt_get_fb_tile_size(data->drm_fd, modifier, dummy.plane_bpp[i], &tile_w, &tile_h);
+
+ if (tile_w > max_tile_w)
+ max_tile_w = tile_w;
+ }
+ igt_remove_fb(data->drm_fd, &dummy);
+
+ /*
+ * Pixman requires the stride to be aligned to 32-bits, which is
+ * reflected in the initial value of max_tile_w and the hw
+ * may require a multiple of tile width, choose biggest of the 2.
+ */
+ *stride = ALIGN(*stride, max_tile_w);
+}
+
+static void update_tiled_modifier(igt_plane_t *plane, uint32_t width,
uint32_t height, uint32_t format,
uint64_t *modifier)
{
- if (igt_plane_has_format_mod(plane, format,
- DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED)) {
- igt_debug("Selecting VC4 T-tiling\n");
-
- *modifier = DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED;
- } else if (igt_plane_has_format_mod(plane, format,
- DRM_FORMAT_MOD_BROADCOM_SAND256)) {
+ if (*modifier == DRM_FORMAT_MOD_BROADCOM_SAND256) {
/* Randomize the column height to less than twice the minimum. */
size_t column_height = (rand() % height) + height;
@@ -728,90 +759,84 @@ static void select_tiled_modifier(igt_plane_t *plane, uint32_t width,
column_height);
*modifier = DRM_FORMAT_MOD_BROADCOM_SAND256_COL_HEIGHT(column_height);
- } else {
- *modifier = DRM_FORMAT_MOD_LINEAR;
}
}
-static void randomize_plane_format_stride(igt_plane_t *plane,
- uint32_t width, uint32_t height,
- uint32_t *format, uint64_t *modifier,
- size_t *stride, bool allow_yuv)
+static void randomize_plane_setup(data_t *data, igt_plane_t *plane,
+ drmModeModeInfo *mode,
+ uint32_t *width, uint32_t *height,
+ uint32_t *format, uint64_t *modifier,
+ bool allow_yuv)
{
- size_t stride_min;
- uint32_t *formats_array;
- unsigned int formats_count;
+ int min_dim;
+ uint32_t idx[plane->format_mod_count];
unsigned int count = 0;
unsigned int i;
- bool tiled;
- int index;
-
- igt_format_array_fill(&formats_array, &formats_count, allow_yuv);
/* First pass to count the supported formats. */
- for (i = 0; i < formats_count; i++)
- if (igt_plane_has_format_mod(plane, formats_array[i],
- DRM_FORMAT_MOD_LINEAR))
- count++;
+ for (i = 0; i < plane->format_mod_count; i++)
+ if (igt_fb_supported_format(plane->formats[i]) &&
+ (allow_yuv || !igt_format_is_yuv(plane->formats[i])))
+ idx[count++] = i;
igt_assert(count > 0);
- index = rand() % count;
-
- /* Second pass to get the index-th supported format. */
- for (i = 0; i < formats_count; i++) {
- if (!igt_plane_has_format_mod(plane, formats_array[i],
- DRM_FORMAT_MOD_LINEAR))
- continue;
-
- if (!index--) {
- *format = formats_array[i];
- break;
- }
- }
-
- free(formats_array);
+ i = idx[rand() % count];
+ *format = plane->formats[i];
+ *modifier = plane->modifiers[i];
- igt_assert(index < 0);
+ update_tiled_modifier(plane, *width, *height, *format, modifier);
- stride_min = width * igt_format_plane_bpp(*format, 0) / 8;
+ /*
+ * Randomize width and height in the mode dimensions range.
+ *
+ * Restrict to a min of 2 * min_dim, this way src_w/h are always at
+ * least min_dim, because src_w = width - (rand % w / 2).
+ *
+ * Use a minimum dimension of 16 for YUV, because planar YUV
+ * subsamples the UV plane.
+ */
+ min_dim = igt_format_is_yuv(*format) ? 16 : 8;
- /* Randomize the stride to less than twice the minimum. */
- *stride = (rand() % stride_min) + stride_min;
+ *width = max((rand() % mode->hdisplay) + 1, 2 * min_dim);
+ *height = max((rand() % mode->vdisplay) + 1, 2 * min_dim);
+}
- /* Pixman requires the stride to be aligned to 32-byte words. */
- *stride = ALIGN(*stride, sizeof(uint32_t));
+static void configure_plane(igt_plane_t *plane, uint32_t src_w, uint32_t src_h,
+ uint32_t src_x, uint32_t src_y, uint32_t crtc_w,
+ uint32_t crtc_h, int32_t crtc_x, int32_t crtc_y,
+ struct igt_fb *fb)
+{
+ igt_plane_set_fb(plane, fb);
- /* Randomize the use of a tiled mode with a 1/4 probability. */
- tiled = ((rand() % 4) == 0);
+ igt_plane_set_position(plane, crtc_x, crtc_y);
+ igt_plane_set_size(plane, crtc_w, crtc_h);
- if (tiled)
- select_tiled_modifier(plane, width, height, *format, modifier);
- else
- *modifier = DRM_FORMAT_MOD_LINEAR;
+ igt_fb_set_position(fb, plane, src_x, src_y);
+ igt_fb_set_size(fb, plane, src_w, src_h);
}
-static void randomize_plane_dimensions(drmModeModeInfo *mode,
- uint32_t *width, uint32_t *height,
- uint32_t *src_w, uint32_t *src_h,
- uint32_t *src_x, uint32_t *src_y,
- uint32_t *crtc_w, uint32_t *crtc_h,
- int32_t *crtc_x, int32_t *crtc_y,
- bool allow_scaling)
+static void randomize_plane_coordinates(data_t *data, igt_plane_t *plane,
+ drmModeModeInfo *mode,
+ struct igt_fb *fb,
+ uint32_t *src_w, uint32_t *src_h,
+ uint32_t *src_x, uint32_t *src_y,
+ uint32_t *crtc_w, uint32_t *crtc_h,
+ int32_t *crtc_x, int32_t *crtc_y,
+ bool allow_scaling)
{
+ bool is_yuv = igt_format_is_yuv(fb->drm_format);
+ uint32_t width = fb->width, height = fb->height;
double ratio;
-
- /* Randomize width and height in the mode dimensions range. */
- *width = (rand() % mode->hdisplay) + 1;
- *height = (rand() % mode->vdisplay) + 1;
+ int ret;
/* Randomize source offset in the first half of the original size. */
- *src_x = rand() % (*width / 2);
- *src_y = rand() % (*height / 2);
+ *src_x = rand() % (width / 2);
+ *src_y = rand() % (height / 2);
/* The source size only includes the active source area. */
- *src_w = *width - *src_x;
- *src_h = *height - *src_y;
+ *src_w = width - *src_x;
+ *src_h = height - *src_y;
if (allow_scaling) {
*crtc_w = (rand() % mode->hdisplay) + 1;
@@ -821,17 +846,22 @@ static void randomize_plane_dimensions(drmModeModeInfo *mode,
* Don't bother with scaling if dimensions are quite close in
* order to get non-scaling cases more frequently. Also limit
* scaling to 3x to avoid agressive filtering that makes
- * comparison less reliable.
+ * comparison less reliable, and don't go above 2x downsampling
+ * to avoid possible hw limitations.
*/
ratio = ((double) *crtc_w / *src_w);
- if (ratio > 0.8 && ratio < 1.2)
+ if (ratio < 0.5)
+ *src_w = *crtc_w * 2;
+ else if (ratio > 0.8 && ratio < 1.2)
*crtc_w = *src_w;
else if (ratio > 3.0)
*crtc_w = *src_w * 3;
ratio = ((double) *crtc_h / *src_h);
- if (ratio > 0.8 && ratio < 1.2)
+ if (ratio < 0.5)
+ *src_h = *crtc_h * 2;
+ else if (ratio > 0.8 && ratio < 1.2)
*crtc_h = *src_h;
else if (ratio > 3.0)
*crtc_h = *src_h * 3;
@@ -846,8 +876,15 @@ static void randomize_plane_dimensions(drmModeModeInfo *mode,
* scaled clipping may result in decimal dimensions, that most
* drivers don't support.
*/
- *crtc_x = rand() % (mode->hdisplay - *crtc_w);
- *crtc_y = rand() % (mode->vdisplay - *crtc_h);
+ if (*crtc_w < mode->hdisplay)
+ *crtc_x = rand() % (mode->hdisplay - *crtc_w);
+ else
+ *crtc_x = 0;
+
+ if (*crtc_h < mode->vdisplay)
+ *crtc_y = rand() % (mode->vdisplay - *crtc_h);
+ else
+ *crtc_y = 0;
} else {
/*
* Randomize the on-crtc position and allow the plane to go
@@ -856,6 +893,62 @@ static void randomize_plane_dimensions(drmModeModeInfo *mode,
*crtc_x = (rand() % mode->hdisplay) - *crtc_w / 2;
*crtc_y = (rand() % mode->vdisplay) - *crtc_h / 2;
}
+
+ configure_plane(plane, *src_w, *src_h, *src_x, *src_y,
+ *crtc_w, *crtc_h, *crtc_x, *crtc_y, fb);
+ ret = igt_display_try_commit_atomic(&data->display,
+ DRM_MODE_ATOMIC_TEST_ONLY |
+ DRM_MODE_ATOMIC_ALLOW_MODESET,
+ NULL);
+ if (!ret)
+ return;
+
+ /* Coordinates are logged in the dumped debug log, so only report w/h on failure here. */
+ igt_assert_f(ret != -ENOSPC,"Failure in testcase, invalid coordinates on a %ux%u fb\n", width, height);
+
+ /* Make YUV coordinates a multiple of 2 and retry the math. */
+ if (is_yuv) {
+ *src_x &= ~1;
+ *src_y &= ~1;
+ *src_w &= ~1;
+ *src_h &= ~1;
+ /* To handle 1:1 scaling, clear crtc_w/h too. */
+ *crtc_w &= ~1;
+ *crtc_h &= ~1;
+
+ if (*crtc_x < 0 && (*crtc_x & 1))
+ (*crtc_x)++;
+ else
+ *crtc_x &= ~1;
+
+ /* If negative, round up to 0 instead of down */
+ if (*crtc_y < 0 && (*crtc_y & 1))
+ (*crtc_y)++;
+ else
+ *crtc_y &= ~1;
+
+ configure_plane(plane, *src_w, *src_h, *src_x, *src_y, *crtc_w,
+ *crtc_h, *crtc_x, *crtc_y, fb);
+ ret = igt_display_try_commit_atomic(&data->display,
+ DRM_MODE_ATOMIC_TEST_ONLY |
+ DRM_MODE_ATOMIC_ALLOW_MODESET,
+ NULL);
+ if (!ret)
+ return;
+ }
+
+ igt_assert(!ret || allow_scaling);
+ igt_info("Scaling ratio %g / %g failed, trying without scaling.\n",
+ ((double) *crtc_w / *src_w), ((double) *crtc_h / *src_h));
+
+ *crtc_w = *src_w;
+ *crtc_h = *src_h;
+
+ configure_plane(plane, *src_w, *src_h, *src_x, *src_y, *crtc_w,
+ *crtc_h, *crtc_x, *crtc_y, fb);
+ igt_display_commit_atomic(&data->display,
+ DRM_MODE_ATOMIC_TEST_ONLY |
+ DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
}
static void blit_plane_cairo(data_t *data, cairo_surface_t *result,
@@ -914,20 +1007,6 @@ static void blit_plane_cairo(data_t *data, cairo_surface_t *result,
cairo_destroy(cr);
}
-static void configure_plane(igt_plane_t *plane, uint32_t src_w, uint32_t src_h,
- uint32_t src_x, uint32_t src_y, uint32_t crtc_w,
- uint32_t crtc_h, int32_t crtc_x, int32_t crtc_y,
- struct igt_fb *fb)
-{
- igt_plane_set_fb(plane, fb);
-
- igt_plane_set_position(plane, crtc_x, crtc_y);
- igt_plane_set_size(plane, crtc_w, crtc_h);
-
- igt_fb_set_position(fb, plane, src_x, src_y);
- igt_fb_set_size(fb, plane, src_w, src_h);
-}
-
static void prepare_randomized_plane(data_t *data,
drmModeModeInfo *mode,
igt_plane_t *plane,
@@ -948,51 +1027,49 @@ static void prepare_randomized_plane(data_t *data,
bool tiled;
int fb_id;
- randomize_plane_dimensions(mode, &overlay_fb_w, &overlay_fb_h,
- &overlay_src_w, &overlay_src_h,
- &overlay_src_x, &overlay_src_y,
- &overlay_crtc_w, &overlay_crtc_h,
- &overlay_crtc_x, &overlay_crtc_y,
- allow_scaling);
+ randomize_plane_setup(data, plane, mode, &overlay_fb_w, &overlay_fb_h,
+ &format, &modifier, allow_yuv);
- igt_debug("Plane %d: framebuffer size %dx%d\n", index,
- overlay_fb_w, overlay_fb_h);
- igt_debug("Plane %d: on-crtc size %dx%d\n", index,
- overlay_crtc_w, overlay_crtc_h);
- igt_debug("Plane %d: on-crtc position %dx%d\n", index,
- overlay_crtc_x, overlay_crtc_y);
- igt_debug("Plane %d: in-framebuffer size %dx%d\n", index,
- overlay_src_w, overlay_src_h);
- igt_debug("Plane %d: in-framebuffer position %dx%d\n", index,
- overlay_src_x, overlay_src_y);
+ tiled = (modifier != LOCAL_DRM_FORMAT_MOD_NONE);
+ igt_debug("Plane %d: framebuffer size %dx%d %s format (%s)\n",
+ index, overlay_fb_w, overlay_fb_h,
+ igt_format_str(format), tiled ? "tiled" : "linear");
/* Get a pattern framebuffer for the overlay plane. */
fb_id = chamelium_get_pattern_fb(data, overlay_fb_w, overlay_fb_h,
DRM_FORMAT_XRGB8888, 32, &pattern_fb);
igt_assert(fb_id > 0);
- randomize_plane_format_stride(plane, overlay_fb_w, overlay_fb_h,
- &format, &modifier, &stride, allow_yuv);
-
- tiled = (modifier != LOCAL_DRM_FORMAT_MOD_NONE);
+ randomize_plane_stride(data, overlay_fb_w, overlay_fb_h,
+ format, modifier, &stride);
- igt_debug("Plane %d: %s format (%s) with stride %ld\n", index,
- igt_format_str(format), tiled ? "tiled" : "linear", stride);
+ igt_debug("Plane %d: stride %ld\n", index, stride);
fb_id = igt_fb_convert_with_stride(overlay_fb, &pattern_fb, format,
modifier, stride);
igt_assert(fb_id > 0);
+ randomize_plane_coordinates(data, plane, mode, overlay_fb,
+ &overlay_src_w, &overlay_src_h,
+ &overlay_src_x, &overlay_src_y,
+ &overlay_crtc_w, &overlay_crtc_h,
+ &overlay_crtc_x, &overlay_crtc_y,
+ allow_scaling);
+
+ igt_debug("Plane %d: in-framebuffer size %dx%d\n", index,
+ overlay_src_w, overlay_src_h);
+ igt_debug("Plane %d: in-framebuffer position %dx%d\n", index,
+ overlay_src_x, overlay_src_y);
+ igt_debug("Plane %d: on-crtc size %dx%d\n", index,
+ overlay_crtc_w, overlay_crtc_h);
+ igt_debug("Plane %d: on-crtc position %dx%d\n", index,
+ overlay_crtc_x, overlay_crtc_y);
+
blit_plane_cairo(data, result_surface, overlay_src_w, overlay_src_h,
overlay_src_x, overlay_src_y,
overlay_crtc_w, overlay_crtc_h,
overlay_crtc_x, overlay_crtc_y, &pattern_fb);
- configure_plane(plane, overlay_src_w, overlay_src_h,
- overlay_src_x, overlay_src_y,
- overlay_crtc_w, overlay_crtc_h,
- overlay_crtc_x, overlay_crtc_y, overlay_fb);
-
/* Remove the original pattern framebuffer. */
igt_remove_fb(data->drm_fd, &pattern_fb);
}
@@ -1068,7 +1145,7 @@ static void test_display_planes_random(data_t *data,
igt_output_count_plane_type(output, DRM_PLANE_TYPE_OVERLAY);
/* Limit the number of planes to a reasonable scene. */
- overlay_planes_max = max(overlay_planes_max, 4);
+ overlay_planes_max = min(overlay_planes_max, 4);
overlay_planes_count = (rand() % overlay_planes_max) + 1;
igt_debug("Using %d overlay planes\n", overlay_planes_count);
@@ -1121,17 +1198,8 @@ static void test_display_planes_random(data_t *data,
chamelium_destroy_frame_dump(dump);
}
- for (i = 0; i < overlay_planes_count; i++) {
- struct igt_fb *overlay_fb = &overlay_fbs[i];
- igt_plane_t *plane;
-
- plane = igt_output_get_plane_type_index(output,
- DRM_PLANE_TYPE_OVERLAY,
- i);
- igt_assert(plane);
-
- igt_remove_fb(data->drm_fd, overlay_fb);
- }
+ for (i = 0; i < overlay_planes_count; i++)
+ igt_remove_fb(data->drm_fd, &overlay_fbs[i]);
free(overlay_fbs);
--
2.20.1
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3.
2019-04-05 12:52 [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3 Maarten Lankhorst
2019-04-05 12:52 ` [igt-dev] [PATCH i-g-t 2/2] tests/kms_chamelium: Fix *-cmp-random and *-crc-random tests, v3 Maarten Lankhorst
@ 2019-04-05 13:54 ` Paul Kocialkowski
2019-04-05 15:38 ` Maarten Lankhorst
2019-04-05 14:08 ` [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/2] " Patchwork
2019-04-06 11:48 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
3 siblings, 1 reply; 17+ messages in thread
From: Paul Kocialkowski @ 2019-04-05 13:54 UTC (permalink / raw)
To: Maarten Lankhorst, igt-dev
Hi,
Le vendredi 05 avril 2019 à 14:52 +0200, Maarten Lankhorst a écrit :
> Ever since commit 3fa65f4b532bd9a5b ("fb: Add support for conversions
> through pixman") we can generate a valid cairo surface for any plane,
> use this to avoid having to implement our own conversion routine.
>
> Instead of duplicating this functionality in igt_fb_convert_with_stride,
> we can simply convert this to a few cairo calls, because we now support
> cairo calls to any of the supported framebuffer formats.
I don't think this is the case: cairo *only* takes linear buffers, so
we need the explicit tiling conversion step after having converted to a
new format with fb_convert. I don't see how it could work otherwise.
Note that in igt, we're only interested in converting from linear to
tiled, not the other way round (for now, at least), so that's the
pipeline that the helper asssumes.
Cheers,
Paul
> This is required to make this function more generic, and convert from any
> format/modifier to any other format/modifier.
>
> Changes since v1:
> - Return fb_id in the cairo case.
> Changes since v2:
> - Remove the manual conversion fallback.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
> lib/igt_fb.c | 59 +++++++++++-----------------------------------------
> 1 file changed, 12 insertions(+), 47 deletions(-)
>
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index 6adf422228e8..b7b78c46b40f 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -2971,58 +2971,23 @@ unsigned int igt_fb_convert_with_stride(struct igt_fb *dst, struct igt_fb *src,
> uint64_t dst_modifier,
> unsigned int dst_stride)
> {
> - struct fb_convert cvt = { };
> - struct igt_fb linear;
> - void *dst_ptr, *src_ptr;
> - uint64_t base_modifier;
> + /* Use the cairo api to convert */
> + cairo_surface_t *surf = igt_get_cairo_surface(src->fd, src);
> + cairo_t *cr;
> int fb_id;
>
> - if (is_vc4_device(src->fd))
> - base_modifier = fourcc_mod_broadcom_mod(dst_modifier);
> - else
> - base_modifier = dst_modifier;
> -
> - fb_id = igt_create_fb_with_bo_size(src->fd, src->width, src->height,
> - dst_fourcc,
> - LOCAL_DRM_FORMAT_MOD_NONE, &linear,
> - 0, dst_stride);
> + fb_id = igt_create_fb_with_bo_size(src->fd, src->width,
> + src->height, dst_fourcc,
> + dst_modifier, dst, 0,
> + dst_stride);
> igt_assert(fb_id > 0);
>
> - src_ptr = igt_fb_map_buffer(src->fd, src);
> - igt_assert(src_ptr);
> -
> - dst_ptr = igt_fb_map_buffer(linear.fd, &linear);
> - igt_assert(dst_ptr);
> -
> - cvt.dst.ptr = dst_ptr;
> - cvt.dst.fb = &linear;
> - cvt.src.ptr = src_ptr;
> - cvt.src.fb = src;
> - fb_convert(&cvt);
> -
> - igt_fb_unmap_buffer(dst, dst_ptr);
> - igt_fb_unmap_buffer(src, src_ptr);
> -
> - switch (base_modifier) {
> - case DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED:
> - fb_id = igt_vc4_fb_t_tiled_convert(dst, &linear);
> - break;
> - case DRM_FORMAT_MOD_BROADCOM_SAND32:
> - case DRM_FORMAT_MOD_BROADCOM_SAND64:
> - case DRM_FORMAT_MOD_BROADCOM_SAND128:
> - case DRM_FORMAT_MOD_BROADCOM_SAND256:
> - fb_id = vc4_fb_sand_tiled_convert(dst, &linear, dst_modifier);
> - break;
> - default:
> - igt_assert(dst_modifier == LOCAL_DRM_FORMAT_MOD_NONE);
> - }
> -
> - igt_assert(fb_id > 0);
> + cr = igt_get_cairo_ctx(dst->fd, dst);
> + cairo_set_source_surface(cr, surf, 0, 0);
> + cairo_paint(cr);
> + igt_put_cairo_ctx(dst->fd, dst, cr);
>
> - if (dst_modifier == LOCAL_DRM_FORMAT_MOD_NONE)
> - *dst = linear;
> - else
> - igt_remove_fb(linear.fd, &linear);
> + cairo_surface_destroy(surf);
>
> return fb_id;
> }
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 17+ messages in thread
* [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3.
2019-04-05 12:52 [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3 Maarten Lankhorst
2019-04-05 12:52 ` [igt-dev] [PATCH i-g-t 2/2] tests/kms_chamelium: Fix *-cmp-random and *-crc-random tests, v3 Maarten Lankhorst
2019-04-05 13:54 ` [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3 Paul Kocialkowski
@ 2019-04-05 14:08 ` Patchwork
2019-04-06 11:48 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
3 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2019-04-05 14:08 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: igt-dev
== Series Details ==
Series: series starting with [i-g-t,1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3.
URL : https://patchwork.freedesktop.org/series/59062/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_5880 -> IGTPW_2800
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/59062/revisions/1/mbox/
Known issues
------------
Here are the changes found in IGTPW_2800 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@amdgpu/amd_basic@cs-compute:
- fi-kbl-8809g: NOTRUN -> FAIL [fdo#108094]
* igt@amdgpu/amd_basic@query-info:
- fi-bsw-kefka: NOTRUN -> SKIP [fdo#109271] +55
* igt@amdgpu/amd_basic@semaphore:
- fi-kbl-7500u: NOTRUN -> SKIP [fdo#109271] +28
* igt@i915_selftest@live_execlists:
- fi-apl-guc: PASS -> INCOMPLETE [fdo#103927] / [fdo#109720]
* igt@kms_busy@basic-flip-a:
- fi-bsw-n3050: NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +1
* igt@kms_busy@basic-flip-c:
- fi-bsw-kefka: NOTRUN -> SKIP [fdo#109271] / [fdo#109278]
* igt@kms_chamelium@common-hpd-after-suspend:
- fi-kbl-7500u: NOTRUN -> DMESG-WARN [fdo#102505] / [fdo#103558] / [fdo#105079] / [fdo#105602]
* igt@kms_chamelium@hdmi-crc-fast:
- fi-bsw-n3050: NOTRUN -> SKIP [fdo#109271] +62
* igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
- fi-byt-clapper: PASS -> FAIL [fdo#103191] / [fdo#107362]
- fi-blb-e6850: PASS -> INCOMPLETE [fdo#107718]
* igt@kms_psr@primary_page_flip:
- fi-skl-lmem: NOTRUN -> SKIP [fdo#109271] +37
* igt@runner@aborted:
- fi-apl-guc: NOTRUN -> FAIL [fdo#108622] / [fdo#109720]
#### Possible fixes ####
* igt@amdgpu/amd_basic@userptr:
- fi-kbl-8809g: DMESG-WARN [fdo#108965] -> PASS
* igt@i915_pm_rpm@module-reload:
- fi-skl-6770hq: FAIL [fdo#108511] -> PASS
* igt@kms_chamelium@dp-crc-fast:
- fi-kbl-7500u: DMESG-WARN [fdo#103841] -> PASS
* igt@kms_frontbuffer_tracking@basic:
- fi-icl-u2: FAIL [fdo#103167] -> PASS
* igt@kms_pipe_crc_basic@hang-read-crc-pipe-a:
- fi-byt-clapper: FAIL [fdo#103191] / [fdo#107362] -> PASS +1
[fdo#102505]: https://bugs.freedesktop.org/show_bug.cgi?id=102505
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
[fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
[fdo#103841]: https://bugs.freedesktop.org/show_bug.cgi?id=103841
[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#105079]: https://bugs.freedesktop.org/show_bug.cgi?id=105079
[fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
[fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
[fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
[fdo#108094]: https://bugs.freedesktop.org/show_bug.cgi?id=108094
[fdo#108511]: https://bugs.freedesktop.org/show_bug.cgi?id=108511
[fdo#108622]: https://bugs.freedesktop.org/show_bug.cgi?id=108622
[fdo#108965]: https://bugs.freedesktop.org/show_bug.cgi?id=108965
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
[fdo#109720]: https://bugs.freedesktop.org/show_bug.cgi?id=109720
Participating hosts (46 -> 43)
------------------------------
Additional (3): fi-bsw-kefka fi-skl-lmem fi-bsw-n3050
Missing (6): fi-kbl-soraka fi-ilk-m540 fi-byt-j1900 fi-bsw-cyan fi-pnv-d510 fi-bdw-samus
Build changes
-------------
* IGT: IGT_4931 -> IGTPW_2800
CI_DRM_5880: 10ec600ac87dc6da25d2784720d8fd029033d3ef @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_2800: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2800/
IGT_4931: 019f892e5d1a0a9643cb726c47ce2d99c14b444f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2800/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3.
2019-04-05 13:54 ` [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3 Paul Kocialkowski
@ 2019-04-05 15:38 ` Maarten Lankhorst
2019-04-08 8:50 ` Maxime Ripard
0 siblings, 1 reply; 17+ messages in thread
From: Maarten Lankhorst @ 2019-04-05 15:38 UTC (permalink / raw)
To: Paul Kocialkowski, igt-dev
Op 05-04-2019 om 15:54 schreef Paul Kocialkowski:
> Hi,
>
> Le vendredi 05 avril 2019 à 14:52 +0200, Maarten Lankhorst a écrit :
>> Ever since commit 3fa65f4b532bd9a5b ("fb: Add support for conversions
>> through pixman") we can generate a valid cairo surface for any plane,
>> use this to avoid having to implement our own conversion routine.
>>
>> Instead of duplicating this functionality in igt_fb_convert_with_stride,
>> we can simply convert this to a few cairo calls, because we now support
>> cairo calls to any of the supported framebuffer formats.
> I don't think this is the case: cairo *only* takes linear buffers, so
> we need the explicit tiling conversion step after having converted to a
> new format with fb_convert. I don't see how it could work otherwise.
>
> Note that in igt, we're only interested in converting from linear to
> tiled, not the other way round (for now, at least), so that's the
> pipeline that the helper asssumes.
Hey,
We have code to handle this exact conversion in i-g-t already, both ways, see igt_get_cairo_surface(). :)
You should be able to extend create_cairo_surface__convert() to handle broadcom tiling as well,
I didn't see a function to convert from tiled to untiled, else I would have plugged it in for you.
~Maarten
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 17+ messages in thread
* [igt-dev] ✓ Fi.CI.IGT: success for series starting with [i-g-t,1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3.
2019-04-05 12:52 [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3 Maarten Lankhorst
` (2 preceding siblings ...)
2019-04-05 14:08 ` [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/2] " Patchwork
@ 2019-04-06 11:48 ` Patchwork
3 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2019-04-06 11:48 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: igt-dev
== Series Details ==
Series: series starting with [i-g-t,1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3.
URL : https://patchwork.freedesktop.org/series/59062/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_5880_full -> IGTPW_2800_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/59062/revisions/1/mbox/
Known issues
------------
Here are the changes found in IGTPW_2800_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_pwrite@stolen-snoop:
- shard-glk: NOTRUN -> SKIP [fdo#109271] +18
* igt@gem_tiled_swapping@non-threaded:
- shard-glk: PASS -> SKIP [fdo#109271]
- shard-apl: PASS -> SKIP [fdo#109271]
* igt@kms_atomic_transition@plane-all-modeset-transition:
- shard-kbl: PASS -> INCOMPLETE [fdo#103665]
- shard-apl: PASS -> INCOMPLETE [fdo#103927]
* igt@kms_busy@basic-modeset-f:
- shard-glk: NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +1
* igt@kms_busy@extended-modeset-hang-newfb-render-b:
- shard-kbl: PASS -> DMESG-WARN [fdo#110222] +1
* igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a:
- shard-kbl: NOTRUN -> DMESG-WARN [fdo#110222]
* igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b:
- shard-snb: PASS -> DMESG-WARN [fdo#110222]
* igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-c:
- shard-hsw: PASS -> DMESG-WARN [fdo#110222] +1
* igt@kms_busy@extended-modeset-hang-oldfb-render-f:
- shard-kbl: NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +1
* igt@kms_cursor_crc@cursor-64x64-onscreen:
- shard-glk: NOTRUN -> FAIL [fdo#103232]
* igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-spr-indfb-draw-render:
- shard-hsw: NOTRUN -> SKIP [fdo#109271] +14
* igt@kms_plane_alpha_blend@pipe-a-alpha-basic:
- shard-kbl: NOTRUN -> FAIL [fdo#108145] / [fdo#108590]
* igt@kms_plane_alpha_blend@pipe-c-alpha-transparant-fb:
- shard-kbl: NOTRUN -> FAIL [fdo#108145] +1
* igt@kms_plane_alpha_blend@pipe-c-constant-alpha-max:
- shard-glk: NOTRUN -> FAIL [fdo#108145]
* igt@kms_setmode@basic:
- shard-kbl: PASS -> FAIL [fdo#99912]
* igt@v3d_mmap@mmap-bad-handle:
- shard-kbl: NOTRUN -> SKIP [fdo#109271] +41
#### Possible fixes ####
* igt@i915_pm_rpm@cursor-dpms:
- shard-glk: DMESG-WARN -> PASS
* igt@kms_busy@extended-modeset-hang-newfb-render-c:
- shard-kbl: DMESG-WARN [fdo#110222] -> PASS
* igt@kms_cursor_crc@cursor-128x42-onscreen:
- shard-apl: FAIL [fdo#103232] -> PASS
* igt@kms_flip@flip-vs-expired-vblank:
- shard-glk: FAIL [fdo#102887] / [fdo#105363] -> PASS
* igt@kms_plane@pixel-format-pipe-c-planes:
- shard-glk: SKIP [fdo#109271] -> PASS
* igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
- shard-kbl: INCOMPLETE [fdo#103665] -> PASS
* igt@kms_rotation_crc@multiplane-rotation-cropping-top:
- shard-kbl: FAIL [fdo#109016] -> PASS
* igt@kms_vblank@pipe-a-ts-continuation-modeset-rpm:
- shard-apl: FAIL [fdo#104894] -> PASS +1
* igt@kms_vblank@pipe-b-query-idle:
- shard-snb: SKIP [fdo#109271] -> PASS
#### Warnings ####
* igt@gem_tiled_swapping@non-threaded:
- shard-hsw: FAIL [fdo#108686] -> INCOMPLETE [fdo#103540] / [fdo#108686]
* igt@kms_frontbuffer_tracking@fbcpsr-2p-scndscrn-pri-shrfb-draw-pwrite:
- shard-hsw: INCOMPLETE [fdo#103540] -> SKIP [fdo#109271]
* igt@runner@aborted:
- shard-glk: ( 2 FAIL ) [fdo#109373] / [k.org#202321] -> FAIL [fdo#109373] / [k.org#202321]
[fdo#102887]: https://bugs.freedesktop.org/show_bug.cgi?id=102887
[fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
[fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
[fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
[fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#108590]: https://bugs.freedesktop.org/show_bug.cgi?id=108590
[fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
[fdo#109016]: https://bugs.freedesktop.org/show_bug.cgi?id=109016
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
[fdo#109373]: https://bugs.freedesktop.org/show_bug.cgi?id=109373
[fdo#110222]: https://bugs.freedesktop.org/show_bug.cgi?id=110222
[fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
[k.org#202321]: https://bugzilla.kernel.org/show_bug.cgi?id=202321
Participating hosts (10 -> 5)
------------------------------
Missing (5): shard-skl pig-hsw-4770r pig-glk-j5005 shard-iclb pig-skl-6260u
Build changes
-------------
* IGT: IGT_4931 -> IGTPW_2800
* Piglit: piglit_4509 -> None
CI_DRM_5880: 10ec600ac87dc6da25d2784720d8fd029033d3ef @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_2800: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2800/
IGT_4931: 019f892e5d1a0a9643cb726c47ce2d99c14b444f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2800/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3.
2019-04-05 15:38 ` Maarten Lankhorst
@ 2019-04-08 8:50 ` Maxime Ripard
2019-04-08 11:04 ` Maarten Lankhorst
0 siblings, 1 reply; 17+ messages in thread
From: Maxime Ripard @ 2019-04-08 8:50 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: igt-dev
[-- Attachment #1.1: Type: text/plain, Size: 1732 bytes --]
Hi Maarten
On Fri, Apr 05, 2019 at 05:38:32PM +0200, Maarten Lankhorst wrote:
> Op 05-04-2019 om 15:54 schreef Paul Kocialkowski:
> > Hi,
> >
> > Le vendredi 05 avril 2019 à 14:52 +0200, Maarten Lankhorst a écrit :
> >> Ever since commit 3fa65f4b532bd9a5b ("fb: Add support for conversions
> >> through pixman") we can generate a valid cairo surface for any plane,
> >> use this to avoid having to implement our own conversion routine.
> >>
> >> Instead of duplicating this functionality in igt_fb_convert_with_stride,
> >> we can simply convert this to a few cairo calls, because we now support
> >> cairo calls to any of the supported framebuffer formats.
> > I don't think this is the case: cairo *only* takes linear buffers, so
> > we need the explicit tiling conversion step after having converted to a
> > new format with fb_convert. I don't see how it could work otherwise.
> >
> > Note that in igt, we're only interested in converting from linear to
> > tiled, not the other way round (for now, at least), so that's the
> > pipeline that the helper asssumes.
>
> We have code to handle this exact conversion in i-g-t already, both
> ways, see igt_get_cairo_surface(). :)
>
> You should be able to extend create_cairo_surface__convert() to
> handle broadcom tiling as well,
>
> I didn't see a function to convert from tiled to untiled, else I
> would have plugged it in for you.
That might be a bit dumb, but why do we need to use cairo all the
time in the first place?
That seems like a waste of resources when the only thing you want to
do is a conversion of one buffer to the other.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 153 bytes --]
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3.
2019-04-08 8:50 ` Maxime Ripard
@ 2019-04-08 11:04 ` Maarten Lankhorst
2019-04-09 14:59 ` Maxime Ripard
0 siblings, 1 reply; 17+ messages in thread
From: Maarten Lankhorst @ 2019-04-08 11:04 UTC (permalink / raw)
To: Maxime Ripard; +Cc: igt-dev
Op 08-04-2019 om 10:50 schreef Maxime Ripard:
> Hi Maarten
>
> On Fri, Apr 05, 2019 at 05:38:32PM +0200, Maarten Lankhorst wrote:
>> Op 05-04-2019 om 15:54 schreef Paul Kocialkowski:
>>> Hi,
>>>
>>> Le vendredi 05 avril 2019 à 14:52 +0200, Maarten Lankhorst a écrit :
>>>> Ever since commit 3fa65f4b532bd9a5b ("fb: Add support for conversions
>>>> through pixman") we can generate a valid cairo surface for any plane,
>>>> use this to avoid having to implement our own conversion routine.
>>>>
>>>> Instead of duplicating this functionality in igt_fb_convert_with_stride,
>>>> we can simply convert this to a few cairo calls, because we now support
>>>> cairo calls to any of the supported framebuffer formats.
>>> I don't think this is the case: cairo *only* takes linear buffers, so
>>> we need the explicit tiling conversion step after having converted to a
>>> new format with fb_convert. I don't see how it could work otherwise.
>>>
>>> Note that in igt, we're only interested in converting from linear to
>>> tiled, not the other way round (for now, at least), so that's the
>>> pipeline that the helper asssumes.
>> We have code to handle this exact conversion in i-g-t already, both
>> ways, see igt_get_cairo_surface(). :)
>>
>> You should be able to extend create_cairo_surface__convert() to
>> handle broadcom tiling as well,
>>
>> I didn't see a function to convert from tiled to untiled, else I
>> would have plugged it in for you.
> That might be a bit dumb, but why do we need to use cairo all the
> time in the first place?
>
> That seems like a waste of resources when the only thing you want to
> do is a conversion of one buffer to the other.
Cairo doesn't have its own internal buffer format, it uses the same pixman
calls that igt_fb_convert_with_stride does, so it's not less efficient.
The extra trip to convert the src buffer back after it's read from, or the
unnecessary conversion of the target buffer before it's overwritten it with
src buffer contents might be less efficient, but I don't think it's worth
optimizing it.
~Maarten
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3.
2019-04-08 11:04 ` Maarten Lankhorst
@ 2019-04-09 14:59 ` Maxime Ripard
0 siblings, 0 replies; 17+ messages in thread
From: Maxime Ripard @ 2019-04-09 14:59 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: igt-dev
[-- Attachment #1.1: Type: text/plain, Size: 2445 bytes --]
Hi,
On Mon, Apr 08, 2019 at 01:04:42PM +0200, Maarten Lankhorst wrote:
> Op 08-04-2019 om 10:50 schreef Maxime Ripard:
> > On Fri, Apr 05, 2019 at 05:38:32PM +0200, Maarten Lankhorst wrote:
> >> Op 05-04-2019 om 15:54 schreef Paul Kocialkowski:
> >>> Hi,
> >>>
> >>> Le vendredi 05 avril 2019 à 14:52 +0200, Maarten Lankhorst a écrit :
> >>>> Ever since commit 3fa65f4b532bd9a5b ("fb: Add support for conversions
> >>>> through pixman") we can generate a valid cairo surface for any plane,
> >>>> use this to avoid having to implement our own conversion routine.
> >>>>
> >>>> Instead of duplicating this functionality in igt_fb_convert_with_stride,
> >>>> we can simply convert this to a few cairo calls, because we now support
> >>>> cairo calls to any of the supported framebuffer formats.
> >>> I don't think this is the case: cairo *only* takes linear buffers, so
> >>> we need the explicit tiling conversion step after having converted to a
> >>> new format with fb_convert. I don't see how it could work otherwise.
> >>>
> >>> Note that in igt, we're only interested in converting from linear to
> >>> tiled, not the other way round (for now, at least), so that's the
> >>> pipeline that the helper asssumes.
> >> We have code to handle this exact conversion in i-g-t already, both
> >> ways, see igt_get_cairo_surface(). :)
> >>
> >> You should be able to extend create_cairo_surface__convert() to
> >> handle broadcom tiling as well,
> >>
> >> I didn't see a function to convert from tiled to untiled, else I
> >> would have plugged it in for you.
> > That might be a bit dumb, but why do we need to use cairo all the
> > time in the first place?
> >
> > That seems like a waste of resources when the only thing you want to
> > do is a conversion of one buffer to the other.
>
> Cairo doesn't have its own internal buffer format, it uses the same pixman
> calls that igt_fb_convert_with_stride does, so it's not less efficient.
>
> The extra trip to convert the src buffer back after it's read from, or the
> unnecessary conversion of the target buffer before it's overwritten it with
> src buffer contents might be less efficient, but I don't think it's worth
> optimizing it.
Right, but I wasn't so much talking about the number of buffers, but
more about the function calls overhead.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 153 bytes --]
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [igt-dev] [i-g-t, 2/2] tests/kms_chamelium: Fix *-cmp-random and *-crc-random tests, v3.
2019-04-05 12:52 ` [igt-dev] [PATCH i-g-t 2/2] tests/kms_chamelium: Fix *-cmp-random and *-crc-random tests, v3 Maarten Lankhorst
@ 2019-05-07 10:42 ` Ser, Simon
0 siblings, 0 replies; 17+ messages in thread
From: Ser, Simon @ 2019-05-07 10:42 UTC (permalink / raw)
To: igt-dev, maarten.lankhorst
Hi,
On Fri, 2019-04-05 at 14:52 +0200, Maarten Lankhorst wrote:
> The random tests allowed potentially invalid things:
> - 1x1 fb's to be created. Force a minimum of 32 on YUV so a scaled
> planar format plane will be at least 16x16. This will fix
> scaled/planar format support in i915 and avoid a div by zero when
> calculating a value modulo h/2 and w/2.
> - Downscaling to any amount, restrict it to 2x to make the test pass.
> - Some hw may not allow scaling, in those cases we should fallback
> to no scaling at all.
> - Attempting to configure a minimum of 4 planes, instead of a maximum.
> This fails with a null pointer deref if the hw doesn't have 4
> configurable overlay planes.
>
> Changes since v1:
> - Enforce a minimum displayed size of 16x16 for intel only,
> otherwise it's 1x1.
> - Fix comments.
> Changes since v2:
> - Fix comments harder.
> - Pick a random format and its modifier from the plane itself,
> instead of using igt_format_array_fill(). This will cause the test
> to use all valid combinations of modifiers and formats.
> - Set minimum dimension to 8 for !yuv, and 16 for YUV.
> - Generate format / modifier before
>
> Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> tests/kms_chamelium.c | 318 +++++++++++++++++++++++++-----------------
> 1 file changed, 193 insertions(+), 125 deletions(-)
>
> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> index 2dc1049d2dda..a55bd86c77f5 100644
> --- a/tests/kms_chamelium.c
> +++ b/tests/kms_chamelium.c
> @@ -710,17 +710,48 @@ test_display_frame_dump(data_t *data, struct chamelium_port *port)
> drmModeFreeConnector(connector);
> }
>
> -static void select_tiled_modifier(igt_plane_t *plane, uint32_t width,
> +
> +static void randomize_plane_stride(data_t *data,
> + uint32_t width, uint32_t height,
> + uint32_t format, uint64_t modifier,
> + size_t *stride)
> +{
> + size_t stride_min;
> + uint32_t max_tile_w = 4, tile_w, tile_h;
> + int i;
> + struct igt_fb dummy;
> +
> + stride_min = width * igt_format_plane_bpp(format, 0) / 8;
> +
> + /* Randomize the stride to less than twice the minimum. */
> + *stride = (rand() % stride_min) + stride_min;
> +
> + /*
> + * Create a dummy FB to determine bpp for each plane, and calculate
> + * the maximum tile width from that.
> + */
> + igt_create_fb(data->drm_fd, 64, 64, format, modifier, &dummy);
> + for (i = 0; i < dummy.num_planes; i++) {
> + igt_get_fb_tile_size(data->drm_fd, modifier, dummy.plane_bpp[i], &tile_w, &tile_h);
> +
> + if (tile_w > max_tile_w)
> + max_tile_w = tile_w;
> + }
> + igt_remove_fb(data->drm_fd, &dummy);
> +
> + /*
> + * Pixman requires the stride to be aligned to 32-bits, which is
> + * reflected in the initial value of max_tile_w and the hw
> + * may require a multiple of tile width, choose biggest of the 2.
> + */
> + *stride = ALIGN(*stride, max_tile_w);
Is max_tile_w guaranteed to be a multiple of 4?
The patch LGTM otherwise, but I'm not comfortable enough with the
codebase yet to mark it as reviewed. Thus, with this fixed:
Acked-by: Simon Ser <simon.ser@intel.com>
> +}
> +
> +static void update_tiled_modifier(igt_plane_t *plane, uint32_t width,
> uint32_t height, uint32_t format,
> uint64_t *modifier)
> {
> - if (igt_plane_has_format_mod(plane, format,
> - DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED)) {
> - igt_debug("Selecting VC4 T-tiling\n");
> -
> - *modifier = DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED;
> - } else if (igt_plane_has_format_mod(plane, format,
> - DRM_FORMAT_MOD_BROADCOM_SAND256)) {
> + if (*modifier == DRM_FORMAT_MOD_BROADCOM_SAND256) {
> /* Randomize the column height to less than twice the minimum. */
> size_t column_height = (rand() % height) + height;
>
> @@ -728,90 +759,84 @@ static void select_tiled_modifier(igt_plane_t *plane, uint32_t width,
> column_height);
>
> *modifier = DRM_FORMAT_MOD_BROADCOM_SAND256_COL_HEIGHT(column_height);
> - } else {
> - *modifier = DRM_FORMAT_MOD_LINEAR;
> }
> }
>
> -static void randomize_plane_format_stride(igt_plane_t *plane,
> - uint32_t width, uint32_t height,
> - uint32_t *format, uint64_t *modifier,
> - size_t *stride, bool allow_yuv)
> +static void randomize_plane_setup(data_t *data, igt_plane_t *plane,
> + drmModeModeInfo *mode,
> + uint32_t *width, uint32_t *height,
> + uint32_t *format, uint64_t *modifier,
> + bool allow_yuv)
> {
> - size_t stride_min;
> - uint32_t *formats_array;
> - unsigned int formats_count;
> + int min_dim;
> + uint32_t idx[plane->format_mod_count];
> unsigned int count = 0;
> unsigned int i;
> - bool tiled;
> - int index;
> -
> - igt_format_array_fill(&formats_array, &formats_count, allow_yuv);
>
> /* First pass to count the supported formats. */
> - for (i = 0; i < formats_count; i++)
> - if (igt_plane_has_format_mod(plane, formats_array[i],
> - DRM_FORMAT_MOD_LINEAR))
> - count++;
> + for (i = 0; i < plane->format_mod_count; i++)
> + if (igt_fb_supported_format(plane->formats[i]) &&
> + (allow_yuv || !igt_format_is_yuv(plane->formats[i])))
> + idx[count++] = i;
>
> igt_assert(count > 0);
>
> - index = rand() % count;
> -
> - /* Second pass to get the index-th supported format. */
> - for (i = 0; i < formats_count; i++) {
> - if (!igt_plane_has_format_mod(plane, formats_array[i],
> - DRM_FORMAT_MOD_LINEAR))
> - continue;
> -
> - if (!index--) {
> - *format = formats_array[i];
> - break;
> - }
> - }
> -
> - free(formats_array);
> + i = idx[rand() % count];
> + *format = plane->formats[i];
> + *modifier = plane->modifiers[i];
>
> - igt_assert(index < 0);
> + update_tiled_modifier(plane, *width, *height, *format, modifier);
>
> - stride_min = width * igt_format_plane_bpp(*format, 0) / 8;
> + /*
> + * Randomize width and height in the mode dimensions range.
> + *
> + * Restrict to a min of 2 * min_dim, this way src_w/h are always at
> + * least min_dim, because src_w = width - (rand % w / 2).
> + *
> + * Use a minimum dimension of 16 for YUV, because planar YUV
> + * subsamples the UV plane.
> + */
> + min_dim = igt_format_is_yuv(*format) ? 16 : 8;
>
> - /* Randomize the stride to less than twice the minimum. */
> - *stride = (rand() % stride_min) + stride_min;
> + *width = max((rand() % mode->hdisplay) + 1, 2 * min_dim);
> + *height = max((rand() % mode->vdisplay) + 1, 2 * min_dim);
> +}
>
> - /* Pixman requires the stride to be aligned to 32-byte words. */
> - *stride = ALIGN(*stride, sizeof(uint32_t));
> +static void configure_plane(igt_plane_t *plane, uint32_t src_w, uint32_t src_h,
> + uint32_t src_x, uint32_t src_y, uint32_t crtc_w,
> + uint32_t crtc_h, int32_t crtc_x, int32_t crtc_y,
> + struct igt_fb *fb)
> +{
> + igt_plane_set_fb(plane, fb);
>
> - /* Randomize the use of a tiled mode with a 1/4 probability. */
> - tiled = ((rand() % 4) == 0);
> + igt_plane_set_position(plane, crtc_x, crtc_y);
> + igt_plane_set_size(plane, crtc_w, crtc_h);
>
> - if (tiled)
> - select_tiled_modifier(plane, width, height, *format, modifier);
> - else
> - *modifier = DRM_FORMAT_MOD_LINEAR;
> + igt_fb_set_position(fb, plane, src_x, src_y);
> + igt_fb_set_size(fb, plane, src_w, src_h);
> }
>
> -static void randomize_plane_dimensions(drmModeModeInfo *mode,
> - uint32_t *width, uint32_t *height,
> - uint32_t *src_w, uint32_t *src_h,
> - uint32_t *src_x, uint32_t *src_y,
> - uint32_t *crtc_w, uint32_t *crtc_h,
> - int32_t *crtc_x, int32_t *crtc_y,
> - bool allow_scaling)
> +static void randomize_plane_coordinates(data_t *data, igt_plane_t *plane,
> + drmModeModeInfo *mode,
> + struct igt_fb *fb,
> + uint32_t *src_w, uint32_t *src_h,
> + uint32_t *src_x, uint32_t *src_y,
> + uint32_t *crtc_w, uint32_t *crtc_h,
> + int32_t *crtc_x, int32_t *crtc_y,
> + bool allow_scaling)
> {
> + bool is_yuv = igt_format_is_yuv(fb->drm_format);
> + uint32_t width = fb->width, height = fb->height;
> double ratio;
> -
> - /* Randomize width and height in the mode dimensions range. */
> - *width = (rand() % mode->hdisplay) + 1;
> - *height = (rand() % mode->vdisplay) + 1;
> + int ret;
>
> /* Randomize source offset in the first half of the original size. */
> - *src_x = rand() % (*width / 2);
> - *src_y = rand() % (*height / 2);
> + *src_x = rand() % (width / 2);
> + *src_y = rand() % (height / 2);
>
> /* The source size only includes the active source area. */
> - *src_w = *width - *src_x;
> - *src_h = *height - *src_y;
> + *src_w = width - *src_x;
> + *src_h = height - *src_y;
>
> if (allow_scaling) {
> *crtc_w = (rand() % mode->hdisplay) + 1;
> @@ -821,17 +846,22 @@ static void randomize_plane_dimensions(drmModeModeInfo *mode,
> * Don't bother with scaling if dimensions are quite close in
> * order to get non-scaling cases more frequently. Also limit
> * scaling to 3x to avoid agressive filtering that makes
> - * comparison less reliable.
> + * comparison less reliable, and don't go above 2x downsampling
> + * to avoid possible hw limitations.
> */
>
> ratio = ((double) *crtc_w / *src_w);
> - if (ratio > 0.8 && ratio < 1.2)
> + if (ratio < 0.5)
> + *src_w = *crtc_w * 2;
> + else if (ratio > 0.8 && ratio < 1.2)
> *crtc_w = *src_w;
> else if (ratio > 3.0)
> *crtc_w = *src_w * 3;
>
> ratio = ((double) *crtc_h / *src_h);
> - if (ratio > 0.8 && ratio < 1.2)
> + if (ratio < 0.5)
> + *src_h = *crtc_h * 2;
> + else if (ratio > 0.8 && ratio < 1.2)
> *crtc_h = *src_h;
> else if (ratio > 3.0)
> *crtc_h = *src_h * 3;
> @@ -846,8 +876,15 @@ static void randomize_plane_dimensions(drmModeModeInfo *mode,
> * scaled clipping may result in decimal dimensions, that most
> * drivers don't support.
> */
> - *crtc_x = rand() % (mode->hdisplay - *crtc_w);
> - *crtc_y = rand() % (mode->vdisplay - *crtc_h);
> + if (*crtc_w < mode->hdisplay)
> + *crtc_x = rand() % (mode->hdisplay - *crtc_w);
> + else
> + *crtc_x = 0;
> +
> + if (*crtc_h < mode->vdisplay)
> + *crtc_y = rand() % (mode->vdisplay - *crtc_h);
> + else
> + *crtc_y = 0;
> } else {
> /*
> * Randomize the on-crtc position and allow the plane to go
> @@ -856,6 +893,62 @@ static void randomize_plane_dimensions(drmModeModeInfo *mode,
> *crtc_x = (rand() % mode->hdisplay) - *crtc_w / 2;
> *crtc_y = (rand() % mode->vdisplay) - *crtc_h / 2;
> }
> +
> + configure_plane(plane, *src_w, *src_h, *src_x, *src_y,
> + *crtc_w, *crtc_h, *crtc_x, *crtc_y, fb);
> + ret = igt_display_try_commit_atomic(&data->display,
> + DRM_MODE_ATOMIC_TEST_ONLY |
> + DRM_MODE_ATOMIC_ALLOW_MODESET,
> + NULL);
> + if (!ret)
> + return;
> +
> + /* Coordinates are logged in the dumped debug log, so only report w/h on failure here. */
> + igt_assert_f(ret != -ENOSPC,"Failure in testcase, invalid coordinates on a %ux%u fb\n", width, height);
> +
> + /* Make YUV coordinates a multiple of 2 and retry the math. */
> + if (is_yuv) {
> + *src_x &= ~1;
> + *src_y &= ~1;
> + *src_w &= ~1;
> + *src_h &= ~1;
> + /* To handle 1:1 scaling, clear crtc_w/h too. */
> + *crtc_w &= ~1;
> + *crtc_h &= ~1;
> +
> + if (*crtc_x < 0 && (*crtc_x & 1))
> + (*crtc_x)++;
> + else
> + *crtc_x &= ~1;
> +
> + /* If negative, round up to 0 instead of down */
> + if (*crtc_y < 0 && (*crtc_y & 1))
> + (*crtc_y)++;
> + else
> + *crtc_y &= ~1;
> +
> + configure_plane(plane, *src_w, *src_h, *src_x, *src_y, *crtc_w,
> + *crtc_h, *crtc_x, *crtc_y, fb);
> + ret = igt_display_try_commit_atomic(&data->display,
> + DRM_MODE_ATOMIC_TEST_ONLY |
> + DRM_MODE_ATOMIC_ALLOW_MODESET,
> + NULL);
> + if (!ret)
> + return;
> + }
> +
> + igt_assert(!ret || allow_scaling);
> + igt_info("Scaling ratio %g / %g failed, trying without scaling.\n",
> + ((double) *crtc_w / *src_w), ((double) *crtc_h / *src_h));
> +
> + *crtc_w = *src_w;
> + *crtc_h = *src_h;
> +
> + configure_plane(plane, *src_w, *src_h, *src_x, *src_y, *crtc_w,
> + *crtc_h, *crtc_x, *crtc_y, fb);
> + igt_display_commit_atomic(&data->display,
> + DRM_MODE_ATOMIC_TEST_ONLY |
> + DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> }
>
> static void blit_plane_cairo(data_t *data, cairo_surface_t *result,
> @@ -914,20 +1007,6 @@ static void blit_plane_cairo(data_t *data, cairo_surface_t *result,
> cairo_destroy(cr);
> }
>
> -static void configure_plane(igt_plane_t *plane, uint32_t src_w, uint32_t src_h,
> - uint32_t src_x, uint32_t src_y, uint32_t crtc_w,
> - uint32_t crtc_h, int32_t crtc_x, int32_t crtc_y,
> - struct igt_fb *fb)
> -{
> - igt_plane_set_fb(plane, fb);
> -
> - igt_plane_set_position(plane, crtc_x, crtc_y);
> - igt_plane_set_size(plane, crtc_w, crtc_h);
> -
> - igt_fb_set_position(fb, plane, src_x, src_y);
> - igt_fb_set_size(fb, plane, src_w, src_h);
> -}
> -
> static void prepare_randomized_plane(data_t *data,
> drmModeModeInfo *mode,
> igt_plane_t *plane,
> @@ -948,51 +1027,49 @@ static void prepare_randomized_plane(data_t *data,
> bool tiled;
> int fb_id;
>
> - randomize_plane_dimensions(mode, &overlay_fb_w, &overlay_fb_h,
> - &overlay_src_w, &overlay_src_h,
> - &overlay_src_x, &overlay_src_y,
> - &overlay_crtc_w, &overlay_crtc_h,
> - &overlay_crtc_x, &overlay_crtc_y,
> - allow_scaling);
> + randomize_plane_setup(data, plane, mode, &overlay_fb_w, &overlay_fb_h,
> + &format, &modifier, allow_yuv);
>
> - igt_debug("Plane %d: framebuffer size %dx%d\n", index,
> - overlay_fb_w, overlay_fb_h);
> - igt_debug("Plane %d: on-crtc size %dx%d\n", index,
> - overlay_crtc_w, overlay_crtc_h);
> - igt_debug("Plane %d: on-crtc position %dx%d\n", index,
> - overlay_crtc_x, overlay_crtc_y);
> - igt_debug("Plane %d: in-framebuffer size %dx%d\n", index,
> - overlay_src_w, overlay_src_h);
> - igt_debug("Plane %d: in-framebuffer position %dx%d\n", index,
> - overlay_src_x, overlay_src_y);
> + tiled = (modifier != LOCAL_DRM_FORMAT_MOD_NONE);
> + igt_debug("Plane %d: framebuffer size %dx%d %s format (%s)\n",
> + index, overlay_fb_w, overlay_fb_h,
> + igt_format_str(format), tiled ? "tiled" : "linear");
>
> /* Get a pattern framebuffer for the overlay plane. */
> fb_id = chamelium_get_pattern_fb(data, overlay_fb_w, overlay_fb_h,
> DRM_FORMAT_XRGB8888, 32, &pattern_fb);
> igt_assert(fb_id > 0);
>
> - randomize_plane_format_stride(plane, overlay_fb_w, overlay_fb_h,
> - &format, &modifier, &stride, allow_yuv);
> -
> - tiled = (modifier != LOCAL_DRM_FORMAT_MOD_NONE);
> + randomize_plane_stride(data, overlay_fb_w, overlay_fb_h,
> + format, modifier, &stride);
>
> - igt_debug("Plane %d: %s format (%s) with stride %ld\n", index,
> - igt_format_str(format), tiled ? "tiled" : "linear", stride);
> + igt_debug("Plane %d: stride %ld\n", index, stride);
>
> fb_id = igt_fb_convert_with_stride(overlay_fb, &pattern_fb, format,
> modifier, stride);
> igt_assert(fb_id > 0);
>
> + randomize_plane_coordinates(data, plane, mode, overlay_fb,
> + &overlay_src_w, &overlay_src_h,
> + &overlay_src_x, &overlay_src_y,
> + &overlay_crtc_w, &overlay_crtc_h,
> + &overlay_crtc_x, &overlay_crtc_y,
> + allow_scaling);
> +
> + igt_debug("Plane %d: in-framebuffer size %dx%d\n", index,
> + overlay_src_w, overlay_src_h);
> + igt_debug("Plane %d: in-framebuffer position %dx%d\n", index,
> + overlay_src_x, overlay_src_y);
> + igt_debug("Plane %d: on-crtc size %dx%d\n", index,
> + overlay_crtc_w, overlay_crtc_h);
> + igt_debug("Plane %d: on-crtc position %dx%d\n", index,
> + overlay_crtc_x, overlay_crtc_y);
> +
> blit_plane_cairo(data, result_surface, overlay_src_w, overlay_src_h,
> overlay_src_x, overlay_src_y,
> overlay_crtc_w, overlay_crtc_h,
> overlay_crtc_x, overlay_crtc_y, &pattern_fb);
>
> - configure_plane(plane, overlay_src_w, overlay_src_h,
> - overlay_src_x, overlay_src_y,
> - overlay_crtc_w, overlay_crtc_h,
> - overlay_crtc_x, overlay_crtc_y, overlay_fb);
> -
> /* Remove the original pattern framebuffer. */
> igt_remove_fb(data->drm_fd, &pattern_fb);
> }
> @@ -1068,7 +1145,7 @@ static void test_display_planes_random(data_t *data,
> igt_output_count_plane_type(output, DRM_PLANE_TYPE_OVERLAY);
>
> /* Limit the number of planes to a reasonable scene. */
> - overlay_planes_max = max(overlay_planes_max, 4);
> + overlay_planes_max = min(overlay_planes_max, 4);
>
> overlay_planes_count = (rand() % overlay_planes_max) + 1;
> igt_debug("Using %d overlay planes\n", overlay_planes_count);
> @@ -1121,17 +1198,8 @@ static void test_display_planes_random(data_t *data,
> chamelium_destroy_frame_dump(dump);
> }
>
> - for (i = 0; i < overlay_planes_count; i++) {
> - struct igt_fb *overlay_fb = &overlay_fbs[i];
> - igt_plane_t *plane;
> -
> - plane = igt_output_get_plane_type_index(output,
> - DRM_PLANE_TYPE_OVERLAY,
> - i);
> - igt_assert(plane);
> -
> - igt_remove_fb(data->drm_fd, overlay_fb);
> - }
> + for (i = 0; i < overlay_planes_count; i++)
> + igt_remove_fb(data->drm_fd, &overlay_fbs[i]);
>
> free(overlay_fbs);
>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3.
2019-05-14 6:34 ` Juha-Pekka Heikkila
@ 2019-05-16 12:59 ` Maarten Lankhorst
0 siblings, 0 replies; 17+ messages in thread
From: Maarten Lankhorst @ 2019-05-16 12:59 UTC (permalink / raw)
To: juhapekka.heikkila, Paul Kocialkowski, igt-dev
Op 14-05-2019 om 08:34 schreef Juha-Pekka Heikkila:
> This change look ok to me. To me it look vc4 tiling handling stay on same level as what it was before as what Paul was worried about.
>
> Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>
Thanks, pushed and 2/2 as well. :)
> On 10.5.2019 19.02, Maarten Lankhorst wrote:
>> Op 10-05-2019 om 17:10 schreef Paul Kocialkowski:
>>> Hi,
>>>
>>> On Fri, 2019-05-10 at 16:19 +0200, Maarten Lankhorst wrote:
>>>> Op 10-05-2019 om 15:43 schreef Paul Kocialkowski:
>>>>> Hi,
>>>>>
>>>>> It looks like I fell behind on reviewing earlier version here, sorry
>>>>> about that.
>>>>>
>>>>> On Fri, 2019-05-10 at 14:33 +0200, Maarten Lankhorst wrote:
>>>>>> Ever since commit 3fa65f4b532bd9a5b ("fb: Add support for conversions
>>>>>> through pixman") we can generate a valid cairo surface for any plane,
>>>>>> use this to avoid having to implement our own conversion routine.
>>>>>>
>>>>>> Instead of duplicating this functionality in igt_fb_convert_with_stride,
>>>>>> we can simply convert this to a few cairo calls, because we now support
>>>>>> cairo calls to any of the supported framebuffer formats.
>>>>> I don't see how that is the case. Cairo definitely does not support our
>>>>> tiled formats, so we can't just pipe them into it.
>>>>>
>>>>> And we already use pixman for converting through the fb_convert call to
>>>>> convert_pixman when possible. Also, my understanding is that cairo is
>>>>> very limited format-wise, so we're much better off using pixman
>>>>> directly (which is is what a non-accelerated cairo surface will do
>>>>> anyway).
>>>>>
>>>>>> This is required to make this function more generic, and convert from any
>>>>>> format/modifier to any other format/modifier.
>>>>> We've already designed it to be generic, we just need conversion
>>>>> helpers from/to (currently only to) hardware-specific tiling modifiers.
>>>>>
>>>>> We can't expect cairo or pixman to do that job and this change pretty
>>>>> much kills support for the vc4 tiling modes we've added.
>>>>>
>>>>> Unless I'm missing something, that's a NACK on my side.
>>>> You have a function that can convert a untiled fb to tiled, but not
>>>> the other way around in igt_vc4.c
>>> We don't need it for anything at the moment but it wouldn't be a
>>> problem to come up with one, sure.
>>>
>>>> If you could provide that, we could convert from any fb/modifier
>>>> format to any fb/modifier format, even with VC4 tiling.
>>> I don't fully get the point of that if our tests are not going to use
>>> it, but it doesn't hurt either :)
>>>
>>>> Our internal cairo hooks already provide helpers for conversion, see
>>>> igt_get_cairo_surface() which calls
>>>> create_cairo_surface__convert()
>>> My feeling is that we should kill use of cairo formats and go with
>>> pixman directly.
>>>
>>>> Some conversions can only be done through cairo, converting between
>>>> P01x and XRGB8888 cannot be done directly,
>>>> our pixman representation for that are done in the floating point
>>>> formats.
>>> I'm not sure I'm following this point, but if there's a conversion that
>>> cairo can do and pixman can't, it feels like the right thing would be
>>> to fix pixman to support it. Is there a reason in particular why this
>>> wouldn't work?
>>>
>>>> The only way to convert between some framebuffer formats and
>>>> modifiers in i915 is by using those conversion functions,
>>>> the fact that igt_fb_convert_with_stride doesn't do those, makes a
>>>> truly random hdmi-cmp-planes-random useless.
>>> So it boils down to a missing format support issue, not really to an
>>> issue with the existing flow.
>>>
>>>> I was getting CRC mismatches because the i915 modifiers weren't
>>>> respected. When I made sure they were being respected
>>>> I ended up with a duplicate of the cairo context stuff. So why not
>>>> lose a little speed and use that?
>>> My counter-suggestion would be to do the opposite and make sure pixman
>>> can deal with these cases on its own.
>>>
>>>> If you write and test a detiler function, I can hook it up for you.
>>>> :)
>>>> If necessary I can do it myself, to move this patch forward.
>>>>
>>>> Cairo shouldn't be much slower than pixman, because internally it
>>>> already uses pixman calls for the backend.
>>> Sure, but that's not cairo's role: cairo is about drawing, while pixman
>>> is about pixel conversion. I think it would be best to stick to that.
>>>
>>> Cheers,
>>>
>>> Paul
>>>
>> What about this?
>>
>> We could draw reference FB's in vc4 t tiled formats now too..
>>
>> ---8<----
>> From 7242edd2cd0096556080e0cda8d4ecea4e3fc58d Mon Sep 17 00:00:00 2001
>> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Date: Tue, 2 Apr 2019 16:54:30 +0200
>> Subject: [PATCH i-g-t] lib/igt_fb: Use cairo conversion in
>> igt_fb_convert_with_stride, v4.
>>
>> Ever since commit 3fa65f4b532bd9a5b ("fb: Add support for conversions
>> through pixman") we can generate a valid cairo surface for any plane,
>> use this to avoid having to implement our own conversion routine.
>>
>> Instead of duplicating this functionality in igt_fb_convert_with_stride,
>> we can simply convert this to a few cairo calls, because we now support
>> cairo calls to any of the supported framebuffer formats.
>>
>> This is required to make this function more generic, and convert from any
>> format/modifier to any other format/modifier.
>>
>> Changes since v1:
>> - Return fb_id in the cairo case.
>> Changes since v2:
>> - Remove the manual conversion fallback.
>> Changes since v3:
>> - Integrate VC4 conversion routines.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
>> ---
>> lib/igt_fb.c | 129 +++++++++++------------
>> lib/igt_vc4.c | 278 ++++++++++++++++++++++++++++++++------------------
>> lib/igt_vc4.h | 12 +--
>> 3 files changed, 242 insertions(+), 177 deletions(-)
>>
>> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
>> index d4929019971c..c6e18397b754 100644
>> --- a/lib/igt_fb.c
>> +++ b/lib/igt_fb.c
>> @@ -1716,17 +1716,25 @@ static void free_linear_mapping(struct fb_blit_upload *blit)
>> struct igt_fb *fb = blit->fb;
>> struct fb_blit_linear *linear = &blit->linear;
>> - gem_munmap(linear->map, linear->fb.size);
>> - gem_set_domain(fd, linear->fb.gem_handle,
>> - I915_GEM_DOMAIN_GTT, 0);
>> + if (igt_vc4_is_tiled(fb->modifier)) {
>> + void *map = igt_vc4_mmap_bo(fd, fb->gem_handle, fb->size, PROT_WRITE);
>> - if (blit->batch)
>> - rendercopy(blit, fb, &linear->fb);
>> - else
>> - blitcopy(fb, &linear->fb);
>> + vc4_fb_convert_plane_to_tiled(fb, map, &linear->fb, &linear->map);
>> +
>> + munmap(map, fb->size);
>> + } else {
>> + gem_munmap(linear->map, linear->fb.size);
>> + gem_set_domain(fd, linear->fb.gem_handle,
>> + I915_GEM_DOMAIN_GTT, 0);
>> +
>> + if (blit->batch)
>> + rendercopy(blit, fb, &linear->fb);
>> + else
>> + blitcopy(fb, &linear->fb);
>> - gem_sync(fd, linear->fb.gem_handle);
>> - gem_close(fd, linear->fb.gem_handle);
>> + gem_sync(fd, linear->fb.gem_handle);
>> + gem_close(fd, linear->fb.gem_handle);
>> + }
>> if (blit->batch) {
>> intel_batchbuffer_free(blit->batch);
>> @@ -1751,7 +1759,7 @@ static void setup_linear_mapping(struct fb_blit_upload *blit)
>> struct igt_fb *fb = blit->fb;
>> struct fb_blit_linear *linear = &blit->linear;
>> - if (use_rendercopy(fb)) {
>> + if (!igt_vc4_is_tiled(fb->modifier) && use_rendercopy(fb)) {
>> blit->bufmgr = drm_intel_bufmgr_gem_init(fd, 4096);
>> blit->batch = intel_batchbuffer_alloc(blit->bufmgr,
>> intel_get_drm_devid(fd));
>> @@ -1771,23 +1779,35 @@ static void setup_linear_mapping(struct fb_blit_upload *blit)
>> igt_assert(linear->fb.gem_handle > 0);
>> - /* Copy fb content to linear BO */
>> - gem_set_domain(fd, linear->fb.gem_handle,
>> - I915_GEM_DOMAIN_GTT, 0);
>> + if (igt_vc4_is_tiled(fb->modifier)) {
>> + void *map = igt_vc4_mmap_bo(fd, fb->gem_handle, fb->size, PROT_READ);
>> - if (blit->batch)
>> - rendercopy(blit, &linear->fb, fb);
>> - else
>> - blitcopy(&linear->fb, fb);
>> + linear->map = igt_vc4_mmap_bo(fd, linear->fb.gem_handle,
>> + linear->fb.size,
>> + PROT_READ | PROT_WRITE);
>> - gem_sync(fd, linear->fb.gem_handle);
>> + vc4_fb_convert_plane_from_tiled(&linear->fb, &linear->map, fb, map);
>> - gem_set_domain(fd, linear->fb.gem_handle,
>> - I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
>> + munmap(map, fb->size);
>> + } else {
>> + /* Copy fb content to linear BO */
>> + gem_set_domain(fd, linear->fb.gem_handle,
>> + I915_GEM_DOMAIN_GTT, 0);
>> +
>> + if (blit->batch)
>> + rendercopy(blit, &linear->fb, fb);
>> + else
>> + blitcopy(&linear->fb, fb);
>> +
>> + gem_sync(fd, linear->fb.gem_handle);
>> +
>> + gem_set_domain(fd, linear->fb.gem_handle,
>> + I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
>> - /* Setup cairo context */
>> - linear->map = gem_mmap__cpu(fd, linear->fb.gem_handle,
>> - 0, linear->fb.size, PROT_READ | PROT_WRITE);
>> + /* Setup cairo context */
>> + linear->map = gem_mmap__cpu(fd, linear->fb.gem_handle,
>> + 0, linear->fb.size, PROT_READ | PROT_WRITE);
>> + }
>> }
>> static void create_cairo_surface__gpu(int fd, struct igt_fb *fb)
>> @@ -2902,7 +2922,7 @@ static void create_cairo_surface__convert(int fd, struct igt_fb *fb)
>> &blit->shadow_fb);
>> igt_assert(blit->shadow_ptr);
>> - if (use_rendercopy(fb) || use_blitter(fb)) {
>> + if (use_rendercopy(fb) || use_blitter(fb) || igt_vc4_is_tiled(fb->modifier)) {
>> setup_linear_mapping(&blit->base);
>> } else {
>> blit->base.linear.fb = *fb;
>> @@ -2983,7 +3003,7 @@ cairo_surface_t *igt_get_cairo_surface(int fd, struct igt_fb *fb)
>> ((f->cairo_id == CAIRO_FORMAT_INVALID) &&
>> (f->pixman_id != PIXMAN_invalid)))
>> create_cairo_surface__convert(fd, fb);
>> - else if (use_blitter(fb) || use_rendercopy(fb))
>> + else if (use_blitter(fb) || use_rendercopy(fb) || igt_vc4_is_tiled(fb->modifier))
>> create_cairo_surface__gpu(fd, fb);
>> else
>> create_cairo_surface__gtt(fd, fb);
>> @@ -3102,58 +3122,23 @@ unsigned int igt_fb_convert_with_stride(struct igt_fb *dst, struct igt_fb *src,
>> uint64_t dst_modifier,
>> unsigned int dst_stride)
>> {
>> - struct fb_convert cvt = { };
>> - struct igt_fb linear;
>> - void *dst_ptr, *src_ptr;
>> - uint64_t base_modifier;
>> + /* Use the cairo api to convert */
>> + cairo_surface_t *surf = igt_get_cairo_surface(src->fd, src);
>> + cairo_t *cr;
>> int fb_id;
>> - if (is_vc4_device(src->fd))
>> - base_modifier = fourcc_mod_broadcom_mod(dst_modifier);
>> - else
>> - base_modifier = dst_modifier;
>> -
>> - fb_id = igt_create_fb_with_bo_size(src->fd, src->width, src->height,
>> - dst_fourcc,
>> - LOCAL_DRM_FORMAT_MOD_NONE, &linear,
>> - 0, dst_stride);
>> + fb_id = igt_create_fb_with_bo_size(src->fd, src->width,
>> + src->height, dst_fourcc,
>> + dst_modifier, dst, 0,
>> + dst_stride);
>> igt_assert(fb_id > 0);
>> - src_ptr = igt_fb_map_buffer(src->fd, src);
>> - igt_assert(src_ptr);
>> -
>> - dst_ptr = igt_fb_map_buffer(linear.fd, &linear);
>> - igt_assert(dst_ptr);
>> -
>> - cvt.dst.ptr = dst_ptr;
>> - cvt.dst.fb = &linear;
>> - cvt.src.ptr = src_ptr;
>> - cvt.src.fb = src;
>> - fb_convert(&cvt);
>> -
>> - igt_fb_unmap_buffer(dst, dst_ptr);
>> - igt_fb_unmap_buffer(src, src_ptr);
>> -
>> - switch (base_modifier) {
>> - case DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED:
>> - fb_id = igt_vc4_fb_t_tiled_convert(dst, &linear);
>> - break;
>> - case DRM_FORMAT_MOD_BROADCOM_SAND32:
>> - case DRM_FORMAT_MOD_BROADCOM_SAND64:
>> - case DRM_FORMAT_MOD_BROADCOM_SAND128:
>> - case DRM_FORMAT_MOD_BROADCOM_SAND256:
>> - fb_id = vc4_fb_sand_tiled_convert(dst, &linear, dst_modifier);
>> - break;
>> - default:
>> - igt_assert(dst_modifier == LOCAL_DRM_FORMAT_MOD_NONE);
>> - }
>> -
>> - igt_assert(fb_id > 0);
>> + cr = igt_get_cairo_ctx(dst->fd, dst);
>> + cairo_set_source_surface(cr, surf, 0, 0);
>> + cairo_paint(cr);
>> + igt_put_cairo_ctx(dst->fd, dst, cr);
>> - if (dst_modifier == LOCAL_DRM_FORMAT_MOD_NONE)
>> - *dst = linear;
>> - else
>> - igt_remove_fb(linear.fd, &linear);
>> + cairo_surface_destroy(surf);
>> return fb_id;
>> }
>> diff --git a/lib/igt_vc4.c b/lib/igt_vc4.c
>> index 9a0ba30b4420..4415fa321ceb 100644
>> --- a/lib/igt_vc4.c
>> +++ b/lib/igt_vc4.c
>> @@ -56,6 +56,23 @@
>> * tests.
>> */
>> +bool igt_vc4_is_tiled(uint64_t modifier)
>> +{
>> + if (modifier >> 56ULL != DRM_FORMAT_MOD_VENDOR_BROADCOM)
>> + return false;
>> +
>> + switch (fourcc_mod_broadcom_mod(modifier)) {
>> + case DRM_FORMAT_MOD_BROADCOM_SAND32:
>> + case DRM_FORMAT_MOD_BROADCOM_SAND64:
>> + case DRM_FORMAT_MOD_BROADCOM_SAND128:
>> + case DRM_FORMAT_MOD_BROADCOM_SAND256:
>> + case DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED:
>> + return true;
>> + default:
>> + return false;
>> + }
>> +}
>> +
>> /**
>> * igt_vc4_get_cleared_bo:
>> * @fd: device file descriptor
>> @@ -178,63 +195,12 @@ bool igt_vc4_purgeable_bo(int fd, int handle, bool purgeable)
>> return arg.retained;
>> }
>> -unsigned int igt_vc4_fb_t_tiled_convert(struct igt_fb *dst, struct igt_fb *src)
>> -{
>> - unsigned int fb_id;
>> - unsigned int i, j;
>> - void *src_buf;
>> - void *dst_buf;
>> - size_t bpp = src->plane_bpp[0];
>> - size_t dst_stride = ALIGN(src->strides[0], 128);
>> -
>> - fb_id = igt_create_fb_with_bo_size(src->fd, src->width, src->height,
>> - src->drm_format,
>> - DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED,
>> - dst, 0, dst_stride);
>> - igt_assert(fb_id > 0);
>> -
>> - igt_assert(bpp == 16 || bpp == 32);
>> -
>> - src_buf = igt_fb_map_buffer(src->fd, src);
>> - igt_assert(src_buf);
>> -
>> - dst_buf = igt_fb_map_buffer(dst->fd, dst);
>> - igt_assert(dst_buf);
>> -
>> - for (i = 0; i < src->height; i++) {
>> - for (j = 0; j < src->width; j++) {
>> - size_t src_offset = src->offsets[0];
>> - size_t dst_offset = dst->offsets[0];
>> -
>> - src_offset += src->strides[0] * i + j * bpp / 8;
>> - dst_offset += igt_vc4_t_tiled_offset(dst_stride,
>> - src->height,
>> - bpp, j, i);
>> -
>> - switch (bpp) {
>> - case 16:
>> - *(uint16_t *)(dst_buf + dst_offset) =
>> - *(uint16_t *)(src_buf + src_offset);
>> - break;
>> - case 32:
>> - *(uint32_t *)(dst_buf + dst_offset) =
>> - *(uint32_t *)(src_buf + src_offset);
>> - break;
>> - }
>> - }
>> - }
>> -
>> - igt_fb_unmap_buffer(src, src_buf);
>> - igt_fb_unmap_buffer(dst, dst_buf);
>> -
>> - return fb_id;
>> -}
>> /* Calculate the t-tile width so that size = width * height * bpp / 8. */
>> #define VC4_T_TILE_W(size, height, bpp) ((size) / (height) / ((bpp) / 8))
>> -size_t igt_vc4_t_tiled_offset(size_t stride, size_t height, size_t bpp,
>> - size_t x, size_t y)
>> +static size_t igt_vc4_t_tiled_offset(size_t stride, size_t height, size_t bpp,
>> + size_t x, size_t y)
>> {
>> const size_t t1k_map_even[] = { 0, 3, 1, 2 };
>> const size_t t1k_map_odd[] = { 2, 1, 3, 0 };
>> @@ -308,18 +274,116 @@ size_t igt_vc4_t_tiled_offset(size_t stride, size_t height, size_t bpp,
>> return offset;
>> }
>> -static void vc4_fb_sand_tiled_convert_plane(struct igt_fb *dst, void *dst_buf,
>> +static void vc4_fb_convert_plane_to_t_tiled(struct igt_fb *dst, void *dst_buf,
>> struct igt_fb *src, void *src_buf,
>> - size_t column_width_bytes,
>> - size_t column_height,
>> unsigned int plane)
>> {
>> + size_t bpp = src->plane_bpp[plane];
>> + unsigned int i, j;
>> +
>> + for (i = 0; i < src->height; i++) {
>> + for (j = 0; j < src->width; j++) {
>> + size_t src_offset = src->offsets[plane];
>> + size_t dst_offset = dst->offsets[plane];
>> +
>> + src_offset += src->strides[plane] * i + j * bpp / 8;
>> + dst_offset += igt_vc4_t_tiled_offset(dst->strides[plane],
>> + dst->height,
>> + bpp, j, i);
>> +
>> + switch (bpp) {
>> + case 16:
>> + *(uint16_t *)(dst_buf + dst_offset) =
>> + *(uint16_t *)(src_buf + src_offset);
>> + break;
>> + case 32:
>> + *(uint32_t *)(dst_buf + dst_offset) =
>> + *(uint32_t *)(src_buf + src_offset);
>> + break;
>> + }
>> + }
>> + }
>> +}
>> +
>> +static void vc4_fb_convert_plane_from_t_tiled(struct igt_fb *dst, void *dst_buf,
>> + struct igt_fb *src, void *src_buf,
>> + unsigned int plane)
>> +{
>> + size_t bpp = src->plane_bpp[plane];
>> + unsigned int i, j;
>> +
>> + for (i = 0; i < src->height; i++) {
>> + for (j = 0; j < src->width; j++) {
>> + size_t src_offset = src->offsets[plane];
>> + size_t dst_offset = dst->offsets[plane];
>> +
>> + src_offset += igt_vc4_t_tiled_offset(src->strides[plane],
>> + src->height,
>> + bpp, j, i);
>> + src_offset += dst->strides[plane] * i + j * bpp / 8;
>> +
>> + switch (bpp) {
>> + case 16:
>> + *(uint16_t *)(dst_buf + dst_offset) =
>> + *(uint16_t *)(src_buf + src_offset);
>> + break;
>> + case 32:
>> + *(uint32_t *)(dst_buf + dst_offset) =
>> + *(uint32_t *)(src_buf + src_offset);
>> + break;
>> + }
>> + }
>> + }
>> +}
>> +
>> +static size_t vc4_sand_tiled_offset(size_t column_width, size_t column_size, size_t x,
>> + size_t y, size_t bpp)
>> +{
>> + size_t offset = 0;
>> + size_t cols_x;
>> + size_t pix_x;
>> +
>> + /* Offset to the beginning of the relevant column. */
>> + cols_x = x / column_width;
>> + offset += cols_x * column_size;
>> +
>> + /* Offset to the relevant pixel. */
>> + pix_x = x % column_width;
>> + offset += (column_width * y + pix_x) * bpp / 8;
>> +
>> + return offset;
>> +}
>> +
>> +static void vc4_fb_convert_plane_to_sand_tiled(struct igt_fb *dst, void *dst_buf,
>> + struct igt_fb *src, void *src_buf,
>> + unsigned int plane)
>> +{
>> + uint64_t modifier_base = fourcc_mod_broadcom_mod(dst->modifier);
>> + uint32_t column_height = fourcc_mod_broadcom_param(dst->modifier);
>> + uint32_t column_width_bytes, column_width, column_size;
>> size_t bpp = dst->plane_bpp[plane];
>> - size_t column_width = column_width_bytes * dst->plane_width[plane] /
>> - dst->width;
>> - size_t column_size = column_width_bytes * column_height;
>> unsigned int i, j;
>> + switch (modifier_base) {
>> + case DRM_FORMAT_MOD_BROADCOM_SAND32:
>> + column_width_bytes = 32;
>> + break;
>> + case DRM_FORMAT_MOD_BROADCOM_SAND64:
>> + column_width_bytes = 64;
>> + break;
>> + case DRM_FORMAT_MOD_BROADCOM_SAND128:
>> + column_width_bytes = 128;
>> + break;
>> + case DRM_FORMAT_MOD_BROADCOM_SAND256:
>> + column_width_bytes = 256;
>> + break;
>> + default:
>> + igt_assert(false);
>> + }
>> +
>> + column_width = column_width_bytes * dst->plane_width[plane] / dst->width;
>> + column_size = column_width_bytes * column_height;
>> +
>> for (i = 0; i < dst->plane_height[plane]; i++) {
>> for (j = 0; j < src->plane_width[plane]; j++) {
>> size_t src_offset = src->offsets[plane];
>> @@ -346,19 +410,15 @@ static void vc4_fb_sand_tiled_convert_plane(struct igt_fb *dst, void *dst_buf,
>> }
>> }
>> -unsigned int vc4_fb_sand_tiled_convert(struct igt_fb *dst, struct igt_fb *src,
>> - uint64_t modifier)
>> +static void vc4_fb_convert_plane_from_sand_tiled(struct igt_fb *dst, void *dst_buf,
>> + struct igt_fb *src, void *src_buf,
>> + unsigned int plane)
>> {
>> - uint64_t modifier_base;
>> - size_t column_width_bytes;
>> - size_t column_height;
>> - unsigned int fb_id;
>> - unsigned int i;
>> - void *src_buf;
>> - void *dst_buf;
>> -
>> - modifier_base = fourcc_mod_broadcom_mod(modifier);
>> - column_height = fourcc_mod_broadcom_param(modifier);
>> + uint64_t modifier_base = fourcc_mod_broadcom_mod(src->modifier);
>> + uint32_t column_height = fourcc_mod_broadcom_param(src->modifier);
>> + uint32_t column_width_bytes, column_width, column_size;
>> + size_t bpp = src->plane_bpp[plane];
>> + unsigned int i, j;
>> switch (modifier_base) {
>> case DRM_FORMAT_MOD_BROADCOM_SAND32:
>> @@ -377,41 +437,63 @@ unsigned int vc4_fb_sand_tiled_convert(struct igt_fb *dst, struct igt_fb *src,
>> igt_assert(false);
>> }
>> - fb_id = igt_create_fb(src->fd, src->width, src->height, src->drm_format,
>> - modifier, dst);
>> - igt_assert(fb_id > 0);
>> + column_width = column_width_bytes * src->plane_width[plane] / src->width;
>> + column_size = column_width_bytes * column_height;
>> +
>> + for (i = 0; i < dst->plane_height[plane]; i++) {
>> + for (j = 0; j < src->plane_width[plane]; j++) {
>> + size_t src_offset = src->offsets[plane];
>> + size_t dst_offset = dst->offsets[plane];
>> - src_buf = igt_fb_map_buffer(src->fd, src);
>> - igt_assert(src_buf);
>> + src_offset += vc4_sand_tiled_offset(column_width,
>> + column_size, j, i,
>> + bpp);
>> + dst_offset += dst->strides[plane] * i + j * bpp / 8;
>> - dst_buf = igt_fb_map_buffer(dst->fd, dst);
>> - igt_assert(dst_buf);
>> + switch (bpp) {
>> + case 8:
>> + *(uint8_t *)(dst_buf + dst_offset) =
>> + *(uint8_t *)(src_buf + src_offset);
>> + break;
>> + case 16:
>> + *(uint16_t *)(dst_buf + dst_offset) =
>> + *(uint16_t *)(src_buf + src_offset);
>> + break;
>> + default:
>> + igt_assert(false);
>> + }
>> + }
>> + }
>> +}
>> - for (i = 0; i < dst->num_planes; i++)
>> - vc4_fb_sand_tiled_convert_plane(dst, dst_buf, src, src_buf,
>> - column_width_bytes,
>> - column_height, i);
>> +void vc4_fb_convert_plane_to_tiled(struct igt_fb *dst, void *dst_buf,
>> + struct igt_fb *src, void *src_buf)
>> +{
>> + unsigned int plane;
>> - igt_fb_unmap_buffer(src, src_buf);
>> - igt_fb_unmap_buffer(dst, dst_buf);
>> + igt_assert(src->modifier == DRM_FORMAT_MOD_LINEAR);
>> + igt_assert(igt_vc4_is_tiled(dst->modifier));
>> - return fb_id;
>> + for (plane = 0; plane < src->num_planes; plane++) {
>> + if (dst->modifier == DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED)
>> + vc4_fb_convert_plane_to_t_tiled(dst, dst_buf, src, src_buf, plane);
>> + else
>> + vc4_fb_convert_plane_to_sand_tiled(dst, dst_buf, src, src_buf, plane);
>> + }
>> }
>> -size_t vc4_sand_tiled_offset(size_t column_width, size_t column_size, size_t x,
>> - size_t y, size_t bpp)
>> +void vc4_fb_convert_plane_from_tiled(struct igt_fb *dst, void *dst_buf,
>> + struct igt_fb *src, void *src_buf)
>> {
>> - size_t offset = 0;
>> - size_t cols_x;
>> - size_t pix_x;
>> -
>> - /* Offset to the beginning of the relevant column. */
>> - cols_x = x / column_width;
>> - offset += cols_x * column_size;
>> + unsigned int plane;
>> - /* Offset to the relevant pixel. */
>> - pix_x = x % column_width;
>> - offset += (column_width * y + pix_x) * bpp / 8;
>> + igt_assert(igt_vc4_is_tiled(src->modifier));
>> + igt_assert(dst->modifier == DRM_FORMAT_MOD_LINEAR);
>> - return offset;
>> + for (plane = 0; plane < src->num_planes; plane++) {
>> + if (src->modifier == DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED)
>> + vc4_fb_convert_plane_from_t_tiled(dst, dst_buf, src, src_buf, plane);
>> + else
>> + vc4_fb_convert_plane_from_sand_tiled(dst, dst_buf, src, src_buf, plane);
>> + }
>> }
>> diff --git a/lib/igt_vc4.h b/lib/igt_vc4.h
>> index a17812698fe5..f32bf398b3eb 100644
>> --- a/lib/igt_vc4.h
>> +++ b/lib/igt_vc4.h
>> @@ -29,16 +29,14 @@ int igt_vc4_create_bo(int fd, size_t size);
>> void *igt_vc4_mmap_bo(int fd, uint32_t handle, uint32_t size, unsigned prot);
>> int igt_vc4_get_param(int fd, uint32_t param, uint64_t *val);
>> bool igt_vc4_purgeable_bo(int fd, int handle, bool purgeable);
>> +bool igt_vc4_is_tiled(uint64_t modifier);
>> void igt_vc4_set_tiling(int fd, uint32_t handle, uint64_t modifier);
>> uint64_t igt_vc4_get_tiling(int fd, uint32_t handle);
>> -unsigned int igt_vc4_fb_t_tiled_convert(struct igt_fb *dst, struct igt_fb *src);
>> -size_t igt_vc4_t_tiled_offset(size_t stride, size_t height, size_t bpp,
>> - size_t x, size_t y);
>> -unsigned int vc4_fb_sand_tiled_convert(struct igt_fb *dst, struct igt_fb *src,
>> - uint64_t modifier);
>> -size_t vc4_sand_tiled_offset(size_t column_width, size_t column_size, size_t x,
>> - size_t y, size_t bpp);
>> +void vc4_fb_convert_plane_to_tiled(struct igt_fb *dst, void *dst_buf,
>> + struct igt_fb *src, void *src_buf);
>> +void vc4_fb_convert_plane_from_tiled(struct igt_fb *dst, void *dst_buf,
>> + struct igt_fb *src, void *src_buf);
>> #endif /* IGT_VC4_H */
>>
>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3.
2019-05-10 16:02 ` Maarten Lankhorst
@ 2019-05-14 6:34 ` Juha-Pekka Heikkila
2019-05-16 12:59 ` Maarten Lankhorst
0 siblings, 1 reply; 17+ messages in thread
From: Juha-Pekka Heikkila @ 2019-05-14 6:34 UTC (permalink / raw)
To: Maarten Lankhorst, Paul Kocialkowski, igt-dev
This change look ok to me. To me it look vc4 tiling handling stay on
same level as what it was before as what Paul was worried about.
Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
On 10.5.2019 19.02, Maarten Lankhorst wrote:
> Op 10-05-2019 om 17:10 schreef Paul Kocialkowski:
>> Hi,
>>
>> On Fri, 2019-05-10 at 16:19 +0200, Maarten Lankhorst wrote:
>>> Op 10-05-2019 om 15:43 schreef Paul Kocialkowski:
>>>> Hi,
>>>>
>>>> It looks like I fell behind on reviewing earlier version here, sorry
>>>> about that.
>>>>
>>>> On Fri, 2019-05-10 at 14:33 +0200, Maarten Lankhorst wrote:
>>>>> Ever since commit 3fa65f4b532bd9a5b ("fb: Add support for conversions
>>>>> through pixman") we can generate a valid cairo surface for any plane,
>>>>> use this to avoid having to implement our own conversion routine.
>>>>>
>>>>> Instead of duplicating this functionality in igt_fb_convert_with_stride,
>>>>> we can simply convert this to a few cairo calls, because we now support
>>>>> cairo calls to any of the supported framebuffer formats.
>>>> I don't see how that is the case. Cairo definitely does not support our
>>>> tiled formats, so we can't just pipe them into it.
>>>>
>>>> And we already use pixman for converting through the fb_convert call to
>>>> convert_pixman when possible. Also, my understanding is that cairo is
>>>> very limited format-wise, so we're much better off using pixman
>>>> directly (which is is what a non-accelerated cairo surface will do
>>>> anyway).
>>>>
>>>>> This is required to make this function more generic, and convert from any
>>>>> format/modifier to any other format/modifier.
>>>> We've already designed it to be generic, we just need conversion
>>>> helpers from/to (currently only to) hardware-specific tiling modifiers.
>>>>
>>>> We can't expect cairo or pixman to do that job and this change pretty
>>>> much kills support for the vc4 tiling modes we've added.
>>>>
>>>> Unless I'm missing something, that's a NACK on my side.
>>> You have a function that can convert a untiled fb to tiled, but not
>>> the other way around in igt_vc4.c
>> We don't need it for anything at the moment but it wouldn't be a
>> problem to come up with one, sure.
>>
>>> If you could provide that, we could convert from any fb/modifier
>>> format to any fb/modifier format, even with VC4 tiling.
>> I don't fully get the point of that if our tests are not going to use
>> it, but it doesn't hurt either :)
>>
>>> Our internal cairo hooks already provide helpers for conversion, see
>>> igt_get_cairo_surface() which calls
>>> create_cairo_surface__convert()
>> My feeling is that we should kill use of cairo formats and go with
>> pixman directly.
>>
>>> Some conversions can only be done through cairo, converting between
>>> P01x and XRGB8888 cannot be done directly,
>>> our pixman representation for that are done in the floating point
>>> formats.
>> I'm not sure I'm following this point, but if there's a conversion that
>> cairo can do and pixman can't, it feels like the right thing would be
>> to fix pixman to support it. Is there a reason in particular why this
>> wouldn't work?
>>
>>> The only way to convert between some framebuffer formats and
>>> modifiers in i915 is by using those conversion functions,
>>> the fact that igt_fb_convert_with_stride doesn't do those, makes a
>>> truly random hdmi-cmp-planes-random useless.
>> So it boils down to a missing format support issue, not really to an
>> issue with the existing flow.
>>
>>> I was getting CRC mismatches because the i915 modifiers weren't
>>> respected. When I made sure they were being respected
>>> I ended up with a duplicate of the cairo context stuff. So why not
>>> lose a little speed and use that?
>> My counter-suggestion would be to do the opposite and make sure pixman
>> can deal with these cases on its own.
>>
>>> If you write and test a detiler function, I can hook it up for you.
>>> :)
>>> If necessary I can do it myself, to move this patch forward.
>>>
>>> Cairo shouldn't be much slower than pixman, because internally it
>>> already uses pixman calls for the backend.
>> Sure, but that's not cairo's role: cairo is about drawing, while pixman
>> is about pixel conversion. I think it would be best to stick to that.
>>
>> Cheers,
>>
>> Paul
>>
> What about this?
>
> We could draw reference FB's in vc4 t tiled formats now too..
>
> ---8<----
> From 7242edd2cd0096556080e0cda8d4ecea4e3fc58d Mon Sep 17 00:00:00 2001
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Date: Tue, 2 Apr 2019 16:54:30 +0200
> Subject: [PATCH i-g-t] lib/igt_fb: Use cairo conversion in
> igt_fb_convert_with_stride, v4.
>
> Ever since commit 3fa65f4b532bd9a5b ("fb: Add support for conversions
> through pixman") we can generate a valid cairo surface for any plane,
> use this to avoid having to implement our own conversion routine.
>
> Instead of duplicating this functionality in igt_fb_convert_with_stride,
> we can simply convert this to a few cairo calls, because we now support
> cairo calls to any of the supported framebuffer formats.
>
> This is required to make this function more generic, and convert from any
> format/modifier to any other format/modifier.
>
> Changes since v1:
> - Return fb_id in the cairo case.
> Changes since v2:
> - Remove the manual conversion fallback.
> Changes since v3:
> - Integrate VC4 conversion routines.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
> lib/igt_fb.c | 129 +++++++++++------------
> lib/igt_vc4.c | 278 ++++++++++++++++++++++++++++++++------------------
> lib/igt_vc4.h | 12 +--
> 3 files changed, 242 insertions(+), 177 deletions(-)
>
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index d4929019971c..c6e18397b754 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -1716,17 +1716,25 @@ static void free_linear_mapping(struct fb_blit_upload *blit)
> struct igt_fb *fb = blit->fb;
> struct fb_blit_linear *linear = &blit->linear;
>
> - gem_munmap(linear->map, linear->fb.size);
> - gem_set_domain(fd, linear->fb.gem_handle,
> - I915_GEM_DOMAIN_GTT, 0);
> + if (igt_vc4_is_tiled(fb->modifier)) {
> + void *map = igt_vc4_mmap_bo(fd, fb->gem_handle, fb->size, PROT_WRITE);
>
> - if (blit->batch)
> - rendercopy(blit, fb, &linear->fb);
> - else
> - blitcopy(fb, &linear->fb);
> + vc4_fb_convert_plane_to_tiled(fb, map, &linear->fb, &linear->map);
> +
> + munmap(map, fb->size);
> + } else {
> + gem_munmap(linear->map, linear->fb.size);
> + gem_set_domain(fd, linear->fb.gem_handle,
> + I915_GEM_DOMAIN_GTT, 0);
> +
> + if (blit->batch)
> + rendercopy(blit, fb, &linear->fb);
> + else
> + blitcopy(fb, &linear->fb);
>
> - gem_sync(fd, linear->fb.gem_handle);
> - gem_close(fd, linear->fb.gem_handle);
> + gem_sync(fd, linear->fb.gem_handle);
> + gem_close(fd, linear->fb.gem_handle);
> + }
>
> if (blit->batch) {
> intel_batchbuffer_free(blit->batch);
> @@ -1751,7 +1759,7 @@ static void setup_linear_mapping(struct fb_blit_upload *blit)
> struct igt_fb *fb = blit->fb;
> struct fb_blit_linear *linear = &blit->linear;
>
> - if (use_rendercopy(fb)) {
> + if (!igt_vc4_is_tiled(fb->modifier) && use_rendercopy(fb)) {
> blit->bufmgr = drm_intel_bufmgr_gem_init(fd, 4096);
> blit->batch = intel_batchbuffer_alloc(blit->bufmgr,
> intel_get_drm_devid(fd));
> @@ -1771,23 +1779,35 @@ static void setup_linear_mapping(struct fb_blit_upload *blit)
>
> igt_assert(linear->fb.gem_handle > 0);
>
> - /* Copy fb content to linear BO */
> - gem_set_domain(fd, linear->fb.gem_handle,
> - I915_GEM_DOMAIN_GTT, 0);
> + if (igt_vc4_is_tiled(fb->modifier)) {
> + void *map = igt_vc4_mmap_bo(fd, fb->gem_handle, fb->size, PROT_READ);
>
> - if (blit->batch)
> - rendercopy(blit, &linear->fb, fb);
> - else
> - blitcopy(&linear->fb, fb);
> + linear->map = igt_vc4_mmap_bo(fd, linear->fb.gem_handle,
> + linear->fb.size,
> + PROT_READ | PROT_WRITE);
>
> - gem_sync(fd, linear->fb.gem_handle);
> + vc4_fb_convert_plane_from_tiled(&linear->fb, &linear->map, fb, map);
>
> - gem_set_domain(fd, linear->fb.gem_handle,
> - I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
> + munmap(map, fb->size);
> + } else {
> + /* Copy fb content to linear BO */
> + gem_set_domain(fd, linear->fb.gem_handle,
> + I915_GEM_DOMAIN_GTT, 0);
> +
> + if (blit->batch)
> + rendercopy(blit, &linear->fb, fb);
> + else
> + blitcopy(&linear->fb, fb);
> +
> + gem_sync(fd, linear->fb.gem_handle);
> +
> + gem_set_domain(fd, linear->fb.gem_handle,
> + I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
>
> - /* Setup cairo context */
> - linear->map = gem_mmap__cpu(fd, linear->fb.gem_handle,
> - 0, linear->fb.size, PROT_READ | PROT_WRITE);
> + /* Setup cairo context */
> + linear->map = gem_mmap__cpu(fd, linear->fb.gem_handle,
> + 0, linear->fb.size, PROT_READ | PROT_WRITE);
> + }
> }
>
> static void create_cairo_surface__gpu(int fd, struct igt_fb *fb)
> @@ -2902,7 +2922,7 @@ static void create_cairo_surface__convert(int fd, struct igt_fb *fb)
> &blit->shadow_fb);
> igt_assert(blit->shadow_ptr);
>
> - if (use_rendercopy(fb) || use_blitter(fb)) {
> + if (use_rendercopy(fb) || use_blitter(fb) || igt_vc4_is_tiled(fb->modifier)) {
> setup_linear_mapping(&blit->base);
> } else {
> blit->base.linear.fb = *fb;
> @@ -2983,7 +3003,7 @@ cairo_surface_t *igt_get_cairo_surface(int fd, struct igt_fb *fb)
> ((f->cairo_id == CAIRO_FORMAT_INVALID) &&
> (f->pixman_id != PIXMAN_invalid)))
> create_cairo_surface__convert(fd, fb);
> - else if (use_blitter(fb) || use_rendercopy(fb))
> + else if (use_blitter(fb) || use_rendercopy(fb) || igt_vc4_is_tiled(fb->modifier))
> create_cairo_surface__gpu(fd, fb);
> else
> create_cairo_surface__gtt(fd, fb);
> @@ -3102,58 +3122,23 @@ unsigned int igt_fb_convert_with_stride(struct igt_fb *dst, struct igt_fb *src,
> uint64_t dst_modifier,
> unsigned int dst_stride)
> {
> - struct fb_convert cvt = { };
> - struct igt_fb linear;
> - void *dst_ptr, *src_ptr;
> - uint64_t base_modifier;
> + /* Use the cairo api to convert */
> + cairo_surface_t *surf = igt_get_cairo_surface(src->fd, src);
> + cairo_t *cr;
> int fb_id;
>
> - if (is_vc4_device(src->fd))
> - base_modifier = fourcc_mod_broadcom_mod(dst_modifier);
> - else
> - base_modifier = dst_modifier;
> -
> - fb_id = igt_create_fb_with_bo_size(src->fd, src->width, src->height,
> - dst_fourcc,
> - LOCAL_DRM_FORMAT_MOD_NONE, &linear,
> - 0, dst_stride);
> + fb_id = igt_create_fb_with_bo_size(src->fd, src->width,
> + src->height, dst_fourcc,
> + dst_modifier, dst, 0,
> + dst_stride);
> igt_assert(fb_id > 0);
>
> - src_ptr = igt_fb_map_buffer(src->fd, src);
> - igt_assert(src_ptr);
> -
> - dst_ptr = igt_fb_map_buffer(linear.fd, &linear);
> - igt_assert(dst_ptr);
> -
> - cvt.dst.ptr = dst_ptr;
> - cvt.dst.fb = &linear;
> - cvt.src.ptr = src_ptr;
> - cvt.src.fb = src;
> - fb_convert(&cvt);
> -
> - igt_fb_unmap_buffer(dst, dst_ptr);
> - igt_fb_unmap_buffer(src, src_ptr);
> -
> - switch (base_modifier) {
> - case DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED:
> - fb_id = igt_vc4_fb_t_tiled_convert(dst, &linear);
> - break;
> - case DRM_FORMAT_MOD_BROADCOM_SAND32:
> - case DRM_FORMAT_MOD_BROADCOM_SAND64:
> - case DRM_FORMAT_MOD_BROADCOM_SAND128:
> - case DRM_FORMAT_MOD_BROADCOM_SAND256:
> - fb_id = vc4_fb_sand_tiled_convert(dst, &linear, dst_modifier);
> - break;
> - default:
> - igt_assert(dst_modifier == LOCAL_DRM_FORMAT_MOD_NONE);
> - }
> -
> - igt_assert(fb_id > 0);
> + cr = igt_get_cairo_ctx(dst->fd, dst);
> + cairo_set_source_surface(cr, surf, 0, 0);
> + cairo_paint(cr);
> + igt_put_cairo_ctx(dst->fd, dst, cr);
>
> - if (dst_modifier == LOCAL_DRM_FORMAT_MOD_NONE)
> - *dst = linear;
> - else
> - igt_remove_fb(linear.fd, &linear);
> + cairo_surface_destroy(surf);
>
> return fb_id;
> }
> diff --git a/lib/igt_vc4.c b/lib/igt_vc4.c
> index 9a0ba30b4420..4415fa321ceb 100644
> --- a/lib/igt_vc4.c
> +++ b/lib/igt_vc4.c
> @@ -56,6 +56,23 @@
> * tests.
> */
>
> +bool igt_vc4_is_tiled(uint64_t modifier)
> +{
> + if (modifier >> 56ULL != DRM_FORMAT_MOD_VENDOR_BROADCOM)
> + return false;
> +
> + switch (fourcc_mod_broadcom_mod(modifier)) {
> + case DRM_FORMAT_MOD_BROADCOM_SAND32:
> + case DRM_FORMAT_MOD_BROADCOM_SAND64:
> + case DRM_FORMAT_MOD_BROADCOM_SAND128:
> + case DRM_FORMAT_MOD_BROADCOM_SAND256:
> + case DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> /**
> * igt_vc4_get_cleared_bo:
> * @fd: device file descriptor
> @@ -178,63 +195,12 @@ bool igt_vc4_purgeable_bo(int fd, int handle, bool purgeable)
> return arg.retained;
> }
>
> -unsigned int igt_vc4_fb_t_tiled_convert(struct igt_fb *dst, struct igt_fb *src)
> -{
> - unsigned int fb_id;
> - unsigned int i, j;
> - void *src_buf;
> - void *dst_buf;
> - size_t bpp = src->plane_bpp[0];
> - size_t dst_stride = ALIGN(src->strides[0], 128);
> -
> - fb_id = igt_create_fb_with_bo_size(src->fd, src->width, src->height,
> - src->drm_format,
> - DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED,
> - dst, 0, dst_stride);
> - igt_assert(fb_id > 0);
> -
> - igt_assert(bpp == 16 || bpp == 32);
> -
> - src_buf = igt_fb_map_buffer(src->fd, src);
> - igt_assert(src_buf);
> -
> - dst_buf = igt_fb_map_buffer(dst->fd, dst);
> - igt_assert(dst_buf);
> -
> - for (i = 0; i < src->height; i++) {
> - for (j = 0; j < src->width; j++) {
> - size_t src_offset = src->offsets[0];
> - size_t dst_offset = dst->offsets[0];
> -
> - src_offset += src->strides[0] * i + j * bpp / 8;
> - dst_offset += igt_vc4_t_tiled_offset(dst_stride,
> - src->height,
> - bpp, j, i);
> -
> - switch (bpp) {
> - case 16:
> - *(uint16_t *)(dst_buf + dst_offset) =
> - *(uint16_t *)(src_buf + src_offset);
> - break;
> - case 32:
> - *(uint32_t *)(dst_buf + dst_offset) =
> - *(uint32_t *)(src_buf + src_offset);
> - break;
> - }
> - }
> - }
> -
> - igt_fb_unmap_buffer(src, src_buf);
> - igt_fb_unmap_buffer(dst, dst_buf);
> -
> - return fb_id;
> -}
>
> /* Calculate the t-tile width so that size = width * height * bpp / 8. */
> #define VC4_T_TILE_W(size, height, bpp) ((size) / (height) / ((bpp) / 8))
>
> -size_t igt_vc4_t_tiled_offset(size_t stride, size_t height, size_t bpp,
> - size_t x, size_t y)
> +static size_t igt_vc4_t_tiled_offset(size_t stride, size_t height, size_t bpp,
> + size_t x, size_t y)
> {
> const size_t t1k_map_even[] = { 0, 3, 1, 2 };
> const size_t t1k_map_odd[] = { 2, 1, 3, 0 };
> @@ -308,18 +274,116 @@ size_t igt_vc4_t_tiled_offset(size_t stride, size_t height, size_t bpp,
> return offset;
> }
>
> -static void vc4_fb_sand_tiled_convert_plane(struct igt_fb *dst, void *dst_buf,
> +static void vc4_fb_convert_plane_to_t_tiled(struct igt_fb *dst, void *dst_buf,
> struct igt_fb *src, void *src_buf,
> - size_t column_width_bytes,
> - size_t column_height,
> unsigned int plane)
> {
> + size_t bpp = src->plane_bpp[plane];
> + unsigned int i, j;
> +
> + for (i = 0; i < src->height; i++) {
> + for (j = 0; j < src->width; j++) {
> + size_t src_offset = src->offsets[plane];
> + size_t dst_offset = dst->offsets[plane];
> +
> + src_offset += src->strides[plane] * i + j * bpp / 8;
> + dst_offset += igt_vc4_t_tiled_offset(dst->strides[plane],
> + dst->height,
> + bpp, j, i);
> +
> + switch (bpp) {
> + case 16:
> + *(uint16_t *)(dst_buf + dst_offset) =
> + *(uint16_t *)(src_buf + src_offset);
> + break;
> + case 32:
> + *(uint32_t *)(dst_buf + dst_offset) =
> + *(uint32_t *)(src_buf + src_offset);
> + break;
> + }
> + }
> + }
> +}
> +
> +static void vc4_fb_convert_plane_from_t_tiled(struct igt_fb *dst, void *dst_buf,
> + struct igt_fb *src, void *src_buf,
> + unsigned int plane)
> +{
> + size_t bpp = src->plane_bpp[plane];
> + unsigned int i, j;
> +
> + for (i = 0; i < src->height; i++) {
> + for (j = 0; j < src->width; j++) {
> + size_t src_offset = src->offsets[plane];
> + size_t dst_offset = dst->offsets[plane];
> +
> + src_offset += igt_vc4_t_tiled_offset(src->strides[plane],
> + src->height,
> + bpp, j, i);
> + src_offset += dst->strides[plane] * i + j * bpp / 8;
> +
> + switch (bpp) {
> + case 16:
> + *(uint16_t *)(dst_buf + dst_offset) =
> + *(uint16_t *)(src_buf + src_offset);
> + break;
> + case 32:
> + *(uint32_t *)(dst_buf + dst_offset) =
> + *(uint32_t *)(src_buf + src_offset);
> + break;
> + }
> + }
> + }
> +}
> +
> +static size_t vc4_sand_tiled_offset(size_t column_width, size_t column_size, size_t x,
> + size_t y, size_t bpp)
> +{
> + size_t offset = 0;
> + size_t cols_x;
> + size_t pix_x;
> +
> + /* Offset to the beginning of the relevant column. */
> + cols_x = x / column_width;
> + offset += cols_x * column_size;
> +
> + /* Offset to the relevant pixel. */
> + pix_x = x % column_width;
> + offset += (column_width * y + pix_x) * bpp / 8;
> +
> + return offset;
> +}
> +
> +static void vc4_fb_convert_plane_to_sand_tiled(struct igt_fb *dst, void *dst_buf,
> + struct igt_fb *src, void *src_buf,
> + unsigned int plane)
> +{
> + uint64_t modifier_base = fourcc_mod_broadcom_mod(dst->modifier);
> + uint32_t column_height = fourcc_mod_broadcom_param(dst->modifier);
> + uint32_t column_width_bytes, column_width, column_size;
> size_t bpp = dst->plane_bpp[plane];
> - size_t column_width = column_width_bytes * dst->plane_width[plane] /
> - dst->width;
> - size_t column_size = column_width_bytes * column_height;
> unsigned int i, j;
>
> + switch (modifier_base) {
> + case DRM_FORMAT_MOD_BROADCOM_SAND32:
> + column_width_bytes = 32;
> + break;
> + case DRM_FORMAT_MOD_BROADCOM_SAND64:
> + column_width_bytes = 64;
> + break;
> + case DRM_FORMAT_MOD_BROADCOM_SAND128:
> + column_width_bytes = 128;
> + break;
> + case DRM_FORMAT_MOD_BROADCOM_SAND256:
> + column_width_bytes = 256;
> + break;
> + default:
> + igt_assert(false);
> + }
> +
> + column_width = column_width_bytes * dst->plane_width[plane] / dst->width;
> + column_size = column_width_bytes * column_height;
> +
> for (i = 0; i < dst->plane_height[plane]; i++) {
> for (j = 0; j < src->plane_width[plane]; j++) {
> size_t src_offset = src->offsets[plane];
> @@ -346,19 +410,15 @@ static void vc4_fb_sand_tiled_convert_plane(struct igt_fb *dst, void *dst_buf,
> }
> }
>
> -unsigned int vc4_fb_sand_tiled_convert(struct igt_fb *dst, struct igt_fb *src,
> - uint64_t modifier)
> +static void vc4_fb_convert_plane_from_sand_tiled(struct igt_fb *dst, void *dst_buf,
> + struct igt_fb *src, void *src_buf,
> + unsigned int plane)
> {
> - uint64_t modifier_base;
> - size_t column_width_bytes;
> - size_t column_height;
> - unsigned int fb_id;
> - unsigned int i;
> - void *src_buf;
> - void *dst_buf;
> -
> - modifier_base = fourcc_mod_broadcom_mod(modifier);
> - column_height = fourcc_mod_broadcom_param(modifier);
> + uint64_t modifier_base = fourcc_mod_broadcom_mod(src->modifier);
> + uint32_t column_height = fourcc_mod_broadcom_param(src->modifier);
> + uint32_t column_width_bytes, column_width, column_size;
> + size_t bpp = src->plane_bpp[plane];
> + unsigned int i, j;
>
> switch (modifier_base) {
> case DRM_FORMAT_MOD_BROADCOM_SAND32:
> @@ -377,41 +437,63 @@ unsigned int vc4_fb_sand_tiled_convert(struct igt_fb *dst, struct igt_fb *src,
> igt_assert(false);
> }
>
> - fb_id = igt_create_fb(src->fd, src->width, src->height, src->drm_format,
> - modifier, dst);
> - igt_assert(fb_id > 0);
> + column_width = column_width_bytes * src->plane_width[plane] / src->width;
> + column_size = column_width_bytes * column_height;
> +
> + for (i = 0; i < dst->plane_height[plane]; i++) {
> + for (j = 0; j < src->plane_width[plane]; j++) {
> + size_t src_offset = src->offsets[plane];
> + size_t dst_offset = dst->offsets[plane];
>
> - src_buf = igt_fb_map_buffer(src->fd, src);
> - igt_assert(src_buf);
> + src_offset += vc4_sand_tiled_offset(column_width,
> + column_size, j, i,
> + bpp);
> + dst_offset += dst->strides[plane] * i + j * bpp / 8;
>
> - dst_buf = igt_fb_map_buffer(dst->fd, dst);
> - igt_assert(dst_buf);
> + switch (bpp) {
> + case 8:
> + *(uint8_t *)(dst_buf + dst_offset) =
> + *(uint8_t *)(src_buf + src_offset);
> + break;
> + case 16:
> + *(uint16_t *)(dst_buf + dst_offset) =
> + *(uint16_t *)(src_buf + src_offset);
> + break;
> + default:
> + igt_assert(false);
> + }
> + }
> + }
> +}
>
> - for (i = 0; i < dst->num_planes; i++)
> - vc4_fb_sand_tiled_convert_plane(dst, dst_buf, src, src_buf,
> - column_width_bytes,
> - column_height, i);
> +void vc4_fb_convert_plane_to_tiled(struct igt_fb *dst, void *dst_buf,
> + struct igt_fb *src, void *src_buf)
> +{
> + unsigned int plane;
>
> - igt_fb_unmap_buffer(src, src_buf);
> - igt_fb_unmap_buffer(dst, dst_buf);
> + igt_assert(src->modifier == DRM_FORMAT_MOD_LINEAR);
> + igt_assert(igt_vc4_is_tiled(dst->modifier));
>
> - return fb_id;
> + for (plane = 0; plane < src->num_planes; plane++) {
> + if (dst->modifier == DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED)
> + vc4_fb_convert_plane_to_t_tiled(dst, dst_buf, src, src_buf, plane);
> + else
> + vc4_fb_convert_plane_to_sand_tiled(dst, dst_buf, src, src_buf, plane);
> + }
> }
>
> -size_t vc4_sand_tiled_offset(size_t column_width, size_t column_size, size_t x,
> - size_t y, size_t bpp)
> +void vc4_fb_convert_plane_from_tiled(struct igt_fb *dst, void *dst_buf,
> + struct igt_fb *src, void *src_buf)
> {
> - size_t offset = 0;
> - size_t cols_x;
> - size_t pix_x;
> -
> - /* Offset to the beginning of the relevant column. */
> - cols_x = x / column_width;
> - offset += cols_x * column_size;
> + unsigned int plane;
>
> - /* Offset to the relevant pixel. */
> - pix_x = x % column_width;
> - offset += (column_width * y + pix_x) * bpp / 8;
> + igt_assert(igt_vc4_is_tiled(src->modifier));
> + igt_assert(dst->modifier == DRM_FORMAT_MOD_LINEAR);
>
> - return offset;
> + for (plane = 0; plane < src->num_planes; plane++) {
> + if (src->modifier == DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED)
> + vc4_fb_convert_plane_from_t_tiled(dst, dst_buf, src, src_buf, plane);
> + else
> + vc4_fb_convert_plane_from_sand_tiled(dst, dst_buf, src, src_buf, plane);
> + }
> }
> diff --git a/lib/igt_vc4.h b/lib/igt_vc4.h
> index a17812698fe5..f32bf398b3eb 100644
> --- a/lib/igt_vc4.h
> +++ b/lib/igt_vc4.h
> @@ -29,16 +29,14 @@ int igt_vc4_create_bo(int fd, size_t size);
> void *igt_vc4_mmap_bo(int fd, uint32_t handle, uint32_t size, unsigned prot);
> int igt_vc4_get_param(int fd, uint32_t param, uint64_t *val);
> bool igt_vc4_purgeable_bo(int fd, int handle, bool purgeable);
> +bool igt_vc4_is_tiled(uint64_t modifier);
>
> void igt_vc4_set_tiling(int fd, uint32_t handle, uint64_t modifier);
> uint64_t igt_vc4_get_tiling(int fd, uint32_t handle);
>
> -unsigned int igt_vc4_fb_t_tiled_convert(struct igt_fb *dst, struct igt_fb *src);
> -size_t igt_vc4_t_tiled_offset(size_t stride, size_t height, size_t bpp,
> - size_t x, size_t y);
> -unsigned int vc4_fb_sand_tiled_convert(struct igt_fb *dst, struct igt_fb *src,
> - uint64_t modifier);
> -size_t vc4_sand_tiled_offset(size_t column_width, size_t column_size, size_t x,
> - size_t y, size_t bpp);
> +void vc4_fb_convert_plane_to_tiled(struct igt_fb *dst, void *dst_buf,
> + struct igt_fb *src, void *src_buf);
> +void vc4_fb_convert_plane_from_tiled(struct igt_fb *dst, void *dst_buf,
> + struct igt_fb *src, void *src_buf);
>
> #endif /* IGT_VC4_H */
>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3.
2019-05-10 15:10 ` Paul Kocialkowski
@ 2019-05-10 16:02 ` Maarten Lankhorst
2019-05-14 6:34 ` Juha-Pekka Heikkila
0 siblings, 1 reply; 17+ messages in thread
From: Maarten Lankhorst @ 2019-05-10 16:02 UTC (permalink / raw)
To: Paul Kocialkowski, igt-dev
Op 10-05-2019 om 17:10 schreef Paul Kocialkowski:
> Hi,
>
> On Fri, 2019-05-10 at 16:19 +0200, Maarten Lankhorst wrote:
>> Op 10-05-2019 om 15:43 schreef Paul Kocialkowski:
>>> Hi,
>>>
>>> It looks like I fell behind on reviewing earlier version here, sorry
>>> about that.
>>>
>>> On Fri, 2019-05-10 at 14:33 +0200, Maarten Lankhorst wrote:
>>>> Ever since commit 3fa65f4b532bd9a5b ("fb: Add support for conversions
>>>> through pixman") we can generate a valid cairo surface for any plane,
>>>> use this to avoid having to implement our own conversion routine.
>>>>
>>>> Instead of duplicating this functionality in igt_fb_convert_with_stride,
>>>> we can simply convert this to a few cairo calls, because we now support
>>>> cairo calls to any of the supported framebuffer formats.
>>> I don't see how that is the case. Cairo definitely does not support our
>>> tiled formats, so we can't just pipe them into it.
>>>
>>> And we already use pixman for converting through the fb_convert call to
>>> convert_pixman when possible. Also, my understanding is that cairo is
>>> very limited format-wise, so we're much better off using pixman
>>> directly (which is is what a non-accelerated cairo surface will do
>>> anyway).
>>>
>>>> This is required to make this function more generic, and convert from any
>>>> format/modifier to any other format/modifier.
>>> We've already designed it to be generic, we just need conversion
>>> helpers from/to (currently only to) hardware-specific tiling modifiers.
>>>
>>> We can't expect cairo or pixman to do that job and this change pretty
>>> much kills support for the vc4 tiling modes we've added.
>>>
>>> Unless I'm missing something, that's a NACK on my side.
>> You have a function that can convert a untiled fb to tiled, but not
>> the other way around in igt_vc4.c
> We don't need it for anything at the moment but it wouldn't be a
> problem to come up with one, sure.
>
>> If you could provide that, we could convert from any fb/modifier
>> format to any fb/modifier format, even with VC4 tiling.
> I don't fully get the point of that if our tests are not going to use
> it, but it doesn't hurt either :)
>
>> Our internal cairo hooks already provide helpers for conversion, see
>> igt_get_cairo_surface() which calls
>> create_cairo_surface__convert()
> My feeling is that we should kill use of cairo formats and go with
> pixman directly.
>
>> Some conversions can only be done through cairo, converting between
>> P01x and XRGB8888 cannot be done directly,
>> our pixman representation for that are done in the floating point
>> formats.
> I'm not sure I'm following this point, but if there's a conversion that
> cairo can do and pixman can't, it feels like the right thing would be
> to fix pixman to support it. Is there a reason in particular why this
> wouldn't work?
>
>> The only way to convert between some framebuffer formats and
>> modifiers in i915 is by using those conversion functions,
>> the fact that igt_fb_convert_with_stride doesn't do those, makes a
>> truly random hdmi-cmp-planes-random useless.
> So it boils down to a missing format support issue, not really to an
> issue with the existing flow.
>
>> I was getting CRC mismatches because the i915 modifiers weren't
>> respected. When I made sure they were being respected
>> I ended up with a duplicate of the cairo context stuff. So why not
>> lose a little speed and use that?
> My counter-suggestion would be to do the opposite and make sure pixman
> can deal with these cases on its own.
>
>> If you write and test a detiler function, I can hook it up for you.
>> :)
>> If necessary I can do it myself, to move this patch forward.
>>
>> Cairo shouldn't be much slower than pixman, because internally it
>> already uses pixman calls for the backend.
> Sure, but that's not cairo's role: cairo is about drawing, while pixman
> is about pixel conversion. I think it would be best to stick to that.
>
> Cheers,
>
> Paul
>
What about this?
We could draw reference FB's in vc4 t tiled formats now too..
---8<----
From 7242edd2cd0096556080e0cda8d4ecea4e3fc58d Mon Sep 17 00:00:00 2001
From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date: Tue, 2 Apr 2019 16:54:30 +0200
Subject: [PATCH i-g-t] lib/igt_fb: Use cairo conversion in
igt_fb_convert_with_stride, v4.
Ever since commit 3fa65f4b532bd9a5b ("fb: Add support for conversions
through pixman") we can generate a valid cairo surface for any plane,
use this to avoid having to implement our own conversion routine.
Instead of duplicating this functionality in igt_fb_convert_with_stride,
we can simply convert this to a few cairo calls, because we now support
cairo calls to any of the supported framebuffer formats.
This is required to make this function more generic, and convert from any
format/modifier to any other format/modifier.
Changes since v1:
- Return fb_id in the cairo case.
Changes since v2:
- Remove the manual conversion fallback.
Changes since v3:
- Integrate VC4 conversion routines.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
lib/igt_fb.c | 129 +++++++++++------------
lib/igt_vc4.c | 278 ++++++++++++++++++++++++++++++++------------------
lib/igt_vc4.h | 12 +--
3 files changed, 242 insertions(+), 177 deletions(-)
diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index d4929019971c..c6e18397b754 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -1716,17 +1716,25 @@ static void free_linear_mapping(struct fb_blit_upload *blit)
struct igt_fb *fb = blit->fb;
struct fb_blit_linear *linear = &blit->linear;
- gem_munmap(linear->map, linear->fb.size);
- gem_set_domain(fd, linear->fb.gem_handle,
- I915_GEM_DOMAIN_GTT, 0);
+ if (igt_vc4_is_tiled(fb->modifier)) {
+ void *map = igt_vc4_mmap_bo(fd, fb->gem_handle, fb->size, PROT_WRITE);
- if (blit->batch)
- rendercopy(blit, fb, &linear->fb);
- else
- blitcopy(fb, &linear->fb);
+ vc4_fb_convert_plane_to_tiled(fb, map, &linear->fb, &linear->map);
+
+ munmap(map, fb->size);
+ } else {
+ gem_munmap(linear->map, linear->fb.size);
+ gem_set_domain(fd, linear->fb.gem_handle,
+ I915_GEM_DOMAIN_GTT, 0);
+
+ if (blit->batch)
+ rendercopy(blit, fb, &linear->fb);
+ else
+ blitcopy(fb, &linear->fb);
- gem_sync(fd, linear->fb.gem_handle);
- gem_close(fd, linear->fb.gem_handle);
+ gem_sync(fd, linear->fb.gem_handle);
+ gem_close(fd, linear->fb.gem_handle);
+ }
if (blit->batch) {
intel_batchbuffer_free(blit->batch);
@@ -1751,7 +1759,7 @@ static void setup_linear_mapping(struct fb_blit_upload *blit)
struct igt_fb *fb = blit->fb;
struct fb_blit_linear *linear = &blit->linear;
- if (use_rendercopy(fb)) {
+ if (!igt_vc4_is_tiled(fb->modifier) && use_rendercopy(fb)) {
blit->bufmgr = drm_intel_bufmgr_gem_init(fd, 4096);
blit->batch = intel_batchbuffer_alloc(blit->bufmgr,
intel_get_drm_devid(fd));
@@ -1771,23 +1779,35 @@ static void setup_linear_mapping(struct fb_blit_upload *blit)
igt_assert(linear->fb.gem_handle > 0);
- /* Copy fb content to linear BO */
- gem_set_domain(fd, linear->fb.gem_handle,
- I915_GEM_DOMAIN_GTT, 0);
+ if (igt_vc4_is_tiled(fb->modifier)) {
+ void *map = igt_vc4_mmap_bo(fd, fb->gem_handle, fb->size, PROT_READ);
- if (blit->batch)
- rendercopy(blit, &linear->fb, fb);
- else
- blitcopy(&linear->fb, fb);
+ linear->map = igt_vc4_mmap_bo(fd, linear->fb.gem_handle,
+ linear->fb.size,
+ PROT_READ | PROT_WRITE);
- gem_sync(fd, linear->fb.gem_handle);
+ vc4_fb_convert_plane_from_tiled(&linear->fb, &linear->map, fb, map);
- gem_set_domain(fd, linear->fb.gem_handle,
- I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
+ munmap(map, fb->size);
+ } else {
+ /* Copy fb content to linear BO */
+ gem_set_domain(fd, linear->fb.gem_handle,
+ I915_GEM_DOMAIN_GTT, 0);
+
+ if (blit->batch)
+ rendercopy(blit, &linear->fb, fb);
+ else
+ blitcopy(&linear->fb, fb);
+
+ gem_sync(fd, linear->fb.gem_handle);
+
+ gem_set_domain(fd, linear->fb.gem_handle,
+ I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
- /* Setup cairo context */
- linear->map = gem_mmap__cpu(fd, linear->fb.gem_handle,
- 0, linear->fb.size, PROT_READ | PROT_WRITE);
+ /* Setup cairo context */
+ linear->map = gem_mmap__cpu(fd, linear->fb.gem_handle,
+ 0, linear->fb.size, PROT_READ | PROT_WRITE);
+ }
}
static void create_cairo_surface__gpu(int fd, struct igt_fb *fb)
@@ -2902,7 +2922,7 @@ static void create_cairo_surface__convert(int fd, struct igt_fb *fb)
&blit->shadow_fb);
igt_assert(blit->shadow_ptr);
- if (use_rendercopy(fb) || use_blitter(fb)) {
+ if (use_rendercopy(fb) || use_blitter(fb) || igt_vc4_is_tiled(fb->modifier)) {
setup_linear_mapping(&blit->base);
} else {
blit->base.linear.fb = *fb;
@@ -2983,7 +3003,7 @@ cairo_surface_t *igt_get_cairo_surface(int fd, struct igt_fb *fb)
((f->cairo_id == CAIRO_FORMAT_INVALID) &&
(f->pixman_id != PIXMAN_invalid)))
create_cairo_surface__convert(fd, fb);
- else if (use_blitter(fb) || use_rendercopy(fb))
+ else if (use_blitter(fb) || use_rendercopy(fb) || igt_vc4_is_tiled(fb->modifier))
create_cairo_surface__gpu(fd, fb);
else
create_cairo_surface__gtt(fd, fb);
@@ -3102,58 +3122,23 @@ unsigned int igt_fb_convert_with_stride(struct igt_fb *dst, struct igt_fb *src,
uint64_t dst_modifier,
unsigned int dst_stride)
{
- struct fb_convert cvt = { };
- struct igt_fb linear;
- void *dst_ptr, *src_ptr;
- uint64_t base_modifier;
+ /* Use the cairo api to convert */
+ cairo_surface_t *surf = igt_get_cairo_surface(src->fd, src);
+ cairo_t *cr;
int fb_id;
- if (is_vc4_device(src->fd))
- base_modifier = fourcc_mod_broadcom_mod(dst_modifier);
- else
- base_modifier = dst_modifier;
-
- fb_id = igt_create_fb_with_bo_size(src->fd, src->width, src->height,
- dst_fourcc,
- LOCAL_DRM_FORMAT_MOD_NONE, &linear,
- 0, dst_stride);
+ fb_id = igt_create_fb_with_bo_size(src->fd, src->width,
+ src->height, dst_fourcc,
+ dst_modifier, dst, 0,
+ dst_stride);
igt_assert(fb_id > 0);
- src_ptr = igt_fb_map_buffer(src->fd, src);
- igt_assert(src_ptr);
-
- dst_ptr = igt_fb_map_buffer(linear.fd, &linear);
- igt_assert(dst_ptr);
-
- cvt.dst.ptr = dst_ptr;
- cvt.dst.fb = &linear;
- cvt.src.ptr = src_ptr;
- cvt.src.fb = src;
- fb_convert(&cvt);
-
- igt_fb_unmap_buffer(dst, dst_ptr);
- igt_fb_unmap_buffer(src, src_ptr);
-
- switch (base_modifier) {
- case DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED:
- fb_id = igt_vc4_fb_t_tiled_convert(dst, &linear);
- break;
- case DRM_FORMAT_MOD_BROADCOM_SAND32:
- case DRM_FORMAT_MOD_BROADCOM_SAND64:
- case DRM_FORMAT_MOD_BROADCOM_SAND128:
- case DRM_FORMAT_MOD_BROADCOM_SAND256:
- fb_id = vc4_fb_sand_tiled_convert(dst, &linear, dst_modifier);
- break;
- default:
- igt_assert(dst_modifier == LOCAL_DRM_FORMAT_MOD_NONE);
- }
-
- igt_assert(fb_id > 0);
+ cr = igt_get_cairo_ctx(dst->fd, dst);
+ cairo_set_source_surface(cr, surf, 0, 0);
+ cairo_paint(cr);
+ igt_put_cairo_ctx(dst->fd, dst, cr);
- if (dst_modifier == LOCAL_DRM_FORMAT_MOD_NONE)
- *dst = linear;
- else
- igt_remove_fb(linear.fd, &linear);
+ cairo_surface_destroy(surf);
return fb_id;
}
diff --git a/lib/igt_vc4.c b/lib/igt_vc4.c
index 9a0ba30b4420..4415fa321ceb 100644
--- a/lib/igt_vc4.c
+++ b/lib/igt_vc4.c
@@ -56,6 +56,23 @@
* tests.
*/
+bool igt_vc4_is_tiled(uint64_t modifier)
+{
+ if (modifier >> 56ULL != DRM_FORMAT_MOD_VENDOR_BROADCOM)
+ return false;
+
+ switch (fourcc_mod_broadcom_mod(modifier)) {
+ case DRM_FORMAT_MOD_BROADCOM_SAND32:
+ case DRM_FORMAT_MOD_BROADCOM_SAND64:
+ case DRM_FORMAT_MOD_BROADCOM_SAND128:
+ case DRM_FORMAT_MOD_BROADCOM_SAND256:
+ case DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED:
+ return true;
+ default:
+ return false;
+ }
+}
+
/**
* igt_vc4_get_cleared_bo:
* @fd: device file descriptor
@@ -178,63 +195,12 @@ bool igt_vc4_purgeable_bo(int fd, int handle, bool purgeable)
return arg.retained;
}
-unsigned int igt_vc4_fb_t_tiled_convert(struct igt_fb *dst, struct igt_fb *src)
-{
- unsigned int fb_id;
- unsigned int i, j;
- void *src_buf;
- void *dst_buf;
- size_t bpp = src->plane_bpp[0];
- size_t dst_stride = ALIGN(src->strides[0], 128);
-
- fb_id = igt_create_fb_with_bo_size(src->fd, src->width, src->height,
- src->drm_format,
- DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED,
- dst, 0, dst_stride);
- igt_assert(fb_id > 0);
-
- igt_assert(bpp == 16 || bpp == 32);
-
- src_buf = igt_fb_map_buffer(src->fd, src);
- igt_assert(src_buf);
-
- dst_buf = igt_fb_map_buffer(dst->fd, dst);
- igt_assert(dst_buf);
-
- for (i = 0; i < src->height; i++) {
- for (j = 0; j < src->width; j++) {
- size_t src_offset = src->offsets[0];
- size_t dst_offset = dst->offsets[0];
-
- src_offset += src->strides[0] * i + j * bpp / 8;
- dst_offset += igt_vc4_t_tiled_offset(dst_stride,
- src->height,
- bpp, j, i);
-
- switch (bpp) {
- case 16:
- *(uint16_t *)(dst_buf + dst_offset) =
- *(uint16_t *)(src_buf + src_offset);
- break;
- case 32:
- *(uint32_t *)(dst_buf + dst_offset) =
- *(uint32_t *)(src_buf + src_offset);
- break;
- }
- }
- }
-
- igt_fb_unmap_buffer(src, src_buf);
- igt_fb_unmap_buffer(dst, dst_buf);
-
- return fb_id;
-}
/* Calculate the t-tile width so that size = width * height * bpp / 8. */
#define VC4_T_TILE_W(size, height, bpp) ((size) / (height) / ((bpp) / 8))
-size_t igt_vc4_t_tiled_offset(size_t stride, size_t height, size_t bpp,
- size_t x, size_t y)
+static size_t igt_vc4_t_tiled_offset(size_t stride, size_t height, size_t bpp,
+ size_t x, size_t y)
{
const size_t t1k_map_even[] = { 0, 3, 1, 2 };
const size_t t1k_map_odd[] = { 2, 1, 3, 0 };
@@ -308,18 +274,116 @@ size_t igt_vc4_t_tiled_offset(size_t stride, size_t height, size_t bpp,
return offset;
}
-static void vc4_fb_sand_tiled_convert_plane(struct igt_fb *dst, void *dst_buf,
+static void vc4_fb_convert_plane_to_t_tiled(struct igt_fb *dst, void *dst_buf,
struct igt_fb *src, void *src_buf,
- size_t column_width_bytes,
- size_t column_height,
unsigned int plane)
{
+ size_t bpp = src->plane_bpp[plane];
+ unsigned int i, j;
+
+ for (i = 0; i < src->height; i++) {
+ for (j = 0; j < src->width; j++) {
+ size_t src_offset = src->offsets[plane];
+ size_t dst_offset = dst->offsets[plane];
+
+ src_offset += src->strides[plane] * i + j * bpp / 8;
+ dst_offset += igt_vc4_t_tiled_offset(dst->strides[plane],
+ dst->height,
+ bpp, j, i);
+
+ switch (bpp) {
+ case 16:
+ *(uint16_t *)(dst_buf + dst_offset) =
+ *(uint16_t *)(src_buf + src_offset);
+ break;
+ case 32:
+ *(uint32_t *)(dst_buf + dst_offset) =
+ *(uint32_t *)(src_buf + src_offset);
+ break;
+ }
+ }
+ }
+}
+
+static void vc4_fb_convert_plane_from_t_tiled(struct igt_fb *dst, void *dst_buf,
+ struct igt_fb *src, void *src_buf,
+ unsigned int plane)
+{
+ size_t bpp = src->plane_bpp[plane];
+ unsigned int i, j;
+
+ for (i = 0; i < src->height; i++) {
+ for (j = 0; j < src->width; j++) {
+ size_t src_offset = src->offsets[plane];
+ size_t dst_offset = dst->offsets[plane];
+
+ src_offset += igt_vc4_t_tiled_offset(src->strides[plane],
+ src->height,
+ bpp, j, i);
+ src_offset += dst->strides[plane] * i + j * bpp / 8;
+
+ switch (bpp) {
+ case 16:
+ *(uint16_t *)(dst_buf + dst_offset) =
+ *(uint16_t *)(src_buf + src_offset);
+ break;
+ case 32:
+ *(uint32_t *)(dst_buf + dst_offset) =
+ *(uint32_t *)(src_buf + src_offset);
+ break;
+ }
+ }
+ }
+}
+
+static size_t vc4_sand_tiled_offset(size_t column_width, size_t column_size, size_t x,
+ size_t y, size_t bpp)
+{
+ size_t offset = 0;
+ size_t cols_x;
+ size_t pix_x;
+
+ /* Offset to the beginning of the relevant column. */
+ cols_x = x / column_width;
+ offset += cols_x * column_size;
+
+ /* Offset to the relevant pixel. */
+ pix_x = x % column_width;
+ offset += (column_width * y + pix_x) * bpp / 8;
+
+ return offset;
+}
+
+static void vc4_fb_convert_plane_to_sand_tiled(struct igt_fb *dst, void *dst_buf,
+ struct igt_fb *src, void *src_buf,
+ unsigned int plane)
+{
+ uint64_t modifier_base = fourcc_mod_broadcom_mod(dst->modifier);
+ uint32_t column_height = fourcc_mod_broadcom_param(dst->modifier);
+ uint32_t column_width_bytes, column_width, column_size;
size_t bpp = dst->plane_bpp[plane];
- size_t column_width = column_width_bytes * dst->plane_width[plane] /
- dst->width;
- size_t column_size = column_width_bytes * column_height;
unsigned int i, j;
+ switch (modifier_base) {
+ case DRM_FORMAT_MOD_BROADCOM_SAND32:
+ column_width_bytes = 32;
+ break;
+ case DRM_FORMAT_MOD_BROADCOM_SAND64:
+ column_width_bytes = 64;
+ break;
+ case DRM_FORMAT_MOD_BROADCOM_SAND128:
+ column_width_bytes = 128;
+ break;
+ case DRM_FORMAT_MOD_BROADCOM_SAND256:
+ column_width_bytes = 256;
+ break;
+ default:
+ igt_assert(false);
+ }
+
+ column_width = column_width_bytes * dst->plane_width[plane] / dst->width;
+ column_size = column_width_bytes * column_height;
+
for (i = 0; i < dst->plane_height[plane]; i++) {
for (j = 0; j < src->plane_width[plane]; j++) {
size_t src_offset = src->offsets[plane];
@@ -346,19 +410,15 @@ static void vc4_fb_sand_tiled_convert_plane(struct igt_fb *dst, void *dst_buf,
}
}
-unsigned int vc4_fb_sand_tiled_convert(struct igt_fb *dst, struct igt_fb *src,
- uint64_t modifier)
+static void vc4_fb_convert_plane_from_sand_tiled(struct igt_fb *dst, void *dst_buf,
+ struct igt_fb *src, void *src_buf,
+ unsigned int plane)
{
- uint64_t modifier_base;
- size_t column_width_bytes;
- size_t column_height;
- unsigned int fb_id;
- unsigned int i;
- void *src_buf;
- void *dst_buf;
-
- modifier_base = fourcc_mod_broadcom_mod(modifier);
- column_height = fourcc_mod_broadcom_param(modifier);
+ uint64_t modifier_base = fourcc_mod_broadcom_mod(src->modifier);
+ uint32_t column_height = fourcc_mod_broadcom_param(src->modifier);
+ uint32_t column_width_bytes, column_width, column_size;
+ size_t bpp = src->plane_bpp[plane];
+ unsigned int i, j;
switch (modifier_base) {
case DRM_FORMAT_MOD_BROADCOM_SAND32:
@@ -377,41 +437,63 @@ unsigned int vc4_fb_sand_tiled_convert(struct igt_fb *dst, struct igt_fb *src,
igt_assert(false);
}
- fb_id = igt_create_fb(src->fd, src->width, src->height, src->drm_format,
- modifier, dst);
- igt_assert(fb_id > 0);
+ column_width = column_width_bytes * src->plane_width[plane] / src->width;
+ column_size = column_width_bytes * column_height;
+
+ for (i = 0; i < dst->plane_height[plane]; i++) {
+ for (j = 0; j < src->plane_width[plane]; j++) {
+ size_t src_offset = src->offsets[plane];
+ size_t dst_offset = dst->offsets[plane];
- src_buf = igt_fb_map_buffer(src->fd, src);
- igt_assert(src_buf);
+ src_offset += vc4_sand_tiled_offset(column_width,
+ column_size, j, i,
+ bpp);
+ dst_offset += dst->strides[plane] * i + j * bpp / 8;
- dst_buf = igt_fb_map_buffer(dst->fd, dst);
- igt_assert(dst_buf);
+ switch (bpp) {
+ case 8:
+ *(uint8_t *)(dst_buf + dst_offset) =
+ *(uint8_t *)(src_buf + src_offset);
+ break;
+ case 16:
+ *(uint16_t *)(dst_buf + dst_offset) =
+ *(uint16_t *)(src_buf + src_offset);
+ break;
+ default:
+ igt_assert(false);
+ }
+ }
+ }
+}
- for (i = 0; i < dst->num_planes; i++)
- vc4_fb_sand_tiled_convert_plane(dst, dst_buf, src, src_buf,
- column_width_bytes,
- column_height, i);
+void vc4_fb_convert_plane_to_tiled(struct igt_fb *dst, void *dst_buf,
+ struct igt_fb *src, void *src_buf)
+{
+ unsigned int plane;
- igt_fb_unmap_buffer(src, src_buf);
- igt_fb_unmap_buffer(dst, dst_buf);
+ igt_assert(src->modifier == DRM_FORMAT_MOD_LINEAR);
+ igt_assert(igt_vc4_is_tiled(dst->modifier));
- return fb_id;
+ for (plane = 0; plane < src->num_planes; plane++) {
+ if (dst->modifier == DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED)
+ vc4_fb_convert_plane_to_t_tiled(dst, dst_buf, src, src_buf, plane);
+ else
+ vc4_fb_convert_plane_to_sand_tiled(dst, dst_buf, src, src_buf, plane);
+ }
}
-size_t vc4_sand_tiled_offset(size_t column_width, size_t column_size, size_t x,
- size_t y, size_t bpp)
+void vc4_fb_convert_plane_from_tiled(struct igt_fb *dst, void *dst_buf,
+ struct igt_fb *src, void *src_buf)
{
- size_t offset = 0;
- size_t cols_x;
- size_t pix_x;
-
- /* Offset to the beginning of the relevant column. */
- cols_x = x / column_width;
- offset += cols_x * column_size;
+ unsigned int plane;
- /* Offset to the relevant pixel. */
- pix_x = x % column_width;
- offset += (column_width * y + pix_x) * bpp / 8;
+ igt_assert(igt_vc4_is_tiled(src->modifier));
+ igt_assert(dst->modifier == DRM_FORMAT_MOD_LINEAR);
- return offset;
+ for (plane = 0; plane < src->num_planes; plane++) {
+ if (src->modifier == DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED)
+ vc4_fb_convert_plane_from_t_tiled(dst, dst_buf, src, src_buf, plane);
+ else
+ vc4_fb_convert_plane_from_sand_tiled(dst, dst_buf, src, src_buf, plane);
+ }
}
diff --git a/lib/igt_vc4.h b/lib/igt_vc4.h
index a17812698fe5..f32bf398b3eb 100644
--- a/lib/igt_vc4.h
+++ b/lib/igt_vc4.h
@@ -29,16 +29,14 @@ int igt_vc4_create_bo(int fd, size_t size);
void *igt_vc4_mmap_bo(int fd, uint32_t handle, uint32_t size, unsigned prot);
int igt_vc4_get_param(int fd, uint32_t param, uint64_t *val);
bool igt_vc4_purgeable_bo(int fd, int handle, bool purgeable);
+bool igt_vc4_is_tiled(uint64_t modifier);
void igt_vc4_set_tiling(int fd, uint32_t handle, uint64_t modifier);
uint64_t igt_vc4_get_tiling(int fd, uint32_t handle);
-unsigned int igt_vc4_fb_t_tiled_convert(struct igt_fb *dst, struct igt_fb *src);
-size_t igt_vc4_t_tiled_offset(size_t stride, size_t height, size_t bpp,
- size_t x, size_t y);
-unsigned int vc4_fb_sand_tiled_convert(struct igt_fb *dst, struct igt_fb *src,
- uint64_t modifier);
-size_t vc4_sand_tiled_offset(size_t column_width, size_t column_size, size_t x,
- size_t y, size_t bpp);
+void vc4_fb_convert_plane_to_tiled(struct igt_fb *dst, void *dst_buf,
+ struct igt_fb *src, void *src_buf);
+void vc4_fb_convert_plane_from_tiled(struct igt_fb *dst, void *dst_buf,
+ struct igt_fb *src, void *src_buf);
#endif /* IGT_VC4_H */
--
2.20.1
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3.
2019-05-10 14:19 ` Maarten Lankhorst
@ 2019-05-10 15:10 ` Paul Kocialkowski
2019-05-10 16:02 ` Maarten Lankhorst
0 siblings, 1 reply; 17+ messages in thread
From: Paul Kocialkowski @ 2019-05-10 15:10 UTC (permalink / raw)
To: Maarten Lankhorst, igt-dev
Hi,
On Fri, 2019-05-10 at 16:19 +0200, Maarten Lankhorst wrote:
> Op 10-05-2019 om 15:43 schreef Paul Kocialkowski:
> > Hi,
> >
> > It looks like I fell behind on reviewing earlier version here, sorry
> > about that.
> >
> > On Fri, 2019-05-10 at 14:33 +0200, Maarten Lankhorst wrote:
> > > Ever since commit 3fa65f4b532bd9a5b ("fb: Add support for conversions
> > > through pixman") we can generate a valid cairo surface for any plane,
> > > use this to avoid having to implement our own conversion routine.
> > >
> > > Instead of duplicating this functionality in igt_fb_convert_with_stride,
> > > we can simply convert this to a few cairo calls, because we now support
> > > cairo calls to any of the supported framebuffer formats.
> > I don't see how that is the case. Cairo definitely does not support our
> > tiled formats, so we can't just pipe them into it.
> >
> > And we already use pixman for converting through the fb_convert call to
> > convert_pixman when possible. Also, my understanding is that cairo is
> > very limited format-wise, so we're much better off using pixman
> > directly (which is is what a non-accelerated cairo surface will do
> > anyway).
> >
> > > This is required to make this function more generic, and convert from any
> > > format/modifier to any other format/modifier.
> > We've already designed it to be generic, we just need conversion
> > helpers from/to (currently only to) hardware-specific tiling modifiers.
> >
> > We can't expect cairo or pixman to do that job and this change pretty
> > much kills support for the vc4 tiling modes we've added.
> >
> > Unless I'm missing something, that's a NACK on my side.
>
> You have a function that can convert a untiled fb to tiled, but not
> the other way around in igt_vc4.c
We don't need it for anything at the moment but it wouldn't be a
problem to come up with one, sure.
> If you could provide that, we could convert from any fb/modifier
> format to any fb/modifier format, even with VC4 tiling.
I don't fully get the point of that if our tests are not going to use
it, but it doesn't hurt either :)
> Our internal cairo hooks already provide helpers for conversion, see
> igt_get_cairo_surface() which calls
> create_cairo_surface__convert()
My feeling is that we should kill use of cairo formats and go with
pixman directly.
> Some conversions can only be done through cairo, converting between
> P01x and XRGB8888 cannot be done directly,
> our pixman representation for that are done in the floating point
> formats.
I'm not sure I'm following this point, but if there's a conversion that
cairo can do and pixman can't, it feels like the right thing would be
to fix pixman to support it. Is there a reason in particular why this
wouldn't work?
> The only way to convert between some framebuffer formats and
> modifiers in i915 is by using those conversion functions,
> the fact that igt_fb_convert_with_stride doesn't do those, makes a
> truly random hdmi-cmp-planes-random useless.
So it boils down to a missing format support issue, not really to an
issue with the existing flow.
> I was getting CRC mismatches because the i915 modifiers weren't
> respected. When I made sure they were being respected
> I ended up with a duplicate of the cairo context stuff. So why not
> lose a little speed and use that?
My counter-suggestion would be to do the opposite and make sure pixman
can deal with these cases on its own.
> If you write and test a detiler function, I can hook it up for you.
> :)
> If necessary I can do it myself, to move this patch forward.
>
> Cairo shouldn't be much slower than pixman, because internally it
> already uses pixman calls for the backend.
Sure, but that's not cairo's role: cairo is about drawing, while pixman
is about pixel conversion. I think it would be best to stick to that.
Cheers,
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3.
2019-05-10 13:43 ` Paul Kocialkowski
@ 2019-05-10 14:19 ` Maarten Lankhorst
2019-05-10 15:10 ` Paul Kocialkowski
0 siblings, 1 reply; 17+ messages in thread
From: Maarten Lankhorst @ 2019-05-10 14:19 UTC (permalink / raw)
To: Paul Kocialkowski, igt-dev
Op 10-05-2019 om 15:43 schreef Paul Kocialkowski:
> Hi,
>
> It looks like I fell behind on reviewing earlier version here, sorry
> about that.
>
> On Fri, 2019-05-10 at 14:33 +0200, Maarten Lankhorst wrote:
>> Ever since commit 3fa65f4b532bd9a5b ("fb: Add support for conversions
>> through pixman") we can generate a valid cairo surface for any plane,
>> use this to avoid having to implement our own conversion routine.
>>
>> Instead of duplicating this functionality in igt_fb_convert_with_stride,
>> we can simply convert this to a few cairo calls, because we now support
>> cairo calls to any of the supported framebuffer formats.
> I don't see how that is the case. Cairo definitely does not support our
> tiled formats, so we can't just pipe them into it.
>
> And we already use pixman for converting through the fb_convert call to
> convert_pixman when possible. Also, my understanding is that cairo is
> very limited format-wise, so we're much better off using pixman
> directly (which is is what a non-accelerated cairo surface will do
> anyway).
>
>> This is required to make this function more generic, and convert from any
>> format/modifier to any other format/modifier.
> We've already designed it to be generic, we just need conversion
> helpers from/to (currently only to) hardware-specific tiling modifiers.
>
> We can't expect cairo or pixman to do that job and this change pretty
> much kills support for the vc4 tiling modes we've added.
>
> Unless I'm missing something, that's a NACK on my side.
You have a function that can convert a untiled fb to tiled, but not the other way around in igt_vc4.c
If you could provide that, we could convert from any fb/modifier format to any fb/modifier format, even with VC4 tiling.
Our internal cairo hooks already provide helpers for conversion, see igt_get_cairo_surface() which calls
create_cairo_surface__convert()
Some conversions can only be done through cairo, converting between P01x and XRGB8888 cannot be done directly,
our pixman representation for that are done in the floating point formats.
The only way to convert between some framebuffer formats and modifiers in i915 is by using those conversion functions,
the fact that igt_fb_convert_with_stride doesn't do those, makes a truly random hdmi-cmp-planes-random useless.
I was getting CRC mismatches because the i915 modifiers weren't respected. When I made sure they were being respected
I ended up with a duplicate of the cairo context stuff. So why not lose a little speed and use that?
If you write and test a detiler function, I can hook it up for you. :)
If necessary I can do it myself, to move this patch forward.
Cairo shouldn't be much slower than pixman, because internally it already uses pixman calls for the backend.
~Maarten
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3.
2019-05-10 12:33 [igt-dev] [PATCH i-g-t 1/2] " Maarten Lankhorst
@ 2019-05-10 13:43 ` Paul Kocialkowski
2019-05-10 14:19 ` Maarten Lankhorst
0 siblings, 1 reply; 17+ messages in thread
From: Paul Kocialkowski @ 2019-05-10 13:43 UTC (permalink / raw)
To: Maarten Lankhorst, igt-dev
Hi,
It looks like I fell behind on reviewing earlier version here, sorry
about that.
On Fri, 2019-05-10 at 14:33 +0200, Maarten Lankhorst wrote:
> Ever since commit 3fa65f4b532bd9a5b ("fb: Add support for conversions
> through pixman") we can generate a valid cairo surface for any plane,
> use this to avoid having to implement our own conversion routine.
>
> Instead of duplicating this functionality in igt_fb_convert_with_stride,
> we can simply convert this to a few cairo calls, because we now support
> cairo calls to any of the supported framebuffer formats.
I don't see how that is the case. Cairo definitely does not support our
tiled formats, so we can't just pipe them into it.
And we already use pixman for converting through the fb_convert call to
convert_pixman when possible. Also, my understanding is that cairo is
very limited format-wise, so we're much better off using pixman
directly (which is is what a non-accelerated cairo surface will do
anyway).
> This is required to make this function more generic, and convert from any
> format/modifier to any other format/modifier.
We've already designed it to be generic, we just need conversion
helpers from/to (currently only to) hardware-specific tiling modifiers.
We can't expect cairo or pixman to do that job and this change pretty
much kills support for the vc4 tiling modes we've added.
Unless I'm missing something, that's a NACK on my side.
Cheers,
Paul
> Changes since v1:
> - Return fb_id in the cairo case.
> Changes since v2:
> - Remove the manual conversion fallback.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
> lib/igt_fb.c | 59 +++++++++++-----------------------------------------
> 1 file changed, 12 insertions(+), 47 deletions(-)
>
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index d4929019971c..d7ccbc19b834 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -3102,58 +3102,23 @@ unsigned int igt_fb_convert_with_stride(struct igt_fb *dst, struct igt_fb *src,
> uint64_t dst_modifier,
> unsigned int dst_stride)
> {
> - struct fb_convert cvt = { };
> - struct igt_fb linear;
> - void *dst_ptr, *src_ptr;
> - uint64_t base_modifier;
> + /* Use the cairo api to convert */
> + cairo_surface_t *surf = igt_get_cairo_surface(src->fd, src);
> + cairo_t *cr;
> int fb_id;
>
> - if (is_vc4_device(src->fd))
> - base_modifier = fourcc_mod_broadcom_mod(dst_modifier);
> - else
> - base_modifier = dst_modifier;
> -
> - fb_id = igt_create_fb_with_bo_size(src->fd, src->width, src->height,
> - dst_fourcc,
> - LOCAL_DRM_FORMAT_MOD_NONE, &linear,
> - 0, dst_stride);
> + fb_id = igt_create_fb_with_bo_size(src->fd, src->width,
> + src->height, dst_fourcc,
> + dst_modifier, dst, 0,
> + dst_stride);
> igt_assert(fb_id > 0);
>
> - src_ptr = igt_fb_map_buffer(src->fd, src);
> - igt_assert(src_ptr);
> -
> - dst_ptr = igt_fb_map_buffer(linear.fd, &linear);
> - igt_assert(dst_ptr);
> -
> - cvt.dst.ptr = dst_ptr;
> - cvt.dst.fb = &linear;
> - cvt.src.ptr = src_ptr;
> - cvt.src.fb = src;
> - fb_convert(&cvt);
> -
> - igt_fb_unmap_buffer(dst, dst_ptr);
> - igt_fb_unmap_buffer(src, src_ptr);
> -
> - switch (base_modifier) {
> - case DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED:
> - fb_id = igt_vc4_fb_t_tiled_convert(dst, &linear);
> - break;
> - case DRM_FORMAT_MOD_BROADCOM_SAND32:
> - case DRM_FORMAT_MOD_BROADCOM_SAND64:
> - case DRM_FORMAT_MOD_BROADCOM_SAND128:
> - case DRM_FORMAT_MOD_BROADCOM_SAND256:
> - fb_id = vc4_fb_sand_tiled_convert(dst, &linear, dst_modifier);
> - break;
> - default:
> - igt_assert(dst_modifier == LOCAL_DRM_FORMAT_MOD_NONE);
> - }
> -
> - igt_assert(fb_id > 0);
> + cr = igt_get_cairo_ctx(dst->fd, dst);
> + cairo_set_source_surface(cr, surf, 0, 0);
> + cairo_paint(cr);
> + igt_put_cairo_ctx(dst->fd, dst, cr);
>
> - if (dst_modifier == LOCAL_DRM_FORMAT_MOD_NONE)
> - *dst = linear;
> - else
> - igt_remove_fb(linear.fd, &linear);
> + cairo_surface_destroy(surf);
>
> return fb_id;
> }
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 17+ messages in thread
* [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3.
@ 2019-05-10 12:33 Maarten Lankhorst
2019-05-10 13:43 ` Paul Kocialkowski
0 siblings, 1 reply; 17+ messages in thread
From: Maarten Lankhorst @ 2019-05-10 12:33 UTC (permalink / raw)
To: igt-dev
Ever since commit 3fa65f4b532bd9a5b ("fb: Add support for conversions
through pixman") we can generate a valid cairo surface for any plane,
use this to avoid having to implement our own conversion routine.
Instead of duplicating this functionality in igt_fb_convert_with_stride,
we can simply convert this to a few cairo calls, because we now support
cairo calls to any of the supported framebuffer formats.
This is required to make this function more generic, and convert from any
format/modifier to any other format/modifier.
Changes since v1:
- Return fb_id in the cairo case.
Changes since v2:
- Remove the manual conversion fallback.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
lib/igt_fb.c | 59 +++++++++++-----------------------------------------
1 file changed, 12 insertions(+), 47 deletions(-)
diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index d4929019971c..d7ccbc19b834 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -3102,58 +3102,23 @@ unsigned int igt_fb_convert_with_stride(struct igt_fb *dst, struct igt_fb *src,
uint64_t dst_modifier,
unsigned int dst_stride)
{
- struct fb_convert cvt = { };
- struct igt_fb linear;
- void *dst_ptr, *src_ptr;
- uint64_t base_modifier;
+ /* Use the cairo api to convert */
+ cairo_surface_t *surf = igt_get_cairo_surface(src->fd, src);
+ cairo_t *cr;
int fb_id;
- if (is_vc4_device(src->fd))
- base_modifier = fourcc_mod_broadcom_mod(dst_modifier);
- else
- base_modifier = dst_modifier;
-
- fb_id = igt_create_fb_with_bo_size(src->fd, src->width, src->height,
- dst_fourcc,
- LOCAL_DRM_FORMAT_MOD_NONE, &linear,
- 0, dst_stride);
+ fb_id = igt_create_fb_with_bo_size(src->fd, src->width,
+ src->height, dst_fourcc,
+ dst_modifier, dst, 0,
+ dst_stride);
igt_assert(fb_id > 0);
- src_ptr = igt_fb_map_buffer(src->fd, src);
- igt_assert(src_ptr);
-
- dst_ptr = igt_fb_map_buffer(linear.fd, &linear);
- igt_assert(dst_ptr);
-
- cvt.dst.ptr = dst_ptr;
- cvt.dst.fb = &linear;
- cvt.src.ptr = src_ptr;
- cvt.src.fb = src;
- fb_convert(&cvt);
-
- igt_fb_unmap_buffer(dst, dst_ptr);
- igt_fb_unmap_buffer(src, src_ptr);
-
- switch (base_modifier) {
- case DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED:
- fb_id = igt_vc4_fb_t_tiled_convert(dst, &linear);
- break;
- case DRM_FORMAT_MOD_BROADCOM_SAND32:
- case DRM_FORMAT_MOD_BROADCOM_SAND64:
- case DRM_FORMAT_MOD_BROADCOM_SAND128:
- case DRM_FORMAT_MOD_BROADCOM_SAND256:
- fb_id = vc4_fb_sand_tiled_convert(dst, &linear, dst_modifier);
- break;
- default:
- igt_assert(dst_modifier == LOCAL_DRM_FORMAT_MOD_NONE);
- }
-
- igt_assert(fb_id > 0);
+ cr = igt_get_cairo_ctx(dst->fd, dst);
+ cairo_set_source_surface(cr, surf, 0, 0);
+ cairo_paint(cr);
+ igt_put_cairo_ctx(dst->fd, dst, cr);
- if (dst_modifier == LOCAL_DRM_FORMAT_MOD_NONE)
- *dst = linear;
- else
- igt_remove_fb(linear.fd, &linear);
+ cairo_surface_destroy(surf);
return fb_id;
}
--
2.20.1
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2019-05-16 12:59 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-05 12:52 [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3 Maarten Lankhorst
2019-04-05 12:52 ` [igt-dev] [PATCH i-g-t 2/2] tests/kms_chamelium: Fix *-cmp-random and *-crc-random tests, v3 Maarten Lankhorst
2019-05-07 10:42 ` [igt-dev] [i-g-t, " Ser, Simon
2019-04-05 13:54 ` [igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3 Paul Kocialkowski
2019-04-05 15:38 ` Maarten Lankhorst
2019-04-08 8:50 ` Maxime Ripard
2019-04-08 11:04 ` Maarten Lankhorst
2019-04-09 14:59 ` Maxime Ripard
2019-04-05 14:08 ` [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/2] " Patchwork
2019-04-06 11:48 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
2019-05-10 12:33 [igt-dev] [PATCH i-g-t 1/2] " Maarten Lankhorst
2019-05-10 13:43 ` Paul Kocialkowski
2019-05-10 14:19 ` Maarten Lankhorst
2019-05-10 15:10 ` Paul Kocialkowski
2019-05-10 16:02 ` Maarten Lankhorst
2019-05-14 6:34 ` Juha-Pekka Heikkila
2019-05-16 12:59 ` Maarten Lankhorst
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.