[igt-dev] [PATCH i-g-t 3/7] tests/gem_render_copy: Test Yf tiling
Katarzyna Dec
katarzyna.dec at intel.com
Fri Mar 8 09:59:55 UTC 2019
On Wed, Mar 06, 2019 at 04:58:02PM -0800, Dhinakaran Pandiyan wrote:
> On Tue, 2019-03-05 at 15:26 +0100, Katarzyna Dec wrote:
> > On Mon, Mar 04, 2019 at 05:47:38PM -0800, Dhinakaran Pandiyan wrote:
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > >
> > > Let's test Yf tiling now that rendercopy can handle it.
> > >
> > > v2: From DK
> > > Set bpp for Yf buffer and rebase.
> > >
> > > Cc: Lukasz Kalamarz <lukasz.kalamarz at intel.com>
> > > Cc: Katarzyna Dec <katarzyna.dec at intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > ---
> > > tests/i915/gem_render_copy.c | 246 +++++++++++++++++++++++++++--
> > > ------
> > > 1 file changed, 195 insertions(+), 51 deletions(-)
> > >
> > > diff --git a/tests/i915/gem_render_copy.c
> > > b/tests/i915/gem_render_copy.c
> > > index 0cd4e50f..afe2df05 100644
> > > --- a/tests/i915/gem_render_copy.c
> > > +++ b/tests/i915/gem_render_copy.c
> > > @@ -72,11 +72,85 @@ static const char *make_filename(const char
> > > *filename)
> > > return buf;
> > > }
> > >
> > > -static void *linear_copy(data_t *data, struct igt_buf *buf)
> > > +static void *yf_ptr(void *ptr,
> > > + unsigned int x, unsigned int y,
> > > + unsigned int stride, unsigned int cpp)
> > > {
> > > - void *map, *linear;
> > > + x *= cpp;
> > > +
> > > + return ptr +
> > > + ((y & ~0x1f) * stride) +
> > > + ((y & 0x10) * 64) +
> > > + ((y & 0x8) * 32) +
> > > + ((y & 0x7) * 16) +
> > > + ((x & ~0x3f) * 32) +
> > > + ((x & 0x20) * 16) +
> > > + ((x & 0x10) * 8) +
> > > + (x & 0xf);
> > > +}
> >
> > I looked through documentation and these ^ values e.g. ~0x1f others
> > x/y & ? are
> > still mysterious. Maybe you can document them somehow?
> The code reflects the internal tiling layout for Yf, which is
> documented in libdrm: include/drm/drm_fourcc.h. I don't see any
> documentation for other tiling formats in IGT either, so not sure how
> exactly you want me to document this. Suggestions welcome :)
>
> But, I think it's better to do any additional documentation work
> outside this series as this is delaying an ICL bug fix.
I looked though libdrm code you mentioned and it still doesn't make any sense to
me. I also looked into gem_render_copy test and I agree that there is not so
much of documentation, but what you are adding is introducing many magic numbers.
Maybe you could add e.g. #defines for this values.
For updating documentation - I agree that bugs fix is urgent, however if we
ommit it now, there will be for sure no time in the future to fix it.
Kasia :)
>
> >
> > >
> > > - igt_assert_eq(posix_memalign(&linear, 16, buf->bo->size), 0);
> > > +static void copy_linear_to_yf(data_t *data, struct igt_buf *buf,
> > > const uint32_t *linear)
> >
> > It would be better to move 'const uin32_t *linear' to next line
> > (aligned with
> > others).
> > > +{
> > > + int height = igt_buf_height(buf);
> > > + int width = igt_buf_width(buf);
> > > + void *map;
> > > +
> > > + gem_set_domain(data->drm_fd, buf->bo->handle,
> > > + I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
> > > + map = gem_mmap__cpu(data->drm_fd, buf->bo->handle, 0,
> > > + buf->bo->size, PROT_READ | PROT_WRITE);
> > > +
> > > + for (int y = 0; y < height; y++) {
> > > + for (int x = 0; x < width; x++) {
> > > + uint32_t *ptr = yf_ptr(map, x, y, buf->stride,
> > > 4);
> >
> > Value '4' here is a cpp. Is it color per pixel? Maybe we can define
> > this value
> > somehow? I want to avoid situation when there will be no people
> > understanding
> > this code onboard.
> Changing this to buf->bpp / 8, that should be more intuitive.
>
> > > +
> > > + *ptr = linear[y * width + x];
> > > + }
> > > + }
> > > +
> > > + munmap(map, buf->bo->size);
> > > +}
> > > +
> > > +static void copy_yf_to_linear(data_t *data, struct igt_buf *buf,
> > > uint32_t *linear)
> > > +{
> > > + int height = igt_buf_height(buf);
> > > + int width = igt_buf_width(buf);
> > > + void *map;
> > > +
> > > + gem_set_domain(data->drm_fd, buf->bo->handle,
> > > + I915_GEM_DOMAIN_CPU, 0);
> > > + map = gem_mmap__cpu(data->drm_fd, buf->bo->handle, 0,
> > > + buf->bo->size, PROT_READ);
> > > +
> > > + for (int y = 0; y < height; y++) {
> > > + for (int x = 0; x < width; x++) {
> > > + uint32_t *ptr = yf_ptr(map, x, y, buf->stride,
> > > 4);
> > > +
> > > + linear[y * width + x] = *ptr;
> > > + }
> > > + }
> > > +
> > > + munmap(map, buf->bo->size);
> > > +}
> > > +
> > > +static void copy_linear_to_gtt(data_t *data, struct igt_buf *buf,
> > > const uint32_t *linear)
> > > +{
> > > + void *map;
> > > +
> > > + gem_set_domain(data->drm_fd, buf->bo->handle,
> > > + I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> > > +
> > > + map = gem_mmap__gtt(data->drm_fd, buf->bo->handle,
> > > + buf->bo->size, PROT_READ | PROT_WRITE);
> > > +
> > > + memcpy(map, linear, buf->bo->size);
> > > +
> > > + munmap(map, buf->bo->size);
> > > +}
> > > +
> > > +static void copy_gtt_to_linear(data_t *data, struct igt_buf *buf,
> > > uint32_t *linear)
> > > +{
> > > + void *map;
> > >
> > > gem_set_domain(data->drm_fd, buf->bo->handle,
> > > I915_GEM_DOMAIN_GTT, 0);
> > > @@ -87,6 +161,18 @@ static void *linear_copy(data_t *data, struct
> > > igt_buf *buf)
> > > igt_memcpy_from_wc(linear, map, buf->bo->size);
> > >
> > > munmap(map, buf->bo->size);
> > > +}
> > > +
> > > +static void *linear_copy(data_t *data, struct igt_buf *buf)
> > > +{
> > > + void *linear;
> > > +
> > > + igt_assert_eq(posix_memalign(&linear, 16, buf->bo->size), 0);
> >
> > There is not silly questions, I believe, what is '16' ^ above? Is
> > this size
> > fixed somehow? I would prefer defining it in some way.
> The alignment is useful to switch to faster implementation of memcpy
> using SSE4 instructions.
>
> > > +
> > > + if (buf->tiling == I915_TILING_Yf)
> > > + copy_yf_to_linear(data, buf, linear);
> > > + else
> > > + copy_gtt_to_linear(data, buf, linear);
> > >
> > > return linear;
> > > }
> > > @@ -173,7 +259,7 @@ static void scratch_buf_draw_pattern(data_t
> > > *data, struct igt_buf *buf,
> > > cairo_surface_t *surface;
> > > cairo_pattern_t *pat;
> > > cairo_t *cr;
> > > - void *map, *linear;
> > > + void *linear;
> > >
> > > linear = linear_copy(data, buf);
> > >
> > > @@ -216,15 +302,10 @@ static void scratch_buf_draw_pattern(data_t
> > > *data, struct igt_buf *buf,
> > >
> > > cairo_surface_destroy(surface);
> > >
> > > - gem_set_domain(data->drm_fd, buf->bo->handle,
> > > - I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> > > -
> > > - map = gem_mmap__gtt(data->drm_fd, buf->bo->handle,
> > > - buf->bo->size, PROT_READ | PROT_WRITE);
> > > -
> > > - memcpy(map, linear, buf->bo->size);
> > > -
> > > - munmap(map, buf->bo->size);
> > > + if (buf->tiling == I915_TILING_Yf)
> > > + copy_linear_to_yf(data, buf, linear);
> > > + else
> > > + copy_linear_to_gtt(data, buf, linear);
> > >
> > > free(linear);
> > > }
> > > @@ -236,36 +317,59 @@ scratch_buf_copy(data_t *data,
> > > {
> > > int width = igt_buf_width(dst);
> > > int height = igt_buf_height(dst);
> > > - uint32_t *linear_dst, *linear_src;
> > > + uint32_t *linear_dst;
> > >
> > > igt_assert_eq(igt_buf_width(dst), igt_buf_width(src));
> > > igt_assert_eq(igt_buf_height(dst), igt_buf_height(src));
> > > igt_assert_eq(dst->bo->size, src->bo->size);
> > >
> > > + w = min(w, width - sx);
> > > + w = min(w, width - dx);
> > > +
> > > + h = min(h, height - sy);
> > > + h = min(h, height - dy);
> > > +
> > > gem_set_domain(data->drm_fd, dst->bo->handle,
> > > I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> > > - gem_set_domain(data->drm_fd, src->bo->handle,
> > > - I915_GEM_DOMAIN_GTT, 0);
> > > -
> > > linear_dst = gem_mmap__gtt(data->drm_fd, dst->bo->handle,
> > > dst->bo->size, PROT_WRITE);
> > > - linear_src = gem_mmap__gtt(data->drm_fd, src->bo->handle,
> > > - src->bo->size, PROT_READ);
> > >
> > > - w = min(w, width - sx);
> > > - w = min(w, width - dx);
> > > + if (src->tiling == I915_TILING_Yf) {
> > > + void *map;
> > >
> > > - h = min(h, height - sy);
> > > - h = min(h, height - dy);
> > > + gem_set_domain(data->drm_fd, src->bo->handle,
> > > + I915_GEM_DOMAIN_CPU, 0);
> > > + map = gem_mmap__cpu(data->drm_fd, src->bo->handle, 0,
> > > + src->bo->size, PROT_READ);
> > > +
> > > + for (int y = 0; y < h; y++) {
> > > + for (int x = 0; x < w; x++) {
> > > + const uint32_t *ptr = yf_ptr(map, sx+x,
> > > sy+y, src->stride, 4);
> >
> > Same comment as above according '4'.
>
> s/4/src->bpp / 8
>
> > > +
> > > + linear_dst[(dy+y) * width + dx+x] =
> > > *ptr;
> > > + }
> > > + }
> > > +
> > > + munmap(map, src->bo->size);
> > > + } else {
> > > + uint32_t *linear_src;
> > > +
> > > + gem_set_domain(data->drm_fd, src->bo->handle,
> > > + I915_GEM_DOMAIN_GTT, 0);
> > >
> > > - for (int y = 0; y < h; y++) {
> > > - igt_memcpy_from_wc(&linear_dst[(dy+y) * width + dx],
> > > - &linear_src[(sy+y) * width + sx],
> > > - w * 4);
> >
> > How can you make sure that w*4 is enough?
> Same as above, will switch to src->bpp / 4 and an assertion for dst-
> >bpp == src->bpp
>
> -DK
>
> > (Again - this can be a silly
> > question). As I see below it can be connected with stride, is it?
> >
> > Kasia
> > > + linear_src = gem_mmap__gtt(data->drm_fd, src->bo-
> > > >handle,
> > > + src->bo->size, PROT_READ);
> > > +
> > > + for (int y = 0; y < h; y++) {
> > > + igt_memcpy_from_wc(&linear_dst[(dy+y) * width +
> > > dx],
> > > + &linear_src[(sy+y) * width +
> > > sx],
> > > + w * 4);
> > > + }
> > > +
> > > + munmap(linear_src, src->bo->size);
> > > }
> > >
> > > munmap(linear_dst, dst->bo->size);
> > > - munmap(linear_src, src->bo->size);
> > > }
> > >
> > > static void scratch_buf_init(data_t *data, struct igt_buf *buf,
> > > @@ -282,7 +386,8 @@ static void scratch_buf_init(data_t *data,
> > > struct igt_buf *buf,
> > > int size;
> > >
> > > igt_require(intel_gen(data->devid) >= 9);
> > > - igt_assert_eq(tiling, I915_TILING_Y);
> > > + igt_assert(tiling == I915_TILING_Y ||
> > > + tiling == I915_TILING_Yf);
> > >
> > > buf->stride = ALIGN(width * 4, 128);
> > > buf->size = buf->stride * height;
> > > @@ -299,8 +404,21 @@ static void scratch_buf_init(data_t *data,
> > > struct igt_buf *buf,
> > >
> > > buf->bo = drm_intel_bo_alloc(data->bufmgr, "", size,
> > > 4096);
> > >
> > > - drm_intel_bo_set_tiling(buf->bo, &tiling, buf->stride);
> > > - igt_assert_eq(tiling, req_tiling);
> > > + if (tiling == I915_TILING_Y) {
> > > + drm_intel_bo_set_tiling(buf->bo, &tiling, buf-
> > > >stride);
> > > + igt_assert_eq(tiling, req_tiling);
> > > + }
> > > + } else if (req_tiling == I915_TILING_Yf) {
> > > + int size;
> > > +
> > > + buf->stride = ALIGN(width * 4, 128);
> > > + buf->size = buf->stride * height;
> > > + buf->tiling = tiling;
> > > + buf->bpp = 32;
> > > +
> > > + size = buf->stride * ALIGN(height, 32);
> > > +
> > > + buf->bo = drm_intel_bo_alloc(data->bufmgr, "", size,
> > > 4096);
> > > } else {
> > > buf->bo = drm_intel_bo_alloc_tiled(data->bufmgr, "",
> > > width, height, 4,
> > > @@ -396,7 +514,7 @@ static void scratch_buf_aux_check(data_t *data,
> > > "Aux surface indicates that nothing was
> > > compressed\n");
> > > }
> > >
> > > -static void test(data_t *data, uint32_t tiling, bool test_ccs)
> > > +static void test(data_t *data, uint32_t tiling, uint64_t
> > > ccs_modifier)
> > > {
> > > struct igt_buf dst, ccs, ref;
> > > struct {
> > > @@ -404,7 +522,7 @@ static void test(data_t *data, uint32_t tiling,
> > > bool test_ccs)
> > > const char *filename;
> > > uint32_t tiling;
> > > int x, y;
> > > - } src[3] = {
> > > + } src[] = {
> > > {
> > > .filename = "source-linear.png",
> > > .tiling = I915_TILING_NONE,
> > > @@ -420,18 +538,31 @@ static void test(data_t *data, uint32_t
> > > tiling, bool test_ccs)
> > > .tiling = I915_TILING_Y,
> > > .x = WIDTH/2+1, .y = 1,
> > > },
> > > + {
> > > + .filename = "source-yf-tiled.png",
> > > + .tiling = I915_TILING_Yf,
> > > + .x = 1, .y = 1,
> > > + },
> > > };
> > >
> > > int opt_dump_aub = igt_aub_dump_enabled();
> > > + int num_src = ARRAY_SIZE(src);
> > > +
> > > + /* no Yf before gen9 */
> > > + if (intel_gen(data->devid) < 9)
> > > + num_src--;
> > > +
> > > + if (tiling == I915_TILING_Yf || ccs_modifier)
> > > + igt_require(intel_gen(data->devid) >= 9);
> > >
> > > - for (int i = 0; i < ARRAY_SIZE(src); i++)
> > > + for (int i = 0; i < num_src; i++)
> > > scratch_buf_init(data, &src[i].buf, WIDTH, HEIGHT,
> > > src[i].tiling, false);
> > > scratch_buf_init(data, &dst, WIDTH, HEIGHT, tiling, false);
> > > - if (test_ccs)
> > > - scratch_buf_init(data, &ccs, WIDTH, HEIGHT,
> > > I915_TILING_Y, true);
> > > + if (ccs_modifier)
> > > + scratch_buf_init(data, &ccs, WIDTH, HEIGHT,
> > > ccs_modifier, true);
> > > scratch_buf_init(data, &ref, WIDTH, HEIGHT, I915_TILING_NONE,
> > > false);
> > >
> > > - for (int i = 0; i < ARRAY_SIZE(src); i++)
> > > + for (int i = 0; i < num_src; i++)
> > > scratch_buf_draw_pattern(data, &src[i].buf,
> > > 0, 0, WIDTH, HEIGHT,
> > > 0, 0, WIDTH, HEIGHT, true);
> > > @@ -442,13 +573,13 @@ static void test(data_t *data, uint32_t
> > > tiling, bool test_ccs)
> > > scratch_buf_copy(data,
> > > &dst, 0, 0, WIDTH, HEIGHT,
> > > &ref, 0, 0);
> > > - for (int i = 0; i < ARRAY_SIZE(src); i++)
> > > + for (int i = 0; i < num_src; i++)
> > > scratch_buf_copy(data,
> > > &src[i].buf, WIDTH/4, HEIGHT/4,
> > > WIDTH/2-2, HEIGHT/2-2,
> > > &ref, src[i].x, src[i].y);
> > >
> > > if (opt_dump_png) {
> > > - for (int i = 0; i < ARRAY_SIZE(src); i++)
> > > + for (int i = 0; i < num_src; i++)
> > > scratch_buf_write_to_png(data, &src[i].buf,
> > > src[i].filename);
> > > scratch_buf_write_to_png(data, &dst,
> > > "destination.png");
> > > scratch_buf_write_to_png(data, &ref, "reference.png");
> > > @@ -468,24 +599,24 @@ static void test(data_t *data, uint32_t
> > > tiling, bool test_ccs)
> > > * |dst|src|
> > > * -------
> > > */
> > > - if (test_ccs)
> > > + if (ccs_modifier)
> > > data->render_copy(data->batch, NULL,
> > > &dst, 0, 0, WIDTH, HEIGHT,
> > > &ccs, 0, 0);
> > >
> > > - for (int i = 0; i < ARRAY_SIZE(src); i++)
> > > + for (int i = 0; i < num_src; i++)
> > > data->render_copy(data->batch, NULL,
> > > &src[i].buf, WIDTH/4, HEIGHT/4,
> > > WIDTH/2-2, HEIGHT/2-2,
> > > - test_ccs ? &ccs : &dst, src[i].x,
> > > src[i].y);
> > > + ccs_modifier ? &ccs : &dst, src[i].x,
> > > src[i].y);
> > >
> > > - if (test_ccs)
> > > + if (ccs_modifier)
> > > data->render_copy(data->batch, NULL,
> > > &ccs, 0, 0, WIDTH, HEIGHT,
> > > &dst, 0, 0);
> > >
> > > if (opt_dump_png){
> > > scratch_buf_write_to_png(data, &dst, "result.png");
> > > - if (test_ccs) {
> > > + if (ccs_modifier) {
> > > scratch_buf_write_to_png(data, &ccs,
> > > "compressed.png");
> > > scratch_buf_aux_write_to_png(data, &ccs,
> > > "compressed-aux.png");
> > > }
> > > @@ -505,7 +636,7 @@ static void test(data_t *data, uint32_t tiling,
> > > bool test_ccs)
> > > scratch_buf_check(data, &dst, &ref, WIDTH - 10, HEIGHT
> > > - 10);
> > > }
> > >
> > > - if (test_ccs)
> > > + if (ccs_modifier)
> > > scratch_buf_aux_check(data, &ccs);
> > > }
> > >
> > > @@ -546,18 +677,31 @@ int main(int argc, char **argv)
> > > }
> > >
> > > igt_subtest("linear")
> > > - test(&data, I915_TILING_NONE, false);
> > > + test(&data, I915_TILING_NONE, 0);
> > > igt_subtest("x-tiled")
> > > - test(&data, I915_TILING_X, false);
> > > + test(&data, I915_TILING_X, 0);
> > > igt_subtest("y-tiled")
> > > - test(&data, I915_TILING_Y, false);
> > > + test(&data, I915_TILING_Y, 0);
> > > + igt_subtest("yf-tiled")
> > > + test(&data, I915_TILING_Yf, 0);
> > >
> > > igt_subtest("y-tiled-ccs-to-linear")
> > > - test(&data, I915_TILING_NONE, true);
> > > + test(&data, I915_TILING_NONE, I915_TILING_Y);
> > > igt_subtest("y-tiled-ccs-to-x-tiled")
> > > - test(&data, I915_TILING_X, true);
> > > + test(&data, I915_TILING_X, I915_TILING_Y);
> > > igt_subtest("y-tiled-ccs-to-y-tiled")
> > > - test(&data, I915_TILING_Y, true);
> > > + test(&data, I915_TILING_Y, I915_TILING_Y);
> > > + igt_subtest("y-tiled-ccs-to-yf-tiled")
> > > + test(&data, I915_TILING_Yf, I915_TILING_Y);
> > > +
> > > + igt_subtest("yf-tiled-ccs-to-linear")
> > > + test(&data, I915_TILING_NONE, I915_TILING_Yf);
> > > + igt_subtest("yf-tiled-ccs-to-x-tiled")
> > > + test(&data, I915_TILING_X, I915_TILING_Yf);
> > > + igt_subtest("yf-tiled-ccs-to-y-tiled")
> > > + test(&data, I915_TILING_Y, I915_TILING_Yf);
> > > + igt_subtest("yf-tiled-ccs-to-yf-tiled")
> > > + test(&data, I915_TILING_Yf, I915_TILING_Yf);
> > >
> > > igt_fixture {
> > > intel_batchbuffer_free(data.batch);
> > > --
> > > 2.17.1
> > >
>
More information about the igt-dev
mailing list