Hi Chris,
On Wed, 27 Sep 2017 13:07:28 +0100
Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
> Quoting Boris Brezillon (2017-09-27 12:51:18)
> > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> > ---
> > tests/Makefile.sources | 1 +
> > tests/vc4_purgeable_bo.c | 274 +++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 275 insertions(+)
> > create mode 100644 tests/vc4_purgeable_bo.c
> >
> > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > index 0adc28a014d2..c78ac9d27921 100644
> > --- a/tests/Makefile.sources
> > +++ b/tests/Makefile.sources
> > @@ -8,6 +8,7 @@ VC4_TESTS = \
> > vc4_create_bo \
> > vc4_dmabuf_poll \
> > vc4_lookup_fail \
> > + vc4_purgeable_bo \
> > vc4_wait_bo \
> > vc4_wait_seqno \
> > $(NULL)
> > diff --git a/tests/vc4_purgeable_bo.c b/tests/vc4_purgeable_bo.c
> > new file mode 100644
> > index 000000000000..e3eaf0c24563
> > --- /dev/null
> > +++ b/tests/vc4_purgeable_bo.c
> > @@ -0,0 +1,274 @@
> > +/*
> > + * Copyright �� 2017 Broadcom
> > + *
> > + * 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 "igt_vc4.h"
> > +#include <unistd.h>
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <fcntl.h>
> > +#include <inttypes.h>
> > +#include <errno.h>
> > +#include <sys/stat.h>
> > +#include <sys/ioctl.h>
> > +#include "vc4_drm.h"
> > +
> > +struct igt_vc4_bo {
> > + struct igt_list node;
> > + int handle;
> > + void *map;
> > + size_t size;
> > +};
> > +
> > +static jmp_buf jmp;
> > +
> > +static void __attribute__((noreturn)) sigtrap(int sig)
> > +{
> > + longjmp(jmp, sig);
> > +}
> > +
> > +static void igt_vc4_alloc_mmap_max_bo(int fd, struct igt_list *list,
> > + size_t size)
> > +{
> > + struct igt_vc4_bo *bo;
> > + struct drm_vc4_create_bo create = {
> > + .size = size,
> > + };
> > +
> > + while (true) {
> > + if (igt_ioctl(fd, DRM_IOCTL_VC4_CREATE_BO, &create))
> > + break;
> > +
> > + bo = malloc(sizeof(*bo));
> > + igt_assert(bo);
> > + bo->handle = create.handle;
> > + bo->size = create.size;
> > + bo->map = igt_vc4_mmap_bo(fd, bo->handle, bo->size,
> > + PROT_READ | PROT_WRITE);
> > + igt_list_add_tail(&bo->node, list);
> > + }
> > +}
> > +
> > +static void igt_vc4_unmap_free_bo_pool(int fd, struct igt_list *list)
> > +{
> > + struct igt_vc4_bo *bo;
> > +
> > + while (!igt_list_empty(list)) {
> > + bo = igt_list_first_entry(list, bo, node);
> > + igt_assert(bo);
> > + igt_list_del(&bo->node);
> > + munmap(bo->map, bo->size);
> > + gem_close(fd, bo->handle);
> > + free(bo);
> > + }
> > +}
> > +
> > +static void igt_vc4_trigger_purge(int fd)
> > +{
>
> May I suggest a /proc/sys/vm/drop_caches-esque interface?
> For when you want to explicitly control reclaim.
Eric suggested to add a debugfs entry to control the purge, I just
thought I didn't really need it since I had a way to trigger this
mechanism without adding yet another userspace -> kernel interface that
will become part of the ABI and will have to be maintained forever.
If you think this is preferable, I'll go for the debugfs hook.
>
> > + struct igt_list list;
> > +
> > + igt_list_init(&list);
> > +
> > + /* Try to allocate as much as we can to trigger a purge. */
> > + igt_vc4_alloc_mmap_max_bo(fd, &list, 64 * 1024);
> > + igt_assert(!igt_list_empty(&list));
> > + igt_vc4_unmap_free_bo_pool(fd, &list);
> > +}
> > +
> > +igt_main
> > +{
> > + struct igt_vc4_bo *bo;
> > + struct igt_list list;
> > + uint32_t *map;
> > + int fd, ret;
> > +
> > + igt_fixture {
> > + fd = drm_open_driver(DRIVER_VC4);
> > + igt_list_init(&list);
> > +
> > + igt_vc4_alloc_mmap_max_bo(fd, &list, 64 * 1024);
> > + igt_assert(!igt_list_empty(&list));
> > + }
>
> igt_subtest("mark-willneed") {
> /* check that setting willneed on a new bo says retained */
> }
Will add this test in the next version.
>
> > +
> > + igt_subtest("mark-purgeable") {
> > + igt_list_for_each(bo, &list, node)
> > + igt_vc4_purgeable_bo(fd, bo->handle, true);
> > +
> > + igt_list_for_each(bo, &list, node)
> > + igt_vc4_purgeable_bo(fd, bo->handle, false);
>
> Hmm, if this fails early, the state of the list is unknown. Subsequent
> tests depend upon the state being known....
Well, yes, that's one of the problem I have with the current
vc4_purgeable_bo testsuite. As soon as one subtest fails it's likely to
impact other subtests.
>
> I am not sure if you want to preallocate all the bo, or at least if you
> do you should walk the list and verify them at the start of each subtest.
Okay, I'll allocate the BO pool at the beginning of each subtest and
release it before leaving the subtest. Note that I'm not guaranteed
that the release operation works, which means the next subtest might
not be able to allocate new BOs and will fail, but this failure is
actually caused by the previous subtest not the one that reports a
failure in IGT.
>
> > + }
> > +
> > + igt_subtest("mark-purgeable-twice") {
> > + bo = igt_list_first_entry(&list, bo, node);
> > + igt_vc4_purgeable_bo(fd, bo->handle, true);
> > + igt_vc4_purgeable_bo(fd, bo->handle, true);
> > + igt_vc4_purgeable_bo(fd, bo->handle, false);
> > + }
> > +
> > + igt_subtest("mark-unpurgeable-twice") {
> > + bo = igt_list_first_entry(&list, bo, node);
> > + igt_vc4_purgeable_bo(fd, bo->handle, true);
> > + igt_vc4_purgeable_bo(fd, bo->handle, false);
> > + igt_vc4_purgeable_bo(fd, bo->handle, false);
> > + }
> > +
> > + igt_subtest("access-purgeable-bo-mem") {
> > + bo = igt_list_first_entry(&list, bo, node);
> > + map = (uint32_t *)bo->map;
> > +
> > + /* Mark the BO as purgeable, but do not try to allocate a new
> > + * BO. This should leave the BO in a non-purged state unless
> > + * someone else tries to allocated a new BO.
> > + */
> > + igt_vc4_purgeable_bo(fd, bo->handle, true);
>
> Do you do immediate reaping of backing storage and vma on setting
> purgeable?
Nope.
> If the bo has been in use in earlier tests, then it will have
> pages that it may not free immediately, so the mapping might succeed?
It's likely what will happen, and that's actually what I intended to
test initially. Eric just pointed out that someone else (external to
the IGT test) might trigger a purge between the igt_vc4_purgeable_bo()
and the *map access.
> (Spots the purged-bo-mem later)
>
> I personally hate "may" tests with a passion. If the expected outcome is
> uncertain, how do you know if what it did matches expectations.
Well, it's not completely uncertain, we just have 2 valid results:
- SIGBUS event is received => the BO has been purged
- nothing happens and the *map assignments works => the BO has not been
purged yet.
>
> > +
> > + /* Accessing a purgeable BO may generate a SIGBUS event if the
> > + * BO has been purged by the system in the meantime.
> > + */
> > + signal(SIGSEGV, sigtrap);
> > + signal(SIGBUS, sigtrap);
> > + ret = setjmp(jmp);
> > + if (!ret)
> > + *map = 0xdeadbeef;
> > + else
> > + igt_assert(ret == SIGBUS);
> > + signal(SIGBUS, SIG_DFL);
> > + signal(SIGSEGV, SIG_DFL);
> > + }
> > +
>
> > + igt_subtest("mark-unpurgeable-not-purged") {
> > + igt_list_for_each(bo, &list, node) {
> > + map = (uint32_t *)bo->map;
> > + *map = 0xdeadbeef;
> > + igt_vc4_purgeable_bo(fd, bo->handle, true);
> > + }
>
> /* lots of intervening time for the system to change state? */
That's true.
>
> > +
> > + igt_list_for_each(bo, &list, node) {
> > + map = (uint32_t *)bo->map;
> > + igt_assert(igt_vc4_purgeable_bo(fd, bo->handle, false));
> > + igt_assert(*map == 0xdeadbeef);
I can change the above 2 lines into:
if (igt_vc4_purgeable_bo(fd, bo->handle, false))
igt_assert(*map == 0xdeadbeef);
else
igt_assert(*map != 0xdeadbeef);
so that it works even if someone triggered the purge between
igt_vc4_purgeable_bo(true) and igt_vc4_purgeable_bo(false).
> > + }
> > + }
Thanks for your review.
Boris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
