[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 at 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);
>