[igt-dev] [PATCH i-g-t v3] tests/kms_big_fb: Make sure huge fbs work correctly
Ville Syrjälä
ville.syrjala at linux.intel.com
Fri May 10 12:31:00 UTC 2019
On Fri, May 10, 2019 at 10:53:51AM +0200, Daniel Vetter wrote:
> On Wed, May 08, 2019 at 07:29:06PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> > Add various tests to excercise huge framebuffers. First some basic
> > sanity checks that the kernel accepts/rejects good/bad addfb2 ioctls,
> > and finally actual scanout tests to make sure we scan out the correct
> > thing when panning around inside large framebuffers.
> >
> > The implementation is i915 specific for now since I chose to use
> > rendercopy/blitter when generating the framebuffer contents. Using
> > the normal cairo stuff was just too slow when dealing with 1GiB+
> > framebuffers. It shouldn't be too hard to plug in some other mechanisms
> > if someone else wants to reuse this test.
> >
> > v2: Add igt_require(format+mod) for the addfb/overflow tests
> > Unset plane fb after TEST_ONLY fail
> > Limit max fb to at most 1/2 RAM or GPU address space
> > Tweak coords to avoid fails with 64bpp linear with 4k screen
> > Make coords even for 90/270 rotated 16bpp
> > Store bpp as uint8_t instead of wasting an entire char*
> > v3: Add blitter path for gen3 (render engine is not capable of
> > handling big fbs).
> > Set fence tiling on gen2/3 in raw addfb tests.
> > Deal with weak max fence stride on gen3.
> > Don't try to use rotation when the prop isn't present.
> > Skip 90/270 test for gen4 (no atomic on those yet so we
> > can't neatly check what's supported).
> > Kernel restricts scanout to mappable portion of ggtt on
> > gmch platforms, so check its size as well
> > Use the correct fb for the legacy setcrtc
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> > tests/Makefile.sources | 1 +
> > tests/kms_big_fb.c | 666 +++++++++++++++++++++++++++++++++++++++++
> > tests/meson.build | 1 +
> > 3 files changed, 668 insertions(+)
> > create mode 100644 tests/kms_big_fb.c
> >
> > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > index 7f921f6c5988..2d5c929e32fc 100644
> > --- a/tests/Makefile.sources
> > +++ b/tests/Makefile.sources
> > @@ -27,6 +27,7 @@ TESTS_progs = \
> > kms_atomic_interruptible \
> > kms_atomic_transition \
> > kms_available_modes_crc \
> > + kms_big_fb \
> > kms_busy \
> > kms_ccs \
> > kms_color \
> > diff --git a/tests/kms_big_fb.c b/tests/kms_big_fb.c
> > new file mode 100644
> > index 000000000000..5235954e2d8c
> > --- /dev/null
> > +++ b/tests/kms_big_fb.c
> > @@ -0,0 +1,666 @@
> > +/*
> > + * Copyright © 2019 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + */
> > +
> > +#include "igt.h"
> > +#include <errno.h>
> > +#include <stdbool.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +
> > +IGT_TEST_DESCRIPTION("Test big framebuffers");
> > +
> > +typedef struct {
> > + int drm_fd;
> > + uint32_t devid;
> > + igt_display_t display;
> > + enum pipe pipe;
> > + igt_output_t *output;
> > + igt_plane_t *plane;
> > + igt_pipe_crc_t *pipe_crc;
> > + struct igt_fb small_fb, big_fb;
> > + uint32_t format;
> > + uint64_t modifier;
> > + int width, height;
> > + igt_rotation_t rotation;
> > + int max_fb_width, max_fb_height;
> > + uint64_t ram_size, aper_size, mappable_size;
> > + igt_render_copyfunc_t render_copy;
> > + drm_intel_bufmgr *bufmgr;
> > + struct intel_batchbuffer *batch;
> > +} data_t;
> > +
> > +static void init_buf(data_t *data,
> > + struct igt_buf *buf,
> > + const struct igt_fb *fb,
> > + const char *name)
> > +{
> > + igt_assert_eq(fb->offsets[0], 0);
> > +
> > + buf->bo = gem_handle_to_libdrm_bo(data->bufmgr, data->drm_fd,
> > + name, fb->gem_handle);
> > + buf->tiling = igt_fb_mod_to_tiling(fb->modifier);
> > + buf->stride = fb->strides[0];
> > + buf->bpp = fb->plane_bpp[0];
> > + buf->size = fb->size;
> > +}
> > +
> > +static void fini_buf(struct igt_buf *buf)
> > +{
> > + drm_intel_bo_unreference(buf->bo);
> > +}
> > +
> > +static void copy_pattern(data_t *data,
> > + struct igt_fb *dst_fb, int dx, int dy,
> > + struct igt_fb *src_fb, int sx, int sy,
> > + int w, int h)
> > +{
> > + struct igt_buf src = {}, dst = {};
> > +
> > + init_buf(data, &src, src_fb, "big fb src");
> > + init_buf(data, &dst, dst_fb, "big fb dst");
> > +
> > + gem_set_domain(data->drm_fd, dst_fb->gem_handle,
> > + I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> > + gem_set_domain(data->drm_fd, src_fb->gem_handle,
> > + I915_GEM_DOMAIN_GTT, 0);
> > +
>
> Maybe a comment here that we rely on the kernel not bumping the fb limits
> past what we can render into?
Sure.
>
> > + if (data->render_copy) {
> > + data->render_copy(data->batch, NULL, &src, sx, sy, w, h, &dst, dx, dy);
> > + } else {
> > + w = min(w, src_fb->width - sx);
> > + w = min(w, dst_fb->width - dx);
> > +
> > + h = min(h, src_fb->height - sy);
> > + h = min(h, dst_fb->height - dy);
> > +
> > + intel_blt_copy(data->batch, src.bo, sx, sy, src.stride,
> > + dst.bo, dx, dy, dst.stride, w, h, dst.bpp);
> > + }
> > +
> > + fini_buf(&dst);
> > + fini_buf(&src);
> > +}
> > +
> > +static void generate_pattern(data_t *data,
> > + struct igt_fb *fb,
> > + int w, int h)
> > +{
> > + struct igt_fb pat_fb;
> > +
> > + igt_create_pattern_fb(data->drm_fd, w, h,
> > + data->format, data->modifier,
> > + &pat_fb);
> > +
> > + for (int y = 0; y < fb->height; y += h) {
> > + for (int x = 0; x < fb->width; x += w) {
> > + copy_pattern(data, fb, x, y,
> > + &pat_fb, 0, 0,
> > + pat_fb.width, pat_fb.height);
> > + w++;
> > + h++;
> > + }
> > + }
> > +
> > + igt_remove_fb(data->drm_fd, &pat_fb);
> > +}
> > +
> > +static bool size_ok(data_t *data, uint64_t size)
> > +{
> > + /*
> > + * The kernel limits scanout to the
> > + * mappable portion of ggtt on gmch platforms.
> > + */
> > + if ((intel_gen(data->devid) < 5 ||
> > + IS_VALLEYVIEW(data->devid) ||
> > + IS_CHERRYVIEW(data->devid)) &&
> > + size > data->mappable_size / 2)
> > + return false;
> > +
> > + /*
> > + * Limit the big fb size to at most half the RAM or half
> > + * the aperture size. Could go a bit higher I suppose since
> > + * we shouldn't need more than one big fb at a time.
> > + */
> > + if (size > data->ram_size / 2 || size > data->aper_size / 2)
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +
> > +static void max_fb_size(data_t *data, int *width, int *height,
> > + uint32_t format, uint64_t modifier)
> > +{
> > + unsigned int stride;
> > + uint64_t size;
> > + int i = 0;
> > +
> > + *width = data->max_fb_width;
> > + *height = data->max_fb_height;
> > +
> > + /* max fence stride is only 8k bytes on gen3 */
> > + if (intel_gen(data->devid) < 4 &&
> > + format == DRM_FORMAT_XRGB8888)
> > + *width = min(*width, 8192 / 4);
> > +
> > + igt_calc_fb_size(data->drm_fd, *width, *height,
> > + format, modifier, &size, &stride);
> > +
> > + while (!size_ok(data, size)) {
> > + if (i++ & 1)
> > + *width >>= 1;
> > + else
> > + *height >>= 1;
> > +
> > + igt_calc_fb_size(data->drm_fd, *width, *height,
> > + format, modifier, &size, &stride);
> > + }
> > +
> > + igt_info("Max usable framebuffer size for format "IGT_FORMAT_FMT" / modifier 0x%"PRIx64": %dx%d\n",
> > + IGT_FORMAT_ARGS(format), modifier,
> > + *width, *height);
> > +}
> > +
> > +static bool test_plane(data_t *data)
> > +{
> > + igt_plane_t *plane = data->plane;
> > + struct igt_fb *small_fb = &data->small_fb;
> > + struct igt_fb *big_fb = &data->big_fb;
> > + int w = big_fb->width - small_fb->width;
> > + int h = big_fb->height - small_fb->height;
> > + struct {
> > + int x, y;
> > + } coords[] = {
> > + /* bunch of coordinates pulled out of thin air */
> > + { 0, 0, },
> > + { w * 4 / 7, h / 5, },
> > + { w * 3 / 7, h / 3, },
> > + { w / 2, h / 2, },
> > + { w / 3, h * 3 / 4, },
> > + { w, h, },
> > + };
> > +
> > + if (!igt_plane_has_format_mod(plane, data->format, data->modifier))
> > + return false;
> > +
> > + if (data->rotation != IGT_ROTATION_0 &&
> > + !igt_plane_has_prop(plane, IGT_PLANE_ROTATION))
> > + return false;
> > +
> > + /* FIXME need atomic on i965/g4x */
> > + if (data->rotation != IGT_ROTATION_0 &&
> > + data->rotation != IGT_ROTATION_180 &&
> > + !data->display.is_atomic)
> > + return false;
> > +
> > + if (igt_plane_has_prop(plane, IGT_PLANE_ROTATION))
> > + igt_plane_set_rotation(plane, data->rotation);
> > + igt_plane_set_position(plane, 0, 0);
> > +
> > + for (int i = 0; i < ARRAY_SIZE(coords); i++) {
> > + igt_crc_t small_crc, big_crc;
> > + int x = coords[i].x;
> > + int y = coords[i].y;
> > +
> > + /* Hardware limitation */
> > + if (data->format == DRM_FORMAT_RGB565 &&
> > + (data->rotation == IGT_ROTATION_90 ||
> > + data->rotation == IGT_ROTATION_270)) {
> > + x &= ~1;
> > + y &= ~1;
> > + }
> > +
> > + /*
> > + * Make a 1:1 copy of the desired part of the big fb
> > + * rather than try to render the same pattern (translated
> > + * accordinly) again via cairo. Something in cairo's
> > + * rendering pipeline introduces slight differences into
> > + * the result if we try that, and so the crc will not match.
> > + */
> > + copy_pattern(data, small_fb, 0, 0, big_fb, x, y,
> > + small_fb->width, small_fb->height);
> > +
> > + igt_plane_set_fb(plane, small_fb);
> > + igt_plane_set_size(plane, data->width, data->height);
> > +
> > + /*
> > + * Try to check that the rotation+format+modifier
> > + * combo is supported.
> > + */
> > + if (i == 0 && data->display.is_atomic &&
> > + igt_display_try_commit_atomic(&data->display,
> > + DRM_MODE_ATOMIC_TEST_ONLY,
> > + NULL) != 0) {
> > + if (igt_plane_has_prop(plane, IGT_PLANE_ROTATION))
> > + igt_plane_set_rotation(plane, IGT_ROTATION_0);
> > + igt_plane_set_fb(plane, NULL);
> > + igt_skip("unsupported plane configuration\n");
>
> This is kinda uncool if we have a plane later on that provokes a skip, but
> we've already run some tests. It's a gap in igt infrastructure that I need
> to fix eventually, but meanwhile you need to count how many skips you
> have, and only skip at the very end of your subtest if you skipped all
> possible combinations you might want to test.
The test only executes on a single plane. But this check does make
the assumption that if one plane can't do the format+mod+rotation
combo then none of them can do it. Which should be OK for i915, but
might make sense to remove that assumption in case someone else
wants to reuse this test.
Not sure there is much to gain from testing on more planes. Hmm. I
guess for the pre-skl there could in theory be some bug in the sprite
code that's not present in the primary plane code, or vice versa.
Although a lot of the code is shared between the plane types so it
probably wouldn't be a bug directly related to remapping. This again
is a somewhat i915 specific assumption though.
Anyways, I think your idea of doing the igt_skip higher up is a good
one.
>
> I think a return false here and changing the higher level logic to
> igt_skip if you didn't break out of the loop should do the trick.
Yeah, should be easy enough.
--
Ville Syrjälä
Intel
More information about the igt-dev
mailing list