Hi Emil,
Thanks for the comments.
Please see below.
Regards,
Alex Bin Xie
________________________________
From: Emil Velikov <emil.l.velikov@xxxxxxxxx>
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>
