[PATCH v1 0/4] iommu/tegra: gart: Couple corrections

[PATCH v1 0/4] iommu/tegra: gart: Couple corrections

[Dmitry Osipenko]

I've added an experimental support of the GART to the Tegra DRM driver
and it ended up with a very positive result. During the testing of the
GART driver I've noticed couple of its minor shortcomings, so here the
patches to remedy them.

Dmitry Osipenko (4):
  iommu/tegra: gart: Don't unnecessarily allocate registers context
  iommu/tegra: gart: Check whether page is already mapped
  iommu/tegra: gart: Move PFN validation out of spinlock
  iommu/tegra: gart: Correct number of unmapped bytes

 drivers/iommu/tegra-gart.c | 49 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 14 deletions(-)

-- 
2.13.2

[PATCH v1 1/4] iommu/tegra: gart: Don't unnecessarily allocate registers context

[Dmitry Osipenko]

GART looses it's state only in case of a deepest suspend level. Let's not
waste memory if machine doesn't support that suspend level.

Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/iommu/tegra-gart.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index 37e708fdbb5a..1557a6a9a438 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -31,6 +31,8 @@
 #include <linux/iommu.h>
 #include <linux/of.h>
 
+#include <soc/tegra/pmc.h>
+
 #include <asm/cacheflush.h>
 
 /* bitmap of the page sizes currently supported */
@@ -354,10 +356,13 @@ static int tegra_gart_suspend(struct device *dev)
 	u32 *data = gart->savedata;
 	unsigned long flags;
 
-	spin_lock_irqsave(&gart->pte_lock, flags);
-	for_each_gart_pte(gart, iova)
-		*(data++) = gart_read_pte(gart, iova);
-	spin_unlock_irqrestore(&gart->pte_lock, flags);
+	if (gart->savedata != NULL) {
+		spin_lock_irqsave(&gart->pte_lock, flags);
+		for_each_gart_pte(gart, iova)
+			*(data++) = gart_read_pte(gart, iova);
+		spin_unlock_irqrestore(&gart->pte_lock, flags);
+	}
+
 	return 0;
 }
 
@@ -366,9 +371,12 @@ static int tegra_gart_resume(struct device *dev)
 	struct gart_device *gart = dev_get_drvdata(dev);
 	unsigned long flags;
 
-	spin_lock_irqsave(&gart->pte_lock, flags);
-	do_gart_setup(gart, gart->savedata);
-	spin_unlock_irqrestore(&gart->pte_lock, flags);
+	if (gart->savedata != NULL) {
+		spin_lock_irqsave(&gart->pte_lock, flags);
+		do_gart_setup(gart, gart->savedata);
+		spin_unlock_irqrestore(&gart->pte_lock, flags);
+	}
+
 	return 0;
 }
 
@@ -412,10 +420,16 @@ static int tegra_gart_probe(struct platform_device *pdev)
 	gart->iovmm_base = (dma_addr_t)res_remap->start;
 	gart->page_count = (resource_size(res_remap) >> GART_PAGE_SHIFT);
 
-	gart->savedata = vmalloc(sizeof(u32) * gart->page_count);
-	if (!gart->savedata) {
-		dev_err(dev, "failed to allocate context save area\n");
-		return -ENOMEM;
+	/*
+	 * The registers store/restore is required only in case of a
+	 * deepest sleep state, do not waste memory if it's unnecessary.
+	 */
+	if (tegra_pmc_get_suspend_mode() == TEGRA_SUSPEND_LP0) {
+		gart->savedata = vmalloc(sizeof(u32) * gart->page_count);
+		if (!gart->savedata) {
+			dev_err(dev, "failed to allocate context save area\n");
+			return -ENOMEM;
+		}
 	}
 
 	platform_set_drvdata(pdev, gart);
-- 
2.13.2

[PATCH v1 2/4] iommu/tegra: gart: Check whether page is already mapped

[Dmitry Osipenko]

Due to a bug, multiple devices may try to map the same IOVA region. We can
catch that case by checking that 'VALID' bit of the GART's page entry is
unset prior to mapping of the page.

Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/iommu/tegra-gart.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index 1557a6a9a438..54699e341110 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -271,6 +271,7 @@ static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
 	struct gart_device *gart = gart_domain->gart;
 	unsigned long flags;
 	unsigned long pfn;
+	unsigned long pte;
 
 	if (!gart_iova_range_valid(gart, iova, bytes))
 		return -EINVAL;
@@ -282,6 +283,12 @@ static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
 		spin_unlock_irqrestore(&gart->pte_lock, flags);
 		return -EINVAL;
 	}
