[PATCH libdrm 1/3] amdgpu: verify the tested device

[Emil Velikov]

Hi Alex,

Things look a lot better imho. There's some ideas below, for the
future if you/others see value in them. Please do not worry about
those now.

On 24 January 2017 at 22:29, Alex Xie <AlexBin.Xie at amd.com> wrote:
> Verify the vender ID and driver name.
> Open all AMDGPU devices.
> Provide an option to open render node.
>
> Tested as root: PASS
> Tested as non-privileged user:
> All tests failed as expected
>
> v2: Return value in the ene of function amdgpu_open_devices.
>     Check the return value of amdgpu_open_devices.
>     amdgpu_test is not for USB device for the time being.
>     Get the name of node from function drmGetDevices2.
>     Drop the legacy drmAvailable() from the test.
>
> Signed-off-by: Alex Xie <AlexBin.Xie at amd.com>
> ---
>  tests/amdgpu/amdgpu_test.c | 145 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 115 insertions(+), 30 deletions(-)
>
> diff --git a/tests/amdgpu/amdgpu_test.c b/tests/amdgpu/amdgpu_test.c
> index 71f357c..d2b00d4 100644
> --- a/tests/amdgpu/amdgpu_test.c
> +++ b/tests/amdgpu/amdgpu_test.c
> @@ -115,6 +115,111 @@ static const char usage[] = "Usage: %s [-hl] [<-s <suite id>> [-t <test id>]]\n"
>  /** Specified options strings for getopt */
>  static const char options[]   = "hls:t:";
>
> +/* Open AMD devices.
> + * Return the number of AMD device openned.
> + */
> +static int amdgpu_open_devices(int open_render_node)
> +{
> +       drmDevicePtr devices[MAX_CARDS_SUPPORTED];
> +       int ret;
> +       int i;
> +       int drm_node;
> +       int amd_index = 0;
> +       int drm_count;
> +       int fd;
> +       drmVersionPtr version;
> +
> +       drm_count = drmGetDevices2(0, devices, MAX_CARDS_SUPPORTED);
> +

> +       for (i = 0; i < drm_count; i++) {
> +               /* If this is not PCI device, skip*/
> +               if (devices[i]->bustype != DRM_BUS_PCI)
> +                       continue;
> +
> +               /* If this is not AMD GPU vender ID, skip*/
> +               if (devices[i]->deviceinfo.pci->vendor_id != 0x1002)
> +                       continue;
> +
Once the filtering is done, it may be that only 2 of the
MAX_CARDS_SUPPORTED acquired fit the criteria.
One could fetch arbitrary large (all?) devices and then cap if/as needed.


> +
> +               /* This node is not available. */
> +               if (fd < 0) continue;
Normally continue should be on the next line.


> +/* Print AMD devices information */
> +static void amdgpu_print_devices()
> +{
> +       int i;
> +       for (i = 0; i < MAX_CARDS_SUPPORTED; i++)
> +               if (drm_amdgpu[i] >=0) {
> +                       /** Display version of DRM driver */
> +                       drmVersionPtr retval = drmGetVersion(drm_amdgpu[0]);
Since we've done this above one might as well store it and cleanups at
amdgpu_close_devices() time.
It comes more applicable as you use drmGetDevice2 to get the device
info with later patch(es). Might as well store all the info that we
fetch during amdgpu_open_devices() ?

Thanks
Emil