| Message ID | 20210107175017.15893-5-nirmoy.das@amd.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | A PCI quirk for resizable BAR 0 on Navi10 | expand |
Am 07.01.21 um 22:32 schrieb Bjorn Helgaas: > On Thu, Jan 07, 2021 at 06:50:17PM +0100, Nirmoy Das wrote: >> RX 5600 XT Pulse advertises support for BAR0 being 256MB, 512MB, >> or 1GB, but it also supports 2GB, 4GB, and 8GB. Add a rebar >> size quirk so that CPU can fully access the BAR0. > This isn't quite accurate. The CPU can fully access BAR 0 no matter > what. I think the problem you're solving is that without this quirk, > BAR 0 isn't big enough to cover the VRAM. > >> Signed-off-by: Christian König <christian.koenig@amd.com> >> Reported-by: kernel test robot <lkp@intel.com> >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > IIRC, these Reported-by lines are from the "cap == 0x3f0" problem. It > would make sense to include these if this patch fixed that problem in > something that had already been merged. But this *hasn't* been > merged, so these lines only make sense to someone who trawls through > the mailing list to find the previous version. > > I don't really think it's worthwhile to include them. It's the same > as giving credit to reviewers, which we typically don't do except via > a Reviewed-by tag (which I think is too strong for this case) or a > "v2" changes note after the "---" line. That doesn't get included in > the git history, but is easily findable via the Link: tags as below. > > If you merge these via a non-PCI tree, please include the "Link:" > lines in the PCI patches, e.g., > > Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fr%2F20210107175017.15893-5-nirmoy.das%40amd.com&data=04%7C01%7Cchristian.koenig%40amd.com%7C33c14f5361e84d9d0e4908d8b353c412%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637456519678601616%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=wY9qaz3DTZA069qznMC9Wvoq9SZIzyJHE0XkaVXoqAc%3D&reserved=0 > > for this one. Obviously the link is different for each patch and will > change if you repost the series. > > I'm not sure why you put the amd patch in the middle of the series. > Seems like it would be slightly prettier and just as safe to put it at > the end. > >> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com> > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > Let me know if you want me to take the series. I will make the suggested changes and take this through the drm subsystem. That makes more sense since it only affects our hardware anyway. > >> --- >> drivers/pci/pci.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index 16216186b51c..b061bbd4afb1 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -3577,7 +3577,14 @@ u32 pci_rebar_get_possible_sizes(struct pci_dev *pdev, int bar) >> return 0; >> >> pci_read_config_dword(pdev, pos + PCI_REBAR_CAP, &cap); >> - return (cap & PCI_REBAR_CAP_SIZES) >> 4; >> + cap = (cap & PCI_REBAR_CAP_SIZES) >> 4; >> + >> + /* Sapphire RX 5600 XT Pulse has an invalid cap dword for BAR 0 */ >> + if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->device == 0x731f && >> + bar == 0 && cap == 0x700) >> + cap = 0x3f00; > I think this is structured wrong. It should be like this so it's > easier to match with the spec: > > cap &= PCI_REBAR_CAP_SIZES; > > if (... && cap == 0x7000) > cap = 0x3f000; > > return cap >> 4; Good point. Thanks, Christian. > >> + >> + return cap; >> } >> EXPORT_SYMBOL(pci_rebar_get_possible_sizes); >> >> -- >> 2.29.2 >>
On Thu, Jan 07, 2021 at 06:50:17PM +0100, Nirmoy Das wrote: > RX 5600 XT Pulse advertises support for BAR0 being 256MB, 512MB, > or 1GB, but it also supports 2GB, 4GB, and 8GB. Add a rebar > size quirk so that CPU can fully access the BAR0. This isn't quite accurate. The CPU can fully access BAR 0 no matter what. I think the problem you're solving is that without this quirk, BAR 0 isn't big enough to cover the VRAM. > Signed-off-by: Christian König <christian.koenig@amd.com> > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> IIRC, these Reported-by lines are from the "cap == 0x3f0" problem. It would make sense to include these if this patch fixed that problem in something that had already been merged. But this *hasn't* been merged, so these lines only make sense to someone who trawls through the mailing list to find the previous version. I don't really think it's worthwhile to include them. It's the same as giving credit to reviewers, which we typically don't do except via a Reviewed-by tag (which I think is too strong for this case) or a "v2" changes note after the "---" line. That doesn't get included in the git history, but is easily findable via the Link: tags as below. If you merge these via a non-PCI tree, please include the "Link:" lines in the PCI patches, e.g., Link: https://lore.kernel.org/r/20210107175017.15893-5-nirmoy.das@amd.com for this one. Obviously the link is different for each patch and will change if you repost the series. I'm not sure why you put the amd patch in the middle of the series. Seems like it would be slightly prettier and just as safe to put it at the end. > Signed-off-by: Nirmoy Das <nirmoy.das@amd.com> Acked-by: Bjorn Helgaas <bhelgaas@google.com> Let me know if you want me to take the series. > --- > drivers/pci/pci.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 16216186b51c..b061bbd4afb1 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -3577,7 +3577,14 @@ u32 pci_rebar_get_possible_sizes(struct pci_dev *pdev, int bar) > return 0; > > pci_read_config_dword(pdev, pos + PCI_REBAR_CAP, &cap); > - return (cap & PCI_REBAR_CAP_SIZES) >> 4; > + cap = (cap & PCI_REBAR_CAP_SIZES) >> 4; > + > + /* Sapphire RX 5600 XT Pulse has an invalid cap dword for BAR 0 */ > + if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->device == 0x731f && > + bar == 0 && cap == 0x700) > + cap = 0x3f00; I think this is structured wrong. It should be like this so it's easier to match with the spec: cap &= PCI_REBAR_CAP_SIZES; if (... && cap == 0x7000) cap = 0x3f000; return cap >> 4; > + > + return cap; > } > EXPORT_SYMBOL(pci_rebar_get_possible_sizes); > > -- > 2.29.2 >
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 16216186b51c..b061bbd4afb1 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -3577,7 +3577,14 @@ u32 pci_rebar_get_possible_sizes(struct pci_dev *pdev, int bar) return 0; pci_read_config_dword(pdev, pos + PCI_REBAR_CAP, &cap); - return (cap & PCI_REBAR_CAP_SIZES) >> 4; + cap = (cap & PCI_REBAR_CAP_SIZES) >> 4; + + /* Sapphire RX 5600 XT Pulse has an invalid cap dword for BAR 0 */ + if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->device == 0x731f && + bar == 0 && cap == 0x700) + cap = 0x3f00; + + return cap; } EXPORT_SYMBOL(pci_rebar_get_possible_sizes);