Hi Alex,
On 24 April 2015 at 16:13, Alex Deucher <alexdeucher@xxxxxxxxx> 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@xxxxxxx>
> ---
> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
