| Message ID | 20230523040232.21756-1-shiwu.zhang@amd.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | drm/amdgpu: add the accelerator pcie class | expand |
[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
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.
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
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.
[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
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.
[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
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 >
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. */