[PATCH 00/10] drm/gma500: Various cleanups to GEM code

[PATCH 00/10] drm/gma500: Various cleanups to GEM code

[Thomas Zimmermann]

Refactor and simplify various parts of the memory management. This
includes locking, initialization and finalizer functions, and code
organization.

Tested on Atom N2800 hardware.

Thomas Zimmermann (10):
  drm/gma500: Remove struct psb_gem_object.npage
  drm/gma500: Acquire reservation lock for GEM objects
  drm/gma500: Move GTT locking into GTT helpers
  drm/gma500: Remove struct psb_gtt.sem sempahore
  drm/gma500: Move GTT setup and restoration into helper funtions
  drm/gma500: Move GTT resume logic out of psb_gtt_init()
  drm/gma500: Cleanup GTT uninit and error handling
  drm/gma500: Split GTT init/resume/fini into GTT and GEM functions
  drm/gma500: Inline psb_gtt_restore()
  drm/gma500: Move GEM memory management functions to gem.c

 drivers/gpu/drm/gma500/gem.c         | 161 ++++++++++++++++--
 drivers/gpu/drm/gma500/gem.h         |  13 +-
 drivers/gpu/drm/gma500/gma_display.c |   8 +-
 drivers/gpu/drm/gma500/gtt.c         | 239 +++++++++++++--------------
 drivers/gpu/drm/gma500/gtt.h         |   8 +-
 drivers/gpu/drm/gma500/power.c       |   5 +-
 drivers/gpu/drm/gma500/psb_drv.c     |  13 +-
 drivers/gpu/drm/gma500/psb_drv.h     |   1 -
 8 files changed, 296 insertions(+), 152 deletions(-)


base-commit: 710a019ad85e96e66f7d75ee7f4733cdd8a2b0d0
prerequisite-patch-id: c2b2f08f0eccc9f5df0c0da49fa1d36267deb11d
prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
prerequisite-patch-id: 6e1032c6302461624f33194c8b8f37103a3fa6ef
-- 
2.35.1

[PATCH 01/10] drm/gma500: Remove struct psb_gem_object.npage

[Thomas Zimmermann]

Calculate the number of pages in the BO's backing storage from
the size. Remove the npage field.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/gma500/gem.c         | 9 +++++----
 drivers/gpu/drm/gma500/gem.h         | 1 -
 drivers/gpu/drm/gma500/gma_display.c | 8 +++-----
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index 8d65af80bb08..fe7d242567c0 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -51,7 +51,6 @@ int psb_gem_pin(struct psb_gem_object *pobj)
 			     (gpu_base + pobj->offset), npages, 0, 0,
 			     PSB_MMU_CACHED_MEMORY);
 
-	pobj->npage = npages;
 	pobj->pages = pages;
 
 out:
@@ -71,6 +70,7 @@ void psb_gem_unpin(struct psb_gem_object *pobj)
 	struct drm_device *dev = obj->dev;
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
 	u32 gpu_base = dev_priv->gtt.gatt_start;
+	unsigned long npages;
 
 	mutex_lock(&dev_priv->gtt_mutex);
 
@@ -81,16 +81,17 @@ void psb_gem_unpin(struct psb_gem_object *pobj)
 	if (pobj->in_gart || pobj->stolen)
 		goto out;
 
+	npages = obj->size / PAGE_SIZE;
+
 	psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu),
-			     (gpu_base + pobj->offset), pobj->npage, 0, 0);
+			     (gpu_base + pobj->offset), npages, 0, 0);
 	psb_gtt_remove_pages(dev_priv, &pobj->resource);
 
 	/* Reset caching flags */
-	set_pages_array_wb(pobj->pages, pobj->npage);
+	set_pages_array_wb(pobj->pages, npages);
 
 	drm_gem_put_pages(obj, pobj->pages, true, false);
 	pobj->pages = NULL;
-	pobj->npage = 0;
 
 out:
 	mutex_unlock(&dev_priv->gtt_mutex);
diff --git a/drivers/gpu/drm/gma500/gem.h b/drivers/gpu/drm/gma500/gem.h
index 79cced40c87f..96ef864b4bf1 100644
--- a/drivers/gpu/drm/gma500/gem.h
+++ b/drivers/gpu/drm/gma500/gem.h
@@ -23,7 +23,6 @@ struct psb_gem_object {
 	bool stolen;			/* Backed from stolen RAM */
 	bool mmapping;			/* Is mmappable */
 	struct page **pages;		/* Backing pages if present */
-	int npage;			/* Number of backing pages */
 };
 
 static inline struct psb_gem_object *to_psb_gem_object(struct drm_gem_object *obj)
diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c
index 60ba7de59139..dd801404cf99 100644
--- a/drivers/gpu/drm/gma500/gma_display.c
+++ b/drivers/gpu/drm/gma500/gma_display.c
@@ -391,11 +391,9 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
 			goto unref_cursor;
 		}
 
-		/* Prevent overflow */
-		if (pobj->npage > 4)
-			cursor_pages = 4;
-		else
-			cursor_pages = pobj->npage;
+		cursor_pages = obj->size / PAGE_SIZE;
+		if (cursor_pages > 4)
+			cursor_pages = 4; /* Prevent overflow */
 
 		/* Copy the cursor to cursor mem */
 		tmp_dst = dev_priv->vram_addr + cursor_pobj->offset;
-- 
2.35.1

[PATCH 02/10] drm/gma500: Acquire reservation lock for GEM objects

[Thomas Zimmermann]

Protect concurrent access to struct psb_gem_object by acquiring
the GEM object's reservation lock; as it's supposed to be. The
use of the GTT mutex can now be moved into GTT code.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/gma500/gem.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index fe7d242567c0..026ce11f7460 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -31,6 +31,10 @@ int psb_gem_pin(struct psb_gem_object *pobj)
 	unsigned int npages;
 	int ret;
 
+	ret = dma_resv_lock(obj->resv, NULL);
+	if (drm_WARN_ONCE(dev, ret, "dma_resv_lock() failed, ret=%d\n", ret))
+		return ret;
+
 	mutex_lock(&dev_priv->gtt_mutex);
 
 	if (pobj->in_gart || pobj->stolen)
@@ -56,11 +60,13 @@ int psb_gem_pin(struct psb_gem_object *pobj)
 out:
 	++pobj->in_gart;
 	mutex_unlock(&dev_priv->gtt_mutex);
+	dma_resv_unlock(obj->resv);
 
 	return 0;
 
 err_mutex_unlock:
 	mutex_unlock(&dev_priv->gtt_mutex);
+	dma_resv_unlock(obj->resv);
 	return ret;
 }
 
@@ -71,6 +77,11 @@ void psb_gem_unpin(struct psb_gem_object *pobj)
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
 	u32 gpu_base = dev_priv->gtt.gatt_start;
 	unsigned long npages;
+	int ret;
+
+	ret = dma_resv_lock(obj->resv, NULL);
+	if (drm_WARN_ONCE(dev, ret, "dma_resv_lock() failed, ret=%d\n", ret))
+		return;
 
 	mutex_lock(&dev_priv->gtt_mutex);
 
@@ -95,6 +106,7 @@ void psb_gem_unpin(struct psb_gem_object *pobj)
 
 out:
 	mutex_unlock(&dev_priv->gtt_mutex);
+	dma_resv_unlock(obj->resv);
 }
 
 static vm_fault_t psb_gem_fault(struct vm_fault *vmf);
-- 
2.35.1

[PATCH 03/10] drm/gma500: Move GTT locking into GTT helpers

[Thomas Zimmermann]

Acquire the GTT mutex in psb_gtt_{insert,remove}_pages(). Remove
locking from callers. Also remove the GTT locking around the resume
code. Resume does not run concurrently with other GTT operations.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/gma500/gem.c | 11 ++---------
 drivers/gpu/drm/gma500/gtt.c | 24 +++++++++++-------------
 2 files changed, 13 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index 026ce11f7460..2f44535f58e7 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -35,15 +35,13 @@ int psb_gem_pin(struct psb_gem_object *pobj)
 	if (drm_WARN_ONCE(dev, ret, "dma_resv_lock() failed, ret=%d\n", ret))
 		return ret;
 
-	mutex_lock(&dev_priv->gtt_mutex);
-
 	if (pobj->in_gart || pobj->stolen)
 		goto out; /* already mapped */
 
 	pages = drm_gem_get_pages(obj);
 	if (IS_ERR(pages)) {
 		ret = PTR_ERR(pages);
-		goto err_mutex_unlock;
+		goto err_dma_resv_unlock;
 	}
 
 	npages = obj->size / PAGE_SIZE;
@@ -59,13 +57,11 @@ int psb_gem_pin(struct psb_gem_object *pobj)
 
 out:
 	++pobj->in_gart;
-	mutex_unlock(&dev_priv->gtt_mutex);
 	dma_resv_unlock(obj->resv);
 
 	return 0;
 
-err_mutex_unlock:
-	mutex_unlock(&dev_priv->gtt_mutex);
+err_dma_resv_unlock:
 	dma_resv_unlock(obj->resv);
 	return ret;
 }
@@ -83,8 +79,6 @@ void psb_gem_unpin(struct psb_gem_object *pobj)
 	if (drm_WARN_ONCE(dev, ret, "dma_resv_lock() failed, ret=%d\n", ret))
 		return;
 
-	mutex_lock(&dev_priv->gtt_mutex);
-
 	WARN_ON(!pobj->in_gart);
 
 	--pobj->in_gart;
@@ -105,7 +99,6 @@ void psb_gem_unpin(struct psb_gem_object *pobj)
 	pobj->pages = NULL;
 
 out:
-	mutex_unlock(&dev_priv->gtt_mutex);
 	dma_resv_unlock(obj->resv);
 }
 
diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
index 309ffe921bfd..4202e88e5f84 100644
--- a/drivers/gpu/drm/gma500/gtt.c
+++ b/drivers/gpu/drm/gma500/gtt.c
@@ -74,11 +74,7 @@ static u32 __iomem *psb_gtt_entry(struct drm_psb_private *pdev, const struct res
 	return pdev->gtt_map + (offset >> PAGE_SHIFT);
 }
 
