From mboxrd@z Thu Jan 1 00:00:00 1970 From: Robin Murphy Subject: Re: [PATCH v2 6/7] iommu/rockchip: use DMA API to map, to flush cache Date: Mon, 13 Jun 2016 11:39:26 +0100 Message-ID: <575E8D5E.9090400@arm.com> References: <1465392392-2003-1-git-send-email-zhengsq@rock-chips.com> <1465392392-2003-7-git-send-email-zhengsq@rock-chips.com> <575E834C.30305@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <575E834C.30305-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: shunqian.zheng-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, Tomasz Figa , Shunqian Zheng Cc: Mark Rutland , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, =?UTF-8?Q?Heiko_St=c3=bcbner?= , David Airlie , linux-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org, dri-devel , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "open list:ARM/Rockchip SoC..." , IOMMU DRIVERS , Rob Herring , simon xue , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" , =?UTF-8?B?5aea5pm65oOF?= List-Id: iommu@lists.linux-foundation.org T24gMTMvMDYvMTYgMTA6NTYsIFNodW5xaWFuIFpoZW5nIHdyb3RlOgo+IEhpCj4KPiBPbiAyMDE2 5bm0MDbmnIgxMOaXpSAxNzoxMCwgVG9tYXN6IEZpZ2Egd3JvdGU6Cj4+IEhpLAo+Pgo+PiBPbiBX ZWQsIEp1biA4LCAyMDE2IGF0IDEwOjI2IFBNLCBTaHVucWlhbiBaaGVuZwo+PiA8emhlbmdzcUBy b2NrLWNoaXBzLmNvbT4gd3JvdGU6Cj4+PiBVc2UgRE1BIEFQSSBpbnN0ZWFkIG9mIGFyY2hpdGVj dHVyZSBpbnRlcm5hbCBmdW5jdGlvbnMgbGlrZQo+Pj4gX19jcHVjX2ZsdXNoX2RjYWNoZV9hcmVh KCkgZXRjLgo+Pj4KPj4+IFRvIHN1cHBvcnQgdGhlIHZpcnR1YWwgZGV2aWNlIGxpa2UgRFJNIHRo ZSB2aXJ0dWFsIHNsYXZlIGlvbW11Cj4+PiBhZGRlZCBpbiB0aGUgcHJldmlvdXMgcGF0Y2gsIGF0 dGFjaGluZyB0byB3aGljaCB0aGUgRFJNIGNhbiB1c2UKPj4+IGl0IG93biBkb21haW4tPmRldiBm b3IgZG1hX21hcF8qKCksIGRtYV9zeW5jXyooKSBldmVuIFZPUCBpcyBkaXNhYmxlZC4KPj4+Cj4+ PiBXaXRoIHRoaXMgcGF0Y2gsIHRoaXMgZHJpdmVyIGlzIGF2YWlsYWJsZSBmb3IgQVJNNjQgbGlr ZSBSSzMzOTkuCj4+Pgo+PiBDb3VsZCB3ZSBpbnN0ZWFkIHNpbXBseSBhbGxvY2F0ZSBjb2hlcmVu dCBtZW1vcnkgZm9yIHBhZ2UgdGFibGVzIHVzaW5nCj4+IGRtYV9hbGxvY19jb2hlcmVudCgpIGFu ZCBza2lwIGFueSBmbHVzaGluZyBvbiBDUFUgc2lkZSBjb21wbGV0ZWx5PyBJZgo+PiBJJ20gbG9v a2luZyBjb3JyZWN0bHksIHRoZSBkcml2ZXIgb25seSByZWFkcyBiYWNrIHRoZSBwYWdlIGRpcmVj dG9yeQo+PiB3aGVuIGNoZWNraW5nIGlmIHRoZXJlIGlzIGEgbmVlZCB0byBhbGxvY2F0ZSBuZXcg cGFnZSB0YWJsZSwgc28gdGhlcmUKPj4gc2hvdWxkbid0IGJlIGFueSBzaWduaWZpY2FudCBwZW5h bHR5IGZvciBkaXNhYmxpbmcgdGhlIGNhY2hlLgo+IEkgdHJ5IHRvIHVzZSBkbWFfYWxsb2NfY29o ZXJlbnQoKSB0byByZXBsYWNlIHRoZSBkbWFfbWFwX3NpbmdsZSgpLAo+IGJ1dCBpdCBkb2Vzbid0 IHdvcmsgZm9yIG1lIHByb3Blcmx5Lgo+IEJlY2F1c2UgdGhlIERSTSB1c2VzIHRoZSBpb21tdV9k bWFfb3BzIGluc3RlYWQgdGhlIHN3aW90bGJfZG1hX29wcyBhZnRlcgo+IGF0dGFjaGluZwo+IHRv IGlvbW11LCBzbyB3aGVuIHRoZSBpb21tdSBkb21haW4gbmVlZCB0byBhbGxvYyBhIG5ldyBwYWdl IGluCj4gcmtfaW9tbXVfbWFwKCksCj4gaXQgd291bGQgY2FsbDoKPiBya19pb21tdV9tYXAoKSAg LS0+IGRtYV9hbGxvY19jb2hlcmVudCgpICAtLT4gb3BzLT5hbGxvYygpICAtLT4KPiBpb21tdV9t YXAoKSAtLT4gcmtfaW9tbXVfbWFwKCkKClRoYXQgc291bmRzIG1vcmUgbGlrZSB5b3UncmUgcGFz c2luZyB0aGUgd3JvbmcgZGV2aWNlIGFyb3VuZCBzb21ld2hlcmUsIApzaW5jZSB0aGlzIGFwcHJv YWNoIGlzIHdvcmtpbmcgZmluZSBmb3Igb3RoZXIgSU9NTVVzOyBzcGVjaWZpY2FsbHksIHRoZSAK ZmxvdyBnb2VzOgoKZG1hX2FsbG9jX2NvaGVyZW50KERSTSBkZXYpICAgLy8gZm9yIGJ1ZmZlcgot LT4gb3BzLT5hbGxvYyhEUk0gZGV2KQogICAgIC0tPiBpb21tdV9kbWFfYWxsb2MoRFJNIGRldikK ICAgICAgICAgLS0+IGlvbW11X21hcCgpCiAgICAgICAgICAgICAtLT4gZG1hX2FsbG9jX2NvaGVy ZW50KElPTU1VIGRldikgIC8vIGZvciBwYWdldGFibGUKICAgICAgICAgICAgICAgICAtLT4gb3Bz LT5hbGxvYyhJT01NVSBkZXYpCiAgICAgICAgICAgICAgICAgICAgIC0tPiBzd2lvdGxiX2FsbG9j KElPTU1VIGRldikKClRoZXJlIHNob3VsZG4ndCBiZSBhbnkgbmVlZCBmb3IgdGhpcyAidmlydHVh bCBJT01NVSIgYXQgYWxsLiBJIHRoaW5rIHRoZSAKRXh5bm9zIERSTSBkcml2ZXIgaXMgaW4gYSBz aW1pbGFyIHNpdHVhdGlvbiBvZiBoYXZpbmcgbXVsdGlwbGUgZGV2aWNlcyAKKGludm9sdmluZyBt dWx0aXBsZSBJT01NVXMpIGJhY2tpbmcgdGhlIHZpcnR1YWwgRFJNIGRldmljZSwgYW5kIHRoYXQg CmRvZXNuJ3Qgc2VlbSB0byBiZSBkb2luZyBhbnl0aGluZyB0aGlzIGNyYXp5IHNvIGl0J3MgcHJv YmFibHkgd29ydGggCnRha2luZyBhIGxvb2sgYXQuCgpSb2Jpbi4KCj4gVGhlbiBJIHRyeSB0byBy ZXNlcnZlIG1lbW9yeSBmb3IgY29oZXJlbnQgc28gdGhhdCwgZG1hX2FsbG9jX2NvaGVyZW50KCkK PiBjYWxscyBkbWFfYWxsb2NfZnJvbV9jb2hlcmVudCgpCj4gYnV0IG5vdCBvcHMtPmFsbG9jKCku IEJ1dCBpdCBkb2Vzbid0IHdvcmsgdG9vIGJlY2F1c2Ugd2hlbiBEUk0gcmVxdWVzdAo+IGJ1ZmZl ciBpdCBuZXZlciB1c2VzIGlvbW11Lgo+Pgo+PiBPdGhlciB0aGFuIHRoYXQsIHBsZWFzZSBzZWUg c29tZSBjb21tZW50cyBpbmxpbmUuCj4+Cj4+PiBTaWduZWQtb2ZmLWJ5OiBTaHVucWlhbiBaaGVu ZyA8emhlbmdzcUByb2NrLWNoaXBzLmNvbT4KPj4+IC0tLQo+Pj4gICBkcml2ZXJzL2lvbW11L3Jv Y2tjaGlwLWlvbW11LmMgfCAxMTMKPj4+ICsrKysrKysrKysrKysrKysrKysrKysrKysrLS0tLS0t LS0tLS0tLS0tCj4+PiAgIDEgZmlsZSBjaGFuZ2VkLCA3MSBpbnNlcnRpb25zKCspLCA0MiBkZWxl dGlvbnMoLSkKPj4+Cj4+PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9pb21tdS9yb2NrY2hpcC1pb21t dS5jCj4+PiBiL2RyaXZlcnMvaW9tbXUvcm9ja2NoaXAtaW9tbXUuYwo+Pj4gaW5kZXggZDZjMzA1 MS4uYWFmZWE2ZSAxMDA2NDQKPj4+IC0tLSBhL2RyaXZlcnMvaW9tbXUvcm9ja2NoaXAtaW9tbXUu Ywo+Pj4gKysrIGIvZHJpdmVycy9pb21tdS9yb2NrY2hpcC1pb21tdS5jCj4+PiBAQCAtNCw4ICs0 LDYgQEAKPj4+ICAgICogcHVibGlzaGVkIGJ5IHRoZSBGcmVlIFNvZnR3YXJlIEZvdW5kYXRpb24u Cj4+PiAgICAqLwo+Pj4KPj4+IC0jaW5jbHVkZSA8YXNtL2NhY2hlZmx1c2guaD4KPj4+IC0jaW5j bHVkZSA8YXNtL3BndGFibGUuaD4KPj4+ICAgI2luY2x1ZGUgPGxpbnV4L2NvbXBpbGVyLmg+Cj4+ PiAgICNpbmNsdWRlIDxsaW51eC9kZWxheS5oPgo+Pj4gICAjaW5jbHVkZSA8bGludXgvZGV2aWNl Lmg+Cj4+PiBAQCAtNjEsOCArNTksNyBAQAo+Pj4gICAjZGVmaW5lIFJLX01NVV9JUlFfQlVTX0VS Uk9SICAgICAweDAyICAvKiBidXMgcmVhZCBlcnJvciAqLwo+Pj4gICAjZGVmaW5lIFJLX01NVV9J UlFfTUFTSyAgICAgICAgICAoUktfTU1VX0lSUV9QQUdFX0ZBVUxUIHwKPj4+IFJLX01NVV9JUlFf QlVTX0VSUk9SKQo+Pj4KPj4+IC0jZGVmaW5lIE5VTV9EVF9FTlRSSUVTIDEwMjQKPj4+IC0jZGVm aW5lIE5VTV9QVF9FTlRSSUVTIDEwMjQKPj4+ICsjZGVmaW5lIE5VTV9UTEJfRU5UUklFUyAxMDI0 IC8qIGZvciBib3RoIERUIGFuZCBQVCAqLwo+PiBJcyBpdCBuZWNlc3NhcnkgdG8gY2hhbmdlIHRo aXMgaW4gdGhpcyBwYXRjaD8gSW4gZ2VuZXJhbCwgaXQncyBub3QgYQo+PiBnb29kIGlkZWEgdG8g bWl4IG11bHRpcGxlIGxvZ2ljYWwgY2hhbmdlcyB0b2dldGhlci4KPiBTdXJlLCB3aWxsIHJlc3Rv cmUgY2hhbmdlcyBpbiB2My4KPj4KPj4+ICAgI2RlZmluZSBTUEFHRV9PUkRFUiAxMgo+Pj4gICAj ZGVmaW5lIFNQQUdFX1NJWkUgKDEgPDwgU1BBR0VfT1JERVIpCj4+PiBAQCAtODIsNyArNzksOSBA QAo+Pj4KPj4+ICAgc3RydWN0IHJrX2lvbW11X2RvbWFpbiB7Cj4+PiAgICAgICAgICBzdHJ1Y3Qg bGlzdF9oZWFkIGlvbW11czsKPj4+ICsgICAgICAgc3RydWN0IGRldmljZSAqZGV2Owo+Pj4gICAg ICAgICAgdTMyICpkdDsgLyogcGFnZSBkaXJlY3RvcnkgdGFibGUgKi8KPj4+ICsgICAgICAgZG1h X2FkZHJfdCBkdF9kbWE7Cj4+PiAgICAgICAgICBzcGlubG9ja190IGlvbW11c19sb2NrOyAvKiBs b2NrIGZvciBpb21tdXMgbGlzdCAqLwo+Pj4gICAgICAgICAgc3BpbmxvY2tfdCBkdF9sb2NrOyAv KiBsb2NrIGZvciBtb2RpZnlpbmcgcGFnZSBkaXJlY3RvcnkKPj4+IHRhYmxlICovCj4+Pgo+Pj4g QEAgLTk4LDE0ICs5NywxMiBAQCBzdHJ1Y3QgcmtfaW9tbXUgewo+Pj4gICAgICAgICAgc3RydWN0 IGlvbW11X2RvbWFpbiAqZG9tYWluOyAvKiBkb21haW4gdG8gd2hpY2ggaW9tbXUgaXMKPj4+IGF0 dGFjaGVkICovCj4+PiAgIH07Cj4+Pgo+Pj4gLXN0YXRpYyBpbmxpbmUgdm9pZCBya190YWJsZV9m bHVzaCh1MzIgKnZhLCB1bnNpZ25lZCBpbnQgY291bnQpCj4+PiArc3RhdGljIGlubGluZSB2b2lk IHJrX3RhYmxlX2ZsdXNoKHN0cnVjdCBkZXZpY2UgKmRldiwgZG1hX2FkZHJfdCBkbWEsCj4+PiAr ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgdW5zaWduZWQgaW50IGNvdW50KQo+Pj4g ICB7Cj4+PiAtICAgICAgIHBoeXNfYWRkcl90IHBhX3N0YXJ0ID0gdmlydF90b19waHlzKHZhKTsK Pj4+IC0gICAgICAgcGh5c19hZGRyX3QgcGFfZW5kID0gdmlydF90b19waHlzKHZhICsgY291bnQp Owo+Pj4gLSAgICAgICBzaXplX3Qgc2l6ZSA9IHBhX2VuZCAtIHBhX3N0YXJ0Owo+Pj4gKyAgICAg ICBzaXplX3Qgc2l6ZSA9IGNvdW50ICogNDsKPj4gSXQgd291bGQgYmUgYSBnb29kIGlkZWEgdG8g c3BlY2lmeSB3aGF0ICJjb3VudCIgaXMuIEknbSBhIGJpdCBjb25mdXNlZAo+PiB0aGF0IGJlZm9y ZSBpdCBtZWFudCBieXRlcyBhbmQgbm93IHNvbWUgbXVsdGlwbGUgb2YgND8KPiAiY291bnQiIG1l YW5zIFBUL0RUIGVudHJ5IGNvdW50IHRvIGZsdXNoIGhlcmUuIEkgd291bGQgYWRkIHNvbWUgbW9y ZQo+IGNvbW1lbnQgb24gaXQuCj4KPiBUaGFuayB5b3UgdmVyeSBtdWNoLAo+IFNodW5xaWFuCj4+ Cj4+IEJlc3QgcmVnYXJkcywKPj4gVG9tYXN6Cj4+Cj4+IF9fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fCj4+IExpbnV4LXJvY2tjaGlwIG1haWxpbmcgbGlzdAo+ PiBMaW51eC1yb2NrY2hpcEBsaXN0cy5pbmZyYWRlYWQub3JnCj4+IGh0dHA6Ly9saXN0cy5pbmZy YWRlYWQub3JnL21haWxtYW4vbGlzdGluZm8vbGludXgtcm9ja2NoaXAKPgo+IF9fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCj4gaW9tbXUgbWFpbGluZyBsaXN0 Cj4gaW9tbXVAbGlzdHMubGludXgtZm91bmRhdGlvbi5vcmcKPiBodHRwczovL2xpc3RzLmxpbnV4 Zm91bmRhdGlvbi5vcmcvbWFpbG1hbi9saXN0aW5mby9pb21tdQoKX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX18KaW9tbXUgbWFpbGluZyBsaXN0CmlvbW11QGxp c3RzLmxpbnV4LWZvdW5kYXRpb24ub3JnCmh0dHBzOi8vbGlzdHMubGludXhmb3VuZGF0aW9uLm9y Zy9tYWlsbWFuL2xpc3RpbmZvL2lvbW11 From mboxrd@z Thu Jan 1 00:00:00 1970 From: robin.murphy@arm.com (Robin Murphy) Date: Mon, 13 Jun 2016 11:39:26 +0100 Subject: [PATCH v2 6/7] iommu/rockchip: use DMA API to map, to flush cache In-Reply-To: <575E834C.30305@gmail.com> References: <1465392392-2003-1-git-send-email-zhengsq@rock-chips.com> <1465392392-2003-7-git-send-email-zhengsq@rock-chips.com> <575E834C.30305@gmail.com> Message-ID: <575E8D5E.9090400@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 13/06/16 10:56, Shunqian Zheng wrote: > Hi > > On 2016?06?10? 17:10, Tomasz Figa wrote: >> Hi, >> >> On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng >> wrote: >>> Use DMA API instead of architecture internal functions like >>> __cpuc_flush_dcache_area() etc. >>> >>> To support the virtual device like DRM the virtual slave iommu >>> added in the previous patch, attaching to which the DRM can use >>> it own domain->dev for dma_map_*(), dma_sync_*() even VOP is disabled. >>> >>> With this patch, this driver is available for ARM64 like RK3399. >>> >> Could we instead simply allocate coherent memory for page tables using >> dma_alloc_coherent() and skip any flushing on CPU side completely? If >> I'm looking correctly, the driver only reads back the page directory >> when checking if there is a need to allocate new page table, so there >> shouldn't be any significant penalty for disabling the cache. > I try to use dma_alloc_coherent() to replace the dma_map_single(), > but it doesn't work for me properly. > Because the DRM uses the iommu_dma_ops instead the swiotlb_dma_ops after > attaching > to iommu, so when the iommu domain need to alloc a new page in > rk_iommu_map(), > it would call: > rk_iommu_map() --> dma_alloc_coherent() --> ops->alloc() --> > iommu_map() --> rk_iommu_map() That sounds more like you're passing the wrong device around somewhere, since this approach is working fine for other IOMMUs; specifically, the flow goes: dma_alloc_coherent(DRM dev) // for buffer --> ops->alloc(DRM dev) --> iommu_dma_alloc(DRM dev) --> iommu_map() --> dma_alloc_coherent(IOMMU dev) // for pagetable --> ops->alloc(IOMMU dev) --> swiotlb_alloc(IOMMU dev) There shouldn't be any need for this "virtual IOMMU" at all. I think the Exynos DRM driver is in a similar situation of having multiple devices (involving multiple IOMMUs) backing the virtual DRM device, and that doesn't seem to be doing anything this crazy so it's probably worth taking a look at. Robin. > Then I try to reserve memory for coherent so that, dma_alloc_coherent() > calls dma_alloc_from_coherent() > but not ops->alloc(). But it doesn't work too because when DRM request > buffer it never uses iommu. >> >> Other than that, please see some comments inline. >> >>> Signed-off-by: Shunqian Zheng >>> --- >>> drivers/iommu/rockchip-iommu.c | 113 >>> ++++++++++++++++++++++++++--------------- >>> 1 file changed, 71 insertions(+), 42 deletions(-) >>> >>> diff --git a/drivers/iommu/rockchip-iommu.c >>> b/drivers/iommu/rockchip-iommu.c >>> index d6c3051..aafea6e 100644 >>> --- a/drivers/iommu/rockchip-iommu.c >>> +++ b/drivers/iommu/rockchip-iommu.c >>> @@ -4,8 +4,6 @@ >>> * published by the Free Software Foundation. >>> */ >>> >>> -#include >>> -#include >>> #include >>> #include >>> #include >>> @@ -61,8 +59,7 @@ >>> #define RK_MMU_IRQ_BUS_ERROR 0x02 /* bus read error */ >>> #define RK_MMU_IRQ_MASK (RK_MMU_IRQ_PAGE_FAULT | >>> RK_MMU_IRQ_BUS_ERROR) >>> >>> -#define NUM_DT_ENTRIES 1024 >>> -#define NUM_PT_ENTRIES 1024 >>> +#define NUM_TLB_ENTRIES 1024 /* for both DT and PT */ >> Is it necessary to change this in this patch? In general, it's not a >> good idea to mix multiple logical changes together. > Sure, will restore changes in v3. >> >>> #define SPAGE_ORDER 12 >>> #define SPAGE_SIZE (1 << SPAGE_ORDER) >>> @@ -82,7 +79,9 @@ >>> >>> struct rk_iommu_domain { >>> struct list_head iommus; >>> + struct device *dev; >>> u32 *dt; /* page directory table */ >>> + dma_addr_t dt_dma; >>> spinlock_t iommus_lock; /* lock for iommus list */ >>> spinlock_t dt_lock; /* lock for modifying page directory >>> table */ >>> >>> @@ -98,14 +97,12 @@ struct rk_iommu { >>> struct iommu_domain *domain; /* domain to which iommu is >>> attached */ >>> }; >>> >>> -static inline void rk_table_flush(u32 *va, unsigned int count) >>> +static inline void rk_table_flush(struct device *dev, dma_addr_t dma, >>> + unsigned int count) >>> { >>> - phys_addr_t pa_start = virt_to_phys(va); >>> - phys_addr_t pa_end = virt_to_phys(va + count); >>> - size_t size = pa_end - pa_start; >>> + size_t size = count * 4; >> It would be a good idea to specify what "count" is. I'm a bit confused >> that before it meant bytes and now some multiple of 4? > "count" means PT/DT entry count to flush here. I would add some more > comment on it. > > Thank you very much, > Shunqian >> >> Best regards, >> Tomasz >> >> _______________________________________________ >> Linux-rockchip mailing list >> Linux-rockchip at lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-rockchip > > _______________________________________________ > iommu mailing list > iommu at lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1422813AbcFMKjd (ORCPT ); Mon, 13 Jun 2016 06:39:33 -0400 Received: from foss.arm.com ([217.140.101.70]:50600 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964910AbcFMKjb (ORCPT ); Mon, 13 Jun 2016 06:39:31 -0400 Subject: Re: [PATCH v2 6/7] iommu/rockchip: use DMA API to map, to flush cache To: shunqian.zheng@gmail.com, Tomasz Figa , Shunqian Zheng References: <1465392392-2003-1-git-send-email-zhengsq@rock-chips.com> <1465392392-2003-7-git-send-email-zhengsq@rock-chips.com> <575E834C.30305@gmail.com> Cc: Mark Rutland , devicetree@vger.kernel.org, =?UTF-8?Q?Heiko_St=c3=bcbner?= , David Airlie , linux@armlinux.org.uk, dri-devel , "linux-kernel@vger.kernel.org" , "open list:ARM/Rockchip SoC..." , IOMMU DRIVERS , Rob Herring , =?UTF-8?B?5aea5pm65oOF?= , "linux-arm-kernel@lists.infradead.org" , simon xue From: Robin Murphy Message-ID: <575E8D5E.9090400@arm.com> Date: Mon, 13 Jun 2016 11:39:26 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <575E834C.30305@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 13/06/16 10:56, Shunqian Zheng wrote: > Hi > > On 2016年06月10日 17:10, Tomasz Figa wrote: >> Hi, >> >> On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng >> wrote: >>> Use DMA API instead of architecture internal functions like >>> __cpuc_flush_dcache_area() etc. >>> >>> To support the virtual device like DRM the virtual slave iommu >>> added in the previous patch, attaching to which the DRM can use >>> it own domain->dev for dma_map_*(), dma_sync_*() even VOP is disabled. >>> >>> With this patch, this driver is available for ARM64 like RK3399. >>> >> Could we instead simply allocate coherent memory for page tables using >> dma_alloc_coherent() and skip any flushing on CPU side completely? If >> I'm looking correctly, the driver only reads back the page directory >> when checking if there is a need to allocate new page table, so there >> shouldn't be any significant penalty for disabling the cache. > I try to use dma_alloc_coherent() to replace the dma_map_single(), > but it doesn't work for me properly. > Because the DRM uses the iommu_dma_ops instead the swiotlb_dma_ops after > attaching > to iommu, so when the iommu domain need to alloc a new page in > rk_iommu_map(), > it would call: > rk_iommu_map() --> dma_alloc_coherent() --> ops->alloc() --> > iommu_map() --> rk_iommu_map() That sounds more like you're passing the wrong device around somewhere, since this approach is working fine for other IOMMUs; specifically, the flow goes: dma_alloc_coherent(DRM dev) // for buffer --> ops->alloc(DRM dev) --> iommu_dma_alloc(DRM dev) --> iommu_map() --> dma_alloc_coherent(IOMMU dev) // for pagetable --> ops->alloc(IOMMU dev) --> swiotlb_alloc(IOMMU dev) There shouldn't be any need for this "virtual IOMMU" at all. I think the Exynos DRM driver is in a similar situation of having multiple devices (involving multiple IOMMUs) backing the virtual DRM device, and that doesn't seem to be doing anything this crazy so it's probably worth taking a look at. Robin. > Then I try to reserve memory for coherent so that, dma_alloc_coherent() > calls dma_alloc_from_coherent() > but not ops->alloc(). But it doesn't work too because when DRM request > buffer it never uses iommu. >> >> Other than that, please see some comments inline. >> >>> Signed-off-by: Shunqian Zheng >>> --- >>> drivers/iommu/rockchip-iommu.c | 113 >>> ++++++++++++++++++++++++++--------------- >>> 1 file changed, 71 insertions(+), 42 deletions(-) >>> >>> diff --git a/drivers/iommu/rockchip-iommu.c >>> b/drivers/iommu/rockchip-iommu.c >>> index d6c3051..aafea6e 100644 >>> --- a/drivers/iommu/rockchip-iommu.c >>> +++ b/drivers/iommu/rockchip-iommu.c >>> @@ -4,8 +4,6 @@ >>> * published by the Free Software Foundation. >>> */ >>> >>> -#include >>> -#include >>> #include >>> #include >>> #include >>> @@ -61,8 +59,7 @@ >>> #define RK_MMU_IRQ_BUS_ERROR 0x02 /* bus read error */ >>> #define RK_MMU_IRQ_MASK (RK_MMU_IRQ_PAGE_FAULT | >>> RK_MMU_IRQ_BUS_ERROR) >>> >>> -#define NUM_DT_ENTRIES 1024 >>> -#define NUM_PT_ENTRIES 1024 >>> +#define NUM_TLB_ENTRIES 1024 /* for both DT and PT */ >> Is it necessary to change this in this patch? In general, it's not a >> good idea to mix multiple logical changes together. > Sure, will restore changes in v3. >> >>> #define SPAGE_ORDER 12 >>> #define SPAGE_SIZE (1 << SPAGE_ORDER) >>> @@ -82,7 +79,9 @@ >>> >>> struct rk_iommu_domain { >>> struct list_head iommus; >>> + struct device *dev; >>> u32 *dt; /* page directory table */ >>> + dma_addr_t dt_dma; >>> spinlock_t iommus_lock; /* lock for iommus list */ >>> spinlock_t dt_lock; /* lock for modifying page directory >>> table */ >>> >>> @@ -98,14 +97,12 @@ struct rk_iommu { >>> struct iommu_domain *domain; /* domain to which iommu is >>> attached */ >>> }; >>> >>> -static inline void rk_table_flush(u32 *va, unsigned int count) >>> +static inline void rk_table_flush(struct device *dev, dma_addr_t dma, >>> + unsigned int count) >>> { >>> - phys_addr_t pa_start = virt_to_phys(va); >>> - phys_addr_t pa_end = virt_to_phys(va + count); >>> - size_t size = pa_end - pa_start; >>> + size_t size = count * 4; >> It would be a good idea to specify what "count" is. I'm a bit confused >> that before it meant bytes and now some multiple of 4? > "count" means PT/DT entry count to flush here. I would add some more > comment on it. > > Thank you very much, > Shunqian >> >> Best regards, >> Tomasz >> >> _______________________________________________ >> Linux-rockchip mailing list >> Linux-rockchip@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-rockchip > > _______________________________________________ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu