[PATCH 0/7] amdgpu, pci: improved BAR resizing support

[PATCH 0/7] amdgpu, pci: improved BAR resizing support

[Darren Salt]

This patch series improves the existing BAR resizing support in amdgpu. By
default, it will attempt to resize BAR0 for each dGPU present to cover the
VRAM, falling back to smaller sizes if necessary, e.g. if there's not enough
address space remaining or support for the size is not advertised.

Basic boot-time (or module load time) options to control this resizing are
implemented: one to control whether resizing is done (and whether the
advertised BAR sizes are ignored) and one to control the maximum BAR size
(where the size would be increased). At present, these are coarse; they
apply to all dGPUs driven by amdgpu.

The override is to cope with GPU VBIOSes which don't properly advertise
supported BAR sizes. This should be quirked somehow; I have yet to determine
how this should be arranged.

Darren Salt (7):
  pci: export PCI BAR size-reading functions
  pci: add BAR bytes->size helper & expose size->bytes helper
  amdgpu: resize BAR0 to the maximum available size, even if it doesn't
    cover VRAM (v2)
  amdgpu: module option controlling whether BAR0 resizing is done
  amdgpu: limit maximum FB BAR size when attempting to enlarge
  pci: allow for overriding the list of advertised BAR sizes
  amdgpu: allow overriding of the GPU's list of supported BAR sizes

 drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  2 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 63 ++++++++++++++++++----
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    | 18 +++++++
 drivers/pci/pci.c                          |  2 +
 drivers/pci/pci.h                          |  6 ---
 drivers/pci/setup-res.c                    |  4 +-
 include/linux/pci.h                        | 15 +++++-
 7 files changed, 91 insertions(+), 19 deletions(-)

-- 
2.20.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/7] pci: export PCI BAR size-reading functions

[Darren Salt]

This is to assist driver modules which do BAR resizing.

Signed-off-by: Darren Salt <devspam@moreofthesa.me.uk>
---
 drivers/pci/pci.c   | 2 ++
 drivers/pci/pci.h   | 2 --
 include/linux/pci.h | 4 ++++
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e578d34095e9..3f6042d9ad83 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
@@ -3600,6 +3601,7 @@ int pci_rebar_get_current_size(struct pci_dev *pdev, int bar)
 	pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
 	return (ctrl & PCI_REBAR_CTRL_BAR_SIZE) >> PCI_REBAR_CTRL_BAR_SHIFT;
 }
+EXPORT_SYMBOL(pci_rebar_get_current_size);
 
 /**
  * pci_rebar_set_size - set a new size for a BAR
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index f86cae9aa1f4..8373d94414e9 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -608,8 +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..5aa035622741 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1226,7 +1226,11 @@ 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 pci_rebar_get_current_size(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);
 void pci_ignore_hotplug(struct pci_dev *dev);
-- 
2.20.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/7] pci: add BAR bytes->size helper & expose size->bytes helper

[Darren Salt]

This is to assist driver modules which do BAR resizing.

Signed-off-by: Darren Salt <devspam@moreofthesa.me.uk>
---
 drivers/pci/pci.h   | 4 ----
 include/linux/pci.h | 9 +++++++++
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 8373d94414e9..9a3837a30369 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -609,10 +609,6 @@ int acpi_get_rc_resources(struct device *dev, const char *hid, u16 segment,
 #endif
 
 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 5aa035622741..5eee18969fe8 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1227,6 +1227,15 @@ 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 order_base_2(((bytes >> 20) | 1)) - 1;
+}
+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 pci_rebar_get_current_size(struct pci_dev *pdev, int bar);
 int __must_check pci_resize_resource(struct pci_dev *dev, int i, int size);
-- 
2.20.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/7] amdgpu: resize BAR0 to the maximum available size, even if it doesn't cover VRAM (v2)

[Darren Salt]

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.

[v2] rewritten to use PCI helper functions; some extra log text.

Signed-off-by: Darren Salt <devspam@moreofthesa.me.uk>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 53 ++++++++++++++++++----
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 6637b84aeb85..1e99ca62a4d2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1106,21 +1106,24 @@ 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, current_size;
+	u32 available_sizes;
 	struct pci_bus *root;
 	struct resource *res;
 	unsigned i;
 	u16 cmd;
 	int r;
+	bool nospc = false;
 
 	/* Bypass for VF */
 	if (amdgpu_sriov_vf(adev))
 		return 0;
 