+	pte = gart_read_pte(gart, iova);
+	if (pte & GART_ENTRY_PHYS_ADDR_VALID) {
+		spin_unlock_irqrestore(&gart->pte_lock, flags);
+		dev_err(gart->dev, "Page entry is used already\n");
+		return -EBUSY;
+	}
 	gart_set_pte(gart, iova, GART_PTE(pfn));
 	FLUSH_GART_REGS(gart);
 	spin_unlock_irqrestore(&gart->pte_lock, flags);
-- 
2.13.2

[PATCH v1 3/4] iommu/tegra: gart: Move PFN validation out of spinlock

[Dmitry Osipenko]

Validation of page frame number doesn't require protection with a spinlock,
let's move it out of spinlock for consistency.

Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/iommu/tegra-gart.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index 54699e341110..55fdb56d85ea 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -276,13 +276,13 @@ static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
 	if (!gart_iova_range_valid(gart, iova, bytes))
 		return -EINVAL;
 
-	spin_lock_irqsave(&gart->pte_lock, flags);
 	pfn = __phys_to_pfn(pa);
 	if (!pfn_valid(pfn)) {
 		dev_err(gart->dev, "Invalid page: %pa\n", &pa);
-		spin_unlock_irqrestore(&gart->pte_lock, flags);
 		return -EINVAL;
 	}
+
+	spin_lock_irqsave(&gart->pte_lock, flags);
 	pte = gart_read_pte(gart, iova);
 	if (pte & GART_ENTRY_PHYS_ADDR_VALID) {
 		spin_unlock_irqrestore(&gart->pte_lock, flags);
-- 
2.13.2

[PATCH v1 4/4] iommu/tegra: gart: Correct number of unmapped bytes

[Dmitry Osipenko]

The iommu_unmap() treats zero bytes number returned by an IOMMU driver as
an indicator that unmapping should be stopped. As a result, GART driver
unmaps only the first page entry of the whole range, which is incorrect.

Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/iommu/tegra-gart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index 55fdb56d85ea..c622f4a4bedd 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -309,7 +309,7 @@ static size_t gart_iommu_unmap(struct iommu_domain *domain, unsigned long iova,
 	gart_set_pte(gart, iova, 0);
 	FLUSH_GART_REGS(gart);
 	spin_unlock_irqrestore(&gart->pte_lock, flags);
-	return 0;
+	return bytes;
 }
 
 static phys_addr_t gart_iommu_iova_to_phys(struct iommu_domain *domain,
-- 
2.13.2

Re: [PATCH v1 0/4] iommu/tegra: gart: Couple corrections

[Joerg Roedel]

On Wed, Jul 05, 2017 at 07:29:44PM +0300, Dmitry Osipenko wrote:
> I've added an experimental support of the GART to the Tegra DRM driver
> and it ended up with a very positive result. During the testing of the
> GART driver I've noticed couple of its minor shortcomings, so here the
> patches to remedy them.
> 
> Dmitry Osipenko (4):
>   iommu/tegra: gart: Don't unnecessarily allocate registers context
>   iommu/tegra: gart: Check whether page is already mapped
>   iommu/tegra: gart: Move PFN validation out of spinlock
>   iommu/tegra: gart: Correct number of unmapped bytes

Patches look good to me (except patch 1, which I am not sure about, but
maybe only due my limited knowledge about the hardware).

So I take them when Thierry gives his Acked-by.



	Joerg

Re: [PATCH v1 0/4] iommu/tegra: gart: Couple corrections

[Dmitry Osipenko]

On 25.07.2017 17:43, Joerg Roedel wrote:
> On Wed, Jul 05, 2017 at 07:29:44PM +0300, Dmitry Osipenko wrote:
>> I've added an experimental support of the GART to the Tegra DRM driver
>> and it ended up with a very positive result. During the testing of the
>> GART driver I've noticed couple of its minor shortcomings, so here the
>> patches to remedy them.
>>
>> Dmitry Osipenko (4):
>>   iommu/tegra: gart: Don't unnecessarily allocate registers context
>>   iommu/tegra: gart: Check whether page is already mapped
>>   iommu/tegra: gart: Move PFN validation out of spinlock
>>   iommu/tegra: gart: Correct number of unmapped bytes
> 
> Patches look good to me (except patch 1, which I am not sure about, but
> maybe only due my limited knowledge about the hardware).
> 
> So I take them when Thierry gives his Acked-by.
> 

Okay, Thierry should get to this patchset eventually.

-- 
Dmitry

Re: [PATCH v1 0/4] iommu/tegra: gart: Couple corrections

[Dmitry Osipenko]

On 29.07.2017 14:04, Dmitry Osipenko wrote:
> On 25.07.2017 17:43, Joerg Roedel wrote:
>> On Wed, Jul 05, 2017 at 07:29:44PM +0300, Dmitry Osipenko wrote:
>>> I've added an experimental support of the GART to the Tegra DRM driver
>>> and it ended up with a very positive result. During the testing of the
>>> GART driver I've noticed couple of its minor shortcomings, so here the
>>> patches to remedy them.
>>>
>>> Dmitry Osipenko (4):
>>>   iommu/tegra: gart: Don't unnecessarily allocate registers context
>>>   iommu/tegra: gart: Check whether page is already mapped
>>>   iommu/tegra: gart: Move PFN validation out of spinlock
>>>   iommu/tegra: gart: Correct number of unmapped bytes
>>
>> Patches look good to me (except patch 1, which I am not sure about, but
>> maybe only due my limited knowledge about the hardware).
>>
>> So I take them when Thierry gives his Acked-by.
>>
> 
> Okay, Thierry should get to this patchset eventually.
> 

BTW, looks like Hiroshi isn't in NVIDIA anymore, his email address isn't valid.
Probably somebody else should take over maintaining of the affected Tegra drivers.

-- 
Dmitry

Re: [PATCH v1 2/4] iommu/tegra: gart: Check whether page is already mapped

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 389 bytes --]

On Wed, Jul 05, 2017 at 07:29:46PM +0300, Dmitry Osipenko wrote:
> Due to a bug, multiple devices may try to map the same IOVA region. We can
> catch that case by checking that 'VALID' bit of the GART's page entry is
> unset prior to mapping of the page.

Due to what bug? Sounds to me like access to the GART should be
exclusive, so that only a single driver can ever access it.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v1 3/4] iommu/tegra: gart: Move PFN validation out of spinlock

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 459 bytes --]