-/*
- * Take our preallocated GTT range and insert the GEM object into
- * the GTT. This is protected via the gtt mutex which the caller
- * must hold.
- */
+/* Acquires GTT mutex internally. */
 void psb_gtt_insert_pages(struct drm_psb_private *pdev, const struct resource *res,
 			  struct page **pages)
 {
@@ -86,6 +82,8 @@ void psb_gtt_insert_pages(struct drm_psb_private *pdev, const struct resource *r
 	u32 __iomem *gtt_slot;
 	u32 pte;
 
+	mutex_lock(&pdev->gtt_mutex);
+
 	/* Write our page entries into the GTT itself */
 
 	npages = resource_size(res) >> PAGE_SHIFT;
@@ -98,19 +96,19 @@ void psb_gtt_insert_pages(struct drm_psb_private *pdev, const struct resource *r
 
 	/* Make sure all the entries are set before we return */
 	ioread32(gtt_slot - 1);
+
+	mutex_unlock(&pdev->gtt_mutex);
 }
 
-/*
- * Remove a preallocated GTT range from the GTT. Overwrite all the
- * page table entries with the dummy page. This is protected via the gtt
- * mutex which the caller must hold.
- */
+/* Acquires GTT mutex internally. */
 void psb_gtt_remove_pages(struct drm_psb_private *pdev, const struct resource *res)
 {
 	resource_size_t npages, i;
 	u32 __iomem *gtt_slot;
 	u32 pte;
 
+	mutex_lock(&pdev->gtt_mutex);
+
 	/* Install scratch page for the resource */
 
 	pte = psb_gtt_mask_pte(page_to_pfn(pdev->scratch_page), PSB_MMU_CACHED_MEMORY);
@@ -123,6 +121,8 @@ void psb_gtt_remove_pages(struct drm_psb_private *pdev, const struct resource *r
 
 	/* Make sure all the entries are set before we return */
 	ioread32(gtt_slot - 1);
+
+	mutex_unlock(&pdev->gtt_mutex);
 }
 
 static void psb_gtt_alloc(struct drm_device *dev)
@@ -306,8 +306,6 @@ int psb_gtt_restore(struct drm_device *dev)
 	struct psb_gem_object *pobj;
 	unsigned int restored = 0, total = 0, size = 0;
 
-	/* On resume, the gtt_mutex is already initialized */
-	mutex_lock(&dev_priv->gtt_mutex);
 	psb_gtt_init(dev, 1);
 
 	while (r != NULL) {
@@ -325,7 +323,7 @@ int psb_gtt_restore(struct drm_device *dev)
 		r = r->sibling;
 		total++;
 	}
-	mutex_unlock(&dev_priv->gtt_mutex);
+
 	DRM_DEBUG_DRIVER("Restored %u of %u gtt ranges (%u KB)", restored,
 			 total, (size / 1024));
 
-- 
2.35.1

[PATCH 04/10] drm/gma500: Remove struct psb_gtt.sem sempahore

[Thomas Zimmermann]

The semaphore at struct psb_mmu_driver.sem protects access to the MMU
fields. Additional locking with struct psb_gtt.sem is unnecessary. Remove
the field and related code.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/gma500/gtt.c     | 7 -------
 drivers/gpu/drm/gma500/gtt.h     | 1 -
 drivers/gpu/drm/gma500/psb_drv.c | 4 ----
 3 files changed, 12 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
index 4202e88e5f84..c7b7cb1f2d13 100644
--- a/drivers/gpu/drm/gma500/gtt.c
+++ b/drivers/gpu/drm/gma500/gtt.c
@@ -125,12 +125,6 @@ void psb_gtt_remove_pages(struct drm_psb_private *pdev, const struct resource *r
 	mutex_unlock(&pdev->gtt_mutex);
 }
 
-static void psb_gtt_alloc(struct drm_device *dev)
-{
-	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
-	init_rwsem(&dev_priv->gtt.sem);
-}
-
 void psb_gtt_takedown(struct drm_device *dev)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
@@ -166,7 +160,6 @@ int psb_gtt_init(struct drm_device *dev, int resume)
 	if (!resume) {
 		mutex_init(&dev_priv->gtt_mutex);
 		mutex_init(&dev_priv->mmap_mutex);
-		psb_gtt_alloc(dev);
 	}
 
 	pg = &dev_priv->gtt;
diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
index ff1dcdd1ff52..31500533ac45 100644
--- a/drivers/gpu/drm/gma500/gtt.h
+++ b/drivers/gpu/drm/gma500/gtt.h
@@ -22,7 +22,6 @@ struct psb_gtt {
 	unsigned gatt_pages;
 	unsigned long stolen_size;
 	unsigned long vram_stolen_size;
-	struct rw_semaphore sem;
 };
 
 /* Exported functions */
diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index eeb681be9c95..7227a8e44d23 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -184,13 +184,11 @@ static void psb_driver_unload(struct drm_device *dev)
 	if (dev_priv->mmu) {
 		struct psb_gtt *pg = &dev_priv->gtt;
 
-		down_read(&pg->sem);
 		psb_mmu_remove_pfn_sequence(
 			psb_mmu_get_default_pd
 			(dev_priv->mmu),
 			pg->mmu_gatt_start,
 			dev_priv->vram_stolen_size >> PAGE_SHIFT);
-		up_read(&pg->sem);
 		psb_mmu_driver_takedown(dev_priv->mmu);
 		dev_priv->mmu = NULL;
 	}
@@ -345,12 +343,10 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags)
 		return ret;
 
 	/* Add stolen memory to SGX MMU */
-	down_read(&pg->sem);
 	ret = psb_mmu_insert_pfn_sequence(psb_mmu_get_default_pd(dev_priv->mmu),
 					  dev_priv->stolen_base >> PAGE_SHIFT,
 					  pg->gatt_start,
 					  pg->stolen_size >> PAGE_SHIFT, 0);
-	up_read(&pg->sem);
 
 	psb_mmu_set_pd_context(psb_mmu_get_default_pd(dev_priv->mmu), 0);
 	psb_mmu_set_pd_context(dev_priv->pf_pd, 1);
-- 
2.35.1

[PATCH 05/10] drm/gma500: Move GTT setup and restoration into helper funtions

[Thomas Zimmermann]

The GTT init and restore functions contain logic to populate the
GTT entries. Move the code into helper functions.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/gma500/gtt.c | 115 +++++++++++++++++++++--------------
 1 file changed, 68 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
index c7b7cb1f2d13..acd50ee26b03 100644
--- a/drivers/gpu/drm/gma500/gtt.c
+++ b/drivers/gpu/drm/gma500/gtt.c
@@ -144,18 +144,79 @@ void psb_gtt_takedown(struct drm_device *dev)
 		iounmap(dev_priv->gtt_map);
 }
 
+/* Clear GTT. Use a scratch page to avoid accidents or scribbles. */
+static void psb_gtt_clear(struct drm_psb_private *pdev)
+{
+	resource_size_t pfn_base;
+	unsigned long i;
+	uint32_t pte;
+
+	pfn_base = page_to_pfn(pdev->scratch_page);
+	pte = psb_gtt_mask_pte(pfn_base, PSB_MMU_CACHED_MEMORY);
+
+	for (i = 0; i < pdev->gtt.gtt_pages; ++i)
+		iowrite32(pte, pdev->gtt_map + i);
+
+	(void)ioread32(pdev->gtt_map + i - 1);
+}
+
+/* Insert vram stolen pages into the GTT. */
+static void psb_gtt_populate_stolen(struct drm_psb_private *pdev)
+{
+	struct drm_device *dev = &pdev->dev;
+	unsigned int pfn_base;
+	unsigned int i, num_pages;
+	uint32_t pte;
+
+	pfn_base = pdev->stolen_base >> PAGE_SHIFT;
+	num_pages = pdev->vram_stolen_size >> PAGE_SHIFT;
+
+	drm_dbg(dev, "Set up %u stolen pages starting at 0x%08x, GTT offset %dK\n",
+		num_pages, pfn_base << PAGE_SHIFT, 0);
+
+	for (i = 0; i < num_pages; ++i) {
+		pte = psb_gtt_mask_pte(pfn_base + i, PSB_MMU_CACHED_MEMORY);
+		iowrite32(pte, pdev->gtt_map + i);
+	}
+
+	(void)ioread32(pdev->gtt_map + i - 1);
+}
+
+/* Re-insert all pinned GEM objects into GTT. */
+static void psb_gtt_populate_resources(struct drm_psb_private *pdev)
+{
+	unsigned int restored = 0, total = 0, size = 0;
+	struct resource *r = pdev->gtt_mem->child;
+	struct drm_device *dev = &pdev->dev;
+	struct psb_gem_object *pobj;
+
+	while (r) {
+		/*
+		 * TODO: GTT restoration needs a refactoring, so that we don't have to touch
+		 *       struct psb_gem_object here. The type represents a GEM object and is
+		 *       not related to the GTT itself.
+		 */
+		pobj = container_of(r, struct psb_gem_object, resource);
+		if (pobj->pages) {
+			psb_gtt_insert_pages(pdev, &pobj->resource, pobj->pages);
+			size += resource_size(&pobj->resource);
+			++restored;
+		}
+		r = r->sibling;
+		++total;
+	}
+
+	drm_dbg(dev, "Restored %u of %u gtt ranges (%u KB)", restored, total, (size / 1024));
+}
+
 int psb_gtt_init(struct drm_device *dev, int resume)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 	unsigned gtt_pages;
 	unsigned long stolen_size, vram_stolen_size;
-	unsigned i, num_pages;
-	unsigned pfn_base;
 	struct psb_gtt *pg;
-
 	int ret = 0;
-	uint32_t pte;
 
 	if (!resume) {
 		mutex_init(&dev_priv->gtt_mutex);
@@ -262,29 +323,9 @@ int psb_gtt_init(struct drm_device *dev, int resume)
 		goto out_err;
 	}
 
-	/*
-	 * Insert vram stolen pages into the GTT
-	 */
-
-	pfn_base = dev_priv->stolen_base >> PAGE_SHIFT;
-	num_pages = vram_stolen_size >> PAGE_SHIFT;
-	dev_dbg(dev->dev, "Set up %d stolen pages starting at 0x%08x, GTT offset %dK\n",
-		num_pages, pfn_base << PAGE_SHIFT, 0);
-	for (i = 0; i < num_pages; ++i) {
-		pte = psb_gtt_mask_pte(pfn_base + i, PSB_MMU_CACHED_MEMORY);
-		iowrite32(pte, dev_priv->gtt_map + i);
-	}
-
-	/*
-	 * Init rest of GTT to the scratch page to avoid accidents or scribbles
-	 */
-
-	pfn_base = page_to_pfn(dev_priv->scratch_page);
-	pte = psb_gtt_mask_pte(pfn_base, PSB_MMU_CACHED_MEMORY);
-	for (; i < gtt_pages; ++i)
-		iowrite32(pte, dev_priv->gtt_map + i);
+	psb_gtt_clear(dev_priv);
+	psb_gtt_populate_stolen(dev_priv);
 
-	(void) ioread32(dev_priv->gtt_map + i - 1);
 	return 0;
 
 out_err:
@@ -295,30 +336,10 @@ int psb_gtt_init(struct drm_device *dev, int resume)
 int psb_gtt_restore(struct drm_device *dev)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
-	struct resource *r = dev_priv->gtt_mem->child;
-	struct psb_gem_object *pobj;
-	unsigned int restored = 0, total = 0, size = 0;
 
 	psb_gtt_init(dev, 1);
 
-	while (r != NULL) {
-		/*
-		 * TODO: GTT restoration needs a refactoring, so that we don't have to touch
-		 *       struct psb_gem_object here. The type represents a GEM object and is
-		 *       not related to the GTT itself.
-		 */
-		pobj = container_of(r, struct psb_gem_object, resource);
-		if (pobj->pages) {
-			psb_gtt_insert_pages(dev_priv, &pobj->resource, pobj->pages);
-			size += pobj->resource.end - pobj->resource.start;
-			restored++;
-		}
-		r = r->sibling;
-		total++;
-	}
-
-	DRM_DEBUG_DRIVER("Restored %u of %u gtt ranges (%u KB)", restored,
-			 total, (size / 1024));
+	psb_gtt_populate_resources(dev_priv);
 
 	return 0;
 }
-- 
2.35.1

[PATCH 06/10] drm/gma500: Move GTT resume logic out of psb_gtt_init()

[Thomas Zimmermann]

The current implementation of psb_gtt_init() also does resume
handling. Move the resume code into its own helper.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/gma500/gtt.c     | 122 ++++++++++++++++++++++++++-----
 drivers/gpu/drm/gma500/gtt.h     |   2 +-
 drivers/gpu/drm/gma500/psb_drv.c |   2 +-
 3 files changed, 104 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
index acd50ee26b03..43ad3ec38c80 100644
--- a/drivers/gpu/drm/gma500/gtt.c
+++ b/drivers/gpu/drm/gma500/gtt.c
@@ -209,7 +209,7 @@ static void psb_gtt_populate_resources(struct drm_psb_private *pdev)
 	drm_dbg(dev, "Restored %u of %u gtt ranges (%u KB)", restored, total, (size / 1024));
 }
 
-int psb_gtt_init(struct drm_device *dev, int resume)
+int psb_gtt_init(struct drm_device *dev)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
@@ -218,10 +218,8 @@ int psb_gtt_init(struct drm_device *dev, int resume)
 	struct psb_gtt *pg;
 	int ret = 0;
 
-	if (!resume) {
-		mutex_init(&dev_priv->gtt_mutex);
-		mutex_init(&dev_priv->mmap_mutex);
-	}
+	mutex_init(&dev_priv->gtt_mutex);
+	mutex_init(&dev_priv->mmap_mutex);
 
 	pg = &dev_priv->gtt;
 
@@ -290,13 +288,6 @@ int psb_gtt_init(struct drm_device *dev, int resume)
 	dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n",
 			dev_priv->stolen_base, vram_stolen_size / 1024);
 
-	if (resume && (gtt_pages != pg->gtt_pages) &&
-	    (stolen_size != pg->stolen_size)) {
-		dev_err(dev->dev, "GTT resume error.\n");
-		ret = -EINVAL;
-		goto out_err;
-	}
-
 	pg->gtt_pages = gtt_pages;
 	pg->stolen_size = stolen_size;
 	dev_priv->vram_stolen_size = vram_stolen_size;
@@ -304,19 +295,14 @@ int psb_gtt_init(struct drm_device *dev, int resume)
 	/*
 	 *	Map the GTT and the stolen memory area
 	 */
-	if (!resume)
-		dev_priv->gtt_map = ioremap(pg->gtt_phys_start,
-						gtt_pages << PAGE_SHIFT);
+	dev_priv->gtt_map = ioremap(pg->gtt_phys_start, gtt_pages << PAGE_SHIFT);
 	if (!dev_priv->gtt_map) {
 		dev_err(dev->dev, "Failure to map gtt.\n");
 		ret = -ENOMEM;
 		goto out_err;
 	}
 
-	if (!resume)
-		dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base,
-						 stolen_size);
-
+	dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base, stolen_size);
 	if (!dev_priv->vram_addr) {
 		dev_err(dev->dev, "Failure to map stolen base.\n");
 		ret = -ENOMEM;
@@ -333,11 +319,107 @@ int psb_gtt_init(struct drm_device *dev, int resume)
 	return ret;
 }
 
