Am 21.12.2016 um 02:59 schrieb Yu, Xiangliang:
-----Original Message----- From: Deucher, Alexander Sent: Tuesday, December 20, 2016 11:53 PM To: Yu, Xiangliang <xiangliang...@amd.com>; Alex Deucher <alexdeuc...@gmail.com> Cc: dl.SRDC_SW_GPUVirtualization <dl.srdc_sw_gpuvirtualizat...@amd.com>; amd-gfx list <amd- g...@lists.freedesktop.org> Subject: RE: [PATCH 09/23] drm/amdgpu: enable virtualization feature for FIJI/TONGA-----Original Message----- From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf Of Yu, Xiangliang Sent: Tuesday, December 20, 2016 12:41 AM To: Alex Deucher Cc: dl.SRDC_SW_GPUVirtualization; amd-gfx list Subject: RE: [PATCH 09/23] drm/amdgpu: enable virtualization feature for FIJI/TONGA-----Original Message----- From: Alex Deucher [mailto:alexdeuc...@gmail.com] Sent: Tuesday, December 20, 2016 7:17 AM To: Yu, Xiangliang <xiangliang...@amd.com> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>; dl.SRDC_SW_GPUVirtualization<dl.srdc_sw_gpuvirtualizat...@amd.com>Subject: Re: [PATCH 09/23] drm/amdgpu: enable virtualization feature for FIJI/TONGA On Sat, Dec 17, 2016 at 11:16 AM, Xiangliang Yu <xiangliang...@amd.com> wrote:According to chip device id to set VF flag, and call virtual interface to setup all realted IP blocks. Signed-off-by: Xiangliang Yu <xiangliang...@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 ++++- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index c4075b7..ab8c8bb5 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -1285,7 +1285,10 @@ static int amdgpu_early_init(structamdgpu_device *adev)else adev->family = AMDGPU_FAMILY_VI; - r = vi_set_ip_blocks(adev); + if (adev->flags & AMD_IS_VF) + r = amd_xgpu_set_ip_blocks(adev);As far as I can see there's no need for a special amd_xgpu_set_ip_blocks() function. Just handle the VF case directly in vi_set_ip_blocks() and avoid all the extra indirection.My idea is that all virtualization chip share one common interface as a special chip family. It will bring some benefits: 1. Logic code is very clear; 2. Avoid to scatter virtualization code throughout all amdgpu components; 3. Easy to support next virtualization chip without change amdgpu code;I don't mind having a separate IP module for special VF related setup, but I think the differences in the list of IP modules is small enough that it doesn't warrant all of the redirection. Basically just: if (VF) { add_ip_module(A); add_ip_module(X); add_ip_module(C); } else { add_ip_module(A); add_ip_module(B); add_ip_module(C); add_ip_module(D); } That way it's obvious in one place which modules are present in the VF and bare metal cases without having to trace through a bunch of indirection. It also makes it easier to update the lists if we ever rework the ip module interface or add a new IP module or something like that.My point is we want to centrally manage all virtualization, like as power play component does. For you, the code is not big difference, but for us, easy to implement new features, support new chips, and add new setting for virtualization. Otherwise, there are lot of virtualization check in amdgpu and bring lot of effects to maintain virtualization code. You know, virtualization is very big feature for graphics. Please think about from what I'm standing.
Sorry, but I have to agree with Alex here. Powerplay is separate because it affects multiple components at the same time.
Virtualization doesn't justify at all having it a separate module like Powerplay. It's just an addition to the existing IP modules, not something completely new.
Additional to that it is likely that virtualization will change over time, so the separation like Alex suggest makes a lot of sense.
Regards, Christian.
+ else + r = vi_set_ip_blocks(adev); if (r) return r; break; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 93c4704..5a18111 100755 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -385,13 +385,13 @@ static const struct pci_device_id pciidlist[] = { {0x1002, 0x6928, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_TONGA}, {0x1002, 0x6929, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_TONGA}, {0x1002, 0x692B, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_TONGA}, - {0x1002, 0x692F, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_TONGA}, + {0x1002, 0x692F, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_TONGA + | AMD_IS_VF}, {0x1002, 0x6930, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_TONGA}, {0x1002, 0x6938, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_TONGA}, {0x1002, 0x6939, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_TONGA}, /* fiji */ {0x1002, 0x7300, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_FIJI}, - {0x1002, 0x730F, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_FIJI}, + {0x1002, 0x730F, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_FIJI | + AMD_IS_VF}, /* carrizo */ {0x1002, 0x9870, PCI_ANY_ID, PCI_ANY_ID, 0, 0,CHIP_CARRIZO|AMD_IS_APU},{0x1002, 0x9874, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_CARRIZO|AMD_IS_APU}, -- 2.7.4 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx_______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx_______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
