Re: [PATCH v8 00/24] PCI: Allow BAR movement during boot and hotplug

[Sergei Miroshnichenko <s.miroshnichenko@yadro.com>]

Hello Christian,

On Tue, 2020-04-28 at 14:59 +0200, Christian König wrote:
> Well that is a really nice surprise. Just FYI the situation with GPUs
> is 
> essentially this:
> 
> a) The BAR to access video memory with the CPU is by default only
> 256MB 
> in size for backward compatibility with 32bit Windows 7 and older.
> 
> b) Modern GPUs easily have 16GB of video memory, but most of that
> used 
> to be accessed only rarely by the CPU. Unfortunately this has
> changed 
> recently by getting more modern graphics APIs in userspace (Vulkan).
> 
> c) Both NVidia as well as AMD used to have a mechanism to map
> different 
> stuff into the 256MB window, but AMD dropped this ability quite some 
> time ago because it was rather inefficient.
> 
> d) Instead for hard of the last 5 years AMD implements the PCI
> standard 
> for dynamic BAR resizing. So what we do is to extend the 256MB BAR
> into 
> 16GB (or whatever is needed) once the OS is started and the driver
> loads.
> 
> The problem with this approach is that sometimes bridges can't be 
> reconfigured and BARs resized because we have other BARs currently
> in 
> use under the same bridge.
> 
> So long story short you have fixed my BAR resizing problem with this 
> patchset as well :D
> 

Thanks for introducing to this problem, it is not yet covered by
this code, so I'll modify the pci_resize_resource(): let it try as
it does now, and if that didn't work - try pci_rescan_bus(), which
moves BARs.

To test this, do I need to trigger a BAR resizing manually, or
drm/amdgpu will do it automatically during init?

May these resized BAR change their start address during init?

> Am 27.04.20 um 20:23 schrieb Sergei Miroshnichenko:
> > ...
> > 
> > Drivers indicate their support of the feature by implementing the
> > new hooks
> > .rescan_prepare() and .rescan_done() in the struct pci_driver. If a
> > driver
> > doesn't yet support the feature, BARs of its devices will be
> > considered as
> > immovable and handled in the same way as resources with the
> > PCI_FIXED flag:
> > they are guaranteed to remain untouched.
> 
> Could we let rescan_prepare() optionally return an error and then 
> consider the BARs in question not movable for the current rescan? 
> Alternatively would it be allowed in the implementation of the 
> rescan_prepare() callback to update the PCI_FIXED flags on the BARs?
> 
> Problem is that we can't know beforehand if a BAR is currently in use
> or 
> not or if we can block the uses until the rescan is completed.

I guess one more optional hook may be added to the pci_driver:

  bool (*bar_fixed)(struct pci_dev *dev, struct resource *res);

So a driver can mark some BARs as fixed, and some - as movable, in
runtime, depending on current conditions.

If rescan_prepare() and rescan_done() hooks are set, but bar_fixed()
isn't, consider every BAR as movable. If bar_fixed() is set and returns
false, the driver must not use it between rescan_prepare() and
rescan_done().

Best regards,
Serge