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