-	/* skip if the bios has already enabled large BAR */
-	if (adev->gmc.real_vram_size &&
-	    (pci_resource_len(adev->pdev, 0) >= adev->gmc.real_vram_size))
+	rbar_size = pci_rebar_bytes_to_size(adev->gmc.real_vram_size);
+	current_size = pci_rebar_get_current_size(adev->pdev, 0);
+
+	/* Skip if the BIOS has already enabled large BAR, covering the VRAM */
+	if (current_size >= rbar_size)
 		return 0;
 
 	/* Check if the root BUS has 64bit memory resources */
@@ -1138,6 +1141,14 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device *adev)
 	if (!res)
 		return 0;
 
+	available_sizes = pci_rebar_get_possible_sizes(adev->pdev, 0);
+	if (available_sizes == 0)
+		return 0;
+
+	dev_dbg(adev->dev, "BIOS-allocated BAR0 was %lluMB; trying to get %lluMB",
+	        current_size < 0 ? 0 : (pci_rebar_size_to_bytes(current_size) >> 20),
+	        pci_rebar_size_to_bytes(rbar_size) >> 20);
+
 	/* 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,
@@ -1150,11 +1161,33 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device *adev)
 
 	pci_release_resource(adev->pdev, 0);
 
-	r = pci_resize_resource(adev->pdev, 0, rbar_size);
-	if (r == -ENOSPC)
-		DRM_INFO("Not enough PCI address space for a large BAR.");
-	else if (r && r != -ENOTSUPP)
-		DRM_ERROR("Problem resizing BAR0 (%d).", r);
+	r = 0;
+	for (; rbar_size >= 0 && rbar_size > current_size; --rbar_size) {
+		/* Skip this size if it isn't advertised.
+		 * This avoids pci_resize_resources returning -EINVAL for that reason.
+		 */
+		if (!(available_sizes & BIT(rbar_size)))
+			continue;
+
+		r = pci_resize_resource(adev->pdev, 0, rbar_size);
+		if (r == 0) {
+			dev_dbg(adev->dev, "Succeeded in resizing to %lluMB.",
+			        pci_rebar_size_to_bytes(rbar_size) >> 20);
+			break;
+		} else if (r == -ENOTSUPP) {
+			dev_info(adev->dev, "BAR resizing not supported.");
+			break;
+		} else if (r == -ENOSPC) {
+			if (!nospc) {
+				/* Warn only the first time */
+				dev_info(adev->dev, "Not enough PCI address space for a large BAR.");
+				nospc = true;
+			}
+		} else {
+			dev_err(adev->dev, "Problem resizing BAR0 (%d).", r);
+			break;
+		}
+	}
 
 	pci_assign_unassigned_bus_resources(adev->pdev->bus);
 
-- 
2.20.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 4/7] amdgpu: module option controlling whether BAR0 resizing is done

[Darren Salt]

---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        | 1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    | 9 +++++++++
 3 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index c228e7470d51..2efce7fa6a4b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -201,6 +201,7 @@ static const bool __maybe_unused no_system_mem_limit;
 
 extern int amdgpu_tmz;
 extern int amdgpu_reset_method;
+extern bool amdgpu_resize_bar;
 
 #ifdef CONFIG_DRM_AMDGPU_SI
 extern int amdgpu_si_support;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 1e99ca62a4d2..292796e9f83d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1115,6 +1115,9 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device *adev)
 	int r;
 	bool nospc = false;
 
+	if (!amdgpu_resize_bar)
+		return 0;
+
 	/* Bypass for VF */
 	if (amdgpu_sriov_vf(adev))
 		return 0;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index cac2724e7615..6df33df74775 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -161,6 +161,7 @@ int amdgpu_force_asic_type = -1;
 int amdgpu_tmz;
 int amdgpu_reset_method = -1; /* auto */
 int amdgpu_num_kcq = -1;