+static int psb_gtt_resume(struct drm_device *dev)
+{
+	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
+	struct pci_dev *pdev = to_pci_dev(dev->dev);
+	unsigned int gtt_pages;
+	unsigned long stolen_size, vram_stolen_size;
+	struct psb_gtt *pg;
+	int ret = 0;
+
+	pg = &dev_priv->gtt;
+
+	/* Enable the GTT */
+	pci_read_config_word(pdev, PSB_GMCH_CTRL, &dev_priv->gmch_ctrl);
+	pci_write_config_word(pdev, PSB_GMCH_CTRL,
+			      dev_priv->gmch_ctrl | _PSB_GMCH_ENABLED);
+
+	dev_priv->pge_ctl = PSB_RVDC32(PSB_PGETBL_CTL);
+	PSB_WVDC32(dev_priv->pge_ctl | _PSB_PGETBL_ENABLED, PSB_PGETBL_CTL);
+	(void) PSB_RVDC32(PSB_PGETBL_CTL);
+
+	/* The root resource we allocate address space from */
+	dev_priv->gtt_initialized = 1;
+
+	pg->gtt_phys_start = dev_priv->pge_ctl & PAGE_MASK;
+
+	/*
+	 *	The video mmu has a hw bug when accessing 0x0D0000000.
+	 *	Make gatt start at 0x0e000,0000. This doesn't actually
+	 *	matter for us but may do if the video acceleration ever
+	 *	gets opened up.
+	 */
+	pg->mmu_gatt_start = 0xE0000000;
+
+	pg->gtt_start = pci_resource_start(pdev, PSB_GTT_RESOURCE);
+	gtt_pages = pci_resource_len(pdev, PSB_GTT_RESOURCE) >> PAGE_SHIFT;
+	/* CDV doesn't report this. In which case the system has 64 gtt pages */
+	if (pg->gtt_start == 0 || gtt_pages == 0) {
+		dev_dbg(dev->dev, "GTT PCI BAR not initialized.\n");
+		gtt_pages = 64;
+		pg->gtt_start = dev_priv->pge_ctl;
+	}
+
+	pg->gatt_start = pci_resource_start(pdev, PSB_GATT_RESOURCE);
+	pg->gatt_pages = pci_resource_len(pdev, PSB_GATT_RESOURCE) >> PAGE_SHIFT;
+	dev_priv->gtt_mem = &pdev->resource[PSB_GATT_RESOURCE];
+
+	if (pg->gatt_pages == 0 || pg->gatt_start == 0) {
+		static struct resource fudge;	/* Preferably peppermint */
+		/*
+		 * This can occur on CDV systems. Fudge it in this case. We
+		 * really don't care what imaginary space is being allocated
+		 * at this point.
+		 */
+		dev_dbg(dev->dev, "GATT PCI BAR not initialized.\n");
+		pg->gatt_start = 0x40000000;
+		pg->gatt_pages = (128 * 1024 * 1024) >> PAGE_SHIFT;
+		/*
+		 * This is a little confusing but in fact the GTT is providing
+		 * a view from the GPU into memory and not vice versa. As such
+		 *  this is really allocating space that is not the same as the
+		 *  CPU address space on CDV.
+		 */
+		fudge.start = 0x40000000;
+		fudge.end = 0x40000000 + 128 * 1024 * 1024 - 1;
+		fudge.name = "fudge";
+		fudge.flags = IORESOURCE_MEM;
+		dev_priv->gtt_mem = &fudge;
+	}
+
+	pci_read_config_dword(pdev, PSB_BSM, &dev_priv->stolen_base);
+	vram_stolen_size = pg->gtt_phys_start - dev_priv->stolen_base - PAGE_SIZE;
+	stolen_size = vram_stolen_size;
+
+	dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n",
+			dev_priv->stolen_base, vram_stolen_size / 1024);
+
+	if ((gtt_pages != pg->gtt_pages) && (stolen_size != pg->stolen_size)) {
+		dev_err(dev->dev, "GTT resume error.\n");
+		ret = -EINVAL;
+		goto out_err;
+	}
+
+	pg->gtt_pages = gtt_pages;
+	pg->stolen_size = stolen_size;
+	dev_priv->vram_stolen_size = vram_stolen_size;
+
+	psb_gtt_clear(dev_priv);
+	psb_gtt_populate_stolen(dev_priv);
+
+	return 0;
+
+out_err:
+	psb_gtt_takedown(dev);
+	return ret;
+}
+
 int psb_gtt_restore(struct drm_device *dev)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
 
-	psb_gtt_init(dev, 1);
+	psb_gtt_resume(dev);
 
 	psb_gtt_populate_resources(dev_priv);
 
diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
index 31500533ac45..cb270ea40794 100644
--- a/drivers/gpu/drm/gma500/gtt.h
+++ b/drivers/gpu/drm/gma500/gtt.h
@@ -25,7 +25,7 @@ struct psb_gtt {
 };
 
 /* Exported functions */
-extern int psb_gtt_init(struct drm_device *dev, int resume);
+int psb_gtt_init(struct drm_device *dev);
 extern void psb_gtt_takedown(struct drm_device *dev);
 extern int psb_gtt_restore(struct drm_device *dev);
 
diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index 7227a8e44d23..2891a3dc8d2e 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -324,7 +324,7 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags)
 
 	set_pages_uc(dev_priv->scratch_page, 1);
 
-	ret = psb_gtt_init(dev, 0);
+	ret = psb_gtt_init(dev);
 	if (ret)
 		goto out_err;
 
-- 
2.35.1

[PATCH 07/10] drm/gma500: Cleanup GTT uninit and error handling

[Thomas Zimmermann]

Replace psb_gtt_takedown() with finalizer function that is only called
for unloading the driver. Use roll-back pattern for error handling in
psb_gtt_init() and _resume(). Also fixes a bug where vmap_addr was never
unmapped.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/gma500/gtt.c     | 49 ++++++++++++++++----------------
 drivers/gpu/drm/gma500/gtt.h     |  2 +-
 drivers/gpu/drm/gma500/psb_drv.c |  2 +-
 drivers/gpu/drm/gma500/psb_drv.h |  1 -
 4 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
index 43ad3ec38c80..99c644a5c5cb 100644
--- a/drivers/gpu/drm/gma500/gtt.c
+++ b/drivers/gpu/drm/gma500/gtt.c
@@ -125,23 +125,20 @@ void psb_gtt_remove_pages(struct drm_psb_private *pdev, const struct resource *r
 	mutex_unlock(&pdev->gtt_mutex);
 }
 
-void psb_gtt_takedown(struct drm_device *dev)
+void psb_gtt_fini(struct drm_device *dev)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 
-	if (dev_priv->gtt_map) {
-		iounmap(dev_priv->gtt_map);
-		dev_priv->gtt_map = NULL;
-	}
-	if (dev_priv->gtt_initialized) {
-		pci_write_config_word(pdev, PSB_GMCH_CTRL,
-				      dev_priv->gmch_ctrl);
-		PSB_WVDC32(dev_priv->pge_ctl, PSB_PGETBL_CTL);
-		(void) PSB_RVDC32(PSB_PGETBL_CTL);
-	}
-	if (dev_priv->vram_addr)
-		iounmap(dev_priv->gtt_map);
+	iounmap(dev_priv->vram_addr);
+	iounmap(dev_priv->gtt_map);
+
+	pci_write_config_word(pdev, PSB_GMCH_CTRL, dev_priv->gmch_ctrl);
+	PSB_WVDC32(dev_priv->pge_ctl, PSB_PGETBL_CTL);
+	(void)PSB_RVDC32(PSB_PGETBL_CTL);
+
+	mutex_destroy(&dev_priv->mmap_mutex);
+	mutex_destroy(&dev_priv->gtt_mutex);
 }
 
 /* Clear GTT. Use a scratch page to avoid accidents or scribbles. */
@@ -233,8 +230,6 @@ int psb_gtt_init(struct drm_device *dev)
 	(void) PSB_RVDC32(PSB_PGETBL_CTL);
 
 	/* The root resource we allocate address space from */
-	dev_priv->gtt_initialized = 1;
-
 	pg->gtt_phys_start = dev_priv->pge_ctl & PAGE_MASK;
 
 	/*
@@ -299,14 +294,14 @@ int psb_gtt_init(struct drm_device *dev)
 	if (!dev_priv->gtt_map) {
 		dev_err(dev->dev, "Failure to map gtt.\n");
 		ret = -ENOMEM;
-		goto out_err;
+		goto err_gtt_disable;
 	}
 
 	dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base, stolen_size);
 	if (!dev_priv->vram_addr) {
 		dev_err(dev->dev, "Failure to map stolen base.\n");
 		ret = -ENOMEM;
-		goto out_err;
+		goto err_iounmap;
 	}
 
 	psb_gtt_clear(dev_priv);
@@ -314,8 +309,14 @@ int psb_gtt_init(struct drm_device *dev)
 
 	return 0;
 
-out_err:
-	psb_gtt_takedown(dev);
+err_iounmap:
+	iounmap(dev_priv->gtt_map);
+err_gtt_disable:
+	pci_write_config_word(pdev, PSB_GMCH_CTRL, dev_priv->gmch_ctrl);
+	PSB_WVDC32(dev_priv->pge_ctl, PSB_PGETBL_CTL);
+	(void)PSB_RVDC32(PSB_PGETBL_CTL);
+	mutex_destroy(&dev_priv->mmap_mutex);
+	mutex_destroy(&dev_priv->gtt_mutex);
 	return ret;
 }
 
@@ -340,8 +341,6 @@ static int psb_gtt_resume(struct drm_device *dev)
 	(void) PSB_RVDC32(PSB_PGETBL_CTL);
 
 	/* The root resource we allocate address space from */
-	dev_priv->gtt_initialized = 1;
-
 	pg->gtt_phys_start = dev_priv->pge_ctl & PAGE_MASK;
 
 	/*
@@ -398,7 +397,7 @@ static int psb_gtt_resume(struct drm_device *dev)
 	if ((gtt_pages != pg->gtt_pages) && (stolen_size != pg->stolen_size)) {
 		dev_err(dev->dev, "GTT resume error.\n");
 		ret = -EINVAL;
-		goto out_err;
+		goto err_gtt_disable;
 	}
 
 	pg->gtt_pages = gtt_pages;
@@ -410,8 +409,10 @@ static int psb_gtt_resume(struct drm_device *dev)
 
 	return 0;
 
-out_err:
-	psb_gtt_takedown(dev);
+err_gtt_disable:
+	pci_write_config_word(pdev, PSB_GMCH_CTRL, dev_priv->gmch_ctrl);
+	PSB_WVDC32(dev_priv->pge_ctl, PSB_PGETBL_CTL);
+	(void)PSB_RVDC32(PSB_PGETBL_CTL);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
index cb270ea40794..5839d5d06adc 100644
--- a/drivers/gpu/drm/gma500/gtt.h
+++ b/drivers/gpu/drm/gma500/gtt.h
@@ -26,7 +26,7 @@ struct psb_gtt {
 
 /* Exported functions */
 int psb_gtt_init(struct drm_device *dev);
-extern void psb_gtt_takedown(struct drm_device *dev);
+void psb_gtt_fini(struct drm_device *dev);
 extern int psb_gtt_restore(struct drm_device *dev);
 
 int psb_gtt_allocate_resource(struct drm_psb_private *pdev, struct resource *res,
diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index 2891a3dc8d2e..41be6e1ac7f9 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -192,7 +192,7 @@ static void psb_driver_unload(struct drm_device *dev)
 		psb_mmu_driver_takedown(dev_priv->mmu);
 		dev_priv->mmu = NULL;
 	}
-	psb_gtt_takedown(dev);
+	psb_gtt_fini(dev);
 	if (dev_priv->scratch_page) {
 		set_pages_wb(dev_priv->scratch_page, 1);
 		__free_page(dev_priv->scratch_page);
diff --git a/drivers/gpu/drm/gma500/psb_drv.h b/drivers/gpu/drm/gma500/psb_drv.h
index 0439b10d3db5..553d03190ce1 100644
--- a/drivers/gpu/drm/gma500/psb_drv.h
+++ b/drivers/gpu/drm/gma500/psb_drv.h
@@ -408,7 +408,6 @@ struct drm_psb_private {
 	uint32_t stolen_base;
 	u8 __iomem *vram_addr;
 	unsigned long vram_stolen_size;
-	int gtt_initialized;
 	u16 gmch_ctrl;		/* Saved GTT setup */
 	u32 pge_ctl;
 
-- 
2.35.1

[PATCH 08/10] drm/gma500: Split GTT init/resume/fini into GTT and GEM functions

[Thomas Zimmermann]

The GTT init, fini and resume functions contain both, GTT and GEM,
code. Split each into a separate GTT and a GEM function. The GEM
code is responsible for mmap_mutex and the stolen memory area. The
rest of the functionality is left in GTT functions.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/gma500/gtt.c     | 118 ++++++++++++++++++++-----------
 drivers/gpu/drm/gma500/gtt.h     |   3 +
 drivers/gpu/drm/gma500/psb_drv.c |   4 ++
 3 files changed, 83 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
index 99c644a5c5cb..0e99774c7e59 100644
--- a/drivers/gpu/drm/gma500/gtt.c
+++ b/drivers/gpu/drm/gma500/gtt.c
@@ -125,19 +125,26 @@ void psb_gtt_remove_pages(struct drm_psb_private *pdev, const struct resource *r
 	mutex_unlock(&pdev->gtt_mutex);
 }
 
+void psb_gem_mm_fini(struct drm_device *dev)
+{
+	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
+
+	iounmap(dev_priv->vram_addr);
+
+	mutex_destroy(&dev_priv->mmap_mutex);
+}
+
 void psb_gtt_fini(struct drm_device *dev)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 
-	iounmap(dev_priv->vram_addr);
 	iounmap(dev_priv->gtt_map);
 
 	pci_write_config_word(pdev, PSB_GMCH_CTRL, dev_priv->gmch_ctrl);
 	PSB_WVDC32(dev_priv->pge_ctl, PSB_PGETBL_CTL);
 	(void)PSB_RVDC32(PSB_PGETBL_CTL);
 
-	mutex_destroy(&dev_priv->mmap_mutex);
 	mutex_destroy(&dev_priv->gtt_mutex);
 }
 
@@ -211,12 +218,10 @@ int psb_gtt_init(struct drm_device *dev)
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 	unsigned gtt_pages;
-	unsigned long stolen_size, vram_stolen_size;
 	struct psb_gtt *pg;
 	int ret = 0;
 
 	mutex_init(&dev_priv->gtt_mutex);
-	mutex_init(&dev_priv->mmap_mutex);
 
 	pg = &dev_priv->gtt;
 
@@ -274,22 +279,8 @@ int psb_gtt_init(struct drm_device *dev)
 		dev_priv->gtt_mem = &fudge;
 	}
 
-	pci_read_config_dword(pdev, PSB_BSM, &dev_priv->stolen_base);
-	vram_stolen_size = pg->gtt_phys_start - dev_priv->stolen_base
-								- PAGE_SIZE;
-
-	stolen_size = vram_stolen_size;
-
-	dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n",
-			dev_priv->stolen_base, vram_stolen_size / 1024);
-
 	pg->gtt_pages = gtt_pages;
-	pg->stolen_size = stolen_size;
-	dev_priv->vram_stolen_size = vram_stolen_size;
 
-	/*
-	 *	Map the GTT and the stolen memory area
-	 */
 	dev_priv->gtt_map = ioremap(pg->gtt_phys_start, gtt_pages << PAGE_SHIFT);
 	if (!dev_priv->gtt_map) {
 		dev_err(dev->dev, "Failure to map gtt.\n");
@@ -297,26 +288,54 @@ int psb_gtt_init(struct drm_device *dev)
 		goto err_gtt_disable;
 	}
 
+	psb_gtt_clear(dev_priv);
+
+	return 0;
+
+err_gtt_disable:
+	pci_write_config_word(pdev, PSB_GMCH_CTRL, dev_priv->gmch_ctrl);
+	PSB_WVDC32(dev_priv->pge_ctl, PSB_PGETBL_CTL);
+	(void)PSB_RVDC32(PSB_PGETBL_CTL);
+	mutex_destroy(&dev_priv->gtt_mutex);
+	return ret;
+}
+
+int psb_gem_mm_init(struct drm_device *dev)
+{
+	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
+	struct pci_dev *pdev = to_pci_dev(dev->dev);
+	unsigned long stolen_size, vram_stolen_size;
+	struct psb_gtt *pg;
+	int ret;
+
+	mutex_init(&dev_priv->mmap_mutex);
+
+	pg = &dev_priv->gtt;
+
+	pci_read_config_dword(pdev, PSB_BSM, &dev_priv->stolen_base);
+	vram_stolen_size = pg->gtt_phys_start - dev_priv->stolen_base - PAGE_SIZE;
+
+	stolen_size = vram_stolen_size;
+
+	dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n",
+		dev_priv->stolen_base, vram_stolen_size / 1024);
+
+	pg->stolen_size = stolen_size;
+	dev_priv->vram_stolen_size = vram_stolen_size;
+
 	dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base, stolen_size);
 	if (!dev_priv->vram_addr) {
 		dev_err(dev->dev, "Failure to map stolen base.\n");
 		ret = -ENOMEM;
-		goto err_iounmap;
+		goto err_mutex_destroy;
 	}
 
-	psb_gtt_clear(dev_priv);
 	psb_gtt_populate_stolen(dev_priv);
 
 	return 0;
 
-err_iounmap:
-	iounmap(dev_priv->gtt_map);
-err_gtt_disable:
-	pci_write_config_word(pdev, PSB_GMCH_CTRL, dev_priv->gmch_ctrl);
-	PSB_WVDC32(dev_priv->pge_ctl, PSB_PGETBL_CTL);
-	(void)PSB_RVDC32(PSB_PGETBL_CTL);
+err_mutex_destroy:
 	mutex_destroy(&dev_priv->mmap_mutex);
-	mutex_destroy(&dev_priv->gtt_mutex);
 	return ret;
 }
 
@@ -325,9 +344,8 @@ static int psb_gtt_resume(struct drm_device *dev)
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 	unsigned int gtt_pages;
-	unsigned long stolen_size, vram_stolen_size;
 	struct psb_gtt *pg;
-	int ret = 0;
+	int ret;
 
 	pg = &dev_priv->gtt;
 
@@ -387,27 +405,15 @@ static int psb_gtt_resume(struct drm_device *dev)
 		dev_priv->gtt_mem = &fudge;
 	}
 
-	pci_read_config_dword(pdev, PSB_BSM, &dev_priv->stolen_base);
-	vram_stolen_size = pg->gtt_phys_start - dev_priv->stolen_base - PAGE_SIZE;
-	stolen_size = vram_stolen_size;
-
-	dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n",
-			dev_priv->stolen_base, vram_stolen_size / 1024);
-
-	if ((gtt_pages != pg->gtt_pages) && (stolen_size != pg->stolen_size)) {
+	if (gtt_pages != pg->gtt_pages) {
 		dev_err(dev->dev, "GTT resume error.\n");
 		ret = -EINVAL;
 		goto err_gtt_disable;
 	}
 
 	pg->gtt_pages = gtt_pages;
-	pg->stolen_size = stolen_size;
-	dev_priv->vram_stolen_size = vram_stolen_size;
 
 	psb_gtt_clear(dev_priv);
-	psb_gtt_populate_stolen(dev_priv);
-
-	return 0;
 
 err_gtt_disable:
 	pci_write_config_word(pdev, PSB_GMCH_CTRL, dev_priv->gmch_ctrl);
@@ -416,11 +422,39 @@ static int psb_gtt_resume(struct drm_device *dev)
 	return ret;
 }
 
+static int psb_gem_mm_resume(struct drm_device *dev)
+{
+	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
+	struct pci_dev *pdev = to_pci_dev(dev->dev);
+	unsigned long stolen_size, vram_stolen_size;
+	struct psb_gtt *pg;
+
+	pg = &dev_priv->gtt;
+
+	pci_read_config_dword(pdev, PSB_BSM, &dev_priv->stolen_base);
+	vram_stolen_size = pg->gtt_phys_start - dev_priv->stolen_base - PAGE_SIZE;
+
+	stolen_size = vram_stolen_size;
+
+	dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n", dev_priv->stolen_base,
+		vram_stolen_size / 1024);
+
+	if (stolen_size != pg->stolen_size) {
+		dev_err(dev->dev, "GTT resume error.\n");
+		return -EINVAL;
+	}
+
+	psb_gtt_populate_stolen(dev_priv);
+
+	return 0;
+}
+
 int psb_gtt_restore(struct drm_device *dev)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
 
 	psb_gtt_resume(dev);
+	psb_gem_mm_resume(dev);
 
 	psb_gtt_populate_resources(dev_priv);
 
diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
index 5839d5d06adc..45e1926dfce1 100644
--- a/drivers/gpu/drm/gma500/gtt.h
+++ b/drivers/gpu/drm/gma500/gtt.h
@@ -37,4 +37,7 @@ void psb_gtt_insert_pages(struct drm_psb_private *pdev, const struct resource *r
 			  struct page **pages);
 void psb_gtt_remove_pages(struct drm_psb_private *pdev, const struct resource *res);
 
+int psb_gem_mm_init(struct drm_device *dev);
+void psb_gem_mm_fini(struct drm_device *dev);
+
 #endif
diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index 41be6e1ac7f9..6547967b51e1 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -192,6 +192,7 @@ static void psb_driver_unload(struct drm_device *dev)
 		psb_mmu_driver_takedown(dev_priv->mmu);
 		dev_priv->mmu = NULL;
 	}
+	psb_gem_mm_fini(dev);
 	psb_gtt_fini(dev);
 	if (dev_priv->scratch_page) {
 		set_pages_wb(dev_priv->scratch_page, 1);
@@ -325,6 +326,9 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags)
 	set_pages_uc(dev_priv->scratch_page, 1);
 
 	ret = psb_gtt_init(dev);
+	if (ret)
+		goto out_err;
+	ret = psb_gem_mm_init(dev);
 	if (ret)
 		goto out_err;
 
-- 
2.35.1

[PATCH 09/10] drm/gma500: Inline psb_gtt_restore()

[Thomas Zimmermann]

Inline psb_gtt_restore() into its only caller in power.c.

Perform the GTT restoration in psb_gem_mm_resume(). The restoration
step is part of GEM anyway and will be moved over at some point.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/gma500/gtt.c   | 15 ++-------------
 drivers/gpu/drm/gma500/gtt.h   |  3 ++-
 drivers/gpu/drm/gma500/power.c |  4 +++-
 3 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
index 0e99774c7e59..9e1b19fcea28 100644
--- a/drivers/gpu/drm/gma500/gtt.c
+++ b/drivers/gpu/drm/gma500/gtt.c
@@ -339,7 +339,7 @@ int psb_gem_mm_init(struct drm_device *dev)
 	return ret;
 }
 
-static int psb_gtt_resume(struct drm_device *dev)
+int psb_gtt_resume(struct drm_device *dev)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
@@ -422,7 +422,7 @@ static int psb_gtt_resume(struct drm_device *dev)
 	return ret;
 }
 
-static int psb_gem_mm_resume(struct drm_device *dev)
+int psb_gem_mm_resume(struct drm_device *dev)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
@@ -445,17 +445,6 @@ static int psb_gem_mm_resume(struct drm_device *dev)
 	}
 
 	psb_gtt_populate_stolen(dev_priv);
-
-	return 0;
-}
-
-int psb_gtt_restore(struct drm_device *dev)
-{
-	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
-
-	psb_gtt_resume(dev);
-	psb_gem_mm_resume(dev);
-
 	psb_gtt_populate_resources(dev_priv);
 
 	return 0;
diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
index 45e1926dfce1..9a6d79200dce 100644
--- a/drivers/gpu/drm/gma500/gtt.h
+++ b/drivers/gpu/drm/gma500/gtt.h
@@ -27,7 +27,7 @@ struct psb_gtt {
 /* Exported functions */
 int psb_gtt_init(struct drm_device *dev);
 void psb_gtt_fini(struct drm_device *dev);
-extern int psb_gtt_restore(struct drm_device *dev);
+int psb_gtt_resume(struct drm_device *dev);
 
 int psb_gtt_allocate_resource(struct drm_psb_private *pdev, struct resource *res,
 			      const char *name, resource_size_t size, resource_size_t align,
@@ -39,5 +39,6 @@ void psb_gtt_remove_pages(struct drm_psb_private *pdev, const struct resource *r
 
 int psb_gem_mm_init(struct drm_device *dev);
 void psb_gem_mm_fini(struct drm_device *dev);
+int psb_gem_mm_resume(struct drm_device *dev);
 
 #endif
diff --git a/drivers/gpu/drm/gma500/power.c b/drivers/gpu/drm/gma500/power.c
index d2a46d96e746..28e472b31698 100644
--- a/drivers/gpu/drm/gma500/power.c
+++ b/drivers/gpu/drm/gma500/power.c
@@ -112,7 +112,9 @@ static void gma_resume_display(struct pci_dev *pdev)
 	pci_write_config_word(pdev, PSB_GMCH_CTRL,
 			dev_priv->gmch_ctrl | _PSB_GMCH_ENABLED);
 
-	psb_gtt_restore(dev); /* Rebuild our GTT mappings */
+	/* Rebuild our GTT mappings */
+	psb_gtt_resume(dev);
+	psb_gem_mm_resume(dev);
 	dev_priv->ops->restore_regs(dev);
 }
 
-- 
2.35.1

[PATCH 10/10] drm/gma500: Move GEM memory management functions to gem.c

[Thomas Zimmermann]

Move GEM functions from gtt.c to gem.c. Adapt some names. No
functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/gma500/gem.c     | 133 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/gma500/gem.h     |  12 +++
 drivers/gpu/drm/gma500/gtt.c     | 127 +----------------------------
 drivers/gpu/drm/gma500/gtt.h     |   5 +-
 drivers/gpu/drm/gma500/power.c   |   1 +
 drivers/gpu/drm/gma500/psb_drv.c |   1 +
 6 files changed, 149 insertions(+), 130 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index 2f44535f58e7..dffe37490206 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -21,6 +21,10 @@
 #include "gem.h"
 #include "psb_drv.h"
 
+/*
+ * PSB GEM object
+ */
+
 int psb_gem_pin(struct psb_gem_object *pobj)
 {
 	struct drm_gem_object *obj = &pobj->base;
@@ -296,3 +300,132 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf)
 
 	return ret;
 }
+
+/*
+ * Memory management
+ */
+
+/* Insert vram stolen pages into the GTT. */
+static void psb_gem_mm_populate_stolen(struct drm_psb_private *pdev)
+{
+	struct drm_device *dev = &pdev->dev;
+	unsigned int pfn_base;
+	unsigned int i, num_pages;
+	uint32_t pte;
+
+	pfn_base = pdev->stolen_base >> PAGE_SHIFT;
+	num_pages = pdev->vram_stolen_size >> PAGE_SHIFT;
+
+	drm_dbg(dev, "Set up %u stolen pages starting at 0x%08x, GTT offset %dK\n",
+		num_pages, pfn_base << PAGE_SHIFT, 0);
+
+	for (i = 0; i < num_pages; ++i) {
+		pte = psb_gtt_mask_pte(pfn_base + i, PSB_MMU_CACHED_MEMORY);
+		iowrite32(pte, pdev->gtt_map + i);
+	}
+
+	(void)ioread32(pdev->gtt_map + i - 1);
+}
+
+int psb_gem_mm_init(struct drm_device *dev)
+{
+	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
+	struct pci_dev *pdev = to_pci_dev(dev->dev);
+	unsigned long stolen_size, vram_stolen_size;
+	struct psb_gtt *pg;
+	int ret;
+
+	mutex_init(&dev_priv->mmap_mutex);
+
+	pg = &dev_priv->gtt;
+
+	pci_read_config_dword(pdev, PSB_BSM, &dev_priv->stolen_base);
+	vram_stolen_size = pg->gtt_phys_start - dev_priv->stolen_base - PAGE_SIZE;
+
+	stolen_size = vram_stolen_size;
+
+	dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n",
+		dev_priv->stolen_base, vram_stolen_size / 1024);
+
+	pg->stolen_size = stolen_size;
+	dev_priv->vram_stolen_size = vram_stolen_size;
+
+	dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base, stolen_size);
+	if (!dev_priv->vram_addr) {
+		dev_err(dev->dev, "Failure to map stolen base.\n");
+		ret = -ENOMEM;
+		goto err_mutex_destroy;
+	}
+
+	psb_gem_mm_populate_stolen(dev_priv);
+
+	return 0;
+
+err_mutex_destroy:
+	mutex_destroy(&dev_priv->mmap_mutex);
+	return ret;
+}
+
+void psb_gem_mm_fini(struct drm_device *dev)
+{
+	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
+
+	iounmap(dev_priv->vram_addr);
+
+	mutex_destroy(&dev_priv->mmap_mutex);
+}
+
+/* Re-insert all pinned GEM objects into GTT. */
+static void psb_gem_mm_populate_resources(struct drm_psb_private *pdev)
+{
+	unsigned int restored = 0, total = 0, size = 0;
+	struct resource *r = pdev->gtt_mem->child;
+	struct drm_device *dev = &pdev->dev;
+	struct psb_gem_object *pobj;
+
+	while (r) {
+		/*
+		 * TODO: GTT restoration needs a refactoring, so that we don't have to touch
+		 *       struct psb_gem_object here. The type represents a GEM object and is
+		 *       not related to the GTT itself.
+		 */
+		pobj = container_of(r, struct psb_gem_object, resource);
+		if (pobj->pages) {
+			psb_gtt_insert_pages(pdev, &pobj->resource, pobj->pages);
+			size += resource_size(&pobj->resource);
+			++restored;
+		}
+		r = r->sibling;
+		++total;
+	}
+
+	drm_dbg(dev, "Restored %u of %u gtt ranges (%u KB)", restored, total, (size / 1024));
+}
+
+int psb_gem_mm_resume(struct drm_device *dev)
+{
+	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
+	struct pci_dev *pdev = to_pci_dev(dev->dev);
+	unsigned long stolen_size, vram_stolen_size;
+	struct psb_gtt *pg;
+
+	pg = &dev_priv->gtt;
+
+	pci_read_config_dword(pdev, PSB_BSM, &dev_priv->stolen_base);
+	vram_stolen_size = pg->gtt_phys_start - dev_priv->stolen_base - PAGE_SIZE;
+
+	stolen_size = vram_stolen_size;
+
+	dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n", dev_priv->stolen_base,
+		vram_stolen_size / 1024);
+
+	if (stolen_size != pg->stolen_size) {
+		dev_err(dev->dev, "GTT resume error.\n");
+		return -EINVAL;
+	}
+
+	psb_gem_mm_populate_stolen(dev_priv);
+	psb_gem_mm_populate_resources(dev_priv);
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/gma500/gem.h b/drivers/gpu/drm/gma500/gem.h
index 96ef864b4bf1..27df6ff4ed37 100644
--- a/drivers/gpu/drm/gma500/gem.h
+++ b/drivers/gpu/drm/gma500/gem.h
@@ -14,6 +14,10 @@
 
 struct drm_device;
 
