From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from NAM10-BN7-obe.outbound.protection.outlook.com (mail-bn7nam10on2047.outbound.protection.outlook.com [40.107.92.47]) by gabe.freedesktop.org (Postfix) with ESMTPS id 6C60910E82B for ; Fri, 6 Jan 2023 02:55:57 +0000 (UTC) Message-ID: Date: Thu, 5 Jan 2023 21:55:43 -0500 Content-Language: en-US To: Kamil Konieczny , igt-dev@lists.freedesktop.org, Vitaly Prosyak , =?UTF-8?Q?Christian_K=c3=b6nig?= References: <20230105005320.157831-1-vitaly.prosyak@amd.com> <20230105005320.157831-2-vitaly.prosyak@amd.com> From: vitaly prosyak In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit MIME-Version: 1.0 Subject: Re: [igt-dev] [PATCH 2/4] lib/amdgpu: move function to another file List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: 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@amd.com wrote: >> From: Vitaly Prosyak >> >> 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 >> --- >> 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 >> + >> #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 >> -#include >> #include >> #include >> #include >> @@ -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 >> #include >> - >> -#define MAX_CARDS_SUPPORTED 4 >> +#include "amd_ip_blocks.h" >> >> struct amd_pci_unplug_setup { >> uint32_t major_version_req; >> -- >> 2.25.1 >>