Re: [PATCH 8/9] drm/amdgpu: Remove driver side BAR release before resize

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4172 bytes --]

On Tue, 11 Nov 2025, Christian König wrote:

> On 11/11/25 12:08, Ilpo Järvinen wrote:
> > On Tue, 11 Nov 2025, Christian König wrote:
> > 
> >> Sorry for the late reply I'm really busy at the moment.
> >>
> >> On 10/28/25 18:35, Ilpo Järvinen wrote:
> >>> PCI core handles releasing device's resources and their rollback in
> >>> case of failure of a BAR resizing operation. Releasing resource prior
> >>> to calling pci_resize_resource() prevents PCI core from restoring the
> >>> BARs as they were.
> >>
> >> I've intentionally didn't do it this way because at least on AMD HW we 
> >> could only release the VRAM and doorbell BAR (both 64bit), but not the 
> >> register BAR (32bit only).
> >>
> >> This patch set looks like the right thing in general, but which BARs are 
> >> now released by pci_resize_resource()?
> >>
> >> If we avoid releasing the 32bit BAR as well then that should work, 
> >> otherwise we will probably cause problems.
> > 
> > After these changes, pci_resize_resource() releases BARs that share the 
> > bridge window with the BAR to be resized. So the answer depends on the 
> > upstream bridge.
> > 
> > However, amdgpu_device_resize_fb_bar() also checks that root bus has a
> > resource with a 64-bit address. That won't tell what the nearest bridge 
> > has though. Maybe that check should be converted to check the resources of 
> > the nearest bus instead? It would make it impossible to have the 
> > 32-bit resource share the bridge window with the 64-bit resources so the 
> > resize would be safe.
> 
> Mhm, I don't think that will work.
> 
> 
> I've added the check for the root bus to avoid a couple of issues during 
> resize, but checking the nearest bridge would block a whole bunch of use 
> cases and isn't even 100% save.
> 
> See one use case of this is that all the BARs of the device start in the 
> same 32bit bridge window (or a mixture of 64bit and 32bit window).

"32bit bridge window" is ambiguous. There are non-prefetchable and 
prefetchable bridge windows, out of which the latter can be 64-bit as 
well. Which one you're talking about?

If a 64-bit prefetchable window exists, pbus_size_mem() nor 
__pci_assign_resource() would not have produced such a configuration where 
they're put into the same bridge window, even before the commit 
ae88d0b9c57f ("PCI: Use pbus_select_window_for_type() during mem window 
sizing") (I think). Now pbus_size_mem() certainly doesn't.

> What we have is that BAR 0 and 2 are 64bit BARs which can (after some 
> preparation) move around freely. But IIRC BAR 4 are the legacy I/O ports 
> and BAR 5 is the 32bit MMIO registers (don't nail me on that, could be 
> just the other way around).
>
> Especially that 32bit MMIO BAR *can't* move! Not only because it is 
> 32bit, but also because the amdgpu driver as well as the HW itself 
> through the VGA emulation, as well as the EFI/VESA/VBIOS code might 
> reference its absolute address.

So if the 64-bit check is replaced with this:

+       /* Check if the parent bridge has a 64-bit (pref) memory resource */
+       res = pci_resource_n(adev->pdev, 0)->parent;
+       /* Trying to resize is pointless without a window above 4GB */
+       if (!(res->flags & IORESOURCE_MEM_64))
		return 0;

...I don't think it's possible for 32-bit resource to share that window 
under _any_ circumstance.

If you say that ->parent somehow points to a non-IORESOURCE_MEM_64 window 
at this point, you're implying allocation for the 64-bit prefetchable 
window was tried and failed, and __pci_assign_resource() then used one of 
its fallbacks.

Are you saying that "some preparation" includes making room for that 
64-bit prefetchable window that failed to assign earlier as I cannot see 
how else it would ever get assigned so that the 64-bit BARs could be moved 
there?

> Could we give pci_resize_resource() a mask of BARs which are save to 
> release?

It is possible.

> Or maybe a flag to indicate that it can only free up 64bit BARs?
> 
> Regards,
> Christian.
> 
> > 
> > Thanks a lot for checking this out!
> > 
> 

-- 
 i.