[PATCH 4/4] drm: add tests/amdgpu (v2)

[Emil Velikov]

Hi Alex,

On 24 April 2015 at 16:13, Alex Deucher <alexdeucher at 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 at 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