Hi Bjorn,
Darren stumbled over an AMD GPU with nonsense in it's resizeable BAR capability dword.
This is most likely fixable with a VBIOS update, but we already sold quite a bunch of those boards with the problem.
The driver still loads without this, but the performance isn't the best.
Do you have any objection to merge this through the drm branch?
Thanks in advance, Christian.
From: Darren Salt devspam@moreofthesa.me.uk
This is to assist driver modules which do BAR resizing.
Signed-off-by: Darren Salt devspam@moreofthesa.me.uk --- drivers/pci/pci.c | 1 + drivers/pci/pci.h | 1 - include/linux/pci.h | 1 + 3 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index e578d34095e9..ef80ed451415 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -3579,6 +3579,7 @@ u32 pci_rebar_get_possible_sizes(struct pci_dev *pdev, int bar) pci_read_config_dword(pdev, pos + PCI_REBAR_CAP, &cap); return (cap & PCI_REBAR_CAP_SIZES) >> 4; } +EXPORT_SYMBOL(pci_rebar_get_possible_sizes);
/** * pci_rebar_get_current_size - get the current size of a BAR diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index f86cae9aa1f4..640ae7d74fc3 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -608,7 +608,6 @@ int acpi_get_rc_resources(struct device *dev, const char *hid, u16 segment, struct resource *res); #endif
-u32 pci_rebar_get_possible_sizes(struct pci_dev *pdev, int bar); int pci_rebar_get_current_size(struct pci_dev *pdev, int bar); int pci_rebar_set_size(struct pci_dev *pdev, int bar, int size); static inline u64 pci_rebar_size_to_bytes(int size) diff --git a/include/linux/pci.h b/include/linux/pci.h index 22207a79762c..9999040cfad9 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1226,6 +1226,7 @@ void pci_update_resource(struct pci_dev *dev, int resno); int __must_check pci_assign_resource(struct pci_dev *dev, int i); int __must_check pci_reassign_resource(struct pci_dev *dev, int i, resource_size_t add_size, resource_size_t align); void pci_release_resource(struct pci_dev *dev, int resno); +u32 pci_rebar_get_possible_sizes(struct pci_dev *pdev, int bar); int __must_check pci_resize_resource(struct pci_dev *dev, int i, int size); int pci_select_bars(struct pci_dev *dev, unsigned long flags); bool pci_device_is_present(struct pci_dev *pdev);
From: Darren Salt devspam@moreofthesa.me.uk
This is to assist driver modules which do BAR resizing.
v2 (chk): Use ilog2 and make the new funtion extra defensive. Also use the new function on the two existing ocassions.
Signed-off-by: Darren Salt devspam@moreofthesa.me.uk Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 +-- drivers/pci/pci.c | 2 +- drivers/pci/pci.h | 4 ---- include/linux/pci.h | 12 ++++++++++++ 4 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index dce0e66b2364..70acd673e3f2 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -1089,8 +1089,7 @@ void amdgpu_device_wb_free(struct amdgpu_device *adev, u32 wb) */ int amdgpu_device_resize_fb_bar(struct amdgpu_device *adev) { - u64 space_needed = roundup_pow_of_two(adev->gmc.real_vram_size); - u32 rbar_size = order_base_2(((space_needed >> 20) | 1)) - 1; + u32 rbar_size = pci_rebar_bytes_to_size(adev->gmc.real_vram_size); struct pci_bus *root; struct resource *res; unsigned i; diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index ef80ed451415..16216186b51c 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1648,7 +1648,7 @@ static void pci_restore_rebar_state(struct pci_dev *pdev) pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl); bar_idx = ctrl & PCI_REBAR_CTRL_BAR_IDX; res = pdev->resource + bar_idx; - size = ilog2(resource_size(res)) - 20; + size = pci_rebar_bytes_to_size(resource_size(res)); ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE; ctrl |= size << PCI_REBAR_CTRL_BAR_SHIFT; pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl); diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 640ae7d74fc3..0fa31ff3d4e4 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -610,10 +610,6 @@ int acpi_get_rc_resources(struct device *dev, const char *hid, u16 segment,
int pci_rebar_get_current_size(struct pci_dev *pdev, int bar); int pci_rebar_set_size(struct pci_dev *pdev, int bar, int size); -static inline u64 pci_rebar_size_to_bytes(int size) -{ - return 1ULL << (size + 20); -}
struct device_node;
diff --git a/include/linux/pci.h b/include/linux/pci.h index 9999040cfad9..673606f871b7 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1226,6 +1226,18 @@ void pci_update_resource(struct pci_dev *dev, int resno); int __must_check pci_assign_resource(struct pci_dev *dev, int i); int __must_check pci_reassign_resource(struct pci_dev *dev, int i, resource_size_t add_size, resource_size_t align); void pci_release_resource(struct pci_dev *dev, int resno); + +static __always_inline int pci_rebar_bytes_to_size(u64 bytes) +{ + bytes = roundup_pow_of_two(bytes); + return max(ilog2(bytes), 20) - 20; +} + +static __always_inline u64 pci_rebar_size_to_bytes(int size) +{ + return 1ULL << (size + 20); +} + u32 pci_rebar_get_possible_sizes(struct pci_dev *pdev, int bar); int __must_check pci_resize_resource(struct pci_dev *dev, int i, int size); int pci_select_bars(struct pci_dev *dev, unsigned long flags);
From: Darren Salt devspam@moreofthesa.me.uk
This allows BAR0 resizing to be done for cards which don't advertise support for a size large enough to cover the VRAM but which do advertise at least one size larger than the default. For example, my RX 5600 XT, which advertises 256MB, 512MB and 1GB.
[v6] (chk) Reduce to only the necessary functionality
[v5] Drop the retry loop…
[v4] Use bit ops to find sizes to try.
[v3] Don't use pci_rebar_get_current_size().
[v2] Rewritten to use PCI helper functions; some extra log text.
Signed-off-by: Darren Salt devspam@moreofthesa.me.uk Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index 70acd673e3f2..da78746174f5 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -1089,7 +1089,7 @@ void amdgpu_device_wb_free(struct amdgpu_device *adev, u32 wb) */ int amdgpu_device_resize_fb_bar(struct amdgpu_device *adev) { - u32 rbar_size = pci_rebar_bytes_to_size(adev->gmc.real_vram_size); + int rbar_size = pci_rebar_bytes_to_size(adev->gmc.real_vram_size); struct pci_bus *root; struct resource *res; unsigned i; @@ -1120,6 +1120,10 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device *adev) if (!res) return 0;
+ /* Limit the BAR size to what is available */ + rbar_size = min(fls(pci_rebar_get_possible_sizes(adev->pdev, 0)) - 1, + rbar_size); + /* Disable memory decoding while we change the BAR addresses and size */ pci_read_config_word(adev->pdev, PCI_COMMAND, &cmd); pci_write_config_word(adev->pdev, PCI_COMMAND,
Otherwise the CPU can't fully access the BAR.
Signed-off-by: Christian König christian.koenig@amd.com --- 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..b66e4703c214 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 == 0x7f00; + + return cap; } EXPORT_SYMBOL(pci_rebar_get_possible_sizes);
On Tue, Jan 5, 2021 at 8:44 AM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Otherwise the CPU can't fully access the BAR.
Signed-off-by: Christian König christian.koenig@amd.com
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..b66e4703c214 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 == 0x7f00;
Perhaps you meant cap = 0x7f00?
return cap;} EXPORT_SYMBOL(pci_rebar_get_possible_sizes);
-- 2.25.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Am 05.01.21 um 17:11 schrieb Ilia Mirkin:
On Tue, Jan 5, 2021 at 8:44 AM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Otherwise the CPU can't fully access the BAR.
Signed-off-by: Christian König christian.koenig@amd.com
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..b66e4703c214 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 == 0x7f00;Perhaps you meant cap = 0x7f00?
Ups, indeed! Thanks for pointing that out.
Christian.
return cap;-- 2.25.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi "Christian,
I love your patch! Perhaps something to improve:
[auto build test WARNING on pci/next] [also build test WARNING on linus/master v5.11-rc2 next-20210104] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Christian-K-nig/pci-export-pci_reba... base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next config: arm64-randconfig-r006-20210105 (attached as .config) compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 5c951623bc8965fa1e89660f2f5f4a2944e4981a) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install arm64 cross compiling tool for clang build # apt-get install binutils-aarch64-linux-gnu # https://github.com/0day-ci/linux/commit/6838a45fc2394ec88455e1fb3998ac865a07... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Christian-K-nig/pci-export-pci_rebar_get_possible_sizes/20210105-224446 git checkout 6838a45fc2394ec88455e1fb3998ac865a077168 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
drivers/pci/pci.c:3611:7: warning: equality comparison result unused [-Wunused-comparison]
cap == 0x7f00; ~~~~^~~~~~~~~ drivers/pci/pci.c:3611:7: note: use '=' to turn this equality comparison into an assignment cap == 0x7f00; ^~ = 1 warning generated.
vim +3611 drivers/pci/pci.c
3587 3588 /** 3589 * pci_rebar_get_possible_sizes - get possible sizes for BAR 3590 * @pdev: PCI device 3591 * @bar: BAR to query 3592 * 3593 * Get the possible sizes of a resizable BAR as bitmask defined in the spec 3594 * (bit 0=1MB, bit 19=512GB). Returns 0 if BAR isn't resizable. 3595 */ 3596 u32 pci_rebar_get_possible_sizes(struct pci_dev *pdev, int bar) 3597 { 3598 int pos; 3599 u32 cap; 3600 3601 pos = pci_rebar_find_pos(pdev, bar); 3602 if (pos < 0) 3603 return 0; 3604 3605 pci_read_config_dword(pdev, pos + PCI_REBAR_CAP, &cap); 3606 cap = (cap & PCI_REBAR_CAP_SIZES) >> 4; 3607 3608 /* Sapphire RX 5600 XT Pulse has an invalid cap dword for BAR 0 */ 3609 if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->device == 0x731f && 3610 bar == 0 && cap == 0x700)
3611 cap == 0x7f00;
3612 3613 return cap; 3614 } 3615 EXPORT_SYMBOL(pci_rebar_get_possible_sizes); 3616
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Christian,
url: https://github.com/0day-ci/linux/commits/Christian-K-nig/pci-export-pci_reba... base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next config: x86_64-randconfig-m001-20210105 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com Reported-by: Dan Carpenter dan.carpenter@oracle.com
smatch warnings: drivers/pci/pci.c:3611 pci_rebar_get_possible_sizes() warn: statement has no effect 22
vim +3611 drivers/pci/pci.c
276b738deb5bf85 Christian König 2017-10-24 3596 u32 pci_rebar_get_possible_sizes(struct pci_dev *pdev, int bar) 276b738deb5bf85 Christian König 2017-10-24 3597 { 276b738deb5bf85 Christian König 2017-10-24 3598 int pos; 276b738deb5bf85 Christian König 2017-10-24 3599 u32 cap; 276b738deb5bf85 Christian König 2017-10-24 3600 276b738deb5bf85 Christian König 2017-10-24 3601 pos = pci_rebar_find_pos(pdev, bar); 276b738deb5bf85 Christian König 2017-10-24 3602 if (pos < 0) 276b738deb5bf85 Christian König 2017-10-24 3603 return 0; 276b738deb5bf85 Christian König 2017-10-24 3604 276b738deb5bf85 Christian König 2017-10-24 3605 pci_read_config_dword(pdev, pos + PCI_REBAR_CAP, &cap); 6838a45fc2394ec Christian König 2021-01-05 3606 cap = (cap & PCI_REBAR_CAP_SIZES) >> 4; 6838a45fc2394ec Christian König 2021-01-05 3607 6838a45fc2394ec Christian König 2021-01-05 3608 /* Sapphire RX 5600 XT Pulse has an invalid cap dword for BAR 0 */ 6838a45fc2394ec Christian König 2021-01-05 3609 if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->device == 0x731f && 6838a45fc2394ec Christian König 2021-01-05 3610 bar == 0 && cap == 0x700) 6838a45fc2394ec Christian König 2021-01-05 @3611 cap == 0x7f00;
== vs =.
6838a45fc2394ec Christian König 2021-01-05 3612 6838a45fc2394ec Christian König 2021-01-05 3613 return cap; 276b738deb5bf85 Christian König 2017-10-24 3614 }
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
_______________________________________________ kbuild mailing list -- kbuild@lists.01.org To unsubscribe send an email to kbuild-leave@lists.01.org
On Tue, Jan 05, 2021 at 02:44:00PM +0100, Christian König wrote:
Hi Bjorn,
Darren stumbled over an AMD GPU with nonsense in it's resizeable BAR capability dword.
This is most likely fixable with a VBIOS update, but we already sold quite a bunch of those boards with the problem.
The driver still loads without this, but the performance isn't the best.
Do you have any objection to merge this through the drm branch?
I'm OK with this in general, but please pay attention to your commit logs:
- Subject line prefix is "PCI: " (capitalized).
- First word of subject is capitalized ("Add ...", "Export ...").
- No period at end of subject ("4/4 ... XT Pulse.").
- No "v2" included in subject line ("PCI: Add BAR ... v2").
- Include parentheses after function names, e.g., "pci_rebar_get_possible_sizes()".
- Commit logs should say directly what the patch does using imperative mood, not just "to assist with X". E.g., "Export pci_rebar_get_possible_sizes() for use by modular drivers." This should make sense even without reading the subject line.
- No "v2" verbiage included in commit log (you can include it after the "---" line if you want). That verbiage in 2/4 also contains two typos.
- 2/4 does two things, so split it into two patches. Then the subject and commit logs will make more sense.
- Splitting 2/4 will also give you an opportunity to explain why we need to change "static inline" to "static __always_inline". If we don't need that change, don't change it.
If we *do* need it, that means we likely need it for many other things in include/linux/pci.h, and I would want to fix all those places at once in a separate patch.
- 4/4 should be more specific about what's wrong. Based on the code, this device advertises support for BAR 0 being 256MB, 512MB, or 1GB, but it actually supports 2GB, 4GB, 8GB, and 16GB as well. Please spell that out in the commit log. "Otherwise the CPU can't fully access the BAR" really doesn't tell me what the patch does.
dri-devel@lists.freedesktop.org
-
Bjorn Helgaas -
Christian König -
Dan Carpenter -
Ilia Mirkin -
kernel test robot