* [igt-dev] [PATCH i-g-t 0/2] switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw
@ 2023-03-27 20:12 Juha-Pekka Heikkila
2023-03-27 20:12 ` [igt-dev] [PATCH i-g-t 1/2] lib/i915/i915_blt: Add offset to block and fast copy Juha-Pekka Heikkila
` (5 more replies)
0 siblings, 6 replies; 14+ messages in thread
From: Juha-Pekka Heikkila @ 2023-03-27 20:12 UTC (permalink / raw)
To: igt-dev
Switch to new blitter functions on framebuffer creation. On Intel CI there's
still some legacy machines with relocation support hence left XY_SRC path as
legacy path.
Juha-Pekka Heikkila (2):
lib/i915/i915_blt: Add offset to block and fast copy
lib/igt_fb: switch blitcopy to use lib/i915/i915_blt functions
lib/i915/i915_blt.c | 12 ++-
lib/i915/i915_blt.h | 1 +
lib/igt_fb.c | 214 +++++++++++++++++++++++++++++++++++++++-----
3 files changed, 201 insertions(+), 26 deletions(-)
--
2.39.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [igt-dev] [PATCH i-g-t 1/2] lib/i915/i915_blt: Add offset to block and fast copy
2023-03-27 20:12 [igt-dev] [PATCH i-g-t 0/2] switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw Juha-Pekka Heikkila
@ 2023-03-27 20:12 ` Juha-Pekka Heikkila
2023-03-27 20:12 ` [igt-dev] [PATCH i-g-t 2/2] lib/igt_fb: switch blitcopy to use lib/i915/i915_blt functions Juha-Pekka Heikkila
` (4 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Juha-Pekka Heikkila @ 2023-03-27 20:12 UTC (permalink / raw)
To: igt-dev
Add offset to src and dst blits, this allow to use i915_blt with multiplane
framebuffers.
Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
---
lib/i915/i915_blt.c | 12 ++++++++----
lib/i915/i915_blt.h | 1 +
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/lib/i915/i915_blt.c b/lib/i915/i915_blt.c
index ef67fe26f..ffaa97b42 100644
--- a/lib/i915/i915_blt.c
+++ b/lib/i915/i915_blt.c
@@ -708,8 +708,10 @@ uint64_t emit_blt_block_copy(int i915,
igt_assert_f(blt, "block-copy requires data to do blit\n");
alignment = gem_detect_safe_alignment(i915);
- src_offset = get_offset(ahnd, blt->src.handle, blt->src.size, alignment);
- dst_offset = get_offset(ahnd, blt->dst.handle, blt->dst.size, alignment);
+ src_offset = get_offset(ahnd, blt->src.handle, blt->src.size, alignment)
+ + blt->src.offset;
+ dst_offset = get_offset(ahnd, blt->dst.handle, blt->dst.size, alignment)
+ + blt->dst.offset;
bb_offset = get_offset(ahnd, blt->bb.handle, blt->bb.size, alignment);
fill_data(&data, blt, src_offset, dst_offset, ext);
@@ -1179,8 +1181,10 @@ uint64_t emit_blt_fast_copy(int i915,
data.dw03.dst_x2 = blt->dst.x2;
data.dw03.dst_y2 = blt->dst.y2;
- src_offset = get_offset(ahnd, blt->src.handle, blt->src.size, alignment);
- dst_offset = get_offset(ahnd, blt->dst.handle, blt->dst.size, alignment);
+ src_offset = get_offset(ahnd, blt->src.handle, blt->src.size, alignment)
+ + blt->src.offset;
+ dst_offset = get_offset(ahnd, blt->dst.handle, blt->dst.size, alignment)
+ + blt->dst.offset;
bb_offset = get_offset(ahnd, blt->bb.handle, blt->bb.size, alignment);
data.dw04.dst_address_lo = dst_offset;
diff --git a/lib/i915/i915_blt.h b/lib/i915/i915_blt.h
index a5f0edd15..3e5bec496 100644
--- a/lib/i915/i915_blt.h
+++ b/lib/i915/i915_blt.h
@@ -80,6 +80,7 @@ struct blt_copy_object {
enum blt_compression compression; /* BC only */
enum blt_compression_type compression_type; /* BC only */
uint32_t pitch;
+ uint32_t offset;
uint16_t x_offset, y_offset;
int16_t x1, y1, x2, y2;
--
2.39.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [igt-dev] [PATCH i-g-t 2/2] lib/igt_fb: switch blitcopy to use lib/i915/i915_blt functions
2023-03-27 20:12 [igt-dev] [PATCH i-g-t 0/2] switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw Juha-Pekka Heikkila
2023-03-27 20:12 ` [igt-dev] [PATCH i-g-t 1/2] lib/i915/i915_blt: Add offset to block and fast copy Juha-Pekka Heikkila
@ 2023-03-27 20:12 ` Juha-Pekka Heikkila
2023-03-28 6:50 ` Zbigniew Kempczyński
2023-03-27 20:34 ` [igt-dev] ✗ GitLab.Pipeline: warning for switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw Patchwork
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Juha-Pekka Heikkila @ 2023-03-27 20:12 UTC (permalink / raw)
To: igt-dev
reduce code duplication
Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
---
lib/igt_fb.c | 214 +++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 192 insertions(+), 22 deletions(-)
diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index ba89e1f60..06ebe4818 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -35,6 +35,8 @@
#include "drmtest.h"
#include "i915/gem_create.h"
#include "i915/gem_mman.h"
+#include "i915/i915_blt.h"
+#include "i915/intel_mocs.h"
#include "igt_aux.h"
#include "igt_color_encoding.h"
#include "igt_fb.h"
@@ -2680,12 +2682,134 @@ static void copy_with_engine(struct fb_blit_upload *blit,
fini_buf(src);
}
+static struct blt_copy_object *blt_fb_init(const struct igt_fb *fb,
+ uint32_t plane, uint32_t memregion)
+{
+ uint32_t name, handle;
+ struct blt_copy_object *blt;
+ enum blt_tiling_type blt_tile;
+ uint64_t stride;
+
+ blt = malloc(sizeof(*blt));
+ igt_assert(blt);
+
+ name = gem_flink(fb->fd, fb->gem_handle);
+ handle = gem_open(fb->fd, name);
+
+ switch (igt_fb_mod_to_tiling(fb->modifier)) {
+ case I915_TILING_X:
+ blt_tile = T_XMAJOR;
+ stride = fb->strides[plane] / 4;
+ break;
+ case I915_TILING_Y:
+ blt_tile = T_YMAJOR;
+ stride = fb->strides[plane] / 4;
+ break;
+ case I915_TILING_4:
+ blt_tile = T_TILE4;
+ stride = fb->strides[plane] / 4;
+ break;
+ case I915_TILING_NONE:
+ default:
+ blt_tile = T_LINEAR;
+ stride = fb->strides[plane];
+ break;
+ }
+
+ blt_set_object(blt, handle, fb->size, memregion,
+ intel_get_uc_mocs(fb->fd),
+ blt_tile,
+ is_ccs_modifier(fb->modifier) ? COMPRESSION_ENABLED : COMPRESSION_DISABLED,
+ is_gen12_mc_ccs_modifier(fb->modifier) ? COMPRESSION_TYPE_MEDIA : COMPRESSION_TYPE_3D);
+
+ blt_set_geom(blt, stride, 0, 0, fb->width, fb->plane_height[plane], 0, 0);
+
+ blt->offset = fb->offsets[plane];
+
+ blt->ptr = gem_mmap__device_coherent(fb->fd, handle, 0, fb->size,
+ PROT_READ | PROT_WRITE);
+ return blt;
+}
+
+static enum blt_color_depth blt_get_bpp(const struct igt_fb *fb)
+{
+ switch (fb->plane_bpp[0]) {
+ case 8:
+ return CD_8bit;
+ case 16:
+ return CD_16bit;
+ case 32:
+ return CD_32bit;
+ case 64:
+ return CD_64bit;
+ case 96:
+ return CD_96bit;
+ case 128:
+ return CD_128bit;
+ default:
+ igt_assert(0);
+ }
+}
+
+#define BLT_TARGET_RC(x) (x.compression == COMPRESSION_ENABLED && \
+ x.compression_type == COMPRESSION_TYPE_3D)
+
+#define BLT_TARGET_MC(x) (x.compression == COMPRESSION_ENABLED && \
+ x.compression_type == COMPRESSION_TYPE_MEDIA)
+
+static uint32_t blt_compression_format(struct blt_copy_data *blt,
+ const struct igt_fb *fb)
+{
+ if (blt->src.compression == COMPRESSION_DISABLED &&
+ blt->dst.compression == COMPRESSION_DISABLED)
+ return 0;
+
+ if (BLT_TARGET_RC(blt->src) || BLT_TARGET_RC(blt->dst)) {
+ switch (blt->color_depth)
+ {
+ case CD_32bit:
+ return 8;
+ default:
+ igt_assert_f(0, "COMPRESSION_TYPE_3D unknown color depth\n");
+ }
+ }
+
+ if (BLT_TARGET_MC(blt->src) || BLT_TARGET_MC(blt->dst)) {
+ switch (fb->drm_format)
+ {
+ case DRM_FORMAT_XRGB8888:
+ return 8;
+ case DRM_FORMAT_XYUV8888:
+ return 9;
+ case DRM_FORMAT_NV12:
+ return 9;
+ case DRM_FORMAT_P010:
+ case DRM_FORMAT_P012:
+ case DRM_FORMAT_P016:
+ return 8;
+ default:
+ igt_assert_f(0, "COMPRESSION_TYPE_MEDIA unknown format\n");
+ }
+ } else {
+ igt_assert_f(0, "unknown compression\n");
+ }
+}
+
static void blitcopy(const struct igt_fb *dst_fb,
const struct igt_fb *src_fb)
{
uint32_t src_tiling, dst_tiling;
uint32_t ctx = 0;
uint64_t ahnd = 0;
+ const intel_ctx_t *ictx = intel_ctx_create_all_physical(src_fb->fd);
+ struct intel_execution_engine2 *e;
+ uint32_t bb;
+ uint64_t bb_size = 4096;
+ struct blt_copy_data blt = {};
+ struct blt_copy_object *src, *dst;
+ struct blt_block_copy_data_ext ext = {}, *pext = NULL;
+ uint32_t mem_region = HAS_FLATCCS(intel_get_drm_devid(src_fb->fd))
+ ? REGION_LMEM(0) : REGION_SMEM;
igt_assert_eq(dst_fb->fd, src_fb->fd);
igt_assert_eq(dst_fb->num_planes, src_fb->num_planes);
@@ -2697,36 +2821,81 @@ static void blitcopy(const struct igt_fb *dst_fb,
igt_require(gem_has_contexts(dst_fb->fd));
ctx = gem_context_create(dst_fb->fd);
ahnd = get_reloc_ahnd(dst_fb->fd, ctx);
+
+ igt_assert(__gem_create_in_memory_regions(src_fb->fd,
+ &bb,
+ &bb_size,
+ mem_region) == 0);
+
+ for_each_ctx_engine(src_fb->fd, ictx, e) {
+ if (gem_engine_can_block_copy(src_fb->fd, e))
+ break;
+ }
}
for (int i = 0; i < dst_fb->num_planes; i++) {
igt_assert_eq(dst_fb->plane_bpp[i], src_fb->plane_bpp[i]);
igt_assert_eq(dst_fb->plane_width[i], src_fb->plane_width[i]);
igt_assert_eq(dst_fb->plane_height[i], src_fb->plane_height[i]);
- /*
- * On GEN12+ X-tiled format support is removed from the fast
- * blit command, so use the XY_SRC blit command for it
- * instead.
- */
+
if (fast_blit_ok(src_fb) && fast_blit_ok(dst_fb)) {
- igt_blitter_fast_copy__raw(dst_fb->fd,
- ahnd, ctx, NULL,
- src_fb->gem_handle,
- src_fb->offsets[i],
- src_fb->strides[i],
- src_tiling,
- 0, 0, /* src_x, src_y */
- src_fb->size,
- dst_fb->plane_width[i],
- dst_fb->plane_height[i],
- dst_fb->plane_bpp[i],
- dst_fb->gem_handle,
- dst_fb->offsets[i],
- dst_fb->strides[i],
- dst_tiling,
- 0, 0 /* dst_x, dst_y */,
- dst_fb->size);
+ memset(&blt, 0, sizeof(blt));
+ blt.color_depth = blt_get_bpp(src_fb);
+
+ src = blt_fb_init(src_fb, i, mem_region);
+ dst = blt_fb_init(dst_fb, i, mem_region);
+
+ blt_set_copy_object(&blt.src, src);
+ blt_set_copy_object(&blt.dst, dst);
+
+ blt_set_batch(&blt.bb, bb, bb_size, mem_region);
+
+ blt_fast_copy(src_fb->fd, ictx, e, ahnd, &blt);
+ gem_sync(src_fb->fd, blt.dst.handle);
+
+ blt_destroy_object(src_fb->fd, src);
+ blt_destroy_object(dst_fb->fd, dst);
+ } else if (ahnd) {
+ /*
+ * On GEN12+ X-tiled format support is removed from
+ * the fast blit command, so use the block copy blit
+ * command for it instead.
+ */
+ src = blt_fb_init(src_fb, i, mem_region);
+ dst = blt_fb_init(dst_fb, i, mem_region);
+
+ memset(&blt, 0, sizeof(blt));
+ blt.print_bb = true;
+ blt.color_depth = blt_get_bpp(src_fb);
+ blt_set_copy_object(&blt.src, src);
+ blt_set_copy_object(&blt.dst, dst);
+
+ if (HAS_FLATCCS(intel_get_drm_devid(src_fb->fd))) {
+ blt_set_object_ext(&ext.src,
+ blt_compression_format(&blt, src_fb),
+ src_fb->width, src_fb->height,
+ SURFACE_TYPE_2D);
+
+ blt_set_object_ext(&ext.dst,
+ blt_compression_format(&blt, dst_fb),
+ dst_fb->width, dst_fb->height,
+ SURFACE_TYPE_2D);
+
+ pext = &ext;
+ }
+
+ blt_set_batch(&blt.bb, bb, bb_size, mem_region);
+
+ blt_block_copy(src_fb->fd, ictx, e, ahnd, &blt, pext);
+ gem_sync(src_fb->fd, blt.dst.handle);
+
+ blt_destroy_object(src_fb->fd, src);
+ blt_destroy_object(dst_fb->fd, dst);
} else {
+ /*
+ * If on legacy hardware where relocations are supported
+ * we'll use XY_SRC blit command instead
+ */
igt_blitter_src_copy(dst_fb->fd,
ahnd, ctx, NULL,
src_fb->gem_handle,
@@ -2750,6 +2919,7 @@ static void blitcopy(const struct igt_fb *dst_fb,
if (ctx)
gem_context_destroy(dst_fb->fd, ctx);
put_ahnd(ahnd);
+ intel_ctx_destroy(src_fb->fd, ictx);
}
static void free_linear_mapping(struct fb_blit_upload *blit)
--
2.39.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [igt-dev] ✗ GitLab.Pipeline: warning for switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw
2023-03-27 20:12 [igt-dev] [PATCH i-g-t 0/2] switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw Juha-Pekka Heikkila
2023-03-27 20:12 ` [igt-dev] [PATCH i-g-t 1/2] lib/i915/i915_blt: Add offset to block and fast copy Juha-Pekka Heikkila
2023-03-27 20:12 ` [igt-dev] [PATCH i-g-t 2/2] lib/igt_fb: switch blitcopy to use lib/i915/i915_blt functions Juha-Pekka Heikkila
@ 2023-03-27 20:34 ` Patchwork
2023-03-27 20:57 ` [igt-dev] ✗ Fi.CI.BAT: failure " Patchwork
` (2 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2023-03-27 20:34 UTC (permalink / raw)
To: Juha-Pekka Heikkila; +Cc: igt-dev
== Series Details ==
Series: switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw
URL : https://patchwork.freedesktop.org/series/115691/
State : warning
== Summary ==
Pipeline status: FAILED.
see https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/pipelines/840569 for the overview.
build:tests-debian-meson-mips has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/38841929):
$ /host/bin/curl -s -L --cacert /host/ca-certificates.crt --retry 4 -f --retry-delay 60 https://gitlab.freedesktop.org/freedesktop/helm-gitlab-infra/-/raw/main/runner-gating/runner-gating.sh | sh -s -- pre_get_sources_script
Checking if the user of the pipeline is allowed...
Checking if the job's project is part of a well-known group...
Thank you for contributing to freedesktop.org
Fetching changes...
Reinitialized existing Git repository in /builds/gfx-ci/igt-ci-tags/.git/
Checking out da24ac32 as detached HEAD (ref is intel/IGTPW_8690)...
Removing build/
Removing lib/i915/perf-configs/__pycache__/
Skipping Git submodules setup
section_end:1679948917:get_sources
section_start:1679948917:step_script
Executing "step_script" stage of the job script
Using docker image sha256:cc55efdc667be826910d414a562c76ce1130a9c15255a0dd115431bc42f83448 for registry.freedesktop.org/gfx-ci/igt-ci-tags/build-debian-mips:commit-da24ac3246920fd14c02a16591084221e278645c with digest registry.freedesktop.org/gfx-ci/igt-ci-tags/build-debian-mips@sha256:c829c44880da4782b7a72626c259ac6904b4bd5f08601e66b3be889ca1c0cf79 ...
section_end:1679948918:step_script
section_start:1679948918:cleanup_file_variables
Cleaning up project directory and file based variables
section_end:1679948919:cleanup_file_variables
ERROR: Job failed (system failure): Error response from daemon: no such image: docker.io/library/sha256:cc55efdc667be826910d414a562c76ce1130a9c15255a0dd115431bc42f83448: image not known (docker.go:535:0s)
build:tests-debian-minimal has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/38841926):
$ /host/bin/curl -s -L --cacert /host/ca-certificates.crt --retry 4 -f --retry-delay 60 https://gitlab.freedesktop.org/freedesktop/helm-gitlab-infra/-/raw/main/runner-gating/runner-gating.sh | sh -s -- pre_get_sources_script
Checking if the user of the pipeline is allowed...
Checking if the job's project is part of a well-known group...
Thank you for contributing to freedesktop.org
Fetching changes...
Reinitialized existing Git repository in /builds/gfx-ci/igt-ci-tags/.git/
Checking out da24ac32 as detached HEAD (ref is intel/IGTPW_8690)...
Removing build/
Removing lib/i915/perf-configs/__pycache__/
Skipping Git submodules setup
section_end:1679948917:get_sources
section_start:1679948917:step_script
Executing "step_script" stage of the job script
Using docker image sha256:08904a47f4efcc161569a9b7f88c458fc6e3e85a2225132246af9cf5cc6c4e5b for registry.freedesktop.org/gfx-ci/igt-ci-tags/build-debian-minimal:commit-da24ac3246920fd14c02a16591084221e278645c with digest registry.freedesktop.org/gfx-ci/igt-ci-tags/build-debian-minimal@sha256:8b51a86fd81e64c501c9521c37fc8ad6f2550976931c510eb0ff4f6a328d477a ...
section_end:1679948917:step_script
section_start:1679948917:cleanup_file_variables
Cleaning up project directory and file based variables
section_end:1679948919:cleanup_file_variables
ERROR: Job failed (system failure): Error response from daemon: container f7d517199d16935569d86fdb090a966a47e97f52eb5388f64c53cecb0a855e5c does not exist in database: no such container (exec.go:78:0s)
== Logs ==
For more details see: https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/pipelines/840569
^ permalink raw reply [flat|nested] 14+ messages in thread
* [igt-dev] ✗ Fi.CI.BAT: failure for switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw
2023-03-27 20:12 [igt-dev] [PATCH i-g-t 0/2] switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw Juha-Pekka Heikkila
` (2 preceding siblings ...)
2023-03-27 20:34 ` [igt-dev] ✗ GitLab.Pipeline: warning for switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw Patchwork
@ 2023-03-27 20:57 ` Patchwork
2023-03-28 6:36 ` [igt-dev] ✓ Fi.CI.BAT: success for switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw (rev2) Patchwork
2023-03-28 14:00 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork
5 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2023-03-27 20:57 UTC (permalink / raw)
To: Juha-Pekka Heikkila; +Cc: igt-dev
[-- Attachment #1: Type: text/plain, Size: 2816 bytes --]
== Series Details ==
Series: switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw
URL : https://patchwork.freedesktop.org/series/115691/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_12921 -> IGTPW_8690
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with IGTPW_8690 absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in IGTPW_8690, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8690/index.html
Participating hosts (37 -> 36)
------------------------------
Missing (1): fi-kbl-soraka
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in IGTPW_8690:
### IGT changes ###
#### Possible regressions ####
* igt@kms_pipe_crc_basic@compare-crc-sanitycheck@pipe-d-dp-1:
- bat-dg2-8: [PASS][1] -> [FAIL][2]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12921/bat-dg2-8/igt@kms_pipe_crc_basic@compare-crc-sanitycheck@pipe-d-dp-1.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8690/bat-dg2-8/igt@kms_pipe_crc_basic@compare-crc-sanitycheck@pipe-d-dp-1.html
Known issues
------------
Here are the changes found in IGTPW_8690 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@i915_selftest@live@requests:
- bat-rpls-2: [PASS][3] -> [ABORT][4] ([i915#4983] / [i915#7913])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12921/bat-rpls-2/igt@i915_selftest@live@requests.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8690/bat-rpls-2/igt@i915_selftest@live@requests.html
* igt@i915_selftest@live@reset:
- bat-rpls-1: [PASS][5] -> [ABORT][6] ([i915#4983])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12921/bat-rpls-1/igt@i915_selftest@live@reset.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8690/bat-rpls-1/igt@i915_selftest@live@reset.html
[i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
[i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
Build changes
-------------
* CI: CI-20190529 -> None
* IGT: IGT_7221 -> IGTPW_8690
CI-20190529: 20190529
CI_DRM_12921: 3de6040ce9900a94ec626662d5c6a227b37eeb1c @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_8690: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8690/index.html
IGT_7221: 4b77c6d85024d22ca521d510f8eee574128fe04f @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8690/index.html
[-- Attachment #2: Type: text/html, Size: 3546 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* [igt-dev] ✓ Fi.CI.BAT: success for switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw (rev2)
2023-03-27 20:12 [igt-dev] [PATCH i-g-t 0/2] switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw Juha-Pekka Heikkila
` (3 preceding siblings ...)
2023-03-27 20:57 ` [igt-dev] ✗ Fi.CI.BAT: failure " Patchwork
@ 2023-03-28 6:36 ` Patchwork
2023-03-28 14:00 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork
5 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2023-03-28 6:36 UTC (permalink / raw)
To: Juha-Pekka Heikkila; +Cc: igt-dev
[-- Attachment #1: Type: text/plain, Size: 4818 bytes --]
== Series Details ==
Series: switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw (rev2)
URL : https://patchwork.freedesktop.org/series/115691/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_12923 -> IGTPW_8694
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/index.html
Participating hosts (36 -> 36)
------------------------------
No changes in participating hosts
Known issues
------------
Here are the changes found in IGTPW_8694 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@i915_selftest@live@gt_heartbeat:
- fi-cfl-8109u: [PASS][1] -> [DMESG-FAIL][2] ([i915#5334])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/fi-cfl-8109u/igt@i915_selftest@live@gt_heartbeat.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/fi-cfl-8109u/igt@i915_selftest@live@gt_heartbeat.html
* igt@i915_selftest@live@slpc:
- bat-rplp-1: [PASS][3] -> [DMESG-FAIL][4] ([i915#6367] / [i915#7913])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/bat-rplp-1/igt@i915_selftest@live@slpc.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/bat-rplp-1/igt@i915_selftest@live@slpc.html
* igt@kms_chamelium_hpd@common-hpd-after-suspend:
- bat-dg2-11: NOTRUN -> [SKIP][5] ([i915#7828])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/bat-dg2-11/igt@kms_chamelium_hpd@common-hpd-after-suspend.html
* igt@kms_pipe_crc_basic@compare-crc-sanitycheck@pipe-d-dp-1:
- bat-dg2-8: [PASS][6] -> [FAIL][7] ([i915#7932])
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/bat-dg2-8/igt@kms_pipe_crc_basic@compare-crc-sanitycheck@pipe-d-dp-1.html
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/bat-dg2-8/igt@kms_pipe_crc_basic@compare-crc-sanitycheck@pipe-d-dp-1.html
* igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence:
- bat-dg2-11: NOTRUN -> [SKIP][8] ([i915#5354])
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html
#### Possible fixes ####
* igt@i915_selftest@live@gt_lrc:
- bat-dg2-11: [INCOMPLETE][9] ([i915#7609] / [i915#7913]) -> [PASS][10]
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/bat-dg2-11/igt@i915_selftest@live@gt_lrc.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/bat-dg2-11/igt@i915_selftest@live@gt_lrc.html
* igt@i915_selftest@live@guc:
- bat-rpls-1: [DMESG-WARN][11] ([i915#7852]) -> [PASS][12]
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/bat-rpls-1/igt@i915_selftest@live@guc.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/bat-rpls-1/igt@i915_selftest@live@guc.html
* igt@kms_pipe_crc_basic@nonblocking-crc@pipe-c-dp-1:
- bat-dg2-8: [FAIL][13] ([i915#7932]) -> [PASS][14]
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-c-dp-1.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-c-dp-1.html
#### Warnings ####
* igt@i915_selftest@live@slpc:
- bat-rpls-2: [DMESG-FAIL][15] ([i915#6367] / [i915#7913] / [i915#7996]) -> [DMESG-FAIL][16] ([i915#6367] / [i915#7913])
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/bat-rpls-2/igt@i915_selftest@live@slpc.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/bat-rpls-2/igt@i915_selftest@live@slpc.html
[i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
[i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
[i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
[i915#7609]: https://gitlab.freedesktop.org/drm/intel/issues/7609
[i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
[i915#7852]: https://gitlab.freedesktop.org/drm/intel/issues/7852
[i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
[i915#7932]: https://gitlab.freedesktop.org/drm/intel/issues/7932
[i915#7996]: https://gitlab.freedesktop.org/drm/intel/issues/7996
Build changes
-------------
* CI: CI-20190529 -> None
* IGT: IGT_7221 -> IGTPW_8694
CI-20190529: 20190529
CI_DRM_12923: cdd32ac83137835a85bad4ca4b4751ea90960e72 @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_8694: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/index.html
IGT_7221: 4b77c6d85024d22ca521d510f8eee574128fe04f @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/index.html
[-- Attachment #2: Type: text/html, Size: 6022 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/2] lib/igt_fb: switch blitcopy to use lib/i915/i915_blt functions
2023-03-27 20:12 ` [igt-dev] [PATCH i-g-t 2/2] lib/igt_fb: switch blitcopy to use lib/i915/i915_blt functions Juha-Pekka Heikkila
@ 2023-03-28 6:50 ` Zbigniew Kempczyński
2023-03-28 14:10 ` Karolina Stolarek
0 siblings, 1 reply; 14+ messages in thread
From: Zbigniew Kempczyński @ 2023-03-28 6:50 UTC (permalink / raw)
To: Juha-Pekka Heikkila; +Cc: igt-dev
On Mon, Mar 27, 2023 at 11:12:54PM +0300, Juha-Pekka Heikkila wrote:
> reduce code duplication
>
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---
> lib/igt_fb.c | 214 +++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 192 insertions(+), 22 deletions(-)
>
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index ba89e1f60..06ebe4818 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -35,6 +35,8 @@
> #include "drmtest.h"
> #include "i915/gem_create.h"
> #include "i915/gem_mman.h"
> +#include "i915/i915_blt.h"
> +#include "i915/intel_mocs.h"
> #include "igt_aux.h"
> #include "igt_color_encoding.h"
> #include "igt_fb.h"
> @@ -2680,12 +2682,134 @@ static void copy_with_engine(struct fb_blit_upload *blit,
> fini_buf(src);
> }
>
> +static struct blt_copy_object *blt_fb_init(const struct igt_fb *fb,
> + uint32_t plane, uint32_t memregion)
> +{
> + uint32_t name, handle;
> + struct blt_copy_object *blt;
> + enum blt_tiling_type blt_tile;
> + uint64_t stride;
> +
> + blt = malloc(sizeof(*blt));
> + igt_assert(blt);
> +
> + name = gem_flink(fb->fd, fb->gem_handle);
> + handle = gem_open(fb->fd, name);
> +
> + switch (igt_fb_mod_to_tiling(fb->modifier)) {
> + case I915_TILING_X:
> + blt_tile = T_XMAJOR;
I wonder to add helper in i915_blt: i915_tiling_to_blt_tile(int tile);
BTW Karolina added asserts which should map 1:1 i915 -> blt. Tile4 is
not added there yet but it can be, so i915_tiline_to_blt_tile() can
simply return tile;
then those few cases could looks like:
case I915_TILING_X:
case I915_TILING_Y:
case I915_TILING_4:
blt_tile = i915_tiling_to_blt_tile(...);
stride = fb->strides[plane] / 4;
break;
case I915_TILING_NONE:
...
> + stride = fb->strides[plane] / 4;
> + break;
> + case I915_TILING_Y:
> + blt_tile = T_YMAJOR;
> + stride = fb->strides[plane] / 4;
> + break;
> + case I915_TILING_4:
> + blt_tile = T_TILE4;
> + stride = fb->strides[plane] / 4;
> + break;
> + case I915_TILING_NONE:
> + default:
> + blt_tile = T_LINEAR;
> + stride = fb->strides[plane];
> + break;
> + }
> +
> + blt_set_object(blt, handle, fb->size, memregion,
> + intel_get_uc_mocs(fb->fd),
> + blt_tile,
> + is_ccs_modifier(fb->modifier) ? COMPRESSION_ENABLED : COMPRESSION_DISABLED,
> + is_gen12_mc_ccs_modifier(fb->modifier) ? COMPRESSION_TYPE_MEDIA : COMPRESSION_TYPE_3D);
> +
> + blt_set_geom(blt, stride, 0, 0, fb->width, fb->plane_height[plane], 0, 0);
> +
> + blt->offset = fb->offsets[plane];
> +
> + blt->ptr = gem_mmap__device_coherent(fb->fd, handle, 0, fb->size,
> + PROT_READ | PROT_WRITE);
> + return blt;
> +}
> +
> +static enum blt_color_depth blt_get_bpp(const struct igt_fb *fb)
> +{
> + switch (fb->plane_bpp[0]) {
> + case 8:
> + return CD_8bit;
> + case 16:
> + return CD_16bit;
> + case 32:
> + return CD_32bit;
> + case 64:
> + return CD_64bit;
> + case 96:
> + return CD_96bit;
> + case 128:
> + return CD_128bit;
> + default:
> + igt_assert(0);
> + }
> +}
> +
> +#define BLT_TARGET_RC(x) (x.compression == COMPRESSION_ENABLED && \
> + x.compression_type == COMPRESSION_TYPE_3D)
> +
> +#define BLT_TARGET_MC(x) (x.compression == COMPRESSION_ENABLED && \
> + x.compression_type == COMPRESSION_TYPE_MEDIA)
> +
> +static uint32_t blt_compression_format(struct blt_copy_data *blt,
> + const struct igt_fb *fb)
> +{
> + if (blt->src.compression == COMPRESSION_DISABLED &&
> + blt->dst.compression == COMPRESSION_DISABLED)
> + return 0;
> +
> + if (BLT_TARGET_RC(blt->src) || BLT_TARGET_RC(blt->dst)) {
> + switch (blt->color_depth)
> + {
> + case CD_32bit:
> + return 8;
> + default:
> + igt_assert_f(0, "COMPRESSION_TYPE_3D unknown color depth\n");
> + }
> + }
> +
> + if (BLT_TARGET_MC(blt->src) || BLT_TARGET_MC(blt->dst)) {
> + switch (fb->drm_format)
> + {
> + case DRM_FORMAT_XRGB8888:
> + return 8;
> + case DRM_FORMAT_XYUV8888:
> + return 9;
> + case DRM_FORMAT_NV12:
> + return 9;
> + case DRM_FORMAT_P010:
> + case DRM_FORMAT_P012:
> + case DRM_FORMAT_P016:
> + return 8;
> + default:
> + igt_assert_f(0, "COMPRESSION_TYPE_MEDIA unknown format\n");
> + }
> + } else {
> + igt_assert_f(0, "unknown compression\n");
> + }
> +}
> +
> static void blitcopy(const struct igt_fb *dst_fb,
> const struct igt_fb *src_fb)
> {
> uint32_t src_tiling, dst_tiling;
> uint32_t ctx = 0;
> uint64_t ahnd = 0;
> + const intel_ctx_t *ictx = intel_ctx_create_all_physical(src_fb->fd);
> + struct intel_execution_engine2 *e;
> + uint32_t bb;
> + uint64_t bb_size = 4096;
> + struct blt_copy_data blt = {};
> + struct blt_copy_object *src, *dst;
> + struct blt_block_copy_data_ext ext = {}, *pext = NULL;
> + uint32_t mem_region = HAS_FLATCCS(intel_get_drm_devid(src_fb->fd))
> + ? REGION_LMEM(0) : REGION_SMEM;
>
> igt_assert_eq(dst_fb->fd, src_fb->fd);
> igt_assert_eq(dst_fb->num_planes, src_fb->num_planes);
> @@ -2697,36 +2821,81 @@ static void blitcopy(const struct igt_fb *dst_fb,
> igt_require(gem_has_contexts(dst_fb->fd));
> ctx = gem_context_create(dst_fb->fd);
> ahnd = get_reloc_ahnd(dst_fb->fd, ctx);
> +
> + igt_assert(__gem_create_in_memory_regions(src_fb->fd,
> + &bb,
> + &bb_size,
> + mem_region) == 0);
> +
> + for_each_ctx_engine(src_fb->fd, ictx, e) {
> + if (gem_engine_can_block_copy(src_fb->fd, e))
> + break;
> + }
> }
>
> for (int i = 0; i < dst_fb->num_planes; i++) {
> igt_assert_eq(dst_fb->plane_bpp[i], src_fb->plane_bpp[i]);
> igt_assert_eq(dst_fb->plane_width[i], src_fb->plane_width[i]);
> igt_assert_eq(dst_fb->plane_height[i], src_fb->plane_height[i]);
> - /*
> - * On GEN12+ X-tiled format support is removed from the fast
> - * blit command, so use the XY_SRC blit command for it
> - * instead.
> - */
> +
> if (fast_blit_ok(src_fb) && fast_blit_ok(dst_fb)) {
I think this blit fail when ahnd == 0 as i915_blt requires softpinning.
'if (ahnd && fast_blit_ok(src_fb) && fast_blit_ok(dst_fb))' should work.
> - igt_blitter_fast_copy__raw(dst_fb->fd,
> - ahnd, ctx, NULL,
> - src_fb->gem_handle,
> - src_fb->offsets[i],
> - src_fb->strides[i],
> - src_tiling,
> - 0, 0, /* src_x, src_y */
> - src_fb->size,
> - dst_fb->plane_width[i],
> - dst_fb->plane_height[i],
> - dst_fb->plane_bpp[i],
> - dst_fb->gem_handle,
> - dst_fb->offsets[i],
> - dst_fb->strides[i],
> - dst_tiling,
> - 0, 0 /* dst_x, dst_y */,
> - dst_fb->size);
> + memset(&blt, 0, sizeof(blt));
> + blt.color_depth = blt_get_bpp(src_fb);
> +
> + src = blt_fb_init(src_fb, i, mem_region);
> + dst = blt_fb_init(dst_fb, i, mem_region);
> +
> + blt_set_copy_object(&blt.src, src);
> + blt_set_copy_object(&blt.dst, dst);
> +
> + blt_set_batch(&blt.bb, bb, bb_size, mem_region);
> +
> + blt_fast_copy(src_fb->fd, ictx, e, ahnd, &blt);
> + gem_sync(src_fb->fd, blt.dst.handle);
> +
> + blt_destroy_object(src_fb->fd, src);
> + blt_destroy_object(dst_fb->fd, dst);
> + } else if (ahnd) {
I would also check if this else supports block-copy.
If you prefer newer instructions (like fast-copy) with i915_blt it
requires valid ahnd (>0). Instead of using wrapper:
ahnd = get_reloc_ahnd(fd, ctx);
you may enforce softpinning by:
ahnd = intel_allocator_open(fd, ctx, INTEL_ALLOCATOR_RELOC);
Then i915_blt functions will work as you expect. But this will change
previous logic to prefer older blits instead new ones.
--
Zbigniew
> + /*
> + * On GEN12+ X-tiled format support is removed from
> + * the fast blit command, so use the block copy blit
> + * command for it instead.
> + */
> + src = blt_fb_init(src_fb, i, mem_region);
> + dst = blt_fb_init(dst_fb, i, mem_region);
> +
> + memset(&blt, 0, sizeof(blt));
> + blt.print_bb = true;
> + blt.color_depth = blt_get_bpp(src_fb);
> + blt_set_copy_object(&blt.src, src);
> + blt_set_copy_object(&blt.dst, dst);
> +
> + if (HAS_FLATCCS(intel_get_drm_devid(src_fb->fd))) {
> + blt_set_object_ext(&ext.src,
> + blt_compression_format(&blt, src_fb),
> + src_fb->width, src_fb->height,
> + SURFACE_TYPE_2D);
> +
> + blt_set_object_ext(&ext.dst,
> + blt_compression_format(&blt, dst_fb),
> + dst_fb->width, dst_fb->height,
> + SURFACE_TYPE_2D);
> +
> + pext = &ext;
> + }
> +
> + blt_set_batch(&blt.bb, bb, bb_size, mem_region);
> +
> + blt_block_copy(src_fb->fd, ictx, e, ahnd, &blt, pext);
> + gem_sync(src_fb->fd, blt.dst.handle);
> +
> + blt_destroy_object(src_fb->fd, src);
> + blt_destroy_object(dst_fb->fd, dst);
> } else {
> + /*
> + * If on legacy hardware where relocations are supported
> + * we'll use XY_SRC blit command instead
> + */
> igt_blitter_src_copy(dst_fb->fd,
> ahnd, ctx, NULL,
> src_fb->gem_handle,
> @@ -2750,6 +2919,7 @@ static void blitcopy(const struct igt_fb *dst_fb,
> if (ctx)
> gem_context_destroy(dst_fb->fd, ctx);
> put_ahnd(ahnd);
> + intel_ctx_destroy(src_fb->fd, ictx);
> }
>
> static void free_linear_mapping(struct fb_blit_upload *blit)
> --
> 2.39.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [igt-dev] ✗ Fi.CI.IGT: failure for switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw (rev2)
2023-03-27 20:12 [igt-dev] [PATCH i-g-t 0/2] switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw Juha-Pekka Heikkila
` (4 preceding siblings ...)
2023-03-28 6:36 ` [igt-dev] ✓ Fi.CI.BAT: success for switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw (rev2) Patchwork
@ 2023-03-28 14:00 ` Patchwork
5 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2023-03-28 14:00 UTC (permalink / raw)
To: Juha-Pekka Heikkila; +Cc: igt-dev
[-- Attachment #1: Type: text/plain, Size: 17146 bytes --]
== Series Details ==
Series: switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw (rev2)
URL : https://patchwork.freedesktop.org/series/115691/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_12923_full -> IGTPW_8694_full
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with IGTPW_8694_full absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in IGTPW_8694_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/index.html
Participating hosts (7 -> 7)
------------------------------
No changes in participating hosts
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in IGTPW_8694_full:
### IGT changes ###
#### Possible regressions ####
* igt@kms_plane_scaling@plane-scaler-with-modifiers-unity-scaling@pipe-a-hdmi-a-1:
- shard-glk: [PASS][1] -> [FAIL][2] +195 similar issues
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/shard-glk8/igt@kms_plane_scaling@plane-scaler-with-modifiers-unity-scaling@pipe-a-hdmi-a-1.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/shard-glk1/igt@kms_plane_scaling@plane-scaler-with-modifiers-unity-scaling@pipe-a-hdmi-a-1.html
* igt@kms_plane_scaling@plane-upscale-with-modifiers-20x20@pipe-b-dp-1:
- shard-apl: [PASS][3] -> [FAIL][4] +128 similar issues
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/shard-apl7/igt@kms_plane_scaling@plane-upscale-with-modifiers-20x20@pipe-b-dp-1.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/shard-apl4/igt@kms_plane_scaling@plane-upscale-with-modifiers-20x20@pipe-b-dp-1.html
#### Warnings ####
* igt@i915_pipe_stress@stress-xrgb8888-ytiled:
- shard-apl: [FAIL][5] ([i915#7036]) -> [FAIL][6]
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/shard-apl1/igt@i915_pipe_stress@stress-xrgb8888-ytiled.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/shard-apl6/igt@i915_pipe_stress@stress-xrgb8888-ytiled.html
* igt@kms_flip_scaled_crc@flip-32bpp-yftile-to-64bpp-yftile-downscaling@pipe-a-valid-mode:
- shard-glk: [SKIP][7] ([fdo#109271]) -> [FAIL][8] +9 similar issues
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/shard-glk9/igt@kms_flip_scaled_crc@flip-32bpp-yftile-to-64bpp-yftile-downscaling@pipe-a-valid-mode.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/shard-glk4/igt@kms_flip_scaled_crc@flip-32bpp-yftile-to-64bpp-yftile-downscaling@pipe-a-valid-mode.html
* igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytilegen12rcccs-upscaling@pipe-a-valid-mode:
- shard-apl: [SKIP][9] ([fdo#109271]) -> [FAIL][10] +9 similar issues
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/shard-apl2/igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytilegen12rcccs-upscaling@pipe-a-valid-mode.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/shard-apl2/igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytilegen12rcccs-upscaling@pipe-a-valid-mode.html
#### Suppressed ####
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* igt@kms_big_fb@y-tiled-64bpp-rotate-180:
- {shard-tglu}: NOTRUN -> [FAIL][11]
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/shard-tglu-3/igt@kms_big_fb@y-tiled-64bpp-rotate-180.html
* igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-32bpp-ytileccs-upscaling@pipe-a-valid-mode:
- {shard-tglu}: [SKIP][12] ([i915#2587] / [i915#2672]) -> [FAIL][13] +1 similar issue
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/shard-tglu-9/igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-32bpp-ytileccs-upscaling@pipe-a-valid-mode.html
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/shard-tglu-2/igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-32bpp-ytileccs-upscaling@pipe-a-valid-mode.html
* igt@kms_flip_scaled_crc@flip-64bpp-xtile-to-16bpp-xtile-upscaling@pipe-a-valid-mode:
- {shard-dg1}: [PASS][14] -> [FAIL][15]
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/shard-dg1-16/igt@kms_flip_scaled_crc@flip-64bpp-xtile-to-16bpp-xtile-upscaling@pipe-a-valid-mode.html
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/shard-dg1-15/igt@kms_flip_scaled_crc@flip-64bpp-xtile-to-16bpp-xtile-upscaling@pipe-a-valid-mode.html
* igt@kms_plane_lowres@tiling-x@pipe-d-hdmi-a-4:
- {shard-dg1}: NOTRUN -> [FAIL][16] +5 similar issues
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/shard-dg1-15/igt@kms_plane_lowres@tiling-x@pipe-d-hdmi-a-4.html
* igt@kms_plane_scaling@plane-scaler-with-clipping-clamping-modifiers@pipe-c-hdmi-a-1:
- {shard-tglu}: [PASS][17] -> [FAIL][18] +118 similar issues
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/shard-tglu-6/igt@kms_plane_scaling@plane-scaler-with-clipping-clamping-modifiers@pipe-c-hdmi-a-1.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/shard-tglu-7/igt@kms_plane_scaling@plane-scaler-with-clipping-clamping-modifiers@pipe-c-hdmi-a-1.html
Known issues
------------
Here are the changes found in IGTPW_8694_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_mmap_gtt@fault-concurrent-x:
- shard-snb: [PASS][19] -> [ABORT][20] ([i915#5161])
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/shard-snb7/igt@gem_mmap_gtt@fault-concurrent-x.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/shard-snb7/igt@gem_mmap_gtt@fault-concurrent-x.html
* igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size:
- shard-apl: [PASS][21] -> [FAIL][22] ([i915#2346]) +1 similar issue
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/shard-apl3/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/shard-apl7/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
* igt@kms_flip@plain-flip-ts-check-interruptible@b-hdmi-a1:
- shard-glk: [PASS][23] -> [FAIL][24] ([i915#2122])
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/shard-glk2/igt@kms_flip@plain-flip-ts-check-interruptible@b-hdmi-a1.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/shard-glk9/igt@kms_flip@plain-flip-ts-check-interruptible@b-hdmi-a1.html
* igt@kms_frontbuffer_tracking@psr-2p-scndscrn-pri-indfb-draw-mmap-cpu:
- shard-snb: NOTRUN -> [SKIP][25] ([fdo#109271]) +55 similar issues
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/shard-snb4/igt@kms_frontbuffer_tracking@psr-2p-scndscrn-pri-indfb-draw-mmap-cpu.html
* igt@kms_rotation_crc@sprite-rotation-90-pos-100-0:
- shard-apl: [PASS][26] -> [DMESG-FAIL][27] ([IGT#6]) +1 similar issue
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/shard-apl3/igt@kms_rotation_crc@sprite-rotation-90-pos-100-0.html
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/shard-apl2/igt@kms_rotation_crc@sprite-rotation-90-pos-100-0.html
#### Possible fixes ####
* igt@gem_exec_fair@basic-pace-solo@rcs0:
- shard-apl: [FAIL][28] ([i915#2842]) -> [PASS][29]
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/shard-apl4/igt@gem_exec_fair@basic-pace-solo@rcs0.html
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/shard-apl1/igt@gem_exec_fair@basic-pace-solo@rcs0.html
* igt@i915_pm_dc@dc9-dpms:
- {shard-tglu}: [SKIP][30] ([i915#4281]) -> [PASS][31]
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/shard-tglu-5/igt@i915_pm_dc@dc9-dpms.html
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/shard-tglu-2/igt@i915_pm_dc@dc9-dpms.html
* igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions:
- shard-glk: [FAIL][32] ([i915#2346]) -> [PASS][33]
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/shard-glk2/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/shard-glk5/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html
* igt@kms_cursor_legacy@forked-move@pipe-b:
- {shard-dg1}: [INCOMPLETE][34] ([i915#8011]) -> [PASS][35]
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/shard-dg1-14/igt@kms_cursor_legacy@forked-move@pipe-b.html
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/shard-dg1-18/igt@kms_cursor_legacy@forked-move@pipe-b.html
#### Warnings ####
* igt@i915_pm_rps@reset:
- shard-snb: [INCOMPLETE][36] ([i915#7790]) -> [DMESG-FAIL][37] ([i915#8319])
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12923/shard-snb4/igt@i915_pm_rps@reset.html
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/shard-snb7/igt@i915_pm_rps@reset.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[IGT#6]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/6
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
[fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
[fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
[fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
[fdo#109303]: https://bugs.freedesktop.org/show_bug.cgi?id=109303
[fdo#109307]: https://bugs.freedesktop.org/show_bug.cgi?id=109307
[fdo#109506]: https://bugs.freedesktop.org/show_bug.cgi?id=109506
[fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
[fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
[fdo#111614]: https://bugs.freedesktop.org/show_bug.cgi?id=111614
[fdo#111615]: https://bugs.freedesktop.org/show_bug.cgi?id=111615
[fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825
[fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
[fdo#112054]: https://bugs.freedesktop.org/show_bug.cgi?id=112054
[fdo#112283]: https://bugs.freedesktop.org/show_bug.cgi?id=112283
[i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
[i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
[i915#1755]: https://gitlab.freedesktop.org/drm/intel/issues/1755
[i915#1839]: https://gitlab.freedesktop.org/drm/intel/issues/1839
[i915#2122]: https://gitlab.freedesktop.org/drm/intel/issues/2122
[i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346
[i915#2433]: https://gitlab.freedesktop.org/drm/intel/issues/2433
[i915#2434]: https://gitlab.freedesktop.org/drm/intel/issues/2434
[i915#2437]: https://gitlab.freedesktop.org/drm/intel/issues/2437
[i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
[i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
[i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587
[i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672
[i915#2681]: https://gitlab.freedesktop.org/drm/intel/issues/2681
[i915#2705]: https://gitlab.freedesktop.org/drm/intel/issues/2705
[i915#284]: https://gitlab.freedesktop.org/drm/intel/issues/284
[i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
[i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281
[i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
[i915#3297]: https://gitlab.freedesktop.org/drm/intel/issues/3297
[i915#3299]: https://gitlab.freedesktop.org/drm/intel/issues/3299
[i915#3359]: https://gitlab.freedesktop.org/drm/intel/issues/3359
[i915#3458]: https://gitlab.freedesktop.org/drm/intel/issues/3458
[i915#3539]: https://gitlab.freedesktop.org/drm/intel/issues/3539
[i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
[i915#3591]: https://gitlab.freedesktop.org/drm/intel/issues/3591
[i915#3638]: https://gitlab.freedesktop.org/drm/intel/issues/3638
[i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689
[i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
[i915#3742]: https://gitlab.freedesktop.org/drm/intel/issues/3742
[i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
[i915#3952]: https://gitlab.freedesktop.org/drm/intel/issues/3952
[i915#4036]: https://gitlab.freedesktop.org/drm/intel/issues/4036
[i915#404]: https://gitlab.freedesktop.org/drm/intel/issues/404
[i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
[i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
[i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
[i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
[i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
[i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
[i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
[i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
[i915#426]: https://gitlab.freedesktop.org/drm/intel/issues/426
[i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
[i915#4281]: https://gitlab.freedesktop.org/drm/intel/issues/4281
[i915#4387]: https://gitlab.freedesktop.org/drm/intel/issues/4387
[i915#4538]: https://gitlab.freedesktop.org/drm/intel/issues/4538
[i915#4565]: https://gitlab.freedesktop.org/drm/intel/issues/4565
[i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
[i915#4771]: https://gitlab.freedesktop.org/drm/intel/issues/4771
[i915#4812]: https://gitlab.freedesktop.org/drm/intel/issues/4812
[i915#4818]: https://gitlab.freedesktop.org/drm/intel/issues/4818
[i915#4833]: https://gitlab.freedesktop.org/drm/intel/issues/4833
[i915#4852]: https://gitlab.freedesktop.org/drm/intel/issues/4852
[i915#4854]: https://gitlab.freedesktop.org/drm/intel/issues/4854
[i915#4859]: https://gitlab.freedesktop.org/drm/intel/issues/4859
[i915#4860]: https://gitlab.freedesktop.org/drm/intel/issues/4860
[i915#4880]: https://gitlab.freedesktop.org/drm/intel/issues/4880
[i915#4881]: https://gitlab.freedesktop.org/drm/intel/issues/4881
[i915#5161]: https://gitlab.freedesktop.org/drm/intel/issues/5161
[i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
[i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235
[i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286
[i915#5288]: https://gitlab.freedesktop.org/drm/intel/issues/5288
[i915#5289]: https://gitlab.freedesktop.org/drm/intel/issues/5289
[i915#5325]: https://gitlab.freedesktop.org/drm/intel/issues/5325
[i915#5563]: https://gitlab.freedesktop.org/drm/intel/issues/5563
[i915#5852]: https://gitlab.freedesktop.org/drm/intel/issues/5852
[i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
[i915#6268]: https://gitlab.freedesktop.org/drm/intel/issues/6268
[i915#6301]: https://gitlab.freedesktop.org/drm/intel/issues/6301
[i915#6433]: https://gitlab.freedesktop.org/drm/intel/issues/6433
[i915#6524]: https://gitlab.freedesktop.org/drm/intel/issues/6524
[i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
[i915#6946]: https://gitlab.freedesktop.org/drm/intel/issues/6946
[i915#6953]: https://gitlab.freedesktop.org/drm/intel/issues/6953
[i915#7036]: https://gitlab.freedesktop.org/drm/intel/issues/7036
[i915#7116]: https://gitlab.freedesktop.org/drm/intel/issues/7116
[i915#7479]: https://gitlab.freedesktop.org/drm/intel/issues/7479
[i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
[i915#7697]: https://gitlab.freedesktop.org/drm/intel/issues/7697
[i915#7711]: https://gitlab.freedesktop.org/drm/intel/issues/7711
[i915#7790]: https://gitlab.freedesktop.org/drm/intel/issues/7790
[i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
[i915#8011]: https://gitlab.freedesktop.org/drm/intel/issues/8011
[i915#8247]: https://gitlab.freedesktop.org/drm/intel/issues/8247
[i915#8292]: https://gitlab.freedesktop.org/drm/intel/issues/8292
[i915#8319]: https://gitlab.freedesktop.org/drm/intel/issues/8319
Build changes
-------------
* CI: CI-20190529 -> None
* IGT: IGT_7221 -> IGTPW_8694
* Piglit: piglit_4509 -> None
CI-20190529: 20190529
CI_DRM_12923: cdd32ac83137835a85bad4ca4b4751ea90960e72 @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_8694: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/index.html
IGT_7221: 4b77c6d85024d22ca521d510f8eee574128fe04f @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8694/index.html
[-- Attachment #2: Type: text/html, Size: 12269 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/2] lib/igt_fb: switch blitcopy to use lib/i915/i915_blt functions
2023-03-28 6:50 ` Zbigniew Kempczyński
@ 2023-03-28 14:10 ` Karolina Stolarek
2023-03-28 17:29 ` Juha-Pekka Heikkila
2023-03-29 17:19 ` Zbigniew Kempczyński
0 siblings, 2 replies; 14+ messages in thread
From: Karolina Stolarek @ 2023-03-28 14:10 UTC (permalink / raw)
To: Zbigniew Kempczyński, Juha-Pekka Heikkila; +Cc: igt-dev
On 28.03.2023 08:50, Zbigniew Kempczyński wrote:
> On Mon, Mar 27, 2023 at 11:12:54PM +0300, Juha-Pekka Heikkila wrote:
>> reduce code duplication
>>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>> ---
>> lib/igt_fb.c | 214 +++++++++++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 192 insertions(+), 22 deletions(-)
>>
>> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
>> index ba89e1f60..06ebe4818 100644
>> --- a/lib/igt_fb.c
>> +++ b/lib/igt_fb.c
>> @@ -35,6 +35,8 @@
>> #include "drmtest.h"
>> #include "i915/gem_create.h"
>> #include "i915/gem_mman.h"
>> +#include "i915/i915_blt.h"
>> +#include "i915/intel_mocs.h"
>> #include "igt_aux.h"
>> #include "igt_color_encoding.h"
>> #include "igt_fb.h"
>> @@ -2680,12 +2682,134 @@ static void copy_with_engine(struct fb_blit_upload *blit,
>> fini_buf(src);
>> }
>>
>> +static struct blt_copy_object *blt_fb_init(const struct igt_fb *fb,
>> + uint32_t plane, uint32_t memregion)
>> +{
>> + uint32_t name, handle;
>> + struct blt_copy_object *blt;
>> + enum blt_tiling_type blt_tile;
>> + uint64_t stride;
>> +
>> + blt = malloc(sizeof(*blt));
>> + igt_assert(blt);
>> +
>> + name = gem_flink(fb->fd, fb->gem_handle);
>> + handle = gem_open(fb->fd, name);
>> +
>> + switch (igt_fb_mod_to_tiling(fb->modifier)) {
>> + case I915_TILING_X:
>> + blt_tile = T_XMAJOR;
>
> I wonder to add helper in i915_blt: i915_tiling_to_blt_tile(int tile);
I wish we could move on from using I915_TILING_* for tiling formats that
don't require fences. It would be great if we could switch on
blt_tiling_type when doing blit copy, especially when we have checks
that ensure the correct mapping.
I mean, we could add such a helper, but do we want to keep it as a
permanent solution?
>
> BTW Karolina added asserts which should map 1:1 i915 -> blt. Tile4 is
> not added there yet but it can be, so i915_tiline_to_blt_tile() can
> simply return tile;
>
> then those few cases could looks like:
>
> case I915_TILING_X:
> case I915_TILING_Y:
> case I915_TILING_4:
> blt_tile = i915_tiling_to_blt_tile(...);
> stride = fb->strides[plane] / 4;
> break;
> case I915_TILING_NONE:
> ...
>
>> + stride = fb->strides[plane] / 4;
>> + break;
>> + case I915_TILING_Y:
>> + blt_tile = T_YMAJOR;
>> + stride = fb->strides[plane] / 4;
>> + break;
>> + case I915_TILING_4:
>> + blt_tile = T_TILE4;
>> + stride = fb->strides[plane] / 4;
>> + break;
>> + case I915_TILING_NONE:
>> + default:
>> + blt_tile = T_LINEAR;
>> + stride = fb->strides[plane];
>> + break;
>> + }
>> +
>> + blt_set_object(blt, handle, fb->size, memregion,
>> + intel_get_uc_mocs(fb->fd),
>> + blt_tile,
>> + is_ccs_modifier(fb->modifier) ? COMPRESSION_ENABLED : COMPRESSION_DISABLED,
>> + is_gen12_mc_ccs_modifier(fb->modifier) ? COMPRESSION_TYPE_MEDIA : COMPRESSION_TYPE_3D);
>> +
>> + blt_set_geom(blt, stride, 0, 0, fb->width, fb->plane_height[plane], 0, 0);
>> +
>> + blt->offset = fb->offsets[plane];
>> +
>> + blt->ptr = gem_mmap__device_coherent(fb->fd, handle, 0, fb->size,
>> + PROT_READ | PROT_WRITE);
>> + return blt;
>> +}
>> +
>> +static enum blt_color_depth blt_get_bpp(const struct igt_fb *fb)
>> +{
>> + switch (fb->plane_bpp[0]) {
>> + case 8:
>> + return CD_8bit;
>> + case 16:
>> + return CD_16bit;
>> + case 32:
>> + return CD_32bit;
>> + case 64:
>> + return CD_64bit;
>> + case 96:
>> + return CD_96bit;
>> + case 128:
>> + return CD_128bit;
>> + default:
>> + igt_assert(0);
>> + }
>> +}
>> +
>> +#define BLT_TARGET_RC(x) (x.compression == COMPRESSION_ENABLED && \
>> + x.compression_type == COMPRESSION_TYPE_3D)
>> +
>> +#define BLT_TARGET_MC(x) (x.compression == COMPRESSION_ENABLED && \
>> + x.compression_type == COMPRESSION_TYPE_MEDIA)
>> +
>> +static uint32_t blt_compression_format(struct blt_copy_data *blt,
>> + const struct igt_fb *fb)
>> +{
>> + if (blt->src.compression == COMPRESSION_DISABLED &&
>> + blt->dst.compression == COMPRESSION_DISABLED)
>> + return 0;
>> +
>> + if (BLT_TARGET_RC(blt->src) || BLT_TARGET_RC(blt->dst)) {
>> + switch (blt->color_depth)
>> + {
>> + case CD_32bit:
>> + return 8;
>> + default:
>> + igt_assert_f(0, "COMPRESSION_TYPE_3D unknown color depth\n");
>> + }
>> + }
>> +
>> + if (BLT_TARGET_MC(blt->src) || BLT_TARGET_MC(blt->dst)) {
>> + switch (fb->drm_format)
>> + {
>> + case DRM_FORMAT_XRGB8888:
>> + return 8;
>> + case DRM_FORMAT_XYUV8888:
>> + return 9;
>> + case DRM_FORMAT_NV12:
>> + return 9;
>> + case DRM_FORMAT_P010:
>> + case DRM_FORMAT_P012:
>> + case DRM_FORMAT_P016:
>> + return 8;
>> + default:
>> + igt_assert_f(0, "COMPRESSION_TYPE_MEDIA unknown format\n");
>> + }
>> + } else {
>> + igt_assert_f(0, "unknown compression\n");
>> + }
>> +}
>> +
>> static void blitcopy(const struct igt_fb *dst_fb,
>> const struct igt_fb *src_fb)
>> {
>> uint32_t src_tiling, dst_tiling;
>> uint32_t ctx = 0;
>> uint64_t ahnd = 0;
>> + const intel_ctx_t *ictx = intel_ctx_create_all_physical(src_fb->fd);
>> + struct intel_execution_engine2 *e;
>> + uint32_t bb;
>> + uint64_t bb_size = 4096;
>> + struct blt_copy_data blt = {};
>> + struct blt_copy_object *src, *dst;
>> + struct blt_block_copy_data_ext ext = {}, *pext = NULL;
>> + uint32_t mem_region = HAS_FLATCCS(intel_get_drm_devid(src_fb->fd))
>> + ? REGION_LMEM(0) : REGION_SMEM;
>>
>> igt_assert_eq(dst_fb->fd, src_fb->fd);
>> igt_assert_eq(dst_fb->num_planes, src_fb->num_planes);
>> @@ -2697,36 +2821,81 @@ static void blitcopy(const struct igt_fb *dst_fb,
>> igt_require(gem_has_contexts(dst_fb->fd));
>> ctx = gem_context_create(dst_fb->fd);
>> ahnd = get_reloc_ahnd(dst_fb->fd, ctx);
>> +
>> + igt_assert(__gem_create_in_memory_regions(src_fb->fd,
>> + &bb,
>> + &bb_size,
>> + mem_region) == 0);
>> +
>> + for_each_ctx_engine(src_fb->fd, ictx, e) {
>> + if (gem_engine_can_block_copy(src_fb->fd, e))
>> + break;
>> + }
>> }
>>
>> for (int i = 0; i < dst_fb->num_planes; i++) {
>> igt_assert_eq(dst_fb->plane_bpp[i], src_fb->plane_bpp[i]);
>> igt_assert_eq(dst_fb->plane_width[i], src_fb->plane_width[i]);
>> igt_assert_eq(dst_fb->plane_height[i], src_fb->plane_height[i]);
>> - /*
>> - * On GEN12+ X-tiled format support is removed from the fast
>> - * blit command, so use the XY_SRC blit command for it
>> - * instead.
>> - */
>> +
>> if (fast_blit_ok(src_fb) && fast_blit_ok(dst_fb)) {
>
> I think this blit fail when ahnd == 0 as i915_blt requires softpinning.
Also, are we fine with the fact that we'll only cover gen 8 and above
(due to the softpinning requirement)?
>
> 'if (ahnd && fast_blit_ok(src_fb) && fast_blit_ok(dst_fb))' should work.
I had a quick look at the function, and I wonder if we could i915_blt
helpers here instead. There seem to be other factors that we have to
consider here, but the display version checks seem similar to what we
have in intel_cmd_info lib.
All the best,
Karolina
>
>> - igt_blitter_fast_copy__raw(dst_fb->fd,
>> - ahnd, ctx, NULL,
>> - src_fb->gem_handle,
>> - src_fb->offsets[i],
>> - src_fb->strides[i],
>> - src_tiling,
>> - 0, 0, /* src_x, src_y */
>> - src_fb->size,
>> - dst_fb->plane_width[i],
>> - dst_fb->plane_height[i],
>> - dst_fb->plane_bpp[i],
>> - dst_fb->gem_handle,
>> - dst_fb->offsets[i],
>> - dst_fb->strides[i],
>> - dst_tiling,
>> - 0, 0 /* dst_x, dst_y */,
>> - dst_fb->size);
>> + memset(&blt, 0, sizeof(blt));
>> + blt.color_depth = blt_get_bpp(src_fb);
>> +
>> + src = blt_fb_init(src_fb, i, mem_region);
>> + dst = blt_fb_init(dst_fb, i, mem_region);
>> +
>> + blt_set_copy_object(&blt.src, src);
>> + blt_set_copy_object(&blt.dst, dst);
>> +
>> + blt_set_batch(&blt.bb, bb, bb_size, mem_region);
>> +
>> + blt_fast_copy(src_fb->fd, ictx, e, ahnd, &blt);
>> + gem_sync(src_fb->fd, blt.dst.handle);
>> +
>> + blt_destroy_object(src_fb->fd, src);
>> + blt_destroy_object(dst_fb->fd, dst);
>> + } else if (ahnd) {
>
> I would also check if this else supports block-copy.
>
> If you prefer newer instructions (like fast-copy) with i915_blt it
> requires valid ahnd (>0). Instead of using wrapper:
>
> ahnd = get_reloc_ahnd(fd, ctx);
>
> you may enforce softpinning by:
>
> ahnd = intel_allocator_open(fd, ctx, INTEL_ALLOCATOR_RELOC);
>
> Then i915_blt functions will work as you expect. But this will change
> previous logic to prefer older blits instead new ones.
>
> --
> Zbigniew
>
>> + /*
>> + * On GEN12+ X-tiled format support is removed from
>> + * the fast blit command, so use the block copy blit
>> + * command for it instead.
>> + */
>> + src = blt_fb_init(src_fb, i, mem_region);
>> + dst = blt_fb_init(dst_fb, i, mem_region);
>> +
>> + memset(&blt, 0, sizeof(blt));
>> + blt.print_bb = true;
>> + blt.color_depth = blt_get_bpp(src_fb);
>> + blt_set_copy_object(&blt.src, src);
>> + blt_set_copy_object(&blt.dst, dst);
>> +
>> + if (HAS_FLATCCS(intel_get_drm_devid(src_fb->fd))) {
>> + blt_set_object_ext(&ext.src,
>> + blt_compression_format(&blt, src_fb),
>> + src_fb->width, src_fb->height,
>> + SURFACE_TYPE_2D);
>> +
>> + blt_set_object_ext(&ext.dst,
>> + blt_compression_format(&blt, dst_fb),
>> + dst_fb->width, dst_fb->height,
>> + SURFACE_TYPE_2D);
>> +
>> + pext = &ext;
>> + }
>> +
>> + blt_set_batch(&blt.bb, bb, bb_size, mem_region);
>> +
>> + blt_block_copy(src_fb->fd, ictx, e, ahnd, &blt, pext);
>> + gem_sync(src_fb->fd, blt.dst.handle);
>> +
>> + blt_destroy_object(src_fb->fd, src);
>> + blt_destroy_object(dst_fb->fd, dst);
>> } else {
>> + /*
>> + * If on legacy hardware where relocations are supported
>> + * we'll use XY_SRC blit command instead
>> + */
>> igt_blitter_src_copy(dst_fb->fd,
>> ahnd, ctx, NULL,
>> src_fb->gem_handle,
>> @@ -2750,6 +2919,7 @@ static void blitcopy(const struct igt_fb *dst_fb,
>> if (ctx)
>> gem_context_destroy(dst_fb->fd, ctx);
>> put_ahnd(ahnd);
>> + intel_ctx_destroy(src_fb->fd, ictx);
>> }
>>
>> static void free_linear_mapping(struct fb_blit_upload *blit)
>> --
>> 2.39.0
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/2] lib/igt_fb: switch blitcopy to use lib/i915/i915_blt functions
2023-03-28 14:10 ` Karolina Stolarek
@ 2023-03-28 17:29 ` Juha-Pekka Heikkila
2023-03-29 6:53 ` Karolina Stolarek
2023-03-29 17:30 ` Zbigniew Kempczyński
2023-03-29 17:19 ` Zbigniew Kempczyński
1 sibling, 2 replies; 14+ messages in thread
From: Juha-Pekka Heikkila @ 2023-03-28 17:29 UTC (permalink / raw)
To: Karolina Stolarek, Zbigniew Kempczyński; +Cc: igt-dev
Hei,
thanks for the comments. This was my first attempt on this and I got
those failures which Zbigniew's comment explained how those happen. I'll
soon send new version.. I was also wondering about insertion of that
offset in the other patch, it somehow feels bit out of place but didn't
find better place for it.
couple of comments below.
On 28.3.2023 17.10, Karolina Stolarek wrote:
> On 28.03.2023 08:50, Zbigniew Kempczyński wrote:
>> On Mon, Mar 27, 2023 at 11:12:54PM +0300, Juha-Pekka Heikkila wrote:
>>> reduce code duplication
>>>
>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>>> ---
>>> lib/igt_fb.c | 214 +++++++++++++++++++++++++++++++++++++++++++++------
>>> 1 file changed, 192 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
>>> index ba89e1f60..06ebe4818 100644
>>> --- a/lib/igt_fb.c
>>> +++ b/lib/igt_fb.c
>>> @@ -35,6 +35,8 @@
>>> #include "drmtest.h"
>>> #include "i915/gem_create.h"
>>> #include "i915/gem_mman.h"
>>> +#include "i915/i915_blt.h"
>>> +#include "i915/intel_mocs.h"
>>> #include "igt_aux.h"
>>> #include "igt_color_encoding.h"
>>> #include "igt_fb.h"
>>> @@ -2680,12 +2682,134 @@ static void copy_with_engine(struct
>>> fb_blit_upload *blit,
>>> fini_buf(src);
>>> }
>>> +static struct blt_copy_object *blt_fb_init(const struct igt_fb *fb,
>>> + uint32_t plane, uint32_t memregion)
>>> +{
>>> + uint32_t name, handle;
>>> + struct blt_copy_object *blt;
>>> + enum blt_tiling_type blt_tile;
>>> + uint64_t stride;
>>> +
>>> + blt = malloc(sizeof(*blt));
>>> + igt_assert(blt);
>>> +
>>> + name = gem_flink(fb->fd, fb->gem_handle);
>>> + handle = gem_open(fb->fd, name);
>>> +
>>> + switch (igt_fb_mod_to_tiling(fb->modifier)) {
>>> + case I915_TILING_X:
>>> + blt_tile = T_XMAJOR;
>>
>> I wonder to add helper in i915_blt: i915_tiling_to_blt_tile(int tile);
>
> I wish we could move on from using I915_TILING_* for tiling formats that
> don't require fences. It would be great if we could switch on
> blt_tiling_type when doing blit copy, especially when we have checks
> that ensure the correct mapping.
>
> I mean, we could add such a helper, but do we want to keep it as a
> permanent solution?
>
>>
>> BTW Karolina added asserts which should map 1:1 i915 -> blt. Tile4 is
>> not added there yet but it can be, so i915_tiline_to_blt_tile() can
>> simply return tile;
>>
>> then those few cases could looks like:
>>
>> case I915_TILING_X:
>> case I915_TILING_Y:
>> case I915_TILING_4:
>> blt_tile = i915_tiling_to_blt_tile(...);
>> stride = fb->strides[plane] / 4;
>> break;
>> case I915_TILING_NONE:
>> ...
i915_tiling_to_blt_tile is coming in some yet unmerged set? I didn't
find it in my igt.
>>
>>> + stride = fb->strides[plane] / 4;
>>> + break;
>>> + case I915_TILING_Y:
>>> + blt_tile = T_YMAJOR;
>>> + stride = fb->strides[plane] / 4;
>>> + break;
>>> + case I915_TILING_4:
>>> + blt_tile = T_TILE4;
>>> + stride = fb->strides[plane] / 4;
>>> + break;
>>> + case I915_TILING_NONE:
>>> + default:
>>> + blt_tile = T_LINEAR;
>>> + stride = fb->strides[plane];
>>> + break;
>>> + }
>>> +
>>> + blt_set_object(blt, handle, fb->size, memregion,
>>> + intel_get_uc_mocs(fb->fd),
>>> + blt_tile,
>>> + is_ccs_modifier(fb->modifier) ? COMPRESSION_ENABLED :
>>> COMPRESSION_DISABLED,
>>> + is_gen12_mc_ccs_modifier(fb->modifier) ?
>>> COMPRESSION_TYPE_MEDIA : COMPRESSION_TYPE_3D);
>>> +
>>> + blt_set_geom(blt, stride, 0, 0, fb->width,
>>> fb->plane_height[plane], 0, 0);
>>> +
>>> + blt->offset = fb->offsets[plane];
>>> +
>>> + blt->ptr = gem_mmap__device_coherent(fb->fd, handle, 0, fb->size,
>>> + PROT_READ | PROT_WRITE);
>>> + return blt;
>>> +}
>>> +
>>> +static enum blt_color_depth blt_get_bpp(const struct igt_fb *fb)
>>> +{
>>> + switch (fb->plane_bpp[0]) {
>>> + case 8:
>>> + return CD_8bit;
>>> + case 16:
>>> + return CD_16bit;
>>> + case 32:
>>> + return CD_32bit;
>>> + case 64:
>>> + return CD_64bit;
>>> + case 96:
>>> + return CD_96bit;
>>> + case 128:
>>> + return CD_128bit;
>>> + default:
>>> + igt_assert(0);
>>> + }
>>> +}
>>> +
>>> +#define BLT_TARGET_RC(x) (x.compression == COMPRESSION_ENABLED && \
>>> + x.compression_type == COMPRESSION_TYPE_3D)
>>> +
>>> +#define BLT_TARGET_MC(x) (x.compression == COMPRESSION_ENABLED && \
>>> + x.compression_type == COMPRESSION_TYPE_MEDIA)
>>> +
>>> +static uint32_t blt_compression_format(struct blt_copy_data *blt,
>>> + const struct igt_fb *fb)
>>> +{
>>> + if (blt->src.compression == COMPRESSION_DISABLED &&
>>> + blt->dst.compression == COMPRESSION_DISABLED)
>>> + return 0;
>>> +
>>> + if (BLT_TARGET_RC(blt->src) || BLT_TARGET_RC(blt->dst)) {
>>> + switch (blt->color_depth)
>>> + {
>>> + case CD_32bit:
>>> + return 8;
>>> + default:
>>> + igt_assert_f(0, "COMPRESSION_TYPE_3D unknown color
>>> depth\n");
>>> + }
>>> + }
>>> +
>>> + if (BLT_TARGET_MC(blt->src) || BLT_TARGET_MC(blt->dst)) {
>>> + switch (fb->drm_format)
>>> + {
>>> + case DRM_FORMAT_XRGB8888:
>>> + return 8;
>>> + case DRM_FORMAT_XYUV8888:
>>> + return 9;
>>> + case DRM_FORMAT_NV12:
>>> + return 9;
>>> + case DRM_FORMAT_P010:
>>> + case DRM_FORMAT_P012:
>>> + case DRM_FORMAT_P016:
>>> + return 8;
>>> + default:
>>> + igt_assert_f(0, "COMPRESSION_TYPE_MEDIA unknown format\n");
>>> + }
>>> + } else {
>>> + igt_assert_f(0, "unknown compression\n");
>>> + }
>>> +}
>>> +
>>> static void blitcopy(const struct igt_fb *dst_fb,
>>> const struct igt_fb *src_fb)
>>> {
>>> uint32_t src_tiling, dst_tiling;
>>> uint32_t ctx = 0;
>>> uint64_t ahnd = 0;
>>> + const intel_ctx_t *ictx =
>>> intel_ctx_create_all_physical(src_fb->fd);
>>> + struct intel_execution_engine2 *e;
>>> + uint32_t bb;
>>> + uint64_t bb_size = 4096;
>>> + struct blt_copy_data blt = {};
>>> + struct blt_copy_object *src, *dst;
>>> + struct blt_block_copy_data_ext ext = {}, *pext = NULL;
>>> + uint32_t mem_region = HAS_FLATCCS(intel_get_drm_devid(src_fb->fd))
>>> + ? REGION_LMEM(0) : REGION_SMEM;
>>> igt_assert_eq(dst_fb->fd, src_fb->fd);
>>> igt_assert_eq(dst_fb->num_planes, src_fb->num_planes);
>>> @@ -2697,36 +2821,81 @@ static void blitcopy(const struct igt_fb
>>> *dst_fb,
>>> igt_require(gem_has_contexts(dst_fb->fd));
>>> ctx = gem_context_create(dst_fb->fd);
>>> ahnd = get_reloc_ahnd(dst_fb->fd, ctx);
>>> +
>>> + igt_assert(__gem_create_in_memory_regions(src_fb->fd,
>>> + &bb,
>>> + &bb_size,
>>> + mem_region) == 0);
>>> +
>>> + for_each_ctx_engine(src_fb->fd, ictx, e) {
>>> + if (gem_engine_can_block_copy(src_fb->fd, e))
>>> + break;
>>> + }
>>> }
>>> for (int i = 0; i < dst_fb->num_planes; i++) {
>>> igt_assert_eq(dst_fb->plane_bpp[i], src_fb->plane_bpp[i]);
>>> igt_assert_eq(dst_fb->plane_width[i], src_fb->plane_width[i]);
>>> igt_assert_eq(dst_fb->plane_height[i],
>>> src_fb->plane_height[i]);
>>> - /*
>>> - * On GEN12+ X-tiled format support is removed from the fast
>>> - * blit command, so use the XY_SRC blit command for it
>>> - * instead.
>>> - */
>>> +
>>> if (fast_blit_ok(src_fb) && fast_blit_ok(dst_fb)) {
>>
>> I think this blit fail when ahnd == 0 as i915_blt requires softpinning.
I think this is what I saw as failures in ci on tgl and some gen9
testboxes. Before sending I tested only with dg2 and adlp so I didn't
see this problem earlier.
>
> Also, are we fine with the fact that we'll only cover gen 8 and above
> (due to the softpinning requirement)?
>
>>
>> 'if (ahnd && fast_blit_ok(src_fb) && fast_blit_ok(dst_fb))' should work.
>
> I had a quick look at the function, and I wonder if we could i915_blt
> helpers here instead. There seem to be other factors that we have to
> consider here, but the display version checks seem similar to what we
> have in intel_cmd_info lib.
>
fast_blit_ok(..) checks also fb for x-tiling so I think I'll stay with
that for now.
>
>>
>>> - igt_blitter_fast_copy__raw(dst_fb->fd,
>>> - ahnd, ctx, NULL,
>>> - src_fb->gem_handle,
>>> - src_fb->offsets[i],
>>> - src_fb->strides[i],
>>> - src_tiling,
>>> - 0, 0, /* src_x, src_y */
>>> - src_fb->size,
>>> - dst_fb->plane_width[i],
>>> - dst_fb->plane_height[i],
>>> - dst_fb->plane_bpp[i],
>>> - dst_fb->gem_handle,
>>> - dst_fb->offsets[i],
>>> - dst_fb->strides[i],
>>> - dst_tiling,
>>> - 0, 0 /* dst_x, dst_y */,
>>> - dst_fb->size);
>>> + memset(&blt, 0, sizeof(blt));
>>> + blt.color_depth = blt_get_bpp(src_fb);
>>> +
>>> + src = blt_fb_init(src_fb, i, mem_region);
>>> + dst = blt_fb_init(dst_fb, i, mem_region);
>>> +
>>> + blt_set_copy_object(&blt.src, src);
>>> + blt_set_copy_object(&blt.dst, dst);
>>> +
>>> + blt_set_batch(&blt.bb, bb, bb_size, mem_region);
>>> +
>>> + blt_fast_copy(src_fb->fd, ictx, e, ahnd, &blt);
>>> + gem_sync(src_fb->fd, blt.dst.handle);
>>> +
>>> + blt_destroy_object(src_fb->fd, src);
>>> + blt_destroy_object(dst_fb->fd, dst);
>>> + } else if (ahnd) {
>>
>> I would also check if this else supports block-copy.
>>
>> If you prefer newer instructions (like fast-copy) with i915_blt it
>> requires valid ahnd (>0). Instead of using wrapper:
>>
>> ahnd = get_reloc_ahnd(fd, ctx);
>>
>> you may enforce softpinning by:
>>
>> ahnd = intel_allocator_open(fd, ctx, INTEL_ALLOCATOR_RELOC);
>>
>> Then i915_blt functions will work as you expect. But this will change
>> previous logic to prefer older blits instead new ones.
>>
I now put above "ahnd && blt_has_block_copy(src_fb->fd)". I'll need to
check test runtimes what will come from different boxes to see if
there's something more needed to do with these, as long as runtimes wont
start to grow I'll be ok either way of getting ahnd.
/Juha-Pekka
>>
>>> + /*
>>> + * On GEN12+ X-tiled format support is removed from
>>> + * the fast blit command, so use the block copy blit
>>> + * command for it instead.
>>> + */
>>> + src = blt_fb_init(src_fb, i, mem_region);
>>> + dst = blt_fb_init(dst_fb, i, mem_region);
>>> +
>>> + memset(&blt, 0, sizeof(blt));
>>> + blt.print_bb = true;
>>> + blt.color_depth = blt_get_bpp(src_fb);
>>> + blt_set_copy_object(&blt.src, src);
>>> + blt_set_copy_object(&blt.dst, dst);
>>> +
>>> + if (HAS_FLATCCS(intel_get_drm_devid(src_fb->fd))) {
>>> + blt_set_object_ext(&ext.src,
>>> + blt_compression_format(&blt, src_fb),
>>> + src_fb->width, src_fb->height,
>>> + SURFACE_TYPE_2D);
>>> +
>>> + blt_set_object_ext(&ext.dst,
>>> + blt_compression_format(&blt, dst_fb),
>>> + dst_fb->width, dst_fb->height,
>>> + SURFACE_TYPE_2D);
>>> +
>>> + pext = &ext;
>>> + }
>>> +
>>> + blt_set_batch(&blt.bb, bb, bb_size, mem_region);
>>> +
>>> + blt_block_copy(src_fb->fd, ictx, e, ahnd, &blt, pext);
>>> + gem_sync(src_fb->fd, blt.dst.handle);
>>> +
>>> + blt_destroy_object(src_fb->fd, src);
>>> + blt_destroy_object(dst_fb->fd, dst);
>>> } else {
>>> + /*
>>> + * If on legacy hardware where relocations are supported
>>> + * we'll use XY_SRC blit command instead
>>> + */
>>> igt_blitter_src_copy(dst_fb->fd,
>>> ahnd, ctx, NULL,
>>> src_fb->gem_handle,
>>> @@ -2750,6 +2919,7 @@ static void blitcopy(const struct igt_fb *dst_fb,
>>> if (ctx)
>>> gem_context_destroy(dst_fb->fd, ctx);
>>> put_ahnd(ahnd);
>>> + intel_ctx_destroy(src_fb->fd, ictx);
>>> }
>>> static void free_linear_mapping(struct fb_blit_upload *blit)
>>> --
>>> 2.39.0
>>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/2] lib/igt_fb: switch blitcopy to use lib/i915/i915_blt functions
2023-03-28 17:29 ` Juha-Pekka Heikkila
@ 2023-03-29 6:53 ` Karolina Stolarek
2023-03-29 17:30 ` Zbigniew Kempczyński
1 sibling, 0 replies; 14+ messages in thread
From: Karolina Stolarek @ 2023-03-29 6:53 UTC (permalink / raw)
To: juhapekka.heikkila, Zbigniew Kempczyński; +Cc: igt-dev
On 28.03.2023 19:29, Juha-Pekka Heikkila wrote:
> Hei,
>
> thanks for the comments. This was my first attempt on this and I got
> those failures which Zbigniew's comment explained how those happen. I'll
> soon send new version.. I was also wondering about insertion of that
> offset in the other patch, it somehow feels bit out of place but didn't
> find better place for it.
>
> couple of comments below.
>
> On 28.3.2023 17.10, Karolina Stolarek wrote:
>> On 28.03.2023 08:50, Zbigniew Kempczyński wrote:
>>> On Mon, Mar 27, 2023 at 11:12:54PM +0300, Juha-Pekka Heikkila wrote:
>>>> reduce code duplication
>>>>
>>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>>>> ---
>>>> lib/igt_fb.c | 214
>>>> +++++++++++++++++++++++++++++++++++++++++++++------
>>>> 1 file changed, 192 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
>>>> index ba89e1f60..06ebe4818 100644
>>>> --- a/lib/igt_fb.c
>>>> +++ b/lib/igt_fb.c
>>>> @@ -35,6 +35,8 @@
>>>> #include "drmtest.h"
>>>> #include "i915/gem_create.h"
>>>> #include "i915/gem_mman.h"
>>>> +#include "i915/i915_blt.h"
>>>> +#include "i915/intel_mocs.h"
>>>> #include "igt_aux.h"
>>>> #include "igt_color_encoding.h"
>>>> #include "igt_fb.h"
>>>> @@ -2680,12 +2682,134 @@ static void copy_with_engine(struct
>>>> fb_blit_upload *blit,
>>>> fini_buf(src);
>>>> }
>>>> +static struct blt_copy_object *blt_fb_init(const struct igt_fb *fb,
>>>> + uint32_t plane, uint32_t memregion)
>>>> +{
>>>> + uint32_t name, handle;
>>>> + struct blt_copy_object *blt;
>>>> + enum blt_tiling_type blt_tile;
>>>> + uint64_t stride;
>>>> +
>>>> + blt = malloc(sizeof(*blt));
>>>> + igt_assert(blt);
>>>> +
>>>> + name = gem_flink(fb->fd, fb->gem_handle);
>>>> + handle = gem_open(fb->fd, name);
>>>> +
>>>> + switch (igt_fb_mod_to_tiling(fb->modifier)) {
>>>> + case I915_TILING_X:
>>>> + blt_tile = T_XMAJOR;
>>>
>>> I wonder to add helper in i915_blt: i915_tiling_to_blt_tile(int tile);
>>
>> I wish we could move on from using I915_TILING_* for tiling formats
>> that don't require fences. It would be great if we could switch on
>> blt_tiling_type when doing blit copy, especially when we have checks
>> that ensure the correct mapping.
>>
>> I mean, we could add such a helper, but do we want to keep it as a
>> permanent solution?
>>
>>>
>>> BTW Karolina added asserts which should map 1:1 i915 -> blt. Tile4 is
>>> not added there yet but it can be, so i915_tiline_to_blt_tile() can
>>> simply return tile;
>>>
>>> then those few cases could looks like:
>>>
>>> case I915_TILING_X:
>>> case I915_TILING_Y:
>>> case I915_TILING_4:
>>> blt_tile = i915_tiling_to_blt_tile(...);
>>> stride = fb->strides[plane] / 4;
>>> break;
>>> case I915_TILING_NONE:
>>> ...
>
> i915_tiling_to_blt_tile is coming in some yet unmerged set? I didn't
> find it in my igt.
No, I believe it was a suggestion to add such a function in. I'm not
working on adding such function at least. In my patches I just use
blt_tiling_type instead of I915_TILING_* definitions (but not sure how
such approach would fit in igt_fb).
>
>>>
>>>> + stride = fb->strides[plane] / 4;
>>>> + break;
>>>> + case I915_TILING_Y:
>>>> + blt_tile = T_YMAJOR;
>>>> + stride = fb->strides[plane] / 4;
>>>> + break;
>>>> + case I915_TILING_4:
>>>> + blt_tile = T_TILE4;
>>>> + stride = fb->strides[plane] / 4;
>>>> + break;
>>>> + case I915_TILING_NONE:
>>>> + default:
>>>> + blt_tile = T_LINEAR;
>>>> + stride = fb->strides[plane];
>>>> + break;
>>>> + }
>>>> +
>>>> + blt_set_object(blt, handle, fb->size, memregion,
>>>> + intel_get_uc_mocs(fb->fd),
>>>> + blt_tile,
>>>> + is_ccs_modifier(fb->modifier) ? COMPRESSION_ENABLED
>>>> : COMPRESSION_DISABLED,
>>>> + is_gen12_mc_ccs_modifier(fb->modifier) ?
>>>> COMPRESSION_TYPE_MEDIA : COMPRESSION_TYPE_3D);
>>>> +
>>>> + blt_set_geom(blt, stride, 0, 0, fb->width,
>>>> fb->plane_height[plane], 0, 0);
>>>> +
>>>> + blt->offset = fb->offsets[plane];
>>>> +
>>>> + blt->ptr = gem_mmap__device_coherent(fb->fd, handle, 0, fb->size,
>>>> + PROT_READ | PROT_WRITE);
>>>> + return blt;
>>>> +}
>>>> +
>>>> +static enum blt_color_depth blt_get_bpp(const struct igt_fb *fb)
>>>> +{
>>>> + switch (fb->plane_bpp[0]) {
>>>> + case 8:
>>>> + return CD_8bit;
>>>> + case 16:
>>>> + return CD_16bit;
>>>> + case 32:
>>>> + return CD_32bit;
>>>> + case 64:
>>>> + return CD_64bit;
>>>> + case 96:
>>>> + return CD_96bit;
>>>> + case 128:
>>>> + return CD_128bit;
>>>> + default:
>>>> + igt_assert(0);
>>>> + }
>>>> +}
>>>> +
>>>> +#define BLT_TARGET_RC(x) (x.compression == COMPRESSION_ENABLED && \
>>>> + x.compression_type == COMPRESSION_TYPE_3D)
>>>> +
>>>> +#define BLT_TARGET_MC(x) (x.compression == COMPRESSION_ENABLED && \
>>>> + x.compression_type == COMPRESSION_TYPE_MEDIA)
>>>> +
>>>> +static uint32_t blt_compression_format(struct blt_copy_data *blt,
>>>> + const struct igt_fb *fb)
>>>> +{
>>>> + if (blt->src.compression == COMPRESSION_DISABLED &&
>>>> + blt->dst.compression == COMPRESSION_DISABLED)
>>>> + return 0;
>>>> +
>>>> + if (BLT_TARGET_RC(blt->src) || BLT_TARGET_RC(blt->dst)) {
>>>> + switch (blt->color_depth)
>>>> + {
>>>> + case CD_32bit:
>>>> + return 8;
>>>> + default:
>>>> + igt_assert_f(0, "COMPRESSION_TYPE_3D unknown color
>>>> depth\n");
>>>> + }
>>>> + }
>>>> +
>>>> + if (BLT_TARGET_MC(blt->src) || BLT_TARGET_MC(blt->dst)) {
>>>> + switch (fb->drm_format)
>>>> + {
>>>> + case DRM_FORMAT_XRGB8888:
>>>> + return 8;
>>>> + case DRM_FORMAT_XYUV8888:
>>>> + return 9;
>>>> + case DRM_FORMAT_NV12:
>>>> + return 9;
>>>> + case DRM_FORMAT_P010:
>>>> + case DRM_FORMAT_P012:
>>>> + case DRM_FORMAT_P016:
>>>> + return 8;
>>>> + default:
>>>> + igt_assert_f(0, "COMPRESSION_TYPE_MEDIA unknown
>>>> format\n");
>>>> + }
>>>> + } else {
>>>> + igt_assert_f(0, "unknown compression\n");
>>>> + }
>>>> +}
>>>> +
>>>> static void blitcopy(const struct igt_fb *dst_fb,
>>>> const struct igt_fb *src_fb)
>>>> {
>>>> uint32_t src_tiling, dst_tiling;
>>>> uint32_t ctx = 0;
>>>> uint64_t ahnd = 0;
>>>> + const intel_ctx_t *ictx =
>>>> intel_ctx_create_all_physical(src_fb->fd);
>>>> + struct intel_execution_engine2 *e;
>>>> + uint32_t bb;
>>>> + uint64_t bb_size = 4096;
>>>> + struct blt_copy_data blt = {};
>>>> + struct blt_copy_object *src, *dst;
>>>> + struct blt_block_copy_data_ext ext = {}, *pext = NULL;
>>>> + uint32_t mem_region = HAS_FLATCCS(intel_get_drm_devid(src_fb->fd))
>>>> + ? REGION_LMEM(0) : REGION_SMEM;
>>>> igt_assert_eq(dst_fb->fd, src_fb->fd);
>>>> igt_assert_eq(dst_fb->num_planes, src_fb->num_planes);
>>>> @@ -2697,36 +2821,81 @@ static void blitcopy(const struct igt_fb
>>>> *dst_fb,
>>>> igt_require(gem_has_contexts(dst_fb->fd));
>>>> ctx = gem_context_create(dst_fb->fd);
>>>> ahnd = get_reloc_ahnd(dst_fb->fd, ctx);
>>>> +
>>>> + igt_assert(__gem_create_in_memory_regions(src_fb->fd,
>>>> + &bb,
>>>> + &bb_size,
>>>> + mem_region) == 0);
>>>> +
>>>> + for_each_ctx_engine(src_fb->fd, ictx, e) {
>>>> + if (gem_engine_can_block_copy(src_fb->fd, e))
>>>> + break;
>>>> + }
>>>> }
>>>> for (int i = 0; i < dst_fb->num_planes; i++) {
>>>> igt_assert_eq(dst_fb->plane_bpp[i], src_fb->plane_bpp[i]);
>>>> igt_assert_eq(dst_fb->plane_width[i],
>>>> src_fb->plane_width[i]);
>>>> igt_assert_eq(dst_fb->plane_height[i],
>>>> src_fb->plane_height[i]);
>>>> - /*
>>>> - * On GEN12+ X-tiled format support is removed from the fast
>>>> - * blit command, so use the XY_SRC blit command for it
>>>> - * instead.
>>>> - */
>>>> +
>>>> if (fast_blit_ok(src_fb) && fast_blit_ok(dst_fb)) {
>>>
>>> I think this blit fail when ahnd == 0 as i915_blt requires softpinning.
>
> I think this is what I saw as failures in ci on tgl and some gen9
> testboxes. Before sending I tested only with dg2 and adlp so I didn't
> see this problem earlier.
>
>>
>> Also, are we fine with the fact that we'll only cover gen 8 and above
>> (due to the softpinning requirement)?
>>
>>>
>>> 'if (ahnd && fast_blit_ok(src_fb) && fast_blit_ok(dst_fb))' should work.
>>
>> I had a quick look at the function, and I wonder if we could i915_blt
>> helpers here instead. There seem to be other factors that we have to
>> consider here, but the display version checks seem similar to what we
>> have in intel_cmd_info lib.
>>
>
> fast_blit_ok(..) checks also fb for x-tiling so I think I'll stay with
> that for now.
Right, that's fine
Many thanks,
Karolina
>
>>
>>>
>>>> - igt_blitter_fast_copy__raw(dst_fb->fd,
>>>> - ahnd, ctx, NULL,
>>>> - src_fb->gem_handle,
>>>> - src_fb->offsets[i],
>>>> - src_fb->strides[i],
>>>> - src_tiling,
>>>> - 0, 0, /* src_x, src_y */
>>>> - src_fb->size,
>>>> - dst_fb->plane_width[i],
>>>> - dst_fb->plane_height[i],
>>>> - dst_fb->plane_bpp[i],
>>>> - dst_fb->gem_handle,
>>>> - dst_fb->offsets[i],
>>>> - dst_fb->strides[i],
>>>> - dst_tiling,
>>>> - 0, 0 /* dst_x, dst_y */,
>>>> - dst_fb->size);
>>>> + memset(&blt, 0, sizeof(blt));
>>>> + blt.color_depth = blt_get_bpp(src_fb);
>>>> +
>>>> + src = blt_fb_init(src_fb, i, mem_region);
>>>> + dst = blt_fb_init(dst_fb, i, mem_region);
>>>> +
>>>> + blt_set_copy_object(&blt.src, src);
>>>> + blt_set_copy_object(&blt.dst, dst);
>>>> +
>>>> + blt_set_batch(&blt.bb, bb, bb_size, mem_region);
>>>> +
>>>> + blt_fast_copy(src_fb->fd, ictx, e, ahnd, &blt);
>>>> + gem_sync(src_fb->fd, blt.dst.handle);
>>>> +
>>>> + blt_destroy_object(src_fb->fd, src);
>>>> + blt_destroy_object(dst_fb->fd, dst);
>>>> + } else if (ahnd) {
>>>
>>> I would also check if this else supports block-copy.
>>>
>>> If you prefer newer instructions (like fast-copy) with i915_blt it
>>> requires valid ahnd (>0). Instead of using wrapper:
>>>
>>> ahnd = get_reloc_ahnd(fd, ctx);
>>>
>>> you may enforce softpinning by:
>>>
>>> ahnd = intel_allocator_open(fd, ctx, INTEL_ALLOCATOR_RELOC);
>>>
>>> Then i915_blt functions will work as you expect. But this will change
>>> previous logic to prefer older blits instead new ones.
>>>
>
> I now put above "ahnd && blt_has_block_copy(src_fb->fd)". I'll need to
> check test runtimes what will come from different boxes to see if
> there's something more needed to do with these, as long as runtimes wont
> start to grow I'll be ok either way of getting ahnd.
>
> /Juha-Pekka
>
>>>
>>>> + /*
>>>> + * On GEN12+ X-tiled format support is removed from
>>>> + * the fast blit command, so use the block copy blit
>>>> + * command for it instead.
>>>> + */
>>>> + src = blt_fb_init(src_fb, i, mem_region);
>>>> + dst = blt_fb_init(dst_fb, i, mem_region);
>>>> +
>>>> + memset(&blt, 0, sizeof(blt));
>>>> + blt.print_bb = true;
>>>> + blt.color_depth = blt_get_bpp(src_fb);
>>>> + blt_set_copy_object(&blt.src, src);
>>>> + blt_set_copy_object(&blt.dst, dst);
>>>> +
>>>> + if (HAS_FLATCCS(intel_get_drm_devid(src_fb->fd))) {
>>>> + blt_set_object_ext(&ext.src,
>>>> + blt_compression_format(&blt, src_fb),
>>>> + src_fb->width, src_fb->height,
>>>> + SURFACE_TYPE_2D);
>>>> +
>>>> + blt_set_object_ext(&ext.dst,
>>>> + blt_compression_format(&blt, dst_fb),
>>>> + dst_fb->width, dst_fb->height,
>>>> + SURFACE_TYPE_2D);
>>>> +
>>>> + pext = &ext;
>>>> + }
>>>> +
>>>> + blt_set_batch(&blt.bb, bb, bb_size, mem_region);
>>>> +
>>>> + blt_block_copy(src_fb->fd, ictx, e, ahnd, &blt, pext);
>>>> + gem_sync(src_fb->fd, blt.dst.handle);
>>>> +
>>>> + blt_destroy_object(src_fb->fd, src);
>>>> + blt_destroy_object(dst_fb->fd, dst);
>>>> } else {
>>>> + /*
>>>> + * If on legacy hardware where relocations are supported
>>>> + * we'll use XY_SRC blit command instead
>>>> + */
>>>> igt_blitter_src_copy(dst_fb->fd,
>>>> ahnd, ctx, NULL,
>>>> src_fb->gem_handle,
>>>> @@ -2750,6 +2919,7 @@ static void blitcopy(const struct igt_fb *dst_fb,
>>>> if (ctx)
>>>> gem_context_destroy(dst_fb->fd, ctx);
>>>> put_ahnd(ahnd);
>>>> + intel_ctx_destroy(src_fb->fd, ictx);
>>>> }
>>>> static void free_linear_mapping(struct fb_blit_upload *blit)
>>>> --
>>>> 2.39.0
>>>>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/2] lib/igt_fb: switch blitcopy to use lib/i915/i915_blt functions
2023-03-28 14:10 ` Karolina Stolarek
2023-03-28 17:29 ` Juha-Pekka Heikkila
@ 2023-03-29 17:19 ` Zbigniew Kempczyński
2023-03-30 7:42 ` Karolina Stolarek
1 sibling, 1 reply; 14+ messages in thread
From: Zbigniew Kempczyński @ 2023-03-29 17:19 UTC (permalink / raw)
To: Karolina Stolarek; +Cc: igt-dev
On Tue, Mar 28, 2023 at 04:10:12PM +0200, Karolina Stolarek wrote:
> On 28.03.2023 08:50, Zbigniew Kempczyński wrote:
> > On Mon, Mar 27, 2023 at 11:12:54PM +0300, Juha-Pekka Heikkila wrote:
> > > reduce code duplication
> > >
> > > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > > ---
> > > lib/igt_fb.c | 214 +++++++++++++++++++++++++++++++++++++++++++++------
> > > 1 file changed, 192 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > > index ba89e1f60..06ebe4818 100644
> > > --- a/lib/igt_fb.c
> > > +++ b/lib/igt_fb.c
> > > @@ -35,6 +35,8 @@
> > > #include "drmtest.h"
> > > #include "i915/gem_create.h"
> > > #include "i915/gem_mman.h"
> > > +#include "i915/i915_blt.h"
> > > +#include "i915/intel_mocs.h"
> > > #include "igt_aux.h"
> > > #include "igt_color_encoding.h"
> > > #include "igt_fb.h"
> > > @@ -2680,12 +2682,134 @@ static void copy_with_engine(struct fb_blit_upload *blit,
> > > fini_buf(src);
> > > }
> > > +static struct blt_copy_object *blt_fb_init(const struct igt_fb *fb,
> > > + uint32_t plane, uint32_t memregion)
> > > +{
> > > + uint32_t name, handle;
> > > + struct blt_copy_object *blt;
> > > + enum blt_tiling_type blt_tile;
> > > + uint64_t stride;
> > > +
> > > + blt = malloc(sizeof(*blt));
> > > + igt_assert(blt);
> > > +
> > > + name = gem_flink(fb->fd, fb->gem_handle);
> > > + handle = gem_open(fb->fd, name);
> > > +
> > > + switch (igt_fb_mod_to_tiling(fb->modifier)) {
> > > + case I915_TILING_X:
> > > + blt_tile = T_XMAJOR;
> >
> > I wonder to add helper in i915_blt: i915_tiling_to_blt_tile(int tile);
>
> I wish we could move on from using I915_TILING_* for tiling formats that
> don't require fences. It would be great if we could switch on
> blt_tiling_type when doing blit copy, especially when we have checks that
> ensure the correct mapping.
>
> I mean, we could add such a helper, but do we want to keep it as a permanent
> solution?
>
I assume we will keep I915_TILING_<fmt> == T_<fmt>. I don't know what
the future will bring, but I assume for each new format we will have
both I915_TILING_<newfmt> and T_<newfmt>.
As a legacy code still uses I915_TILING_<fmt> I would add converter to
enum to make developers happy to not to convert this manually (even if
this is same value).
> >
> > BTW Karolina added asserts which should map 1:1 i915 -> blt. Tile4 is
> > not added there yet but it can be, so i915_tiline_to_blt_tile() can
> > simply return tile;
> >
> > then those few cases could looks like:
> >
> > case I915_TILING_X:
> > case I915_TILING_Y:
> > case I915_TILING_4:
> > blt_tile = i915_tiling_to_blt_tile(...);
> > stride = fb->strides[plane] / 4;
> > break;
> > case I915_TILING_NONE:
> > ...
> >
> > > + stride = fb->strides[plane] / 4;
> > > + break;
> > > + case I915_TILING_Y:
> > > + blt_tile = T_YMAJOR;
> > > + stride = fb->strides[plane] / 4;
> > > + break;
> > > + case I915_TILING_4:
> > > + blt_tile = T_TILE4;
> > > + stride = fb->strides[plane] / 4;
> > > + break;
> > > + case I915_TILING_NONE:
> > > + default:
> > > + blt_tile = T_LINEAR;
> > > + stride = fb->strides[plane];
> > > + break;
> > > + }
> > > +
> > > + blt_set_object(blt, handle, fb->size, memregion,
> > > + intel_get_uc_mocs(fb->fd),
> > > + blt_tile,
> > > + is_ccs_modifier(fb->modifier) ? COMPRESSION_ENABLED : COMPRESSION_DISABLED,
> > > + is_gen12_mc_ccs_modifier(fb->modifier) ? COMPRESSION_TYPE_MEDIA : COMPRESSION_TYPE_3D);
> > > +
> > > + blt_set_geom(blt, stride, 0, 0, fb->width, fb->plane_height[plane], 0, 0);
> > > +
> > > + blt->offset = fb->offsets[plane];
> > > +
> > > + blt->ptr = gem_mmap__device_coherent(fb->fd, handle, 0, fb->size,
> > > + PROT_READ | PROT_WRITE);
> > > + return blt;
> > > +}
> > > +
> > > +static enum blt_color_depth blt_get_bpp(const struct igt_fb *fb)
> > > +{
> > > + switch (fb->plane_bpp[0]) {
> > > + case 8:
> > > + return CD_8bit;
> > > + case 16:
> > > + return CD_16bit;
> > > + case 32:
> > > + return CD_32bit;
> > > + case 64:
> > > + return CD_64bit;
> > > + case 96:
> > > + return CD_96bit;
> > > + case 128:
> > > + return CD_128bit;
> > > + default:
> > > + igt_assert(0);
> > > + }
> > > +}
> > > +
> > > +#define BLT_TARGET_RC(x) (x.compression == COMPRESSION_ENABLED && \
> > > + x.compression_type == COMPRESSION_TYPE_3D)
> > > +
> > > +#define BLT_TARGET_MC(x) (x.compression == COMPRESSION_ENABLED && \
> > > + x.compression_type == COMPRESSION_TYPE_MEDIA)
> > > +
> > > +static uint32_t blt_compression_format(struct blt_copy_data *blt,
> > > + const struct igt_fb *fb)
> > > +{
> > > + if (blt->src.compression == COMPRESSION_DISABLED &&
> > > + blt->dst.compression == COMPRESSION_DISABLED)
> > > + return 0;
> > > +
> > > + if (BLT_TARGET_RC(blt->src) || BLT_TARGET_RC(blt->dst)) {
> > > + switch (blt->color_depth)
> > > + {
> > > + case CD_32bit:
> > > + return 8;
> > > + default:
> > > + igt_assert_f(0, "COMPRESSION_TYPE_3D unknown color depth\n");
> > > + }
> > > + }
> > > +
> > > + if (BLT_TARGET_MC(blt->src) || BLT_TARGET_MC(blt->dst)) {
> > > + switch (fb->drm_format)
> > > + {
> > > + case DRM_FORMAT_XRGB8888:
> > > + return 8;
> > > + case DRM_FORMAT_XYUV8888:
> > > + return 9;
> > > + case DRM_FORMAT_NV12:
> > > + return 9;
> > > + case DRM_FORMAT_P010:
> > > + case DRM_FORMAT_P012:
> > > + case DRM_FORMAT_P016:
> > > + return 8;
> > > + default:
> > > + igt_assert_f(0, "COMPRESSION_TYPE_MEDIA unknown format\n");
> > > + }
> > > + } else {
> > > + igt_assert_f(0, "unknown compression\n");
> > > + }
> > > +}
> > > +
> > > static void blitcopy(const struct igt_fb *dst_fb,
> > > const struct igt_fb *src_fb)
> > > {
> > > uint32_t src_tiling, dst_tiling;
> > > uint32_t ctx = 0;
> > > uint64_t ahnd = 0;
> > > + const intel_ctx_t *ictx = intel_ctx_create_all_physical(src_fb->fd);
> > > + struct intel_execution_engine2 *e;
> > > + uint32_t bb;
> > > + uint64_t bb_size = 4096;
> > > + struct blt_copy_data blt = {};
> > > + struct blt_copy_object *src, *dst;
> > > + struct blt_block_copy_data_ext ext = {}, *pext = NULL;
> > > + uint32_t mem_region = HAS_FLATCCS(intel_get_drm_devid(src_fb->fd))
> > > + ? REGION_LMEM(0) : REGION_SMEM;
> > > igt_assert_eq(dst_fb->fd, src_fb->fd);
> > > igt_assert_eq(dst_fb->num_planes, src_fb->num_planes);
> > > @@ -2697,36 +2821,81 @@ static void blitcopy(const struct igt_fb *dst_fb,
> > > igt_require(gem_has_contexts(dst_fb->fd));
> > > ctx = gem_context_create(dst_fb->fd);
> > > ahnd = get_reloc_ahnd(dst_fb->fd, ctx);
> > > +
> > > + igt_assert(__gem_create_in_memory_regions(src_fb->fd,
> > > + &bb,
> > > + &bb_size,
> > > + mem_region) == 0);
> > > +
> > > + for_each_ctx_engine(src_fb->fd, ictx, e) {
> > > + if (gem_engine_can_block_copy(src_fb->fd, e))
> > > + break;
> > > + }
> > > }
> > > for (int i = 0; i < dst_fb->num_planes; i++) {
> > > igt_assert_eq(dst_fb->plane_bpp[i], src_fb->plane_bpp[i]);
> > > igt_assert_eq(dst_fb->plane_width[i], src_fb->plane_width[i]);
> > > igt_assert_eq(dst_fb->plane_height[i], src_fb->plane_height[i]);
> > > - /*
> > > - * On GEN12+ X-tiled format support is removed from the fast
> > > - * blit command, so use the XY_SRC blit command for it
> > > - * instead.
> > > - */
> > > +
> > > if (fast_blit_ok(src_fb) && fast_blit_ok(dst_fb)) {
> >
> > I think this blit fail when ahnd == 0 as i915_blt requires softpinning.
>
> Also, are we fine with the fact that we'll only cover gen 8 and above (due
> to the softpinning requirement)?
Hmm, instead of fast_blit_ok() we should use here blt_has_fast_copy() and
blt_fast_copy_supports_tiling(). Gen8 won't be covered here as fast copy
doesn't exists there. BTW it looks a little bit weird .cmds_info on gen9
pointing to gen11_cmds_info. I started to look on _cmds_info first to find
out is fast_copy supported on gen8 and gen9 and I thought it is not.
Consider adding separated entry for gen9, even at the cost of additional
cmds_info variable.
>
> >
> > 'if (ahnd && fast_blit_ok(src_fb) && fast_blit_ok(dst_fb))' should work.
>
> I had a quick look at the function, and I wonder if we could i915_blt
> helpers here instead. There seem to be other factors that we have to
> consider here, but the display version checks seem similar to what we have
> in intel_cmd_info lib.
Yes, i915_blt helpers are more reliable for platforms.
--
Zbigniew
>
>
> All the best,
> Karolina
>
> >
> > > - igt_blitter_fast_copy__raw(dst_fb->fd,
> > > - ahnd, ctx, NULL,
> > > - src_fb->gem_handle,
> > > - src_fb->offsets[i],
> > > - src_fb->strides[i],
> > > - src_tiling,
> > > - 0, 0, /* src_x, src_y */
> > > - src_fb->size,
> > > - dst_fb->plane_width[i],
> > > - dst_fb->plane_height[i],
> > > - dst_fb->plane_bpp[i],
> > > - dst_fb->gem_handle,
> > > - dst_fb->offsets[i],
> > > - dst_fb->strides[i],
> > > - dst_tiling,
> > > - 0, 0 /* dst_x, dst_y */,
> > > - dst_fb->size);
> > > + memset(&blt, 0, sizeof(blt));
> > > + blt.color_depth = blt_get_bpp(src_fb);
> > > +
> > > + src = blt_fb_init(src_fb, i, mem_region);
> > > + dst = blt_fb_init(dst_fb, i, mem_region);
> > > +
> > > + blt_set_copy_object(&blt.src, src);
> > > + blt_set_copy_object(&blt.dst, dst);
> > > +
> > > + blt_set_batch(&blt.bb, bb, bb_size, mem_region);
> > > +
> > > + blt_fast_copy(src_fb->fd, ictx, e, ahnd, &blt);
> > > + gem_sync(src_fb->fd, blt.dst.handle);
> > > +
> > > + blt_destroy_object(src_fb->fd, src);
> > > + blt_destroy_object(dst_fb->fd, dst);
> > > + } else if (ahnd) {
> >
> > I would also check if this else supports block-copy.
> >
> > If you prefer newer instructions (like fast-copy) with i915_blt it
> > requires valid ahnd (>0). Instead of using wrapper:
> >
> > ahnd = get_reloc_ahnd(fd, ctx);
> >
> > you may enforce softpinning by:
> >
> > ahnd = intel_allocator_open(fd, ctx, INTEL_ALLOCATOR_RELOC);
> >
> > Then i915_blt functions will work as you expect. But this will change
> > previous logic to prefer older blits instead new ones.
> >
> > --
> > Zbigniew
> >
> > > + /*
> > > + * On GEN12+ X-tiled format support is removed from
> > > + * the fast blit command, so use the block copy blit
> > > + * command for it instead.
> > > + */
> > > + src = blt_fb_init(src_fb, i, mem_region);
> > > + dst = blt_fb_init(dst_fb, i, mem_region);
> > > +
> > > + memset(&blt, 0, sizeof(blt));
> > > + blt.print_bb = true;
> > > + blt.color_depth = blt_get_bpp(src_fb);
> > > + blt_set_copy_object(&blt.src, src);
> > > + blt_set_copy_object(&blt.dst, dst);
> > > +
> > > + if (HAS_FLATCCS(intel_get_drm_devid(src_fb->fd))) {
> > > + blt_set_object_ext(&ext.src,
> > > + blt_compression_format(&blt, src_fb),
> > > + src_fb->width, src_fb->height,
> > > + SURFACE_TYPE_2D);
> > > +
> > > + blt_set_object_ext(&ext.dst,
> > > + blt_compression_format(&blt, dst_fb),
> > > + dst_fb->width, dst_fb->height,
> > > + SURFACE_TYPE_2D);
> > > +
> > > + pext = &ext;
> > > + }
> > > +
> > > + blt_set_batch(&blt.bb, bb, bb_size, mem_region);
> > > +
> > > + blt_block_copy(src_fb->fd, ictx, e, ahnd, &blt, pext);
> > > + gem_sync(src_fb->fd, blt.dst.handle);
> > > +
> > > + blt_destroy_object(src_fb->fd, src);
> > > + blt_destroy_object(dst_fb->fd, dst);
> > > } else {
> > > + /*
> > > + * If on legacy hardware where relocations are supported
> > > + * we'll use XY_SRC blit command instead
> > > + */
> > > igt_blitter_src_copy(dst_fb->fd,
> > > ahnd, ctx, NULL,
> > > src_fb->gem_handle,
> > > @@ -2750,6 +2919,7 @@ static void blitcopy(const struct igt_fb *dst_fb,
> > > if (ctx)
> > > gem_context_destroy(dst_fb->fd, ctx);
> > > put_ahnd(ahnd);
> > > + intel_ctx_destroy(src_fb->fd, ictx);
> > > }
> > > static void free_linear_mapping(struct fb_blit_upload *blit)
> > > --
> > > 2.39.0
> > >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/2] lib/igt_fb: switch blitcopy to use lib/i915/i915_blt functions
2023-03-28 17:29 ` Juha-Pekka Heikkila
2023-03-29 6:53 ` Karolina Stolarek
@ 2023-03-29 17:30 ` Zbigniew Kempczyński
1 sibling, 0 replies; 14+ messages in thread
From: Zbigniew Kempczyński @ 2023-03-29 17:30 UTC (permalink / raw)
To: Juha-Pekka Heikkila; +Cc: igt-dev
On Tue, Mar 28, 2023 at 08:29:40PM +0300, Juha-Pekka Heikkila wrote:
> Hei,
>
> thanks for the comments. This was my first attempt on this and I got those
> failures which Zbigniew's comment explained how those happen. I'll soon send
> new version.. I was also wondering about insertion of that offset in the
> other patch, it somehow feels bit out of place but didn't find better place
> for it.
I'm sorry, I forgot comment 'offset' field.
There's no problem from adding it, I wondered to add those at the end
of the structure with detailed description. And 'offset' is a little bit
confusing. Maybe 'plane_offset' would be better?
>
> couple of comments below.
>
> On 28.3.2023 17.10, Karolina Stolarek wrote:
> > On 28.03.2023 08:50, Zbigniew Kempczyński wrote:
> > > On Mon, Mar 27, 2023 at 11:12:54PM +0300, Juha-Pekka Heikkila wrote:
> > > > reduce code duplication
> > > >
> > > > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > > > ---
> > > > lib/igt_fb.c | 214 +++++++++++++++++++++++++++++++++++++++++++++------
> > > > 1 file changed, 192 insertions(+), 22 deletions(-)
> > > >
> > > > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > > > index ba89e1f60..06ebe4818 100644
> > > > --- a/lib/igt_fb.c
> > > > +++ b/lib/igt_fb.c
> > > > @@ -35,6 +35,8 @@
> > > > #include "drmtest.h"
> > > > #include "i915/gem_create.h"
> > > > #include "i915/gem_mman.h"
> > > > +#include "i915/i915_blt.h"
> > > > +#include "i915/intel_mocs.h"
> > > > #include "igt_aux.h"
> > > > #include "igt_color_encoding.h"
> > > > #include "igt_fb.h"
> > > > @@ -2680,12 +2682,134 @@ static void copy_with_engine(struct
> > > > fb_blit_upload *blit,
> > > > fini_buf(src);
> > > > }
> > > > +static struct blt_copy_object *blt_fb_init(const struct igt_fb *fb,
> > > > + uint32_t plane, uint32_t memregion)
> > > > +{
> > > > + uint32_t name, handle;
> > > > + struct blt_copy_object *blt;
> > > > + enum blt_tiling_type blt_tile;
> > > > + uint64_t stride;
> > > > +
> > > > + blt = malloc(sizeof(*blt));
> > > > + igt_assert(blt);
> > > > +
> > > > + name = gem_flink(fb->fd, fb->gem_handle);
> > > > + handle = gem_open(fb->fd, name);
> > > > +
> > > > + switch (igt_fb_mod_to_tiling(fb->modifier)) {
> > > > + case I915_TILING_X:
> > > > + blt_tile = T_XMAJOR;
> > >
> > > I wonder to add helper in i915_blt: i915_tiling_to_blt_tile(int tile);
> >
> > I wish we could move on from using I915_TILING_* for tiling formats that
> > don't require fences. It would be great if we could switch on
> > blt_tiling_type when doing blit copy, especially when we have checks
> > that ensure the correct mapping.
> >
> > I mean, we could add such a helper, but do we want to keep it as a
> > permanent solution?
> >
> > >
> > > BTW Karolina added asserts which should map 1:1 i915 -> blt. Tile4 is
> > > not added there yet but it can be, so i915_tiline_to_blt_tile() can
> > > simply return tile;
> > >
> > > then those few cases could looks like:
> > >
> > > case I915_TILING_X:
> > > case I915_TILING_Y:
> > > case I915_TILING_4:
> > > blt_tile = i915_tiling_to_blt_tile(...);
> > > stride = fb->strides[plane] / 4;
> > > break;
> > > case I915_TILING_NONE:
> > > ...
>
> i915_tiling_to_blt_tile is coming in some yet unmerged set? I didn't find it
> in my igt.
No, I suggested you could add this. Assumption is we want 1:1 mapping
between I915_TILING_<fmt> and T_<fmt> so simple return int to enum value
is enough.
>
> > >
> > > > + stride = fb->strides[plane] / 4;
> > > > + break;
> > > > + case I915_TILING_Y:
> > > > + blt_tile = T_YMAJOR;
> > > > + stride = fb->strides[plane] / 4;
> > > > + break;
> > > > + case I915_TILING_4:
> > > > + blt_tile = T_TILE4;
> > > > + stride = fb->strides[plane] / 4;
> > > > + break;
> > > > + case I915_TILING_NONE:
> > > > + default:
> > > > + blt_tile = T_LINEAR;
> > > > + stride = fb->strides[plane];
> > > > + break;
> > > > + }
> > > > +
> > > > + blt_set_object(blt, handle, fb->size, memregion,
> > > > + intel_get_uc_mocs(fb->fd),
> > > > + blt_tile,
> > > > + is_ccs_modifier(fb->modifier) ?
> > > > COMPRESSION_ENABLED : COMPRESSION_DISABLED,
> > > > + is_gen12_mc_ccs_modifier(fb->modifier) ?
> > > > COMPRESSION_TYPE_MEDIA : COMPRESSION_TYPE_3D);
> > > > +
> > > > + blt_set_geom(blt, stride, 0, 0, fb->width,
> > > > fb->plane_height[plane], 0, 0);
> > > > +
> > > > + blt->offset = fb->offsets[plane];
> > > > +
> > > > + blt->ptr = gem_mmap__device_coherent(fb->fd, handle, 0, fb->size,
> > > > + PROT_READ | PROT_WRITE);
> > > > + return blt;
> > > > +}
> > > > +
> > > > +static enum blt_color_depth blt_get_bpp(const struct igt_fb *fb)
> > > > +{
> > > > + switch (fb->plane_bpp[0]) {
> > > > + case 8:
> > > > + return CD_8bit;
> > > > + case 16:
> > > > + return CD_16bit;
> > > > + case 32:
> > > > + return CD_32bit;
> > > > + case 64:
> > > > + return CD_64bit;
> > > > + case 96:
> > > > + return CD_96bit;
> > > > + case 128:
> > > > + return CD_128bit;
> > > > + default:
> > > > + igt_assert(0);
> > > > + }
> > > > +}
> > > > +
> > > > +#define BLT_TARGET_RC(x) (x.compression == COMPRESSION_ENABLED && \
> > > > + x.compression_type == COMPRESSION_TYPE_3D)
> > > > +
> > > > +#define BLT_TARGET_MC(x) (x.compression == COMPRESSION_ENABLED && \
> > > > + x.compression_type == COMPRESSION_TYPE_MEDIA)
> > > > +
> > > > +static uint32_t blt_compression_format(struct blt_copy_data *blt,
> > > > + const struct igt_fb *fb)
> > > > +{
> > > > + if (blt->src.compression == COMPRESSION_DISABLED &&
> > > > + blt->dst.compression == COMPRESSION_DISABLED)
> > > > + return 0;
> > > > +
> > > > + if (BLT_TARGET_RC(blt->src) || BLT_TARGET_RC(blt->dst)) {
> > > > + switch (blt->color_depth)
> > > > + {
> > > > + case CD_32bit:
> > > > + return 8;
> > > > + default:
> > > > + igt_assert_f(0, "COMPRESSION_TYPE_3D unknown color
> > > > depth\n");
> > > > + }
> > > > + }
> > > > +
> > > > + if (BLT_TARGET_MC(blt->src) || BLT_TARGET_MC(blt->dst)) {
> > > > + switch (fb->drm_format)
> > > > + {
> > > > + case DRM_FORMAT_XRGB8888:
> > > > + return 8;
> > > > + case DRM_FORMAT_XYUV8888:
> > > > + return 9;
> > > > + case DRM_FORMAT_NV12:
> > > > + return 9;
> > > > + case DRM_FORMAT_P010:
> > > > + case DRM_FORMAT_P012:
> > > > + case DRM_FORMAT_P016:
> > > > + return 8;
> > > > + default:
> > > > + igt_assert_f(0, "COMPRESSION_TYPE_MEDIA unknown format\n");
> > > > + }
> > > > + } else {
> > > > + igt_assert_f(0, "unknown compression\n");
> > > > + }
> > > > +}
> > > > +
> > > > static void blitcopy(const struct igt_fb *dst_fb,
> > > > const struct igt_fb *src_fb)
> > > > {
> > > > uint32_t src_tiling, dst_tiling;
> > > > uint32_t ctx = 0;
> > > > uint64_t ahnd = 0;
> > > > + const intel_ctx_t *ictx =
> > > > intel_ctx_create_all_physical(src_fb->fd);
> > > > + struct intel_execution_engine2 *e;
> > > > + uint32_t bb;
> > > > + uint64_t bb_size = 4096;
> > > > + struct blt_copy_data blt = {};
> > > > + struct blt_copy_object *src, *dst;
> > > > + struct blt_block_copy_data_ext ext = {}, *pext = NULL;
> > > > + uint32_t mem_region = HAS_FLATCCS(intel_get_drm_devid(src_fb->fd))
> > > > + ? REGION_LMEM(0) : REGION_SMEM;
> > > > igt_assert_eq(dst_fb->fd, src_fb->fd);
> > > > igt_assert_eq(dst_fb->num_planes, src_fb->num_planes);
> > > > @@ -2697,36 +2821,81 @@ static void blitcopy(const struct igt_fb
> > > > *dst_fb,
> > > > igt_require(gem_has_contexts(dst_fb->fd));
> > > > ctx = gem_context_create(dst_fb->fd);
> > > > ahnd = get_reloc_ahnd(dst_fb->fd, ctx);
> > > > +
> > > > + igt_assert(__gem_create_in_memory_regions(src_fb->fd,
> > > > + &bb,
> > > > + &bb_size,
> > > > + mem_region) == 0);
> > > > +
> > > > + for_each_ctx_engine(src_fb->fd, ictx, e) {
> > > > + if (gem_engine_can_block_copy(src_fb->fd, e))
> > > > + break;
> > > > + }
> > > > }
> > > > for (int i = 0; i < dst_fb->num_planes; i++) {
> > > > igt_assert_eq(dst_fb->plane_bpp[i], src_fb->plane_bpp[i]);
> > > > igt_assert_eq(dst_fb->plane_width[i], src_fb->plane_width[i]);
> > > > igt_assert_eq(dst_fb->plane_height[i],
> > > > src_fb->plane_height[i]);
> > > > - /*
> > > > - * On GEN12+ X-tiled format support is removed from the fast
> > > > - * blit command, so use the XY_SRC blit command for it
> > > > - * instead.
> > > > - */
> > > > +
> > > > if (fast_blit_ok(src_fb) && fast_blit_ok(dst_fb)) {
> > >
> > > I think this blit fail when ahnd == 0 as i915_blt requires softpinning.
>
> I think this is what I saw as failures in ci on tgl and some gen9 testboxes.
> Before sending I tested only with dg2 and adlp so I didn't see this problem
> earlier.
Karolina did a good job with intel_cmds_info and i915_blt helpers which
allows test if command/tiling is supported on dedicated platform.
We may start to replacing above and not fully reliable functions
(fast_blit_ok()) with i915_blt checkers.
>
> >
> > Also, are we fine with the fact that we'll only cover gen 8 and above
> > (due to the softpinning requirement)?
> >
> > >
> > > 'if (ahnd && fast_blit_ok(src_fb) && fast_blit_ok(dst_fb))' should work.
> >
> > I had a quick look at the function, and I wonder if we could i915_blt
> > helpers here instead. There seem to be other factors that we have to
> > consider here, but the display version checks seem similar to what we
> > have in intel_cmd_info lib.
> >
>
> fast_blit_ok(..) checks also fb for x-tiling so I think I'll stay with that
> for now.
I suggest replace fast_blit_ok() body and use blt_has_fast_copy() and
blt_block_copy_supports_tiling() but I won't insist.
>
> >
> > >
> > > > - igt_blitter_fast_copy__raw(dst_fb->fd,
> > > > - ahnd, ctx, NULL,
> > > > - src_fb->gem_handle,
> > > > - src_fb->offsets[i],
> > > > - src_fb->strides[i],
> > > > - src_tiling,
> > > > - 0, 0, /* src_x, src_y */
> > > > - src_fb->size,
> > > > - dst_fb->plane_width[i],
> > > > - dst_fb->plane_height[i],
> > > > - dst_fb->plane_bpp[i],
> > > > - dst_fb->gem_handle,
> > > > - dst_fb->offsets[i],
> > > > - dst_fb->strides[i],
> > > > - dst_tiling,
> > > > - 0, 0 /* dst_x, dst_y */,
> > > > - dst_fb->size);
> > > > + memset(&blt, 0, sizeof(blt));
> > > > + blt.color_depth = blt_get_bpp(src_fb);
> > > > +
> > > > + src = blt_fb_init(src_fb, i, mem_region);
> > > > + dst = blt_fb_init(dst_fb, i, mem_region);
> > > > +
> > > > + blt_set_copy_object(&blt.src, src);
> > > > + blt_set_copy_object(&blt.dst, dst);
> > > > +
> > > > + blt_set_batch(&blt.bb, bb, bb_size, mem_region);
> > > > +
> > > > + blt_fast_copy(src_fb->fd, ictx, e, ahnd, &blt);
> > > > + gem_sync(src_fb->fd, blt.dst.handle);
> > > > +
> > > > + blt_destroy_object(src_fb->fd, src);
> > > > + blt_destroy_object(dst_fb->fd, dst);
> > > > + } else if (ahnd) {
> > >
> > > I would also check if this else supports block-copy.
> > >
> > > If you prefer newer instructions (like fast-copy) with i915_blt it
> > > requires valid ahnd (>0). Instead of using wrapper:
> > >
> > > ahnd = get_reloc_ahnd(fd, ctx);
> > >
> > > you may enforce softpinning by:
> > >
> > > ahnd = intel_allocator_open(fd, ctx, INTEL_ALLOCATOR_RELOC);
> > >
> > > Then i915_blt functions will work as you expect. But this will change
> > > previous logic to prefer older blits instead new ones.
> > >
>
> I now put above "ahnd && blt_has_block_copy(src_fb->fd)". I'll need to check
> test runtimes what will come from different boxes to see if there's
> something more needed to do with these, as long as runtimes wont start to
> grow I'll be ok either way of getting ahnd.
Ok, looks good to me (I would also check tilings supported by the command,
but this is not a must).
--
Zbigniew
>
> /Juha-Pekka
>
> > >
> > > > + /*
> > > > + * On GEN12+ X-tiled format support is removed from
> > > > + * the fast blit command, so use the block copy blit
> > > > + * command for it instead.
> > > > + */
> > > > + src = blt_fb_init(src_fb, i, mem_region);
> > > > + dst = blt_fb_init(dst_fb, i, mem_region);
> > > > +
> > > > + memset(&blt, 0, sizeof(blt));
> > > > + blt.print_bb = true;
> > > > + blt.color_depth = blt_get_bpp(src_fb);
> > > > + blt_set_copy_object(&blt.src, src);
> > > > + blt_set_copy_object(&blt.dst, dst);
> > > > +
> > > > + if (HAS_FLATCCS(intel_get_drm_devid(src_fb->fd))) {
> > > > + blt_set_object_ext(&ext.src,
> > > > + blt_compression_format(&blt, src_fb),
> > > > + src_fb->width, src_fb->height,
> > > > + SURFACE_TYPE_2D);
> > > > +
> > > > + blt_set_object_ext(&ext.dst,
> > > > + blt_compression_format(&blt, dst_fb),
> > > > + dst_fb->width, dst_fb->height,
> > > > + SURFACE_TYPE_2D);
> > > > +
> > > > + pext = &ext;
> > > > + }
> > > > +
> > > > + blt_set_batch(&blt.bb, bb, bb_size, mem_region);
> > > > +
> > > > + blt_block_copy(src_fb->fd, ictx, e, ahnd, &blt, pext);
> > > > + gem_sync(src_fb->fd, blt.dst.handle);
> > > > +
> > > > + blt_destroy_object(src_fb->fd, src);
> > > > + blt_destroy_object(dst_fb->fd, dst);
> > > > } else {
> > > > + /*
> > > > + * If on legacy hardware where relocations are supported
> > > > + * we'll use XY_SRC blit command instead
> > > > + */
> > > > igt_blitter_src_copy(dst_fb->fd,
> > > > ahnd, ctx, NULL,
> > > > src_fb->gem_handle,
> > > > @@ -2750,6 +2919,7 @@ static void blitcopy(const struct igt_fb *dst_fb,
> > > > if (ctx)
> > > > gem_context_destroy(dst_fb->fd, ctx);
> > > > put_ahnd(ahnd);
> > > > + intel_ctx_destroy(src_fb->fd, ictx);
> > > > }
> > > > static void free_linear_mapping(struct fb_blit_upload *blit)
> > > > --
> > > > 2.39.0
> > > >
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/2] lib/igt_fb: switch blitcopy to use lib/i915/i915_blt functions
2023-03-29 17:19 ` Zbigniew Kempczyński
@ 2023-03-30 7:42 ` Karolina Stolarek
0 siblings, 0 replies; 14+ messages in thread
From: Karolina Stolarek @ 2023-03-30 7:42 UTC (permalink / raw)
To: Zbigniew Kempczyński; +Cc: igt-dev
On 29.03.2023 19:19, Zbigniew Kempczyński wrote:
> On Tue, Mar 28, 2023 at 04:10:12PM +0200, Karolina Stolarek wrote:
>> On 28.03.2023 08:50, Zbigniew Kempczyński wrote:
>>> On Mon, Mar 27, 2023 at 11:12:54PM +0300, Juha-Pekka Heikkila wrote:
>>>> reduce code duplication
>>>>
>>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>>>> ---
>>>> lib/igt_fb.c | 214 +++++++++++++++++++++++++++++++++++++++++++++------
>>>> 1 file changed, 192 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
>>>> index ba89e1f60..06ebe4818 100644
>>>> --- a/lib/igt_fb.c
>>>> +++ b/lib/igt_fb.c
>>>> @@ -35,6 +35,8 @@
>>>> #include "drmtest.h"
>>>> #include "i915/gem_create.h"
>>>> #include "i915/gem_mman.h"
>>>> +#include "i915/i915_blt.h"
>>>> +#include "i915/intel_mocs.h"
>>>> #include "igt_aux.h"
>>>> #include "igt_color_encoding.h"
>>>> #include "igt_fb.h"
>>>> @@ -2680,12 +2682,134 @@ static void copy_with_engine(struct fb_blit_upload *blit,
>>>> fini_buf(src);
>>>> }
>>>> +static struct blt_copy_object *blt_fb_init(const struct igt_fb *fb,
>>>> + uint32_t plane, uint32_t memregion)
>>>> +{
>>>> + uint32_t name, handle;
>>>> + struct blt_copy_object *blt;
>>>> + enum blt_tiling_type blt_tile;
>>>> + uint64_t stride;
>>>> +
>>>> + blt = malloc(sizeof(*blt));
>>>> + igt_assert(blt);
>>>> +
>>>> + name = gem_flink(fb->fd, fb->gem_handle);
>>>> + handle = gem_open(fb->fd, name);
>>>> +
>>>> + switch (igt_fb_mod_to_tiling(fb->modifier)) {
>>>> + case I915_TILING_X:
>>>> + blt_tile = T_XMAJOR;
>>>
>>> I wonder to add helper in i915_blt: i915_tiling_to_blt_tile(int tile);
>>
>> I wish we could move on from using I915_TILING_* for tiling formats that
>> don't require fences. It would be great if we could switch on
>> blt_tiling_type when doing blit copy, especially when we have checks that
>> ensure the correct mapping.
>>
>> I mean, we could add such a helper, but do we want to keep it as a permanent
>> solution?
>>
>
> I assume we will keep I915_TILING_<fmt> == T_<fmt>. I don't know what
> the future will bring, but I assume for each new format we will have
> both I915_TILING_<newfmt> and T_<newfmt>.
>
> As a legacy code still uses I915_TILING_<fmt> I would add converter to
> enum to make developers happy to not to convert this manually (even if
> this is same value).
>
Right, I see what you mean. I just pass "newer" definitions in :)
But I know that others might not know that they're equivalent, and we
should provide a function that will convert the values.
>>>
>>> BTW Karolina added asserts which should map 1:1 i915 -> blt. Tile4 is
>>> not added there yet but it can be, so i915_tiline_to_blt_tile() can
>>> simply return tile;
>>>
>>> then those few cases could looks like:
>>>
>>> case I915_TILING_X:
>>> case I915_TILING_Y:
>>> case I915_TILING_4:
>>> blt_tile = i915_tiling_to_blt_tile(...);
>>> stride = fb->strides[plane] / 4;
>>> break;
>>> case I915_TILING_NONE:
>>> ...
>>>
>>>> + stride = fb->strides[plane] / 4;
>>>> + break;
>>>> + case I915_TILING_Y:
>>>> + blt_tile = T_YMAJOR;
>>>> + stride = fb->strides[plane] / 4;
>>>> + break;
>>>> + case I915_TILING_4:
>>>> + blt_tile = T_TILE4;
>>>> + stride = fb->strides[plane] / 4;
>>>> + break;
>>>> + case I915_TILING_NONE:
>>>> + default:
>>>> + blt_tile = T_LINEAR;
>>>> + stride = fb->strides[plane];
>>>> + break;
>>>> + }
>>>> +
>>>> + blt_set_object(blt, handle, fb->size, memregion,
>>>> + intel_get_uc_mocs(fb->fd),
>>>> + blt_tile,
>>>> + is_ccs_modifier(fb->modifier) ? COMPRESSION_ENABLED : COMPRESSION_DISABLED,
>>>> + is_gen12_mc_ccs_modifier(fb->modifier) ? COMPRESSION_TYPE_MEDIA : COMPRESSION_TYPE_3D);
>>>> +
>>>> + blt_set_geom(blt, stride, 0, 0, fb->width, fb->plane_height[plane], 0, 0);
>>>> +
>>>> + blt->offset = fb->offsets[plane];
>>>> +
>>>> + blt->ptr = gem_mmap__device_coherent(fb->fd, handle, 0, fb->size,
>>>> + PROT_READ | PROT_WRITE);
>>>> + return blt;
>>>> +}
>>>> +
>>>> +static enum blt_color_depth blt_get_bpp(const struct igt_fb *fb)
>>>> +{
>>>> + switch (fb->plane_bpp[0]) {
>>>> + case 8:
>>>> + return CD_8bit;
>>>> + case 16:
>>>> + return CD_16bit;
>>>> + case 32:
>>>> + return CD_32bit;
>>>> + case 64:
>>>> + return CD_64bit;
>>>> + case 96:
>>>> + return CD_96bit;
>>>> + case 128:
>>>> + return CD_128bit;
>>>> + default:
>>>> + igt_assert(0);
>>>> + }
>>>> +}
>>>> +
>>>> +#define BLT_TARGET_RC(x) (x.compression == COMPRESSION_ENABLED && \
>>>> + x.compression_type == COMPRESSION_TYPE_3D)
>>>> +
>>>> +#define BLT_TARGET_MC(x) (x.compression == COMPRESSION_ENABLED && \
>>>> + x.compression_type == COMPRESSION_TYPE_MEDIA)
>>>> +
>>>> +static uint32_t blt_compression_format(struct blt_copy_data *blt,
>>>> + const struct igt_fb *fb)
>>>> +{
>>>> + if (blt->src.compression == COMPRESSION_DISABLED &&
>>>> + blt->dst.compression == COMPRESSION_DISABLED)
>>>> + return 0;
>>>> +
>>>> + if (BLT_TARGET_RC(blt->src) || BLT_TARGET_RC(blt->dst)) {
>>>> + switch (blt->color_depth)
>>>> + {
>>>> + case CD_32bit:
>>>> + return 8;
>>>> + default:
>>>> + igt_assert_f(0, "COMPRESSION_TYPE_3D unknown color depth\n");
>>>> + }
>>>> + }
>>>> +
>>>> + if (BLT_TARGET_MC(blt->src) || BLT_TARGET_MC(blt->dst)) {
>>>> + switch (fb->drm_format)
>>>> + {
>>>> + case DRM_FORMAT_XRGB8888:
>>>> + return 8;
>>>> + case DRM_FORMAT_XYUV8888:
>>>> + return 9;
>>>> + case DRM_FORMAT_NV12:
>>>> + return 9;
>>>> + case DRM_FORMAT_P010:
>>>> + case DRM_FORMAT_P012:
>>>> + case DRM_FORMAT_P016:
>>>> + return 8;
>>>> + default:
>>>> + igt_assert_f(0, "COMPRESSION_TYPE_MEDIA unknown format\n");
>>>> + }
>>>> + } else {
>>>> + igt_assert_f(0, "unknown compression\n");
>>>> + }
>>>> +}
>>>> +
>>>> static void blitcopy(const struct igt_fb *dst_fb,
>>>> const struct igt_fb *src_fb)
>>>> {
>>>> uint32_t src_tiling, dst_tiling;
>>>> uint32_t ctx = 0;
>>>> uint64_t ahnd = 0;
>>>> + const intel_ctx_t *ictx = intel_ctx_create_all_physical(src_fb->fd);
>>>> + struct intel_execution_engine2 *e;
>>>> + uint32_t bb;
>>>> + uint64_t bb_size = 4096;
>>>> + struct blt_copy_data blt = {};
>>>> + struct blt_copy_object *src, *dst;
>>>> + struct blt_block_copy_data_ext ext = {}, *pext = NULL;
>>>> + uint32_t mem_region = HAS_FLATCCS(intel_get_drm_devid(src_fb->fd))
>>>> + ? REGION_LMEM(0) : REGION_SMEM;
>>>> igt_assert_eq(dst_fb->fd, src_fb->fd);
>>>> igt_assert_eq(dst_fb->num_planes, src_fb->num_planes);
>>>> @@ -2697,36 +2821,81 @@ static void blitcopy(const struct igt_fb *dst_fb,
>>>> igt_require(gem_has_contexts(dst_fb->fd));
>>>> ctx = gem_context_create(dst_fb->fd);
>>>> ahnd = get_reloc_ahnd(dst_fb->fd, ctx);
>>>> +
>>>> + igt_assert(__gem_create_in_memory_regions(src_fb->fd,
>>>> + &bb,
>>>> + &bb_size,
>>>> + mem_region) == 0);
>>>> +
>>>> + for_each_ctx_engine(src_fb->fd, ictx, e) {
>>>> + if (gem_engine_can_block_copy(src_fb->fd, e))
>>>> + break;
>>>> + }
>>>> }
>>>> for (int i = 0; i < dst_fb->num_planes; i++) {
>>>> igt_assert_eq(dst_fb->plane_bpp[i], src_fb->plane_bpp[i]);
>>>> igt_assert_eq(dst_fb->plane_width[i], src_fb->plane_width[i]);
>>>> igt_assert_eq(dst_fb->plane_height[i], src_fb->plane_height[i]);
>>>> - /*
>>>> - * On GEN12+ X-tiled format support is removed from the fast
>>>> - * blit command, so use the XY_SRC blit command for it
>>>> - * instead.
>>>> - */
>>>> +
>>>> if (fast_blit_ok(src_fb) && fast_blit_ok(dst_fb)) {
>>>
>>> I think this blit fail when ahnd == 0 as i915_blt requires softpinning.
>>
>> Also, are we fine with the fact that we'll only cover gen 8 and above (due
>> to the softpinning requirement)?
>
> Hmm, instead of fast_blit_ok() we should use here blt_has_fast_copy() and
> blt_fast_copy_supports_tiling().
I'm just confused by I915_FORMAT_MOD_X_TILED check. Is it the same as
checking for TileX? If so, then we could indeed rewrite it in such a way.
> Gen8 won't be covered here as fast copy doesn't exists there.
Right, by this comment I meant if we want to use a different blit as
well, or are we fine with using fast_copy only.
> BTW it looks a little bit weird .cmds_info on gen9
> pointing to gen11_cmds_info. I started to look on _cmds_info first to find
> out is fast_copy supported on gen8 and gen9 and I thought it is not.
Oh my, things are complicated over there, I agree. If time permits, I
shall re-review the naming convention used there.
> Consider adding separated entry for gen9, even at the cost of additional
> cmds_info variable.
I believe it'll duplicate what's in gen11, wouldn't it? Nevermind, I'll
check it out and see if I can make it less confusing.
Many thanks,
Karolina
>
>>
>>>
>>> 'if (ahnd && fast_blit_ok(src_fb) && fast_blit_ok(dst_fb))' should work.
>>
>> I had a quick look at the function, and I wonder if we could i915_blt
>> helpers here instead. There seem to be other factors that we have to
>> consider here, but the display version checks seem similar to what we have
>> in intel_cmd_info lib.
>
> Yes, i915_blt helpers are more reliable for platforms.
>
> --
> Zbigniew
>
>>
>>
>> All the best,
>> Karolina
>>
>>>
>>>> - igt_blitter_fast_copy__raw(dst_fb->fd,
>>>> - ahnd, ctx, NULL,
>>>> - src_fb->gem_handle,
>>>> - src_fb->offsets[i],
>>>> - src_fb->strides[i],
>>>> - src_tiling,
>>>> - 0, 0, /* src_x, src_y */
>>>> - src_fb->size,
>>>> - dst_fb->plane_width[i],
>>>> - dst_fb->plane_height[i],
>>>> - dst_fb->plane_bpp[i],
>>>> - dst_fb->gem_handle,
>>>> - dst_fb->offsets[i],
>>>> - dst_fb->strides[i],
>>>> - dst_tiling,
>>>> - 0, 0 /* dst_x, dst_y */,
>>>> - dst_fb->size);
>>>> + memset(&blt, 0, sizeof(blt));
>>>> + blt.color_depth = blt_get_bpp(src_fb);
>>>> +
>>>> + src = blt_fb_init(src_fb, i, mem_region);
>>>> + dst = blt_fb_init(dst_fb, i, mem_region);
>>>> +
>>>> + blt_set_copy_object(&blt.src, src);
>>>> + blt_set_copy_object(&blt.dst, dst);
>>>> +
>>>> + blt_set_batch(&blt.bb, bb, bb_size, mem_region);
>>>> +
>>>> + blt_fast_copy(src_fb->fd, ictx, e, ahnd, &blt);
>>>> + gem_sync(src_fb->fd, blt.dst.handle);
>>>> +
>>>> + blt_destroy_object(src_fb->fd, src);
>>>> + blt_destroy_object(dst_fb->fd, dst);
>>>> + } else if (ahnd) {
>>>
>>> I would also check if this else supports block-copy.
>>>
>>> If you prefer newer instructions (like fast-copy) with i915_blt it
>>> requires valid ahnd (>0). Instead of using wrapper:
>>>
>>> ahnd = get_reloc_ahnd(fd, ctx);
>>>
>>> you may enforce softpinning by:
>>>
>>> ahnd = intel_allocator_open(fd, ctx, INTEL_ALLOCATOR_RELOC);
>>>
>>> Then i915_blt functions will work as you expect. But this will change
>>> previous logic to prefer older blits instead new ones.
>>>
>>> --
>>> Zbigniew
>>>
>>>> + /*
>>>> + * On GEN12+ X-tiled format support is removed from
>>>> + * the fast blit command, so use the block copy blit
>>>> + * command for it instead.
>>>> + */
>>>> + src = blt_fb_init(src_fb, i, mem_region);
>>>> + dst = blt_fb_init(dst_fb, i, mem_region);
>>>> +
>>>> + memset(&blt, 0, sizeof(blt));
>>>> + blt.print_bb = true;
>>>> + blt.color_depth = blt_get_bpp(src_fb);
>>>> + blt_set_copy_object(&blt.src, src);
>>>> + blt_set_copy_object(&blt.dst, dst);
>>>> +
>>>> + if (HAS_FLATCCS(intel_get_drm_devid(src_fb->fd))) {
>>>> + blt_set_object_ext(&ext.src,
>>>> + blt_compression_format(&blt, src_fb),
>>>> + src_fb->width, src_fb->height,
>>>> + SURFACE_TYPE_2D);
>>>> +
>>>> + blt_set_object_ext(&ext.dst,
>>>> + blt_compression_format(&blt, dst_fb),
>>>> + dst_fb->width, dst_fb->height,
>>>> + SURFACE_TYPE_2D);
>>>> +
>>>> + pext = &ext;
>>>> + }
>>>> +
>>>> + blt_set_batch(&blt.bb, bb, bb_size, mem_region);
>>>> +
>>>> + blt_block_copy(src_fb->fd, ictx, e, ahnd, &blt, pext);
>>>> + gem_sync(src_fb->fd, blt.dst.handle);
>>>> +
>>>> + blt_destroy_object(src_fb->fd, src);
>>>> + blt_destroy_object(dst_fb->fd, dst);
>>>> } else {
>>>> + /*
>>>> + * If on legacy hardware where relocations are supported
>>>> + * we'll use XY_SRC blit command instead
>>>> + */
>>>> igt_blitter_src_copy(dst_fb->fd,
>>>> ahnd, ctx, NULL,
>>>> src_fb->gem_handle,
>>>> @@ -2750,6 +2919,7 @@ static void blitcopy(const struct igt_fb *dst_fb,
>>>> if (ctx)
>>>> gem_context_destroy(dst_fb->fd, ctx);
>>>> put_ahnd(ahnd);
>>>> + intel_ctx_destroy(src_fb->fd, ictx);
>>>> }
>>>> static void free_linear_mapping(struct fb_blit_upload *blit)
>>>> --
>>>> 2.39.0
>>>>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-03-30 7:42 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-27 20:12 [igt-dev] [PATCH i-g-t 0/2] switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw Juha-Pekka Heikkila
2023-03-27 20:12 ` [igt-dev] [PATCH i-g-t 1/2] lib/i915/i915_blt: Add offset to block and fast copy Juha-Pekka Heikkila
2023-03-27 20:12 ` [igt-dev] [PATCH i-g-t 2/2] lib/igt_fb: switch blitcopy to use lib/i915/i915_blt functions Juha-Pekka Heikkila
2023-03-28 6:50 ` Zbigniew Kempczyński
2023-03-28 14:10 ` Karolina Stolarek
2023-03-28 17:29 ` Juha-Pekka Heikkila
2023-03-29 6:53 ` Karolina Stolarek
2023-03-29 17:30 ` Zbigniew Kempczyński
2023-03-29 17:19 ` Zbigniew Kempczyński
2023-03-30 7:42 ` Karolina Stolarek
2023-03-27 20:34 ` [igt-dev] ✗ GitLab.Pipeline: warning for switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw Patchwork
2023-03-27 20:57 ` [igt-dev] ✗ Fi.CI.BAT: failure " Patchwork
2023-03-28 6:36 ` [igt-dev] ✓ Fi.CI.BAT: success for switch lib/igt_fb.c to use lib/i915/i915_blt functions for blitter on Intel hw (rev2) Patchwork
2023-03-28 14:00 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork
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.