+bool amdgpu_resize_bar = true;
 
 struct amdgpu_mgpu_info mgpu_info = {
 	.mutex = __MUTEX_INITIALIZER(mgpu_info.mutex),
@@ -807,6 +808,14 @@ module_param_named(bad_page_threshold, amdgpu_bad_page_threshold, int, 0444);
 MODULE_PARM_DESC(num_kcq, "number of kernel compute queue user want to setup (8 if set to greater than 8 or less than 0, only affect gfx 8+)");
 module_param_named(num_kcq, amdgpu_num_kcq, int, 0444);
 
+/**
+ * DOC: resize_bar (bool)
+ * Control whether FB BAR should be resized.
+ * Enabled by default.
+ */
+module_param_named(resize_bar, amdgpu_resize_bar, bool, 0444);
+MODULE_PARM_DESC(resize_bar, "Controls whether the FB BAR should be resized (default = true).");
+
 static const struct pci_device_id pciidlist[] = {
 #ifdef  CONFIG_DRM_AMDGPU_SI
 	{0x1002, 0x6780, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_TAHITI},
-- 
2.20.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 5/7] amdgpu: limit maximum FB BAR size when attempting to enlarge

[Darren Salt]

This is coarse, applying to all dGPUs.
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        | 1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 +++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    | 9 +++++++++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 2efce7fa6a4b..c844e2a8500a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -202,6 +202,7 @@ static const bool __maybe_unused no_system_mem_limit;
 extern int amdgpu_tmz;
 extern int amdgpu_reset_method;
 extern bool amdgpu_resize_bar;
+extern int amdgpu_max_bar_size;
 
 #ifdef CONFIG_DRM_AMDGPU_SI
 extern int amdgpu_si_support;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 292796e9f83d..b6c5ee490cbf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1125,7 +1125,13 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device *adev)
 	rbar_size = pci_rebar_bytes_to_size(adev->gmc.real_vram_size);
 	current_size = pci_rebar_get_current_size(adev->pdev, 0);
 
-	/* Skip if the BIOS has already enabled large BAR, covering the VRAM */
+	/* Skip if the BIOS has already enabled large BAR, covering the VRAM (or >= limit, if set) */
+	if (amdgpu_max_bar_size >= 0) {
+		const u32 max_size = max(amdgpu_max_bar_size, 8); /* clamp to min. 256MB */
+
+		if (rbar_size > max_size)
+			rbar_size = max_size;
+	}
 	if (current_size >= rbar_size)
 		return 0;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 6df33df74775..0542843c7d63 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -162,6 +162,7 @@ int amdgpu_tmz;
 int amdgpu_reset_method = -1; /* auto */
 int amdgpu_num_kcq = -1;
 bool amdgpu_resize_bar = true;
+int amdgpu_max_bar_size = -1;
 
 struct amdgpu_mgpu_info mgpu_info = {
 	.mutex = __MUTEX_INITIALIZER(mgpu_info.mutex),
@@ -816,6 +817,14 @@ module_param_named(num_kcq, amdgpu_num_kcq, int, 0444);
 module_param_named(resize_bar, amdgpu_resize_bar, bool, 0444);
 MODULE_PARM_DESC(resize_bar, "Controls whether the FB BAR should be resized (default = true).");
 
+/**
+ * DOC: max_bar_size (int)
+ * The maximum BAR size, in megabytes. Only affects BARs which the BIOS hasn't already made larger.
+ * Unlimited by default.
+ */
+module_param_named(max_bar_size, amdgpu_max_bar_size, int, 0444);
+MODULE_PARM_DESC(max_bar_size, "Maximum FB BAR size, log2(megabytes) (default = -1, meaning unlimited; minimum is 8 for 256MB).");
+
 static const struct pci_device_id pciidlist[] = {
 #ifdef  CONFIG_DRM_AMDGPU_SI
 	{0x1002, 0x6780, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_TAHITI},
-- 
2.20.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 6/7] pci: allow for overriding the list of advertised BAR sizes

[Darren Salt]

This is intended for devices which are known to work with BAR sizes other
than those which they advertise; usually larger.
---
 drivers/pci/setup-res.c | 4 ++--
 include/linux/pci.h     | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 43eda101fcf4..3651754de433 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -407,7 +407,7 @@ void pci_release_resource(struct pci_dev *dev, int resno)
 }
 EXPORT_SYMBOL(pci_release_resource);
 
-int pci_resize_resource(struct pci_dev *dev, int resno, int size)
+int pci_resize_resource(struct pci_dev *dev, int resno, int size, bool forced)
 {
 	struct resource *res = dev->resource + resno;
 	int old, ret;
@@ -426,7 +426,7 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size)
 	if (!sizes)
 		return -ENOTSUPP;
 
-	if (!(sizes & BIT(size)))
+	if (!forced && !(sizes & BIT(size)))
 		return -EINVAL;
 
 	old = pci_rebar_get_current_size(dev, resno);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 5eee18969fe8..f33494d92512 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1238,7 +1238,7 @@ static __always_inline u64 pci_rebar_size_to_bytes(int size)
 }
 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 __must_check pci_resize_resource(struct pci_dev *dev, int i, int size);
