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

["Deucher, Alexander" <Alexander.Deucher-5C7GfCeVMHo@public.gmane.org>]

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@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@gmail.com]
> > Sent: Tuesday, December 20, 2016 7:17 AM
> > To: Yu, Xiangliang <Xiangliang.Yu@amd.com>
> > Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>;
> > dl.SRDC_SW_GPUVirtualization
> <dl.SRDC_SW_GPUVirtualization@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@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@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.

Alex

> > Alex
> >
> > > +               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