On Wed, Jul 05, 2017 at 07:29:47PM +0300, Dmitry Osipenko wrote:
> Validation of page frame number doesn't require protection with a spinlock,
> let's move it out of spinlock for consistency.
> 
> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/iommu/tegra-gart.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Acked-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 4/4] iommu/tegra: gart: Correct number of unmapped bytes

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 794 bytes --]

On Wed, Jul 05, 2017 at 07:29:48PM +0300, Dmitry Osipenko wrote:
> The iommu_unmap() treats zero bytes number returned by an IOMMU driver as
> an indicator that unmapping should be stopped. As a result, GART driver
> unmaps only the first page entry of the whole range, which is incorrect.
> 
> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/iommu/tegra-gart.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Is this perhaps the bug that you were referring to in an earlier patch?
Looks to me like you'd run into that bug everytime you try mapping an
IOVA that wasn't properly unmapped before.

Anyway, this looks like the right thing to do:

Acked-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 1/4] iommu/tegra: gart: Don't unnecessarily allocate registers context

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

On Wed, Jul 05, 2017 at 07:29:45PM +0300, Dmitry Osipenko wrote:
> GART looses it's state only in case of a deepest suspend level. Let's not
> waste memory if machine doesn't support that suspend level.
> 
> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/iommu/tegra-gart.c | 36 +++++++++++++++++++++++++-----------
>  1 file changed, 25 insertions(+), 11 deletions(-)

I'm not sure about this. The savedata region uses 32 KiB of memory,
which is really not a whole lot.

In addition, the suspend mode can be set in different places during the
boot process, some of which fairly late. I'm concerned that this boot
order dependency is going to come back to bite us eventually.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 2/4] iommu/tegra: gart: Check whether page is already mapped

[Dmitry Osipenko]

On 26.09.2017 14:06, Thierry Reding wrote:
> On Wed, Jul 05, 2017 at 07:29:46PM +0300, Dmitry Osipenko wrote:
>> Due to a bug, multiple devices may try to map the same IOVA region. We can
>> catch that case by checking that 'VALID' bit of the GART's page entry is
>> unset prior to mapping of the page.
> 
> Due to what bug? Sounds to me like access to the GART should be
> exclusive, so that only a single driver can ever access it.
> 

Actually, there are a lot of peripherals behind the GART. But yes, probably we
would use it exclusively for the GPU allocations.

In a case of the GPU allocations there could be a bug in the allocation code
(drm_mm_scan) that would cause re-mapping of the already mapped pages, we would
be able to catch such a bug.

-- 
Dmitry

Re: [PATCH v1 1/4] iommu/tegra: gart: Don't unnecessarily allocate registers context

[Dmitry Osipenko]

On 26.09.2017 14:19, Thierry Reding wrote:
> On Wed, Jul 05, 2017 at 07:29:45PM +0300, Dmitry Osipenko wrote:
>> GART looses it's state only in case of a deepest suspend level. Let's not
>> waste memory if machine doesn't support that suspend level.
>>
>> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/iommu/tegra-gart.c | 36 +++++++++++++++++++++++++-----------
>>  1 file changed, 25 insertions(+), 11 deletions(-)
> 
> I'm not sure about this. The savedata region uses 32 KiB of memory,
> which is really not a whole lot.
> 
> In addition, the suspend mode can be set in different places during the
> boot process, some of which fairly late. I'm concerned that this boot
> order dependency is going to come back to bite us eventually.
> 