+int __must_check pci_resize_resource(struct pci_dev *dev, int i, int size, bool forced);
 
 int pci_select_bars(struct pci_dev *dev, unsigned long flags);
 bool pci_device_is_present(struct pci_dev *pdev);
-- 
2.20.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 7/7] amdgpu: allow overriding of the GPU's list of supported BAR sizes

[Darren Salt]

Some cards don't advertise a BAR size which covers all of the VRAM.

Mine, a Sapphire RX 5600 XT Pulse, advertises only 256MB, 512MB and 1GB.
Despite this, it works fine with the full 6GB visible via an 8GB BAR.
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        | 2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    | 8 ++++----
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index c844e2a8500a..a64a9ac92ac1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -201,7 +201,7 @@ static const bool __maybe_unused no_system_mem_limit;
 
 extern int amdgpu_tmz;
 extern int amdgpu_reset_method;
-extern bool amdgpu_resize_bar;
+extern int amdgpu_resize_bar;
 extern int amdgpu_max_bar_size;
 
 #ifdef CONFIG_DRM_AMDGPU_SI
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index b6c5ee490cbf..0f04686ed6c0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1114,6 +1114,7 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device *adev)
 	u16 cmd;
 	int r;
 	bool nospc = false;
+	const bool force = amdgpu_resize_bar == 2;
 
 	if (!amdgpu_resize_bar)
 		return 0;
@@ -1175,10 +1176,10 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device *adev)
 		/* Skip this size if it isn't advertised.
 		 * This avoids pci_resize_resources returning -EINVAL for that reason.
 		 */
-		if (!(available_sizes & BIT(rbar_size)))
+		if (!force && !(available_sizes & BIT(rbar_size)))
 			continue;
 
