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

["Christian König" <christian.koenig@amd.com>]

On 11/11/25 13:56, Ilpo Järvinen wrote:
> 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?

The non-prefetchable 32bit window.

> 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.

I need to double check, but if I'm not completely mistaken that is assigned by the BIOS.

Here is an example of a "good" configuration where both VRAM (BAR0) and doorbell (BAR2) is in the prefetchable window and MMIO in the non-prefetchable:

Device:
	Region 0: Memory at 80000000 (64-bit, prefetchable) [size=256M]
	Region 2: Memory at 90000000 (64-bit, prefetchable) [size=2M]
	Region 4: I/O ports at 3000 [size=256]
	Region 5: Memory at 9f300000 (32-bit, non-prefetchable) [size=1M]

Bridge:
	Memory behind bridge: 9f300000-9f4fffff [size=2M] [32-bit]
	Prefetchable memory behind bridge: 80000000-901fffff [size=258M] [32-bit]

And here is an example of another system where things are mixed up:

Device:
	Region 0: Memory at 2c00000000 (64-bit, prefetchable) [size=256M]
	Region 2: Memory at 94000000 (64-bit, prefetchable) [size=2M]
	Region 4: I/O ports at 1000 [size=256]
	Region 5: Memory at 94600000 (32-bit, non-prefetchable) [size=512K]

Bridge:
	Memory behind bridge: 94000000-946fffff [size=7M] [32-bit]
	Prefetchable memory behind bridge: 2c00000000-2c107fffff [size=264M] [32-bit]

In that example the doorbell ended up in the non-prefetchable window for some reason. And that config comes in all possible variations.

On AMD GPUs both BAR0 and BAR2 are resizeable.

So far we have only implemented resizing of BAR0, but essentially we want to have both for some use cases.

>> 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.

Well see the example above. I have SSH access to a system where exactly that is the configuration.

> 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.

No, as I said that comes from the BIOS.

> 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?

No, at least from the amdgpu driver side we don't touch the resource allocation at all.

In this case preparation means disabling the VGA emulation, cause otherwise trying to resize the BAR can just cause a spontaneous system reboot for some reason. 

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

Then let us solve this issue by this somehow.

Regards,
Christian.

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