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

[Emil Velikov]

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