-		r = pci_resize_resource(adev->pdev, 0, rbar_size);
+		r = pci_resize_resource(adev->pdev, 0, rbar_size, force);
 		if (r == 0) {
 			dev_dbg(adev->dev, "Succeeded in resizing to %lluMB.",
 			        pci_rebar_size_to_bytes(rbar_size) >> 20);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 0542843c7d63..468ca3725890 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -161,7 +161,7 @@ int amdgpu_force_asic_type = -1;
 int amdgpu_tmz;
 int amdgpu_reset_method = -1; /* auto */
 int amdgpu_num_kcq = -1;
-bool amdgpu_resize_bar = true;
+int amdgpu_resize_bar = 1;
 int amdgpu_max_bar_size = -1;
 
 struct amdgpu_mgpu_info mgpu_info = {
@@ -810,12 +810,12 @@ MODULE_PARM_DESC(num_kcq, "number of kernel compute queue user want to setup (8
 module_param_named(num_kcq, amdgpu_num_kcq, int, 0444);
 
 /**
- * DOC: resize_bar (bool)
+ * DOC: resize_bar (int)
  * Control whether FB BAR should be resized.
  * Enabled by default.
  */
-module_param_named(resize_bar, amdgpu_resize_bar, bool, 0444);
-MODULE_PARM_DESC(resize_bar, "Controls whether the FB BAR should be resized (default = true).");
+module_param_named(resize_bar, amdgpu_resize_bar, int, 0444);
+MODULE_PARM_DESC(resize_bar, "Controls whether the FB BAR should be resized (0 = off, 1 = on (default), 2 = override the GPU's supported sizes).");
 
 /**
  * DOC: max_bar_size (int)
-- 
2.20.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/7] pci: export PCI BAR size-reading functions

[Christian König]

Am 11.12.20 um 01:55 schrieb Darren Salt:
> This is to assist driver modules which do BAR resizing.
>
> Signed-off-by: Darren Salt <devspam@moreofthesa.me.uk>
> ---
>   drivers/pci/pci.c   | 2 ++
>   drivers/pci/pci.h   | 2 --
>   include/linux/pci.h | 4 ++++
>   3 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e578d34095e9..3f6042d9ad83 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
> @@ -3600,6 +3601,7 @@ int pci_rebar_get_current_size(struct pci_dev *pdev, int bar)
>   	pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
>   	return (ctrl & PCI_REBAR_CTRL_BAR_SIZE) >> PCI_REBAR_CTRL_BAR_SHIFT;
>   }
> +EXPORT_SYMBOL(pci_rebar_get_current_size);

This is unnecessary. You can just look at the resource size instead 
which is also more defensive regarding problems/errors.

Christian.

>   
>   /**
>    * pci_rebar_set_size - set a new size for a BAR
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index f86cae9aa1f4..8373d94414e9 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -608,8 +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..5aa035622741 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1226,7 +1226,11 @@ 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 pci_rebar_get_current_size(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);
>   void pci_ignore_hotplug(struct pci_dev *dev);

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/7] amdgpu: module option controlling whether BAR0 resizing is done

[Christian König]

NAK, that is exactly what we are trying to avoid.

Am 11.12.20 um 01:55 schrieb Darren Salt:
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h        | 1 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 +++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    | 9 +++++++++
>   3 files changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index c228e7470d51..2efce7fa6a4b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -201,6 +201,7 @@ static const bool __maybe_unused no_system_mem_limit;
>   
>   extern int amdgpu_tmz;
>   extern int amdgpu_reset_method;
> +extern bool amdgpu_resize_bar;
>   
>   #ifdef CONFIG_DRM_AMDGPU_SI
>   extern int amdgpu_si_support;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 1e99ca62a4d2..292796e9f83d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1115,6 +1115,9 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device *adev)
>   	int r;
>   	bool nospc = false;
>   
> +	if (!amdgpu_resize_bar)
> +		return 0;
> +
>   	/* Bypass for VF */
>   	if (amdgpu_sriov_vf(adev))
>   		return 0;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index cac2724e7615..6df33df74775 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -161,6 +161,7 @@ int amdgpu_force_asic_type = -1;
>   int amdgpu_tmz;
>   int amdgpu_reset_method = -1; /* auto */
>   int amdgpu_num_kcq = -1;
> +bool amdgpu_resize_bar = true;
>   
>   struct amdgpu_mgpu_info mgpu_info = {
>   	.mutex = __MUTEX_INITIALIZER(mgpu_info.mutex),
> @@ -807,6 +808,14 @@ module_param_named(bad_page_threshold, amdgpu_bad_page_threshold, int, 0444);
>   MODULE_PARM_DESC(num_kcq, "number of kernel compute queue user want to setup (8 if set to greater than 8 or less than 0, only affect gfx 8+)");
>   module_param_named(num_kcq, amdgpu_num_kcq, int, 0444);
>   
> +/**
> + * DOC: resize_bar (bool)
> + * Control whether FB BAR should be resized.
> + * Enabled by default.
> + */
> +module_param_named(resize_bar, amdgpu_resize_bar, bool, 0444);
> +MODULE_PARM_DESC(resize_bar, "Controls whether the FB BAR should be resized (default = true).");
> +
>   static const struct pci_device_id pciidlist[] = {
>   #ifdef  CONFIG_DRM_AMDGPU_SI
>   	{0x1002, 0x6780, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_TAHITI},

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/7] amdgpu: resize BAR0 to the maximum available size, even if it doesn't cover VRAM (v2)

[Christian König]

Am 11.12.20 um 01:55 schrieb Darren Salt:
> 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.
>
> [v2] rewritten to use PCI helper functions; some extra log text.
>
> Signed-off-by: Darren Salt <devspam@moreofthesa.me.uk>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 53 ++++++++++++++++++----
>   1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 6637b84aeb85..1e99ca62a4d2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1106,21 +1106,24 @@ 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, current_size;
> +	u32 available_sizes;
>   	struct pci_bus *root;
>   	struct resource *res;
>   	unsigned i;
>   	u16 cmd;
>   	int r;
> +	bool nospc = false;
>   
>   	/* Bypass for VF */
>   	if (amdgpu_sriov_vf(adev))
>   		return 0;
>   
> -	/* skip if the bios has already enabled large BAR */
> -	if (adev->gmc.real_vram_size &&
> -	    (pci_resource_len(adev->pdev, 0) >= adev->gmc.real_vram_size))
> +	rbar_size = pci_rebar_bytes_to_size(adev->gmc.real_vram_size);
> +	current_size = pci_rebar_get_current_size(adev->pdev, 0);
> +
> +	/* Skip if the BIOS has already enabled large BAR, covering the VRAM */
> +	if (current_size >= rbar_size)

You should probably keep the comparison as it is and check the resource 
length against the VRAM size instead.

>   		return 0;
>   
>   	/* Check if the root BUS has 64bit memory resources */
> @@ -1138,6 +1141,14 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device *adev)
>   	if (!res)
>   		return 0;
>   
> +	available_sizes = pci_rebar_get_possible_sizes(adev->pdev, 0);
> +	if (available_sizes == 0)
> +		return 0;
> +
> +	dev_dbg(adev->dev, "BIOS-allocated BAR0 was %lluMB; trying to get %lluMB",
> +	        current_size < 0 ? 0 : (pci_rebar_size_to_bytes(current_size) >> 20),
> +	        pci_rebar_size_to_bytes(rbar_size) >> 20);

Please no extra debugging output, we spam syslog that enough with the 
existing resize.

> +
>   	/* 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,
> @@ -1150,11 +1161,33 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device *adev)
>   
>   	pci_release_resource(adev->pdev, 0);
>   
> -	r = pci_resize_resource(adev->pdev, 0, rbar_size);
> -	if (r == -ENOSPC)
> -		DRM_INFO("Not enough PCI address space for a large BAR.");
> -	else if (r && r != -ENOTSUPP)
> -		DRM_ERROR("Problem resizing BAR0 (%d).", r);
> +	r = 0;
> +	for (; rbar_size >= 0 && rbar_size > current_size; --rbar_size) {

Well exactly that try and error is a rather big NAK.

What you need to do instead is to look at the return value from 
pci_rebar_get_possible_sizes() and determine the size closed to the 
desired one.

E.g. when need a size of 13 is needed you first check if any bit >= 13 
are set. You can use the ffs() for this.

If that isn't the case use fls() to get the highest set bit < 13.

Regards,
Christian.

> +		/* Skip this size if it isn't advertised.
> +		 * This avoids pci_resize_resources returning -EINVAL for that reason.
> +		 */
> +		if (!(available_sizes & BIT(rbar_size)))
> +			continue;
> +
> +		r = pci_resize_resource(adev->pdev, 0, rbar_size);
> +		if (r == 0) {
> +			dev_dbg(adev->dev, "Succeeded in resizing to %lluMB.",
> +			        pci_rebar_size_to_bytes(rbar_size) >> 20);
> +			break;
> +		} else if (r == -ENOTSUPP) {
> +			dev_info(adev->dev, "BAR resizing not supported.");
> +			break;
> +		} else if (r == -ENOSPC) {
> +			if (!nospc) {
> +				/* Warn only the first time */
> +				dev_info(adev->dev, "Not enough PCI address space for a large BAR.");
> +				nospc = true;
> +			}
> +		} else {
> +			dev_err(adev->dev, "Problem resizing BAR0 (%d).", r);
> +			break;
> +		}
> +	}
>   
>   	pci_assign_unassigned_bus_resources(adev->pdev->bus);
>   

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/7] amdgpu: resize BAR0 to the maximum available size, even if it doesn't cover VRAM (v2)

[Darren Salt]

I demand that Christian König may or may not have written...

> Am 11.12.20 um 01:55 schrieb Darren Salt:
[snip]
>> +	rbar_size = pci_rebar_bytes_to_size(adev->gmc.real_vram_size);
>> +	current_size = pci_rebar_get_current_size(adev->pdev, 0);
>> +
>> +	/* Skip if the BIOS has already enabled large BAR, covering the VRAM */
>> +	if (current_size >= rbar_size)

> You should probably keep the comparison as it is and check the resource 
> length against the VRAM size instead.

Perhaps. I wonder, though, if I should do

    if (adev->gmc.real_vram_size == 0)
      return;

instead of the first part of the original condition.

[snip]
>> +	dev_dbg(adev->dev, "BIOS-allocated BAR0 was %lluMB; trying to get %lluMB",
>> +	        current_size < 0 ? 0 : (pci_rebar_size_to_bytes(current_size) >> 20),
>> +	        pci_rebar_size_to_bytes(rbar_size) >> 20);

> Please no extra debugging output, we spam syslog that enough with the 
> existing resize.

Okay, I'll dispose of that.
 
[snip]
>> -	r = pci_resize_resource(adev->pdev, 0, rbar_size);
>> -	if (r == -ENOSPC)
>> -		DRM_INFO("Not enough PCI address space for a large BAR.");
>> -	else if (r && r != -ENOTSUPP)
>> -		DRM_ERROR("Problem resizing BAR0 (%d).", r);
>> +	r = 0;
>> +	for (; rbar_size >= 0 && rbar_size > current_size; --rbar_size) {
>> +		/* Skip this size if it isn't advertised.
>> +		 * This avoids pci_resize_resources returning -EINVAL for that reason.
>> +		 */
>> +		if (!(available_sizes & BIT(rbar_size)))
>> +			continue;

> Well exactly that try and error is a rather big NAK.

> What you need to do instead is to look at the return value from
> pci_rebar_get_possible_sizes() and determine the size closed to the desired
> one. […]

Well… there's that rapid reject immediately following; and the override patch
alters that condition.

> E.g. when need a size of 13 is needed you first check if any bit >= 13 
> are set. You can use the ffs() for this.

So… find the lowest bit set, after masking out bits 0 to (rbar_size-1),
and try to re-allocate accordingly.

I could have it check for larger sizes if that fails, but I don't think that
it's worth it. If the BAR size is >= 2× the VRAM size, it's a waste of
address space; and the advertisement of such a size is arguably a VBIOS bug
anyway.

> If that isn't the case use fls() to get the highest set bit < 13.

That suggests that it'll be easiest to clear each bit after the corresponding
size is checked, I think. Also, this looks like it's adding complexity to
try to make rarely-executed code slightly faster in some cases (I can't see
it helping where available_sizes == 0x3F00, for example).

Incidentally, is it worth trying to reduce the BAR size at all? Thinking
mainly of two situations – limiting the maximum size, and the BIOS having
allocated one much too large.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/7] amdgpu: resize BAR0 to the maximum available size, even if it doesn't cover VRAM (v2)

[Alex Deucher]

On Fri, Dec 11, 2020 at 1:49 PM Darren Salt <devspam@moreofthesa.me.uk> wrote:
>
> I demand that Christian König may or may not have written...
>
> > Am 11.12.20 um 01:55 schrieb Darren Salt:
> [snip]
> >> +    rbar_size = pci_rebar_bytes_to_size(adev->gmc.real_vram_size);
> >> +    current_size = pci_rebar_get_current_size(adev->pdev, 0);
> >> +
> >> +    /* Skip if the BIOS has already enabled large BAR, covering the VRAM */
> >> +    if (current_size >= rbar_size)
>
> > You should probably keep the comparison as it is and check the resource
> > length against the VRAM size instead.
>
> Perhaps. I wonder, though, if I should do
>
>     if (adev->gmc.real_vram_size == 0)
>       return;
>
> instead of the first part of the original condition.
>
> [snip]
> >> +    dev_dbg(adev->dev, "BIOS-allocated BAR0 was %lluMB; trying to get %lluMB",
> >> +            current_size < 0 ? 0 : (pci_rebar_size_to_bytes(current_size) >> 20),
> >> +            pci_rebar_size_to_bytes(rbar_size) >> 20);
>
> > Please no extra debugging output, we spam syslog that enough with the
> > existing resize.
>
> Okay, I'll dispose of that.
>
> [snip]
> >> -    r = pci_resize_resource(adev->pdev, 0, rbar_size);
> >> -    if (r == -ENOSPC)
> >> -            DRM_INFO("Not enough PCI address space for a large BAR.");
> >> -    else if (r && r != -ENOTSUPP)
> >> -            DRM_ERROR("Problem resizing BAR0 (%d).", r);
> >> +    r = 0;
> >> +    for (; rbar_size >= 0 && rbar_size > current_size; --rbar_size) {
> >> +            /* Skip this size if it isn't advertised.
> >> +             * This avoids pci_resize_resources returning -EINVAL for that reason.
> >> +             */
> >> +            if (!(available_sizes & BIT(rbar_size)))
> >> +                    continue;
>
> > Well exactly that try and error is a rather big NAK.
>
> > What you need to do instead is to look at the return value from
> > pci_rebar_get_possible_sizes() and determine the size closed to the desired
> > one. […]
>
> Well… there's that rapid reject immediately following; and the override patch
> alters that condition.
>
> > E.g. when need a size of 13 is needed you first check if any bit >= 13
> > are set. You can use the ffs() for this.
>
> So… find the lowest bit set, after masking out bits 0 to (rbar_size-1),
> and try to re-allocate accordingly.
>
> I could have it check for larger sizes if that fails, but I don't think that
> it's worth it. If the BAR size is >= 2× the VRAM size, it's a waste of
> address space; and the advertisement of such a size is arguably a VBIOS bug
> anyway.
>
> > If that isn't the case use fls() to get the highest set bit < 13.
>
> That suggests that it'll be easiest to clear each bit after the corresponding
> size is checked, I think. Also, this looks like it's adding complexity to
> try to make rarely-executed code slightly faster in some cases (I can't see
> it helping where available_sizes == 0x3F00, for example).
>
> Incidentally, is it worth trying to reduce the BAR size at all? Thinking
> mainly of two situations – limiting the maximum size, and the BIOS having
> allocated one much too large.

In theory we could on resource constrained systems.  E.g., if you have
a lot of devices and a limited MMIO window, but I think on most recent
AMD GPUs, 256M is the smallest size and the default.

Alex
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/7] amdgpu: resize BAR0 to the maximum available size, even if it doesn't cover VRAM (v2)

[Christian König]

Am 11.12.20 um 18:31 schrieb Darren Salt:
> [SNIP]
>> If that isn't the case use fls() to get the highest set bit < 13.
> That suggests that it'll be easiest to clear each bit after the corresponding
> size is checked, I think.

Ok, well you don't seem to understand my constrain here: Never check 
more than one size!

Find the first valid size which is good for you and then try to resize 
to that one, if this doesn't work abort.

> Also, this looks like it's adding complexity to
> try to make rarely-executed code slightly faster in some cases (I can't see
> it helping where available_sizes == 0x3F00, for example).

The intention here is not to make the code faster, but to prevent issues 
on system where we probe multiple GPUs at once.

Regards,
Christian.

> Incidentally, is it worth trying to reduce the BAR size at all? Thinking
> mainly of two situations – limiting the maximum size, and the BIOS having
> allocated one much too large.
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/7] amdgpu: resize BAR0 to the maximum available size, even if it doesn't cover VRAM (v2)

