Hi Bjorn,
sorry for not responding earlier and thanks for picking this thread up
again.
Am 01.06.2017 um 22:14 schrieb Bjorn Helgaas:
[+cc ADMGPU, DRM folks]
On Tue, May 09, 2017 at 06:49:07PM +0200, Christian König wrote:
[SNIP]
+/**
+ * amdgpu_resize_bar0 - try to resize BAR0
+ *
+ * @adev: amdgpu_device pointer
+ *
+ * Try to resize BAR0 to make all VRAM CPU accessible.
+ */
+void amdgpu_resize_bar0(struct amdgpu_device *adev)
+{
+ u64 space_needed = roundup_pow_of_two(adev->mc.real_vram_size);
+ u32 rbar_size = order_base_2(((space_needed >> 20) | 1)) -1;
+ u16 cmd;
+ int r;
+
+ /* Free the doorbell mapping, it most likely needs to move as well */
+ amdgpu_doorbell_fini(adev);
+ pci_release_resource(adev->pdev, 2);
+
+ /* 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,
+ cmd & ~PCI_COMMAND_MEMORY);
+
+ 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);
+
+ pci_write_config_word(adev->pdev, PCI_COMMAND, cmd);
+
+ /* When the doorbell BAR isn't available we have no chance of
+ * using the device.
+ */
+ BUG_ON(amdgpu_doorbell_init(adev));
This amdgpu_doorbell_fini()/amdgpu_doorbell_init() thing doesn't look
right. amdgpu_device_init() only calls amdgpu_doorbell_init() for
"adev->asic_type >= CHIP_BONAIRE", but we call it unconditionally
here.
This is the call graph:
amdgpu_device_init
adev->rmmio_base = pci_resource_start(adev->pdev, 5) # 2 for < BONAIRE
adev->rmmio = ioremap(adev->rmmio_base, ...)
DRM_INFO("register mmio base: 0x%08X\n", (uint32_t)adev->rmmio_base)
if (adev->asic_type >= CHIP_BONAIRE) {
amdgpu_doorbell_init
adev->doorbell.base = pci_resource_start(adev->pdev, 2)
adev->doorbell.ptr = ioremap(adev->doorbell.base, ...)
}
amdgpu_init
gmc_v7_0_sw_init # gmc_v7_0_ip_funcs.sw_init
gmc_v7_0_mc_init
+ amdgpu_resize_bar0
+ amdgpu_doorbell_fini
+ pci_release_resource(adev->pdev, 2)
+ pci_resize_resource(adev->pdev, 0, size)
+ amdgpu_doorbell_init
adev->mc.aper_base = pci_resource_start(adev->pdev, 0)
If "asic_type < CHIP_BONAIRE", we ioremapped BAR 2 in
amdgpu_device_init(), then we released the resource here and never
updated the ioremap.
The first hardware with a resizeable BAR I could find is a Tonga, and
that is even a generation later than Bonaire.
So we are never going to call this code on earlier hardware generations.
But I think I will just move the asic_type check into the function to be
absolute sure.
From the PCI core perspective, it would be much cleaner to do the BAR
resize before the driver calls pci_enable_device(). If that could be
done, there would be no need for this sort of shutdown/reinit stuff
and we wouldn't have to worry about issues like these. The amdgpu
init path is pretty complicated, so I don't know whether this is
possible.
I completely agree on this and it is actually the approach I tried first.
There are just two problems with this approach:
1. When the amdgpu driver is loaded there can already be the VGA
console, Vesa or EFI driver active for the device and displaying the
splash screen.
When we resize and most likely relocate the BAR while those drivers are
active it will certainly cause problems.
What amdgpu does before trying to resize the BAR is kicking out other
driver and making sure it has exclusive access to the hardware.
2. Without taking a look at the registers you don't know how much memory
there actually is on the board.
We could always resize it to the maximum supported, but that would mean
we could easily waste 128GB of address space while the hardware only has
8GB of VRAM.
That would not necessarily hurt us when we have enough address space,
but at least kind of sucks.
I would also like to simplify the driver usage model and get the
PCI_COMMAND_MEMORY twiddling into the PCI core instead of the driver.
Ideally, the driver would do something like this:
pci_resize_resource(adev->pdev, 0, rbar_size);
pci_enable_device(adev->pdev);
And the PCI core would be something along these lines:
int pci_resize_resource(dev, bar, size)
{
if (pci_is_enabled(dev))
return -EBUSY;
pci_disable_decoding(dev); # turn off MEM, IO decoding
pci_release_resources(dev); # (all of them)
err = pci_resize_bar(dev, bar, size); # change BAR size (only)
if (err)
return err;
pci_assign_resources(dev); # reassign all "dev" resources
return 0;
}
I already tried the approach with releasing all resources, but it didn't
worked so well.
When resizing fails because we don't have enough address space then we
at least want to get back to a working config.
Releasing everything makes that rather tricky, since I would then need
to keep a backup of the old config and try to restore it.
Additional to that I'm not sure if releasing the register BAR and
relocating it works with amdgpu.
Regards,
Christian.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
