From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Christian_K=c3=b6nig?= Subject: Re: [PATCH v6 5/5] drm/amdgpu: resize VRAM BAR for CPU access v2 Date: Fri, 9 Jun 2017 10:59:38 +0200 Message-ID: <55fb9500-5062-78ea-93c3-0f3e850dd31f@vodafone.de> References: <1494348547-1465-1-git-send-email-deathsimple@vodafone.de> <1494348547-1465-6-git-send-email-deathsimple@vodafone.de> <20170601201430.GC12257@bhelgaas-glaptop.roam.corp.google.com> <72e061a5-9684-06e2-3021-80de8ca97bd7@vodafone.de> <20170602202631.GA1452@bhelgaas-glaptop.roam.corp.google.com> <779a883a-265a-ae04-b0cd-8cb3599f0dc0@vodafone.de> <20170606231019.GC12672@bhelgaas-glaptop.roam.corp.google.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20170606231019.GC12672-1RhO1Y9PlrlHTL0Zs8A6p5iNqAH0jzoTYJqu5kTmcBRl57MIdRCFDg@public.gmane.org> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Sender: "amd-gfx" To: Bjorn Helgaas Cc: David Airlie , linux-pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, platform-driver-x86-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, Alex Deucher List-Id: platform-driver-x86.vger.kernel.org QW0gMDcuMDYuMjAxNyB1bSAwMToxMCBzY2hyaWViIEJqb3JuIEhlbGdhYXM6Cj4gW1NOSVBdCj4g V2hhdCBpZiB0aGUgZHJpdmVyIGRpZCBzb21ldGhpbmcgbGlrZSB0aGlzOgo+Cj4gICAgcGNpX2Rp c2FibGVfZGVjb2RpbmcoZGV2LCBJT1JFU09VUkNFX01FTSk7Cj4gICAgcGNpX3JlbGVhc2VfcmVz b3VyY2UoZGV2LCAyKTsKPiAgICBwY2lfcmVzaXplX2JhcihkZXYsIGJhciwgc2l6ZSk7Cj4gICAg cGNpX2Fzc2lnbl9yZXNvdXJjZXMoZGV2KTsKPiAgICBwY2lfZW5hYmxlX2RlY29kaW5nKGRldiwg SU9SRVNPVVJDRV9NRU0pOwo+Cj4gVGhhdCB3b3VsZCByZXF1aXJlIGFkZGluZyBwY2lfZW5hYmxl L2Rpc2FibGVfZGVjb2RpbmcoKSB0byB0aGUgZHJpdmVyCj4gQVBJLCBhbG9uZyB3aXRoIHRoZSBy ZXF1aXJlbWVudCB0aGF0IHRoZSBkcml2ZXIgZGlzY2FyZCBhbmQgcmVtYXAKPiBzb21lIHJlc291 cmNlcyBhZnRlciBwY2lfZW5hYmxlX2RlY29kaW5nKCkuICBJIHRoaW5rCj4gcGNpX2VuYWJsZV9k ZWNvZGluZygpIHdvdWxkIGxvb2sgbXVjaCBsaWtlIHRoZSBleGlzdGluZwo+IHBjaV9lbmFibGVf cmVzb3VyY2VzKCkgZXhjZXB0IHRha2luZyBhIHJlc291cmNlIHR5cGUgaW5zdGVhZCBvZiBhCj4g Yml0bWFzay4KClRoaXMgaXMgcHJldHR5IGNsb3NlIHRvIHdoYXQgd2UgYWxyZWFkeSBkby4gSSdt IGdvaW5nIHRvIHNlbmQgb3V0IGFuIAp1cGRhdGVkIHZlcnNpb24gb2YgdGhlIHBhdGNoIGluIGEg bWludXRlLgoKT25lIGRpZmZlcmVuY2UgdGhhdCBJIHN0aWxsIGhhdmUgaW4gdGhpcyBwYXRjaCBp cwo+ICsgICAgICAgcGNpX3JlYWRfY29uZmlnX3dvcmQoYWRldi0+cGRldiwgUENJX0NPTU1BTkQs ICZjbWQpOwo+ICsgICAgICAgcGNpX3dyaXRlX2NvbmZpZ193b3JkKGFkZXYtPnBkZXYsIFBDSV9D T01NQU5ELAo+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGNtZCAmIH5QQ0lfQ09NTUFO RF9NRU1PUlkpOwppbiB0aGUgZHJpdmVyIGluc3RlYWQgb2YgInBjaV9kaXNhYmxlX2RlY29kaW5n KGRldiwgSU9SRVNPVVJDRV9NRU0pOyIKCkkgdGhvdWdodCB0aGF0IGludHJvZHVjaW5nIGEgbmV3 IGludGVyZmFjZSBmb3IgdGhpcyB0d28gbGluZXMgd291bGQgYmUgCm92ZXJraWxsLCBidXQgaWYg eW91IGZpbmQgaXQgY2xlYW5lciB0byBhZGQgdGhlIG5ldyBpbnRlcmZhY2UgSSBjYW4gCmNoYW5n ZSB0aGF0LgoKPiBJIHdvdWxkICpwcmVmZXIqIGlmIHdlIHJlbGVhc2VkIGFuZCByZWFzc2lnbmVk IGFsbCByZXNvdXJjZXMsIGJlY2F1c2UKPiB0aGVuIHRoZSBjb3JlIGhhcyBjb21wbGV0ZSBmbGV4 aWJpbGl0eSB0byBtb3ZlIHRoaW5ncyBhcm91bmQsIGFuZCBpdCdzCj4gZWFzeSB0byBkb2N1bWVu dCB0aGF0IHBjaV9hc3NpZ25fcmVzb3VyY2VzKCkgbWVhbnMgeW91IGhhdmUgdG8KPiByZXJlYWQv cmVtYXAgZXZlcnl0aGluZy4KSSd2ZSB0cmllZCB0aGlzIGFuZCBpdCBkb2Vzbid0IHdvcmsgYXQg YWxsLCBzdXJwcmlzaW5nbHkgdGhlIEkvTyBwb3J0cyAKdHVybmVkIG91dCB0byBiZSBub3QgdGhl IGZpcnN0IHByb2JsZW0gSSd2ZSByYW4gaW50by4KClJlbGVhc2luZyBhbGwgcmVzb3VyY2VzIG1l YW5zIHRoYXQgd2UgYWxzbyB0cnkgdG8gcmVsZWFzZSB0aGUgCjB4YTAwMC0weGJmZmZmIChWR0Ep IGFuZCAweGMwMDAwLTB4ZGZmZmYgKFZCSU9TKSByYW5nZXMuCgpUaGV5IGNhbiBiZSByZWFzc2ln bmVkLCBidXQgc29tZWhvdyB0aGF0IHNlZW1zIHRvIGNhdXNlIHByb2JsZW1zIGxhdGVyIG9uLgoK PiBJZiB0aGUgZHJpdmVyIG9ubHkgcmVsZWFzZXMgc3BlY2lmaWVkIEJBUnMsIHRoZSBwY2lfYXNz aWduX3Jlc291cmNlcygpCj4gaW50ZXJmYWNlIGJlY29tZXMgInlvdSBuZWVkIHRvIHJlcmVhZC9y ZW1hcCB0aGUgQkFSIHlvdSByZXNpemVkLCBwbHVzCj4gYW55IG90aGVyIEJBUnMgeW91IHJlbGVh c2VkIi4gIEl0J3MgYSBsaXR0bGUgbW9yZSBjb21wbGljYXRlZCB0bwo+IGRlc2NyaWJlIGFuZCBt b3JlIGRlcGVuZGVudCBvbiBwcmV2aW91cyBkcml2ZXIgYWN0aW9ucy4KSG93IGFib3V0IHRoZSBk cml2ZXIgcmVsZWFzZXMgYWxsIHJlc291cmNlcyBpdCB3YW50cyB0byBtb3ZlLCBpbmNsdWRpbmcg CnRoZSByZXNpemVkIGFuZCByZWFsbG9jYXRlcyB0aGVtIGFmdGVyIHRoZSByZXNpemUgaXMgZG9u ZT8KCj4KPiBCdXQgcmVsZWFzaW5nIG9ubHkgdGhlIHNwZWNpZmllZCBCQVIgd2lsbCBtYWtlIHlv dXIgbGlmZSBlYXNpZXIgYW5kCj4gaGVscCB3aXRoIHRoZSBmYWN0IHRoYXQgbXVsdGlwbGUgZHJp dmVycyBtaWdodCBiZSB1c2luZyB0aGUgc2FtZSBCQVIKPiAoSSBoYXZlIHRvIHJhaXNlIG15IGV5 ZWJyb3dzIGF0IHRoYXQpLCBzbyBJIHRoaW5rIEknbSBPSyB3aXRoIGl0LiAgQW5kCj4gaXQgd291 bGQgYWxzbyBzaWRlLXN0ZXAgdGhlICJjYW4ndCByZXN0b3JlIHByZXZpb3VzIHN0YXRlIiBwcm9i bGVtLgpJIGFncmVlIHRoYXQgaXQgaXMgYSBiaXQgbW9yZSBkb2N1bWVudGF0aW9uIHdvcmsgdG8g ZGVzY3JpYmUgaG93IHRoZSAKaW50ZXJmYWNlIHdvcmtzLCBidXQgaXQgaXMgY2xlYXJseSBsZXNz IHByb2JsZW1hdGljIGR1cmluZyBydW50aW1lLgoKUGxlYXNlIHRha2UgYSBsb29rIGF0IHRoZSBu ZXcgdmVyc2lvbiBvZiB0aGUgcGF0Y2hlcyBhbmQgbGV0IG1lIGtub3cgCndoYXQgeW91IHRoaW5r LgoKUmVnYXJkcywKQ2hyaXN0aWFuLgoKPgo+IEl0J3MgYW4gImludGVyZXN0aW5nIiBhc3ltbWV0 cnkgdGhhdCBwY2lfZW5hYmxlX2RldmljZSgpIHR1cm5zIG9uIEJBUgo+IGRlY29kaW5nIGJ1dCBk b2Vzbid0IHRvdWNoIGJ1cyBtYXN0ZXJpbmcsIHdoaWxlIHBjaV9kaXNhYmxlX2RldmljZSgpCj4g dHVybnMgb2ZmIGJ1cyBtYXN0ZXJpbmcgYnV0IGRvZXNuJ3QgdG91Y2ggQkFSIGRlY29kaW5nLgo+ Cj4gQmpvcm4KCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f XwphbWQtZ2Z4IG1haWxpbmcgbGlzdAphbWQtZ2Z4QGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRw czovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2FtZC1nZngK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: Subject: Re: [PATCH v6 5/5] drm/amdgpu: resize VRAM BAR for CPU access v2 To: Bjorn Helgaas Cc: linux-pci@vger.kernel.org, platform-driver-x86@vger.kernel.org, Alex Deucher , David Airlie , amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org References: <1494348547-1465-1-git-send-email-deathsimple@vodafone.de> <1494348547-1465-6-git-send-email-deathsimple@vodafone.de> <20170601201430.GC12257@bhelgaas-glaptop.roam.corp.google.com> <72e061a5-9684-06e2-3021-80de8ca97bd7@vodafone.de> <20170602202631.GA1452@bhelgaas-glaptop.roam.corp.google.com> <779a883a-265a-ae04-b0cd-8cb3599f0dc0@vodafone.de> <20170606231019.GC12672@bhelgaas-glaptop.roam.corp.google.com> From: =?UTF-8?Q?Christian_K=c3=b6nig?= Message-ID: <55fb9500-5062-78ea-93c3-0f3e850dd31f@vodafone.de> Date: Fri, 9 Jun 2017 10:59:38 +0200 MIME-Version: 1.0 In-Reply-To: <20170606231019.GC12672@bhelgaas-glaptop.roam.corp.google.com> Content-Type: text/plain; charset=utf-8; format=flowed List-ID: 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