[Darren Salt]

I demand that Christian König may or may not have written...

> Am 11.12.20 um 18:31 schrieb Darren Salt:
>< [SNIP]
>>> If that isn't the case use fls() to get the highest set bit < 13.
>> That suggests that it'll be easiest to clear each bit after the
>> corresponding size is checked, I think.

> Ok, well you don't seem to understand my constrain here: Never check 
> more than one size!

Checking context: it appeared to be the for() statement itself rather than
the whole loop. It didn't help that “try and error” looks like “try to error”
to me; I now think that you meant “trial and error”.

> Find the first valid size which is good for you and then try to resize 
> to that one, if this doesn't work abort.

So, basically, stop even when the next size down would be fine, e.g. 16GB →
8GB. Just as well that size limiting is done in a later patch, though I
wonder whether that should allow per-GPU configuration (not something for
this patch series, though).

>> Also, this looks like it's adding complexity to try to make
>> rarely-executed code slightly faster in some cases (I can't see it
>> helping where available_sizes == 0x3F00, for example).

> The intention here is not to make the code faster, but to prevent issues on
> system where we probe multiple GPUs at once.

Presumably simultaneous probes and random ordering…
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/7] amdgpu: resize BAR0 to the maximum available size, even if it doesn't cover VRAM (v2)

[Christian König]

Am 14.12.20 um 16:46 schrieb Darren Salt:
> I demand that Christian König may or may not have written...
>
>> Am 11.12.20 um 18:31 schrieb Darren Salt:
>> < [SNIP]
>>>> If that isn't the case use fls() to get the highest set bit < 13.
>>> That suggests that it'll be easiest to clear each bit after the
>>> corresponding size is checked, I think.
>> Ok, well you don't seem to understand my constrain here: Never check
>> more than one size!
> Checking context: it appeared to be the for() statement itself rather than
> the whole loop. It didn't help that “try and error” looks like “try to error”
> to me; I now think that you meant “trial and error”.

I'm not a native speaker of English, so bear with me if I get such stuff 
wrong.

>> Find the first valid size which is good for you and then try to resize
>> to that one, if this doesn't work abort.
> So, basically, stop even when the next size down would be fine, e.g. 16GB →
> 8GB. Just as well that size limiting is done in a later patch, though I
> wonder whether that should allow per-GPU configuration (not something for
> this patch series, though).

Yes, just try the whole dance with exactly one size and not multiple times.

>
>>> Also, this looks like it's adding complexity to try to make
>>> rarely-executed code slightly faster in some cases (I can't see it
>>> helping where available_sizes == 0x3F00, for example).
>> The intention here is not to make the code faster, but to prevent issues on
>> system where we probe multiple GPUs at once.
> Presumably simultaneous probes and random ordering…

Exactly.

Good news is that I think I've figured out why that happens and our 
hardware/VBIOS engineers are investigating now what's the best approach 
to make thinks work as it should :)

We most likely will end up with a static quirk for this 
PCI-ID/revision/board/manufacturer.

Regards,
Christian.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx