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

Message ID df570fb6649da6bc3b5f2a68afcb5471e6148269.1499270277.git.digetx@gmail.com
State New
Headers show

Commit Message

Dmitry Osipenko July 5, 2017, 4:29 p.m.
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@gmail.com>
---
 drivers/iommu/tegra-gart.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

Comments

Thierry Reding Sept. 26, 2017, 11:19 a.m. | #1
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@gmail.com>
> ---
>  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
Dmitry Osipenko Sept. 26, 2017, 2:15 p.m. | #2
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@gmail.com>
>> ---
>>  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...

Patch

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);