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

Submitted by Yu, Xiangliang on Dec. 17, 2016, 4:16 p.m.

Details

Message ID 1481991405-30422-10-git-send-email-Xiangliang.Yu@amd.com
State New
Headers show
Series "转发: [PATCH 07/23] drm/amdgpu: create new directory for GPU virtualization" ( rev: 3 2 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Yu, Xiangliang Dec. 17, 2016, 4:16 p.m.
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(-)

Patch hide | download patch | download mbox

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);
+		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},

Comments

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.

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
agree with Alex on that, the patch looks overhead on the ip blocks cherry-picking logic, we could keep it as simple as original style,


as long as we know current device is a VF device, we shall know how many ip blocks need be cherry-picked and assembled together


BR Monk

________________________________
发件人: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> 代表 Alex Deucher <alexdeucher@gmail.com>
发送时间: 2016年12月20日 7:17:26
收件人: Yu, Xiangliang
抄送: dl.SRDC_SW_GPUVirtualization; amd-gfx list
主题: 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.

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
> -----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;

> 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
> -----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
> -----Original Message-----

> From: Deucher, Alexander

> Sent: Tuesday, December 20, 2016 11:53 PM

> To: Yu, Xiangliang <Xiangliang.Yu@amd.com>; Alex Deucher

> <alexdeucher@gmail.com>

> Cc: dl.SRDC_SW_GPUVirtualization

> <dl.SRDC_SW_GPUVirtualization@amd.com>; amd-gfx list <amd-

> gfx@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@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.


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.

> > >

> > > > +               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
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@amd.com>; Alex Deucher
>> <alexdeucher@gmail.com>
>> Cc: dl.SRDC_SW_GPUVirtualization
>> <dl.SRDC_SW_GPUVirtualization@amd.com>; amd-gfx list <amd-
>> gfx@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@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.
> 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