<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <style type="text/css" style="display:none;"></style> </head> <body dir="ltr"> <div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Arial,Helvetica,sans-serif;" dir="ltr"> Hi Emil, Thanks for the comments. Please see below. Regards, Alex Bin Xie <div style="color: rgb(0, 0, 0);"> <div> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="x_divRplyFwdMsg" dir="ltr">From: Emil Velikov <emil.l.velikov@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 <div> </div> </div> </div> <div class="PlainText">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@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@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 </div> </div> </div> </body> </html>