Re: [PATCH v6 5/5] drm/amdgpu: resize VRAM BAR for CPU access v2

["Christian König" <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>]

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-0xbffff (VGA) and 0xc0000-0xdffff (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


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx