[PATCH] drm/amdgpu: add the accelerator pcie class

[PATCH] drm/amdgpu: add the accelerator pcie class

[Shiwu Zhang]

v2: add the base class id for accelerator (lijo)

Signed-off-by: Shiwu Zhang <shiwu.zhang@amd.com>
Acked-by: Lijo Lazar <lijo.lazar@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 5 +++++
 include/linux/pci_ids.h                 | 3 +++
 2 files changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 3d91e123f9bd..5d652e6f0b1e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2042,6 +2042,11 @@ static const struct pci_device_id pciidlist[] = {
 	  .class_mask = 0xffffff,
 	  .driver_data = CHIP_IP_DISCOVERY },
 
+	{ PCI_DEVICE(0x1002, PCI_ANY_ID),
+	  .class = PCI_CLASS_ACCELERATOR_PROCESSING << 8,
+	  .class_mask = 0xffffff,
+	  .driver_data = CHIP_IP_DISCOVERY },
+
 	{0, 0, 0}
 };
 
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index b362d90eb9b0..4918ff26a987 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -151,6 +151,9 @@
 #define PCI_CLASS_SP_DPIO		0x1100
 #define PCI_CLASS_SP_OTHER		0x1180
 
+#define PCI_BASE_CLASS_ACCELERATOR	0x12
+#define PCI_CLASS_ACCELERATOR_PROCESSING	0x1200
+
 #define PCI_CLASS_OTHERS		0xff
 
 /* Vendors and devices.  Sort key: vendor first, device next. */
-- 
2.17.1

RE: [PATCH] drm/amdgpu: add the accelerator pcie class

[Zhang, Morris]

[AMD Official Use Only - General]

Hi Bjorn,

Can we merge the below change through amdgpu tree ?  Thanks!

--Brs,
Morris Zhang
MLSE Linux  ML SRDC
Ext. 25147

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Shiwu
> Zhang
> Sent: Tuesday, May 23, 2023 12:03 PM
> To: amd-gfx@lists.freedesktop.org; linux-pci@vger.kernel.org;
> bhelgaas@google.com
> Subject: [PATCH] drm/amdgpu: add the accelerator pcie class
>
> v2: add the base class id for accelerator (lijo)
>
> Signed-off-by: Shiwu Zhang <shiwu.zhang@amd.com>
> Acked-by: Lijo Lazar <lijo.lazar@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 5 +++++
>  include/linux/pci_ids.h                 | 3 +++
>  2 files changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 3d91e123f9bd..5d652e6f0b1e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2042,6 +2042,11 @@ static const struct pci_device_id pciidlist[] = {
>         .class_mask = 0xffffff,
>         .driver_data = CHIP_IP_DISCOVERY },
>
> +     { PCI_DEVICE(0x1002, PCI_ANY_ID),
> +       .class = PCI_CLASS_ACCELERATOR_PROCESSING << 8,
> +       .class_mask = 0xffffff,
> +       .driver_data = CHIP_IP_DISCOVERY },
> +
>       {0, 0, 0}
>  };
>
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h index
> b362d90eb9b0..4918ff26a987 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -151,6 +151,9 @@
>  #define PCI_CLASS_SP_DPIO            0x1100
>  #define PCI_CLASS_SP_OTHER           0x1180
>
> +#define PCI_BASE_CLASS_ACCELERATOR   0x12
> +#define PCI_CLASS_ACCELERATOR_PROCESSING     0x1200
> +
>  #define PCI_CLASS_OTHERS             0xff
>
>  /* Vendors and devices.  Sort key: vendor first, device next. */
> --
> 2.17.1

Re: [PATCH] drm/amdgpu: add the accelerator pcie class

[Christoph Hellwig]

On Tue, May 23, 2023 at 12:02:32PM +0800, Shiwu Zhang wrote:
> +	{ PCI_DEVICE(0x1002, PCI_ANY_ID),
> +	  .class = PCI_CLASS_ACCELERATOR_PROCESSING << 8,
> +	  .class_mask = 0xffffff,
> +	  .driver_data = CHIP_IP_DISCOVERY },

Probing for every single device of a given class for a single vendor
to a driver is just fundamentaly wrong.  Please list the actual IDs
that the driver can handle.

Re: [PATCH] drm/amdgpu: add the accelerator pcie class

[Christoph Hellwig]

On Tue, May 23, 2023 at 12:02:32PM +0800, Shiwu Zhang wrote:
> +	{ PCI_DEVICE(0x1002, PCI_ANY_ID),
> +	  .class = PCI_CLASS_ACCELERATOR_PROCESSING << 8,
> +	  .class_mask = 0xffffff,
> +	  .driver_data = CHIP_IP_DISCOVERY },

Probing for every single device of a given class for a single vendor
to a driver is just fundamentaly wrong.  Please list the actual IDs
that the driver can handle.

Re: [PATCH] drm/amdgpu: add the accelerator pcie class

[Alex Deucher]

On Tue, May 23, 2023 at 5:25 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, May 23, 2023 at 12:02:32PM +0800, Shiwu Zhang wrote:
> > +     { PCI_DEVICE(0x1002, PCI_ANY_ID),
> > +       .class = PCI_CLASS_ACCELERATOR_PROCESSING << 8,
> > +       .class_mask = 0xffffff,
> > +       .driver_data = CHIP_IP_DISCOVERY },
>
> Probing for every single device of a given class for a single vendor
> to a driver is just fundamentaly wrong.  Please list the actual IDs
> that the driver can handle.

How so?  The driver handles all devices of that class.  We already do
that for PCI_CLASS_DISPLAY_VGA and PCI_CLASS_DISPLAY_OTHER.  Other
drivers do similar things.  The hda audio driver does the same thing
for PCI_CLASS_MULTIMEDIA_HD_AUDIO for example.

Alex

Re: [PATCH] drm/amdgpu: add the accelerator pcie class

[Alex Deucher]

On Tue, May 23, 2023 at 5:25 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, May 23, 2023 at 12:02:32PM +0800, Shiwu Zhang wrote:
> > +     { PCI_DEVICE(0x1002, PCI_ANY_ID),
> > +       .class = PCI_CLASS_ACCELERATOR_PROCESSING << 8,
> > +       .class_mask = 0xffffff,
> > +       .driver_data = CHIP_IP_DISCOVERY },
>
> Probing for every single device of a given class for a single vendor
> to a driver is just fundamentaly wrong.  Please list the actual IDs
> that the driver can handle.

How so?  The driver handles all devices of that class.  We already do
that for PCI_CLASS_DISPLAY_VGA and PCI_CLASS_DISPLAY_OTHER.  Other
drivers do similar things.  The hda audio driver does the same thing
for PCI_CLASS_MULTIMEDIA_HD_AUDIO for example.

Alex

Re: [PATCH] drm/amdgpu: add the accelerator pcie class

[Christoph Hellwig]

On Tue, May 23, 2023 at 10:02:32AM -0400, Alex Deucher wrote:
> On Tue, May 23, 2023 at 5:25 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Tue, May 23, 2023 at 12:02:32PM +0800, Shiwu Zhang wrote:
> > > +     { PCI_DEVICE(0x1002, PCI_ANY_ID),
> > > +       .class = PCI_CLASS_ACCELERATOR_PROCESSING << 8,
> > > +       .class_mask = 0xffffff,
> > > +       .driver_data = CHIP_IP_DISCOVERY },
> >
> > Probing for every single device of a given class for a single vendor
> > to a driver is just fundamentaly wrong.  Please list the actual IDs
> > that the driver can handle.
> 
> How so?  The driver handles all devices of that class.  We already do
> that for PCI_CLASS_DISPLAY_VGA and PCI_CLASS_DISPLAY_OTHER.  Other
> drivers do similar things.

How is that going to work in the long run?  The chances of totally
incompatbile devices from the same vendor appearing is absolutely given.

> The hda audio driver does the same thing
> for PCI_CLASS_MULTIMEDIA_HD_AUDIO for example.
>

That, just like PCI_CLASS_STORAGE_EXPRESS is a different case, as
the class is associated with an actual documented programming interface.

Re: [PATCH] drm/amdgpu: add the accelerator pcie class

[Christoph Hellwig]

On Tue, May 23, 2023 at 10:02:32AM -0400, Alex Deucher wrote:
> On Tue, May 23, 2023 at 5:25 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Tue, May 23, 2023 at 12:02:32PM +0800, Shiwu Zhang wrote:
> > > +     { PCI_DEVICE(0x1002, PCI_ANY_ID),
> > > +       .class = PCI_CLASS_ACCELERATOR_PROCESSING << 8,
> > > +       .class_mask = 0xffffff,
> > > +       .driver_data = CHIP_IP_DISCOVERY },
> >
> > Probing for every single device of a given class for a single vendor
> > to a driver is just fundamentaly wrong.  Please list the actual IDs
> > that the driver can handle.
> 
> How so?  The driver handles all devices of that class.  We already do
> that for PCI_CLASS_DISPLAY_VGA and PCI_CLASS_DISPLAY_OTHER.  Other
> drivers do similar things.

How is that going to work in the long run?  The chances of totally
incompatbile devices from the same vendor appearing is absolutely given.

> The hda audio driver does the same thing
> for PCI_CLASS_MULTIMEDIA_HD_AUDIO for example.
>

That, just like PCI_CLASS_STORAGE_EXPRESS is a different case, as
the class is associated with an actual documented programming interface.

RE: [PATCH] drm/amdgpu: add the accelerator pcie class

[Deucher, Alexander]

[AMD Official Use Only - General]

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Christoph Hellwig
> Sent: Thursday, May 25, 2023 5:47 AM
> To: Alex Deucher <alexdeucher@gmail.com>
> Cc: Christoph Hellwig <hch@infradead.org>; bhelgaas@google.com; amd-
> gfx@lists.freedesktop.org; Zhang, Morris <Shiwu.Zhang@amd.com>; linux-
> pci@vger.kernel.org
> Subject: Re: [PATCH] drm/amdgpu: add the accelerator pcie class
> 
> On Tue, May 23, 2023 at 10:02:32AM -0400, Alex Deucher wrote:
> > On Tue, May 23, 2023 at 5:25 AM Christoph Hellwig <hch@infradead.org>
> wrote:
> > >
> > > On Tue, May 23, 2023 at 12:02:32PM +0800, Shiwu Zhang wrote:
> > > > +     { PCI_DEVICE(0x1002, PCI_ANY_ID),
> > > > +       .class = PCI_CLASS_ACCELERATOR_PROCESSING << 8,
> > > > +       .class_mask = 0xffffff,
> > > > +       .driver_data = CHIP_IP_DISCOVERY },
> > >
> > > Probing for every single device of a given class for a single vendor
> > > to a driver is just fundamentaly wrong.  Please list the actual IDs
> > > that the driver can handle.
> >
> > How so?  The driver handles all devices of that class.  We already do
> > that for PCI_CLASS_DISPLAY_VGA and PCI_CLASS_DISPLAY_OTHER.  Other
> > drivers do similar things.
> 
> How is that going to work in the long run?  The chances of totally
> incompatbile devices from the same vendor appearing is absolutely given.
> 

We already handle this today for CLASS_DISPLAY via a data table provided on our hardware that details the components on the board.  The driver can then determine whether or not that combination of components is supported.  If the data table doesn't exist or isn’t parse-able, or the components enumerated are not supported, the driver doesn't load.

Alex

RE: [PATCH] drm/amdgpu: add the accelerator pcie class

[Deucher, Alexander]

[AMD Official Use Only - General]

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Christoph Hellwig
> Sent: Thursday, May 25, 2023 5:47 AM
> To: Alex Deucher <alexdeucher@gmail.com>
> Cc: Christoph Hellwig <hch@infradead.org>; bhelgaas@google.com; amd-
> gfx@lists.freedesktop.org; Zhang, Morris <Shiwu.Zhang@amd.com>; linux-
> pci@vger.kernel.org
> Subject: Re: [PATCH] drm/amdgpu: add the accelerator pcie class
> 
> On Tue, May 23, 2023 at 10:02:32AM -0400, Alex Deucher wrote:
> > On Tue, May 23, 2023 at 5:25 AM Christoph Hellwig <hch@infradead.org>
> wrote:
> > >
> > > On Tue, May 23, 2023 at 12:02:32PM +0800, Shiwu Zhang wrote:
> > > > +     { PCI_DEVICE(0x1002, PCI_ANY_ID),
> > > > +       .class = PCI_CLASS_ACCELERATOR_PROCESSING << 8,
> > > > +       .class_mask = 0xffffff,
> > > > +       .driver_data = CHIP_IP_DISCOVERY },
> > >
> > > Probing for every single device of a given class for a single vendor
> > > to a driver is just fundamentaly wrong.  Please list the actual IDs
> > > that the driver can handle.
> >
> > How so?  The driver handles all devices of that class.  We already do
> > that for PCI_CLASS_DISPLAY_VGA and PCI_CLASS_DISPLAY_OTHER.  Other
> > drivers do similar things.
> 
> How is that going to work in the long run?  The chances of totally
> incompatbile devices from the same vendor appearing is absolutely given.
> 

We already handle this today for CLASS_DISPLAY via a data table provided on our hardware that details the components on the board.  The driver can then determine whether or not that combination of components is supported.  If the data table doesn't exist or isn’t parse-able, or the components enumerated are not supported, the driver doesn't load.

Alex

Re: [PATCH] drm/amdgpu: add the accelerator pcie class

[Christoph Hellwig]

On Thu, May 25, 2023 at 08:52:06PM +0000, Deucher, Alexander wrote:
> We already handle this today for CLASS_DISPLAY via a data table provided on our hardware that details the components on the board.  The driver can then determine whether or not that combination of components is supported.  If the data table doesn't exist or isn’t parse-able, or the components enumerated are not supported, the driver doesn't load.

But things like module loading and initramfs generation still work
off the ID table and not your internal tables.

Re: [PATCH] drm/amdgpu: add the accelerator pcie class

[Christoph Hellwig]

On Thu, May 25, 2023 at 08:52:06PM +0000, Deucher, Alexander wrote:
> We already handle this today for CLASS_DISPLAY via a data table provided on our hardware that details the components on the board.  The driver can then determine whether or not that combination of components is supported.  If the data table doesn't exist or isn’t parse-able, or the components enumerated are not supported, the driver doesn't load.

But things like module loading and initramfs generation still work
off the ID table and not your internal tables.

RE: [PATCH] drm/amdgpu: add the accelerator pcie class

[Deucher, Alexander]

[Public]

> -----Original Message-----
> From: Christoph Hellwig <hch@infradead.org>
> Sent: Friday, May 26, 2023 2:55 AM
> To: Deucher, Alexander <Alexander.Deucher@amd.com>
> Cc: Christoph Hellwig <hch@infradead.org>; Alex Deucher
> <alexdeucher@gmail.com>; bhelgaas@google.com; amd-
> gfx@lists.freedesktop.org; Zhang, Morris <Shiwu.Zhang@amd.com>; linux-
> pci@vger.kernel.org
> Subject: Re: [PATCH] drm/amdgpu: add the accelerator pcie class
>
> On Thu, May 25, 2023 at 08:52:06PM +0000, Deucher, Alexander wrote:
> > We already handle this today for CLASS_DISPLAY via a data table provided on
> our hardware that details the components on the board.  The driver can then
> determine whether or not that combination of components is supported.  If
> the data table doesn't exist or isn’t parse-able, or the components
> enumerated are not supported, the driver doesn't load.
>
> But things like module loading and initramfs generation still work off the ID
> table and not your internal tables.

Sure, and everything still works fine.  If the device is too new and the driver doesn't have support for it, it doesn't bind and returns -ENODEV when it probes the device.

Alex

RE: [PATCH] drm/amdgpu: add the accelerator pcie class

[Deucher, Alexander]

[Public]

> -----Original Message-----
> From: Christoph Hellwig <hch@infradead.org>
> Sent: Friday, May 26, 2023 2:55 AM
> To: Deucher, Alexander <Alexander.Deucher@amd.com>
> Cc: Christoph Hellwig <hch@infradead.org>; Alex Deucher
> <alexdeucher@gmail.com>; bhelgaas@google.com; amd-
> gfx@lists.freedesktop.org; Zhang, Morris <Shiwu.Zhang@amd.com>; linux-
> pci@vger.kernel.org
> Subject: Re: [PATCH] drm/amdgpu: add the accelerator pcie class
>
> On Thu, May 25, 2023 at 08:52:06PM +0000, Deucher, Alexander wrote:
> > We already handle this today for CLASS_DISPLAY via a data table provided on
> our hardware that details the components on the board.  The driver can then
> determine whether or not that combination of components is supported.  If
> the data table doesn't exist or isn’t parse-able, or the components
> enumerated are not supported, the driver doesn't load.
>
> But things like module loading and initramfs generation still work off the ID
> table and not your internal tables.

Sure, and everything still works fine.  If the device is too new and the driver doesn't have support for it, it doesn't bind and returns -ENODEV when it probes the device.

Alex

Re: [PATCH] drm/amdgpu: add the accelerator pcie class

[Bjorn Helgaas]

s/pcie/PCIe/ in the subject.

On Tue, May 23, 2023 at 12:02:32PM +0800, Shiwu Zhang wrote:
> v2: add the base class id for accelerator (lijo)

Please include a commit log.  For PCI, the "v2: ..." stuff would go
below the "---" so it doesn't get included in the git commit.  I don't
know what the DRM convention is.

It's OK if the commit log repeats the subject.  The subject is like
the title of a story -- it's not the first sentence of the story.

Please include a spec citation for the PCI_BASE_CLASS_ACCELERATOR
values in the commit log.  I think it's something like "PCI Code and
ID Assignment, r1.9, sec 1, 1.19".

> Signed-off-by: Shiwu Zhang <shiwu.zhang@amd.com>
> Acked-by: Lijo Lazar <lijo.lazar@amd.com>

With the above:

Acked-by: Bjorn Helgaas <bhelgaas@google.com>  # pci_ids.h

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 5 +++++
>  include/linux/pci_ids.h                 | 3 +++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 3d91e123f9bd..5d652e6f0b1e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2042,6 +2042,11 @@ static const struct pci_device_id pciidlist[] = {
>  	  .class_mask = 0xffffff,
>  	  .driver_data = CHIP_IP_DISCOVERY },
>  
> +	{ PCI_DEVICE(0x1002, PCI_ANY_ID),
> +	  .class = PCI_CLASS_ACCELERATOR_PROCESSING << 8,
> +	  .class_mask = 0xffffff,
> +	  .driver_data = CHIP_IP_DISCOVERY },
> +
>  	{0, 0, 0}
>  };
>  
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index b362d90eb9b0..4918ff26a987 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -151,6 +151,9 @@
>  #define PCI_CLASS_SP_DPIO		0x1100
>  #define PCI_CLASS_SP_OTHER		0x1180
>  
> +#define PCI_BASE_CLASS_ACCELERATOR	0x12
> +#define PCI_CLASS_ACCELERATOR_PROCESSING	0x1200
> +
>  #define PCI_CLASS_OTHERS		0xff
>  
>  /* Vendors and devices.  Sort key: vendor first, device next. */
> -- 
> 2.17.1
> 

Re: [PATCH] drm/amdgpu: add the accelerator pcie class

[Bjorn Helgaas]

s/pcie/PCIe/ in the subject.

On Tue, May 23, 2023 at 12:02:32PM +0800, Shiwu Zhang wrote:
> v2: add the base class id for accelerator (lijo)

Please include a commit log.  For PCI, the "v2: ..." stuff would go
below the "---" so it doesn't get included in the git commit.  I don't
know what the DRM convention is.

It's OK if the commit log repeats the subject.  The subject is like
the title of a story -- it's not the first sentence of the story.

Please include a spec citation for the PCI_BASE_CLASS_ACCELERATOR
values in the commit log.  I think it's something like "PCI Code and
ID Assignment, r1.9, sec 1, 1.19".

> Signed-off-by: Shiwu Zhang <shiwu.zhang@amd.com>
> Acked-by: Lijo Lazar <lijo.lazar@amd.com>

With the above:

Acked-by: Bjorn Helgaas <bhelgaas@google.com>  # pci_ids.h

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 5 +++++
>  include/linux/pci_ids.h                 | 3 +++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 3d91e123f9bd..5d652e6f0b1e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2042,6 +2042,11 @@ static const struct pci_device_id pciidlist[] = {
>  	  .class_mask = 0xffffff,
>  	  .driver_data = CHIP_IP_DISCOVERY },
>  
> +	{ PCI_DEVICE(0x1002, PCI_ANY_ID),
> +	  .class = PCI_CLASS_ACCELERATOR_PROCESSING << 8,
> +	  .class_mask = 0xffffff,
> +	  .driver_data = CHIP_IP_DISCOVERY },
> +
>  	{0, 0, 0}
>  };
>  
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index b362d90eb9b0..4918ff26a987 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -151,6 +151,9 @@
>  #define PCI_CLASS_SP_DPIO		0x1100
>  #define PCI_CLASS_SP_OTHER		0x1180
>  
> +#define PCI_BASE_CLASS_ACCELERATOR	0x12
> +#define PCI_CLASS_ACCELERATOR_PROCESSING	0x1200
> +
>  #define PCI_CLASS_OTHERS		0xff
>  
>  /* Vendors and devices.  Sort key: vendor first, device next. */
> -- 
> 2.17.1
>