Hi Alex,
On 24 April 2015 at 16:13, Alex Deucher alexdeucher@gmail.com wrote:
This adds some basic unit tests for the new amdgpu driver.
v2: use common util_math.h
Can I put forward a few suggestions: - Can we use calloc over malloc. It will likely save you/others some headaches in the future. - Use capital letters for header guards - Annotate the CU_SuiteInfo/CU_TestInfo as static.
Signed-off-by: Alex Deucher alexander.deucher@amd.com
configure.ac | 23 ++ tests/Makefile.am | 6 + tests/amdgpu/Makefile.am | 24 ++ tests/amdgpu/amdgpu_test.c | 241 +++++++++++++ tests/amdgpu/amdgpu_test.h | 119 +++++++ tests/amdgpu/basic_tests.c | 676 ++++++++++++++++++++++++++++++++++++ tests/amdgpu/bo_tests.c | 151 ++++++++ tests/amdgpu/cs_tests.c | 319 +++++++++++++++++ tests/amdgpu/uvd_messages.h | 813 ++++++++++++++++++++++++++++++++++++++++++++ tests/kmstest/main.c | 1 + 10 files changed, 2373 insertions(+) create mode 100644 tests/amdgpu/Makefile.am create mode 100644 tests/amdgpu/amdgpu_test.c create mode 100644 tests/amdgpu/amdgpu_test.h create mode 100644 tests/amdgpu/basic_tests.c create mode 100644 tests/amdgpu/bo_tests.c create mode 100644 tests/amdgpu/cs_tests.c create mode 100644 tests/amdgpu/uvd_messages.h
diff --git a/tests/amdgpu/Makefile.am b/tests/amdgpu/Makefile.am new file mode 100644 index 0000000..ba7339d --- /dev/null +++ b/tests/amdgpu/Makefile.am @@ -0,0 +1,24 @@
+LDADD = $(top_builddir)/libdrm.la \
$(top_builddir)/amdgpu/libdrm_amdgpu.la
...
+amdgpu_test_LDFLAGS = $(CUNIT_LIBS)
LDFLAGS should not be used for LIBS (normally) - please fold it with LDADD above;
amdgpu_test_LDADD = \ $(top_builddir)/libdrm.la \ $(top_builddir)/amdgpu/libdrm_amdgpu.la \ $(CUNIT_LIBS)
+amdgpu_test_SOURCES = \
amdgpu_test.c \basic_tests.c \bo_tests.c \cs_tests.c
Please add the headers in the list. amdgpu_test.h uvd_messages.h
diff --git a/tests/amdgpu/amdgpu_test.c b/tests/amdgpu/amdgpu_test.c new file mode 100644 index 0000000..fc14b70 --- /dev/null +++ b/tests/amdgpu/amdgpu_test.c @@ -0,0 +1,241 @@
+int drm_amdgpu[MAX_CARDS_SUPPORTED];
We're using only a single one in the tests. Why the array ?
+int main(int argc, char **argv) +{
for (i = 0; i < MAX_CARDS_SUPPORTED; i++)drm_amdgpu[i] = 0;
0 is a valid fd number.
/* Try to open all possible radeon connections* Right now: Open only the 0.*/printf("Try to open the card 0..\n");drm_amdgpu[0] = open("/dev/dri/card0", O_RDWR | O_CLOEXEC);if (drm_amdgpu[0] == 1) {
1 is a valid fd number. Perhaps < 0 ?
/** Display version of DRM driver */drmVersionPtr retval;drm_version_t *version = drmMalloc(sizeof(*version));version->name_len = 0;version->name = NULL;version->date_len = 0;version->date = NULL;version->desc_len = 0;version->desc = NULL;if (drmIoctl(drm_amdgpu[0], DRM_IOCTL_VERSION, version)) {perror("Could not get DRM driver version\n");drmFree(version);exit(EXIT_FAILURE);}if (version->name_len)version->name = drmMalloc(version->name_len + 1);if (version->date_len)version->date = drmMalloc(version->date_len + 1);if (version->desc_len)version->desc = drmMalloc(version->desc_len + 1);if (drmIoctl(drm_amdgpu[0], DRM_IOCTL_VERSION, version)) {perror("Could not get information about DRM driver");drmFree(version);exit(EXIT_FAILURE);}/* The results might not be null-terminated strings. Add zero */if (version->name_len)version->name[version->name_len] = '\0';if (version->date_len)version->date[version->date_len] = '\0';if (version->desc_len)version->desc[version->desc_len] = '\0';printf("DRM Driver: Name: [%s] : Date [%s] : Description [%s]\n",version->name, version->date, version->desc);
Please use drmGetVersion/drmFreeVersion rather than open coding it. This implementation leaks memory.
drmFree(version);/* Initialize test suites to run *//* initialize the CUnit test registry */if (CUE_SUCCESS != CU_initialize_registry())
Swap CUE_SUCCESS and CU_initialize_registry() ?
return CU_get_error();
Close the fd before return/exit(). The rest of the could use it as well.
/* Register suites. */if (CU_register_suites(suites) != CUE_SUCCESS) {fprintf(stderr, "suite registration failed - %s\n",CU_get_error_msg());
Not familiar with CUnit, but looks like we should use CU_cleanup_registry() here. Same for the rest of the file ?
exit(EXIT_FAILURE);}
diff --git a/tests/amdgpu/basic_tests.c b/tests/amdgpu/basic_tests.c new file mode 100644 index 0000000..c53f6a0 --- /dev/null +++ b/tests/amdgpu/basic_tests.c
+#define SDMA_PKT_HEADER_op_offset 0 +#define SDMA_PKT_HEADER_op_mask 0x000000FF +#define SDMA_PKT_HEADER_op_shift 0 +#define SDMA_PKT_HEADER_OP(x) (((x) & SDMA_PKT_HEADER_op_mask) << SDMA_PKT_HEADER_op_shift)
Unused ?
+static void amdgpu_command_submission_sdma_const_fill(void) +{
pm4 = malloc(pm4_dw * 4);
pm4 = calloc(pm4_dw, sizeof(*pm4));
loop = 0;while(loop < 3) {
Bikeshed: Most of the patch uses for loops
+}
+static void amdgpu_command_submission_sdma_copy_linear(void) +{
Same suggestions as amdgpu_command_submission_sdma_const_fill
Cheers Emil