From mboxrd@z Thu Jan 1 00:00:00 1970 From: Inki Dae Subject: Re: [PATCH] drm/exynos: gem: Drop NONCONTIG flag for buffers allocated without IOMMU Date: Mon, 13 Nov 2017 10:24:10 +0900 Message-ID: <5A08F43A.8010204@samsung.com> References: <20171031162813.1546-1-m.szyprowski@samsung.com> <5A051757.9020101@samsung.com> <065ca47d-11e8-adb3-e264-a874a0b84067@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-reply-to: <065ca47d-11e8-adb3-e264-a874a0b84067@samsung.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Marek Szyprowski , dri-devel@lists.freedesktop.org, linux-samsung-soc@vger.kernel.org Cc: Marian Mihailescu , Seung-Woo Kim , stable@vger.kernel.org, Krzysztof Kozlowski , Bartlomiej Zolnierkiewicz List-Id: linux-samsung-soc@vger.kernel.org SGkgTWFyZWssCgoyMDE364WEIDEx7JuUIDEw7J28IDE2OjM17JeQIE1hcmVrIFN6eXByb3dza2kg 7J20KOqwgCkg7JO0IOq4gDoKPiBEZWFyIElua2ksCj4gCj4gT24gMjAxNy0xMS0xMCAwNDowNCwg SW5raSBEYWUgd3JvdGU6Cj4+IDIwMTfrhYQgMTHsm5QgMDHsnbwgMDE6Mjjsl5AgTWFyZWsgU3p5 cHJvd3NraSDsnbQo6rCAKSDsk7Qg6riAOgo+Pj4gV2hlbiBubyBJT01NVSBpcyBhdmFpbGFibGUs IGFsbCBHRU0gYnVmZmVycyBhbGxvY2F0ZWQgYnkgRXh5bm9zIERSTSBkcml2ZXIKPj4+IGFyZSBj b250aWd1b3VzLCBiZWNhdXNlIG9mIHRoZSB1bmRlcmx5aW5nIGRtYV9hbGxvY19hdHRycygpIGZ1 bmN0aW9uCj4+PiBwcm92aWRlcyBvbmx5IHN1Y2ggYnVmZmVycy4gSW4gc3VjaCBjYXNlIGl0IG1h a2VzIG5vIHNlbnNlIHRvIGtlZXAKPj4gV2hhdCBpZiB1c2VyIGRpc2FibGVkIENNQSBzdXBwb3J0 PyBJbiB0aGlzIGNhc2UsIGl0IGd1YXJhbnRlZXMgYWxzbyB0byBhbGxvY2F0ZSBwaHlzaWNhbGx5 IGNvbnRpZ3VvdXMgbWVtb3J5Pwo+PiBJIHRoaW5rIGl0IGRlcGVuZHMgb24gQ01BIHN1cHBvcnQg c28gd291bGRuJ3QgYmUgdHJ1ZS4KPiAKPiBkbWFfYWxsb2NfYXR0cnMoKSBhbHdheXMgZ3VhcmFu dGVlcyB0aGUgY29udGlndWl0eSBvZiB0aGUgYWxsb2NhdGVkIG1lbW9yeQo+IGluIERNQSBhZGRy ZXNzIHNwYWNlLiBXaGVuIE5PSU9NTVUgaXMgYXZhaWxhYmxlLCB0aGlzIG1lYW4gdGhhdCB0aGUg YWxsb2NhdGVkCj4gYnVmZmVyIGlzIGNvbnRpZ3VvdXMgaW4gdGhlIHBoeXNpY2FsIG1lbW9yeS4g V2hlbiBDTUEgaXMgZGlzYWJsZWQsCj4gZG1hX2FsbG9jX2F0dHJzKCkgdXNlcyBhbGxvY19wYWdl cygpIGFsbG9jYXRvci4gVGhlIGRyYXdiYWNrIG9mIGFsbG9jX3BhZ2VzKCkKPiBpcyB0aGF0IGl0 IGVhc2lseSBmYWlscyBpZiBwaHlzaWNhbCBtZW1vcnkgaXMgZnJhZ21lbnRlZCBhbmQgaXQgY2Fu bm90Cj4gYWxsb2NhdGUgbWVtb3J5IGxhcmdlciB0aGFuIE1BWF9PUkRFUiAoNE1pQiBpbiBjYXNl IG9mIEFSTTMyYml0KS4KCllvdSBhcmUgcmlnaHQuIFdpdGhvdXQgSU9NTVUgc3VwcG90IGFsbG9j X3BhZ2VzIGFsd2F5cyB0cnlpZXMgdG8gYWxsb2NhdGUgY29udGlndW91cyBtZW1vcnksCm9yZGVy ID0gZ2V0X29yZGVyKHNpemUpOwpwYWdlID0gYWxsb2NfcGFnZXMoLi4uLCBvcmRlcik7CmlmICgh cGFnZSkKCXJldHVybiBOVUxMOwouLi4KCj4gCj4+IFJlYWwgcHJvYmxlbSBJIHRoaW5rIGlzIHRo YXQgdXNlciBkb24ndCBrbm93IHdoZXRoZXIgdGhlIGdlbSBidWZmZXIgYWxsb2NhdGVkIHdpdGgg Q09OVElHIG9yIE5PTkNPTlRJRyBmbGFnIGNhbiBiZSB1c2VkIGFzIGEgU0NBTk9VVCBidWZmZXIu Cj4+IFNvIHVzZXIgY2FuIHJlcXVlc3QgYSBwYWdlIGZsaXAgd2l0aCBOT05DT05USUcgYnVmZmVy IHRvIGtlcm5lbCB3aGljaCBkb2Vzbid0IHN1cHBvcnQgSU9NTVUuCj4+Cj4+IEFuZCBhbm90aGVy IGlzIHRoYXQgdXNlciBtYXkgd2FudCB0byB1c2UgTk9OQ09OVElHIGJ1ZmZlciBmb3IgYW5vdGhl ciBwdXJwb3NlLCBub3Qgc2Nhbm91dC4KPj4gU28gaWYgd2UgZW5mb3JjZSBvbiB1c2luZyBDT05U SUcgYnVmZmVyIG9uIGtlcm5lbCB3aXRob3V0IElPTU1VIHN1cHBvcnQsIHRoZW4gaXQgd291bGRu J3QgYmUgcmVhbGx5IHdoYXQgdXNlciBpbnRlbmRlZC4KPiAKPiBXaGVuIElPTU1VIHN1cHBvcnQg aXMgZGlzYWJsZWQsIEFOWSBidWZmZXIgYWxsb2NhdGVkIHdpdGggZG1hX2FsbG9jX2F0dHJzKCkK PiB3aWxsIGJlIGNvbnRpZ3VvdXMsIHNvIEkgc2VlIG5vIHBvaW50IHByb3BhZ2F0aW5nIGluY29y cmVjdCBmbGFnIGZvciBpdC4KPiAKPiBUaGUgb25seSB3YXkgdG8gY3JlYXRlIGEgTk9OQ09OVElH IGJ1ZmZlciB3aXRoIElPTU1VIGRpc2FibGVkIGlzIHRvIGltcG9ydAo+IGl0IGZyb20gb3RoZXIg ZHJpdmVyIGFuZCB0aGlzIHBhdGggaXMgYWxyZWFkeSBoYW5kbGVkIGNvcnJlY3RseS4KPiAKPj4g TXkgaWRlYSBpcyB0byBwcm92aWRlIGEgbmV3IGZsYWcgLSBpLmUuLCBFWFlOT1NfQk9fU0NBTk9V VCAtIHdoaWNoIGNhbiBhbGxvY2F0ZSBhIGJ1ZmZlciB3aXRoIGEgZGlmZmVyZW50IGFsbG9jYXRp b24gdHlwZSAtIENPTlRJRyBvciBOT05DT05USUcgLSBhY2NvcmRpbmcgdG8gSU9NTVUgc3VwcG9y dC4KPj4gQW5kIGFueSBwYWdlIGZsaXAgcmVxdWVzdCB3aXRoIE5PTkNPTlRJRyBidWZmZXIgdG8g a2VybmVsIHdpdGhvdXQgSU9NTVUgc3VwcG9ydCBzaG91bGQgZmFpbCBhbmQgaXQgaGFzIHRvIHJl dHVybiBhIGVycm9yIHdpdGggYSBwcm9wZXIgZXJyb3IgbWVzc2FnZS4KPiAKPiBJIGRvbid0IHRo aW5rIHRoYXQgd2UgbmVlZCBpdC4gV2l0aCB0aGUgcHJvcG9zZWQgcGF0Y2ggdGhlIHNhbWUgdXNl cnNwYWNlIHdpbGwKPiB3b3JrIGZpbmUgaW4gYm90aCBjYXNlcyBJT01NVSBlbmFibGVkIGFuZCBk aXNhYmxlZCwgZXZlbiBpZiBpdCBhbGxvY2F0ZSBHRU0KPiB3aXRoIE5PTkNPTlRJRyBmbGFnLiBX ZSBjYW4gYXNzdW1lIHRoYXQgQ09OVElHIGlzIGEgc3BlY2lhbCBjYXNlIG9mIE5PTkNPTlRJRwo+ IHRoZW4uCgpUaGUgcHJvYmxlbSBpcyByZWFsbHkgbm90IHdoZXRoZXIgdXNlciBzcGFjZSB3aWxs IHdvcmsgZmluZSBvciBub3QgYnV0IGlzIHRoYXQgdGhpcyBtYXkgYmUgbm90IHdoYXQgdXNlciBz cGFjZSB3YW50ZWQuCkkuZS4sIHVzZXIgc3BhY2UgZXhwZWN0cyB0aGUgYWxsb2NhdGVkIGJ1ZmZl ciBpcyBOT05DT05USUcgYnVmZmVyIGJlY2F1c2UgdXNlciBzcGFjZSByZXF1ZXN0ZWQgTk9OQ09O VElHIGJ1ZmZlciBidXQga2VybmVsIGludGVybmFsbHksIHRoaXMgaXMgY2hhbmdlZCB0byBDT05U SUcgYnVmZmVyLgoKU28geW91IGNvdWxkIHByb3ZpZGUgc29tZSBpbmZvcm1hdGlvbiAtIG1heWJl IHdhcm5pbmcgbWVzc2FnZT8/IC0gdG8gdXNlciBzcGFjZSB0aGUgYnVmZmVyIHR5cGUgaXMgY2hh bmdlZCB0byBDT05USUcgYnVmZmVyIGR1ZSB0byBOTyBJT01NVSBzdXBwb3J0LgoKUmVnYXJkaW5n IHRoZSBidWZmZXIgdHlwZSBFeHlub3MgRFJNIGhhcywgSSdtIG5vdCBzdXJlIHRoYXQgcHJvdmlk aW5nIENPTlRJRyBhbmQgTk9OQ09OVElHIGJ1ZmZlciB0eXBlcyB0byB1c2VyIHNwYWNlIGlzIHJl YXNvbmFibGUgYmVjYXVzZSBJIHRoaW5rIHN1Y2ggYnVmZmVyIHR5cGVzIC0gcGh5c2ljYWxseSBj b250aWd1b3VzIG9yIG5vbiBjb250aWd1b3VzIC0gc2hvdWxkIGJlIHRyYW5zcGFyZW50IHRvIHVz ZXIgc3BhY2UuCkFuZCBJIGxvb2tlZCBpbnRvIG90aGVyIEFSTSBEUk0gZHJpdmVycyAtIG9tYXAg YW5kIG1zbSBwcm92aWRlcyBTQ0FOT1VUIGJ1ZmZlciB0eXBlLCB0ZWdyYSBhbmQgcm9ja2NoaXAg aGF2ZSBubyBzdWNoIGJ1ZmZlciB0eXBlcyAoY29udGlndW91cyBvciBub24tY29udGlndW91cyku CgpUaGlzIGlzIGFsc28gYSB0aGluZyBJIGZlbHQgd2hpbGUgd29ya2luZyBvbiBrZXJuZWwgaW4g Y29sbGFib3JhdGluZyB3aXRoIFBsYXRmb3JtIGd1eXMgLSB3ZSBtYWRlIHVzZXIgc3BhY2VzIHRv IGNhcmUgYWJvdXQgdGhlIG1lbW9yeSBhbGxvY2F0aW9uIHdheS4KCkkgd29uZGVyIHdoeSB5b3Ug c2F5IFNDQU5PVVQgdHlwZSBpcyBpbmNvcnJlY3QgZmxhZy4KClRoYW5rcywKSW5raSBEYWUKCj4g Cj4+PiBCT19OT05DT05USUcgZmxhZyBmb3IgdGhlIGFsbG9jYXRlZCBHRU0gYnVmZmVycy4gVGhp cyBhbGxvd3MgdG8gYXZvaWQKPj4+IGZhaWx1cmVzIGZvciBidWZmZXIgY29udGlndWl0eSBjaGVj a3MgaW4gdGhlIHN1YnNlcXVlbnQgb3BlcmF0aW9ucyBvbiBHRU0KPj4+IG9iamVjdHMuCj4+Pgo+ Pj4gU2lnbmVkLW9mZi1ieTogTWFyZWsgU3p5cHJvd3NraSA8bS5zenlwcm93c2tpQHNhbXN1bmcu Y29tPgo+Pj4gQ0M6IHN0YWJsZUB2Z2VyLmtlcm5lbC5vcmcgIyB2NC40Kwo+Pj4gLS0tCj4+PiBU aGlzIGlzc3VlIGlzIHRoZXJlIHNpbmNlIGNvbW1pdCAwNTE5ZjlhMTJkMDExICgiZHJtL2V4eW5v czogYWRkIGlvbW11Cj4+PiBzdXBwb3J0IGZvciBleHlub3MgZHJtIGZyYW1ld29yayIpLCBidXQg dGhpcyBwYXRjaCBhcHBsaWVzIGNsZWFubHkKPj4+IG9ubHkgdG8gdjQuNCsga2VybmVsIHJlbGVh c2VzIGR1ZSBjaGFuZ2VzIGluIHRoZSBzdXJyb3VuZGluZyBjb2RlLgo+Pj4gLS0tCj4+PiAgIGRy aXZlcnMvZ3B1L2RybS9leHlub3MvZXh5bm9zX2RybV9nZW0uYyB8IDcgKysrKysrKwo+Pj4gICAx IGZpbGUgY2hhbmdlZCwgNyBpbnNlcnRpb25zKCspCj4+Pgo+Pj4gZGlmZiAtLWdpdCBhL2RyaXZl cnMvZ3B1L2RybS9leHlub3MvZXh5bm9zX2RybV9nZW0uYyBiL2RyaXZlcnMvZ3B1L2RybS9leHlu b3MvZXh5bm9zX2RybV9nZW0uYwo+Pj4gaW5kZXggMDJmOTc4YmI5MjYxLi40NzZjMDBmZTE5OTgg MTAwNjQ0Cj4+PiAtLS0gYS9kcml2ZXJzL2dwdS9kcm0vZXh5bm9zL2V4eW5vc19kcm1fZ2VtLmMK Pj4+ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9leHlub3MvZXh5bm9zX2RybV9nZW0uYwo+Pj4gQEAg LTIyNyw2ICsyMjcsMTMgQEAgc3RydWN0IGV4eW5vc19kcm1fZ2VtICpleHlub3NfZHJtX2dlbV9j cmVhdGUoc3RydWN0IGRybV9kZXZpY2UgKmRldiwKPj4+ICAgICAgIGlmIChJU19FUlIoZXh5bm9z X2dlbSkpCj4+PiAgICAgICAgICAgcmV0dXJuIGV4eW5vc19nZW07Cj4+PiAgICsgICAgLyoKPj4+ ICsgICAgICogd2hlbiBubyBJT01NVSBpcyBhdmFpbGFibGUsIGFsbCBhbGxvY2F0ZWQgYnVmZmVy cyBhcmUgY29udGlndW91cwo+Pj4gKyAgICAgKiBhbnl3YXksIHNvIGRyb3AgRVhZTk9TX0JPX05P TkNPTlRJRyBmbGFnCj4+PiArICAgICAqLwo+Pj4gKyAgICBpZiAoIWlzX2RybV9pb21tdV9zdXBw b3J0ZWQoZGV2KSkKPj4+ICsgICAgICAgIGZsYWdzICY9IH5FWFlOT1NfQk9fTk9OQ09OVElHOwo+ PiBTbyB0aGlzIGNvdWxkIGJlIGEgdGVtcGFyYXJpbHkgc29sdXRpb24gdW50aWwgdGhlIG5ldyBm bGFnIGlzIGFkZGVkLCBhbmQgeW91IG1heSBuZWVkIHRvIG1vZGlmeSBhYm92ZSBjb21tZW50cy4K Pj4KPj4gVGhhbmtzLAo+PiBJbmtpIERhZQo+Pgo+Pj4gKwo+Pj4gICAgICAgLyogc2V0IG1lbW9y eSB0eXBlIGFuZCBjYWNoZSBhdHRyaWJ1dGUgZnJvbSB1c2VyIHNpZGUuICovCj4+PiAgICAgICBl eHlub3NfZ2VtLT5mbGFncyA9IGZsYWdzOwo+Pj4gIAo+Pgo+IAo+IEJlc3QgcmVnYXJkcwpfX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFp bGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5m cmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout3.samsung.com ([203.254.224.33]:39788 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751062AbdKMBYO (ORCPT ); Sun, 12 Nov 2017 20:24:14 -0500 MIME-version: 1.0 Content-transfer-encoding: 8BIT Content-type: text/plain; charset="utf-8" Message-id: <5A08F43A.8010204@samsung.com> Date: Mon, 13 Nov 2017 10:24:10 +0900 From: Inki Dae To: Marek Szyprowski , dri-devel@lists.freedesktop.org, linux-samsung-soc@vger.kernel.org Cc: Seung-Woo Kim , Bartlomiej Zolnierkiewicz , Krzysztof Kozlowski , Marian Mihailescu , stable@vger.kernel.org Subject: Re: [PATCH] drm/exynos: gem: Drop NONCONTIG flag for buffers allocated without IOMMU In-reply-to: <065ca47d-11e8-adb3-e264-a874a0b84067@samsung.com> References: <20171031162813.1546-1-m.szyprowski@samsung.com> <5A051757.9020101@samsung.com> <065ca47d-11e8-adb3-e264-a874a0b84067@samsung.com> Sender: stable-owner@vger.kernel.org List-ID: Hi Marek, 2017년 11월 10일 16:35에 Marek Szyprowski 이(가) 쓴 글: > Dear Inki, > > On 2017-11-10 04:04, Inki Dae wrote: >> 2017년 11월 01일 01:28에 Marek Szyprowski 이(가) 쓴 글: >>> When no IOMMU is available, all GEM buffers allocated by Exynos DRM driver >>> are contiguous, because of the underlying dma_alloc_attrs() function >>> provides only such buffers. In such case it makes no sense to keep >> What if user disabled CMA support? In this case, it guarantees also to allocate physically contiguous memory? >> I think it depends on CMA support so wouldn't be true. > > dma_alloc_attrs() always guarantees the contiguity of the allocated memory > in DMA address space. When NOIOMMU is available, this mean that the allocated > buffer is contiguous in the physical memory. When CMA is disabled, > dma_alloc_attrs() uses alloc_pages() allocator. The drawback of alloc_pages() > is that it easily fails if physical memory is fragmented and it cannot > allocate memory larger than MAX_ORDER (4MiB in case of ARM32bit). You are right. Without IOMMU suppot alloc_pages always tryies to allocate contiguous memory, order = get_order(size); page = alloc_pages(..., order); if (!page) return NULL; ... > >> Real problem I think is that user don't know whether the gem buffer allocated with CONTIG or NONCONTIG flag can be used as a SCANOUT buffer. >> So user can request a page flip with NONCONTIG buffer to kernel which doesn't support IOMMU. >> >> And another is that user may want to use NONCONTIG buffer for another purpose, not scanout. >> So if we enforce on using CONTIG buffer on kernel without IOMMU support, then it wouldn't be really what user intended. > > When IOMMU support is disabled, ANY buffer allocated with dma_alloc_attrs() > will be contiguous, so I see no point propagating incorrect flag for it. > > The only way to create a NONCONTIG buffer with IOMMU disabled is to import > it from other driver and this path is already handled correctly. > >> My idea is to provide a new flag - i.e., EXYNOS_BO_SCANOUT - which can allocate a buffer with a different allocation type - CONTIG or NONCONTIG - according to IOMMU support. >> And any page flip request with NONCONTIG buffer to kernel without IOMMU support should fail and it has to return a error with a proper error message. > > I don't think that we need it. With the proposed patch the same userspace will > work fine in both cases IOMMU enabled and disabled, even if it allocate GEM > with NONCONTIG flag. We can assume that CONTIG is a special case of NONCONTIG > then. The problem is really not whether user space will work fine or not but is that this may be not what user space wanted. I.e., user space expects the allocated buffer is NONCONTIG buffer because user space requested NONCONTIG buffer but kernel internally, this is changed to CONTIG buffer. So you could provide some information - maybe warning message?? - to user space the buffer type is changed to CONTIG buffer due to NO IOMMU support. Regarding the buffer type Exynos DRM has, I'm not sure that providing CONTIG and NONCONTIG buffer types to user space is reasonable because I think such buffer types - physically contiguous or non contiguous - should be transparent to user space. And I looked into other ARM DRM drivers - omap and msm provides SCANOUT buffer type, tegra and rockchip have no such buffer types (contiguous or non-contiguous). This is also a thing I felt while working on kernel in collaborating with Platform guys - we made user spaces to care about the memory allocation way. I wonder why you say SCANOUT type is incorrect flag. Thanks, Inki Dae > >>> BO_NONCONTIG flag for the allocated GEM buffers. This allows to avoid >>> failures for buffer contiguity checks in the subsequent operations on GEM >>> objects. >>> >>> Signed-off-by: Marek Szyprowski >>> CC: stable@vger.kernel.org # v4.4+ >>> --- >>> This issue is there since commit 0519f9a12d011 ("drm/exynos: add iommu >>> support for exynos drm framework"), but this patch applies cleanly >>> only to v4.4+ kernel releases due changes in the surrounding code. >>> --- >>> drivers/gpu/drm/exynos/exynos_drm_gem.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c >>> index 02f978bb9261..476c00fe1998 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c >>> @@ -227,6 +227,13 @@ struct exynos_drm_gem *exynos_drm_gem_create(struct drm_device *dev, >>> if (IS_ERR(exynos_gem)) >>> return exynos_gem; >>> + /* >>> + * when no IOMMU is available, all allocated buffers are contiguous >>> + * anyway, so drop EXYNOS_BO_NONCONTIG flag >>> + */ >>> + if (!is_drm_iommu_supported(dev)) >>> + flags &= ~EXYNOS_BO_NONCONTIG; >> So this could be a tempararily solution until the new flag is added, and you may need to modify above comments. >> >> Thanks, >> Inki Dae >> >>> + >>> /* set memory type and cache attribute from user side. */ >>> exynos_gem->flags = flags; >>> >> > > Best regards