Re: [PATCH v6 5/5] drm/amdgpu: resize VRAM BAR for CPU access v2
Am 07.06.2017 um 01:10 schrieb Bjorn Helgaas: [SNIP] What if the driver did something like this: pci_disable_decoding(dev, IORESOURCE_MEM); pci_release_resource(dev, 2); pci_resize_bar(dev, bar, size); pci_assign_resources(dev); pci_enable_decoding(dev, IORESOURCE_MEM); That would require adding pci_enable/disable_decoding() to the driver API, along with the requirement that the driver discard and remap some resources after pci_enable_decoding(). I think pci_enable_decoding() would look much like the existing pci_enable_resources() except taking a resource type instead of a bitmask. This is pretty close to what we already do. I'm going to send out an updated version of the patch in a minute. One difference that I still have in this patch is + pci_read_config_word(adev->pdev, PCI_COMMAND, &cmd); + pci_write_config_word(adev->pdev, PCI_COMMAND, + cmd & ~PCI_COMMAND_MEMORY); in the driver instead of "pci_disable_decoding(dev, IORESOURCE_MEM);" I thought that introducing a new interface for this two lines would be overkill, but if you find it cleaner to add the new interface I can change that. I would *prefer* if we released and reassigned all resources, because then the core has complete flexibility to move things around, and it's easy to document that pci_assign_resources() means you have to reread/remap everything. I've tried this and it doesn't work at all, surprisingly the I/O ports turned out to be not the first problem I've ran into. Releasing all resources means that we also try to release the 0xa000-0xb (VGA) and 0xc-0xd (VBIOS) ranges. They can be reassigned, but somehow that seems to cause problems later on. If the driver only releases specified BARs, the pci_assign_resources() interface becomes "you need to reread/remap the BAR you resized, plus any other BARs you released". It's a little more complicated to describe and more dependent on previous driver actions. How about the driver releases all resources it wants to move, including the resized and reallocates them after the resize is done? But releasing only the specified BAR will make your life easier and help with the fact that multiple drivers might be using the same BAR (I have to raise my eyebrows at that), so I think I'm OK with it. And it would also side-step the "can't restore previous state" problem. I agree that it is a bit more documentation work to describe how the interface works, but it is clearly less problematic during runtime. Please take a look at the new version of the patches and let me know what you think. Regards, Christian. It's an "interesting" asymmetry that pci_enable_device() turns on BAR decoding but doesn't touch bus mastering, while pci_disable_device() turns off bus mastering but doesn't touch BAR decoding. Bjorn ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH v6 5/5] drm/amdgpu: resize VRAM BAR for CPU access v2
> -Original Message- > From: Bjorn Helgaas [mailto:helg...@kernel.org] > Sent: Tuesday, June 06, 2017 7:10 PM > To: Christian König > Cc: linux-...@vger.kernel.org; platform-driver-...@vger.kernel.org; > Deucher, Alexander; David Airlie; amd-...@lists.freedesktop.org; dri- > de...@lists.freedesktop.org > Subject: Re: [PATCH v6 5/5] drm/amdgpu: resize VRAM BAR for CPU access > v2 > > On Tue, Jun 06, 2017 at 01:51:11PM +0200, Christian König wrote: > > Am 02.06.2017 um 22:26 schrieb Bjorn Helgaas: > > >On Fri, Jun 02, 2017 at 11:32:21AM +0200, Christian König wrote: > > >>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)); > > > >>> 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. > > >I don't understand the problem here yet. If you need to enable the > > >device, then disable it, resize, and re-enable it, that's fine. > > > > The issue is we never enable the device ourself in amdgpu, except > > for some rare cases during resume. > > > > In most of the cases we have to handle this is the primary display &
Re: [PATCH v6 5/5] drm/amdgpu: resize VRAM BAR for CPU access v2
On Tue, Jun 06, 2017 at 01:51:11PM +0200, Christian König wrote: > Am 02.06.2017 um 22:26 schrieb Bjorn Helgaas: > >On Fri, Jun 02, 2017 at 11:32:21AM +0200, Christian König wrote: > >>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)); > >>> 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. > >I don't understand the problem here yet. If you need to enable the > >device, then disable it, resize, and re-enable it, that's fine. > > The issue is we never enable the device ourself in amdgpu, except > for some rare cases during resume. > > In most of the cases we have to handle this is the primary display > device which is enabled by either the BIOS, VGA console, VesaFB or > EFIFB. Amdgpu just kicks out whatever driver was responsible for the > device previously and takes over. > > I could of course do the disable/resize/reenable dance, but I would > rather want to avoid that. > > The hardware is most likely already displaying a boot splash and we > want to transit to the desktop without any flickering (at least > that's the long term goal). Completely disabling the device to do > this doesn't sounds like a good idea if we want that. > > >The important thing I'm looking for is that the resize happens before > >a pci_enable_device(), because pci_enable_device() is the sync point > >where the PCI core enables resources and makes them available to the > >driver. Drivers know that they can't look at the resources before > >that point. There's a little bit of text about this in [1]. > > Yeah, I understand that. But wouldn't it be sufficient to just > disable memory decoding during the resize? > > I can easily guarantee that the CPU isn't accessing the BAR during > the time (we need to do this for changing the memory clocks as > well), but I have a bad gut feeling completely turning of the device > while we are still displaying stuff. pci_disable_device() doesn't turn off the device; it only disables bus mastering (and some of the arch-specific pcibios_disable_device() implementations do a little more). But it's certainly the wrong direction -- it disables DMA, which has nothing to do with the BAR decoding we're interested in. What if the driver did something like this: pci_disable_decoding(dev, IORESOURCE_MEM); pci_release_resource(dev, 2); pci_resize_bar(dev, bar, size); pci_assign_resources(dev); pci_enable_decoding(dev, IORESOURCE_MEM); That would require adding pci_enable/disable_decoding() to the driver API, along with the requirement that the driver
Re: [PATCH v6 5/5] drm/amdgpu: resize VRAM BAR for CPU access v2
On Tue, Jun 6, 2017 at 7:51 AM, Christian König wrote: > Am 02.06.2017 um 22:26 schrieb Bjorn Helgaas: >> >> On Fri, Jun 02, 2017 at 11:32:21AM +0200, Christian König wrote: >>> >>> 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. >> >> The problem with that is that it's impossible for a code reader to >> verify that. So adding a check is ugly but I think makes it more >> readable. > > > Good point. I will just move the check into the function itself, that should > handle all such cases. > 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. >> >> I don't understand the problem here yet. If you need to enable the >> device, then disable it, resize, and re-enable it, that's fine. > > > The issue is we never enable the device ourself in amdgpu, except for some > rare cases during resume.
Re: [PATCH v6 5/5] drm/amdgpu: resize VRAM BAR for CPU access v2
Am 02.06.2017 um 22:26 schrieb Bjorn Helgaas: On Fri, Jun 02, 2017 at 11:32:21AM +0200, Christian König wrote: 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. The problem with that is that it's impossible for a code reader to verify that. So adding a check is ugly but I think makes it more readable. Good point. I will just move the check into the function itself, that should handle all such cases. 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. I don't understand the problem here yet. If you need to enable the device, then disable it, resize, and re-enable it, that's fine. The issue is we never enable the device ourself in amdgpu, except for some rare cases during resume. In most of the cases we have to handle this is the primary display device which is enabled by either the BIOS, VGA console, VesaFB or EFIFB. Amdgpu just kicks out whatever driver was responsible for the device previously and takes over. I could of course do the disable/resize/reenable dance, but I would rather want to avoid that. The hardware is most likely already displaying a boot splash and we want to transit to the desktop without any flickering (at least that's the long term goal). Completely disabling the device to do this doesn't sounds like a good idea if we want that. The important thing I'm look
Re: [PATCH v6 5/5] drm/amdgpu: resize VRAM BAR for CPU access v2
On Fri, Jun 02, 2017 at 11:32:21AM +0200, Christian König wrote: > 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. The problem with that is that it's impossible for a code reader to verify that. So adding a check is ugly but I think makes it more readable. > 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. I don't understand the problem here yet. If you need to enable the device, then disable it, resize, and re-enable it, that's fine. The important thing I'm looking for is that the resize happens before a pci_enable_device(), because pci_enable_device() is the sync point where the PCI core enables resources and makes them available to the driver. Drivers know that they can't look at the resources before that point. There's a little bit of text about this in [1]. > 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 h
Re: [PATCH v6 5/5] drm/amdgpu: resize VRAM BAR for CPU access v2
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;
Re: [PATCH v6 5/5] drm/amdgpu: resize VRAM BAR for CPU access v2
[+cc ADMGPU, DRM folks] On Tue, May 09, 2017 at 06:49:07PM +0200, Christian König wrote: > From: Christian König > > Try to resize BAR0 to let CPU access all of VRAM. > > v2: rebased, style cleanups, disable mem decode before resize, > handle gmc_v9 as well, round size up to power of two. > > Signed-off-by: Christian König > --- > drivers/gpu/drm/amd/amdgpu/amdgpu.h| 1 + > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 37 > ++ > drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c | 8 --- > drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c | 8 --- > drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 10 > 5 files changed, 54 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > index 5310781..d6f5286 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > @@ -1884,6 +1884,7 @@ uint64_t amdgpu_ttm_tt_pte_flags(struct amdgpu_device > *adev, struct ttm_tt *ttm, >struct ttm_mem_reg *mem); > void amdgpu_vram_location(struct amdgpu_device *adev, struct amdgpu_mc *mc, > u64 base); > void amdgpu_gtt_location(struct amdgpu_device *adev, struct amdgpu_mc *mc); > +void amdgpu_resize_bar0(struct amdgpu_device *adev); > void amdgpu_ttm_set_active_vram_size(struct amdgpu_device *adev, u64 size); > int amdgpu_ttm_init(struct amdgpu_device *adev); > void amdgpu_ttm_fini(struct amdgpu_device *adev); > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index 2719c02..4e83a56 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -694,6 +694,43 @@ void amdgpu_gtt_location(struct amdgpu_device *adev, > struct amdgpu_mc *mc) > mc->gtt_size >> 20, mc->gtt_start, mc->gtt_end); > } > > +/** > + * 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. 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 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);