+/*
+ * PSB GEM object
+ */
+
 struct psb_gem_object {
 	struct drm_gem_object base;
 
@@ -36,4 +40,12 @@ psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen,
 int psb_gem_pin(struct psb_gem_object *pobj);
 void psb_gem_unpin(struct psb_gem_object *pobj);
 
+/*
+ * Memory management
+ */
+
+int psb_gem_mm_init(struct drm_device *dev);
+void psb_gem_mm_fini(struct drm_device *dev);
+int psb_gem_mm_resume(struct drm_device *dev);
+
 #endif
diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
index 9e1b19fcea28..b03feec64f01 100644
--- a/drivers/gpu/drm/gma500/gtt.c
+++ b/drivers/gpu/drm/gma500/gtt.c
@@ -49,7 +49,7 @@ int psb_gtt_allocate_resource(struct drm_psb_private *pdev, struct resource *res
  *
  *	Set the GTT entry for the appropriate memory type.
  */
-static inline uint32_t psb_gtt_mask_pte(uint32_t pfn, int type)
+uint32_t psb_gtt_mask_pte(uint32_t pfn, int type)
 {
 	uint32_t mask = PSB_PTE_VALID;
 
@@ -125,15 +125,6 @@ void psb_gtt_remove_pages(struct drm_psb_private *pdev, const struct resource *r
 	mutex_unlock(&pdev->gtt_mutex);
 }
 
-void psb_gem_mm_fini(struct drm_device *dev)
-{
-	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
-
-	iounmap(dev_priv->vram_addr);
-
-	mutex_destroy(&dev_priv->mmap_mutex);
-}
-
 void psb_gtt_fini(struct drm_device *dev)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
@@ -164,55 +155,6 @@ static void psb_gtt_clear(struct drm_psb_private *pdev)
 	(void)ioread32(pdev->gtt_map + i - 1);
 }
 
-/* Insert vram stolen pages into the GTT. */
-static void psb_gtt_populate_stolen(struct drm_psb_private *pdev)
-{
-	struct drm_device *dev = &pdev->dev;
-	unsigned int pfn_base;
-	unsigned int i, num_pages;
-	uint32_t pte;
-
-	pfn_base = pdev->stolen_base >> PAGE_SHIFT;
-	num_pages = pdev->vram_stolen_size >> PAGE_SHIFT;
-
-	drm_dbg(dev, "Set up %u stolen pages starting at 0x%08x, GTT offset %dK\n",
-		num_pages, pfn_base << PAGE_SHIFT, 0);
-
-	for (i = 0; i < num_pages; ++i) {
-		pte = psb_gtt_mask_pte(pfn_base + i, PSB_MMU_CACHED_MEMORY);
-		iowrite32(pte, pdev->gtt_map + i);
-	}
-
-	(void)ioread32(pdev->gtt_map + i - 1);
-}
-
-/* Re-insert all pinned GEM objects into GTT. */
-static void psb_gtt_populate_resources(struct drm_psb_private *pdev)
-{
-	unsigned int restored = 0, total = 0, size = 0;
-	struct resource *r = pdev->gtt_mem->child;
-	struct drm_device *dev = &pdev->dev;
-	struct psb_gem_object *pobj;
-
-	while (r) {
-		/*
-		 * TODO: GTT restoration needs a refactoring, so that we don't have to touch
-		 *       struct psb_gem_object here. The type represents a GEM object and is
-		 *       not related to the GTT itself.
-		 */
-		pobj = container_of(r, struct psb_gem_object, resource);
-		if (pobj->pages) {
-			psb_gtt_insert_pages(pdev, &pobj->resource, pobj->pages);
-			size += resource_size(&pobj->resource);
-			++restored;
-		}
-		r = r->sibling;
-		++total;
-	}
-
-	drm_dbg(dev, "Restored %u of %u gtt ranges (%u KB)", restored, total, (size / 1024));
-}
-
 int psb_gtt_init(struct drm_device *dev)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
@@ -300,45 +242,6 @@ int psb_gtt_init(struct drm_device *dev)
 	return ret;
 }
 
-int psb_gem_mm_init(struct drm_device *dev)
-{
-	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
-	struct pci_dev *pdev = to_pci_dev(dev->dev);
-	unsigned long stolen_size, vram_stolen_size;
-	struct psb_gtt *pg;
-	int ret;
-
-	mutex_init(&dev_priv->mmap_mutex);
-
-	pg = &dev_priv->gtt;
-
-	pci_read_config_dword(pdev, PSB_BSM, &dev_priv->stolen_base);
-	vram_stolen_size = pg->gtt_phys_start - dev_priv->stolen_base - PAGE_SIZE;
-
-	stolen_size = vram_stolen_size;
-
-	dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n",
-		dev_priv->stolen_base, vram_stolen_size / 1024);
-
-	pg->stolen_size = stolen_size;
-	dev_priv->vram_stolen_size = vram_stolen_size;
-
-	dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base, stolen_size);
-	if (!dev_priv->vram_addr) {
-		dev_err(dev->dev, "Failure to map stolen base.\n");
-		ret = -ENOMEM;
-		goto err_mutex_destroy;
-	}
-
-	psb_gtt_populate_stolen(dev_priv);
-
-	return 0;
-
-err_mutex_destroy:
-	mutex_destroy(&dev_priv->mmap_mutex);
-	return ret;
-}
-
 int psb_gtt_resume(struct drm_device *dev)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
@@ -421,31 +324,3 @@ int psb_gtt_resume(struct drm_device *dev)
 	(void)PSB_RVDC32(PSB_PGETBL_CTL);
 	return ret;
 }
-
-int psb_gem_mm_resume(struct drm_device *dev)
-{
-	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
-	struct pci_dev *pdev = to_pci_dev(dev->dev);
-	unsigned long stolen_size, vram_stolen_size;
-	struct psb_gtt *pg;
-
-	pg = &dev_priv->gtt;
-
-	pci_read_config_dword(pdev, PSB_BSM, &dev_priv->stolen_base);
-	vram_stolen_size = pg->gtt_phys_start - dev_priv->stolen_base - PAGE_SIZE;
-
-	stolen_size = vram_stolen_size;
-
-	dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n", dev_priv->stolen_base,
-		vram_stolen_size / 1024);
-
-	if (stolen_size != pg->stolen_size) {
-		dev_err(dev->dev, "GTT resume error.\n");
-		return -EINVAL;
-	}
-
-	psb_gtt_populate_stolen(dev_priv);
-	psb_gtt_populate_resources(dev_priv);
-
-	return 0;
-}
diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
index 9a6d79200dce..d5a5fd4466c1 100644
--- a/drivers/gpu/drm/gma500/gtt.h
+++ b/drivers/gpu/drm/gma500/gtt.h
@@ -33,12 +33,9 @@ int psb_gtt_allocate_resource(struct drm_psb_private *pdev, struct resource *res
 			      const char *name, resource_size_t size, resource_size_t align,
 			      bool stolen, u32 *offset);
 
+uint32_t psb_gtt_mask_pte(uint32_t pfn, int type);
 void psb_gtt_insert_pages(struct drm_psb_private *pdev, const struct resource *res,
 			  struct page **pages);
 void psb_gtt_remove_pages(struct drm_psb_private *pdev, const struct resource *res);
 
-int psb_gem_mm_init(struct drm_device *dev);
-void psb_gem_mm_fini(struct drm_device *dev);
-int psb_gem_mm_resume(struct drm_device *dev);
-
 #endif
diff --git a/drivers/gpu/drm/gma500/power.c b/drivers/gpu/drm/gma500/power.c
index 28e472b31698..6f917cfef65b 100644
--- a/drivers/gpu/drm/gma500/power.c
+++ b/drivers/gpu/drm/gma500/power.c
@@ -28,6 +28,7 @@
  *    Alan Cox <alan@linux.intel.com>
  */
 
+#include "gem.h"
 #include "power.h"
 #include "psb_drv.h"
 #include "psb_reg.h"
diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index 6547967b51e1..e30b58184156 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -28,6 +28,7 @@
 #include <drm/drm_vblank.h>
 
 #include "framebuffer.h"
+#include "gem.h"
 #include "intel_bios.h"
 #include "mid_bios.h"
 #include "power.h"
-- 
2.35.1

Re: [PATCH 00/10] drm/gma500: Various cleanups to GEM code

[Patrik Jakobsson]

On Sun, Mar 6, 2022 at 9:36 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Refactor and simplify various parts of the memory management. This
> includes locking, initialization and finalizer functions, and code
> organization.
>
> Tested on Atom N2800 hardware.

Hi Thomas, nice cleanups!

All patches are:
Acked-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>

>
> Thomas Zimmermann (10):
>   drm/gma500: Remove struct psb_gem_object.npage
>   drm/gma500: Acquire reservation lock for GEM objects
>   drm/gma500: Move GTT locking into GTT helpers
>   drm/gma500: Remove struct psb_gtt.sem sempahore
>   drm/gma500: Move GTT setup and restoration into helper funtions
>   drm/gma500: Move GTT resume logic out of psb_gtt_init()
>   drm/gma500: Cleanup GTT uninit and error handling
>   drm/gma500: Split GTT init/resume/fini into GTT and GEM functions
>   drm/gma500: Inline psb_gtt_restore()
>   drm/gma500: Move GEM memory management functions to gem.c
>
>  drivers/gpu/drm/gma500/gem.c         | 161 ++++++++++++++++--
>  drivers/gpu/drm/gma500/gem.h         |  13 +-
>  drivers/gpu/drm/gma500/gma_display.c |   8 +-
>  drivers/gpu/drm/gma500/gtt.c         | 239 +++++++++++++--------------
>  drivers/gpu/drm/gma500/gtt.h         |   8 +-
>  drivers/gpu/drm/gma500/power.c       |   5 +-
>  drivers/gpu/drm/gma500/psb_drv.c     |  13 +-
>  drivers/gpu/drm/gma500/psb_drv.h     |   1 -
>  8 files changed, 296 insertions(+), 152 deletions(-)
>
>
> base-commit: 710a019ad85e96e66f7d75ee7f4733cdd8a2b0d0
> prerequisite-patch-id: c2b2f08f0eccc9f5df0c0da49fa1d36267deb11d
> prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
> prerequisite-patch-id: 6e1032c6302461624f33194c8b8f37103a3fa6ef
> --
> 2.35.1
>

Re: [PATCH 06/10] drm/gma500: Move GTT resume logic out of psb_gtt_init()

[Sam Ravnborg]

Hi Thomas,

One comment below.

On Sun, Mar 06, 2022 at 09:36:15PM +0100, Thomas Zimmermann wrote:
> The current implementation of psb_gtt_init() also does resume
> handling. Move the resume code into its own helper.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/gma500/gtt.c     | 122 ++++++++++++++++++++++++++-----
>  drivers/gpu/drm/gma500/gtt.h     |   2 +-
>  drivers/gpu/drm/gma500/psb_drv.c |   2 +-
>  3 files changed, 104 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
> index acd50ee26b03..43ad3ec38c80 100644
> --- a/drivers/gpu/drm/gma500/gtt.c
> +++ b/drivers/gpu/drm/gma500/gtt.c
> @@ -209,7 +209,7 @@ static void psb_gtt_populate_resources(struct drm_psb_private *pdev)
>  	drm_dbg(dev, "Restored %u of %u gtt ranges (%u KB)", restored, total, (size / 1024));
>  }
>  
> -int psb_gtt_init(struct drm_device *dev, int resume)
> +int psb_gtt_init(struct drm_device *dev)
>  {
>  	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>  	struct pci_dev *pdev = to_pci_dev(dev->dev);
> @@ -218,10 +218,8 @@ int psb_gtt_init(struct drm_device *dev, int resume)
>  	struct psb_gtt *pg;
>  	int ret = 0;
>  
> -	if (!resume) {
> -		mutex_init(&dev_priv->gtt_mutex);
> -		mutex_init(&dev_priv->mmap_mutex);
> -	}
> +	mutex_init(&dev_priv->gtt_mutex);
> +	mutex_init(&dev_priv->mmap_mutex);
>  
>  	pg = &dev_priv->gtt;
>  
> @@ -290,13 +288,6 @@ int psb_gtt_init(struct drm_device *dev, int resume)
>  	dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n",
>  			dev_priv->stolen_base, vram_stolen_size / 1024);
>  
> -	if (resume && (gtt_pages != pg->gtt_pages) &&
> -	    (stolen_size != pg->stolen_size)) {
> -		dev_err(dev->dev, "GTT resume error.\n");
> -		ret = -EINVAL;
> -		goto out_err;
> -	}
> -
>  	pg->gtt_pages = gtt_pages;
>  	pg->stolen_size = stolen_size;
>  	dev_priv->vram_stolen_size = vram_stolen_size;
> @@ -304,19 +295,14 @@ int psb_gtt_init(struct drm_device *dev, int resume)
>  	/*
>  	 *	Map the GTT and the stolen memory area
>  	 */
> -	if (!resume)
> -		dev_priv->gtt_map = ioremap(pg->gtt_phys_start,
> -						gtt_pages << PAGE_SHIFT);
> +	dev_priv->gtt_map = ioremap(pg->gtt_phys_start, gtt_pages << PAGE_SHIFT);
>  	if (!dev_priv->gtt_map) {
>  		dev_err(dev->dev, "Failure to map gtt.\n");
>  		ret = -ENOMEM;
>  		goto out_err;
>  	}
>  
> -	if (!resume)
> -		dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base,
> -						 stolen_size);
> -
> +	dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base, stolen_size);
>  	if (!dev_priv->vram_addr) {
>  		dev_err(dev->dev, "Failure to map stolen base.\n");
>  		ret = -ENOMEM;
> @@ -333,11 +319,107 @@ int psb_gtt_init(struct drm_device *dev, int resume)
>  	return ret;
>  }
>
The below is a lot of duplicated complex code.
Can you add one more helper for this?

> +static int psb_gtt_resume(struct drm_device *dev)
> +{
> +	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> +	struct pci_dev *pdev = to_pci_dev(dev->dev);
> +	unsigned int gtt_pages;
> +	unsigned long stolen_size, vram_stolen_size;
> +	struct psb_gtt *pg;
> +	int ret = 0;
> +
> +	pg = &dev_priv->gtt;

static void psb_enable_gtt(..)
{
> +
> +	/* Enable the GTT */
> +	pci_read_config_word(pdev, PSB_GMCH_CTRL, &dev_priv->gmch_ctrl);
> +	pci_write_config_word(pdev, PSB_GMCH_CTRL,
> +			      dev_priv->gmch_ctrl | _PSB_GMCH_ENABLED);
> +
> +	dev_priv->pge_ctl = PSB_RVDC32(PSB_PGETBL_CTL);
> +	PSB_WVDC32(dev_priv->pge_ctl | _PSB_PGETBL_ENABLED, PSB_PGETBL_CTL);
> +	(void) PSB_RVDC32(PSB_PGETBL_CTL);
> +
> +	/* The root resource we allocate address space from */
> +	dev_priv->gtt_initialized = 1;
> +
> +	pg->gtt_phys_start = dev_priv->pge_ctl & PAGE_MASK;
> +
> +	/*
> +	 *	The video mmu has a hw bug when accessing 0x0D0000000.
> +	 *	Make gatt start at 0x0e000,0000. This doesn't actually
> +	 *	matter for us but may do if the video acceleration ever
> +	 *	gets opened up.
> +	 */
> +	pg->mmu_gatt_start = 0xE0000000;
> +
> +	pg->gtt_start = pci_resource_start(pdev, PSB_GTT_RESOURCE);
> +	gtt_pages = pci_resource_len(pdev, PSB_GTT_RESOURCE) >> PAGE_SHIFT;
> +	/* CDV doesn't report this. In which case the system has 64 gtt pages */
> +	if (pg->gtt_start == 0 || gtt_pages == 0) {
> +		dev_dbg(dev->dev, "GTT PCI BAR not initialized.\n");
> +		gtt_pages = 64;
> +		pg->gtt_start = dev_priv->pge_ctl;
> +	}
> +
> +	pg->gatt_start = pci_resource_start(pdev, PSB_GATT_RESOURCE);
> +	pg->gatt_pages = pci_resource_len(pdev, PSB_GATT_RESOURCE) >> PAGE_SHIFT;
> +	dev_priv->gtt_mem = &pdev->resource[PSB_GATT_RESOURCE];
> +
> +	if (pg->gatt_pages == 0 || pg->gatt_start == 0) {
> +		static struct resource fudge;	/* Preferably peppermint */
> +		/*
> +		 * This can occur on CDV systems. Fudge it in this case. We
> +		 * really don't care what imaginary space is being allocated
> +		 * at this point.
> +		 */
> +		dev_dbg(dev->dev, "GATT PCI BAR not initialized.\n");
> +		pg->gatt_start = 0x40000000;
> +		pg->gatt_pages = (128 * 1024 * 1024) >> PAGE_SHIFT;
> +		/*
> +		 * This is a little confusing but in fact the GTT is providing
> +		 * a view from the GPU into memory and not vice versa. As such
> +		 *  this is really allocating space that is not the same as the
> +		 *  CPU address space on CDV.
> +		 */
> +		fudge.start = 0x40000000;
> +		fudge.end = 0x40000000 + 128 * 1024 * 1024 - 1;
> +		fudge.name = "fudge";
> +		fudge.flags = IORESOURCE_MEM;
> +		dev_priv->gtt_mem = &fudge;
> +	}
> +
> +	pci_read_config_dword(pdev, PSB_BSM, &dev_priv->stolen_base);
> +	vram_stolen_size = pg->gtt_phys_start - dev_priv->stolen_base - PAGE_SIZE;
> +	stolen_size = vram_stolen_size;
> +
> +	dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n",
> +			dev_priv->stolen_base, vram_stolen_size / 1024);
}

And then use this helper in both init and resume?

> +
> +	if ((gtt_pages != pg->gtt_pages) && (stolen_size != pg->stolen_size)) {
> +		dev_err(dev->dev, "GTT resume error.\n");
> +		ret = -EINVAL;
> +		goto out_err;
> +	}
> +

> +	pg->gtt_pages = gtt_pages;
> +	pg->stolen_size = stolen_size;
Not needed for resume, we just checked them.

> +	dev_priv->vram_stolen_size = vram_stolen_size;
> +
> +	psb_gtt_clear(dev_priv);
> +	psb_gtt_populate_stolen(dev_priv);
> +
> +	return 0;
> +
> +out_err:
> +	psb_gtt_takedown(dev);
> +	return ret;
> +}
> +
>  int psb_gtt_restore(struct drm_device *dev)
>  {
>  	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>  
> -	psb_gtt_init(dev, 1);
> +	psb_gtt_resume(dev);
>  
>  	psb_gtt_populate_resources(dev_priv);
>  
> diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
> index 31500533ac45..cb270ea40794 100644
> --- a/drivers/gpu/drm/gma500/gtt.h
> +++ b/drivers/gpu/drm/gma500/gtt.h
> @@ -25,7 +25,7 @@ struct psb_gtt {
>  };
>  
>  /* Exported functions */
> -extern int psb_gtt_init(struct drm_device *dev, int resume);
> +int psb_gtt_init(struct drm_device *dev);
>  extern void psb_gtt_takedown(struct drm_device *dev);
>  extern int psb_gtt_restore(struct drm_device *dev);
A cleanup patch to remove all extern would be nice.
But that's not related to this series.

	Sam

Re: [PATCH 06/10] drm/gma500: Move GTT resume logic out of psb_gtt_init()

[Patrik Jakobsson]

On Mon, Mar 7, 2022 at 8:07 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Thomas,
>
> One comment below.
>
> On Sun, Mar 06, 2022 at 09:36:15PM +0100, Thomas Zimmermann wrote:
> > The current implementation of psb_gtt_init() also does resume
> > handling. Move the resume code into its own helper.
> >
> > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > ---
> >  drivers/gpu/drm/gma500/gtt.c     | 122 ++++++++++++++++++++++++++-----
> >  drivers/gpu/drm/gma500/gtt.h     |   2 +-
> >  drivers/gpu/drm/gma500/psb_drv.c |   2 +-
> >  3 files changed, 104 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
> > index acd50ee26b03..43ad3ec38c80 100644
> > --- a/drivers/gpu/drm/gma500/gtt.c
> > +++ b/drivers/gpu/drm/gma500/gtt.c
> > @@ -209,7 +209,7 @@ static void psb_gtt_populate_resources(struct drm_psb_private *pdev)
> >       drm_dbg(dev, "Restored %u of %u gtt ranges (%u KB)", restored, total, (size / 1024));
> >  }
> >
> > -int psb_gtt_init(struct drm_device *dev, int resume)
> > +int psb_gtt_init(struct drm_device *dev)
> >  {
> >       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> >       struct pci_dev *pdev = to_pci_dev(dev->dev);
> > @@ -218,10 +218,8 @@ int psb_gtt_init(struct drm_device *dev, int resume)
> >       struct psb_gtt *pg;
> >       int ret = 0;
> >
> > -     if (!resume) {
> > -             mutex_init(&dev_priv->gtt_mutex);
> > -             mutex_init(&dev_priv->mmap_mutex);
> > -     }
> > +     mutex_init(&dev_priv->gtt_mutex);
> > +     mutex_init(&dev_priv->mmap_mutex);
> >
> >       pg = &dev_priv->gtt;
> >
> > @@ -290,13 +288,6 @@ int psb_gtt_init(struct drm_device *dev, int resume)
> >       dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n",
> >                       dev_priv->stolen_base, vram_stolen_size / 1024);
> >
> > -     if (resume && (gtt_pages != pg->gtt_pages) &&
> > -         (stolen_size != pg->stolen_size)) {
> > -             dev_err(dev->dev, "GTT resume error.\n");
> > -             ret = -EINVAL;
> > -             goto out_err;
> > -     }
> > -
> >       pg->gtt_pages = gtt_pages;
> >       pg->stolen_size = stolen_size;
> >       dev_priv->vram_stolen_size = vram_stolen_size;
> > @@ -304,19 +295,14 @@ int psb_gtt_init(struct drm_device *dev, int resume)
> >       /*
> >        *      Map the GTT and the stolen memory area
> >        */
> > -     if (!resume)
> > -             dev_priv->gtt_map = ioremap(pg->gtt_phys_start,
> > -                                             gtt_pages << PAGE_SHIFT);
> > +     dev_priv->gtt_map = ioremap(pg->gtt_phys_start, gtt_pages << PAGE_SHIFT);
> >       if (!dev_priv->gtt_map) {
> >               dev_err(dev->dev, "Failure to map gtt.\n");
> >               ret = -ENOMEM;
> >               goto out_err;
> >       }
> >
> > -     if (!resume)
> > -             dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base,
> > -                                              stolen_size);
> > -
> > +     dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base, stolen_size);
> >       if (!dev_priv->vram_addr) {
> >               dev_err(dev->dev, "Failure to map stolen base.\n");
> >               ret = -ENOMEM;
> > @@ -333,11 +319,107 @@ int psb_gtt_init(struct drm_device *dev, int resume)
> >       return ret;
> >  }
> >
> The below is a lot of duplicated complex code.
> Can you add one more helper for this?

I was thinking the same but figured it would be an easy follow up
patch. But sure, why not fix it here already.

While at it, the gtt enable/disable code could be put in helpers as well.

>
> > +static int psb_gtt_resume(struct drm_device *dev)
> > +{
> > +     struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> > +     struct pci_dev *pdev = to_pci_dev(dev->dev);
> > +     unsigned int gtt_pages;
> > +     unsigned long stolen_size, vram_stolen_size;
> > +     struct psb_gtt *pg;
> > +     int ret = 0;
> > +
> > +     pg = &dev_priv->gtt;
>
> static void psb_enable_gtt(..)
> {
> > +
> > +     /* Enable the GTT */
> > +     pci_read_config_word(pdev, PSB_GMCH_CTRL, &dev_priv->gmch_ctrl);
> > +     pci_write_config_word(pdev, PSB_GMCH_CTRL,
> > +                           dev_priv->gmch_ctrl | _PSB_GMCH_ENABLED);
> > +
> > +     dev_priv->pge_ctl = PSB_RVDC32(PSB_PGETBL_CTL);
> > +     PSB_WVDC32(dev_priv->pge_ctl | _PSB_PGETBL_ENABLED, PSB_PGETBL_CTL);
> > +     (void) PSB_RVDC32(PSB_PGETBL_CTL);
> > +
> > +     /* The root resource we allocate address space from */
> > +     dev_priv->gtt_initialized = 1;
> > +
> > +     pg->gtt_phys_start = dev_priv->pge_ctl & PAGE_MASK;
> > +
> > +     /*
> > +      *      The video mmu has a hw bug when accessing 0x0D0000000.
> > +      *      Make gatt start at 0x0e000,0000. This doesn't actually
> > +      *      matter for us but may do if the video acceleration ever
> > +      *      gets opened up.
> > +      */
> > +     pg->mmu_gatt_start = 0xE0000000;
> > +
> > +     pg->gtt_start = pci_resource_start(pdev, PSB_GTT_RESOURCE);
> > +     gtt_pages = pci_resource_len(pdev, PSB_GTT_RESOURCE) >> PAGE_SHIFT;
> > +     /* CDV doesn't report this. In which case the system has 64 gtt pages */
> > +     if (pg->gtt_start == 0 || gtt_pages == 0) {
> > +             dev_dbg(dev->dev, "GTT PCI BAR not initialized.\n");
> > +             gtt_pages = 64;
> > +             pg->gtt_start = dev_priv->pge_ctl;
> > +     }
> > +
> > +     pg->gatt_start = pci_resource_start(pdev, PSB_GATT_RESOURCE);
> > +     pg->gatt_pages = pci_resource_len(pdev, PSB_GATT_RESOURCE) >> PAGE_SHIFT;
> > +     dev_priv->gtt_mem = &pdev->resource[PSB_GATT_RESOURCE];
> > +
> > +     if (pg->gatt_pages == 0 || pg->gatt_start == 0) {
> > +             static struct resource fudge;   /* Preferably peppermint */
> > +             /*
> > +              * This can occur on CDV systems. Fudge it in this case. We
> > +              * really don't care what imaginary space is being allocated
> > +              * at this point.
> > +              */
> > +             dev_dbg(dev->dev, "GATT PCI BAR not initialized.\n");
> > +             pg->gatt_start = 0x40000000;
> > +             pg->gatt_pages = (128 * 1024 * 1024) >> PAGE_SHIFT;
> > +             /*
> > +              * This is a little confusing but in fact the GTT is providing
> > +              * a view from the GPU into memory and not vice versa. As such
> > +              *  this is really allocating space that is not the same as the
> > +              *  CPU address space on CDV.
> > +              */
> > +             fudge.start = 0x40000000;
> > +             fudge.end = 0x40000000 + 128 * 1024 * 1024 - 1;
> > +             fudge.name = "fudge";
> > +             fudge.flags = IORESOURCE_MEM;
> > +             dev_priv->gtt_mem = &fudge;
> > +     }
> > +
> > +     pci_read_config_dword(pdev, PSB_BSM, &dev_priv->stolen_base);
> > +     vram_stolen_size = pg->gtt_phys_start - dev_priv->stolen_base - PAGE_SIZE;
> > +     stolen_size = vram_stolen_size;
> > +
> > +     dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n",
> > +                     dev_priv->stolen_base, vram_stolen_size / 1024);
> }
>
> And then use this helper in both init and resume?
>
> > +
> > +     if ((gtt_pages != pg->gtt_pages) && (stolen_size != pg->stolen_size)) {
> > +             dev_err(dev->dev, "GTT resume error.\n");
> > +             ret = -EINVAL;
> > +             goto out_err;
> > +     }
> > +
>
> > +     pg->gtt_pages = gtt_pages;
> > +     pg->stolen_size = stolen_size;
> Not needed for resume, we just checked them.
>
> > +     dev_priv->vram_stolen_size = vram_stolen_size;
> > +
> > +     psb_gtt_clear(dev_priv);
> > +     psb_gtt_populate_stolen(dev_priv);
> > +
> > +     return 0;
> > +
> > +out_err:
> > +     psb_gtt_takedown(dev);
> > +     return ret;
> > +}
> > +
> >  int psb_gtt_restore(struct drm_device *dev)
> >  {
> >       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> >
> > -     psb_gtt_init(dev, 1);
> > +     psb_gtt_resume(dev);
> >
> >       psb_gtt_populate_resources(dev_priv);
> >
> > diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
> > index 31500533ac45..cb270ea40794 100644
> > --- a/drivers/gpu/drm/gma500/gtt.h
> > +++ b/drivers/gpu/drm/gma500/gtt.h
> > @@ -25,7 +25,7 @@ struct psb_gtt {
> >  };
> >
> >  /* Exported functions */
> > -extern int psb_gtt_init(struct drm_device *dev, int resume);
> > +int psb_gtt_init(struct drm_device *dev);
> >  extern void psb_gtt_takedown(struct drm_device *dev);
> >  extern int psb_gtt_restore(struct drm_device *dev);
> A cleanup patch to remove all extern would be nice.
> But that's not related to this series.
>
>         Sam

Re: [PATCH 06/10] drm/gma500: Move GTT resume logic out of psb_gtt_init()

[Thomas Zimmermann]

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

Hi Sam and Patrik

Am 07.03.22 um 22:02 schrieb Patrik Jakobsson:
> On Mon, Mar 7, 2022 at 8:07 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>>
>> Hi Thomas,
>>
>> One comment below.
>>
>> On Sun, Mar 06, 2022 at 09:36:15PM +0100, Thomas Zimmermann wrote:
>>> The current implementation of psb_gtt_init() also does resume
>>> handling. Move the resume code into its own helper.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>>   drivers/gpu/drm/gma500/gtt.c     | 122 ++++++++++++++++++++++++++-----
>>>   drivers/gpu/drm/gma500/gtt.h     |   2 +-
>>>   drivers/gpu/drm/gma500/psb_drv.c |   2 +-
>>>   3 files changed, 104 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
>>> index acd50ee26b03..43ad3ec38c80 100644
>>> --- a/drivers/gpu/drm/gma500/gtt.c
>>> +++ b/drivers/gpu/drm/gma500/gtt.c
>>> @@ -209,7 +209,7 @@ static void psb_gtt_populate_resources(struct drm_psb_private *pdev)
>>>        drm_dbg(dev, "Restored %u of %u gtt ranges (%u KB)", restored, total, (size / 1024));
>>>   }
>>>
>>> -int psb_gtt_init(struct drm_device *dev, int resume)
>>> +int psb_gtt_init(struct drm_device *dev)
>>>   {
>>>        struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>>>        struct pci_dev *pdev = to_pci_dev(dev->dev);
>>> @@ -218,10 +218,8 @@ int psb_gtt_init(struct drm_device *dev, int resume)
>>>        struct psb_gtt *pg;
>>>        int ret = 0;
>>>
>>> -     if (!resume) {
>>> -             mutex_init(&dev_priv->gtt_mutex);
>>> -             mutex_init(&dev_priv->mmap_mutex);
>>> -     }
>>> +     mutex_init(&dev_priv->gtt_mutex);
>>> +     mutex_init(&dev_priv->mmap_mutex);
>>>
>>>        pg = &dev_priv->gtt;
>>>
>>> @@ -290,13 +288,6 @@ int psb_gtt_init(struct drm_device *dev, int resume)
>>>        dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n",
>>>                        dev_priv->stolen_base, vram_stolen_size / 1024);
>>>
>>> -     if (resume && (gtt_pages != pg->gtt_pages) &&
>>> -         (stolen_size != pg->stolen_size)) {
>>> -             dev_err(dev->dev, "GTT resume error.\n");
>>> -             ret = -EINVAL;
>>> -             goto out_err;
>>> -     }
>>> -
>>>        pg->gtt_pages = gtt_pages;
>>>        pg->stolen_size = stolen_size;
>>>        dev_priv->vram_stolen_size = vram_stolen_size;
>>> @@ -304,19 +295,14 @@ int psb_gtt_init(struct drm_device *dev, int resume)
>>>        /*
>>>         *      Map the GTT and the stolen memory area
>>>         */
>>> -     if (!resume)
>>> -             dev_priv->gtt_map = ioremap(pg->gtt_phys_start,
>>> -                                             gtt_pages << PAGE_SHIFT);
>>> +     dev_priv->gtt_map = ioremap(pg->gtt_phys_start, gtt_pages << PAGE_SHIFT);
>>>        if (!dev_priv->gtt_map) {
>>>                dev_err(dev->dev, "Failure to map gtt.\n");
>>>                ret = -ENOMEM;
>>>                goto out_err;
>>>        }
>>>
>>> -     if (!resume)
>>> -             dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base,
>>> -                                              stolen_size);
>>> -
>>> +     dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base, stolen_size);
>>>        if (!dev_priv->vram_addr) {
>>>                dev_err(dev->dev, "Failure to map stolen base.\n");
>>>                ret = -ENOMEM;
>>> @@ -333,11 +319,107 @@ int psb_gtt_init(struct drm_device *dev, int resume)
>>>        return ret;
>>>   }
>>>
>> The below is a lot of duplicated complex code.
>> Can you add one more helper for this?
> 

That makes a lot of sense.

> I was thinking the same but figured it would be an easy follow up
> patch. But sure, why not fix it here already.
> 
> While at it, the gtt enable/disable code could be put in helpers as well.

Patrik, do you know why the code re-reads all these values during 
resume? Do the values change?  The driver never seems to do anything 
with the updated values. For example, it doesn't update the GTT mapping 
if gtt_phys_start changes.  Can we remove all that code from the resume 
function?

Best regards
Thomas

> 
>>
>>> +static int psb_gtt_resume(struct drm_device *dev)
>>> +{
>>> +     struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>>> +     struct pci_dev *pdev = to_pci_dev(dev->dev);
>>> +     unsigned int gtt_pages;
>>> +     unsigned long stolen_size, vram_stolen_size;
>>> +     struct psb_gtt *pg;
>>> +     int ret = 0;
>>> +
>>> +     pg = &dev_priv->gtt;
>>
>> static void psb_enable_gtt(..)
>> {
>>> +
>>> +     /* Enable the GTT */
>>> +     pci_read_config_word(pdev, PSB_GMCH_CTRL, &dev_priv->gmch_ctrl);
>>> +     pci_write_config_word(pdev, PSB_GMCH_CTRL,
>>> +                           dev_priv->gmch_ctrl | _PSB_GMCH_ENABLED);
>>> +
>>> +     dev_priv->pge_ctl = PSB_RVDC32(PSB_PGETBL_CTL);
>>> +     PSB_WVDC32(dev_priv->pge_ctl | _PSB_PGETBL_ENABLED, PSB_PGETBL_CTL);
>>> +     (void) PSB_RVDC32(PSB_PGETBL_CTL);
>>> +
>>> +     /* The root resource we allocate address space from */
>>> +     dev_priv->gtt_initialized = 1;
>>> +
>>> +     pg->gtt_phys_start = dev_priv->pge_ctl & PAGE_MASK;
>>> +
>>> +     /*
>>> +      *      The video mmu has a hw bug when accessing 0x0D0000000.
>>> +      *      Make gatt start at 0x0e000,0000. This doesn't actually
>>> +      *      matter for us but may do if the video acceleration ever
>>> +      *      gets opened up.
>>> +      */
>>> +     pg->mmu_gatt_start = 0xE0000000;
>>> +
>>> +     pg->gtt_start = pci_resource_start(pdev, PSB_GTT_RESOURCE);
>>> +     gtt_pages = pci_resource_len(pdev, PSB_GTT_RESOURCE) >> PAGE_SHIFT;
>>> +     /* CDV doesn't report this. In which case the system has 64 gtt pages */
>>> +     if (pg->gtt_start == 0 || gtt_pages == 0) {
>>> +             dev_dbg(dev->dev, "GTT PCI BAR not initialized.\n");
>>> +             gtt_pages = 64;
>>> +             pg->gtt_start = dev_priv->pge_ctl;
>>> +     }
>>> +
>>> +     pg->gatt_start = pci_resource_start(pdev, PSB_GATT_RESOURCE);
>>> +     pg->gatt_pages = pci_resource_len(pdev, PSB_GATT_RESOURCE) >> PAGE_SHIFT;
>>> +     dev_priv->gtt_mem = &pdev->resource[PSB_GATT_RESOURCE];
>>> +
>>> +     if (pg->gatt_pages == 0 || pg->gatt_start == 0) {
>>> +             static struct resource fudge;   /* Preferably peppermint */
>>> +             /*
>>> +              * This can occur on CDV systems. Fudge it in this case. We
>>> +              * really don't care what imaginary space is being allocated
>>> +              * at this point.
>>> +              */
>>> +             dev_dbg(dev->dev, "GATT PCI BAR not initialized.\n");
>>> +             pg->gatt_start = 0x40000000;
>>> +             pg->gatt_pages = (128 * 1024 * 1024) >> PAGE_SHIFT;
>>> +             /*
>>> +              * This is a little confusing but in fact the GTT is providing
>>> +              * a view from the GPU into memory and not vice versa. As such
>>> +              *  this is really allocating space that is not the same as the
>>> +              *  CPU address space on CDV.
>>> +              */
>>> +             fudge.start = 0x40000000;
>>> +             fudge.end = 0x40000000 + 128 * 1024 * 1024 - 1;
>>> +             fudge.name = "fudge";
>>> +             fudge.flags = IORESOURCE_MEM;
>>> +             dev_priv->gtt_mem = &fudge;
>>> +     }
>>> +
>>> +     pci_read_config_dword(pdev, PSB_BSM, &dev_priv->stolen_base);
>>> +     vram_stolen_size = pg->gtt_phys_start - dev_priv->stolen_base - PAGE_SIZE;
>>> +     stolen_size = vram_stolen_size;
>>> +
>>> +     dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n",
>>> +                     dev_priv->stolen_base, vram_stolen_size / 1024);
>> }
>>
>> And then use this helper in both init and resume?
>>
>>> +
>>> +     if ((gtt_pages != pg->gtt_pages) && (stolen_size != pg->stolen_size)) {
>>> +             dev_err(dev->dev, "GTT resume error.\n");
>>> +             ret = -EINVAL;
>>> +             goto out_err;
>>> +     }
>>> +
>>
>>> +     pg->gtt_pages = gtt_pages;
>>> +     pg->stolen_size = stolen_size;
>> Not needed for resume, we just checked them.
>>
>>> +     dev_priv->vram_stolen_size = vram_stolen_size;
>>> +
>>> +     psb_gtt_clear(dev_priv);
>>> +     psb_gtt_populate_stolen(dev_priv);
>>> +
>>> +     return 0;
>>> +
>>> +out_err:
>>> +     psb_gtt_takedown(dev);
>>> +     return ret;
>>> +}
>>> +
>>>   int psb_gtt_restore(struct drm_device *dev)
>>>   {
>>>        struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>>>
>>> -     psb_gtt_init(dev, 1);
>>> +     psb_gtt_resume(dev);
>>>
>>>        psb_gtt_populate_resources(dev_priv);
>>>
>>> diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
>>> index 31500533ac45..cb270ea40794 100644
>>> --- a/drivers/gpu/drm/gma500/gtt.h
>>> +++ b/drivers/gpu/drm/gma500/gtt.h
>>> @@ -25,7 +25,7 @@ struct psb_gtt {
>>>   };
>>>
>>>   /* Exported functions */
>>> -extern int psb_gtt_init(struct drm_device *dev, int resume);
>>> +int psb_gtt_init(struct drm_device *dev);
>>>   extern void psb_gtt_takedown(struct drm_device *dev);
>>>   extern int psb_gtt_restore(struct drm_device *dev);
>> A cleanup patch to remove all extern would be nice.
>> But that's not related to this series.
>>
>>          Sam

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 06/10] drm/gma500: Move GTT resume logic out of psb_gtt_init()

[Patrik Jakobsson]

On Tue, Mar 8, 2022 at 9:48 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi Sam and Patrik
>
> Am 07.03.22 um 22:02 schrieb Patrik Jakobsson:
> > On Mon, Mar 7, 2022 at 8:07 PM Sam Ravnborg <sam@ravnborg.org> wrote:
> >>
> >> Hi Thomas,
> >>
> >> One comment below.
> >>
> >> On Sun, Mar 06, 2022 at 09:36:15PM +0100, Thomas Zimmermann wrote:
> >>> The current implementation of psb_gtt_init() also does resume
> >>> handling. Move the resume code into its own helper.
> >>>
> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>> ---
> >>>   drivers/gpu/drm/gma500/gtt.c     | 122 ++++++++++++++++++++++++++-----
> >>>   drivers/gpu/drm/gma500/gtt.h     |   2 +-
> >>>   drivers/gpu/drm/gma500/psb_drv.c |   2 +-
> >>>   3 files changed, 104 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
> >>> index acd50ee26b03..43ad3ec38c80 100644
> >>> --- a/drivers/gpu/drm/gma500/gtt.c
> >>> +++ b/drivers/gpu/drm/gma500/gtt.c
> >>> @@ -209,7 +209,7 @@ static void psb_gtt_populate_resources(struct drm_psb_private *pdev)
> >>>        drm_dbg(dev, "Restored %u of %u gtt ranges (%u KB)", restored, total, (size / 1024));
> >>>   }
> >>>
> >>> -int psb_gtt_init(struct drm_device *dev, int resume)
> >>> +int psb_gtt_init(struct drm_device *dev)
> >>>   {
> >>>        struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> >>>        struct pci_dev *pdev = to_pci_dev(dev->dev);
> >>> @@ -218,10 +218,8 @@ int psb_gtt_init(struct drm_device *dev, int resume)
> >>>        struct psb_gtt *pg;
> >>>        int ret = 0;
> >>>
> >>> -     if (!resume) {
> >>> -             mutex_init(&dev_priv->gtt_mutex);
> >>> -             mutex_init(&dev_priv->mmap_mutex);
> >>> -     }
> >>> +     mutex_init(&dev_priv->gtt_mutex);
> >>> +     mutex_init(&dev_priv->mmap_mutex);
> >>>
> >>>        pg = &dev_priv->gtt;
> >>>
> >>> @@ -290,13 +288,6 @@ int psb_gtt_init(struct drm_device *dev, int resume)
> >>>        dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n",
> >>>                        dev_priv->stolen_base, vram_stolen_size / 1024);
> >>>
> >>> -     if (resume && (gtt_pages != pg->gtt_pages) &&
> >>> -         (stolen_size != pg->stolen_size)) {
> >>> -             dev_err(dev->dev, "GTT resume error.\n");
> >>> -             ret = -EINVAL;
> >>> -             goto out_err;
> >>> -     }
> >>> -
> >>>        pg->gtt_pages = gtt_pages;
> >>>        pg->stolen_size = stolen_size;
> >>>        dev_priv->vram_stolen_size = vram_stolen_size;
> >>> @@ -304,19 +295,14 @@ int psb_gtt_init(struct drm_device *dev, int resume)
> >>>        /*
> >>>         *      Map the GTT and the stolen memory area
> >>>         */
> >>> -     if (!resume)
> >>> -             dev_priv->gtt_map = ioremap(pg->gtt_phys_start,
> >>> -                                             gtt_pages << PAGE_SHIFT);
> >>> +     dev_priv->gtt_map = ioremap(pg->gtt_phys_start, gtt_pages << PAGE_SHIFT);
> >>>        if (!dev_priv->gtt_map) {
> >>>                dev_err(dev->dev, "Failure to map gtt.\n");
> >>>                ret = -ENOMEM;
> >>>                goto out_err;
> >>>        }
> >>>
> >>> -     if (!resume)
> >>> -             dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base,
> >>> -                                              stolen_size);
> >>> -
> >>> +     dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base, stolen_size);
> >>>        if (!dev_priv->vram_addr) {
> >>>                dev_err(dev->dev, "Failure to map stolen base.\n");
> >>>                ret = -ENOMEM;
> >>> @@ -333,11 +319,107 @@ int psb_gtt_init(struct drm_device *dev, int resume)
> >>>        return ret;
> >>>   }
> >>>
> >> The below is a lot of duplicated complex code.
> >> Can you add one more helper for this?
> >
>
> That makes a lot of sense.
>
> > I was thinking the same but figured it would be an easy follow up
> > patch. But sure, why not fix it here already.
> >
> > While at it, the gtt enable/disable code could be put in helpers as well.
>
> Patrik, do you know why the code re-reads all these values during
> resume? Do the values change?  The driver never seems to do anything
> with the updated values. For example, it doesn't update the GTT mapping
> if gtt_phys_start changes.  Can we remove all that code from the resume
> function?

In theory these values can change if you hibernate, make changes in
the bios and then resume. I think I've seen bioses that can change the
stolen size and/or aperture size. Not sure if that would also change
the value in eg. pge_ctl or the size of the gtt. I would have to do
some testing on real hardware to figure this out.
But as you say, the above scenario is already broken. For the time
being, can we perhaps just fail gracefully?

Re: [PATCH 06/10] drm/gma500: Move GTT resume logic out of psb_gtt_init()

[Thomas Zimmermann]

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

Hi

Am 08.03.22 um 13:07 schrieb Patrik Jakobsson:
> On Tue, Mar 8, 2022 at 9:48 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Hi Sam and Patrik
>>
>> Am 07.03.22 um 22:02 schrieb Patrik Jakobsson:
>>> On Mon, Mar 7, 2022 at 8:07 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>>>>
>>>> Hi Thomas,
>>>>
>>>> One comment below.
>>>>
>>>> On Sun, Mar 06, 2022 at 09:36:15PM +0100, Thomas Zimmermann wrote:
>>>>> The current implementation of psb_gtt_init() also does resume
>>>>> handling. Move the resume code into its own helper.
>>>>>
>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>> ---
>>>>>    drivers/gpu/drm/gma500/gtt.c     | 122 ++++++++++++++++++++++++++-----
>>>>>    drivers/gpu/drm/gma500/gtt.h     |   2 +-
>>>>>    drivers/gpu/drm/gma500/psb_drv.c |   2 +-
>>>>>    3 files changed, 104 insertions(+), 22 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
>>>>> index acd50ee26b03..43ad3ec38c80 100644
>>>>> --- a/drivers/gpu/drm/gma500/gtt.c
>>>>> +++ b/drivers/gpu/drm/gma500/gtt.c
>>>>> @@ -209,7 +209,7 @@ static void psb_gtt_populate_resources(struct drm_psb_private *pdev)
>>>>>         drm_dbg(dev, "Restored %u of %u gtt ranges (%u KB)", restored, total, (size / 1024));
>>>>>    }
>>>>>
>>>>> -int psb_gtt_init(struct drm_device *dev, int resume)
>>>>> +int psb_gtt_init(struct drm_device *dev)
>>>>>    {
>>>>>         struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>>>>>         struct pci_dev *pdev = to_pci_dev(dev->dev);
>>>>> @@ -218,10 +218,8 @@ int psb_gtt_init(struct drm_device *dev, int resume)
>>>>>         struct psb_gtt *pg;
>>>>>         int ret = 0;
>>>>>
>>>>> -     if (!resume) {
>>>>> -             mutex_init(&dev_priv->gtt_mutex);
>>>>> -             mutex_init(&dev_priv->mmap_mutex);
>>>>> -     }
>>>>> +     mutex_init(&dev_priv->gtt_mutex);
>>>>> +     mutex_init(&dev_priv->mmap_mutex);
>>>>>
>>>>>         pg = &dev_priv->gtt;
>>>>>
>>>>> @@ -290,13 +288,6 @@ int psb_gtt_init(struct drm_device *dev, int resume)
>>>>>         dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n",
>>>>>                         dev_priv->stolen_base, vram_stolen_size / 1024);
>>>>>
>>>>> -     if (resume && (gtt_pages != pg->gtt_pages) &&
>>>>> -         (stolen_size != pg->stolen_size)) {
>>>>> -             dev_err(dev->dev, "GTT resume error.\n");
>>>>> -             ret = -EINVAL;
>>>>> -             goto out_err;
>>>>> -     }
>>>>> -
>>>>>         pg->gtt_pages = gtt_pages;
>>>>>         pg->stolen_size = stolen_size;
>>>>>         dev_priv->vram_stolen_size = vram_stolen_size;
>>>>> @@ -304,19 +295,14 @@ int psb_gtt_init(struct drm_device *dev, int resume)
>>>>>         /*
>>>>>          *      Map the GTT and the stolen memory area
>>>>>          */
>>>>> -     if (!resume)
>>>>> -             dev_priv->gtt_map = ioremap(pg->gtt_phys_start,
>>>>> -                                             gtt_pages << PAGE_SHIFT);
>>>>> +     dev_priv->gtt_map = ioremap(pg->gtt_phys_start, gtt_pages << PAGE_SHIFT);
>>>>>         if (!dev_priv->gtt_map) {
>>>>>                 dev_err(dev->dev, "Failure to map gtt.\n");
>>>>>                 ret = -ENOMEM;
>>>>>                 goto out_err;
>>>>>         }
>>>>>
>>>>> -     if (!resume)
>>>>> -             dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base,
>>>>> -                                              stolen_size);
>>>>> -
>>>>> +     dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base, stolen_size);
>>>>>         if (!dev_priv->vram_addr) {
>>>>>                 dev_err(dev->dev, "Failure to map stolen base.\n");
>>>>>                 ret = -ENOMEM;
>>>>> @@ -333,11 +319,107 @@ int psb_gtt_init(struct drm_device *dev, int resume)
>>>>>         return ret;
>>>>>    }
>>>>>
>>>> The below is a lot of duplicated complex code.
>>>> Can you add one more helper for this?
>>>
>>
>> That makes a lot of sense.
>>
>>> I was thinking the same but figured it would be an easy follow up
>>> patch. But sure, why not fix it here already.
>>>
>>> While at it, the gtt enable/disable code could be put in helpers as well.
>>
>> Patrik, do you know why the code re-reads all these values during
>> resume? Do the values change?  The driver never seems to do anything
>> with the updated values. For example, it doesn't update the GTT mapping
>> if gtt_phys_start changes.  Can we remove all that code from the resume
>> function?
> 
> In theory these values can change if you hibernate, make changes in
> the bios and then resume. I think I've seen bioses that can change the
> stolen size and/or aperture size. Not sure if that would also change
> the value in eg. pge_ctl or the size of the gtt. I would have to do
> some testing on real hardware to figure this out.
> But as you say, the above scenario is already broken. For the time
> being, can we perhaps just fail gracefully?

Ah, thanks for the explanation. I'll leave the current logic as-is.

Best regards
Thomas


-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]