Indeed, suspend mode is set after drivers been probed, my bad.

On the other hand LP0 support haven't been implemented for years, some of
drivers do not implement suspend/resume. So maybe it's worth to give up on LP0
entirely...

-- 
Dmitry

Re: [PATCH v1 2/4] iommu/tegra: gart: Check whether page is already mapped

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1907 bytes --]

On Tue, Sep 26, 2017 at 04:49:52PM +0300, Dmitry Osipenko wrote:
> On 26.09.2017 14:06, Thierry Reding wrote:
> > On Wed, Jul 05, 2017 at 07:29:46PM +0300, Dmitry Osipenko wrote:
> >> Due to a bug, multiple devices may try to map the same IOVA region. We can
> >> catch that case by checking that 'VALID' bit of the GART's page entry is
> >> unset prior to mapping of the page.
> > 
> > Due to what bug? Sounds to me like access to the GART should be
> > exclusive, so that only a single driver can ever access it.
> > 
> 
> Actually, there are a lot of peripherals behind the GART. But yes, probably we
> would use it exclusively for the GPU allocations.
> 
> In a case of the GPU allocations there could be a bug in the allocation code
> (drm_mm_scan) that would cause re-mapping of the already mapped pages, we would
> be able to catch such a bug.

Sounds to me like something that should be opt-in. Given that we could
potentially map a lot of memory, the additional read seems like it could
incur a significant performance penalty.

How about protecting this by some Kconfig symbol (or debug module
parameter) and adding a WARN instead of failing the mapping operation?

The reason is that we really should be using the GART from a single
driver because it is a shared memory region. To avoid drivers from
treading on each others' feet the allocations must be managed by some
central entity. So either something like DMA API should manage it, or
we hardwire it to Tegra DRM explicitly.

And if a central allocator is responsible for all mappings through the
GART, then we should assume that it isn't buggy, and therefore the extra
check of the PTEs should not be needed in most cases, hence we can get a
performance boost by disabling the check by default.

Of course if there is not much performance impact, maybe it is okay to
keep it always enabled.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 2/4] iommu/tegra: gart: Check whether page is already mapped

[Dmitry Osipenko]

On 26.09.2017 19:07, Thierry Reding wrote:
> On Tue, Sep 26, 2017 at 04:49:52PM +0300, Dmitry Osipenko wrote:
>> On 26.09.2017 14:06, Thierry Reding wrote:
>>> On Wed, Jul 05, 2017 at 07:29:46PM +0300, Dmitry Osipenko wrote:
>>>> Due to a bug, multiple devices may try to map the same IOVA region. We can
>>>> catch that case by checking that 'VALID' bit of the GART's page entry is
>>>> unset prior to mapping of the page.
>>>
>>> Due to what bug? Sounds to me like access to the GART should be
>>> exclusive, so that only a single driver can ever access it.
>>>
>>
>> Actually, there are a lot of peripherals behind the GART. But yes, probably we
>> would use it exclusively for the GPU allocations.
>>
>> In a case of the GPU allocations there could be a bug in the allocation code
>> (drm_mm_scan) that would cause re-mapping of the already mapped pages, we would
>> be able to catch such a bug.
> 
> Sounds to me like something that should be opt-in. Given that we could
> potentially map a lot of memory, the additional read seems like it could
> incur a significant performance penalty.
> 
> How about protecting this by some Kconfig symbol (or debug module
> parameter) and adding a WARN instead of failing the mapping operation?
> 
> The reason is that we really should be using the GART from a single
> driver because it is a shared memory region. To avoid drivers from
> treading on each others' feet the allocations must be managed by some
> central entity. So either something like DMA API should manage it, or
> we hardwire it to Tegra DRM explicitly.
> 
> And if a central allocator is responsible for all mappings through the
> GART, then we should assume that it isn't buggy, and therefore the extra
> check of the PTEs should not be needed in most cases, hence we can get a
> performance boost by disabling the check by default.
> 
> Of course if there is not much performance impact, maybe it is okay to
> keep it always enabled.
> 

I had a similar thought about making this checking and the "proper" unmapping
optional, but decided that it is not very worthy to implement it now because
caching of mappings helps to get rid of all IOVA expenses. The "checking" is
nearly "free" in terms of performance, it's the unmapping that causes measurable
performance impact. Maybe the improper unmapping was made intentionally for
better performance.

Given that you've made suggestion to make the checking optional, I'm going to
update this patchset, adding a new "debug" Kconfig option.

-- 
Dmitry