From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH igt] tests/kms_fence_pin_leak: Exercise full ppgtt fence pin_count leak in the kernel Date: Tue, 13 May 2014 11:24:48 +0300 Message-ID: <20140513082448.GF18465@intel.com> References: <1392383167-19948-3-git-send-email-daniel.vetter@ffwll.ch> <1399916784-30841-1-git-send-email-ville.syrjala@linux.intel.com> <20140512183407.GE25056@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 4AF946EA1C for ; Tue, 13 May 2014 01:24:52 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140512183407.GE25056@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Mon, May 12, 2014 at 08:34:07PM +0200, Daniel Vetter wrote: > On Mon, May 12, 2014 at 08:46:24PM +0300, ville.syrjala@linux.intel.com w= rote: > > From: Ville Syrj=E4l=E4 > > = > > The kernel full ppgtt support has a bug where it can drop a pinned > > fence to the floor, hence we leak the pin_count as the subsequent > > fence unpin becomes a nop. We can trigger it easily by unbinding a > > buffer from a ppgtt address space while the buffer is simultaneosly > > being used for scanout. > > = > > Monitor i915_gem_fence_regs to detect the leak while performing > > the required set of operations in a loop. > = > do we really need that part? I've hoped just leaking enough fences until > the kernel blows up should be sufficient. Sure if you want to let it run for >5 years. That's how long it would take to make the pin_count overflow on my IVB machine, assuming you hit the bug on every iteration of the loop, which for some reason doesn't seem to be the case here. Another option would be to trick the the pin_count to leak for every fence registers. Then we wouldn't be able to find a new fence and we'd get -EDEADLK from i915_find_fence_reg(). If I try to make sure all fences have an object associated with them, it should always pick a fence with pin_count=3D=3D0, so it seems like this approach should work. I'll work on a it a bit and we'll see... > Generally I'm vary of using debugfs in new tests a bit since we > essentially then lock down that debugfs file as abi. And since no one > easily notices when a test stops catching a bug that's usually bad. Hence > also why I want basic testcases for debugfs infrastructure to make sure it > keeps on working. > -Daniel > = > > = > > Signed-off-by: Ville Syrj=E4l=E4 > > --- > > tests/Makefile.sources | 1 + > > tests/kms_fence_pin_leak.c | 224 +++++++++++++++++++++++++++++++++++++= ++++++++ > > 2 files changed, 225 insertions(+) > > create mode 100644 tests/kms_fence_pin_leak.c > > = > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources > > index 5d5dc46..393c4a2 100644 > > --- a/tests/Makefile.sources > > +++ b/tests/Makefile.sources > > @@ -60,6 +60,7 @@ TESTS_progs_M =3D \ > > kms_addfb \ > > kms_cursor_crc \ > > kms_fbc_crc \ > > + kms_fence_pin_leak \ > > kms_flip \ > > kms_flip_tiling \ > > kms_pipe_crc_basic \ > > diff --git a/tests/kms_fence_pin_leak.c b/tests/kms_fence_pin_leak.c > > new file mode 100644 > > index 0000000..f299212 > > --- /dev/null > > +++ b/tests/kms_fence_pin_leak.c > > @@ -0,0 +1,224 @@ > > +/* > > + * Copyright =A9 2014 Intel Corporation > > + * > > + * Permission is hereby granted, free of charge, to any person obtaini= ng a > > + * copy of this software and associated documentation files (the "Soft= ware"), > > + * to deal in the Software without restriction, including without limi= tation > > + * the rights to use, copy, modify, merge, publish, distribute, sublic= ense, > > + * and/or sell copies of the Software, and to permit persons to whom t= he > > + * Software is furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice (including th= e 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, EXP= RESS OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABI= LITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT = SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES O= R OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARI= SING > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER= DEALINGS > > + * IN THE SOFTWARE. > > + * > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include "drmtest.h" > > +#include "igt_debugfs.h" > > +#include "igt_kms.h" > > +#include "ioctl_wrappers.h" > > +#include "intel_chipset.h" > > + > > +typedef struct { > > + int drm_fd; > > + uint32_t devid; > > + drm_intel_bufmgr *bufmgr; > > + igt_display_t display; > > +} data_t; > > + > > +static void exec_nop(data_t *data, uint32_t handle, drm_intel_context = *context) > > +{ > > + drm_intel_bo *dst; > > + struct intel_batchbuffer *batch; > > + > > + dst =3D gem_handle_to_libdrm_bo(data->bufmgr, data->drm_fd, "", handl= e); > > + igt_assert(dst); > > + > > + batch =3D intel_batchbuffer_alloc(data->bufmgr, data->devid); > > + igt_assert(batch); > > + > > + /* add the reloc to make sure the kernel will think we write to dst */ > > + BEGIN_BATCH(4); > > + OUT_BATCH(MI_BATCH_BUFFER_END); > > + OUT_BATCH(MI_NOOP); > > + OUT_RELOC(dst, I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0); > > + OUT_BATCH(MI_NOOP); > > + ADVANCE_BATCH(); > > + > > + intel_batchbuffer_flush_with_context(batch, context); > > + intel_batchbuffer_free(batch); > > + > > + drm_intel_bo_unreference(dst); > > +} > > + > > +static unsigned int total_fence_pin_count(void) > > +{ > > + unsigned int pinned =3D 0; > > + char buf[128]; > > + FILE *f; > > + > > + f =3D igt_debugfs_fopen("i915_gem_fence_regs", "r"); > > + igt_assert(f); > > + > > + for (;;) { > > + unsigned int pin_count =3D 0; > > + char *ptr; > > + > > + ptr =3D fgets(buf, sizeof(buf), f); > > + if (!ptr) > > + break; > > + > > + if (sscanf(ptr, "Fence %*u, pin count =3D %u", &pin_count) =3D=3D 1) > > + pinned +=3D pin_count; > > + } > > + > > + fclose(f); > > + > > + return pinned; > > +} > > + > > +static bool run_single_test(data_t *data, enum pipe pipe, igt_output_t= *output) > > +{ > > + igt_display_t *display =3D &data->display; > > + drmModeModeInfo *mode; > > + igt_plane_t *primary; > > + unsigned int pin_count; > > + struct igt_fb fb[2]; > > + int i; > > + > > + igt_output_set_pipe(output, pipe); > > + igt_display_commit(display); > > + > > + if (!output->valid) { > > + igt_output_set_pipe(output, PIPE_ANY); > > + igt_display_commit(display); > > + return false; > > + } > > + > > + mode =3D igt_output_get_mode(output); > > + primary =3D igt_output_get_plane(output, IGT_PLANE_PRIMARY); > > + > > + igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay, > > + DRM_FORMAT_XRGB8888, > > + true, /* need a fence so must be tiled */ > > + 0.0, 0.0, 0.0, > > + &fb[0]); > > + igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay, > > + DRM_FORMAT_XRGB8888, > > + true, /* need a fence so must be tiled */ > > + 0.0, 0.0, 0.0, > > + &fb[1]); > > + > > + igt_plane_set_fb(primary, &fb[0]); > > + igt_display_commit(display); > > + > > + /* sample the fence pin_counts now that we have one scanout buffer pi= nned */ > > + pin_count =3D total_fence_pin_count(); > > + > > + for (i =3D 0; i < 64; i++) { > > + drm_intel_context *ctx; > > + > > + /* > > + * Link fb.gem_handle to the ppgtt vm of ctx so that the context > > + * destruction will unbind the obj from the ppgtt vm in question. > > + */ > > + ctx =3D drm_intel_gem_context_create(data->bufmgr); > > + igt_assert(ctx); > > + exec_nop(data, fb[i&1].gem_handle, ctx); > > + drm_intel_gem_context_destroy(ctx); > > + > > + /* Force a context switch to make sure ctx gets destroyed for real. = */ > > + exec_nop(data, fb[i&1].gem_handle, NULL); > > + > > + gem_sync(data->drm_fd, fb[i&1].gem_handle); > > + > > + /* > > + * Pin the new buffer and unpin the old buffer from display. If > > + * the kernel is buggy the ppgtt unbind will have dropped the > > + * fence for the old buffer, and now the display code will try > > + * to unpin only to find no fence there. So the pin_count will leak. > > + */ > > + igt_plane_set_fb(primary, &fb[!(i&1)]); > > + igt_display_commit(display); > > + > > + /* > > + * We always have exactly one scanout buffer, so the fence pin_count > > + * should remain static. Fail the test if we detect a leak. > > + */ > > + igt_assert(pin_count =3D=3D total_fence_pin_count()); > > + > > + printf("."); > > + fflush(stdout); > > + } > > + > > + igt_plane_set_fb(primary, NULL); > > + igt_output_set_pipe(output, PIPE_ANY); > > + igt_display_commit(display); > > + > > + igt_remove_fb(data->drm_fd, &fb[1]); > > + igt_remove_fb(data->drm_fd, &fb[0]); > > + > > + printf("\n"); > > + > > + return true; > > +} > > + > > +static void run_test(data_t *data) > > +{ > > + igt_display_t *display =3D &data->display; > > + igt_output_t *output; > > + enum pipe p; > > + > > + for_each_connected_output(display, output) { > > + for (p =3D 0; p < igt_display_get_n_pipes(display); p++) { > > + if (run_single_test(data, p, output)) > > + return; /* one time ought to be enough */ > > + } > > + } > > + > > + igt_skip("no valid crtc/connector combinations found\n"); > > +} > > + > > +igt_simple_main > > +{ > > + drm_intel_context *ctx; > > + data_t data =3D {}; > > + > > + igt_skip_on_simulation(); > > + > > + data.drm_fd =3D drm_open_any(); > > + > > + data.devid =3D intel_get_drm_devid(data.drm_fd); > > + > > + igt_set_vt_graphics_mode(); > > + > > + data.bufmgr =3D drm_intel_bufmgr_gem_init(data.drm_fd, 4096); > > + igt_assert(data.bufmgr); > > + drm_intel_bufmgr_gem_enable_reuse(data.bufmgr); > > + > > + igt_display_init(&data.display, data.drm_fd); > > + > > + ctx =3D drm_intel_gem_context_create(data.bufmgr); > > + igt_require(ctx); > > + drm_intel_gem_context_destroy(ctx); > > + > > + run_test(&data); > > + > > + drm_intel_bufmgr_destroy(data.bufmgr); > > + igt_display_fini(&data.display); > > +} > > -- = > > 1.8.3.2 > > = > = > -- = > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- = Ville Syrj=E4l=E4 Intel OTC