diff mbox

[4/4] igt/gem_stolen: Verify contents of stolen-backed objects across hibernation

Message ID 1449048293-4335-5-git-send-email-ankitprasad.r.sharma@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

ankitprasad.r.sharma@intel.com Dec. 2, 2015, 9:24 a.m. UTC 
From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>

This patch verifies if the contents of the stolen backed object were
preserved across hibernation. This is to validate kernel changes related
to moving stolen-backed objects to shmem on hibernation.

Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
---
 tests/gem_stolen.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

Comments

Tvrtko Ursulin Dec. 3, 2015, 12:14 p.m. UTC | #1 
Hi,

On 02/12/15 09:24, ankitprasad.r.sharma@intel.com wrote:
> From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
>
> This patch verifies if the contents of the stolen backed object were
> preserved across hibernation. This is to validate kernel changes related
> to moving stolen-backed objects to shmem on hibernation.
>
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>

Please Cc me if it is expected I keep reviewing this.

> ---
>   tests/gem_stolen.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 86 insertions(+)
>
> diff --git a/tests/gem_stolen.c b/tests/gem_stolen.c
> index 3374716..1f13fb0 100644
> --- a/tests/gem_stolen.c
> +++ b/tests/gem_stolen.c
> @@ -290,6 +290,89 @@ static void stolen_fill_purge_test(int fd)
>   		gem_close(fd, handle[i]);
>   }
>
> +static void stolen_hibernate(int fd)
> +{
> +	drm_intel_bo *bo;
> +	drm_intel_bo *src, *dest;
> +	int obj_count = 0, i = 0;
> +	int _ret = 0, j = 0;

Don't need to init _ret and j.

> +	uint32_t handle[MAX_OBJECTS], src_handle;
> +	uint32_t *virt;
> +
> +	gem_require_stolen_support(fd);
> +
> +	src_handle = gem_create(fd, SIZE);
> +	igt_assert(!src_handle);

gem_create already asserts this, but isn't it also reversed? Does it run 
and work?

> +	src = gem_handle_to_libdrm_bo(bufmgr, fd,
> +				     "bo", src_handle);
> +	igt_assert(src != NULL);
> +
> +	_ret = drm_intel_gem_bo_map_gtt(src);
> +	igt_assert(!_ret);

I think igt_assert_eq(ret, 0) is a preferred way nowadays.

Also a question, what is the special significance of _ret vs just normal 
ret in this test?

> +
> +	virt = src->virtual;
> +	for (j = 0; j < SIZE/DWORD_SIZE; j++)
> +		virt[j] = DATA;

memset?

> +
> +	drm_intel_bo_unmap(src);
> +	/* Exhaust Stolen space */
> +	do {
> +		handle[i] = __gem_create_stolen(fd, SIZE);
> +		if (handle[i] != 0) {

Maybe it would be more readable and with less indentation:

for (i = 0; i < MAX_OBJECTS; i++) {
	handle[i] = __gem_create_stolen(...);
	if (!handle[i])
		break;

	...

	obj_count++;
}

> +			bo = gem_handle_to_libdrm_bo(bufmgr, fd,
> +						     "verify_bo", handle[i]);
> +			igt_assert(bo != NULL);
> +			_ret = drm_intel_gem_bo_map_gtt(bo);
> +			igt_assert(!_ret);
> +
> +			virt = bo->virtual;
> +			for (j = 0; j < SIZE/DWORD_SIZE; j++)
> +				igt_assert(!virt[j]);

Again, people will probably want igt_assert_eq.

> +
> +			drm_intel_bo_unmap(bo);
> +			drm_intel_bo_unreference(bo);
> +
> +			obj_count++;
> +		}
> +
> +		i++;
> +	} while (handle[i-1] && i < MAX_OBJECTS);
> +
> +	igt_assert(obj_count > 0);
> +
> +	for (i = 0; i < obj_count; i++) {
> +		dest = gem_handle_to_libdrm_bo(bufmgr, fd,
> +					       "dst_bo", handle[i]);
> +		igt_assert(dest != NULL);
> +		intel_copy_bo(batch, dest, src, SIZE);
> +		drm_intel_bo_unreference(dest);
> +	}

Probably worth doing a verification step after each blit. So after 
resume is not the first time you are checking it.

> +	drm_intel_bo_unreference(src);
> +
> +	igt_system_hibernate_autoresume();
> +	/* Check if the object's memory contents are intact
> +	 * across hibernation.
> +	 */
> +	for (i = 0; i < obj_count; i++) {
> +		bo = gem_handle_to_libdrm_bo(bufmgr, fd,
> +					     "verify_bo", handle[i]);
> +		igt_assert(bo != NULL);
> +		_ret = drm_intel_gem_bo_map_gtt(bo);
> +		igt_assert(!_ret);
> +		virt = bo->virtual;
> +		for (j = 0; j < SIZE/DWORD_SIZE; j++)
> +			igt_assert_eq(virt[j], DATA);
> +
> +		drm_intel_bo_unmap(bo);
> +		drm_intel_bo_unreference(bo);
> +	}

Would it be interesting to fill the bos with some more elaborate pattern 
to potentially catch more potential corruption types?

Perhaps incrementing dwords would simply do?

> +	gem_close(fd, src_handle);
> +	for (i = 0; i < obj_count; i++)
> +		gem_close(fd, handle[i]);
> +}
> +
>   static void
>   stolen_no_mmap(int fd)
>   {
> @@ -353,6 +436,9 @@ igt_main
>   	igt_subtest("stolen-fill-purge")
>   		stolen_fill_purge_test(fd);
>
> +	igt_subtest("stolen-hibernate")
> +		stolen_hibernate(fd);
> +
>   	igt_fixture {
>   		intel_batchbuffer_free(batch);
>   		drm_intel_bufmgr_destroy(bufmgr);
>

Regards,

Tvrtko
Tvrtko Ursulin Dec. 4, 2015, 10:04 a.m. UTC | #2 
Hi,

One more thing below:

On 02/12/15 09:24, ankitprasad.r.sharma@intel.com wrote:
> From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
>
> This patch verifies if the contents of the stolen backed object were
> preserved across hibernation. This is to validate kernel changes related
> to moving stolen-backed objects to shmem on hibernation.
>
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> ---
>   tests/gem_stolen.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 86 insertions(+)
>
> diff --git a/tests/gem_stolen.c b/tests/gem_stolen.c
> index 3374716..1f13fb0 100644
> --- a/tests/gem_stolen.c
> +++ b/tests/gem_stolen.c
> @@ -290,6 +290,89 @@ static void stolen_fill_purge_test(int fd)
>   		gem_close(fd, handle[i]);
>   }
>
> +static void stolen_hibernate(int fd)
> +{
> +	drm_intel_bo *bo;
> +	drm_intel_bo *src, *dest;
> +	int obj_count = 0, i = 0;
> +	int _ret = 0, j = 0;
> +	uint32_t handle[MAX_OBJECTS], src_handle;
> +	uint32_t *virt;
> +
> +	gem_require_stolen_support(fd);
> +
> +	src_handle = gem_create(fd, SIZE);
> +	igt_assert(!src_handle);
> +	src = gem_handle_to_libdrm_bo(bufmgr, fd,
> +				     "bo", src_handle);
> +	igt_assert(src != NULL);
> +
> +	_ret = drm_intel_gem_bo_map_gtt(src);
> +	igt_assert(!_ret);
> +
> +	virt = src->virtual;
> +	for (j = 0; j < SIZE/DWORD_SIZE; j++)
> +		virt[j] = DATA;
> +
> +	drm_intel_bo_unmap(src);
> +	/* Exhaust Stolen space */
> +	do {
> +		handle[i] = __gem_create_stolen(fd, SIZE);
> +		if (handle[i] != 0) {
> +			bo = gem_handle_to_libdrm_bo(bufmgr, fd,
> +						     "verify_bo", handle[i]);
> +			igt_assert(bo != NULL);
> +			_ret = drm_intel_gem_bo_map_gtt(bo);
> +			igt_assert(!_ret);
> +
> +			virt = bo->virtual;
> +			for (j = 0; j < SIZE/DWORD_SIZE; j++)
> +				igt_assert(!virt[j]);
> +
> +			drm_intel_bo_unmap(bo);
> +			drm_intel_bo_unreference(bo);
> +
> +			obj_count++;
> +		}
> +
> +		i++;
> +	} while (handle[i-1] && i < MAX_OBJECTS);
> +
> +	igt_assert(obj_count > 0);

Would it make sense to require a bit more than one object?

Maybe considering all the platforms we could come up with a more 
demanding expectation on what does it mean to exhaust the stolen space?

Maybe you could even query stolen size and set the expectation based on 
that? Is that possible?

Regards,

Tvrtko

> +	for (i = 0; i < obj_count; i++) {
> +		dest = gem_handle_to_libdrm_bo(bufmgr, fd,
> +					       "dst_bo", handle[i]);
> +		igt_assert(dest != NULL);
> +		intel_copy_bo(batch, dest, src, SIZE);
> +		drm_intel_bo_unreference(dest);
> +	}
> +
> +	drm_intel_bo_unreference(src);
> +
> +	igt_system_hibernate_autoresume();
> +	/* Check if the object's memory contents are intact
> +	 * across hibernation.
> +	 */
> +	for (i = 0; i < obj_count; i++) {
> +		bo = gem_handle_to_libdrm_bo(bufmgr, fd,
> +					     "verify_bo", handle[i]);
> +		igt_assert(bo != NULL);
> +		_ret = drm_intel_gem_bo_map_gtt(bo);
> +		igt_assert(!_ret);
> +		virt = bo->virtual;
> +		for (j = 0; j < SIZE/DWORD_SIZE; j++)
> +			igt_assert_eq(virt[j], DATA);
> +
> +		drm_intel_bo_unmap(bo);
> +		drm_intel_bo_unreference(bo);
> +	}
> +
> +	gem_close(fd, src_handle);
> +	for (i = 0; i < obj_count; i++)
> +		gem_close(fd, handle[i]);
> +}
> +
>   static void
>   stolen_no_mmap(int fd)
>   {
> @@ -353,6 +436,9 @@ igt_main
>   	igt_subtest("stolen-fill-purge")
>   		stolen_fill_purge_test(fd);
>
> +	igt_subtest("stolen-hibernate")
> +		stolen_hibernate(fd);
> +
>   	igt_fixture {
>   		intel_batchbuffer_free(batch);
>   		drm_intel_bufmgr_destroy(bufmgr);
>
diff mbox

Patch

diff --git a/tests/gem_stolen.c b/tests/gem_stolen.c
index 3374716..1f13fb0 100644
--- a/tests/gem_stolen.c
+++ b/tests/gem_stolen.c
@@ -290,6 +290,89 @@  static void stolen_fill_purge_test(int fd)
 		gem_close(fd, handle[i]);
 }
 
+static void stolen_hibernate(int fd)
+{
+	drm_intel_bo *bo;
+	drm_intel_bo *src, *dest;
+	int obj_count = 0, i = 0;
+	int _ret = 0, j = 0;
+	uint32_t handle[MAX_OBJECTS], src_handle;
+	uint32_t *virt;
+
+	gem_require_stolen_support(fd);
+
+	src_handle = gem_create(fd, SIZE);
+	igt_assert(!src_handle);
+	src = gem_handle_to_libdrm_bo(bufmgr, fd,
+				     "bo", src_handle);
+	igt_assert(src != NULL);
+
+	_ret = drm_intel_gem_bo_map_gtt(src);
+	igt_assert(!_ret);
+
+	virt = src->virtual;
+	for (j = 0; j < SIZE/DWORD_SIZE; j++)
+		virt[j] = DATA;
+
+	drm_intel_bo_unmap(src);
+	/* Exhaust Stolen space */
+	do {
+		handle[i] = __gem_create_stolen(fd, SIZE);
+		if (handle[i] != 0) {
+			bo = gem_handle_to_libdrm_bo(bufmgr, fd,
+						     "verify_bo", handle[i]);
+			igt_assert(bo != NULL);
+			_ret = drm_intel_gem_bo_map_gtt(bo);
+			igt_assert(!_ret);
+
+			virt = bo->virtual;
+			for (j = 0; j < SIZE/DWORD_SIZE; j++)
+				igt_assert(!virt[j]);
+
+			drm_intel_bo_unmap(bo);
+			drm_intel_bo_unreference(bo);
+
+			obj_count++;
+		}
+
+		i++;
+	} while (handle[i-1] && i < MAX_OBJECTS);
+
+	igt_assert(obj_count > 0);
+
+	for (i = 0; i < obj_count; i++) {
+		dest = gem_handle_to_libdrm_bo(bufmgr, fd,
+					       "dst_bo", handle[i]);
+		igt_assert(dest != NULL);
+		intel_copy_bo(batch, dest, src, SIZE);
+		drm_intel_bo_unreference(dest);
+	}
+
+	drm_intel_bo_unreference(src);
+
+	igt_system_hibernate_autoresume();
+	/* Check if the object's memory contents are intact
+	 * across hibernation.
+	 */
+	for (i = 0; i < obj_count; i++) {
+		bo = gem_handle_to_libdrm_bo(bufmgr, fd,
+					     "verify_bo", handle[i]);
+		igt_assert(bo != NULL);
+		_ret = drm_intel_gem_bo_map_gtt(bo);
+		igt_assert(!_ret);
+		virt = bo->virtual;
+		for (j = 0; j < SIZE/DWORD_SIZE; j++)
+			igt_assert_eq(virt[j], DATA);
+
+		drm_intel_bo_unmap(bo);
+		drm_intel_bo_unreference(bo);
+	}
+
+	gem_close(fd, src_handle);
+	for (i = 0; i < obj_count; i++)
+		gem_close(fd, handle[i]);
+}
+
 static void
 stolen_no_mmap(int fd)
 {
@@ -353,6 +436,9 @@  igt_main
 	igt_subtest("stolen-fill-purge")
 		stolen_fill_purge_test(fd);
 
+	igt_subtest("stolen-hibernate")
+		stolen_hibernate(fd);
+
 	igt_fixture {
 		intel_batchbuffer_free(batch);
 		drm_intel_bufmgr_destroy(bufmgr);