All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: "Michał Winiarski" <michal.winiarski@intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] tests/gem_userptr_blits: subtests for MAP_FIXED mappings of regular bo
Date: Mon, 29 Jun 2015 15:01:12 +0100	[thread overview]
Message-ID: <55914FA8.4060305@linux.intel.com> (raw)
In-Reply-To: <1435575591-3510-1-git-send-email-michal.winiarski@intel.com>


On 06/29/2015 11:59 AM, Michał Winiarski wrote:
> When the the memory backing the userptr object is freed by the user, it's
> possible to trigger recursive deadlock caused by operations done on
> different BO mapped in that region, triggering invalidate.
>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>   tests/gem_userptr_blits.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 83 insertions(+)
>
> diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
> index 1f2cc96..3fe8f90 100644
> --- a/tests/gem_userptr_blits.c
> +++ b/tests/gem_userptr_blits.c
> @@ -640,6 +640,80 @@ static void test_forked_access(int fd)
>   	free(ptr2);
>   }
>
> +static int test_map_fixed_invalidate(int fd, bool overlap)
> +{
> +	void *ptr;
> +	void *map;
> +	int i;
> +	int num_handles = overlap ? 2 : 1;
> +	uint32_t handle[num_handles];
> +	uint32_t mmap_handle;
> +	struct drm_i915_gem_mmap_gtt mmap_arg;
> +
> +	igt_assert(posix_memalign(&ptr, PAGE_SIZE, PAGE_SIZE) == 0);
> +	for (i=0; i<num_handles; i++)
> +		igt_assert(gem_userptr(fd, ptr, PAGE_SIZE, 0, &handle[i]) == 0);
> +	free(ptr);

I am not sure we can rely on free triggering munmap(2) here, I think 
this is just glibc implementation detail. So I would suggest allocating 
with mmap and freeing with munmap.

> +
> +	mmap_handle = gem_create(fd, PAGE_SIZE);
> +
> +	memset(&mmap_arg, 0, sizeof(mmap_arg));
> +	mmap_arg.handle = mmap_handle;
> +	do_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP_GTT, &mmap_arg);
> +	map = mmap(ptr, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED,
> +			fd, mmap_arg.offset);
> +	igt_assert(map != MAP_FAILED);
> +
> +	*(uint32_t*)map = 0xdead;
> +	gem_set_tiling(fd, mmap_handle, 2, 512 * 4);
> +	munmap(map, PAGE_SIZE);
> +
> +	for (i=0; i<num_handles; i++)
> +		gem_close(fd, handle[i]);
> +
> +	return 0;
> +}
> +
> +static int test_map_fixed_partial_overlap(int fd)
> +{
> +	void *ptr;
> +	void *map;
> +	uint32_t handle;
> +	uint32_t mmap_handle;
> +	struct drm_i915_gem_mmap_gtt mmap_arg;
> +	struct drm_i915_gem_set_domain set_domain;
> +
> +	igt_assert(posix_memalign(&ptr, PAGE_SIZE, sizeof(linear)) == 0);
> +	handle = create_userptr(fd, 0, ptr);
> +	copy(fd, handle, handle, 0);
> +
> +	mmap_handle = gem_create(fd, PAGE_SIZE);
> +
> +	memset(&mmap_arg, 0, sizeof(mmap_arg));
> +	mmap_arg.handle = mmap_handle;
> +	do_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP_GTT, &mmap_arg);
> +	map = mmap(ptr, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED,
> +			fd, mmap_arg.offset);
> +	igt_assert(map != MAP_FAILED);
> +
> +	*(uint32_t*)map = 0xdead;
> +
> +	memset(&set_domain, 0, sizeof(set_domain));
> +	set_domain.handle = handle;
> +	set_domain.read_domains = I915_GEM_DOMAIN_GTT;
> +	set_domain.write_domain = I915_GEM_DOMAIN_GTT;
> +	igt_assert((drmIoctl(fd, DRM_IOCTL_I915_GEM_SET_DOMAIN, &set_domain) != 0) &&
> +			errno == EFAULT);
> +
> +	free(ptr);
> +	munmap(map, PAGE_SIZE);
> +
> +	gem_close(fd, handle);
> +	gem_close(fd, mmap_handle);
> +
> +	return 0;
> +}
> +
>   static int test_forbidden_ops(int fd)
>   {
>   	struct drm_i915_gem_pread gem_pread;
> @@ -1489,6 +1563,15 @@ int main(int argc, char **argv)
>   	igt_subtest("stress-mm-invalidate-close-overlap")
>   		test_invalidate_close_race(fd, true);
>
> +	igt_subtest("map-fixed-invalidate")
> +		test_map_fixed_invalidate(fd, false);
> +
> +	igt_subtest("map-fixed-invalidate-overlap")
> +		test_map_fixed_invalidate(fd, true);
> +
> +	igt_subtest("map-fixed-partial-overlap")
> +		test_map_fixed_partial_overlap(fd);
> +

It is a bit confusing how overlap means two different things between 
subtests. In one it is two userptr objects that are overlapping and in 
another it is the mmap of a normal GEM bo overlapping the userptr range. 
I mean, in all subtests mmap always overlap the userptr.

Also, should there be a subtest which mmaps the userptr bo itself with 
MAP_FIXED, to the same or overlapping range?


Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

  parent reply	other threads:[~2015-06-29 14:01 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-06-29 10:59 [PATCH] tests/gem_userptr_blits: subtests for MAP_FIXED mappings of regular bo Michał Winiarski
2015-06-29 11:09 ` [PATCH] drm/i915: Fix userptr deadlock with MAP_FIXED Chris Wilson
2015-06-29 11:17   ` [PATCH v2] " Chris Wilson
2015-06-29 15:57     ` Michał Winiarski
2015-06-29 15:57       ` Michał Winiarski
2015-06-30 14:52       ` Tvrtko Ursulin
2015-06-30 14:52         ` [Intel-gfx] " Tvrtko Ursulin
2015-06-30 15:31         ` Chris Wilson
2015-06-30 15:47           ` Chris Wilson
2015-06-29 14:01 ` Tvrtko Ursulin [this message]
2015-06-29 14:07   ` [PATCH] tests/gem_userptr_blits: subtests for MAP_FIXED mappings of regular bo Chris Wilson
2015-06-29 14:15     ` Tvrtko Ursulin
2015-06-29 14:25       ` Chris Wilson
2015-06-29 14:56         ` Tvrtko Ursulin
2015-06-29 15:03           ` Chris Wilson
2015-06-30 15:01 ` [PATCH v2] tests/gem_userptr_blits: subtests for MAP_FIXED mappings of regular BO Michał Winiarski
2015-06-30 16:55   ` [PATCH 1/3] drm/i915: Only update the current userptr worker Chris Wilson
2015-06-30 16:55     ` [PATCH 2/3] drm/i915: Fix userptr deadlock with MAP_FIXED Chris Wilson
2015-07-01 11:14       ` Tvrtko Ursulin
2015-07-01 11:14         ` [Intel-gfx] " Tvrtko Ursulin
2015-07-01 11:29         ` Chris Wilson
2015-06-30 16:55     ` [PATCH 3/3] drm/i915: Use a task to cancel the userptr on invalidate_range Chris Wilson
2015-07-01 12:56       ` Tvrtko Ursulin
2015-07-03 11:00         ` Chris Wilson
2015-07-02 16:40       ` shuang.he
2015-07-03 11:03       ` [PATCH] " Chris Wilson
2015-07-01  9:48     ` [PATCH 1/3] drm/i915: Only update the current userptr worker Tvrtko Ursulin
2015-07-01  9:59       ` Chris Wilson
2015-07-01 10:58         ` Tvrtko Ursulin
2015-07-01 11:09           ` Chris Wilson
2015-07-01 12:26             ` Tvrtko Ursulin
2015-07-01 13:11               ` Chris Wilson
2015-07-03 10:48     ` Michał Winiarski
2015-07-03 10:53       ` Chris Wilson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=55914FA8.4060305@linux.intel.com \
    --to=tvrtko.ursulin@linux.intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=michal.winiarski@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.