Hi Bjorn,
I cleaned up the patch series[1] that Christian sent earlier to fix wrongly exported resizable bar capability dword by VBIOS of RX 5600 XT Pulse card.
I didn't split #2 patch instead merged amdgpu changes of #2 patch to #3 patch and removed changes related to pci_rebar_size_to_bytes() as it isn't needed anymore.
Apart from that I clean up rest of the patches as you suggested.
Please let me know if I missed something.
Regards, Nirmoy
[1] https://www.spinics.net/lists/linux-pci/msg103422.html
-- 2.29.2
From: Darren Salt devspam@moreofthesa.me.uk
Export pci_rebar_get_possible_sizes() for use by modular drivers.
Signed-off-by: Darren Salt devspam@moreofthesa.me.uk Signed-off-by: Nirmoy Das nirmoy.das@amd.com --- 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);
On Thu, Jan 07, 2021 at 06:50:14PM +0100, Nirmoy Das wrote:
From: Darren Salt devspam@moreofthesa.me.uk
Export pci_rebar_get_possible_sizes() for use by modular drivers.
Signed-off-by: Darren Salt devspam@moreofthesa.me.uk Signed-off-by: Nirmoy Das nirmoy.das@amd.com
Acked-by: Bjorn Helgaas bhelgaas@google.com
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); -- 2.29.2
Users of pci_resize_resource() need a way to calculate bar size from desired bytes. Add a helper function and export it so that modular drivers can use it.
Signed-off-by: Darren Salt devspam@moreofthesa.me.uk Signed-off-by: Christian König christian.koenig@amd.com Signed-off-by: Nirmoy Das nirmoy.das@amd.com --- drivers/pci/pci.c | 2 +- include/linux/pci.h | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-)
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/include/linux/pci.h b/include/linux/pci.h index 9999040cfad9..77fed01523e0 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1226,6 +1226,12 @@ 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 inline int pci_rebar_bytes_to_size(u64 bytes) +{ + bytes = roundup_pow_of_two(bytes); + return max(ilog2(bytes), 20) - 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);
On Thu, Jan 07, 2021 at 06:50:15PM +0100, Nirmoy Das wrote:
Users of pci_resize_resource() need a way to calculate bar size from desired bytes. Add a helper function and export it so that modular drivers can use it.
s/bar/BAR/
Signed-off-by: Darren Salt devspam@moreofthesa.me.uk Signed-off-by: Christian König christian.koenig@amd.com Signed-off-by: Nirmoy Das nirmoy.das@amd.com
Acked-by: Bjorn Helgaas bhelgaas@google.com
drivers/pci/pci.c | 2 +- include/linux/pci.h | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-)
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));diff --git a/include/linux/pci.h b/include/linux/pci.h index 9999040cfad9..77fed01523e0 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1226,6 +1226,12 @@ 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 inline int pci_rebar_bytes_to_size(u64 bytes) +{
- bytes = roundup_pow_of_two(bytes);
- return max(ilog2(bytes), 20) - 20;
This isn't returning a "size", is it? It looks like it's returning the log2 of the number of MB the BAR will be, i.e., the encoding used by the Resizable BAR Control register "BAR Size" field. Needs a brief comment to that effect and/or a different function name.
+}
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); -- 2.29.2
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.
Signed-off-by: Darren Salt devspam@moreofthesa.me.uk Signed-off-by: Christian König christian.koenig@amd.com Signed-off-by: Nirmoy Das nirmoy.das@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 dce0e66b2364..390f2cc13df7 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -1090,7 +1090,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; + int rbar_size = pci_rebar_bytes_to_size(adev->gmc.real_vram_size); struct pci_bus *root; struct resource *res; unsigned i; @@ -1121,6 +1121,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,
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.
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 Signed-off-by: Nirmoy Das nirmoy.das@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..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);
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
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.kerne...
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
dri-devel@lists.freedesktop.org
-
Bjorn Helgaas -
Christian König -
Nirmoy Das