[igt-dev] [PATCH 2/4] lib/amdgpu: move function to another file
vitaly prosyak
vprosyak at amd.com
Fri Jan 6 02:55:43 UTC 2023
Hi Kamil,
Thanks for the review!
I believe all your suggestions are addressed :
https://patchwork.freedesktop.org/series/112465/
All 4 patches validated with ./scripts/checkpatch.pl
Thanks, Vitaly
On 2023-01-05 11:24, Kamil Konieczny wrote:
> Hi Vitaly,
>
> On 2023-01-04 at 19:53:18 -0500, vitaly.prosyak at amd.com wrote:
>> From: Vitaly Prosyak <vitaly.prosyak at amd.com>
>>
>> No functional change just cosmetic.
> Please do not mix in adding some debug printing with code moving.
>
>> Move function amdgpu_open_devices to the shared file
>> since other tests will also use it.
>>
>> Signed-off-by: Vitaly Prosyak <vitaly.prosyak at amd.com>
>> ---
>> lib/amdgpu/amd_ip_blocks.c | 94 ++++++++++++++++++++++++++++++++++---
>> lib/amdgpu/amd_ip_blocks.h | 5 ++
>> lib/amdgpu/amd_pci_unplug.c | 69 ---------------------------
>> lib/amdgpu/amd_pci_unplug.h | 3 +-
>> 4 files changed, 93 insertions(+), 78 deletions(-)
>>
>> diff --git a/lib/amdgpu/amd_ip_blocks.c b/lib/amdgpu/amd_ip_blocks.c
>> index 3331c40bd..ce1bddffc 100644
>> --- a/lib/amdgpu/amd_ip_blocks.c
>> +++ b/lib/amdgpu/amd_ip_blocks.c
>> @@ -22,6 +22,8 @@
>> *
>> *
>> */
>> +#include <fcntl.h>
>> +
>> #include "amd_memory.h"
>> #include "amd_ip_blocks.h"
>> #include "amd_PM4.h"
>> @@ -616,21 +618,30 @@ int setup_amdgpu_ip_blocks(uint32_t major, uint32_t minor, struct amdgpu_gpu_inf
>> igt_info("amdgpu: unknown (family_id, chip_external_rev): (%u, %u)\n",
>> amdinfo->family_id, amdinfo->chip_external_rev);
>> return -1;
>> + } else {
>> + igt_info("amdgpu: %s (family_id, chip_external_rev): (%u, %u)\n",
>> + info->name, amdinfo->family_id, amdinfo->chip_external_rev);
>> }
>>
>> - if (info->family >= CHIP_SIENNA_CICHLID)
>> + if (info->family >= CHIP_SIENNA_CICHLID) {
>> info->chip_class = GFX10_3;
>> - else if (info->family >= CHIP_NAVI10)
>> + igt_info("amdgpu: chip_class GFX10_3\n");
> --------------- ^
> Do you really want that at every function use ? Maybe better
> to use igt_debug and have separate function for printing ?
>
>> + } else if (info->family >= CHIP_NAVI10) {
>> info->chip_class = GFX10;
>> - else if (info->family >= CHIP_VEGA10)
>> + igt_info("amdgpu: chip_class GFX10\n");
>> + } else if (info->family >= CHIP_VEGA10) {
>> info->chip_class = GFX9;
>> - else if (info->family >= CHIP_TONGA)
>> + igt_info("amdgpu: chip_class GFX9\n");
>> + } else if (info->family >= CHIP_TONGA) {
>> info->chip_class = GFX8;
>> - else if (info->family >= CHIP_BONAIRE)
>> + igt_info("amdgpu: chip_class GFX8\n");
>> + } else if (info->family >= CHIP_BONAIRE) {
>> info->chip_class = GFX7;
>> - else if (info->family >= CHIP_TAHITI)
>> + igt_info("amdgpu: chip_class GFX7\n");
>> + } else if (info->family >= CHIP_TAHITI) {
>> info->chip_class = GFX6;
>> - else {
>> + igt_info("amdgpu: chip_class GFX6\n");
>> + } else {
>> igt_info("amdgpu: Unknown family.\n");
> Here you can print detailed info.
>
>> return -1;
>> }
>> @@ -663,3 +674,72 @@ int setup_amdgpu_ip_blocks(uint32_t major, uint32_t minor, struct amdgpu_gpu_inf
>>
>> return 0;
>> }
>> +
> Please write description of each new library function.
>
>> +int
>> +amdgpu_open_devices(bool open_render_node, int max_cards_supported, int drm_amdgpu_fds[])
>> +{
>> + drmDevicePtr devices[MAX_CARDS_SUPPORTED];
>> + int i;
>> + int drm_node;
>> + int amd_index = 0;
>> + int drm_count;
>> + int fd;
>> + drmVersionPtr version;
>> +
>> + for (i = 0; i < max_cards_supported && i < MAX_CARDS_SUPPORTED; i++)
>> + drm_amdgpu_fds[i] = -1;
>> +
>> + drm_count = drmGetDevices2(0, devices, MAX_CARDS_SUPPORTED);
>> +
>> + if (drm_count < 0) {
>> + fprintf(stderr, "drmGetDevices2() returned an error %d\n", drm_count);
> --------------- ^
> We have igt_info() and igt_debug() messages, please use them.
>
>> + return 0;
>> + }
>> +
>> + for (i = 0; i < drm_count; i++) {
>> + /* If this is not PCI device, skip*/
>> + if (devices[i]->bustype != DRM_BUS_PCI)
>> + continue;
>> +
>> + /* If this is not AMD GPU vender ID, skip*/
>> + if (devices[i]->deviceinfo.pci->vendor_id != 0x1002)
>> + continue;
>> +
>> + if (open_render_node)
>> + drm_node = DRM_NODE_RENDER;
>> + else
>> + drm_node = DRM_NODE_PRIMARY;
>> +
>> + fd = -1;
>> + if (devices[i]->available_nodes & 1 << drm_node)
>> + fd = open(
>> + devices[i]->nodes[drm_node],
>> + O_RDWR | O_CLOEXEC);
>> +
>> + /* This node is not available. */
>> + if (fd < 0) continue;
> --------------------------- ^
> This should be at line below.
>
>> +
>> + version = drmGetVersion(fd);
>> + if (!version) {
>> + fprintf(stderr, "Warning: Cannot get version for %s." "Error is %s\n",
> ---------------------------------------------------------------------------- ^^
> Did you mean putting this at line below ?
>
> Please use checkpatch script from linux kernel before sending
> patches, it can find some problems with formatting and spelling.
>
> If that was in original code then mention that corrections in
> patch message.
>
>> + devices[i]->nodes[drm_node], strerror(errno));
>> + close(fd);
>> + continue;
>> + }
>> +
>> + if (strcmp(version->name, "amdgpu")) {
>> + /* This is not AMDGPU driver, skip.*/
>> + drmFreeVersion(version);
>> + close(fd);
>> + continue;
>> + }
>> +
>> + drmFreeVersion(version);
>> +
>> + drm_amdgpu_fds[amd_index] = fd;
>> + amd_index++;
>> + }
>> +
>> + drmFreeDevices(devices, drm_count);
>> + return amd_index;
>> +}
>> diff --git a/lib/amdgpu/amd_ip_blocks.h b/lib/amdgpu/amd_ip_blocks.h
>> index 908aacde0..b3620c00f 100644
>> --- a/lib/amdgpu/amd_ip_blocks.h
>> +++ b/lib/amdgpu/amd_ip_blocks.h
>> @@ -27,6 +27,8 @@
>>
>> #include "amd_registers.h"
>>
>> +#define MAX_CARDS_SUPPORTED 4
>> +
>> enum amd_ip_block_type {
>> AMD_IP_GFX,
>> AMD_IP_COMPUTE,
>> @@ -136,4 +138,7 @@ struct amdgpu_cmd_base* get_cmd_base(void);
>>
>> void free_cmd_base(struct amdgpu_cmd_base *base);
>>
>> +int
>> +amdgpu_open_devices(bool open_render_node, int max_cards_supported, int drm_amdgpu_fds[]);
> ------------------------------------------------ ^^
> Two spaces, one is enough.
>
> Regards,
> Kamil
>
>> +
>> #endif
>> diff --git a/lib/amdgpu/amd_pci_unplug.c b/lib/amdgpu/amd_pci_unplug.c
>> index 28b3ae393..078398b5e 100644
>> --- a/lib/amdgpu/amd_pci_unplug.c
>> +++ b/lib/amdgpu/amd_pci_unplug.c
>> @@ -21,7 +21,6 @@
>> *
>> */
>> #include <linux/limits.h>
>> -#include <sys/types.h>
>> #include <fcntl.h>
>> #include <sys/stat.h>
>> #include <pthread.h>
>> @@ -35,74 +34,6 @@
>> #include "xalloc.h"
>> #include "amd_ip_blocks.h"
>>
>> -static int
>> -amdgpu_open_devices(bool open_render_node, int max_cards_supported, int drm_amdgpu_fds[])
>> -{
>> - drmDevicePtr devices[MAX_CARDS_SUPPORTED];
>> - int i;
>> - int drm_node;
>> - int amd_index = 0;
>> - int drm_count;
>> - int fd;
>> - drmVersionPtr version;
>> -
>> - for (i = 0; i < max_cards_supported && i < MAX_CARDS_SUPPORTED; i++)
>> - drm_amdgpu_fds[i] = -1;
>> -
>> - 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 PCI device, skip*/
>> - if (devices[i]->bustype != DRM_BUS_PCI)
>> - continue;
>> -
>> - /* If this is not AMD GPU vender ID, skip*/
>> - if (devices[i]->deviceinfo.pci->vendor_id != 0x1002)
>> - continue;
>> -
>> - if (open_render_node)
>> - drm_node = DRM_NODE_RENDER;
>> - else
>> - drm_node = DRM_NODE_PRIMARY;
>> -
>> - fd = -1;
>> - if (devices[i]->available_nodes & 1 << drm_node)
>> - fd = open(
>> - devices[i]->nodes[drm_node],
>> - O_RDWR | O_CLOEXEC);
>> -
>> - /* This node is not available. */
>> - if (fd < 0) continue;
>> -
>> - version = drmGetVersion(fd);
>> - if (!version) {
>> - fprintf(stderr, "Warning: Cannot get version for %s." "Error is %s\n",
>> - devices[i]->nodes[drm_node], strerror(errno));
>> - close(fd);
>> - continue;
>> - }
>> -
>> - if (strcmp(version->name, "amdgpu")) {
>> - /* This is not AMDGPU driver, skip.*/
>> - drmFreeVersion(version);
>> - close(fd);
>> - continue;
>> - }
>> -
>> - drmFreeVersion(version);
>> -
>> - drm_amdgpu_fds[amd_index] = fd;
>> - amd_index++;
>> - }
>> -
>> - drmFreeDevices(devices, drm_count);
>> - return amd_index;
>> -}
>> static bool
>> amdgpu_node_is_drm(int maj, int min)
>> {
>> diff --git a/lib/amdgpu/amd_pci_unplug.h b/lib/amdgpu/amd_pci_unplug.h
>> index 509b6ec4c..35d4dce3a 100644
>> --- a/lib/amdgpu/amd_pci_unplug.h
>> +++ b/lib/amdgpu/amd_pci_unplug.h
>> @@ -26,8 +26,7 @@
>>
>> #include <amdgpu.h>
>> #include <amdgpu_drm.h>
>> -
>> -#define MAX_CARDS_SUPPORTED 4
>> +#include "amd_ip_blocks.h"
>>
>> struct amd_pci_unplug_setup {
>> uint32_t major_version_req;
>> --
>> 2.25.1
>>
More information about the igt-dev
mailing list