[PATCH 09/23] drm/amdgpu: enable virtualization feature for FIJI/TONGA

Thu Dec 22 09:48:11 UTC 2016

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.Yu at amd.com>; Alex Deucher
>> <alexdeucher at gmail.com>
>> Cc: dl.SRDC_SW_GPUVirtualization
>> <dl.SRDC_SW_GPUVirtualization at amd.com>; amd-gfx list <amd-
>> gfx at lists.freedesktop.org>
>> Subject: RE: [PATCH 09/23] drm/amdgpu: enable virtualization feature for
>> FIJI/TONGA
>>
>>> -----Original Message-----
>>> From: amd-gfx [mailto:amd-gfx-bounces at 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:alexdeucher at gmail.com]
>>>> Sent: Tuesday, December 20, 2016 7:17 AM
>>>> To: Yu, Xiangliang <Xiangliang.Yu at amd.com>
>>>> Cc: amd-gfx list <amd-gfx at lists.freedesktop.org>;
>>>> dl.SRDC_SW_GPUVirtualization
>>> <dl.SRDC_SW_GPUVirtualization at 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.Yu at 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.Yu at 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(struct
>>>> amdgpu_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 at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx