On 20 January 2017 at 19:14, Xie, AlexBin <AlexBin.Xie at amd.com> wrote:
> 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.
>
Please avoid nodes[0] and use the symbolic name - nodes[DRM_NODE_PRIMARY].
Comment applies for any DRM_NODE_*.
>> + 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).
>
Indeed that's correct. It looks a bit strange to have it separate, but
not my call.
Unrelated:
Fwiw please can drop the legacy drmAvailable() from the test, if you
have a minute.
Thanks !
Emil
