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

[Xie, AlexBin]

Hi Emil,


Thanks for the comments.


Please see below.


Regards,

Alex Bin Xie


________________________________
From: Emil Velikov <emil.l.velikov at gmail.com>
Sent: Friday, January 20, 2017 8:18 AM
To: Xie, AlexBin
Cc: amd-gfx mailing list
Subject: Re: [PATCH libdrm 1/3] amdgpu: verify the tested device

Hi Alex,

Thanks for doing this. There's a few nitpicks on top of what David and
Christian has spotted.

On 19 January 2017 at 22:53, 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
>
> Signed-off-by: Alex Xie <AlexBin.Xie at amd.com>
> ---
>  tests/amdgpu/amdgpu_test.c | 144 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 121 insertions(+), 23 deletions(-)
>
> diff --git a/tests/amdgpu/amdgpu_test.c b/tests/amdgpu/amdgpu_test.c
> index 71f357c..e42ef9d 100644
> --- a/tests/amdgpu/amdgpu_test.c
> +++ b/tests/amdgpu/amdgpu_test.c
> @@ -115,6 +115,119 @@ 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 j;
> +       int amd_index = 0;
> +       int drm_count;
> +       int fd;
> +       char *device_name;
> +       drmVersionPtr version;
> +
> +       drm_count = drmGetDevices2(0, devices, MAX_CARDS_SUPPORTED);
> +
> +       if (drm_count < 0) {
> +               fprintf(stderr,
> +                       "drmGetDevices2() returned an error %d\n",
> +                       drm_count);
> +               return 0;
> +       }
> +
> +       for (i = 0; i < drm_count; i++) {
> +               /* If this is not AMD GPU vender ID, skip*/
> +               if (devices[i]->bustype == DRM_BUS_PCI)
> +                       if (devices[i]->deviceinfo.pci->vendor_id != 0x1002)
> +                               continue;
> +
> +               for (j = 0; j < DRM_NODE_MAX; j++) {
> +                       if (devices[i]->available_nodes & 1 << j) {
> +                               fd = open(
> +                                       devices[i]->nodes[j],
> +                                       O_RDONLY | O_CLOEXEC,
> +                                       0);
> +                               if (fd < 0) continue;
> +                       }
You don't need to iterate over all the available nodes. Just fetch the
PRIMARY or RENDER based on open_render_node.
Note that a device can be missing some node types (say RENDER) so make
sure the available_nodes bitmask is set.

[Alex Bin Xie]:
That was my original design, but later I changed.
I knew this work for the current xf86drm.c. But is the nodes[0]  always primary?
I was afraid that this may be changed in future. I searched all drm tests. I did not see an example.

So amdgpu_test will be the first to access nodes like: open(devices[i]->nodes[DRM_NODE_PRIMARY]), for example.

> +                       if (open_render_node)
> +                               device_name = drmGetRenderDeviceNameFromFd(fd);
> +                       else
> +                               device_name = drmGetPrimaryDeviceNameFromFd(fd);
> +
> +                       close(fd);
> +
> +                       drm_amdgpu[amd_index] = open(device_name,
> +                                                       O_RDWR | O_CLOEXEC);
> +
> +                       if (drm_amdgpu[amd_index] >= 0)
> +                               amd_index++;
> +
> +                       free(device_name);
> +
With the above comment this becomes redundant.

> +                       /* We have open this device. Go to next device.*/
> +                       break;
> +               }
> +       }
> +
Here you want to initialise the remainder of drm_amdgpu[] (since
drm_count can be less than MAX_CARDS_SUPPORTED) with -1.
Otherwise you'll have fun experiences during close/print (below).

[Alex Bin Xie]: This initialization is done in existing main() function (not introduced this patch).

-Emil
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20170120/2e603109/attachment-0001.html>