Re: [PATCH v2 6/7] iommu/rockchip: use DMA API to map, to flush cache

From: Tomasz Figa <tfiga@google.com>
To: shunqian.zheng@gmail.com
Cc: "Shunqian Zheng" <zhengsq@rock-chips.com>,
	"Mark Rutland" <mark.rutland@arm.com>,
	devicetree@vger.kernel.org, "Heiko Stübner" <heiko@sntech.de>,
	"David Airlie" <airlied@linux.ie>,
	"Joerg Roedel" <joro@8bytes.org>,
	linux@armlinux.org.uk,
	dri-devel <dri-devel@lists.freedesktop.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"open list:ARM/Rockchip SoC..."
	<linux-rockchip@lists.infradead.org>,
	"open list:IOMMU DRIVERS <iommu@lists.linux-foundation.org>,
	Rob Herring <robh+dt@kernel.org>, simon xue <xxm@rock-chips.com>,
	linux-arm-kernel@lists.infradead.org" <mark.yao@rock-chips.com>
Subject: Re: [PATCH v2 6/7] iommu/rockchip: use DMA API to map, to flush cache
Date: Mon, 13 Jun 2016 19:41:26 +0900	[thread overview]
Message-ID: <CAAFQd5AXq9c0X6nQOvbgiBNgJp3UfDNmwC7iQnFGunE=7zdO1A@mail.gmail.com> (raw)
In-Reply-To: <575E8B6F.1040103@gmail.com>

On Mon, Jun 13, 2016 at 7:31 PM, Shunqian Zheng
<shunqian.zheng@gmail.com> wrote:
> HI,
>
>
> On 2016年06月13日 18:21, Tomasz Figa wrote:
>>
>> On Mon, Jun 13, 2016 at 6:56 PM, Shunqian Zheng
>> <shunqian.zheng@gmail.com> wrote:
>>>
>>> Hi
>>>
>>> On 2016年06月10日 17:10, Tomasz Figa wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng <zhengsq@rock-chips.com>
>>>> 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()
>>
>> It shouldn't call iommu_map(), because the IOMMU is not behind another
>> IOMMU. Are you sure you called dma_alloc_coherent() on behalf of the
>> IOMMU struct device and not the DRM device?
>
> I called dma_alloc_coherent() with DRM device but not the IOMMU device,
> because DRM didn't attach to any iommu. Even allocating an virtual one when
> attaching, the iommu->dev
> is DRM device though.
> Am I right here?

What I meant, is that even though rk_iommu_map() is called for DRM
device, the page table allocation happening inside should be called
for the IOMMU device itself, because it's the consumer of these page
tables.

Best regards,
Tomasz