[PATCH 00/10] drm/gma500: Refactor GEM code

[PATCH 00/10] drm/gma500: Refactor GEM code

[Thomas Zimmermann]

Bring GEM code up to current standards and untangle the connection to
GTT helpers.

The allocation and pinning helpers for struct gtt_range are located in
the GTT code, but actually part of the GEM implementation. The patchset
moves them to GEM code and refactors much of the implementation. Most
of the GTT code is then independend from the struct gtt_range, while
the GEM code does not contain GTT management.

In addition to internal refiactoring, patches 2 to 4 update the rest of
the driver to use the GEM interfaces for object allocation and release.

Finally, rename struct gtt_range to struct psb_gem_object to designate
it as a 'real' GEM object.

Future work: with the GEM and GTT code separated, future patchsets can
implement on-demand release of GTT entries, or remove the perma-mapping
of stolen memory. Dma-buf support might also be added.

Tested on Atom N2800 hardware.

Thomas Zimmermann (10):
  drm/gma500: Move helpers for struct gtt_range from gtt.c to gem.c
  drm/gma500: Use to_gtt_range() everywhere
  drm/gma500: Reimplement psb_gem_create()
  drm/gma500: Allocate GTT ranges in stolen memory with psb_gem_create()
  drm/gma500: Rename psb_gtt_{pin,unpin}() to psb_gem_{pin,unpin}()
  drm/gma500: Inline psb_gtt_attach_pages() and psb_gtt_detach_pages()
  drm/gma500: Inline psb_gtt_{alloc,free}_range() into rsp callers
  drm/gma500: Set page-caching flags in GEM pin/unpin
  drm/gma500: Rewrite GTT page insert/remove without struct gtt_range
  drm/gma500: Rename struct gtt_range to struct psb_gem_object

 drivers/gpu/drm/gma500/framebuffer.c       |  52 +---
 drivers/gpu/drm/gma500/gem.c               | 227 +++++++++++----
 drivers/gpu/drm/gma500/gem.h               |  28 +-
 drivers/gpu/drm/gma500/gma_display.c       |  51 ++--
 drivers/gpu/drm/gma500/gtt.c               | 320 ++++-----------------
 drivers/gpu/drm/gma500/gtt.h               |  29 +-
 drivers/gpu/drm/gma500/oaktrail_crtc.c     |   3 +-
 drivers/gpu/drm/gma500/psb_intel_display.c |  17 +-
 drivers/gpu/drm/gma500/psb_intel_drv.h     |   2 +-
 9 files changed, 310 insertions(+), 419 deletions(-)

--
2.33.0

[PATCH 01/10] drm/gma500: Move helpers for struct gtt_range from gtt.c to gem.c

[Thomas Zimmermann]

Allocation and pinning helpers for struct gtt_range are GEM functions,
so move them to gem.c. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/gma500/framebuffer.c       |   1 -
 drivers/gpu/drm/gma500/gem.c               | 133 +++++++++++++--
 drivers/gpu/drm/gma500/gem.h               |   8 +
 drivers/gpu/drm/gma500/gma_display.c       |   1 +
 drivers/gpu/drm/gma500/gtt.c               | 190 +--------------------
 drivers/gpu/drm/gma500/gtt.h               |  11 +-
 drivers/gpu/drm/gma500/psb_intel_display.c |   1 +
 7 files changed, 136 insertions(+), 209 deletions(-)

diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
index 321e416489a9..ce92d11bd20f 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -25,7 +25,6 @@
 
 #include "framebuffer.h"
 #include "gem.h"
-#include "gtt.h"
 #include "psb_drv.h"
 #include "psb_intel_drv.h"
 #include "psb_intel_reg.h"
diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index 5ae54c9d2819..734bcb7a80c8 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -19,6 +19,89 @@
 #include "gem.h"
 #include "psb_drv.h"
 
+static int psb_gtt_attach_pages(struct gtt_range *gt)
+{
+	struct page **pages;
+
+	WARN_ON(gt->pages);
+
+	pages = drm_gem_get_pages(&gt->gem);
+	if (IS_ERR(pages))
+		return PTR_ERR(pages);
+
+	gt->npage = gt->gem.size / PAGE_SIZE;
+	gt->pages = pages;
+
+	return 0;
+}
+
+static void psb_gtt_detach_pages(struct gtt_range *gt)
+{
+	drm_gem_put_pages(&gt->gem, gt->pages, true, false);
+	gt->pages = NULL;
+}
+
+int psb_gtt_pin(struct gtt_range *gt)
+{
+	int ret = 0;
+	struct drm_device *dev = gt->gem.dev;
+	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
+	u32 gpu_base = dev_priv->gtt.gatt_start;
+
+	mutex_lock(&dev_priv->gtt_mutex);
+
+	if (gt->in_gart == 0 && gt->stolen == 0) {
+		ret = psb_gtt_attach_pages(gt);
+		if (ret < 0)
+			goto out;
+		ret = psb_gtt_insert(dev, gt, 0);
+		if (ret < 0) {
+			psb_gtt_detach_pages(gt);
+			goto out;
+		}
+		psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu),
+				     gt->pages, (gpu_base + gt->offset),
+				     gt->npage, 0, 0, PSB_MMU_CACHED_MEMORY);
+	}
+	gt->in_gart++;
+out:
+	mutex_unlock(&dev_priv->gtt_mutex);
+	return ret;
+}
+
+void psb_gtt_unpin(struct gtt_range *gt)
+{
+	struct drm_device *dev = gt->gem.dev;
+	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
+	u32 gpu_base = dev_priv->gtt.gatt_start;
+
+	mutex_lock(&dev_priv->gtt_mutex);
+
+	WARN_ON(!gt->in_gart);
+
+	gt->in_gart--;
+	if (gt->in_gart == 0 && gt->stolen == 0) {
+		psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu),
+				     (gpu_base + gt->offset), gt->npage, 0, 0);
+		psb_gtt_remove(dev, gt);
+		psb_gtt_detach_pages(gt);
+	}
+
+	mutex_unlock(&dev_priv->gtt_mutex);
+}
+
+void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt)
+{
+	/* Undo the mmap pin if we are destroying the object */
+	if (gt->mmapping) {
+		psb_gtt_unpin(gt);
+		gt->mmapping = 0;
+	}
+	WARN_ON(gt->in_gart && !gt->stolen);
+	release_resource(&gt->resource);
+	kfree(gt);
+}
+
 static vm_fault_t psb_gem_fault(struct vm_fault *vmf);
 
 static void psb_gem_free_object(struct drm_gem_object *obj)
@@ -44,19 +127,43 @@ const struct drm_gem_object_funcs psb_gem_object_funcs = {
 	.vm_ops = &psb_gem_vm_ops,
 };
 
-/**
- *	psb_gem_create		-	create a mappable object
- *	@file: the DRM file of the client
- *	@dev: our device
- *	@size: the size requested
- *	@handlep: returned handle (opaque number)
- *	@stolen: unused
- *	@align: unused
- *
- *	Create a GEM object, fill in the boilerplate and attach a handle to
- *	it so that userspace can speak about it. This does the core work
- *	for the various methods that do/will create GEM objects for things
- */
+struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len,
+				      const char *name, int backed, u32 align)
+{
+	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
+	struct gtt_range *gt;
+	struct resource *r = dev_priv->gtt_mem;
+	int ret;
+	unsigned long start, end;
+
+	if (backed) {
+		/* The start of the GTT is the stolen pages */
+		start = r->start;
+		end = r->start + dev_priv->gtt.stolen_size - 1;
+	} else {
+		/* The rest we will use for GEM backed objects */
+		start = r->start + dev_priv->gtt.stolen_size;
+		end = r->end;
+	}
+
+	gt = kzalloc(sizeof(struct gtt_range), GFP_KERNEL);
+	if (gt == NULL)
+		return NULL;
+	gt->resource.name = name;
+	gt->stolen = backed;
+	gt->in_gart = backed;
+	/* Ensure this is set for non GEM objects */
+	gt->gem.dev = dev;
+	ret = allocate_resource(dev_priv->gtt_mem, &gt->resource,
+				len, start, end, align, NULL, NULL);
+	if (ret == 0) {
+		gt->offset = gt->resource.start - r->start;
+		return gt;
+	}
+	kfree(gt);
+	return NULL;
+}
+
 int psb_gem_create(struct drm_file *file, struct drm_device *dev, u64 size,
 		   u32 *handlep, int stolen, u32 align)
 {
diff --git a/drivers/gpu/drm/gma500/gem.h b/drivers/gpu/drm/gma500/gem.h
index bae6454ead29..275494aedd4c 100644
--- a/drivers/gpu/drm/gma500/gem.h
+++ b/drivers/gpu/drm/gma500/gem.h
@@ -8,6 +8,8 @@
 #ifndef _GEM_H
 #define _GEM_H
 
+#include <drm/drm_gem.h>
+
 struct drm_device;
 
 extern const struct drm_gem_object_funcs psb_gem_object_funcs;
@@ -15,4 +17,10 @@ extern const struct drm_gem_object_funcs psb_gem_object_funcs;
 extern int psb_gem_create(struct drm_file *file, struct drm_device *dev,
 			  u64 size, u32 *handlep, int stolen, u32 align);
 
+struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len, const char *name,
+				      int backed, u32 align);
+void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt);
+int psb_gtt_pin(struct gtt_range *gt);
+void psb_gtt_unpin(struct gtt_range *gt);
+
 #endif
diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c
index cbcecbaa041b..ecf8153416ac 100644
--- a/drivers/gpu/drm/gma500/gma_display.c
+++ b/drivers/gpu/drm/gma500/gma_display.c
@@ -15,6 +15,7 @@
 #include <drm/drm_vblank.h>
 
 #include "framebuffer.h"
+#include "gem.h"
 #include "gma_display.h"
 #include "psb_drv.h"
 #include "psb_intel_drv.h"
diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
index 55a2a6919533..0aacf7122e32 100644
--- a/drivers/gpu/drm/gma500/gtt.c
+++ b/drivers/gpu/drm/gma500/gtt.c
@@ -71,8 +71,7 @@ static u32 __iomem *psb_gtt_entry(struct drm_device *dev, struct gtt_range *r)
  *	the GTT. This is protected via the gtt mutex which the caller
  *	must hold.
  */
-static int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r,
-			  int resume)
+int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume)
 {
 	u32 __iomem *gtt_slot;
 	u32 pte;
@@ -116,7 +115,7 @@ static int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r,
  *	page table entries with the dummy page. This is protected via the gtt
  *	mutex which the caller must hold.
  */
-static void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r)
+void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
 	u32 __iomem *gtt_slot;
@@ -135,191 +134,6 @@ static void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r)
 	set_pages_array_wb(r->pages, r->npage);
 }
 
-/**
- *	psb_gtt_attach_pages	-	attach and pin GEM pages
- *	@gt: the gtt range
- *
- *	Pin and build an in kernel list of the pages that back our GEM object.
- *	While we hold this the pages cannot be swapped out. This is protected
- *	via the gtt mutex which the caller must hold.
- */
-static int psb_gtt_attach_pages(struct gtt_range *gt)
-{
-	struct page **pages;
-
-	WARN_ON(gt->pages);
-
-	pages = drm_gem_get_pages(&gt->gem);
-	if (IS_ERR(pages))
-		return PTR_ERR(pages);
-
-	gt->npage = gt->gem.size / PAGE_SIZE;
-	gt->pages = pages;
-
-	return 0;
-}
-
-/**
- *	psb_gtt_detach_pages	-	attach and pin GEM pages
- *	@gt: the gtt range
- *
- *	Undo the effect of psb_gtt_attach_pages. At this point the pages
- *	must have been removed from the GTT as they could now be paged out
- *	and move bus address. This is protected via the gtt mutex which the
- *	caller must hold.
- */
-static void psb_gtt_detach_pages(struct gtt_range *gt)
-{
-	drm_gem_put_pages(&gt->gem, gt->pages, true, false);
-	gt->pages = NULL;
-}
-
-/**
- *	psb_gtt_pin		-	pin pages into the GTT
- *	@gt: range to pin
- *
- *	Pin a set of pages into the GTT. The pins are refcounted so that
- *	multiple pins need multiple unpins to undo.
- *
- *	Non GEM backed objects treat this as a no-op as they are always GTT
- *	backed objects.
- */
-int psb_gtt_pin(struct gtt_range *gt)
-{
-	int ret = 0;
-	struct drm_device *dev = gt->gem.dev;
-	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
-	u32 gpu_base = dev_priv->gtt.gatt_start;
-
-	mutex_lock(&dev_priv->gtt_mutex);
-
-	if (gt->in_gart == 0 && gt->stolen == 0) {
-		ret = psb_gtt_attach_pages(gt);
-		if (ret < 0)
-			goto out;
-		ret = psb_gtt_insert(dev, gt, 0);
-		if (ret < 0) {
-			psb_gtt_detach_pages(gt);
-			goto out;
-		}
-		psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu),
-				     gt->pages, (gpu_base + gt->offset),
-				     gt->npage, 0, 0, PSB_MMU_CACHED_MEMORY);
-	}
-	gt->in_gart++;
-out:
-	mutex_unlock(&dev_priv->gtt_mutex);
-	return ret;
-}
-
-/**
- *	psb_gtt_unpin		-	Drop a GTT pin requirement
- *	@gt: range to pin
- *
- *	Undoes the effect of psb_gtt_pin. On the last drop the GEM object
- *	will be removed from the GTT which will also drop the page references
- *	and allow the VM to clean up or page stuff.
- *
- *	Non GEM backed objects treat this as a no-op as they are always GTT
- *	backed objects.
- */
-void psb_gtt_unpin(struct gtt_range *gt)
-{
-	struct drm_device *dev = gt->gem.dev;
-	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
-	u32 gpu_base = dev_priv->gtt.gatt_start;
-
-	mutex_lock(&dev_priv->gtt_mutex);
-
-	WARN_ON(!gt->in_gart);
-
-	gt->in_gart--;
-	if (gt->in_gart == 0 && gt->stolen == 0) {
-		psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu),
-				     (gpu_base + gt->offset), gt->npage, 0, 0);
-		psb_gtt_remove(dev, gt);
-		psb_gtt_detach_pages(gt);
-	}
-
-	mutex_unlock(&dev_priv->gtt_mutex);
-}
-
-/*
- *	GTT resource allocator - allocate and manage GTT address space
- */
-
-/**
- *	psb_gtt_alloc_range	-	allocate GTT address space
- *	@dev: Our DRM device
- *	@len: length (bytes) of address space required
- *	@name: resource name
- *	@backed: resource should be backed by stolen pages
- *	@align: requested alignment
- *
- *	Ask the kernel core to find us a suitable range of addresses
- *	to use for a GTT mapping.
- *
- *	Returns a gtt_range structure describing the object, or NULL on
- *	error. On successful return the resource is both allocated and marked
- *	as in use.
- */
-struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len,
-				      const char *name, int backed, u32 align)
-{
-	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
-	struct gtt_range *gt;
-	struct resource *r = dev_priv->gtt_mem;
-	int ret;
-	unsigned long start, end;
-
-	if (backed) {
-		/* The start of the GTT is the stolen pages */
-		start = r->start;
-		end = r->start + dev_priv->gtt.stolen_size - 1;
-	} else {
-		/* The rest we will use for GEM backed objects */
-		start = r->start + dev_priv->gtt.stolen_size;
-		end = r->end;
-	}
-
-	gt = kzalloc(sizeof(struct gtt_range), GFP_KERNEL);
-	if (gt == NULL)
-		return NULL;
-	gt->resource.name = name;
-	gt->stolen = backed;
-	gt->in_gart = backed;
-	/* Ensure this is set for non GEM objects */
-	gt->gem.dev = dev;
-	ret = allocate_resource(dev_priv->gtt_mem, &gt->resource,
-				len, start, end, align, NULL, NULL);
-	if (ret == 0) {
-		gt->offset = gt->resource.start - r->start;
-		return gt;
-	}
-	kfree(gt);
-	return NULL;
-}
-
-/**
- *	psb_gtt_free_range	-	release GTT address space
- *	@dev: our DRM device
- *	@gt: a mapping created with psb_gtt_alloc_range
- *
- *	Release a resource that was allocated with psb_gtt_alloc_range. If the
- *	object has been pinned by mmap users we clean this up here currently.
- */
-void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt)
-{
-	/* Undo the mmap pin if we are destroying the object */
-	if (gt->mmapping) {
-		psb_gtt_unpin(gt);
-		gt->mmapping = 0;
-	}
-	WARN_ON(gt->in_gart && !gt->stolen);
-	release_resource(&gt->resource);
-	kfree(gt);
-}
-
 static void psb_gtt_alloc(struct drm_device *dev)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
index 2bf165849ebe..36162b545570 100644
--- a/drivers/gpu/drm/gma500/gtt.h
+++ b/drivers/gpu/drm/gma500/gtt.h
@@ -41,12 +41,9 @@ struct gtt_range {
 
 #define to_gtt_range(x) container_of(x, struct gtt_range, gem)
 
-extern struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len,
-					     const char *name, int backed,
-					     u32 align);
-extern void psb_gtt_kref_put(struct gtt_range *gt);
-extern void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt);
-extern int psb_gtt_pin(struct gtt_range *gt);
-extern void psb_gtt_unpin(struct gtt_range *gt);
 extern int psb_gtt_restore(struct drm_device *dev);
+
+int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume);
+void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r);
+
 #endif
diff --git a/drivers/gpu/drm/gma500/psb_intel_display.c b/drivers/gpu/drm/gma500/psb_intel_display.c
index f5f259fde88e..18d5f7e5889e 100644
--- a/drivers/gpu/drm/gma500/psb_intel_display.c
+++ b/drivers/gpu/drm/gma500/psb_intel_display.c
@@ -12,6 +12,7 @@
 #include <drm/drm_plane_helper.h>
 
 #include "framebuffer.h"
+#include "gem.h"
 #include "gma_display.h"
 #include "power.h"
 #include "psb_drv.h"
-- 
2.33.0

Re: [PATCH 01/10] drm/gma500: Move helpers for struct gtt_range from gtt.c to gem.c

[Patrik Jakobsson]

On Tue, Sep 28, 2021 at 10:44 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Allocation and pinning helpers for struct gtt_range are GEM functions,
> so move them to gem.c. No functional changes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/gma500/framebuffer.c       |   1 -
>  drivers/gpu/drm/gma500/gem.c               | 133 +++++++++++++--
>  drivers/gpu/drm/gma500/gem.h               |   8 +
>  drivers/gpu/drm/gma500/gma_display.c       |   1 +
>  drivers/gpu/drm/gma500/gtt.c               | 190 +--------------------
>  drivers/gpu/drm/gma500/gtt.h               |  11 +-
>  drivers/gpu/drm/gma500/psb_intel_display.c |   1 +
>  7 files changed, 136 insertions(+), 209 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
> index 321e416489a9..ce92d11bd20f 100644
> --- a/drivers/gpu/drm/gma500/framebuffer.c
> +++ b/drivers/gpu/drm/gma500/framebuffer.c
> @@ -25,7 +25,6 @@
>
>  #include "framebuffer.h"
>  #include "gem.h"
> -#include "gtt.h"
>  #include "psb_drv.h"
>  #include "psb_intel_drv.h"
>  #include "psb_intel_reg.h"
> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
> index 5ae54c9d2819..734bcb7a80c8 100644
> --- a/drivers/gpu/drm/gma500/gem.c
> +++ b/drivers/gpu/drm/gma500/gem.c
> @@ -19,6 +19,89 @@
>  #include "gem.h"
>  #include "psb_drv.h"
>
> +static int psb_gtt_attach_pages(struct gtt_range *gt)
> +{
> +       struct page **pages;
> +
> +       WARN_ON(gt->pages);
> +
> +       pages = drm_gem_get_pages(&gt->gem);
> +       if (IS_ERR(pages))
> +               return PTR_ERR(pages);
> +
> +       gt->npage = gt->gem.size / PAGE_SIZE;
> +       gt->pages = pages;
> +
> +       return 0;
> +}
> +
> +static void psb_gtt_detach_pages(struct gtt_range *gt)
> +{
> +       drm_gem_put_pages(&gt->gem, gt->pages, true, false);
> +       gt->pages = NULL;
> +}
> +
> +int psb_gtt_pin(struct gtt_range *gt)
> +{
> +       int ret = 0;
> +       struct drm_device *dev = gt->gem.dev;
> +       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> +       u32 gpu_base = dev_priv->gtt.gatt_start;
> +
> +       mutex_lock(&dev_priv->gtt_mutex);
> +
> +       if (gt->in_gart == 0 && gt->stolen == 0) {
> +               ret = psb_gtt_attach_pages(gt);
> +               if (ret < 0)
> +                       goto out;
> +               ret = psb_gtt_insert(dev, gt, 0);
> +               if (ret < 0) {
> +                       psb_gtt_detach_pages(gt);
> +                       goto out;
> +               }
> +               psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu),
> +                                    gt->pages, (gpu_base + gt->offset),
> +                                    gt->npage, 0, 0, PSB_MMU_CACHED_MEMORY);
> +       }
> +       gt->in_gart++;
> +out:
> +       mutex_unlock(&dev_priv->gtt_mutex);
> +       return ret;
> +}
> +
> +void psb_gtt_unpin(struct gtt_range *gt)
> +{
> +       struct drm_device *dev = gt->gem.dev;
> +       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> +       u32 gpu_base = dev_priv->gtt.gatt_start;
> +
> +       mutex_lock(&dev_priv->gtt_mutex);
> +
> +       WARN_ON(!gt->in_gart);
> +
> +       gt->in_gart--;
> +       if (gt->in_gart == 0 && gt->stolen == 0) {
> +               psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu),
> +                                    (gpu_base + gt->offset), gt->npage, 0, 0);
> +               psb_gtt_remove(dev, gt);
> +               psb_gtt_detach_pages(gt);
> +       }
> +
> +       mutex_unlock(&dev_priv->gtt_mutex);
> +}
> +
> +void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt)
> +{
> +       /* Undo the mmap pin if we are destroying the object */
> +       if (gt->mmapping) {
> +               psb_gtt_unpin(gt);
> +               gt->mmapping = 0;
> +       }
> +       WARN_ON(gt->in_gart && !gt->stolen);
> +       release_resource(&gt->resource);
> +       kfree(gt);
> +}
> +
>  static vm_fault_t psb_gem_fault(struct vm_fault *vmf);
>
>  static void psb_gem_free_object(struct drm_gem_object *obj)
> @@ -44,19 +127,43 @@ const struct drm_gem_object_funcs psb_gem_object_funcs = {
>         .vm_ops = &psb_gem_vm_ops,
>  };
>
> -/**
> - *     psb_gem_create          -       create a mappable object
> - *     @file: the DRM file of the client
> - *     @dev: our device
> - *     @size: the size requested
> - *     @handlep: returned handle (opaque number)
> - *     @stolen: unused
> - *     @align: unused
> - *
> - *     Create a GEM object, fill in the boilerplate and attach a handle to
> - *     it so that userspace can speak about it. This does the core work
> - *     for the various methods that do/will create GEM objects for things
> - */
> +struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len,
> +                                     const char *name, int backed, u32 align)
> +{
> +       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> +       struct gtt_range *gt;
> +       struct resource *r = dev_priv->gtt_mem;
> +       int ret;
> +       unsigned long start, end;
> +
> +       if (backed) {
> +               /* The start of the GTT is the stolen pages */
> +               start = r->start;
> +               end = r->start + dev_priv->gtt.stolen_size - 1;
> +       } else {
> +               /* The rest we will use for GEM backed objects */
> +               start = r->start + dev_priv->gtt.stolen_size;
> +               end = r->end;
> +       }
> +
> +       gt = kzalloc(sizeof(struct gtt_range), GFP_KERNEL);
> +       if (gt == NULL)
> +               return NULL;
> +       gt->resource.name = name;
> +       gt->stolen = backed;
> +       gt->in_gart = backed;
> +       /* Ensure this is set for non GEM objects */
> +       gt->gem.dev = dev;
> +       ret = allocate_resource(dev_priv->gtt_mem, &gt->resource,
> +                               len, start, end, align, NULL, NULL);

Not something for this patch but I think we're missing locking around
resource alloc and release.

> +       if (ret == 0) {
> +               gt->offset = gt->resource.start - r->start;
> +               return gt;
> +       }
> +       kfree(gt);
> +       return NULL;
> +}
> +
>  int psb_gem_create(struct drm_file *file, struct drm_device *dev, u64 size,
>                    u32 *handlep, int stolen, u32 align)
>  {
> diff --git a/drivers/gpu/drm/gma500/gem.h b/drivers/gpu/drm/gma500/gem.h
> index bae6454ead29..275494aedd4c 100644
> --- a/drivers/gpu/drm/gma500/gem.h
> +++ b/drivers/gpu/drm/gma500/gem.h
> @@ -8,6 +8,8 @@
>  #ifndef _GEM_H
>  #define _GEM_H
>
> +#include <drm/drm_gem.h>
> +
>  struct drm_device;
>
>  extern const struct drm_gem_object_funcs psb_gem_object_funcs;
> @@ -15,4 +17,10 @@ extern const struct drm_gem_object_funcs psb_gem_object_funcs;
>  extern int psb_gem_create(struct drm_file *file, struct drm_device *dev,
>                           u64 size, u32 *handlep, int stolen, u32 align);
>
> +struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len, const char *name,
> +                                     int backed, u32 align);
> +void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt);
> +int psb_gtt_pin(struct gtt_range *gt);
> +void psb_gtt_unpin(struct gtt_range *gt);
> +
>  #endif
> diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c
> index cbcecbaa041b..ecf8153416ac 100644
> --- a/drivers/gpu/drm/gma500/gma_display.c
> +++ b/drivers/gpu/drm/gma500/gma_display.c
> @@ -15,6 +15,7 @@
>  #include <drm/drm_vblank.h>
>
>  #include "framebuffer.h"
> +#include "gem.h"
>  #include "gma_display.h"
>  #include "psb_drv.h"
>  #include "psb_intel_drv.h"
> diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
> index 55a2a6919533..0aacf7122e32 100644
> --- a/drivers/gpu/drm/gma500/gtt.c
> +++ b/drivers/gpu/drm/gma500/gtt.c
> @@ -71,8 +71,7 @@ static u32 __iomem *psb_gtt_entry(struct drm_device *dev, struct gtt_range *r)
>   *     the GTT. This is protected via the gtt mutex which the caller
>   *     must hold.
>   */
> -static int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r,
> -                         int resume)
> +int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume)
>  {
>         u32 __iomem *gtt_slot;
>         u32 pte;
> @@ -116,7 +115,7 @@ static int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r,
>   *     page table entries with the dummy page. This is protected via the gtt
>   *     mutex which the caller must hold.
>   */
> -static void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r)
> +void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r)
>  {
>         struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>         u32 __iomem *gtt_slot;
> @@ -135,191 +134,6 @@ static void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r)
>         set_pages_array_wb(r->pages, r->npage);
>  }
>
> -/**
> - *     psb_gtt_attach_pages    -       attach and pin GEM pages
> - *     @gt: the gtt range
> - *
> - *     Pin and build an in kernel list of the pages that back our GEM object.
> - *     While we hold this the pages cannot be swapped out. This is protected
> - *     via the gtt mutex which the caller must hold.
> - */

The documentation about locking is still relevant. I'm not a big fan
of documenting the obvious but locking is an exception.

> -static int psb_gtt_attach_pages(struct gtt_range *gt)
> -{
> -       struct page **pages;
> -
> -       WARN_ON(gt->pages);
> -
> -       pages = drm_gem_get_pages(&gt->gem);
> -       if (IS_ERR(pages))
> -               return PTR_ERR(pages);
> -
> -       gt->npage = gt->gem.size / PAGE_SIZE;
> -       gt->pages = pages;
> -
> -       return 0;
> -}
> -
> -/**
> - *     psb_gtt_detach_pages    -       attach and pin GEM pages
> - *     @gt: the gtt range
> - *
> - *     Undo the effect of psb_gtt_attach_pages. At this point the pages
> - *     must have been removed from the GTT as they could now be paged out
> - *     and move bus address. This is protected via the gtt mutex which the
> - *     caller must hold.
> - */

Same thing about locking here


> -static void psb_gtt_detach_pages(struct gtt_range *gt)
> -{
> -       drm_gem_put_pages(&gt->gem, gt->pages, true, false);
> -       gt->pages = NULL;
> -}
> -
> -/**
> - *     psb_gtt_pin             -       pin pages into the GTT
> - *     @gt: range to pin
> - *
> - *     Pin a set of pages into the GTT. The pins are refcounted so that
> - *     multiple pins need multiple unpins to undo.
> - *
> - *     Non GEM backed objects treat this as a no-op as they are always GTT
> - *     backed objects.
> - */
> -int psb_gtt_pin(struct gtt_range *gt)
> -{
> -       int ret = 0;
> -       struct drm_device *dev = gt->gem.dev;
> -       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> -       u32 gpu_base = dev_priv->gtt.gatt_start;
> -
> -       mutex_lock(&dev_priv->gtt_mutex);
> -
> -       if (gt->in_gart == 0 && gt->stolen == 0) {
> -               ret = psb_gtt_attach_pages(gt);
> -               if (ret < 0)
> -                       goto out;
> -               ret = psb_gtt_insert(dev, gt, 0);
> -               if (ret < 0) {
> -                       psb_gtt_detach_pages(gt);
> -                       goto out;
> -               }
> -               psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu),
> -                                    gt->pages, (gpu_base + gt->offset),
> -                                    gt->npage, 0, 0, PSB_MMU_CACHED_MEMORY);
> -       }
> -       gt->in_gart++;
> -out:
> -       mutex_unlock(&dev_priv->gtt_mutex);
> -       return ret;
> -}
> -
> -/**
> - *     psb_gtt_unpin           -       Drop a GTT pin requirement
> - *     @gt: range to pin
> - *
> - *     Undoes the effect of psb_gtt_pin. On the last drop the GEM object
> - *     will be removed from the GTT which will also drop the page references
> - *     and allow the VM to clean up or page stuff.
> - *
> - *     Non GEM backed objects treat this as a no-op as they are always GTT
> - *     backed objects.
> - */
> -void psb_gtt_unpin(struct gtt_range *gt)
> -{
> -       struct drm_device *dev = gt->gem.dev;
> -       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> -       u32 gpu_base = dev_priv->gtt.gatt_start;
> -
> -       mutex_lock(&dev_priv->gtt_mutex);
> -
> -       WARN_ON(!gt->in_gart);
> -
> -       gt->in_gart--;
> -       if (gt->in_gart == 0 && gt->stolen == 0) {
> -               psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu),
> -                                    (gpu_base + gt->offset), gt->npage, 0, 0);
> -               psb_gtt_remove(dev, gt);
> -               psb_gtt_detach_pages(gt);
> -       }
> -
> -       mutex_unlock(&dev_priv->gtt_mutex);
> -}
> -
> -/*
> - *     GTT resource allocator - allocate and manage GTT address space
> - */
> -
> -/**
> - *     psb_gtt_alloc_range     -       allocate GTT address space
> - *     @dev: Our DRM device
> - *     @len: length (bytes) of address space required
> - *     @name: resource name
> - *     @backed: resource should be backed by stolen pages
> - *     @align: requested alignment
> - *
> - *     Ask the kernel core to find us a suitable range of addresses
> - *     to use for a GTT mapping.
> - *
> - *     Returns a gtt_range structure describing the object, or NULL on
> - *     error. On successful return the resource is both allocated and marked
> - *     as in use.
> - */
> -struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len,
> -                                     const char *name, int backed, u32 align)
> -{
> -       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> -       struct gtt_range *gt;
> -       struct resource *r = dev_priv->gtt_mem;
> -       int ret;
> -       unsigned long start, end;
> -
> -       if (backed) {
> -               /* The start of the GTT is the stolen pages */
> -               start = r->start;
> -               end = r->start + dev_priv->gtt.stolen_size - 1;
> -       } else {
> -               /* The rest we will use for GEM backed objects */
> -               start = r->start + dev_priv->gtt.stolen_size;
> -               end = r->end;
> -       }
> -
> -       gt = kzalloc(sizeof(struct gtt_range), GFP_KERNEL);
> -       if (gt == NULL)
> -               return NULL;
> -       gt->resource.name = name;
> -       gt->stolen = backed;
> -       gt->in_gart = backed;
> -       /* Ensure this is set for non GEM objects */
> -       gt->gem.dev = dev;
> -       ret = allocate_resource(dev_priv->gtt_mem, &gt->resource,
> -                               len, start, end, align, NULL, NULL);
> -       if (ret == 0) {
> -               gt->offset = gt->resource.start - r->start;
> -               return gt;
> -       }
> -       kfree(gt);
> -       return NULL;
> -}
> -
> -/**
> - *     psb_gtt_free_range      -       release GTT address space
> - *     @dev: our DRM device
> - *     @gt: a mapping created with psb_gtt_alloc_range
> - *
> - *     Release a resource that was allocated with psb_gtt_alloc_range. If the
> - *     object has been pinned by mmap users we clean this up here currently.
> - */
> -void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt)
> -{
> -       /* Undo the mmap pin if we are destroying the object */
> -       if (gt->mmapping) {
> -               psb_gtt_unpin(gt);
> -               gt->mmapping = 0;
> -       }
> -       WARN_ON(gt->in_gart && !gt->stolen);
> -       release_resource(&gt->resource);
> -       kfree(gt);
> -}
> -
>  static void psb_gtt_alloc(struct drm_device *dev)
>  {
>         struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
> index 2bf165849ebe..36162b545570 100644
> --- a/drivers/gpu/drm/gma500/gtt.h
> +++ b/drivers/gpu/drm/gma500/gtt.h
> @@ -41,12 +41,9 @@ struct gtt_range {
>
>  #define to_gtt_range(x) container_of(x, struct gtt_range, gem)
>
> -extern struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len,
> -                                            const char *name, int backed,
> -                                            u32 align);
> -extern void psb_gtt_kref_put(struct gtt_range *gt);
> -extern void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt);
> -extern int psb_gtt_pin(struct gtt_range *gt);
> -extern void psb_gtt_unpin(struct gtt_range *gt);
>  extern int psb_gtt_restore(struct drm_device *dev);
> +
> +int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume);
> +void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r);
> +
>  #endif
> diff --git a/drivers/gpu/drm/gma500/psb_intel_display.c b/drivers/gpu/drm/gma500/psb_intel_display.c
> index f5f259fde88e..18d5f7e5889e 100644
> --- a/drivers/gpu/drm/gma500/psb_intel_display.c
> +++ b/drivers/gpu/drm/gma500/psb_intel_display.c
> @@ -12,6 +12,7 @@
>  #include <drm/drm_plane_helper.h>
>
>  #include "framebuffer.h"
> +#include "gem.h"
>  #include "gma_display.h"
>  #include "power.h"
>  #include "psb_drv.h"
> --
> 2.33.0
>

Re: [PATCH 01/10] drm/gma500: Move helpers for struct gtt_range from gtt.c to gem.c

[Thomas Zimmermann]

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

Hi

Am 03.10.21 um 00:13 schrieb Patrik Jakobsson:
> On Tue, Sep 28, 2021 at 10:44 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Allocation and pinning helpers for struct gtt_range are GEM functions,
>> so move them to gem.c. No functional changes.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/gma500/framebuffer.c       |   1 -
>>   drivers/gpu/drm/gma500/gem.c               | 133 +++++++++++++--
>>   drivers/gpu/drm/gma500/gem.h               |   8 +
>>   drivers/gpu/drm/gma500/gma_display.c       |   1 +
>>   drivers/gpu/drm/gma500/gtt.c               | 190 +--------------------
>>   drivers/gpu/drm/gma500/gtt.h               |  11 +-
>>   drivers/gpu/drm/gma500/psb_intel_display.c |   1 +
>>   7 files changed, 136 insertions(+), 209 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
>> index 321e416489a9..ce92d11bd20f 100644
>> --- a/drivers/gpu/drm/gma500/framebuffer.c
>> +++ b/drivers/gpu/drm/gma500/framebuffer.c
>> @@ -25,7 +25,6 @@
>>
>>   #include "framebuffer.h"
>>   #include "gem.h"
>> -#include "gtt.h"
>>   #include "psb_drv.h"
>>   #include "psb_intel_drv.h"
>>   #include "psb_intel_reg.h"
>> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
>> index 5ae54c9d2819..734bcb7a80c8 100644
>> --- a/drivers/gpu/drm/gma500/gem.c
>> +++ b/drivers/gpu/drm/gma500/gem.c
>> @@ -19,6 +19,89 @@
>>   #include "gem.h"
>>   #include "psb_drv.h"
>>
>> +static int psb_gtt_attach_pages(struct gtt_range *gt)
>> +{
>> +       struct page **pages;
>> +
>> +       WARN_ON(gt->pages);
>> +
>> +       pages = drm_gem_get_pages(&gt->gem);
>> +       if (IS_ERR(pages))
>> +               return PTR_ERR(pages);
>> +
>> +       gt->npage = gt->gem.size / PAGE_SIZE;
>> +       gt->pages = pages;
>> +
>> +       return 0;
>> +}
>> +
>> +static void psb_gtt_detach_pages(struct gtt_range *gt)
>> +{
>> +       drm_gem_put_pages(&gt->gem, gt->pages, true, false);
>> +       gt->pages = NULL;
>> +}
>> +
>> +int psb_gtt_pin(struct gtt_range *gt)
>> +{
>> +       int ret = 0;
>> +       struct drm_device *dev = gt->gem.dev;
>> +       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>> +       u32 gpu_base = dev_priv->gtt.gatt_start;
>> +
>> +       mutex_lock(&dev_priv->gtt_mutex);
>> +
>> +       if (gt->in_gart == 0 && gt->stolen == 0) {
>> +               ret = psb_gtt_attach_pages(gt);
>> +               if (ret < 0)
>> +                       goto out;
>> +               ret = psb_gtt_insert(dev, gt, 0);
>> +               if (ret < 0) {
>> +                       psb_gtt_detach_pages(gt);
>> +                       goto out;
>> +               }
>> +               psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu),
>> +                                    gt->pages, (gpu_base + gt->offset),
>> +                                    gt->npage, 0, 0, PSB_MMU_CACHED_MEMORY);
>> +       }
>> +       gt->in_gart++;
>> +out:
>> +       mutex_unlock(&dev_priv->gtt_mutex);
>> +       return ret;
>> +}
>> +
>> +void psb_gtt_unpin(struct gtt_range *gt)
>> +{
>> +       struct drm_device *dev = gt->gem.dev;
>> +       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>> +       u32 gpu_base = dev_priv->gtt.gatt_start;
>> +
>> +       mutex_lock(&dev_priv->gtt_mutex);
>> +
>> +       WARN_ON(!gt->in_gart);
>> +
>> +       gt->in_gart--;
>> +       if (gt->in_gart == 0 && gt->stolen == 0) {
>> +               psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu),
>> +                                    (gpu_base + gt->offset), gt->npage, 0, 0);
>> +               psb_gtt_remove(dev, gt);
>> +               psb_gtt_detach_pages(gt);
>> +       }
>> +
>> +       mutex_unlock(&dev_priv->gtt_mutex);
>> +}
>> +
>> +void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt)
>> +{
>> +       /* Undo the mmap pin if we are destroying the object */
>> +       if (gt->mmapping) {
>> +               psb_gtt_unpin(gt);
>> +               gt->mmapping = 0;
>> +       }
>> +       WARN_ON(gt->in_gart && !gt->stolen);
>> +       release_resource(&gt->resource);
>> +       kfree(gt);
>> +}
>> +
>>   static vm_fault_t psb_gem_fault(struct vm_fault *vmf);
>>
>>   static void psb_gem_free_object(struct drm_gem_object *obj)
>> @@ -44,19 +127,43 @@ const struct drm_gem_object_funcs psb_gem_object_funcs = {
>>          .vm_ops = &psb_gem_vm_ops,
>>   };
>>
>> -/**
>> - *     psb_gem_create          -       create a mappable object
>> - *     @file: the DRM file of the client
>> - *     @dev: our device
>> - *     @size: the size requested
>> - *     @handlep: returned handle (opaque number)
>> - *     @stolen: unused
>> - *     @align: unused
>> - *
>> - *     Create a GEM object, fill in the boilerplate and attach a handle to
>> - *     it so that userspace can speak about it. This does the core work
>> - *     for the various methods that do/will create GEM objects for things
>> - */
>> +struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len,
>> +                                     const char *name, int backed, u32 align)
>> +{
>> +       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>> +       struct gtt_range *gt;
>> +       struct resource *r = dev_priv->gtt_mem;
>> +       int ret;
>> +       unsigned long start, end;
>> +
>> +       if (backed) {
>> +               /* The start of the GTT is the stolen pages */
>> +               start = r->start;
>> +               end = r->start + dev_priv->gtt.stolen_size - 1;
>> +       } else {
>> +               /* The rest we will use for GEM backed objects */
>> +               start = r->start + dev_priv->gtt.stolen_size;
>> +               end = r->end;
>> +       }
>> +
>> +       gt = kzalloc(sizeof(struct gtt_range), GFP_KERNEL);
>> +       if (gt == NULL)
>> +               return NULL;
>> +       gt->resource.name = name;
>> +       gt->stolen = backed;
>> +       gt->in_gart = backed;
>> +       /* Ensure this is set for non GEM objects */
>> +       gt->gem.dev = dev;
>> +       ret = allocate_resource(dev_priv->gtt_mem, &gt->resource,
>> +                               len, start, end, align, NULL, NULL);
> 
> Not something for this patch but I think we're missing locking around
> resource alloc and release.

There's a lock being taken within the function. [1]

> 
>> +       if (ret == 0) {
>> +               gt->offset = gt->resource.start - r->start;
>> +               return gt;
>> +       }
>> +       kfree(gt);
>> +       return NULL;
>> +}
>> +
>>   int psb_gem_create(struct drm_file *file, struct drm_device *dev, u64 size,
>>                     u32 *handlep, int stolen, u32 align)
>>   {
>> diff --git a/drivers/gpu/drm/gma500/gem.h b/drivers/gpu/drm/gma500/gem.h
>> index bae6454ead29..275494aedd4c 100644
>> --- a/drivers/gpu/drm/gma500/gem.h
>> +++ b/drivers/gpu/drm/gma500/gem.h
>> @@ -8,6 +8,8 @@
>>   #ifndef _GEM_H
>>   #define _GEM_H
>>
>> +#include <drm/drm_gem.h>
>> +
>>   struct drm_device;
>>
>>   extern const struct drm_gem_object_funcs psb_gem_object_funcs;
>> @@ -15,4 +17,10 @@ extern const struct drm_gem_object_funcs psb_gem_object_funcs;
>>   extern int psb_gem_create(struct drm_file *file, struct drm_device *dev,
>>                            u64 size, u32 *handlep, int stolen, u32 align);
>>
>> +struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len, const char *name,
>> +                                     int backed, u32 align);
>> +void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt);
>> +int psb_gtt_pin(struct gtt_range *gt);
>> +void psb_gtt_unpin(struct gtt_range *gt);
>> +
>>   #endif
>> diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c
>> index cbcecbaa041b..ecf8153416ac 100644
>> --- a/drivers/gpu/drm/gma500/gma_display.c
>> +++ b/drivers/gpu/drm/gma500/gma_display.c
>> @@ -15,6 +15,7 @@
>>   #include <drm/drm_vblank.h>
>>
>>   #include "framebuffer.h"
>> +#include "gem.h"
>>   #include "gma_display.h"
>>   #include "psb_drv.h"
>>   #include "psb_intel_drv.h"
>> diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
>> index 55a2a6919533..0aacf7122e32 100644
>> --- a/drivers/gpu/drm/gma500/gtt.c
>> +++ b/drivers/gpu/drm/gma500/gtt.c
>> @@ -71,8 +71,7 @@ static u32 __iomem *psb_gtt_entry(struct drm_device *dev, struct gtt_range *r)
>>    *     the GTT. This is protected via the gtt mutex which the caller
>>    *     must hold.
>>    */
>> -static int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r,
>> -                         int resume)
>> +int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume)
>>   {
>>          u32 __iomem *gtt_slot;
>>          u32 pte;
>> @@ -116,7 +115,7 @@ static int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r,
>>    *     page table entries with the dummy page. This is protected via the gtt
>>    *     mutex which the caller must hold.
>>    */
>> -static void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r)
>> +void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r)
>>   {
>>          struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>>          u32 __iomem *gtt_slot;
>> @@ -135,191 +134,6 @@ static void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r)
>>          set_pages_array_wb(r->pages, r->npage);
>>   }
>>
>> -/**
>> - *     psb_gtt_attach_pages    -       attach and pin GEM pages
>> - *     @gt: the gtt range
>> - *
>> - *     Pin and build an in kernel list of the pages that back our GEM object.
>> - *     While we hold this the pages cannot be swapped out. This is protected
>> - *     via the gtt mutex which the caller must hold.
>> - */
> 
> The documentation about locking is still relevant. I'm not a big fan
> of documenting the obvious but locking is an exception.

Make sense. I'll reduce the kerneldoc comment to the notes on locking.

Best regards
Thomas

[1] https://elixir.bootlin.com/linux/latest/source/kernel/resource.c#L745

> 
>> -static int psb_gtt_attach_pages(struct gtt_range *gt)
>> -{
>> -       struct page **pages;
>> -
>> -       WARN_ON(gt->pages);
>> -
>> -       pages = drm_gem_get_pages(&gt->gem);
>> -       if (IS_ERR(pages))
>> -               return PTR_ERR(pages);
>> -
>> -       gt->npage = gt->gem.size / PAGE_SIZE;
>> -       gt->pages = pages;
>> -
>> -       return 0;
>> -}
>> -
>> -/**
>> - *     psb_gtt_detach_pages    -       attach and pin GEM pages
>> - *     @gt: the gtt range
>> - *
>> - *     Undo the effect of psb_gtt_attach_pages. At this point the pages
>> - *     must have been removed from the GTT as they could now be paged out
>> - *     and move bus address. This is protected via the gtt mutex which the
>> - *     caller must hold.
>> - */
> 
> Same thing about locking here
> 
> 
>> -static void psb_gtt_detach_pages(struct gtt_range *gt)
>> -{
>> -       drm_gem_put_pages(&gt->gem, gt->pages, true, false);
>> -       gt->pages = NULL;
>> -}
>> -
>> -/**
>> - *     psb_gtt_pin             -       pin pages into the GTT
>> - *     @gt: range to pin
>> - *
>> - *     Pin a set of pages into the GTT. The pins are refcounted so that
>> - *     multiple pins need multiple unpins to undo.
>> - *
>> - *     Non GEM backed objects treat this as a no-op as they are always GTT
>> - *     backed objects.
>> - */
>> -int psb_gtt_pin(struct gtt_range *gt)
>> -{
>> -       int ret = 0;
>> -       struct drm_device *dev = gt->gem.dev;
>> -       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>> -       u32 gpu_base = dev_priv->gtt.gatt_start;
>> -
>> -       mutex_lock(&dev_priv->gtt_mutex);
>> -
>> -       if (gt->in_gart == 0 && gt->stolen == 0) {
>> -               ret = psb_gtt_attach_pages(gt);
>> -               if (ret < 0)
>> -                       goto out;
>> -               ret = psb_gtt_insert(dev, gt, 0);
>> -               if (ret < 0) {
>> -                       psb_gtt_detach_pages(gt);
>> -                       goto out;
>> -               }
>> -               psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu),
>> -                                    gt->pages, (gpu_base + gt->offset),
>> -                                    gt->npage, 0, 0, PSB_MMU_CACHED_MEMORY);
>> -       }
>> -       gt->in_gart++;
>> -out:
>> -       mutex_unlock(&dev_priv->gtt_mutex);
>> -       return ret;
>> -}
>> -
>> -/**
>> - *     psb_gtt_unpin           -       Drop a GTT pin requirement
>> - *     @gt: range to pin
>> - *
>> - *     Undoes the effect of psb_gtt_pin. On the last drop the GEM object
>> - *     will be removed from the GTT which will also drop the page references
>> - *     and allow the VM to clean up or page stuff.
>> - *
>> - *     Non GEM backed objects treat this as a no-op as they are always GTT
>> - *     backed objects.
>> - */
>> -void psb_gtt_unpin(struct gtt_range *gt)
>> -{
>> -       struct drm_device *dev = gt->gem.dev;
>> -       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>> -       u32 gpu_base = dev_priv->gtt.gatt_start;
>> -
>> -       mutex_lock(&dev_priv->gtt_mutex);
>> -
>> -       WARN_ON(!gt->in_gart);
>> -
>> -       gt->in_gart--;
>> -       if (gt->in_gart == 0 && gt->stolen == 0) {
>> -               psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu),
>> -                                    (gpu_base + gt->offset), gt->npage, 0, 0);
>> -               psb_gtt_remove(dev, gt);
>> -               psb_gtt_detach_pages(gt);
>> -       }
>> -
>> -       mutex_unlock(&dev_priv->gtt_mutex);
>> -}
>> -
>> -/*
>> - *     GTT resource allocator - allocate and manage GTT address space
>> - */
>> -
>> -/**
>> - *     psb_gtt_alloc_range     -       allocate GTT address space
>> - *     @dev: Our DRM device
>> - *     @len: length (bytes) of address space required
>> - *     @name: resource name
>> - *     @backed: resource should be backed by stolen pages
>> - *     @align: requested alignment
>> - *
>> - *     Ask the kernel core to find us a suitable range of addresses
>> - *     to use for a GTT mapping.
>> - *
>> - *     Returns a gtt_range structure describing the object, or NULL on
>> - *     error. On successful return the resource is both allocated and marked
>> - *     as in use.
>> - */
>> -struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len,
>> -                                     const char *name, int backed, u32 align)
>> -{
>> -       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>> -       struct gtt_range *gt;
>> -       struct resource *r = dev_priv->gtt_mem;
>> -       int ret;
>> -       unsigned long start, end;
>> -
>> -       if (backed) {
>> -               /* The start of the GTT is the stolen pages */
>> -               start = r->start;
>> -               end = r->start + dev_priv->gtt.stolen_size - 1;
>> -       } else {
>> -               /* The rest we will use for GEM backed objects */
>> -               start = r->start + dev_priv->gtt.stolen_size;
>> -               end = r->end;
>> -       }
>> -
>> -       gt = kzalloc(sizeof(struct gtt_range), GFP_KERNEL);
>> -       if (gt == NULL)
>> -               return NULL;
>> -       gt->resource.name = name;
>> -       gt->stolen = backed;
>> -       gt->in_gart = backed;
>> -       /* Ensure this is set for non GEM objects */
>> -       gt->gem.dev = dev;
>> -       ret = allocate_resource(dev_priv->gtt_mem, &gt->resource,
>> -                               len, start, end, align, NULL, NULL);
>> -       if (ret == 0) {
>> -               gt->offset = gt->resource.start - r->start;
>> -               return gt;
>> -       }
>> -       kfree(gt);
>> -       return NULL;
>> -}
>> -
>> -/**
>> - *     psb_gtt_free_range      -       release GTT address space
>> - *     @dev: our DRM device
>> - *     @gt: a mapping created with psb_gtt_alloc_range
>> - *
>> - *     Release a resource that was allocated with psb_gtt_alloc_range. If the
>> - *     object has been pinned by mmap users we clean this up here currently.
>> - */
>> -void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt)
>> -{
>> -       /* Undo the mmap pin if we are destroying the object */
>> -       if (gt->mmapping) {
>> -               psb_gtt_unpin(gt);
>> -               gt->mmapping = 0;
>> -       }
>> -       WARN_ON(gt->in_gart && !gt->stolen);
>> -       release_resource(&gt->resource);
>> -       kfree(gt);
>> -}
>> -
>>   static void psb_gtt_alloc(struct drm_device *dev)
>>   {
>>          struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>> diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
>> index 2bf165849ebe..36162b545570 100644
>> --- a/drivers/gpu/drm/gma500/gtt.h
>> +++ b/drivers/gpu/drm/gma500/gtt.h
>> @@ -41,12 +41,9 @@ struct gtt_range {
>>
>>   #define to_gtt_range(x) container_of(x, struct gtt_range, gem)
>>
>> -extern struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len,
>> -                                            const char *name, int backed,
>> -                                            u32 align);
>> -extern void psb_gtt_kref_put(struct gtt_range *gt);
>> -extern void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt);
>> -extern int psb_gtt_pin(struct gtt_range *gt);
>> -extern void psb_gtt_unpin(struct gtt_range *gt);
>>   extern int psb_gtt_restore(struct drm_device *dev);
>> +
>> +int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume);
>> +void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r);
>> +
>>   #endif
>> diff --git a/drivers/gpu/drm/gma500/psb_intel_display.c b/drivers/gpu/drm/gma500/psb_intel_display.c
>> index f5f259fde88e..18d5f7e5889e 100644
>> --- a/drivers/gpu/drm/gma500/psb_intel_display.c
>> +++ b/drivers/gpu/drm/gma500/psb_intel_display.c
>> @@ -12,6 +12,7 @@
>>   #include <drm/drm_plane_helper.h>
>>
>>   #include "framebuffer.h"
>> +#include "gem.h"
>>   #include "gma_display.h"
>>   #include "power.h"
>>   #include "psb_drv.h"
>> --
>> 2.33.0
>>

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

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

[PATCH 02/10] drm/gma500: Use to_gtt_range() everywhere

[Thomas Zimmermann]

Convert upcasts from struct drm_gem_object to struct gtt_range to
to_gtt_range(). Some places used container_of() directly.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/gma500/gem.c         | 4 ++--
 drivers/gpu/drm/gma500/gma_display.c | 7 +++----
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index 734bcb7a80c8..ff2c1d64689e 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -106,7 +106,7 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf);
 
 static void psb_gem_free_object(struct drm_gem_object *obj)
 {
-	struct gtt_range *gtt = container_of(obj, struct gtt_range, gem);
+	struct gtt_range *gtt = to_gtt_range(obj);
 
 	/* Remove the list map if one is present */
 	drm_gem_free_mmap_offset(obj);
@@ -256,7 +256,7 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf)
 	dev = obj->dev;
 	dev_priv = to_drm_psb_private(dev);
 
-	r = container_of(obj, struct gtt_range, gem);	/* Get the gtt range */
+	r = to_gtt_range(obj);
 
 	/* Make sure we don't parallel update on a fault, nor move or remove
 	   something from beneath our feet */
diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c
index ecf8153416ac..8285358fac01 100644
--- a/drivers/gpu/drm/gma500/gma_display.c
+++ b/drivers/gpu/drm/gma500/gma_display.c
@@ -349,8 +349,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
 
 		/* Unpin the old GEM object */
 		if (gma_crtc->cursor_obj) {
-			gt = container_of(gma_crtc->cursor_obj,
-					  struct gtt_range, gem);
+			gt = to_gtt_range(gma_crtc->cursor_obj);
 			psb_gtt_unpin(gt);
 			drm_gem_object_put(gma_crtc->cursor_obj);
 			gma_crtc->cursor_obj = NULL;
@@ -376,7 +375,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
 		goto unref_cursor;
 	}
 
-	gt = container_of(obj, struct gtt_range, gem);
+	gt = to_gtt_range(obj);
 
 	/* Pin the memory into the GTT */
 	ret = psb_gtt_pin(gt);
@@ -426,7 +425,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
 
 	/* unpin the old bo */
 	if (gma_crtc->cursor_obj) {
-		gt = container_of(gma_crtc->cursor_obj, struct gtt_range, gem);
+		gt = to_gtt_range(gma_crtc->cursor_obj);
 		psb_gtt_unpin(gt);
 		drm_gem_object_put(gma_crtc->cursor_obj);
 	}
-- 
2.33.0

Re: [PATCH 02/10] drm/gma500: Use to_gtt_range() everywhere

[Patrik Jakobsson]

On Tue, Sep 28, 2021 at 10:44 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Convert upcasts from struct drm_gem_object to struct gtt_range to
> to_gtt_range(). Some places used container_of() directly.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

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


> ---
>  drivers/gpu/drm/gma500/gem.c         | 4 ++--
>  drivers/gpu/drm/gma500/gma_display.c | 7 +++----
>  2 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
> index 734bcb7a80c8..ff2c1d64689e 100644
> --- a/drivers/gpu/drm/gma500/gem.c
> +++ b/drivers/gpu/drm/gma500/gem.c
> @@ -106,7 +106,7 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf);
>
>  static void psb_gem_free_object(struct drm_gem_object *obj)
>  {
> -       struct gtt_range *gtt = container_of(obj, struct gtt_range, gem);
> +       struct gtt_range *gtt = to_gtt_range(obj);
>
>         /* Remove the list map if one is present */
>         drm_gem_free_mmap_offset(obj);
> @@ -256,7 +256,7 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf)
>         dev = obj->dev;
>         dev_priv = to_drm_psb_private(dev);
>
> -       r = container_of(obj, struct gtt_range, gem);   /* Get the gtt range */
> +       r = to_gtt_range(obj);
>
>         /* Make sure we don't parallel update on a fault, nor move or remove
>            something from beneath our feet */
> diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c
> index ecf8153416ac..8285358fac01 100644
> --- a/drivers/gpu/drm/gma500/gma_display.c
> +++ b/drivers/gpu/drm/gma500/gma_display.c
> @@ -349,8 +349,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
>
>                 /* Unpin the old GEM object */
>                 if (gma_crtc->cursor_obj) {
> -                       gt = container_of(gma_crtc->cursor_obj,
> -                                         struct gtt_range, gem);
> +                       gt = to_gtt_range(gma_crtc->cursor_obj);
>                         psb_gtt_unpin(gt);
>                         drm_gem_object_put(gma_crtc->cursor_obj);
>                         gma_crtc->cursor_obj = NULL;
> @@ -376,7 +375,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
>                 goto unref_cursor;
>         }
>
> -       gt = container_of(obj, struct gtt_range, gem);
> +       gt = to_gtt_range(obj);
>
>         /* Pin the memory into the GTT */
>         ret = psb_gtt_pin(gt);
> @@ -426,7 +425,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
>
>         /* unpin the old bo */
>         if (gma_crtc->cursor_obj) {
> -               gt = container_of(gma_crtc->cursor_obj, struct gtt_range, gem);
> +               gt = to_gtt_range(gma_crtc->cursor_obj);
>                 psb_gtt_unpin(gt);
>                 drm_gem_object_put(gma_crtc->cursor_obj);
>         }
> --
> 2.33.0
>

[PATCH 03/10] drm/gma500: Reimplement psb_gem_create()

[Thomas Zimmermann]

Implement psb_gem_create() for general use. Create the GEM handle in
psb_gem_create_dumb(). Allows to use psb_gem_create() for creating all
of the GEM objects.

While at it, clean-up drm_gem_dumb_create() to make it more readable.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/gma500/gem.c | 93 ++++++++++++++++++++++--------------
 drivers/gpu/drm/gma500/gem.h |  4 +-
 2 files changed, 59 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index ff2c1d64689e..8f4bcf9cf912 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -164,45 +164,36 @@ struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len,
 	return NULL;
 }
 
-int psb_gem_create(struct drm_file *file, struct drm_device *dev, u64 size,
-		   u32 *handlep, int stolen, u32 align)
+struct gtt_range *
+psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, u32 align)
 {
-	struct gtt_range *r;
+	struct gtt_range *gt;
+	struct drm_gem_object *obj;
 	int ret;
-	u32 handle;
 
 	size = roundup(size, PAGE_SIZE);
 
-	/* Allocate our object - for now a direct gtt range which is not
-	   stolen memory backed */
-	r = psb_gtt_alloc_range(dev, size, "gem", 0, PAGE_SIZE);
-	if (r == NULL) {
+	gt = psb_gtt_alloc_range(dev, size, name, stolen, align);
+	if (!gt) {
 		dev_err(dev->dev, "no memory for %lld byte GEM object\n", size);
-		return -ENOSPC;
+		return ERR_PTR(-ENOSPC);
 	}
-	r->gem.funcs = &psb_gem_object_funcs;
-	/* Initialize the extra goodies GEM needs to do all the hard work */
-	if (drm_gem_object_init(dev, &r->gem, size) != 0) {
-		psb_gtt_free_range(dev, r);
-		/* GEM doesn't give an error code so use -ENOMEM */
-		dev_err(dev->dev, "GEM init failed for %lld\n", size);
-		return -ENOMEM;
-	}
-	/* Limit the object to 32bit mappings */
-	mapping_set_gfp_mask(r->gem.filp->f_mapping, GFP_KERNEL | __GFP_DMA32);
-	/* Give the object a handle so we can carry it more easily */
-	ret = drm_gem_handle_create(file, &r->gem, &handle);
-	if (ret) {
-		dev_err(dev->dev, "GEM handle failed for %p, %lld\n",
-							&r->gem, size);
-		drm_gem_object_release(&r->gem);
-		psb_gtt_free_range(dev, r);
-		return ret;
-	}
-	/* We have the initial and handle reference but need only one now */
-	drm_gem_object_put(&r->gem);
-	*handlep = handle;
-	return 0;
+	obj = &gt->gem;
+
+	obj->funcs = &psb_gem_object_funcs;
+
+	ret = drm_gem_object_init(dev, obj, size);
+	if (ret)
+		goto err_psb_gtt_free_range;
+
+	/* Limit the object to 32-bit mappings */
+	mapping_set_gfp_mask(obj->filp->f_mapping, GFP_KERNEL | __GFP_DMA32);
+
+	return gt;
+
+err_psb_gtt_free_range:
+	psb_gtt_free_range(dev, gt);
+	return ERR_PTR(ret);
 }
 
 /**
@@ -218,10 +209,40 @@ int psb_gem_create(struct drm_file *file, struct drm_device *dev, u64 size,
 int psb_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
 			struct drm_mode_create_dumb *args)
 {
-	args->pitch = ALIGN(args->width * ((args->bpp + 7) / 8), 64);
-	args->size = args->pitch * args->height;
-	return psb_gem_create(file, dev, args->size, &args->handle, 0,
-			      PAGE_SIZE);
+	size_t pitch, size;
+	struct gtt_range *gt;
+	struct drm_gem_object *obj;
+	u32 handle;
+	int ret;
+
+	pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
+	pitch = ALIGN(pitch, 64);
+
+	size = pitch * args->height;
+	size = roundup(size, PAGE_SIZE);
+	if (!size)
+		return -EINVAL;
+
+	gt = psb_gem_create(dev, size, "gem", false, PAGE_SIZE);
+	if (IS_ERR(gt))
+		return PTR_ERR(gt);
+	obj = &gt->gem;
+
+	ret = drm_gem_handle_create(file, obj, &handle);
+	if (ret)
+		goto err_drm_gem_object_put;
+
+	drm_gem_object_put(obj);
+
+	args->pitch = pitch;
+	args->size = size;
+	args->handle = handle;
+
+	return 0;
+
+err_drm_gem_object_put:
+	drm_gem_object_put(obj);
+	return ret;
 }
 
 /**
diff --git a/drivers/gpu/drm/gma500/gem.h b/drivers/gpu/drm/gma500/gem.h
index 275494aedd4c..ad76127dc719 100644
--- a/drivers/gpu/drm/gma500/gem.h
+++ b/drivers/gpu/drm/gma500/gem.h
@@ -14,8 +14,8 @@ struct drm_device;
 
 extern const struct drm_gem_object_funcs psb_gem_object_funcs;
 
-extern int psb_gem_create(struct drm_file *file, struct drm_device *dev,
-			  u64 size, u32 *handlep, int stolen, u32 align);
+struct gtt_range *
+psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, u32 align);
 
 struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len, const char *name,
 				      int backed, u32 align);
-- 
2.33.0

Re: [PATCH 03/10] drm/gma500: Reimplement psb_gem_create()

[Patrik Jakobsson]

On Tue, Sep 28, 2021 at 10:44 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Implement psb_gem_create() for general use. Create the GEM handle in
> psb_gem_create_dumb(). Allows to use psb_gem_create() for creating all
> of the GEM objects.
>
> While at it, clean-up drm_gem_dumb_create() to make it more readable.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

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


> ---
>  drivers/gpu/drm/gma500/gem.c | 93 ++++++++++++++++++++++--------------
>  drivers/gpu/drm/gma500/gem.h |  4 +-
>  2 files changed, 59 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
> index ff2c1d64689e..8f4bcf9cf912 100644
> --- a/drivers/gpu/drm/gma500/gem.c
> +++ b/drivers/gpu/drm/gma500/gem.c
> @@ -164,45 +164,36 @@ struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len,
>         return NULL;
>  }
>
> -int psb_gem_create(struct drm_file *file, struct drm_device *dev, u64 size,
> -                  u32 *handlep, int stolen, u32 align)
> +struct gtt_range *
> +psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, u32 align)
>  {
> -       struct gtt_range *r;
> +       struct gtt_range *gt;
> +       struct drm_gem_object *obj;
>         int ret;
> -       u32 handle;
>
>         size = roundup(size, PAGE_SIZE);
>
> -       /* Allocate our object - for now a direct gtt range which is not
> -          stolen memory backed */
> -       r = psb_gtt_alloc_range(dev, size, "gem", 0, PAGE_SIZE);
> -       if (r == NULL) {
> +       gt = psb_gtt_alloc_range(dev, size, name, stolen, align);
> +       if (!gt) {
>                 dev_err(dev->dev, "no memory for %lld byte GEM object\n", size);
> -               return -ENOSPC;
> +               return ERR_PTR(-ENOSPC);
>         }
> -       r->gem.funcs = &psb_gem_object_funcs;
> -       /* Initialize the extra goodies GEM needs to do all the hard work */
> -       if (drm_gem_object_init(dev, &r->gem, size) != 0) {
> -               psb_gtt_free_range(dev, r);
> -               /* GEM doesn't give an error code so use -ENOMEM */
> -               dev_err(dev->dev, "GEM init failed for %lld\n", size);
> -               return -ENOMEM;
> -       }
> -       /* Limit the object to 32bit mappings */
> -       mapping_set_gfp_mask(r->gem.filp->f_mapping, GFP_KERNEL | __GFP_DMA32);
> -       /* Give the object a handle so we can carry it more easily */
> -       ret = drm_gem_handle_create(file, &r->gem, &handle);
> -       if (ret) {
> -               dev_err(dev->dev, "GEM handle failed for %p, %lld\n",
> -                                                       &r->gem, size);
> -               drm_gem_object_release(&r->gem);
> -               psb_gtt_free_range(dev, r);
> -               return ret;
> -       }
> -       /* We have the initial and handle reference but need only one now */
> -       drm_gem_object_put(&r->gem);
> -       *handlep = handle;
> -       return 0;
> +       obj = &gt->gem;
> +
> +       obj->funcs = &psb_gem_object_funcs;
> +
> +       ret = drm_gem_object_init(dev, obj, size);
> +       if (ret)
> +               goto err_psb_gtt_free_range;
> +
> +       /* Limit the object to 32-bit mappings */
> +       mapping_set_gfp_mask(obj->filp->f_mapping, GFP_KERNEL | __GFP_DMA32);
> +
> +       return gt;
> +
> +err_psb_gtt_free_range:
> +       psb_gtt_free_range(dev, gt);
> +       return ERR_PTR(ret);
>  }
>
>  /**
> @@ -218,10 +209,40 @@ int psb_gem_create(struct drm_file *file, struct drm_device *dev, u64 size,
>  int psb_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
>                         struct drm_mode_create_dumb *args)
>  {
> -       args->pitch = ALIGN(args->width * ((args->bpp + 7) / 8), 64);
> -       args->size = args->pitch * args->height;
> -       return psb_gem_create(file, dev, args->size, &args->handle, 0,
> -                             PAGE_SIZE);
> +       size_t pitch, size;
> +       struct gtt_range *gt;
> +       struct drm_gem_object *obj;
> +       u32 handle;
> +       int ret;
> +
> +       pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
> +       pitch = ALIGN(pitch, 64);
> +
> +       size = pitch * args->height;
> +       size = roundup(size, PAGE_SIZE);
> +       if (!size)
> +               return -EINVAL;
> +
> +       gt = psb_gem_create(dev, size, "gem", false, PAGE_SIZE);
> +       if (IS_ERR(gt))
> +               return PTR_ERR(gt);
> +       obj = &gt->gem;
> +
> +       ret = drm_gem_handle_create(file, obj, &handle);
> +       if (ret)
> +               goto err_drm_gem_object_put;
> +
> +       drm_gem_object_put(obj);
> +
> +       args->pitch = pitch;
> +       args->size = size;
> +       args->handle = handle;
> +
> +       return 0;
> +
> +err_drm_gem_object_put:
> +       drm_gem_object_put(obj);
> +       return ret;
>  }
>
>  /**
> diff --git a/drivers/gpu/drm/gma500/gem.h b/drivers/gpu/drm/gma500/gem.h
> index 275494aedd4c..ad76127dc719 100644
> --- a/drivers/gpu/drm/gma500/gem.h
> +++ b/drivers/gpu/drm/gma500/gem.h
> @@ -14,8 +14,8 @@ struct drm_device;
>
>  extern const struct drm_gem_object_funcs psb_gem_object_funcs;
>
> -extern int psb_gem_create(struct drm_file *file, struct drm_device *dev,
> -                         u64 size, u32 *handlep, int stolen, u32 align);
> +struct gtt_range *
> +psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, u32 align);
>
>  struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len, const char *name,
>                                       int backed, u32 align);
> --
> 2.33.0
>

[PATCH 04/10] drm/gma500: Allocate GTT ranges in stolen memory with psb_gem_create()

[Thomas Zimmermann]

Support private objects for stolen memory in psb_gem_create() and
convert users to psb_gem_create(). For stolen memory, psb_gem_create()
now initializes the GEM object via drm_gem_private_object_init().

In the fbdev setup, replace the open-coded initialization of struct
gtt_range with a call to psb_gem_create(). Use drm_gem_object_put()
for release.

In the cursor setup, use psb_gem_create() and get a real GEM object.
Previously the allocated instance of struct gtt_range was only partially
initialized. Release the cursor GEM object in gma_crtc_destroy(). The
release was missing from the original code.

With the conversion of all callers to psb_gem_create(), the extern
declarations of psb_gtt_alloc_range, psb_gtt_free_range and
psb_gem_object_func are not required any longer. Declare them as
static inline.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/gma500/framebuffer.c       | 44 ++++++----------------
 drivers/gpu/drm/gma500/gem.c               | 22 ++++++-----
 drivers/gpu/drm/gma500/gem.h               |  5 ---
 drivers/gpu/drm/gma500/gma_display.c       |  3 ++
 drivers/gpu/drm/gma500/psb_intel_display.c |  5 +--
 5 files changed, 29 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
index ce92d11bd20f..3ea6679ccd38 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -224,31 +224,6 @@ static struct drm_framebuffer *psb_framebuffer_create
 	return fb;
 }
 
-/**
- *	psbfb_alloc		-	allocate frame buffer memory
- *	@dev: the DRM device
- *	@aligned_size: space needed
- *
- *	Allocate the frame buffer. In the usual case we get a GTT range that
- *	is stolen memory backed and life is simple. If there isn't sufficient
- *	we fail as we don't have the virtual mapping space to really vmap it
- *	and the kernel console code can't handle non linear framebuffers.
- *
- *	Re-address this as and if the framebuffer layer grows this ability.
- */
-static struct gtt_range *psbfb_alloc(struct drm_device *dev, int aligned_size)
-{
-	struct gtt_range *backing;
-	/* Begin by trying to use stolen memory backing */
-	backing = psb_gtt_alloc_range(dev, aligned_size, "fb", 1, PAGE_SIZE);
-	if (backing) {
-		backing->gem.funcs = &psb_gem_object_funcs;
-		drm_gem_private_object_init(dev, &backing->gem, aligned_size);
-		return backing;
-	}
-	return NULL;
-}
-
 /**
  *	psbfb_create		-	create a framebuffer
  *	@fb_helper: the framebuffer helper
@@ -268,6 +243,7 @@ static int psbfb_create(struct drm_fb_helper *fb_helper,
 	int size;
 	int ret;
 	struct gtt_range *backing;
+	struct drm_gem_object *obj;
 	u32 bpp, depth;
 
 	mode_cmd.width = sizes->surface_width;
@@ -285,24 +261,25 @@ static int psbfb_create(struct drm_fb_helper *fb_helper,
 	size = ALIGN(size, PAGE_SIZE);
 
 	/* Allocate the framebuffer in the GTT with stolen page backing */
-	backing = psbfb_alloc(dev, size);
-	if (backing == NULL)
-		return -ENOMEM;
+	backing = psb_gem_create(dev, size, "fb", true, PAGE_SIZE);
+	if (IS_ERR(backing))
+		return PTR_ERR(backing);
+	obj = &backing->gem;
 
 	memset(dev_priv->vram_addr + backing->offset, 0, size);
 
 	info = drm_fb_helper_alloc_fbi(fb_helper);
 	if (IS_ERR(info)) {
 		ret = PTR_ERR(info);
-		goto out;
+		goto err_drm_gem_object_put;
 	}
 
 	mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth);
 
-	fb = psb_framebuffer_create(dev, &mode_cmd, &backing->gem);
+	fb = psb_framebuffer_create(dev, &mode_cmd, obj);
 	if (IS_ERR(fb)) {
 		ret = PTR_ERR(fb);
-		goto out;
+		goto err_drm_gem_object_put;
 	}
 
 	fb_helper->fb = fb;
@@ -333,8 +310,9 @@ static int psbfb_create(struct drm_fb_helper *fb_helper,
 	dev_dbg(dev->dev, "allocated %dx%d fb\n", fb->width, fb->height);
 
 	return 0;
-out:
-	psb_gtt_free_range(dev, backing);
+
+err_drm_gem_object_put:
+	drm_gem_object_put(obj);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index 8f4bcf9cf912..4acab39a583a 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -90,7 +90,7 @@ void psb_gtt_unpin(struct gtt_range *gt)
 	mutex_unlock(&dev_priv->gtt_mutex);
 }
 
-void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt)
+static void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt)
 {
 	/* Undo the mmap pin if we are destroying the object */
 	if (gt->mmapping) {
@@ -122,13 +122,13 @@ static const struct vm_operations_struct psb_gem_vm_ops = {
 	.close = drm_gem_vm_close,
 };
 
-const struct drm_gem_object_funcs psb_gem_object_funcs = {
+static const struct drm_gem_object_funcs psb_gem_object_funcs = {
 	.free = psb_gem_free_object,
 	.vm_ops = &psb_gem_vm_ops,
 };
 
-struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len,
-				      const char *name, int backed, u32 align)
+static struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len,
+					     const char *name, int backed, u32 align)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
 	struct gtt_range *gt;
@@ -182,12 +182,16 @@ psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen,
 
 	obj->funcs = &psb_gem_object_funcs;
 
-	ret = drm_gem_object_init(dev, obj, size);
-	if (ret)
-		goto err_psb_gtt_free_range;
+	if (stolen) {
+		drm_gem_private_object_init(dev, obj, size);
+	} else {
+		ret = drm_gem_object_init(dev, obj, size);
+		if (ret)
+			goto err_psb_gtt_free_range;
 
-	/* Limit the object to 32-bit mappings */
-	mapping_set_gfp_mask(obj->filp->f_mapping, GFP_KERNEL | __GFP_DMA32);
+		/* Limit the object to 32-bit mappings */
+		mapping_set_gfp_mask(obj->filp->f_mapping, GFP_KERNEL | __GFP_DMA32);
+	}
 
 	return gt;
 
diff --git a/drivers/gpu/drm/gma500/gem.h b/drivers/gpu/drm/gma500/gem.h
index ad76127dc719..6b67c58cbed5 100644
--- a/drivers/gpu/drm/gma500/gem.h
+++ b/drivers/gpu/drm/gma500/gem.h
@@ -12,14 +12,9 @@
 
 struct drm_device;
 
-extern const struct drm_gem_object_funcs psb_gem_object_funcs;
-
 struct gtt_range *
 psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, u32 align);
 
-struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len, const char *name,
-				      int backed, u32 align);
-void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt);
 int psb_gtt_pin(struct gtt_range *gt);
 void psb_gtt_unpin(struct gtt_range *gt);
 
diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c
index 8285358fac01..8c95b50034a5 100644
--- a/drivers/gpu/drm/gma500/gma_display.c
+++ b/drivers/gpu/drm/gma500/gma_display.c
@@ -498,6 +498,9 @@ void gma_crtc_destroy(struct drm_crtc *crtc)
 {
 	struct gma_crtc *gma_crtc = to_gma_crtc(crtc);
 
+	if (gma_crtc->cursor_gt)
+		drm_gem_object_put(&gma_crtc->cursor_gt->gem);
+
 	kfree(gma_crtc->crtc_state);
 	drm_crtc_cleanup(crtc);
 	kfree(gma_crtc);
diff --git a/drivers/gpu/drm/gma500/psb_intel_display.c b/drivers/gpu/drm/gma500/psb_intel_display.c
index 18d5f7e5889e..b5e9118c01a4 100644
--- a/drivers/gpu/drm/gma500/psb_intel_display.c
+++ b/drivers/gpu/drm/gma500/psb_intel_display.c
@@ -461,9 +461,8 @@ static void psb_intel_cursor_init(struct drm_device *dev,
 		/* Allocate 4 pages of stolen mem for a hardware cursor. That
 		 * is enough for the 64 x 64 ARGB cursors we support.
 		 */
-		cursor_gt = psb_gtt_alloc_range(dev, 4 * PAGE_SIZE, "cursor", 1,
-						PAGE_SIZE);
-		if (!cursor_gt) {
+		cursor_gt = psb_gem_create(dev, 4 * PAGE_SIZE, "cursor", true, PAGE_SIZE);
+		if (IS_ERR(cursor_gt)) {
 			gma_crtc->cursor_gt = NULL;
 			goto out;
 		}
-- 
2.33.0

Re: [PATCH 04/10] drm/gma500: Allocate GTT ranges in stolen memory with psb_gem_create()

[Patrik Jakobsson]

On Tue, Sep 28, 2021 at 10:44 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Support private objects for stolen memory in psb_gem_create() and
> convert users to psb_gem_create(). For stolen memory, psb_gem_create()
> now initializes the GEM object via drm_gem_private_object_init().
>
> In the fbdev setup, replace the open-coded initialization of struct
> gtt_range with a call to psb_gem_create(). Use drm_gem_object_put()
> for release.
>
> In the cursor setup, use psb_gem_create() and get a real GEM object.
> Previously the allocated instance of struct gtt_range was only partially
> initialized. Release the cursor GEM object in gma_crtc_destroy(). The
> release was missing from the original code.
>
> With the conversion of all callers to psb_gem_create(), the extern
> declarations of psb_gtt_alloc_range, psb_gtt_free_range and
> psb_gem_object_func are not required any longer. Declare them as
> static inline.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

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


> ---
>  drivers/gpu/drm/gma500/framebuffer.c       | 44 ++++++----------------
>  drivers/gpu/drm/gma500/gem.c               | 22 ++++++-----
>  drivers/gpu/drm/gma500/gem.h               |  5 ---
>  drivers/gpu/drm/gma500/gma_display.c       |  3 ++
>  drivers/gpu/drm/gma500/psb_intel_display.c |  5 +--
>  5 files changed, 29 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
> index ce92d11bd20f..3ea6679ccd38 100644
> --- a/drivers/gpu/drm/gma500/framebuffer.c
> +++ b/drivers/gpu/drm/gma500/framebuffer.c
> @@ -224,31 +224,6 @@ static struct drm_framebuffer *psb_framebuffer_create
>         return fb;
>  }
>
> -/**
> - *     psbfb_alloc             -       allocate frame buffer memory
> - *     @dev: the DRM device
> - *     @aligned_size: space needed
> - *
> - *     Allocate the frame buffer. In the usual case we get a GTT range that
> - *     is stolen memory backed and life is simple. If there isn't sufficient
> - *     we fail as we don't have the virtual mapping space to really vmap it
> - *     and the kernel console code can't handle non linear framebuffers.
> - *
> - *     Re-address this as and if the framebuffer layer grows this ability.
> - */
> -static struct gtt_range *psbfb_alloc(struct drm_device *dev, int aligned_size)
> -{
> -       struct gtt_range *backing;
> -       /* Begin by trying to use stolen memory backing */
> -       backing = psb_gtt_alloc_range(dev, aligned_size, "fb", 1, PAGE_SIZE);
> -       if (backing) {
> -               backing->gem.funcs = &psb_gem_object_funcs;
> -               drm_gem_private_object_init(dev, &backing->gem, aligned_size);
> -               return backing;
> -       }
> -       return NULL;
> -}
> -
>  /**
>   *     psbfb_create            -       create a framebuffer
>   *     @fb_helper: the framebuffer helper
> @@ -268,6 +243,7 @@ static int psbfb_create(struct drm_fb_helper *fb_helper,
>         int size;
>         int ret;
>         struct gtt_range *backing;
> +       struct drm_gem_object *obj;
>         u32 bpp, depth;
>
>         mode_cmd.width = sizes->surface_width;
> @@ -285,24 +261,25 @@ static int psbfb_create(struct drm_fb_helper *fb_helper,
>         size = ALIGN(size, PAGE_SIZE);
>
>         /* Allocate the framebuffer in the GTT with stolen page backing */
> -       backing = psbfb_alloc(dev, size);
> -       if (backing == NULL)
> -               return -ENOMEM;
> +       backing = psb_gem_create(dev, size, "fb", true, PAGE_SIZE);
> +       if (IS_ERR(backing))
> +               return PTR_ERR(backing);
> +       obj = &backing->gem;
>
>         memset(dev_priv->vram_addr + backing->offset, 0, size);
>
>         info = drm_fb_helper_alloc_fbi(fb_helper);
>         if (IS_ERR(info)) {
>                 ret = PTR_ERR(info);
> -               goto out;
> +               goto err_drm_gem_object_put;
>         }
>
>         mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth);
>
> -       fb = psb_framebuffer_create(dev, &mode_cmd, &backing->gem);
> +       fb = psb_framebuffer_create(dev, &mode_cmd, obj);
>         if (IS_ERR(fb)) {
>                 ret = PTR_ERR(fb);
> -               goto out;
> +               goto err_drm_gem_object_put;
>         }
>
>         fb_helper->fb = fb;
> @@ -333,8 +310,9 @@ static int psbfb_create(struct drm_fb_helper *fb_helper,
>         dev_dbg(dev->dev, "allocated %dx%d fb\n", fb->width, fb->height);
>
>         return 0;
> -out:
> -       psb_gtt_free_range(dev, backing);
> +
> +err_drm_gem_object_put:
> +       drm_gem_object_put(obj);
>         return ret;
>  }
>
> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
> index 8f4bcf9cf912..4acab39a583a 100644
> --- a/drivers/gpu/drm/gma500/gem.c
> +++ b/drivers/gpu/drm/gma500/gem.c
> @@ -90,7 +90,7 @@ void psb_gtt_unpin(struct gtt_range *gt)
>         mutex_unlock(&dev_priv->gtt_mutex);
>  }
>
> -void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt)
> +static void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt)
>  {
>         /* Undo the mmap pin if we are destroying the object */
>         if (gt->mmapping) {
> @@ -122,13 +122,13 @@ static const struct vm_operations_struct psb_gem_vm_ops = {
>         .close = drm_gem_vm_close,
>  };
>
> -const struct drm_gem_object_funcs psb_gem_object_funcs = {
> +static const struct drm_gem_object_funcs psb_gem_object_funcs = {
>         .free = psb_gem_free_object,
>         .vm_ops = &psb_gem_vm_ops,
>  };
>
> -struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len,
> -                                     const char *name, int backed, u32 align)
> +static struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len,
> +                                            const char *name, int backed, u32 align)
>  {
>         struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>         struct gtt_range *gt;
> @@ -182,12 +182,16 @@ psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen,
>
>         obj->funcs = &psb_gem_object_funcs;
>
> -       ret = drm_gem_object_init(dev, obj, size);
> -       if (ret)
> -               goto err_psb_gtt_free_range;
> +       if (stolen) {
> +               drm_gem_private_object_init(dev, obj, size);
> +       } else {
> +               ret = drm_gem_object_init(dev, obj, size);
> +               if (ret)
> +                       goto err_psb_gtt_free_range;
>
> -       /* Limit the object to 32-bit mappings */
> -       mapping_set_gfp_mask(obj->filp->f_mapping, GFP_KERNEL | __GFP_DMA32);
> +               /* Limit the object to 32-bit mappings */
> +               mapping_set_gfp_mask(obj->filp->f_mapping, GFP_KERNEL | __GFP_DMA32);
> +       }
>
>         return gt;
>
> diff --git a/drivers/gpu/drm/gma500/gem.h b/drivers/gpu/drm/gma500/gem.h
> index ad76127dc719..6b67c58cbed5 100644
> --- a/drivers/gpu/drm/gma500/gem.h
> +++ b/drivers/gpu/drm/gma500/gem.h
> @@ -12,14 +12,9 @@
>
>  struct drm_device;
>
> -extern const struct drm_gem_object_funcs psb_gem_object_funcs;
> -
>  struct gtt_range *
>  psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, u32 align);
>
> -struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len, const char *name,
> -                                     int backed, u32 align);
> -void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt);
>  int psb_gtt_pin(struct gtt_range *gt);
>  void psb_gtt_unpin(struct gtt_range *gt);
>
> diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c
> index 8285358fac01..8c95b50034a5 100644
> --- a/drivers/gpu/drm/gma500/gma_display.c
> +++ b/drivers/gpu/drm/gma500/gma_display.c
> @@ -498,6 +498,9 @@ void gma_crtc_destroy(struct drm_crtc *crtc)
>  {
>         struct gma_crtc *gma_crtc = to_gma_crtc(crtc);
>
> +       if (gma_crtc->cursor_gt)
> +               drm_gem_object_put(&gma_crtc->cursor_gt->gem);
> +
>         kfree(gma_crtc->crtc_state);
>         drm_crtc_cleanup(crtc);
>         kfree(gma_crtc);
> diff --git a/drivers/gpu/drm/gma500/psb_intel_display.c b/drivers/gpu/drm/gma500/psb_intel_display.c
> index 18d5f7e5889e..b5e9118c01a4 100644
> --- a/drivers/gpu/drm/gma500/psb_intel_display.c
> +++ b/drivers/gpu/drm/gma500/psb_intel_display.c
> @@ -461,9 +461,8 @@ static void psb_intel_cursor_init(struct drm_device *dev,
>                 /* Allocate 4 pages of stolen mem for a hardware cursor. That
>                  * is enough for the 64 x 64 ARGB cursors we support.
>                  */
> -               cursor_gt = psb_gtt_alloc_range(dev, 4 * PAGE_SIZE, "cursor", 1,
> -                                               PAGE_SIZE);
> -               if (!cursor_gt) {
> +               cursor_gt = psb_gem_create(dev, 4 * PAGE_SIZE, "cursor", true, PAGE_SIZE);
> +               if (IS_ERR(cursor_gt)) {
>                         gma_crtc->cursor_gt = NULL;
>                         goto out;
>                 }
> --
> 2.33.0
>

[PATCH 05/10] drm/gma500: Rename psb_gtt_{pin, unpin}() to psb_gem_{pin, unpin}()

[Thomas Zimmermann]

Rename psb_gtt_pin() to psb_gem_pin() to reflect the semantics of the
function. Same for psb_gtt_unpin(). No functional changes.

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

diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index 4acab39a583a..369910d0091e 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -41,7 +41,7 @@ static void psb_gtt_detach_pages(struct gtt_range *gt)
 	gt->pages = NULL;
 }
 
-int psb_gtt_pin(struct gtt_range *gt)
+int psb_gem_pin(struct gtt_range *gt)
 {
 	int ret = 0;
 	struct drm_device *dev = gt->gem.dev;
@@ -69,7 +69,7 @@ int psb_gtt_pin(struct gtt_range *gt)
 	return ret;
 }
 
-void psb_gtt_unpin(struct gtt_range *gt)
+void psb_gem_unpin(struct gtt_range *gt)
 {
 	struct drm_device *dev = gt->gem.dev;
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
@@ -94,7 +94,7 @@ static void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt)
 {
 	/* Undo the mmap pin if we are destroying the object */
 	if (gt->mmapping) {
-		psb_gtt_unpin(gt);
+		psb_gem_unpin(gt);
 		gt->mmapping = 0;
 	}
 	WARN_ON(gt->in_gart && !gt->stolen);
@@ -290,7 +290,7 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf)
 	/* For now the mmap pins the object and it stays pinned. As things
 	   stand that will do us no harm */
 	if (r->mmapping == 0) {
-		err = psb_gtt_pin(r);
+		err = psb_gem_pin(r);
 		if (err < 0) {
 			dev_err(dev->dev, "gma500: pin failed: %d\n", err);
 			ret = vmf_error(err);
diff --git a/drivers/gpu/drm/gma500/gem.h b/drivers/gpu/drm/gma500/gem.h
index 6b67c58cbed5..21c86df482a6 100644
--- a/drivers/gpu/drm/gma500/gem.h
+++ b/drivers/gpu/drm/gma500/gem.h
@@ -15,7 +15,7 @@ struct drm_device;
 struct gtt_range *
 psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, u32 align);
 
-int psb_gtt_pin(struct gtt_range *gt);
-void psb_gtt_unpin(struct gtt_range *gt);
+int psb_gem_pin(struct gtt_range *gt);
+void psb_gem_unpin(struct gtt_range *gt);
 
 #endif
diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c
index 8c95b50034a5..6d0470b27bc5 100644
--- a/drivers/gpu/drm/gma500/gma_display.c
+++ b/drivers/gpu/drm/gma500/gma_display.c
@@ -75,7 +75,7 @@ int gma_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 
 	/* We are displaying this buffer, make sure it is actually loaded
 	   into the GTT */
-	ret = psb_gtt_pin(gtt);
+	ret = psb_gem_pin(gtt);
 	if (ret < 0)
 		goto gma_pipe_set_base_exit;
 	start = gtt->offset;
@@ -126,7 +126,7 @@ int gma_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 gma_pipe_cleaner:
 	/* If there was a previous display we can now unpin it */
 	if (old_fb)
-		psb_gtt_unpin(to_gtt_range(old_fb->obj[0]));
+		psb_gem_unpin(to_gtt_range(old_fb->obj[0]));
 
 gma_pipe_set_base_exit:
 	gma_power_end(dev);
@@ -350,7 +350,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
 		/* Unpin the old GEM object */
 		if (gma_crtc->cursor_obj) {
 			gt = to_gtt_range(gma_crtc->cursor_obj);
-			psb_gtt_unpin(gt);
+			psb_gem_unpin(gt);
 			drm_gem_object_put(gma_crtc->cursor_obj);
 			gma_crtc->cursor_obj = NULL;
 		}
@@ -378,7 +378,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
 	gt = to_gtt_range(obj);
 
 	/* Pin the memory into the GTT */
-	ret = psb_gtt_pin(gt);
+	ret = psb_gem_pin(gt);
 	if (ret) {
 		dev_err(dev->dev, "Can not pin down handle 0x%x\n", handle);
 		goto unref_cursor;
@@ -426,7 +426,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
 	/* unpin the old bo */
 	if (gma_crtc->cursor_obj) {
 		gt = to_gtt_range(gma_crtc->cursor_obj);
-		psb_gtt_unpin(gt);
+		psb_gem_unpin(gt);
 		drm_gem_object_put(gma_crtc->cursor_obj);
 	}
 
@@ -490,7 +490,7 @@ void gma_crtc_disable(struct drm_crtc *crtc)
 
 	if (crtc->primary->fb) {
 		gt = to_gtt_range(crtc->primary->fb->obj[0]);
-		psb_gtt_unpin(gt);
+		psb_gem_unpin(gt);
 	}
 }
 
-- 
2.33.0

Re: [PATCH 05/10] drm/gma500: Rename psb_gtt_{pin, unpin}() to psb_gem_{pin, unpin}()

[Patrik Jakobsson]

On Tue, Sep 28, 2021 at 10:44 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Rename psb_gtt_pin() to psb_gem_pin() to reflect the semantics of the
> function. Same for psb_gtt_unpin(). No functional changes.

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


>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/gma500/gem.c         |  8 ++++----
>  drivers/gpu/drm/gma500/gem.h         |  4 ++--
>  drivers/gpu/drm/gma500/gma_display.c | 12 ++++++------
>  3 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
> index 4acab39a583a..369910d0091e 100644
> --- a/drivers/gpu/drm/gma500/gem.c
> +++ b/drivers/gpu/drm/gma500/gem.c
> @@ -41,7 +41,7 @@ static void psb_gtt_detach_pages(struct gtt_range *gt)
>         gt->pages = NULL;
>  }
>
> -int psb_gtt_pin(struct gtt_range *gt)
> +int psb_gem_pin(struct gtt_range *gt)
>  {
>         int ret = 0;
>         struct drm_device *dev = gt->gem.dev;
> @@ -69,7 +69,7 @@ int psb_gtt_pin(struct gtt_range *gt)
>         return ret;
>  }
>
> -void psb_gtt_unpin(struct gtt_range *gt)
> +void psb_gem_unpin(struct gtt_range *gt)
>  {
>         struct drm_device *dev = gt->gem.dev;
>         struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> @@ -94,7 +94,7 @@ static void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt)
>  {
>         /* Undo the mmap pin if we are destroying the object */
>         if (gt->mmapping) {
> -               psb_gtt_unpin(gt);
> +               psb_gem_unpin(gt);
>                 gt->mmapping = 0;
>         }
>         WARN_ON(gt->in_gart && !gt->stolen);
> @@ -290,7 +290,7 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf)
>         /* For now the mmap pins the object and it stays pinned. As things
>            stand that will do us no harm */
>         if (r->mmapping == 0) {
> -               err = psb_gtt_pin(r);
> +               err = psb_gem_pin(r);
>                 if (err < 0) {
>                         dev_err(dev->dev, "gma500: pin failed: %d\n", err);
>                         ret = vmf_error(err);
> diff --git a/drivers/gpu/drm/gma500/gem.h b/drivers/gpu/drm/gma500/gem.h
> index 6b67c58cbed5..21c86df482a6 100644
> --- a/drivers/gpu/drm/gma500/gem.h
> +++ b/drivers/gpu/drm/gma500/gem.h
> @@ -15,7 +15,7 @@ struct drm_device;
>  struct gtt_range *
>  psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, u32 align);
>
> -int psb_gtt_pin(struct gtt_range *gt);
> -void psb_gtt_unpin(struct gtt_range *gt);
> +int psb_gem_pin(struct gtt_range *gt);
> +void psb_gem_unpin(struct gtt_range *gt);
>
>  #endif
> diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c
> index 8c95b50034a5..6d0470b27bc5 100644
> --- a/drivers/gpu/drm/gma500/gma_display.c
> +++ b/drivers/gpu/drm/gma500/gma_display.c
> @@ -75,7 +75,7 @@ int gma_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>
>         /* We are displaying this buffer, make sure it is actually loaded
>            into the GTT */
> -       ret = psb_gtt_pin(gtt);
> +       ret = psb_gem_pin(gtt);
>         if (ret < 0)
>                 goto gma_pipe_set_base_exit;
>         start = gtt->offset;
> @@ -126,7 +126,7 @@ int gma_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>  gma_pipe_cleaner:
>         /* If there was a previous display we can now unpin it */
>         if (old_fb)
> -               psb_gtt_unpin(to_gtt_range(old_fb->obj[0]));
> +               psb_gem_unpin(to_gtt_range(old_fb->obj[0]));
>
>  gma_pipe_set_base_exit:
>         gma_power_end(dev);
> @@ -350,7 +350,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
>                 /* Unpin the old GEM object */
>                 if (gma_crtc->cursor_obj) {
>                         gt = to_gtt_range(gma_crtc->cursor_obj);
> -                       psb_gtt_unpin(gt);
> +                       psb_gem_unpin(gt);
>                         drm_gem_object_put(gma_crtc->cursor_obj);
>                         gma_crtc->cursor_obj = NULL;
>                 }
> @@ -378,7 +378,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
>         gt = to_gtt_range(obj);
>
>         /* Pin the memory into the GTT */
> -       ret = psb_gtt_pin(gt);
> +       ret = psb_gem_pin(gt);
>         if (ret) {
>                 dev_err(dev->dev, "Can not pin down handle 0x%x\n", handle);
>                 goto unref_cursor;
> @@ -426,7 +426,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
>         /* unpin the old bo */
>         if (gma_crtc->cursor_obj) {
>                 gt = to_gtt_range(gma_crtc->cursor_obj);
> -               psb_gtt_unpin(gt);
> +               psb_gem_unpin(gt);
>                 drm_gem_object_put(gma_crtc->cursor_obj);
>         }
>
> @@ -490,7 +490,7 @@ void gma_crtc_disable(struct drm_crtc *crtc)
>
>         if (crtc->primary->fb) {
>                 gt = to_gtt_range(crtc->primary->fb->obj[0]);
> -               psb_gtt_unpin(gt);
> +               psb_gem_unpin(gt);
>         }
>  }
>
> --
> 2.33.0
>

[PATCH 06/10] drm/gma500: Inline psb_gtt_attach_pages() and psb_gtt_detach_pages()

[Thomas Zimmermann]

psb_gtt_attach_pages() are not GTT functions but deal with the GEM
object's SHMEM pages. The only callers of psb_gtt_attach_pages() and
psb_gtt_detach_pages() are the GEM pin helpers. Inline the calls and
cleanup the resulting code.

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

diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index 369910d0091e..a48d7d5ed026 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -19,53 +19,45 @@
 #include "gem.h"
 #include "psb_drv.h"
 
-static int psb_gtt_attach_pages(struct gtt_range *gt)
+int psb_gem_pin(struct gtt_range *gt)
 {
+	int ret = 0;
+	struct drm_device *dev = gt->gem.dev;
+	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
+	u32 gpu_base = dev_priv->gtt.gatt_start;
 	struct page **pages;
+	unsigned int npages;
 
-	WARN_ON(gt->pages);
+	mutex_lock(&dev_priv->gtt_mutex);
+
+	if (gt->in_gart || gt->stolen)
+		goto out; /* already mapped */
 
 	pages = drm_gem_get_pages(&gt->gem);
 	if (IS_ERR(pages))
 		return PTR_ERR(pages);
 
-	gt->npage = gt->gem.size / PAGE_SIZE;
-	gt->pages = pages;
-
-	return 0;
-}
+	npages = gt->gem.size / PAGE_SIZE;
 
-static void psb_gtt_detach_pages(struct gtt_range *gt)
-{
-	drm_gem_put_pages(&gt->gem, gt->pages, true, false);
-	gt->pages = NULL;
-}
+	ret = psb_gtt_insert(dev, gt, 0);
+	if (ret)
+		goto err_drm_gem_put_pages;
 
-int psb_gem_pin(struct gtt_range *gt)
-{
-	int ret = 0;
-	struct drm_device *dev = gt->gem.dev;
-	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
-	u32 gpu_base = dev_priv->gtt.gatt_start;
+	psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu), pages,
+			     (gpu_base + gt->offset), npages, 0, 0,
+			     PSB_MMU_CACHED_MEMORY);
 
-	mutex_lock(&dev_priv->gtt_mutex);
+	gt->npage = npages;
+	gt->pages = pages;
 
-	if (gt->in_gart == 0 && gt->stolen == 0) {
-		ret = psb_gtt_attach_pages(gt);
-		if (ret < 0)
-			goto out;
-		ret = psb_gtt_insert(dev, gt, 0);
-		if (ret < 0) {
-			psb_gtt_detach_pages(gt);
-			goto out;
-		}
-		psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu),
-				     gt->pages, (gpu_base + gt->offset),
-				     gt->npage, 0, 0, PSB_MMU_CACHED_MEMORY);
-	}
-	gt->in_gart++;
 out:
+	++gt->in_gart;
 	mutex_unlock(&dev_priv->gtt_mutex);
+
+	return 0;
+
+err_drm_gem_put_pages:
+	drm_gem_put_pages(&gt->gem, pages, true, false);
 	return ret;
 }
 
@@ -79,14 +71,19 @@ void psb_gem_unpin(struct gtt_range *gt)
 
 	WARN_ON(!gt->in_gart);
 
-	gt->in_gart--;
-	if (gt->in_gart == 0 && gt->stolen == 0) {
-		psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu),
+	--gt->in_gart;
+
+	if (gt->in_gart || gt->stolen)
+		goto out;
+
+	psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu),
 				     (gpu_base + gt->offset), gt->npage, 0, 0);
-		psb_gtt_remove(dev, gt);
-		psb_gtt_detach_pages(gt);
-	}
+	psb_gtt_remove(dev, gt);
 
+	drm_gem_put_pages(&gt->gem, gt->pages, true, false);
+	gt->pages = NULL;
+
+out:
 	mutex_unlock(&dev_priv->gtt_mutex);
 }
 
-- 
2.33.0

Re: [PATCH 06/10] drm/gma500: Inline psb_gtt_attach_pages() and psb_gtt_detach_pages()

[Patrik Jakobsson]

On Tue, Sep 28, 2021 at 10:44 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> psb_gtt_attach_pages() are not GTT functions but deal with the GEM
> object's SHMEM pages. The only callers of psb_gtt_attach_pages() and
> psb_gtt_detach_pages() are the GEM pin helpers. Inline the calls and
> cleanup the resulting code.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/gma500/gem.c | 75 +++++++++++++++++-------------------
>  1 file changed, 36 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
> index 369910d0091e..a48d7d5ed026 100644
> --- a/drivers/gpu/drm/gma500/gem.c
> +++ b/drivers/gpu/drm/gma500/gem.c
> @@ -19,53 +19,45 @@
>  #include "gem.h"
>  #include "psb_drv.h"
>
> -static int psb_gtt_attach_pages(struct gtt_range *gt)
> +int psb_gem_pin(struct gtt_range *gt)
>  {
> +       int ret = 0;
> +       struct drm_device *dev = gt->gem.dev;
> +       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> +       u32 gpu_base = dev_priv->gtt.gatt_start;
>         struct page **pages;
> +       unsigned int npages;
>
> -       WARN_ON(gt->pages);
> +       mutex_lock(&dev_priv->gtt_mutex);
> +
> +       if (gt->in_gart || gt->stolen)
> +               goto out; /* already mapped */
>
>         pages = drm_gem_get_pages(&gt->gem);
>         if (IS_ERR(pages))
>                 return PTR_ERR(pages);

You're not releasing gtt_mutex here


>
> -       gt->npage = gt->gem.size / PAGE_SIZE;
> -       gt->pages = pages;
> -
> -       return 0;
> -}
> +       npages = gt->gem.size / PAGE_SIZE;
>
> -static void psb_gtt_detach_pages(struct gtt_range *gt)
> -{
> -       drm_gem_put_pages(&gt->gem, gt->pages, true, false);
> -       gt->pages = NULL;
> -}
> +       ret = psb_gtt_insert(dev, gt, 0);
> +       if (ret)
> +               goto err_drm_gem_put_pages;
>
> -int psb_gem_pin(struct gtt_range *gt)
> -{
> -       int ret = 0;
> -       struct drm_device *dev = gt->gem.dev;
> -       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> -       u32 gpu_base = dev_priv->gtt.gatt_start;
> +       psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu), pages,
> +                            (gpu_base + gt->offset), npages, 0, 0,
> +                            PSB_MMU_CACHED_MEMORY);
>
> -       mutex_lock(&dev_priv->gtt_mutex);
> +       gt->npage = npages;
> +       gt->pages = pages;
>
> -       if (gt->in_gart == 0 && gt->stolen == 0) {
> -               ret = psb_gtt_attach_pages(gt);
> -               if (ret < 0)
> -                       goto out;
> -               ret = psb_gtt_insert(dev, gt, 0);
> -               if (ret < 0) {
> -                       psb_gtt_detach_pages(gt);
> -                       goto out;
> -               }
> -               psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu),
> -                                    gt->pages, (gpu_base + gt->offset),
> -                                    gt->npage, 0, 0, PSB_MMU_CACHED_MEMORY);
> -       }
> -       gt->in_gart++;
>  out:
> +       ++gt->in_gart;
>         mutex_unlock(&dev_priv->gtt_mutex);
> +
> +       return 0;
> +
> +err_drm_gem_put_pages:
> +       drm_gem_put_pages(&gt->gem, pages, true, false);
>         return ret;
>  }
>
> @@ -79,14 +71,19 @@ void psb_gem_unpin(struct gtt_range *gt)
>
>         WARN_ON(!gt->in_gart);
>
> -       gt->in_gart--;
> -       if (gt->in_gart == 0 && gt->stolen == 0) {
> -               psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu),
> +       --gt->in_gart;
> +
> +       if (gt->in_gart || gt->stolen)
> +               goto out;
> +
> +       psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu),
>                                      (gpu_base + gt->offset), gt->npage, 0, 0);
> -               psb_gtt_remove(dev, gt);
> -               psb_gtt_detach_pages(gt);
> -       }
> +       psb_gtt_remove(dev, gt);
>
> +       drm_gem_put_pages(&gt->gem, gt->pages, true, false);
> +       gt->pages = NULL;
> +
> +out:
>         mutex_unlock(&dev_priv->gtt_mutex);
>  }
>
> --
> 2.33.0
>

[PATCH 07/10] drm/gma500: Inline psb_gtt_{alloc, free}_range() into rsp callers

[Thomas Zimmermann]

psb_gtt_alloc_range() allocates struct gtt_range, create the GTT resource
and performs some half-baked initialization. Inline the function into its
only caller psb_gem_create(). For creating the GTT resource, introduce a
new helper, psb_gtt_alloc_resource() that hides the details of the GTT.

For psb_gtt_free_range(), inline the function into its only caller
psb_gem_free_object(). While at it, remove the explicit invocation of
drm_gem_free_mmap_offset(). The mmap offset is already released by
drm_gem_object_release().

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/gma500/gem.c | 94 ++++++++++++------------------------
 drivers/gpu/drm/gma500/gtt.c | 27 +++++++++++
 drivers/gpu/drm/gma500/gtt.h |  6 +++
 3 files changed, 65 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index a48d7d5ed026..46209e10dcc2 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -87,30 +87,22 @@ void psb_gem_unpin(struct gtt_range *gt)
 	mutex_unlock(&dev_priv->gtt_mutex);
 }
 
-static void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt)
-{
-	/* Undo the mmap pin if we are destroying the object */
-	if (gt->mmapping) {
-		psb_gem_unpin(gt);
-		gt->mmapping = 0;
-	}
-	WARN_ON(gt->in_gart && !gt->stolen);
-	release_resource(&gt->resource);
-	kfree(gt);
-}
-
 static vm_fault_t psb_gem_fault(struct vm_fault *vmf);
 
 static void psb_gem_free_object(struct drm_gem_object *obj)
 {
-	struct gtt_range *gtt = to_gtt_range(obj);
+	struct gtt_range *gt = to_gtt_range(obj);
 
-	/* Remove the list map if one is present */
-	drm_gem_free_mmap_offset(obj);
 	drm_gem_object_release(obj);
 
-	/* This must occur last as it frees up the memory of the GEM object */
-	psb_gtt_free_range(obj->dev, gtt);
+	/* Undo the mmap pin if we are destroying the object */
+	if (gt->mmapping)
+		psb_gem_unpin(gt);
+
+	WARN_ON(gt->in_gart && !gt->stolen);
+
+	release_resource(&gt->resource);
+	kfree(gt);
 }
 
 static const struct vm_operations_struct psb_gem_vm_ops = {
@@ -124,59 +116,35 @@ static const struct drm_gem_object_funcs psb_gem_object_funcs = {
 	.vm_ops = &psb_gem_vm_ops,
 };
 
-static struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len,
-					     const char *name, int backed, u32 align)
-{
-	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
-	struct gtt_range *gt;
-	struct resource *r = dev_priv->gtt_mem;
-	int ret;
-	unsigned long start, end;
-
-	if (backed) {
-		/* The start of the GTT is the stolen pages */
-		start = r->start;
-		end = r->start + dev_priv->gtt.stolen_size - 1;
-	} else {
-		/* The rest we will use for GEM backed objects */
-		start = r->start + dev_priv->gtt.stolen_size;
-		end = r->end;
-	}
-
-	gt = kzalloc(sizeof(struct gtt_range), GFP_KERNEL);
-	if (gt == NULL)
-		return NULL;
-	gt->resource.name = name;
-	gt->stolen = backed;
-	gt->in_gart = backed;
-	/* Ensure this is set for non GEM objects */
-	gt->gem.dev = dev;
-	ret = allocate_resource(dev_priv->gtt_mem, &gt->resource,
-				len, start, end, align, NULL, NULL);
-	if (ret == 0) {
-		gt->offset = gt->resource.start - r->start;
-		return gt;
-	}
-	kfree(gt);
-	return NULL;
-}
-
 struct gtt_range *
 psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, u32 align)
 {
+	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
 	struct gtt_range *gt;
 	struct drm_gem_object *obj;
 	int ret;
 
 	size = roundup(size, PAGE_SIZE);
 
-	gt = psb_gtt_alloc_range(dev, size, name, stolen, align);
-	if (!gt) {
-		dev_err(dev->dev, "no memory for %lld byte GEM object\n", size);
-		return ERR_PTR(-ENOSPC);
-	}
+	gt = kzalloc(sizeof(*gt), GFP_KERNEL);
+	if (!gt)
+		return ERR_PTR(-ENOMEM);
 	obj = &gt->gem;
 
+	/* GTT resource */
+
+	ret = psb_gtt_allocate_resource(dev_priv, &gt->resource, name, size, align, stolen,
+					&gt->offset);
+	if (ret)
+		goto err_kfree;
+
+	if (stolen) {
+		gt->stolen = true;
+		gt->in_gart = 1;
+	}
+
+	/* GEM object */
+
 	obj->funcs = &psb_gem_object_funcs;
 
 	if (stolen) {
@@ -184,7 +152,7 @@ psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen,
 	} else {
 		ret = drm_gem_object_init(dev, obj, size);
 		if (ret)
-			goto err_psb_gtt_free_range;
+			goto err_release_resource;
 
 		/* Limit the object to 32-bit mappings */
 		mapping_set_gfp_mask(obj->filp->f_mapping, GFP_KERNEL | __GFP_DMA32);
@@ -192,8 +160,10 @@ psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen,
 
 	return gt;
 
-err_psb_gtt_free_range:
-	psb_gtt_free_range(dev, gt);
+err_release_resource:
+	release_resource(&gt->resource);
+err_kfree:
+	kfree(gt);
 	return ERR_PTR(ret);
 }
 
diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
index 0aacf7122e32..5d940fdbe6b8 100644
--- a/drivers/gpu/drm/gma500/gtt.c
+++ b/drivers/gpu/drm/gma500/gtt.c
@@ -18,6 +18,33 @@
  *	GTT resource allocator - manage page mappings in GTT space
  */
 
+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[static 1])
+{
+	struct resource *root = pdev->gtt_mem;
+	resource_size_t start, end;
+	int ret;
+
+	if (stolen) {
+		/* The start of the GTT is backed by stolen pages. */
+		start = root->start;
+		end = root->start + pdev->gtt.stolen_size - 1;
+	} else {
+		/* The rest is backed by system pages. */
+		start = root->start + pdev->gtt.stolen_size;
+		end = root->end;
+	}
+
+	res->name = name;
+	ret = allocate_resource(root, res, size, start, end, align, NULL, NULL);
+	if (ret)
+		return ret;
+	*offset = res->start - root->start;
+
+	return 0;
+}
+
 /**
  *	psb_gtt_mask_pte	-	generate GTT pte entry
  *	@pfn: page number to encode
diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
index 36162b545570..459a03141e8b 100644
--- a/drivers/gpu/drm/gma500/gtt.h
+++ b/drivers/gpu/drm/gma500/gtt.h
@@ -10,6 +10,8 @@
 
 #include <drm/drm_gem.h>
 
+struct drm_psb_private;
+
 /* This wants cleaning up with respect to the psb_dev and un-needed stuff */
 struct psb_gtt {
 	uint32_t gatt_start;
@@ -43,6 +45,10 @@ struct gtt_range {
 
 extern int psb_gtt_restore(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,
+			      bool stolen, u32 offset[static 1]);
+
 int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume);
 void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r);
 
-- 
2.33.0

Re: [PATCH 07/10] drm/gma500: Inline psb_gtt_{alloc,free}_range() into rsp callers

[Patrik Jakobsson]

On Tue, Sep 28, 2021 at 10:44 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> psb_gtt_alloc_range() allocates struct gtt_range, create the GTT resource
> and performs some half-baked initialization. Inline the function into its
> only caller psb_gem_create(). For creating the GTT resource, introduce a
> new helper, psb_gtt_alloc_resource() that hides the details of the GTT.
>
> For psb_gtt_free_range(), inline the function into its only caller
> psb_gem_free_object(). While at it, remove the explicit invocation of
> drm_gem_free_mmap_offset(). The mmap offset is already released by
> drm_gem_object_release().
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/gma500/gem.c | 94 ++++++++++++------------------------
>  drivers/gpu/drm/gma500/gtt.c | 27 +++++++++++
>  drivers/gpu/drm/gma500/gtt.h |  6 +++
>  3 files changed, 65 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
> index a48d7d5ed026..46209e10dcc2 100644
> --- a/drivers/gpu/drm/gma500/gem.c
> +++ b/drivers/gpu/drm/gma500/gem.c
> @@ -87,30 +87,22 @@ void psb_gem_unpin(struct gtt_range *gt)
>         mutex_unlock(&dev_priv->gtt_mutex);
>  }
>
> -static void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt)
> -{
> -       /* Undo the mmap pin if we are destroying the object */
> -       if (gt->mmapping) {
> -               psb_gem_unpin(gt);
> -               gt->mmapping = 0;
> -       }
> -       WARN_ON(gt->in_gart && !gt->stolen);
> -       release_resource(&gt->resource);
> -       kfree(gt);
> -}
> -
>  static vm_fault_t psb_gem_fault(struct vm_fault *vmf);
>
>  static void psb_gem_free_object(struct drm_gem_object *obj)
>  {
> -       struct gtt_range *gtt = to_gtt_range(obj);
> +       struct gtt_range *gt = to_gtt_range(obj);
>
> -       /* Remove the list map if one is present */
> -       drm_gem_free_mmap_offset(obj);
>         drm_gem_object_release(obj);
>
> -       /* This must occur last as it frees up the memory of the GEM object */
> -       psb_gtt_free_range(obj->dev, gtt);
> +       /* Undo the mmap pin if we are destroying the object */
> +       if (gt->mmapping)
> +               psb_gem_unpin(gt);
> +
> +       WARN_ON(gt->in_gart && !gt->stolen);
> +
> +       release_resource(&gt->resource);
> +       kfree(gt);
>  }
>
>  static const struct vm_operations_struct psb_gem_vm_ops = {
> @@ -124,59 +116,35 @@ static const struct drm_gem_object_funcs psb_gem_object_funcs = {
>         .vm_ops = &psb_gem_vm_ops,
>  };
>
> -static struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len,
> -                                            const char *name, int backed, u32 align)
> -{
> -       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> -       struct gtt_range *gt;
> -       struct resource *r = dev_priv->gtt_mem;
> -       int ret;
> -       unsigned long start, end;
> -
> -       if (backed) {
> -               /* The start of the GTT is the stolen pages */
> -               start = r->start;
> -               end = r->start + dev_priv->gtt.stolen_size - 1;
> -       } else {
> -               /* The rest we will use for GEM backed objects */
> -               start = r->start + dev_priv->gtt.stolen_size;
> -               end = r->end;
> -       }
> -
> -       gt = kzalloc(sizeof(struct gtt_range), GFP_KERNEL);
> -       if (gt == NULL)
> -               return NULL;
> -       gt->resource.name = name;
> -       gt->stolen = backed;
> -       gt->in_gart = backed;
> -       /* Ensure this is set for non GEM objects */
> -       gt->gem.dev = dev;
> -       ret = allocate_resource(dev_priv->gtt_mem, &gt->resource,
> -                               len, start, end, align, NULL, NULL);
> -       if (ret == 0) {
> -               gt->offset = gt->resource.start - r->start;
> -               return gt;
> -       }
> -       kfree(gt);
> -       return NULL;
> -}
> -
>  struct gtt_range *
>  psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, u32 align)
>  {
> +       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>         struct gtt_range *gt;
>         struct drm_gem_object *obj;
>         int ret;
>
>         size = roundup(size, PAGE_SIZE);
>
> -       gt = psb_gtt_alloc_range(dev, size, name, stolen, align);
> -       if (!gt) {
> -               dev_err(dev->dev, "no memory for %lld byte GEM object\n", size);
> -               return ERR_PTR(-ENOSPC);
> -       }
> +       gt = kzalloc(sizeof(*gt), GFP_KERNEL);
> +       if (!gt)
> +               return ERR_PTR(-ENOMEM);
>         obj = &gt->gem;
>
> +       /* GTT resource */
> +
> +       ret = psb_gtt_allocate_resource(dev_priv, &gt->resource, name, size, align, stolen,
> +                                       &gt->offset);
> +       if (ret)
> +               goto err_kfree;
> +
> +       if (stolen) {
> +               gt->stolen = true;
> +               gt->in_gart = 1;
> +       }
> +
> +       /* GEM object */
> +
>         obj->funcs = &psb_gem_object_funcs;
>
>         if (stolen) {
> @@ -184,7 +152,7 @@ psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen,
>         } else {
>                 ret = drm_gem_object_init(dev, obj, size);
>                 if (ret)
> -                       goto err_psb_gtt_free_range;
> +                       goto err_release_resource;
>
>                 /* Limit the object to 32-bit mappings */
>                 mapping_set_gfp_mask(obj->filp->f_mapping, GFP_KERNEL | __GFP_DMA32);
> @@ -192,8 +160,10 @@ psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen,
>
>         return gt;
>
> -err_psb_gtt_free_range:
> -       psb_gtt_free_range(dev, gt);
> +err_release_resource:
> +       release_resource(&gt->resource);
> +err_kfree:
> +       kfree(gt);
>         return ERR_PTR(ret);
>  }
>
> diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
> index 0aacf7122e32..5d940fdbe6b8 100644
> --- a/drivers/gpu/drm/gma500/gtt.c
> +++ b/drivers/gpu/drm/gma500/gtt.c
> @@ -18,6 +18,33 @@
>   *     GTT resource allocator - manage page mappings in GTT space
>   */
>
> +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[static 1])

Why [static 1]?


> +{
> +       struct resource *root = pdev->gtt_mem;
> +       resource_size_t start, end;
> +       int ret;
> +
> +       if (stolen) {
> +               /* The start of the GTT is backed by stolen pages. */
> +               start = root->start;
> +               end = root->start + pdev->gtt.stolen_size - 1;
> +       } else {
> +               /* The rest is backed by system pages. */
> +               start = root->start + pdev->gtt.stolen_size;
> +               end = root->end;
> +       }
> +
> +       res->name = name;
> +       ret = allocate_resource(root, res, size, start, end, align, NULL, NULL);
> +       if (ret)
> +               return ret;
> +       *offset = res->start - root->start;
> +
> +       return 0;
> +}
> +
>  /**
>   *     psb_gtt_mask_pte        -       generate GTT pte entry
>   *     @pfn: page number to encode
> diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
> index 36162b545570..459a03141e8b 100644
> --- a/drivers/gpu/drm/gma500/gtt.h
> +++ b/drivers/gpu/drm/gma500/gtt.h
> @@ -10,6 +10,8 @@
>
>  #include <drm/drm_gem.h>
>
> +struct drm_psb_private;
> +
>  /* This wants cleaning up with respect to the psb_dev and un-needed stuff */
>  struct psb_gtt {
>         uint32_t gatt_start;
> @@ -43,6 +45,10 @@ struct gtt_range {
>
>  extern int psb_gtt_restore(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,
> +                             bool stolen, u32 offset[static 1]);
> +
>  int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume);
>  void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r);
>
> --
> 2.33.0
>

Re: [PATCH 07/10] drm/gma500: Inline psb_gtt_{alloc,free}_range() into rsp callers

[Thomas Zimmermann]

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

Hi

Am 03.10.21 um 00:14 schrieb Patrik Jakobsson:
> On Tue, Sep 28, 2021 at 10:44 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> psb_gtt_alloc_range() allocates struct gtt_range, create the GTT resource
>> and performs some half-baked initialization. Inline the function into its
>> only caller psb_gem_create(). For creating the GTT resource, introduce a
>> new helper, psb_gtt_alloc_resource() that hides the details of the GTT.
>>
>> For psb_gtt_free_range(), inline the function into its only caller
>> psb_gem_free_object(). While at it, remove the explicit invocation of
>> drm_gem_free_mmap_offset(). The mmap offset is already released by
>> drm_gem_object_release().
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/gma500/gem.c | 94 ++++++++++++------------------------
>>   drivers/gpu/drm/gma500/gtt.c | 27 +++++++++++
>>   drivers/gpu/drm/gma500/gtt.h |  6 +++
>>   3 files changed, 65 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
>> index a48d7d5ed026..46209e10dcc2 100644
>> --- a/drivers/gpu/drm/gma500/gem.c
>> +++ b/drivers/gpu/drm/gma500/gem.c
>> @@ -87,30 +87,22 @@ void psb_gem_unpin(struct gtt_range *gt)
>>          mutex_unlock(&dev_priv->gtt_mutex);
>>   }
>>
>> -static void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt)
>> -{
>> -       /* Undo the mmap pin if we are destroying the object */
>> -       if (gt->mmapping) {
>> -               psb_gem_unpin(gt);
>> -               gt->mmapping = 0;
>> -       }
>> -       WARN_ON(gt->in_gart && !gt->stolen);
>> -       release_resource(&gt->resource);
>> -       kfree(gt);
>> -}
>> -
>>   static vm_fault_t psb_gem_fault(struct vm_fault *vmf);
>>
>>   static void psb_gem_free_object(struct drm_gem_object *obj)
>>   {
>> -       struct gtt_range *gtt = to_gtt_range(obj);
>> +       struct gtt_range *gt = to_gtt_range(obj);
>>
>> -       /* Remove the list map if one is present */
>> -       drm_gem_free_mmap_offset(obj);
>>          drm_gem_object_release(obj);
>>
>> -       /* This must occur last as it frees up the memory of the GEM object */
>> -       psb_gtt_free_range(obj->dev, gtt);
>> +       /* Undo the mmap pin if we are destroying the object */
>> +       if (gt->mmapping)
>> +               psb_gem_unpin(gt);
>> +
>> +       WARN_ON(gt->in_gart && !gt->stolen);
>> +
>> +       release_resource(&gt->resource);
>> +       kfree(gt);
>>   }
>>
>>   static const struct vm_operations_struct psb_gem_vm_ops = {
>> @@ -124,59 +116,35 @@ static const struct drm_gem_object_funcs psb_gem_object_funcs = {
>>          .vm_ops = &psb_gem_vm_ops,
>>   };
>>
>> -static struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len,
>> -                                            const char *name, int backed, u32 align)
>> -{
>> -       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>> -       struct gtt_range *gt;
>> -       struct resource *r = dev_priv->gtt_mem;
>> -       int ret;
>> -       unsigned long start, end;
>> -
>> -       if (backed) {
>> -               /* The start of the GTT is the stolen pages */
>> -               start = r->start;
>> -               end = r->start + dev_priv->gtt.stolen_size - 1;
>> -       } else {
>> -               /* The rest we will use for GEM backed objects */
>> -               start = r->start + dev_priv->gtt.stolen_size;
>> -               end = r->end;
>> -       }
>> -
>> -       gt = kzalloc(sizeof(struct gtt_range), GFP_KERNEL);
>> -       if (gt == NULL)
>> -               return NULL;
>> -       gt->resource.name = name;
>> -       gt->stolen = backed;
>> -       gt->in_gart = backed;
>> -       /* Ensure this is set for non GEM objects */
>> -       gt->gem.dev = dev;
>> -       ret = allocate_resource(dev_priv->gtt_mem, &gt->resource,
>> -                               len, start, end, align, NULL, NULL);
>> -       if (ret == 0) {
>> -               gt->offset = gt->resource.start - r->start;
>> -               return gt;
>> -       }
>> -       kfree(gt);
>> -       return NULL;
>> -}
>> -
>>   struct gtt_range *
>>   psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, u32 align)
>>   {
>> +       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>>          struct gtt_range *gt;
>>          struct drm_gem_object *obj;
>>          int ret;
>>
>>          size = roundup(size, PAGE_SIZE);
>>
>> -       gt = psb_gtt_alloc_range(dev, size, name, stolen, align);
>> -       if (!gt) {
>> -               dev_err(dev->dev, "no memory for %lld byte GEM object\n", size);
>> -               return ERR_PTR(-ENOSPC);
>> -       }
>> +       gt = kzalloc(sizeof(*gt), GFP_KERNEL);
>> +       if (!gt)
>> +               return ERR_PTR(-ENOMEM);
>>          obj = &gt->gem;
>>
>> +       /* GTT resource */
>> +
>> +       ret = psb_gtt_allocate_resource(dev_priv, &gt->resource, name, size, align, stolen,
>> +                                       &gt->offset);
>> +       if (ret)
>> +               goto err_kfree;
>> +
>> +       if (stolen) {
>> +               gt->stolen = true;
>> +               gt->in_gart = 1;
>> +       }
>> +
>> +       /* GEM object */
>> +
>>          obj->funcs = &psb_gem_object_funcs;
>>
>>          if (stolen) {
>> @@ -184,7 +152,7 @@ psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen,
>>          } else {
>>                  ret = drm_gem_object_init(dev, obj, size);
>>                  if (ret)
>> -                       goto err_psb_gtt_free_range;
>> +                       goto err_release_resource;
>>
>>                  /* Limit the object to 32-bit mappings */
>>                  mapping_set_gfp_mask(obj->filp->f_mapping, GFP_KERNEL | __GFP_DMA32);
>> @@ -192,8 +160,10 @@ psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen,
>>
>>          return gt;
>>
>> -err_psb_gtt_free_range:
>> -       psb_gtt_free_range(dev, gt);
>> +err_release_resource:
>> +       release_resource(&gt->resource);
>> +err_kfree:
>> +       kfree(gt);
>>          return ERR_PTR(ret);
>>   }
>>
>> diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
>> index 0aacf7122e32..5d940fdbe6b8 100644
>> --- a/drivers/gpu/drm/gma500/gtt.c
>> +++ b/drivers/gpu/drm/gma500/gtt.c
>> @@ -18,6 +18,33 @@
>>    *     GTT resource allocator - manage page mappings in GTT space
>>    */
>>
>> +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[static 1])
> 
> Why [static 1]?

[static N] a notation that tells the C compiler that there are at least 
N elements in the array. Passing an array with less or NULL results in 
an error (with clang at least). IMHO we shoudld use it everywhere, but 
then we'd get complains on coding style, I guess.

Best regards
Thomas

> 
> 
>> +{
>> +       struct resource *root = pdev->gtt_mem;
>> +       resource_size_t start, end;
>> +       int ret;
>> +
>> +       if (stolen) {
>> +               /* The start of the GTT is backed by stolen pages. */
>> +               start = root->start;
>> +               end = root->start + pdev->gtt.stolen_size - 1;
>> +       } else {
>> +               /* The rest is backed by system pages. */
>> +               start = root->start + pdev->gtt.stolen_size;
>> +               end = root->end;
>> +       }
>> +
>> +       res->name = name;
>> +       ret = allocate_resource(root, res, size, start, end, align, NULL, NULL);
>> +       if (ret)
>> +               return ret;
>> +       *offset = res->start - root->start;
>> +
>> +       return 0;
>> +}
>> +
>>   /**
>>    *     psb_gtt_mask_pte        -       generate GTT pte entry
>>    *     @pfn: page number to encode
>> diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
>> index 36162b545570..459a03141e8b 100644
>> --- a/drivers/gpu/drm/gma500/gtt.h
>> +++ b/drivers/gpu/drm/gma500/gtt.h
>> @@ -10,6 +10,8 @@
>>
>>   #include <drm/drm_gem.h>
>>
>> +struct drm_psb_private;
>> +
>>   /* This wants cleaning up with respect to the psb_dev and un-needed stuff */
>>   struct psb_gtt {
>>          uint32_t gatt_start;
>> @@ -43,6 +45,10 @@ struct gtt_range {
>>
>>   extern int psb_gtt_restore(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,
>> +                             bool stolen, u32 offset[static 1]);
>> +
>>   int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume);
>>   void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r);
>>
>> --
>> 2.33.0
>>

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

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

[PATCH 08/10] drm/gma500: Set page-caching flags in GEM pin/unpin

[Thomas Zimmermann]

Caching of the GEM object's backing pages are unrelated to GTT
management. Move the respective calls from GTT code to GEM code.

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

diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index 46209e10dcc2..a88d51a3aa2a 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -13,6 +13,8 @@
 
 #include <linux/pagemap.h>
 
+#include <asm/set_memory.h>
+
 #include <drm/drm.h>
 #include <drm/drm_vma_manager.h>
 
@@ -39,7 +41,9 @@ int psb_gem_pin(struct gtt_range *gt)
 
 	npages = gt->gem.size / PAGE_SIZE;
 
-	ret = psb_gtt_insert(dev, gt, 0);
+	set_pages_array_wc(pages, npages);
+
+	ret = psb_gtt_insert(dev, gt);
 	if (ret)
 		goto err_drm_gem_put_pages;
 
@@ -80,6 +84,9 @@ void psb_gem_unpin(struct gtt_range *gt)
 				     (gpu_base + gt->offset), gt->npage, 0, 0);
 	psb_gtt_remove(dev, gt);
 
+	/* Reset caching flags */
+	set_pages_array_wb(gt->pages, gt->npage);
+
 	drm_gem_put_pages(&gt->gem, gt->pages, true, false);
 	gt->pages = NULL;
 
diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
index 5d940fdbe6b8..244de618e612 100644
--- a/drivers/gpu/drm/gma500/gtt.c
+++ b/drivers/gpu/drm/gma500/gtt.c
@@ -7,10 +7,6 @@
  *	    Alan Cox <alan@linux.intel.com>
  */
 
-#include <linux/shmem_fs.h>
-
-#include <asm/set_memory.h>
-
 #include "psb_drv.h"
 
 
@@ -92,17 +88,15 @@ static u32 __iomem *psb_gtt_entry(struct drm_device *dev, struct gtt_range *r)
  *	psb_gtt_insert	-	put an object into the GTT
  *	@dev: our DRM device
  *	@r: our GTT range
- *	@resume: on resume
  *
  *	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.
  */
-int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume)
+int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r)
 {
 	u32 __iomem *gtt_slot;
 	u32 pte;
-	struct page **pages;
 	int i;
 
 	if (r->pages == NULL) {
@@ -113,12 +107,6 @@ int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume)
 	WARN_ON(r->stolen);	/* refcount these maybe ? */
 
 	gtt_slot = psb_gtt_entry(dev, r);
-	pages = r->pages;
-
-	if (!resume) {
-		/* Make sure changes are visible to the GPU */
-		set_pages_array_wc(pages, r->npage);
-	}
 
 	/* Write our page entries into the GTT itself */
 	for (i = 0; i < r->npage; i++) {
@@ -158,7 +146,6 @@ void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r)
 	for (i = 0; i < r->npage; i++)
 		iowrite32(pte, gtt_slot++);
 	ioread32(gtt_slot - 1);
-	set_pages_array_wb(r->pages, r->npage);
 }
 
 static void psb_gtt_alloc(struct drm_device *dev)
@@ -349,7 +336,7 @@ int psb_gtt_restore(struct drm_device *dev)
 	while (r != NULL) {
 		range = container_of(r, struct gtt_range, resource);
 		if (range->pages) {
-			psb_gtt_insert(dev, range, 1);
+			psb_gtt_insert(dev, range);
 			size += range->resource.end - range->resource.start;
 			restored++;
 		}
diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
index 459a03141e8b..7af71617e0c5 100644
--- a/drivers/gpu/drm/gma500/gtt.h
+++ b/drivers/gpu/drm/gma500/gtt.h
@@ -49,7 +49,7 @@ 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[static 1]);
 
-int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume);
+int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r);
 void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r);
 
 #endif
-- 
2.33.0

Re: [PATCH 08/10] drm/gma500: Set page-caching flags in GEM pin/unpin

[Patrik Jakobsson]

On Tue, Sep 28, 2021 at 10:44 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Caching of the GEM object's backing pages are unrelated to GTT
> management. Move the respective calls from GTT code to GEM code.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/gma500/gem.c |  9 ++++++++-
>  drivers/gpu/drm/gma500/gtt.c | 17 ++---------------
>  drivers/gpu/drm/gma500/gtt.h |  2 +-
>  3 files changed, 11 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
> index 46209e10dcc2..a88d51a3aa2a 100644
> --- a/drivers/gpu/drm/gma500/gem.c
> +++ b/drivers/gpu/drm/gma500/gem.c
> @@ -13,6 +13,8 @@
>
>  #include <linux/pagemap.h>
>
> +#include <asm/set_memory.h>
> +
>  #include <drm/drm.h>
>  #include <drm/drm_vma_manager.h>
>
> @@ -39,7 +41,9 @@ int psb_gem_pin(struct gtt_range *gt)
>
>         npages = gt->gem.size / PAGE_SIZE;
>
> -       ret = psb_gtt_insert(dev, gt, 0);
> +       set_pages_array_wc(pages, npages);
> +
> +       ret = psb_gtt_insert(dev, gt);
>         if (ret)
>                 goto err_drm_gem_put_pages;
>
> @@ -80,6 +84,9 @@ void psb_gem_unpin(struct gtt_range *gt)
>                                      (gpu_base + gt->offset), gt->npage, 0, 0);
>         psb_gtt_remove(dev, gt);
>
> +       /* Reset caching flags */
> +       set_pages_array_wb(gt->pages, gt->npage);
> +
>         drm_gem_put_pages(&gt->gem, gt->pages, true, false);
>         gt->pages = NULL;
>
> diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
> index 5d940fdbe6b8..244de618e612 100644
> --- a/drivers/gpu/drm/gma500/gtt.c
> +++ b/drivers/gpu/drm/gma500/gtt.c
> @@ -7,10 +7,6 @@
>   *         Alan Cox <alan@linux.intel.com>
>   */
>
> -#include <linux/shmem_fs.h>
> -
> -#include <asm/set_memory.h>
> -
>  #include "psb_drv.h"
>
>
> @@ -92,17 +88,15 @@ static u32 __iomem *psb_gtt_entry(struct drm_device *dev, struct gtt_range *r)
>   *     psb_gtt_insert  -       put an object into the GTT
>   *     @dev: our DRM device
>   *     @r: our GTT range
> - *     @resume: on resume
>   *
>   *     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.
>   */
> -int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume)
> +int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r)
>  {
>         u32 __iomem *gtt_slot;
>         u32 pte;
> -       struct page **pages;
>         int i;
>
>         if (r->pages == NULL) {
> @@ -113,12 +107,6 @@ int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume)
>         WARN_ON(r->stolen);     /* refcount these maybe ? */
>
>         gtt_slot = psb_gtt_entry(dev, r);
> -       pages = r->pages;
> -
> -       if (!resume) {
> -               /* Make sure changes are visible to the GPU */
> -               set_pages_array_wc(pages, r->npage);
> -       }

I don't quite remember why we have this but I suspect something bad
happened when setting WC on a page with WC already set. Did you try
hibernation?

>
>         /* Write our page entries into the GTT itself */
>         for (i = 0; i < r->npage; i++) {
> @@ -158,7 +146,6 @@ void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r)
>         for (i = 0; i < r->npage; i++)
>                 iowrite32(pte, gtt_slot++);
>         ioread32(gtt_slot - 1);
> -       set_pages_array_wb(r->pages, r->npage);
>  }
>
>  static void psb_gtt_alloc(struct drm_device *dev)
> @@ -349,7 +336,7 @@ int psb_gtt_restore(struct drm_device *dev)
>         while (r != NULL) {
>                 range = container_of(r, struct gtt_range, resource);
>                 if (range->pages) {
> -                       psb_gtt_insert(dev, range, 1);
> +                       psb_gtt_insert(dev, range);
>                         size += range->resource.end - range->resource.start;
>                         restored++;
>                 }
> diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
> index 459a03141e8b..7af71617e0c5 100644
> --- a/drivers/gpu/drm/gma500/gtt.h
> +++ b/drivers/gpu/drm/gma500/gtt.h
> @@ -49,7 +49,7 @@ 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[static 1]);
>
> -int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume);
> +int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r);
>  void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r);
>
>  #endif
> --
> 2.33.0
>

Re: [PATCH 08/10] drm/gma500: Set page-caching flags in GEM pin/unpin

[Thomas Zimmermann]

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

Hi

Am 03.10.21 um 00:15 schrieb Patrik Jakobsson:
> On Tue, Sep 28, 2021 at 10:44 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Caching of the GEM object's backing pages are unrelated to GTT
>> management. Move the respective calls from GTT code to GEM code.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/gma500/gem.c |  9 ++++++++-
>>   drivers/gpu/drm/gma500/gtt.c | 17 ++---------------
>>   drivers/gpu/drm/gma500/gtt.h |  2 +-
>>   3 files changed, 11 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
>> index 46209e10dcc2..a88d51a3aa2a 100644
>> --- a/drivers/gpu/drm/gma500/gem.c
>> +++ b/drivers/gpu/drm/gma500/gem.c
>> @@ -13,6 +13,8 @@
>>
>>   #include <linux/pagemap.h>
>>
>> +#include <asm/set_memory.h>
>> +
>>   #include <drm/drm.h>
>>   #include <drm/drm_vma_manager.h>
>>
>> @@ -39,7 +41,9 @@ int psb_gem_pin(struct gtt_range *gt)
>>
>>          npages = gt->gem.size / PAGE_SIZE;
>>
>> -       ret = psb_gtt_insert(dev, gt, 0);
>> +       set_pages_array_wc(pages, npages);
>> +
>> +       ret = psb_gtt_insert(dev, gt);
>>          if (ret)
>>                  goto err_drm_gem_put_pages;
>>
>> @@ -80,6 +84,9 @@ void psb_gem_unpin(struct gtt_range *gt)
>>                                       (gpu_base + gt->offset), gt->npage, 0, 0);
>>          psb_gtt_remove(dev, gt);
>>
>> +       /* Reset caching flags */
>> +       set_pages_array_wb(gt->pages, gt->npage);
>> +
>>          drm_gem_put_pages(&gt->gem, gt->pages, true, false);
>>          gt->pages = NULL;
>>
>> diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
>> index 5d940fdbe6b8..244de618e612 100644
>> --- a/drivers/gpu/drm/gma500/gtt.c
>> +++ b/drivers/gpu/drm/gma500/gtt.c
>> @@ -7,10 +7,6 @@
>>    *         Alan Cox <alan@linux.intel.com>
>>    */
>>
>> -#include <linux/shmem_fs.h>
>> -
>> -#include <asm/set_memory.h>
>> -
>>   #include "psb_drv.h"
>>
>>
>> @@ -92,17 +88,15 @@ static u32 __iomem *psb_gtt_entry(struct drm_device *dev, struct gtt_range *r)
>>    *     psb_gtt_insert  -       put an object into the GTT
>>    *     @dev: our DRM device
>>    *     @r: our GTT range
>> - *     @resume: on resume
>>    *
>>    *     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.
>>    */
>> -int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume)
>> +int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r)
>>   {
>>          u32 __iomem *gtt_slot;
>>          u32 pte;
>> -       struct page **pages;
>>          int i;
>>
>>          if (r->pages == NULL) {
>> @@ -113,12 +107,6 @@ int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume)
>>          WARN_ON(r->stolen);     /* refcount these maybe ? */
>>
>>          gtt_slot = psb_gtt_entry(dev, r);
>> -       pages = r->pages;
>> -
>> -       if (!resume) {
>> -               /* Make sure changes are visible to the GPU */
>> -               set_pages_array_wc(pages, r->npage);
>> -       }
> 
> I don't quite remember why we have this but I suspect something bad
> happened when setting WC on a page with WC already set. Did you try
> hibernation?

I took the code as-is. I guess that reading back BO memory with read 
caching enabled didn't work well when fbdev acceleration was still around.

Best regards
Thomas

> 
>>
>>          /* Write our page entries into the GTT itself */
>>          for (i = 0; i < r->npage; i++) {
>> @@ -158,7 +146,6 @@ void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r)
>>          for (i = 0; i < r->npage; i++)
>>                  iowrite32(pte, gtt_slot++);
>>          ioread32(gtt_slot - 1);
>> -       set_pages_array_wb(r->pages, r->npage);
>>   }
>>
>>   static void psb_gtt_alloc(struct drm_device *dev)
>> @@ -349,7 +336,7 @@ int psb_gtt_restore(struct drm_device *dev)
>>          while (r != NULL) {
>>                  range = container_of(r, struct gtt_range, resource);
>>                  if (range->pages) {
>> -                       psb_gtt_insert(dev, range, 1);
>> +                       psb_gtt_insert(dev, range);
>>                          size += range->resource.end - range->resource.start;
>>                          restored++;
>>                  }
>> diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
>> index 459a03141e8b..7af71617e0c5 100644
>> --- a/drivers/gpu/drm/gma500/gtt.h
>> +++ b/drivers/gpu/drm/gma500/gtt.h
>> @@ -49,7 +49,7 @@ 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[static 1]);
>>
>> -int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume);
>> +int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r);
>>   void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r);
>>
>>   #endif
>> --
>> 2.33.0
>>

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

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

[PATCH 09/10] drm/gma500: Rewrite GTT page insert/remove without struct gtt_range

[Thomas Zimmermann]

struct gtt_range represents a GEM object and should not be used for GTT
setup. Change psb_gtt_insert() and psb_gtt_remove() to receive all
necessary parameters from their caller. This also eliminates possible
failure from psb_gtt_insert().

There's one exception in psb_gtt_restore(), which requires an upcast
from struct resource to struct gtt_range when restoring the GTT after
hibernation. A possible solution would track the GEM objects that need
restoration separately from the GTT resource.

Rename the functions to psb_gtt_insert_pages() and psb_gtt_remove_pages()
to reflect their similarity to MMU interfaces.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/gma500/gem.c | 13 ++----
 drivers/gpu/drm/gma500/gtt.c | 87 ++++++++++++------------------------
 drivers/gpu/drm/gma500/gtt.h |  5 ++-
 3 files changed, 35 insertions(+), 70 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index a88d51a3aa2a..fd556ba2c044 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -23,7 +23,6 @@
 
 int psb_gem_pin(struct gtt_range *gt)
 {
-	int ret = 0;
 	struct drm_device *dev = gt->gem.dev;
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
 	u32 gpu_base = dev_priv->gtt.gatt_start;
@@ -43,10 +42,7 @@ int psb_gem_pin(struct gtt_range *gt)
 
 	set_pages_array_wc(pages, npages);
 
-	ret = psb_gtt_insert(dev, gt);
-	if (ret)
-		goto err_drm_gem_put_pages;
-
+	psb_gtt_insert_pages(dev_priv, &gt->resource, pages);
 	psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu), pages,
 			     (gpu_base + gt->offset), npages, 0, 0,
 			     PSB_MMU_CACHED_MEMORY);
@@ -59,10 +55,6 @@ int psb_gem_pin(struct gtt_range *gt)
 	mutex_unlock(&dev_priv->gtt_mutex);
 
 	return 0;
-
-err_drm_gem_put_pages:
-	drm_gem_put_pages(&gt->gem, pages, true, false);
-	return ret;
 }
 
 void psb_gem_unpin(struct gtt_range *gt)
@@ -82,13 +74,14 @@ void psb_gem_unpin(struct gtt_range *gt)
 
 	psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu),
 				     (gpu_base + gt->offset), gt->npage, 0, 0);
-	psb_gtt_remove(dev, gt);
+	psb_gtt_remove_pages(dev_priv, &gt->resource);
 
 	/* Reset caching flags */
 	set_pages_array_wb(gt->pages, gt->npage);
 
 	drm_gem_put_pages(&gt->gem, gt->pages, true, false);
 	gt->pages = NULL;
+	gt->npage = 0;
 
 out:
 	mutex_unlock(&dev_priv->gtt_mutex);
diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
index 244de618e612..cf71a2396c16 100644
--- a/drivers/gpu/drm/gma500/gtt.c
+++ b/drivers/gpu/drm/gma500/gtt.c
@@ -66,85 +66,51 @@ static inline uint32_t psb_gtt_mask_pte(uint32_t pfn, int type)
 	return (pfn << PAGE_SHIFT) | mask;
 }
 
-/**
- *	psb_gtt_entry		-	find the GTT entries for a gtt_range
- *	@dev: our DRM device
- *	@r: our GTT range
- *
- *	Given a gtt_range object return the GTT offset of the page table
- *	entries for this gtt_range
- */
-static u32 __iomem *psb_gtt_entry(struct drm_device *dev, struct gtt_range *r)
+static u32 __iomem *psb_gtt_entry(struct drm_psb_private *pdev, const struct resource *res)
 {
-	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
-	unsigned long offset;
-
-	offset = r->resource.start - dev_priv->gtt_mem->start;
+	unsigned long offset = res->start - pdev->gtt_mem->start;
 
-	return dev_priv->gtt_map + (offset >> PAGE_SHIFT);
+	return pdev->gtt_map + (offset >> PAGE_SHIFT);
 }
 
-/**
- *	psb_gtt_insert	-	put an object into the GTT
- *	@dev: our DRM device
- *	@r: our GTT range
- *
- *	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.
- */
-int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r)
+void psb_gtt_insert_pages(struct drm_psb_private *pdev, const struct resource *res,
+			  struct page **pages)
 {
+	resource_size_t npages, i;
 	u32 __iomem *gtt_slot;
 	u32 pte;
-	int i;
 
-	if (r->pages == NULL) {
-		WARN_ON(1);
-		return -EINVAL;
-	}
-
-	WARN_ON(r->stolen);	/* refcount these maybe ? */
+	/* Write our page entries into the GTT itself */
 
-	gtt_slot = psb_gtt_entry(dev, r);
+	npages = resource_size(res) >> PAGE_SHIFT;
+	gtt_slot = psb_gtt_entry(pdev, res);
 
-	/* Write our page entries into the GTT itself */
-	for (i = 0; i < r->npage; i++) {
-		pte = psb_gtt_mask_pte(page_to_pfn(r->pages[i]),
-				       PSB_MMU_CACHED_MEMORY);
-		iowrite32(pte, gtt_slot++);
+	for (i = 0; i < npages; ++i, ++gtt_slot) {
+		pte = psb_gtt_mask_pte(page_to_pfn(pages[i]), PSB_MMU_CACHED_MEMORY);
+		iowrite32(pte, gtt_slot);
 	}
 
 	/* Make sure all the entries are set before we return */
 	ioread32(gtt_slot - 1);
-
-	return 0;
 }
 
-/**
- *	psb_gtt_remove	-	remove an object from the GTT
- *	@dev: our DRM device
- *	@r: our GTT range
- *
- *	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.
- */
-void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r)
+void psb_gtt_remove_pages(struct drm_psb_private *pdev, const struct resource *res)
 {
-	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
+	resource_size_t npages, i;
 	u32 __iomem *gtt_slot;
 	u32 pte;
-	int i;
 
-	WARN_ON(r->stolen);
+	/* Install scratch page for the resource */
+
+	pte = psb_gtt_mask_pte(page_to_pfn(pdev->scratch_page), PSB_MMU_CACHED_MEMORY);
 
-	gtt_slot = psb_gtt_entry(dev, r);
-	pte = psb_gtt_mask_pte(page_to_pfn(dev_priv->scratch_page),
-			       PSB_MMU_CACHED_MEMORY);
+	npages = resource_size(res) >> PAGE_SHIFT;
+	gtt_slot = psb_gtt_entry(pdev, res);
 
-	for (i = 0; i < r->npage; i++)
-		iowrite32(pte, gtt_slot++);
+	for (i = 0; i < npages; ++i, ++gtt_slot)
+		iowrite32(pte, gtt_slot);
+
+	/* Make sure all the entries are set before we return */
 	ioread32(gtt_slot - 1);
 }
 
@@ -334,9 +300,14 @@ int psb_gtt_restore(struct drm_device *dev)
 	psb_gtt_init(dev, 1);
 
 	while (r != NULL) {
+		/*
+		 * TODO: GTT restoration needs a refactoring, so that we don't have to touch
+		 *       struct gtt_range here. The type represents a GEM object and is not
+		 *       related to the GTT itself.
+		 */
 		range = container_of(r, struct gtt_range, resource);
 		if (range->pages) {
-			psb_gtt_insert(dev, range);
+			psb_gtt_insert_pages(dev_priv, &range->resource, range->pages);
 			size += range->resource.end - range->resource.start;
 			restored++;
 		}
diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
index 7af71617e0c5..6a28e24a18b7 100644
--- a/drivers/gpu/drm/gma500/gtt.h
+++ b/drivers/gpu/drm/gma500/gtt.h
@@ -49,7 +49,8 @@ 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[static 1]);
 
-int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r);
-void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r);
+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);
 
 #endif
-- 
2.33.0

Re: [PATCH 09/10] drm/gma500: Rewrite GTT page insert/remove without struct gtt_range

[Patrik Jakobsson]

On Tue, Sep 28, 2021 at 10:44 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> struct gtt_range represents a GEM object and should not be used for GTT
> setup. Change psb_gtt_insert() and psb_gtt_remove() to receive all
> necessary parameters from their caller. This also eliminates possible
> failure from psb_gtt_insert().
>
> There's one exception in psb_gtt_restore(), which requires an upcast
> from struct resource to struct gtt_range when restoring the GTT after
> hibernation. A possible solution would track the GEM objects that need
> restoration separately from the GTT resource.

We could also treat the GTT as registers and save/restore it that way.
OFC that approach would waste a bit of memory.


>
> Rename the functions to psb_gtt_insert_pages() and psb_gtt_remove_pages()
> to reflect their similarity to MMU interfaces.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/gma500/gem.c | 13 ++----
>  drivers/gpu/drm/gma500/gtt.c | 87 ++++++++++++------------------------
>  drivers/gpu/drm/gma500/gtt.h |  5 ++-
>  3 files changed, 35 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
> index a88d51a3aa2a..fd556ba2c044 100644
> --- a/drivers/gpu/drm/gma500/gem.c
> +++ b/drivers/gpu/drm/gma500/gem.c
> @@ -23,7 +23,6 @@
>
>  int psb_gem_pin(struct gtt_range *gt)
>  {
> -       int ret = 0;
>         struct drm_device *dev = gt->gem.dev;
>         struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>         u32 gpu_base = dev_priv->gtt.gatt_start;
> @@ -43,10 +42,7 @@ int psb_gem_pin(struct gtt_range *gt)
>
>         set_pages_array_wc(pages, npages);
>
> -       ret = psb_gtt_insert(dev, gt);
> -       if (ret)
> -               goto err_drm_gem_put_pages;
> -
> +       psb_gtt_insert_pages(dev_priv, &gt->resource, pages);
>         psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu), pages,
>                              (gpu_base + gt->offset), npages, 0, 0,
>                              PSB_MMU_CACHED_MEMORY);
> @@ -59,10 +55,6 @@ int psb_gem_pin(struct gtt_range *gt)
>         mutex_unlock(&dev_priv->gtt_mutex);
>
>         return 0;
> -
> -err_drm_gem_put_pages:
> -       drm_gem_put_pages(&gt->gem, pages, true, false);
> -       return ret;
>  }
>
>  void psb_gem_unpin(struct gtt_range *gt)
> @@ -82,13 +74,14 @@ void psb_gem_unpin(struct gtt_range *gt)
>
>         psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu),
>                                      (gpu_base + gt->offset), gt->npage, 0, 0);
> -       psb_gtt_remove(dev, gt);
> +       psb_gtt_remove_pages(dev_priv, &gt->resource);
>
>         /* Reset caching flags */
>         set_pages_array_wb(gt->pages, gt->npage);
>
>         drm_gem_put_pages(&gt->gem, gt->pages, true, false);
>         gt->pages = NULL;
> +       gt->npage = 0;
>
>  out:
>         mutex_unlock(&dev_priv->gtt_mutex);
> diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
> index 244de618e612..cf71a2396c16 100644
> --- a/drivers/gpu/drm/gma500/gtt.c
> +++ b/drivers/gpu/drm/gma500/gtt.c
> @@ -66,85 +66,51 @@ static inline uint32_t psb_gtt_mask_pte(uint32_t pfn, int type)
>         return (pfn << PAGE_SHIFT) | mask;
>  }
>
> -/**
> - *     psb_gtt_entry           -       find the GTT entries for a gtt_range
> - *     @dev: our DRM device
> - *     @r: our GTT range
> - *
> - *     Given a gtt_range object return the GTT offset of the page table
> - *     entries for this gtt_range
> - */
> -static u32 __iomem *psb_gtt_entry(struct drm_device *dev, struct gtt_range *r)
> +static u32 __iomem *psb_gtt_entry(struct drm_psb_private *pdev, const struct resource *res)
>  {
> -       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> -       unsigned long offset;
> -
> -       offset = r->resource.start - dev_priv->gtt_mem->start;
> +       unsigned long offset = res->start - pdev->gtt_mem->start;
>
> -       return dev_priv->gtt_map + (offset >> PAGE_SHIFT);
> +       return pdev->gtt_map + (offset >> PAGE_SHIFT);
>  }
>
> -/**
> - *     psb_gtt_insert  -       put an object into the GTT
> - *     @dev: our DRM device
> - *     @r: our GTT range
> - *
> - *     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.
> - */
> -int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r)
> +void psb_gtt_insert_pages(struct drm_psb_private *pdev, const struct resource *res,
> +                         struct page **pages)
>  {
> +       resource_size_t npages, i;
>         u32 __iomem *gtt_slot;
>         u32 pte;
> -       int i;
>
> -       if (r->pages == NULL) {
> -               WARN_ON(1);
> -               return -EINVAL;
> -       }
> -
> -       WARN_ON(r->stolen);     /* refcount these maybe ? */
> +       /* Write our page entries into the GTT itself */
>
> -       gtt_slot = psb_gtt_entry(dev, r);
> +       npages = resource_size(res) >> PAGE_SHIFT;
> +       gtt_slot = psb_gtt_entry(pdev, res);
>
> -       /* Write our page entries into the GTT itself */
> -       for (i = 0; i < r->npage; i++) {
> -               pte = psb_gtt_mask_pte(page_to_pfn(r->pages[i]),
> -                                      PSB_MMU_CACHED_MEMORY);
> -               iowrite32(pte, gtt_slot++);
> +       for (i = 0; i < npages; ++i, ++gtt_slot) {
> +               pte = psb_gtt_mask_pte(page_to_pfn(pages[i]), PSB_MMU_CACHED_MEMORY);
> +               iowrite32(pte, gtt_slot);
>         }
>
>         /* Make sure all the entries are set before we return */
>         ioread32(gtt_slot - 1);
> -
> -       return 0;
>  }
>
> -/**
> - *     psb_gtt_remove  -       remove an object from the GTT
> - *     @dev: our DRM device
> - *     @r: our GTT range
> - *
> - *     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.
> - */
> -void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r)
> +void psb_gtt_remove_pages(struct drm_psb_private *pdev, const struct resource *res)
>  {
> -       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> +       resource_size_t npages, i;
>         u32 __iomem *gtt_slot;
>         u32 pte;
> -       int i;
>
> -       WARN_ON(r->stolen);
> +       /* Install scratch page for the resource */
> +
> +       pte = psb_gtt_mask_pte(page_to_pfn(pdev->scratch_page), PSB_MMU_CACHED_MEMORY);
>
> -       gtt_slot = psb_gtt_entry(dev, r);
> -       pte = psb_gtt_mask_pte(page_to_pfn(dev_priv->scratch_page),
> -                              PSB_MMU_CACHED_MEMORY);
> +       npages = resource_size(res) >> PAGE_SHIFT;
> +       gtt_slot = psb_gtt_entry(pdev, res);
>
> -       for (i = 0; i < r->npage; i++)
> -               iowrite32(pte, gtt_slot++);
> +       for (i = 0; i < npages; ++i, ++gtt_slot)
> +               iowrite32(pte, gtt_slot);
> +
> +       /* Make sure all the entries are set before we return */
>         ioread32(gtt_slot - 1);
>  }
>
> @@ -334,9 +300,14 @@ int psb_gtt_restore(struct drm_device *dev)
>         psb_gtt_init(dev, 1);
>
>         while (r != NULL) {
> +               /*
> +                * TODO: GTT restoration needs a refactoring, so that we don't have to touch
> +                *       struct gtt_range here. The type represents a GEM object and is not
> +                *       related to the GTT itself.
> +                */
>                 range = container_of(r, struct gtt_range, resource);
>                 if (range->pages) {
> -                       psb_gtt_insert(dev, range);
> +                       psb_gtt_insert_pages(dev_priv, &range->resource, range->pages);
>                         size += range->resource.end - range->resource.start;
>                         restored++;
>                 }
> diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
> index 7af71617e0c5..6a28e24a18b7 100644
> --- a/drivers/gpu/drm/gma500/gtt.h
> +++ b/drivers/gpu/drm/gma500/gtt.h
> @@ -49,7 +49,8 @@ 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[static 1]);
>
> -int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r);
> -void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r);
> +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);
>
>  #endif
> --
> 2.33.0
>

Re: [PATCH 09/10] drm/gma500: Rewrite GTT page insert/remove without struct gtt_range

[Thomas Zimmermann]

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

Hi

Am 03.10.21 um 00:15 schrieb Patrik Jakobsson:
> On Tue, Sep 28, 2021 at 10:44 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> struct gtt_range represents a GEM object and should not be used for GTT
>> setup. Change psb_gtt_insert() and psb_gtt_remove() to receive all
>> necessary parameters from their caller. This also eliminates possible
>> failure from psb_gtt_insert().
>>
>> There's one exception in psb_gtt_restore(), which requires an upcast
>> from struct resource to struct gtt_range when restoring the GTT after
>> hibernation. A possible solution would track the GEM objects that need
>> restoration separately from the GTT resource.
> 
> We could also treat the GTT as registers and save/restore it that way.
> OFC that approach would waste a bit of memory.

For me, the next step would be to split-off the GEM code from the 
remaining GTT code. That would be an opportunity to improve this. I 
think the current code with its call to pin might be broken anyway and 
messes up the reference counting during resume.

If the driver ever switches to atomic modesetting, we could use the 
suspend helpers and have the mode reprogrammed during resume. That 
should resolve the whole issue.

Best regards
Thomas

> 
> 
>>
>> Rename the functions to psb_gtt_insert_pages() and psb_gtt_remove_pages()
>> to reflect their similarity to MMU interfaces.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/gma500/gem.c | 13 ++----
>>   drivers/gpu/drm/gma500/gtt.c | 87 ++++++++++++------------------------
>>   drivers/gpu/drm/gma500/gtt.h |  5 ++-
>>   3 files changed, 35 insertions(+), 70 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
>> index a88d51a3aa2a..fd556ba2c044 100644
>> --- a/drivers/gpu/drm/gma500/gem.c
>> +++ b/drivers/gpu/drm/gma500/gem.c
>> @@ -23,7 +23,6 @@
>>
>>   int psb_gem_pin(struct gtt_range *gt)
>>   {
>> -       int ret = 0;
>>          struct drm_device *dev = gt->gem.dev;
>>          struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>>          u32 gpu_base = dev_priv->gtt.gatt_start;
>> @@ -43,10 +42,7 @@ int psb_gem_pin(struct gtt_range *gt)
>>
>>          set_pages_array_wc(pages, npages);
>>
>> -       ret = psb_gtt_insert(dev, gt);
>> -       if (ret)
>> -               goto err_drm_gem_put_pages;
>> -
>> +       psb_gtt_insert_pages(dev_priv, &gt->resource, pages);
>>          psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu), pages,
>>                               (gpu_base + gt->offset), npages, 0, 0,
>>                               PSB_MMU_CACHED_MEMORY);
>> @@ -59,10 +55,6 @@ int psb_gem_pin(struct gtt_range *gt)
>>          mutex_unlock(&dev_priv->gtt_mutex);
>>
>>          return 0;
>> -
>> -err_drm_gem_put_pages:
>> -       drm_gem_put_pages(&gt->gem, pages, true, false);
>> -       return ret;
>>   }
>>
>>   void psb_gem_unpin(struct gtt_range *gt)
>> @@ -82,13 +74,14 @@ void psb_gem_unpin(struct gtt_range *gt)
>>
>>          psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu),
>>                                       (gpu_base + gt->offset), gt->npage, 0, 0);
>> -       psb_gtt_remove(dev, gt);
>> +       psb_gtt_remove_pages(dev_priv, &gt->resource);
>>
>>          /* Reset caching flags */
>>          set_pages_array_wb(gt->pages, gt->npage);
>>
>>          drm_gem_put_pages(&gt->gem, gt->pages, true, false);
>>          gt->pages = NULL;
>> +       gt->npage = 0;
>>
>>   out:
>>          mutex_unlock(&dev_priv->gtt_mutex);
>> diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
>> index 244de618e612..cf71a2396c16 100644
>> --- a/drivers/gpu/drm/gma500/gtt.c
>> +++ b/drivers/gpu/drm/gma500/gtt.c
>> @@ -66,85 +66,51 @@ static inline uint32_t psb_gtt_mask_pte(uint32_t pfn, int type)
>>          return (pfn << PAGE_SHIFT) | mask;
>>   }
>>
>> -/**
>> - *     psb_gtt_entry           -       find the GTT entries for a gtt_range
>> - *     @dev: our DRM device
>> - *     @r: our GTT range
>> - *
>> - *     Given a gtt_range object return the GTT offset of the page table
>> - *     entries for this gtt_range
>> - */
>> -static u32 __iomem *psb_gtt_entry(struct drm_device *dev, struct gtt_range *r)
>> +static u32 __iomem *psb_gtt_entry(struct drm_psb_private *pdev, const struct resource *res)
>>   {
>> -       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>> -       unsigned long offset;
>> -
>> -       offset = r->resource.start - dev_priv->gtt_mem->start;
>> +       unsigned long offset = res->start - pdev->gtt_mem->start;
>>
>> -       return dev_priv->gtt_map + (offset >> PAGE_SHIFT);
>> +       return pdev->gtt_map + (offset >> PAGE_SHIFT);
>>   }
>>
>> -/**
>> - *     psb_gtt_insert  -       put an object into the GTT
>> - *     @dev: our DRM device
>> - *     @r: our GTT range
>> - *
>> - *     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.
>> - */
>> -int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r)
>> +void psb_gtt_insert_pages(struct drm_psb_private *pdev, const struct resource *res,
>> +                         struct page **pages)
>>   {
>> +       resource_size_t npages, i;
>>          u32 __iomem *gtt_slot;
>>          u32 pte;
>> -       int i;
>>
>> -       if (r->pages == NULL) {
>> -               WARN_ON(1);
>> -               return -EINVAL;
>> -       }
>> -
>> -       WARN_ON(r->stolen);     /* refcount these maybe ? */
>> +       /* Write our page entries into the GTT itself */
>>
>> -       gtt_slot = psb_gtt_entry(dev, r);
>> +       npages = resource_size(res) >> PAGE_SHIFT;
>> +       gtt_slot = psb_gtt_entry(pdev, res);
>>
>> -       /* Write our page entries into the GTT itself */
>> -       for (i = 0; i < r->npage; i++) {
>> -               pte = psb_gtt_mask_pte(page_to_pfn(r->pages[i]),
>> -                                      PSB_MMU_CACHED_MEMORY);
>> -               iowrite32(pte, gtt_slot++);
>> +       for (i = 0; i < npages; ++i, ++gtt_slot) {
>> +               pte = psb_gtt_mask_pte(page_to_pfn(pages[i]), PSB_MMU_CACHED_MEMORY);
>> +               iowrite32(pte, gtt_slot);
>>          }
>>
>>          /* Make sure all the entries are set before we return */
>>          ioread32(gtt_slot - 1);
>> -
>> -       return 0;
>>   }
>>
>> -/**
>> - *     psb_gtt_remove  -       remove an object from the GTT
>> - *     @dev: our DRM device
>> - *     @r: our GTT range
>> - *
>> - *     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.
>> - */
>> -void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r)
>> +void psb_gtt_remove_pages(struct drm_psb_private *pdev, const struct resource *res)
>>   {
>> -       struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>> +       resource_size_t npages, i;
>>          u32 __iomem *gtt_slot;
>>          u32 pte;
>> -       int i;
>>
>> -       WARN_ON(r->stolen);
>> +       /* Install scratch page for the resource */
>> +
>> +       pte = psb_gtt_mask_pte(page_to_pfn(pdev->scratch_page), PSB_MMU_CACHED_MEMORY);
>>
>> -       gtt_slot = psb_gtt_entry(dev, r);
>> -       pte = psb_gtt_mask_pte(page_to_pfn(dev_priv->scratch_page),
>> -                              PSB_MMU_CACHED_MEMORY);
>> +       npages = resource_size(res) >> PAGE_SHIFT;
>> +       gtt_slot = psb_gtt_entry(pdev, res);
>>
>> -       for (i = 0; i < r->npage; i++)
>> -               iowrite32(pte, gtt_slot++);
>> +       for (i = 0; i < npages; ++i, ++gtt_slot)
>> +               iowrite32(pte, gtt_slot);
>> +
>> +       /* Make sure all the entries are set before we return */
>>          ioread32(gtt_slot - 1);
>>   }
>>
>> @@ -334,9 +300,14 @@ int psb_gtt_restore(struct drm_device *dev)
>>          psb_gtt_init(dev, 1);
>>
>>          while (r != NULL) {
>> +               /*
>> +                * TODO: GTT restoration needs a refactoring, so that we don't have to touch
>> +                *       struct gtt_range here. The type represents a GEM object and is not
>> +                *       related to the GTT itself.
>> +                */
>>                  range = container_of(r, struct gtt_range, resource);
>>                  if (range->pages) {
>> -                       psb_gtt_insert(dev, range);
>> +                       psb_gtt_insert_pages(dev_priv, &range->resource, range->pages);
>>                          size += range->resource.end - range->resource.start;
>>                          restored++;
>>                  }
>> diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
>> index 7af71617e0c5..6a28e24a18b7 100644
>> --- a/drivers/gpu/drm/gma500/gtt.h
>> +++ b/drivers/gpu/drm/gma500/gtt.h
>> @@ -49,7 +49,8 @@ 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[static 1]);
>>
>> -int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r);
>> -void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r);
>> +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);
>>
>>   #endif
>> --
>> 2.33.0
>>

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

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

[PATCH 10/10] drm/gma500: Rename struct gtt_range to struct psb_gem_object

[Thomas Zimmermann]

struct gtt_range represents a GEM object. Rename the structure to struct
psb_gem_object and update all users. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/gma500/framebuffer.c       |   9 +-
 drivers/gpu/drm/gma500/gem.c               | 106 +++++++++++----------
 drivers/gpu/drm/gma500/gem.h               |  25 ++++-
 drivers/gpu/drm/gma500/gma_display.c       |  50 +++++-----
 drivers/gpu/drm/gma500/gtt.c               |  15 +--
 drivers/gpu/drm/gma500/gtt.h               |  15 ---
 drivers/gpu/drm/gma500/oaktrail_crtc.c     |   3 +-
 drivers/gpu/drm/gma500/psb_intel_display.c |  15 ++-
 drivers/gpu/drm/gma500/psb_intel_drv.h     |   2 +-
 9 files changed, 123 insertions(+), 117 deletions(-)

diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
index 3ea6679ccd38..45df9de22007 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -81,14 +81,13 @@ static vm_fault_t psbfb_vm_fault(struct vm_fault *vmf)
 	struct drm_framebuffer *fb = vma->vm_private_data;
 	struct drm_device *dev = fb->dev;
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
-	struct gtt_range *gtt = to_gtt_range(fb->obj[0]);
+	struct psb_gem_object *pobj = to_psb_gem_object(fb->obj[0]);
 	int page_num;
 	int i;
 	unsigned long address;
 	vm_fault_t ret = VM_FAULT_SIGBUS;
 	unsigned long pfn;
-	unsigned long phys_addr = (unsigned long)dev_priv->stolen_base +
-				  gtt->offset;
+	unsigned long phys_addr = (unsigned long)dev_priv->stolen_base + pobj->offset;
 
 	page_num = vma_pages(vma);
 	address = vmf->address - (vmf->pgoff << PAGE_SHIFT);
@@ -242,7 +241,7 @@ static int psbfb_create(struct drm_fb_helper *fb_helper,
 	struct drm_mode_fb_cmd2 mode_cmd;
 	int size;
 	int ret;
-	struct gtt_range *backing;
+	struct psb_gem_object *backing;
 	struct drm_gem_object *obj;
 	u32 bpp, depth;
 
@@ -264,7 +263,7 @@ static int psbfb_create(struct drm_fb_helper *fb_helper,
 	backing = psb_gem_create(dev, size, "fb", true, PAGE_SIZE);
 	if (IS_ERR(backing))
 		return PTR_ERR(backing);
-	obj = &backing->gem;
+	obj = &backing->base;
 
 	memset(dev_priv->vram_addr + backing->offset, 0, size);
 
diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index fd556ba2c044..9b7052153cab 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -21,9 +21,10 @@
 #include "gem.h"
 #include "psb_drv.h"
 
-int psb_gem_pin(struct gtt_range *gt)
+int psb_gem_pin(struct psb_gem_object *pobj)
 {
-	struct drm_device *dev = gt->gem.dev;
+	struct drm_gem_object *obj = &pobj->base;
+	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;
 	struct page **pages;
@@ -31,57 +32,58 @@ int psb_gem_pin(struct gtt_range *gt)
 
 	mutex_lock(&dev_priv->gtt_mutex);
 
-	if (gt->in_gart || gt->stolen)
+	if (pobj->in_gart || pobj->stolen)
 		goto out; /* already mapped */
 
-	pages = drm_gem_get_pages(&gt->gem);
+	pages = drm_gem_get_pages(obj);
 	if (IS_ERR(pages))
 		return PTR_ERR(pages);
 
-	npages = gt->gem.size / PAGE_SIZE;
+	npages = obj->size / PAGE_SIZE;
 
 	set_pages_array_wc(pages, npages);
 
-	psb_gtt_insert_pages(dev_priv, &gt->resource, pages);
+	psb_gtt_insert_pages(dev_priv, &pobj->resource, pages);
 	psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu), pages,
-			     (gpu_base + gt->offset), npages, 0, 0,
+			     (gpu_base + pobj->offset), npages, 0, 0,
 			     PSB_MMU_CACHED_MEMORY);
 
-	gt->npage = npages;
-	gt->pages = pages;
+	pobj->npage = npages;
+	pobj->pages = pages;
 
 out:
-	++gt->in_gart;
+	++pobj->in_gart;
 	mutex_unlock(&dev_priv->gtt_mutex);
 
 	return 0;
 }
 
-void psb_gem_unpin(struct gtt_range *gt)
+void psb_gem_unpin(struct psb_gem_object *pobj)
 {
-	struct drm_device *dev = gt->gem.dev;
+	struct drm_gem_object *obj = &pobj->base;
+	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;
 
 	mutex_lock(&dev_priv->gtt_mutex);
 
-	WARN_ON(!gt->in_gart);
+	WARN_ON(!pobj->in_gart);
 
-	--gt->in_gart;
+	--pobj->in_gart;
 
-	if (gt->in_gart || gt->stolen)
+	if (pobj->in_gart || pobj->stolen)
 		goto out;
 
 	psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu),
-				     (gpu_base + gt->offset), gt->npage, 0, 0);
-	psb_gtt_remove_pages(dev_priv, &gt->resource);
+			     (gpu_base + pobj->offset), pobj->npage, 0, 0);
+	psb_gtt_remove_pages(dev_priv, &pobj->resource);
 
 	/* Reset caching flags */
-	set_pages_array_wb(gt->pages, gt->npage);
+	set_pages_array_wb(pobj->pages, pobj->npage);
 
-	drm_gem_put_pages(&gt->gem, gt->pages, true, false);
-	gt->pages = NULL;
-	gt->npage = 0;
+	drm_gem_put_pages(obj, pobj->pages, true, false);
+	pobj->pages = NULL;
+	pobj->npage = 0;
 
 out:
 	mutex_unlock(&dev_priv->gtt_mutex);
@@ -91,18 +93,18 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf);
 
 static void psb_gem_free_object(struct drm_gem_object *obj)
 {
-	struct gtt_range *gt = to_gtt_range(obj);
+	struct psb_gem_object *pobj = to_psb_gem_object(obj);
 
 	drm_gem_object_release(obj);
 
 	/* Undo the mmap pin if we are destroying the object */
-	if (gt->mmapping)
-		psb_gem_unpin(gt);
+	if (pobj->mmapping)
+		psb_gem_unpin(pobj);
 
-	WARN_ON(gt->in_gart && !gt->stolen);
+	WARN_ON(pobj->in_gart && !pobj->stolen);
 
-	release_resource(&gt->resource);
-	kfree(gt);
+	release_resource(&pobj->resource);
+	kfree(pobj);
 }
 
 static const struct vm_operations_struct psb_gem_vm_ops = {
@@ -116,31 +118,31 @@ static const struct drm_gem_object_funcs psb_gem_object_funcs = {
 	.vm_ops = &psb_gem_vm_ops,
 };
 
-struct gtt_range *
+struct psb_gem_object *
 psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, u32 align)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
-	struct gtt_range *gt;
+	struct psb_gem_object *pobj;
 	struct drm_gem_object *obj;
 	int ret;
 
 	size = roundup(size, PAGE_SIZE);
 
-	gt = kzalloc(sizeof(*gt), GFP_KERNEL);
-	if (!gt)
+	pobj = kzalloc(sizeof(*pobj), GFP_KERNEL);
+	if (!pobj)
 		return ERR_PTR(-ENOMEM);
-	obj = &gt->gem;
+	obj = &pobj->base;
 
 	/* GTT resource */
 
-	ret = psb_gtt_allocate_resource(dev_priv, &gt->resource, name, size, align, stolen,
-					&gt->offset);
+	ret = psb_gtt_allocate_resource(dev_priv, &pobj->resource, name, size, align, stolen,
+					&pobj->offset);
 	if (ret)
 		goto err_kfree;
 
 	if (stolen) {
-		gt->stolen = true;
-		gt->in_gart = 1;
+		pobj->stolen = true;
+		pobj->in_gart = 1;
 	}
 
 	/* GEM object */
@@ -158,12 +160,12 @@ psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen,
 		mapping_set_gfp_mask(obj->filp->f_mapping, GFP_KERNEL | __GFP_DMA32);
 	}
 
-	return gt;
+	return pobj;
 
 err_release_resource:
-	release_resource(&gt->resource);
+	release_resource(&pobj->resource);
 err_kfree:
-	kfree(gt);
+	kfree(pobj);
 	return ERR_PTR(ret);
 }
 
@@ -181,7 +183,7 @@ int psb_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
 			struct drm_mode_create_dumb *args)
 {
 	size_t pitch, size;
-	struct gtt_range *gt;
+	struct psb_gem_object *pobj;
 	struct drm_gem_object *obj;
 	u32 handle;
 	int ret;
@@ -194,10 +196,10 @@ int psb_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
 	if (!size)
 		return -EINVAL;
 
-	gt = psb_gem_create(dev, size, "gem", false, PAGE_SIZE);
-	if (IS_ERR(gt))
-		return PTR_ERR(gt);
-	obj = &gt->gem;
+	pobj = psb_gem_create(dev, size, "gem", false, PAGE_SIZE);
+	if (IS_ERR(pobj))
+		return PTR_ERR(pobj);
+	obj = &pobj->base;
 
 	ret = drm_gem_handle_create(file, obj, &handle);
 	if (ret)
@@ -236,7 +238,7 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
 	struct drm_gem_object *obj;
-	struct gtt_range *r;
+	struct psb_gem_object *pobj;
 	int err;
 	vm_fault_t ret;
 	unsigned long pfn;
@@ -248,7 +250,7 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf)
 	dev = obj->dev;
 	dev_priv = to_drm_psb_private(dev);
 
-	r = to_gtt_range(obj);
+	pobj = to_psb_gem_object(obj);
 
 	/* Make sure we don't parallel update on a fault, nor move or remove
 	   something from beneath our feet */
@@ -256,14 +258,14 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf)
 
 	/* For now the mmap pins the object and it stays pinned. As things
 	   stand that will do us no harm */
-	if (r->mmapping == 0) {
-		err = psb_gem_pin(r);
+	if (pobj->mmapping == 0) {
+		err = psb_gem_pin(pobj);
 		if (err < 0) {
 			dev_err(dev->dev, "gma500: pin failed: %d\n", err);
 			ret = vmf_error(err);
 			goto fail;
 		}
-		r->mmapping = 1;
+		pobj->mmapping = 1;
 	}
 
 	/* Page relative to the VMA start - we must calculate this ourselves
@@ -271,10 +273,10 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf)
 	page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT;
 
 	/* CPU view of the page, don't go via the GART for CPU writes */
-	if (r->stolen)
-		pfn = (dev_priv->stolen_base + r->offset) >> PAGE_SHIFT;
+	if (pobj->stolen)
+		pfn = (dev_priv->stolen_base + pobj->offset) >> PAGE_SHIFT;
 	else
-		pfn = page_to_pfn(r->pages[page_offset]);
+		pfn = page_to_pfn(pobj->pages[page_offset]);
 	ret = vmf_insert_pfn(vma, vmf->address, pfn);
 fail:
 	mutex_unlock(&dev_priv->mmap_mutex);
diff --git a/drivers/gpu/drm/gma500/gem.h b/drivers/gpu/drm/gma500/gem.h
index 21c86df482a6..79cced40c87f 100644
--- a/drivers/gpu/drm/gma500/gem.h
+++ b/drivers/gpu/drm/gma500/gem.h
@@ -8,14 +8,33 @@
 #ifndef _GEM_H
 #define _GEM_H
 
+#include <linux/kernel.h>
+
 #include <drm/drm_gem.h>
 
 struct drm_device;
 
-struct gtt_range *
+struct psb_gem_object {
+	struct drm_gem_object base;
+
+	struct resource resource;	/* GTT resource for our allocation */
+	u32 offset;			/* GTT offset of our object */
+	int in_gart;			/* Currently in the GART (ref ct) */
+	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)
+{
+	return container_of(obj, struct psb_gem_object, base);
+}
+
+struct psb_gem_object *
 psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, u32 align);
 
-int psb_gem_pin(struct gtt_range *gt);
-void psb_gem_unpin(struct gtt_range *gt);
+int psb_gem_pin(struct psb_gem_object *pobj);
+void psb_gem_unpin(struct psb_gem_object *pobj);
 
 #endif
diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c
index 6d0470b27bc5..99da3118131a 100644
--- a/drivers/gpu/drm/gma500/gma_display.c
+++ b/drivers/gpu/drm/gma500/gma_display.c
@@ -55,7 +55,7 @@ int gma_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
 	struct gma_crtc *gma_crtc = to_gma_crtc(crtc);
 	struct drm_framebuffer *fb = crtc->primary->fb;
-	struct gtt_range *gtt;
+	struct psb_gem_object *pobj;
 	int pipe = gma_crtc->pipe;
 	const struct psb_offset *map = &dev_priv->regmap[pipe];
 	unsigned long start, offset;
@@ -71,14 +71,14 @@ int gma_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 		goto gma_pipe_cleaner;
 	}
 
-	gtt = to_gtt_range(fb->obj[0]);
+	pobj = to_psb_gem_object(fb->obj[0]);
 
 	/* We are displaying this buffer, make sure it is actually loaded
 	   into the GTT */
-	ret = psb_gem_pin(gtt);
+	ret = psb_gem_pin(pobj);
 	if (ret < 0)
 		goto gma_pipe_set_base_exit;
-	start = gtt->offset;
+	start = pobj->offset;
 	offset = y * fb->pitches[0] + x * fb->format->cpp[0];
 
 	REG_WRITE(map->stride, fb->pitches[0]);
@@ -126,7 +126,7 @@ int gma_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 gma_pipe_cleaner:
 	/* If there was a previous display we can now unpin it */
 	if (old_fb)
-		psb_gem_unpin(to_gtt_range(old_fb->obj[0]));
+		psb_gem_unpin(to_psb_gem_object(old_fb->obj[0]));
 
 gma_pipe_set_base_exit:
 	gma_power_end(dev);
@@ -332,8 +332,8 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
 	uint32_t base = (pipe == 0) ? CURABASE : CURBBASE;
 	uint32_t temp;
 	size_t addr = 0;
-	struct gtt_range *gt;
-	struct gtt_range *cursor_gt = gma_crtc->cursor_gt;
+	struct psb_gem_object *pobj;
+	struct psb_gem_object *cursor_pobj = gma_crtc->cursor_pobj;
 	struct drm_gem_object *obj;
 	void *tmp_dst, *tmp_src;
 	int ret = 0, i, cursor_pages;
@@ -349,8 +349,8 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
 
 		/* Unpin the old GEM object */
 		if (gma_crtc->cursor_obj) {
-			gt = to_gtt_range(gma_crtc->cursor_obj);
-			psb_gem_unpin(gt);
+			pobj = to_psb_gem_object(gma_crtc->cursor_obj);
+			psb_gem_unpin(pobj);
 			drm_gem_object_put(gma_crtc->cursor_obj);
 			gma_crtc->cursor_obj = NULL;
 		}
@@ -375,40 +375,40 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
 		goto unref_cursor;
 	}
 
-	gt = to_gtt_range(obj);
+	pobj = to_psb_gem_object(obj);
 
 	/* Pin the memory into the GTT */
-	ret = psb_gem_pin(gt);
+	ret = psb_gem_pin(pobj);
 	if (ret) {
 		dev_err(dev->dev, "Can not pin down handle 0x%x\n", handle);
 		goto unref_cursor;
 	}
 
 	if (dev_priv->ops->cursor_needs_phys) {
-		if (cursor_gt == NULL) {
+		if (!cursor_pobj) {
 			dev_err(dev->dev, "No hardware cursor mem available");
 			ret = -ENOMEM;
 			goto unref_cursor;
 		}
 
 		/* Prevent overflow */
-		if (gt->npage > 4)
+		if (pobj->npage > 4)
 			cursor_pages = 4;
 		else
-			cursor_pages = gt->npage;
+			cursor_pages = pobj->npage;
 
 		/* Copy the cursor to cursor mem */
-		tmp_dst = dev_priv->vram_addr + cursor_gt->offset;
+		tmp_dst = dev_priv->vram_addr + cursor_pobj->offset;
 		for (i = 0; i < cursor_pages; i++) {
-			tmp_src = kmap(gt->pages[i]);
+			tmp_src = kmap(pobj->pages[i]);
 			memcpy(tmp_dst, tmp_src, PAGE_SIZE);
-			kunmap(gt->pages[i]);
+			kunmap(pobj->pages[i]);
 			tmp_dst += PAGE_SIZE;
 		}
 
 		addr = gma_crtc->cursor_addr;
 	} else {
-		addr = gt->offset;
+		addr = pobj->offset;
 		gma_crtc->cursor_addr = addr;
 	}
 
@@ -425,8 +425,8 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
 
 	/* unpin the old bo */
 	if (gma_crtc->cursor_obj) {
-		gt = to_gtt_range(gma_crtc->cursor_obj);
-		psb_gem_unpin(gt);
+		pobj = to_psb_gem_object(gma_crtc->cursor_obj);
+		psb_gem_unpin(pobj);
 		drm_gem_object_put(gma_crtc->cursor_obj);
 	}
 
@@ -483,14 +483,14 @@ void gma_crtc_commit(struct drm_crtc *crtc)
 
 void gma_crtc_disable(struct drm_crtc *crtc)
 {
-	struct gtt_range *gt;
+	struct psb_gem_object *pobj;
 	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
 
 	crtc_funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
 
 	if (crtc->primary->fb) {
-		gt = to_gtt_range(crtc->primary->fb->obj[0]);
-		psb_gem_unpin(gt);
+		pobj = to_psb_gem_object(crtc->primary->fb->obj[0]);
+		psb_gem_unpin(pobj);
 	}
 }
 
@@ -498,8 +498,8 @@ void gma_crtc_destroy(struct drm_crtc *crtc)
 {
 	struct gma_crtc *gma_crtc = to_gma_crtc(crtc);
 
-	if (gma_crtc->cursor_gt)
-		drm_gem_object_put(&gma_crtc->cursor_gt->gem);
+	if (gma_crtc->cursor_pobj)
+		drm_gem_object_put(&gma_crtc->cursor_pobj->base);
 
 	kfree(gma_crtc->crtc_state);
 	drm_crtc_cleanup(crtc);
diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
index cf71a2396c16..75e483c88ceb 100644
--- a/drivers/gpu/drm/gma500/gtt.c
+++ b/drivers/gpu/drm/gma500/gtt.c
@@ -7,6 +7,7 @@
  *	    Alan Cox <alan@linux.intel.com>
  */
 
+#include "gem.h" /* TODO: for struct psb_gem_object, see psb_gtt_restore() */
 #include "psb_drv.h"
 
 
@@ -292,7 +293,7 @@ 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 gtt_range *range;
+	struct psb_gem_object *pobj;
 	unsigned int restored = 0, total = 0, size = 0;
 
 	/* On resume, the gtt_mutex is already initialized */
@@ -302,13 +303,13 @@ int psb_gtt_restore(struct drm_device *dev)
 	while (r != NULL) {
 		/*
 		 * TODO: GTT restoration needs a refactoring, so that we don't have to touch
-		 *       struct gtt_range here. The type represents a GEM object and is not
-		 *       related to the GTT itself.
+		 *       struct psb_gem_object here. The type represents a GEM object and is
+		 *       not related to the GTT itself.
 		 */
-		range = container_of(r, struct gtt_range, resource);
-		if (range->pages) {
-			psb_gtt_insert_pages(dev_priv, &range->resource, range->pages);
-			size += range->resource.end - range->resource.start;
+		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;
diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
index 6a28e24a18b7..29649395c531 100644
--- a/drivers/gpu/drm/gma500/gtt.h
+++ b/drivers/gpu/drm/gma500/gtt.h
@@ -28,21 +28,6 @@ struct psb_gtt {
 /* Exported functions */
 extern int psb_gtt_init(struct drm_device *dev, int resume);
 extern void psb_gtt_takedown(struct drm_device *dev);
-
-/* Each gtt_range describes an allocation in the GTT area */
-struct gtt_range {
-	struct resource resource;	/* Resource for our allocation */
-	u32 offset;			/* GTT offset of our object */
-	struct drm_gem_object gem;	/* GEM high level stuff */
-	int in_gart;			/* Currently in the GART (ref ct) */
-	bool stolen;			/* Backed from stolen RAM */
-	bool mmapping;			/* Is mmappable */
-	struct page **pages;		/* Backing pages if present */
-	int npage;			/* Number of backing pages */
-};
-
-#define to_gtt_range(x) container_of(x, struct gtt_range, gem)
-
 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/oaktrail_crtc.c b/drivers/gpu/drm/gma500/oaktrail_crtc.c
index c6b115954b7d..36c7c2686c90 100644
--- a/drivers/gpu/drm/gma500/oaktrail_crtc.c
+++ b/drivers/gpu/drm/gma500/oaktrail_crtc.c
@@ -10,6 +10,7 @@
 #include <drm/drm_fourcc.h>
 
 #include "framebuffer.h"
+#include "gem.h"
 #include "gma_display.h"
 #include "power.h"
 #include "psb_drv.h"
@@ -608,7 +609,7 @@ static int oaktrail_pipe_set_base(struct drm_crtc *crtc,
 	if (!gma_power_begin(dev, true))
 		return 0;
 
-	start = to_gtt_range(fb->obj[0])->offset;
+	start = to_psb_gem_object(fb->obj[0])->offset;
 	offset = y * fb->pitches[0] + x * fb->format->cpp[0];
 
 	REG_WRITE(map->stride, fb->pitches[0]);
diff --git a/drivers/gpu/drm/gma500/psb_intel_display.c b/drivers/gpu/drm/gma500/psb_intel_display.c
index b5e9118c01a4..d5f95212934e 100644
--- a/drivers/gpu/drm/gma500/psb_intel_display.c
+++ b/drivers/gpu/drm/gma500/psb_intel_display.c
@@ -455,22 +455,21 @@ static void psb_intel_cursor_init(struct drm_device *dev,
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
 	u32 control[3] = { CURACNTR, CURBCNTR, CURCCNTR };
 	u32 base[3] = { CURABASE, CURBBASE, CURCBASE };
-	struct gtt_range *cursor_gt;
+	struct psb_gem_object *cursor_pobj;
 
 	if (dev_priv->ops->cursor_needs_phys) {
 		/* Allocate 4 pages of stolen mem for a hardware cursor. That
 		 * is enough for the 64 x 64 ARGB cursors we support.
 		 */
-		cursor_gt = psb_gem_create(dev, 4 * PAGE_SIZE, "cursor", true, PAGE_SIZE);
-		if (IS_ERR(cursor_gt)) {
-			gma_crtc->cursor_gt = NULL;
+		cursor_pobj = psb_gem_create(dev, 4 * PAGE_SIZE, "cursor", true, PAGE_SIZE);
+		if (IS_ERR(cursor_pobj)) {
+			gma_crtc->cursor_pobj = NULL;
 			goto out;
 		}
-		gma_crtc->cursor_gt = cursor_gt;
-		gma_crtc->cursor_addr = dev_priv->stolen_base +
-							cursor_gt->offset;
+		gma_crtc->cursor_pobj = cursor_pobj;
+		gma_crtc->cursor_addr = dev_priv->stolen_base + cursor_pobj->offset;
 	} else {
-		gma_crtc->cursor_gt = NULL;
+		gma_crtc->cursor_pobj = NULL;
 	}
 
 out:
diff --git a/drivers/gpu/drm/gma500/psb_intel_drv.h b/drivers/gpu/drm/gma500/psb_intel_drv.h
index 5340225d6997..db3e757328fe 100644
--- a/drivers/gpu/drm/gma500/psb_intel_drv.h
+++ b/drivers/gpu/drm/gma500/psb_intel_drv.h
@@ -140,7 +140,7 @@ struct gma_crtc {
 	int pipe;
 	int plane;
 	uint32_t cursor_addr;
-	struct gtt_range *cursor_gt;
+	struct psb_gem_object *cursor_pobj;
 	u8 lut_adj[256];
 	struct psb_intel_framebuffer *fbdev_fb;
 	/* a mode_set for fbdev users on this crtc */
-- 
2.33.0

Re: [PATCH 10/10] drm/gma500: Rename struct gtt_range to struct psb_gem_object

[Patrik Jakobsson]

On Tue, Sep 28, 2021 at 10:44 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> struct gtt_range represents a GEM object. Rename the structure to struct
> psb_gem_object and update all users. No functional changes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

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


> ---
>  drivers/gpu/drm/gma500/framebuffer.c       |   9 +-
>  drivers/gpu/drm/gma500/gem.c               | 106 +++++++++++----------
>  drivers/gpu/drm/gma500/gem.h               |  25 ++++-
>  drivers/gpu/drm/gma500/gma_display.c       |  50 +++++-----
>  drivers/gpu/drm/gma500/gtt.c               |  15 +--
>  drivers/gpu/drm/gma500/gtt.h               |  15 ---
>  drivers/gpu/drm/gma500/oaktrail_crtc.c     |   3 +-
>  drivers/gpu/drm/gma500/psb_intel_display.c |  15 ++-
>  drivers/gpu/drm/gma500/psb_intel_drv.h     |   2 +-
>  9 files changed, 123 insertions(+), 117 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
> index 3ea6679ccd38..45df9de22007 100644
> --- a/drivers/gpu/drm/gma500/framebuffer.c
> +++ b/drivers/gpu/drm/gma500/framebuffer.c
> @@ -81,14 +81,13 @@ static vm_fault_t psbfb_vm_fault(struct vm_fault *vmf)
>         struct drm_framebuffer *fb = vma->vm_private_data;
>         struct drm_device *dev = fb->dev;
>         struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> -       struct gtt_range *gtt = to_gtt_range(fb->obj[0]);
> +       struct psb_gem_object *pobj = to_psb_gem_object(fb->obj[0]);
>         int page_num;
>         int i;
>         unsigned long address;
>         vm_fault_t ret = VM_FAULT_SIGBUS;
>         unsigned long pfn;
> -       unsigned long phys_addr = (unsigned long)dev_priv->stolen_base +
> -                                 gtt->offset;
> +       unsigned long phys_addr = (unsigned long)dev_priv->stolen_base + pobj->offset;
>
>         page_num = vma_pages(vma);
>         address = vmf->address - (vmf->pgoff << PAGE_SHIFT);
> @@ -242,7 +241,7 @@ static int psbfb_create(struct drm_fb_helper *fb_helper,
>         struct drm_mode_fb_cmd2 mode_cmd;
>         int size;
>         int ret;
> -       struct gtt_range *backing;
> +       struct psb_gem_object *backing;
>         struct drm_gem_object *obj;
>         u32 bpp, depth;
>
> @@ -264,7 +263,7 @@ static int psbfb_create(struct drm_fb_helper *fb_helper,
>         backing = psb_gem_create(dev, size, "fb", true, PAGE_SIZE);
>         if (IS_ERR(backing))
>                 return PTR_ERR(backing);
> -       obj = &backing->gem;
> +       obj = &backing->base;
>
>         memset(dev_priv->vram_addr + backing->offset, 0, size);
>
> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
> index fd556ba2c044..9b7052153cab 100644
> --- a/drivers/gpu/drm/gma500/gem.c
> +++ b/drivers/gpu/drm/gma500/gem.c
> @@ -21,9 +21,10 @@
>  #include "gem.h"
>  #include "psb_drv.h"
>
> -int psb_gem_pin(struct gtt_range *gt)
> +int psb_gem_pin(struct psb_gem_object *pobj)
>  {
> -       struct drm_device *dev = gt->gem.dev;
> +       struct drm_gem_object *obj = &pobj->base;
> +       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;
>         struct page **pages;
> @@ -31,57 +32,58 @@ int psb_gem_pin(struct gtt_range *gt)
>
>         mutex_lock(&dev_priv->gtt_mutex);
>
> -       if (gt->in_gart || gt->stolen)
> +       if (pobj->in_gart || pobj->stolen)
>                 goto out; /* already mapped */
>
> -       pages = drm_gem_get_pages(&gt->gem);
> +       pages = drm_gem_get_pages(obj);
>         if (IS_ERR(pages))
>                 return PTR_ERR(pages);
>
> -       npages = gt->gem.size / PAGE_SIZE;
> +       npages = obj->size / PAGE_SIZE;
>
>         set_pages_array_wc(pages, npages);
>
> -       psb_gtt_insert_pages(dev_priv, &gt->resource, pages);
> +       psb_gtt_insert_pages(dev_priv, &pobj->resource, pages);
>         psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu), pages,
> -                            (gpu_base + gt->offset), npages, 0, 0,
> +                            (gpu_base + pobj->offset), npages, 0, 0,
>                              PSB_MMU_CACHED_MEMORY);
>
> -       gt->npage = npages;
> -       gt->pages = pages;
> +       pobj->npage = npages;
> +       pobj->pages = pages;
>
>  out:
> -       ++gt->in_gart;
> +       ++pobj->in_gart;
>         mutex_unlock(&dev_priv->gtt_mutex);
>
>         return 0;
>  }
>
> -void psb_gem_unpin(struct gtt_range *gt)
> +void psb_gem_unpin(struct psb_gem_object *pobj)
>  {
> -       struct drm_device *dev = gt->gem.dev;
> +       struct drm_gem_object *obj = &pobj->base;
> +       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;
>
>         mutex_lock(&dev_priv->gtt_mutex);
>
> -       WARN_ON(!gt->in_gart);
> +       WARN_ON(!pobj->in_gart);
>
> -       --gt->in_gart;
> +       --pobj->in_gart;
>
> -       if (gt->in_gart || gt->stolen)
> +       if (pobj->in_gart || pobj->stolen)
>                 goto out;
>
>         psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu),
> -                                    (gpu_base + gt->offset), gt->npage, 0, 0);
> -       psb_gtt_remove_pages(dev_priv, &gt->resource);
> +                            (gpu_base + pobj->offset), pobj->npage, 0, 0);
> +       psb_gtt_remove_pages(dev_priv, &pobj->resource);
>
>         /* Reset caching flags */
> -       set_pages_array_wb(gt->pages, gt->npage);
> +       set_pages_array_wb(pobj->pages, pobj->npage);
>
> -       drm_gem_put_pages(&gt->gem, gt->pages, true, false);
> -       gt->pages = NULL;
> -       gt->npage = 0;
> +       drm_gem_put_pages(obj, pobj->pages, true, false);
> +       pobj->pages = NULL;
> +       pobj->npage = 0;
>
>  out:
>         mutex_unlock(&dev_priv->gtt_mutex);
> @@ -91,18 +93,18 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf);
>
>  static void psb_gem_free_object(struct drm_gem_object *obj)
>  {
> -       struct gtt_range *gt = to_gtt_range(obj);
> +       struct psb_gem_object *pobj = to_psb_gem_object(obj);
>
>         drm_gem_object_release(obj);
>
>         /* Undo the mmap pin if we are destroying the object */
> -       if (gt->mmapping)
> -               psb_gem_unpin(gt);
> +       if (pobj->mmapping)
> +               psb_gem_unpin(pobj);
>
> -       WARN_ON(gt->in_gart && !gt->stolen);
> +       WARN_ON(pobj->in_gart && !pobj->stolen);
>
> -       release_resource(&gt->resource);
> -       kfree(gt);
> +       release_resource(&pobj->resource);
> +       kfree(pobj);
>  }
>
>  static const struct vm_operations_struct psb_gem_vm_ops = {
> @@ -116,31 +118,31 @@ static const struct drm_gem_object_funcs psb_gem_object_funcs = {
>         .vm_ops = &psb_gem_vm_ops,
>  };
>
> -struct gtt_range *
> +struct psb_gem_object *
>  psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, u32 align)
>  {
>         struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> -       struct gtt_range *gt;
> +       struct psb_gem_object *pobj;
>         struct drm_gem_object *obj;
>         int ret;
>
>         size = roundup(size, PAGE_SIZE);
>
> -       gt = kzalloc(sizeof(*gt), GFP_KERNEL);
> -       if (!gt)
> +       pobj = kzalloc(sizeof(*pobj), GFP_KERNEL);
> +       if (!pobj)
>                 return ERR_PTR(-ENOMEM);
> -       obj = &gt->gem;
> +       obj = &pobj->base;
>
>         /* GTT resource */
>
> -       ret = psb_gtt_allocate_resource(dev_priv, &gt->resource, name, size, align, stolen,
> -                                       &gt->offset);
> +       ret = psb_gtt_allocate_resource(dev_priv, &pobj->resource, name, size, align, stolen,
> +                                       &pobj->offset);
>         if (ret)
>                 goto err_kfree;
>
>         if (stolen) {
> -               gt->stolen = true;
> -               gt->in_gart = 1;
> +               pobj->stolen = true;
> +               pobj->in_gart = 1;
>         }
>
>         /* GEM object */
> @@ -158,12 +160,12 @@ psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen,
>                 mapping_set_gfp_mask(obj->filp->f_mapping, GFP_KERNEL | __GFP_DMA32);
>         }
>
> -       return gt;
> +       return pobj;
>
>  err_release_resource:
> -       release_resource(&gt->resource);
> +       release_resource(&pobj->resource);
>  err_kfree:
> -       kfree(gt);
> +       kfree(pobj);
>         return ERR_PTR(ret);
>  }
>
> @@ -181,7 +183,7 @@ int psb_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
>                         struct drm_mode_create_dumb *args)
>  {
>         size_t pitch, size;
> -       struct gtt_range *gt;
> +       struct psb_gem_object *pobj;
>         struct drm_gem_object *obj;
>         u32 handle;
>         int ret;
> @@ -194,10 +196,10 @@ int psb_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
>         if (!size)
>                 return -EINVAL;
>
> -       gt = psb_gem_create(dev, size, "gem", false, PAGE_SIZE);
> -       if (IS_ERR(gt))
> -               return PTR_ERR(gt);
> -       obj = &gt->gem;
> +       pobj = psb_gem_create(dev, size, "gem", false, PAGE_SIZE);
> +       if (IS_ERR(pobj))
> +               return PTR_ERR(pobj);
> +       obj = &pobj->base;
>
>         ret = drm_gem_handle_create(file, obj, &handle);
>         if (ret)
> @@ -236,7 +238,7 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf)
>  {
>         struct vm_area_struct *vma = vmf->vma;
>         struct drm_gem_object *obj;
> -       struct gtt_range *r;
> +       struct psb_gem_object *pobj;
>         int err;
>         vm_fault_t ret;
>         unsigned long pfn;
> @@ -248,7 +250,7 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf)
>         dev = obj->dev;
>         dev_priv = to_drm_psb_private(dev);
>
> -       r = to_gtt_range(obj);
> +       pobj = to_psb_gem_object(obj);
>
>         /* Make sure we don't parallel update on a fault, nor move or remove
>            something from beneath our feet */
> @@ -256,14 +258,14 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf)
>
>         /* For now the mmap pins the object and it stays pinned. As things
>            stand that will do us no harm */
> -       if (r->mmapping == 0) {
> -               err = psb_gem_pin(r);
> +       if (pobj->mmapping == 0) {
> +               err = psb_gem_pin(pobj);
>                 if (err < 0) {
>                         dev_err(dev->dev, "gma500: pin failed: %d\n", err);
>                         ret = vmf_error(err);
>                         goto fail;
>                 }
> -               r->mmapping = 1;
> +               pobj->mmapping = 1;
>         }
>
>         /* Page relative to the VMA start - we must calculate this ourselves
> @@ -271,10 +273,10 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf)
>         page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT;
>
>         /* CPU view of the page, don't go via the GART for CPU writes */
> -       if (r->stolen)
> -               pfn = (dev_priv->stolen_base + r->offset) >> PAGE_SHIFT;
> +       if (pobj->stolen)
> +               pfn = (dev_priv->stolen_base + pobj->offset) >> PAGE_SHIFT;
>         else
> -               pfn = page_to_pfn(r->pages[page_offset]);
> +               pfn = page_to_pfn(pobj->pages[page_offset]);
>         ret = vmf_insert_pfn(vma, vmf->address, pfn);
>  fail:
>         mutex_unlock(&dev_priv->mmap_mutex);
> diff --git a/drivers/gpu/drm/gma500/gem.h b/drivers/gpu/drm/gma500/gem.h
> index 21c86df482a6..79cced40c87f 100644
> --- a/drivers/gpu/drm/gma500/gem.h
> +++ b/drivers/gpu/drm/gma500/gem.h
> @@ -8,14 +8,33 @@
>  #ifndef _GEM_H
>  #define _GEM_H
>
> +#include <linux/kernel.h>
> +
>  #include <drm/drm_gem.h>
>
>  struct drm_device;
>
> -struct gtt_range *
> +struct psb_gem_object {
> +       struct drm_gem_object base;
> +
> +       struct resource resource;       /* GTT resource for our allocation */
> +       u32 offset;                     /* GTT offset of our object */
> +       int in_gart;                    /* Currently in the GART (ref ct) */
> +       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)
> +{
> +       return container_of(obj, struct psb_gem_object, base);
> +}
> +
> +struct psb_gem_object *
>  psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, u32 align);
>
> -int psb_gem_pin(struct gtt_range *gt);
> -void psb_gem_unpin(struct gtt_range *gt);
> +int psb_gem_pin(struct psb_gem_object *pobj);
> +void psb_gem_unpin(struct psb_gem_object *pobj);
>
>  #endif
> diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c
> index 6d0470b27bc5..99da3118131a 100644
> --- a/drivers/gpu/drm/gma500/gma_display.c
> +++ b/drivers/gpu/drm/gma500/gma_display.c
> @@ -55,7 +55,7 @@ int gma_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>         struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>         struct gma_crtc *gma_crtc = to_gma_crtc(crtc);
>         struct drm_framebuffer *fb = crtc->primary->fb;
> -       struct gtt_range *gtt;
> +       struct psb_gem_object *pobj;
>         int pipe = gma_crtc->pipe;
>         const struct psb_offset *map = &dev_priv->regmap[pipe];
>         unsigned long start, offset;
> @@ -71,14 +71,14 @@ int gma_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>                 goto gma_pipe_cleaner;
>         }
>
> -       gtt = to_gtt_range(fb->obj[0]);
> +       pobj = to_psb_gem_object(fb->obj[0]);
>
>         /* We are displaying this buffer, make sure it is actually loaded
>            into the GTT */
> -       ret = psb_gem_pin(gtt);
> +       ret = psb_gem_pin(pobj);
>         if (ret < 0)
>                 goto gma_pipe_set_base_exit;
> -       start = gtt->offset;
> +       start = pobj->offset;
>         offset = y * fb->pitches[0] + x * fb->format->cpp[0];
>
>         REG_WRITE(map->stride, fb->pitches[0]);
> @@ -126,7 +126,7 @@ int gma_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>  gma_pipe_cleaner:
>         /* If there was a previous display we can now unpin it */
>         if (old_fb)
> -               psb_gem_unpin(to_gtt_range(old_fb->obj[0]));
> +               psb_gem_unpin(to_psb_gem_object(old_fb->obj[0]));
>
>  gma_pipe_set_base_exit:
>         gma_power_end(dev);
> @@ -332,8 +332,8 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
>         uint32_t base = (pipe == 0) ? CURABASE : CURBBASE;
>         uint32_t temp;
>         size_t addr = 0;
> -       struct gtt_range *gt;
> -       struct gtt_range *cursor_gt = gma_crtc->cursor_gt;
> +       struct psb_gem_object *pobj;
> +       struct psb_gem_object *cursor_pobj = gma_crtc->cursor_pobj;
>         struct drm_gem_object *obj;
>         void *tmp_dst, *tmp_src;
>         int ret = 0, i, cursor_pages;
> @@ -349,8 +349,8 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
>
>                 /* Unpin the old GEM object */
>                 if (gma_crtc->cursor_obj) {
> -                       gt = to_gtt_range(gma_crtc->cursor_obj);
> -                       psb_gem_unpin(gt);
> +                       pobj = to_psb_gem_object(gma_crtc->cursor_obj);
> +                       psb_gem_unpin(pobj);
>                         drm_gem_object_put(gma_crtc->cursor_obj);
>                         gma_crtc->cursor_obj = NULL;
>                 }
> @@ -375,40 +375,40 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
>                 goto unref_cursor;
>         }
>
> -       gt = to_gtt_range(obj);
> +       pobj = to_psb_gem_object(obj);
>
>         /* Pin the memory into the GTT */
> -       ret = psb_gem_pin(gt);
> +       ret = psb_gem_pin(pobj);
>         if (ret) {
>                 dev_err(dev->dev, "Can not pin down handle 0x%x\n", handle);
>                 goto unref_cursor;
>         }
>
>         if (dev_priv->ops->cursor_needs_phys) {
> -               if (cursor_gt == NULL) {
> +               if (!cursor_pobj) {
>                         dev_err(dev->dev, "No hardware cursor mem available");
>                         ret = -ENOMEM;
>                         goto unref_cursor;
>                 }
>
>                 /* Prevent overflow */
> -               if (gt->npage > 4)
> +               if (pobj->npage > 4)
>                         cursor_pages = 4;
>                 else
> -                       cursor_pages = gt->npage;
> +                       cursor_pages = pobj->npage;
>
>                 /* Copy the cursor to cursor mem */
> -               tmp_dst = dev_priv->vram_addr + cursor_gt->offset;
> +               tmp_dst = dev_priv->vram_addr + cursor_pobj->offset;
>                 for (i = 0; i < cursor_pages; i++) {
> -                       tmp_src = kmap(gt->pages[i]);
> +                       tmp_src = kmap(pobj->pages[i]);
>                         memcpy(tmp_dst, tmp_src, PAGE_SIZE);
> -                       kunmap(gt->pages[i]);
> +                       kunmap(pobj->pages[i]);
>                         tmp_dst += PAGE_SIZE;
>                 }
>
>                 addr = gma_crtc->cursor_addr;
>         } else {
> -               addr = gt->offset;
> +               addr = pobj->offset;
>                 gma_crtc->cursor_addr = addr;
>         }
>
> @@ -425,8 +425,8 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
>
>         /* unpin the old bo */
>         if (gma_crtc->cursor_obj) {
> -               gt = to_gtt_range(gma_crtc->cursor_obj);
> -               psb_gem_unpin(gt);
> +               pobj = to_psb_gem_object(gma_crtc->cursor_obj);
> +               psb_gem_unpin(pobj);
>                 drm_gem_object_put(gma_crtc->cursor_obj);
>         }
>
> @@ -483,14 +483,14 @@ void gma_crtc_commit(struct drm_crtc *crtc)
>
>  void gma_crtc_disable(struct drm_crtc *crtc)
>  {
> -       struct gtt_range *gt;
> +       struct psb_gem_object *pobj;
>         const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
>
>         crtc_funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
>
>         if (crtc->primary->fb) {
> -               gt = to_gtt_range(crtc->primary->fb->obj[0]);
> -               psb_gem_unpin(gt);
> +               pobj = to_psb_gem_object(crtc->primary->fb->obj[0]);
> +               psb_gem_unpin(pobj);
>         }
>  }
>
> @@ -498,8 +498,8 @@ void gma_crtc_destroy(struct drm_crtc *crtc)
>  {
>         struct gma_crtc *gma_crtc = to_gma_crtc(crtc);
>
> -       if (gma_crtc->cursor_gt)
> -               drm_gem_object_put(&gma_crtc->cursor_gt->gem);
> +       if (gma_crtc->cursor_pobj)
> +               drm_gem_object_put(&gma_crtc->cursor_pobj->base);
>
>         kfree(gma_crtc->crtc_state);
>         drm_crtc_cleanup(crtc);
> diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
> index cf71a2396c16..75e483c88ceb 100644
> --- a/drivers/gpu/drm/gma500/gtt.c
> +++ b/drivers/gpu/drm/gma500/gtt.c
> @@ -7,6 +7,7 @@
>   *         Alan Cox <alan@linux.intel.com>
>   */
>
> +#include "gem.h" /* TODO: for struct psb_gem_object, see psb_gtt_restore() */
>  #include "psb_drv.h"
>
>
> @@ -292,7 +293,7 @@ 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 gtt_range *range;
> +       struct psb_gem_object *pobj;
>         unsigned int restored = 0, total = 0, size = 0;
>
>         /* On resume, the gtt_mutex is already initialized */
> @@ -302,13 +303,13 @@ int psb_gtt_restore(struct drm_device *dev)
>         while (r != NULL) {
>                 /*
>                  * TODO: GTT restoration needs a refactoring, so that we don't have to touch
> -                *       struct gtt_range here. The type represents a GEM object and is not
> -                *       related to the GTT itself.
> +                *       struct psb_gem_object here. The type represents a GEM object and is
> +                *       not related to the GTT itself.
>                  */
> -               range = container_of(r, struct gtt_range, resource);
> -               if (range->pages) {
> -                       psb_gtt_insert_pages(dev_priv, &range->resource, range->pages);
> -                       size += range->resource.end - range->resource.start;
> +               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;
> diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h
> index 6a28e24a18b7..29649395c531 100644
> --- a/drivers/gpu/drm/gma500/gtt.h
> +++ b/drivers/gpu/drm/gma500/gtt.h
> @@ -28,21 +28,6 @@ struct psb_gtt {
>  /* Exported functions */
>  extern int psb_gtt_init(struct drm_device *dev, int resume);
>  extern void psb_gtt_takedown(struct drm_device *dev);
> -
> -/* Each gtt_range describes an allocation in the GTT area */
> -struct gtt_range {
> -       struct resource resource;       /* Resource for our allocation */
> -       u32 offset;                     /* GTT offset of our object */
> -       struct drm_gem_object gem;      /* GEM high level stuff */
> -       int in_gart;                    /* Currently in the GART (ref ct) */
> -       bool stolen;                    /* Backed from stolen RAM */
> -       bool mmapping;                  /* Is mmappable */
> -       struct page **pages;            /* Backing pages if present */
> -       int npage;                      /* Number of backing pages */
> -};
> -
> -#define to_gtt_range(x) container_of(x, struct gtt_range, gem)
> -
>  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/oaktrail_crtc.c b/drivers/gpu/drm/gma500/oaktrail_crtc.c
> index c6b115954b7d..36c7c2686c90 100644
> --- a/drivers/gpu/drm/gma500/oaktrail_crtc.c
> +++ b/drivers/gpu/drm/gma500/oaktrail_crtc.c
> @@ -10,6 +10,7 @@
>  #include <drm/drm_fourcc.h>
>
>  #include "framebuffer.h"
> +#include "gem.h"
>  #include "gma_display.h"
>  #include "power.h"
>  #include "psb_drv.h"
> @@ -608,7 +609,7 @@ static int oaktrail_pipe_set_base(struct drm_crtc *crtc,
>         if (!gma_power_begin(dev, true))
>                 return 0;
>
> -       start = to_gtt_range(fb->obj[0])->offset;
> +       start = to_psb_gem_object(fb->obj[0])->offset;
>         offset = y * fb->pitches[0] + x * fb->format->cpp[0];
>
>         REG_WRITE(map->stride, fb->pitches[0]);
> diff --git a/drivers/gpu/drm/gma500/psb_intel_display.c b/drivers/gpu/drm/gma500/psb_intel_display.c
> index b5e9118c01a4..d5f95212934e 100644
> --- a/drivers/gpu/drm/gma500/psb_intel_display.c
> +++ b/drivers/gpu/drm/gma500/psb_intel_display.c
> @@ -455,22 +455,21 @@ static void psb_intel_cursor_init(struct drm_device *dev,
>         struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>         u32 control[3] = { CURACNTR, CURBCNTR, CURCCNTR };
>         u32 base[3] = { CURABASE, CURBBASE, CURCBASE };
> -       struct gtt_range *cursor_gt;
> +       struct psb_gem_object *cursor_pobj;
>
>         if (dev_priv->ops->cursor_needs_phys) {
>                 /* Allocate 4 pages of stolen mem for a hardware cursor. That
>                  * is enough for the 64 x 64 ARGB cursors we support.
>                  */
> -               cursor_gt = psb_gem_create(dev, 4 * PAGE_SIZE, "cursor", true, PAGE_SIZE);
> -               if (IS_ERR(cursor_gt)) {
> -                       gma_crtc->cursor_gt = NULL;
> +               cursor_pobj = psb_gem_create(dev, 4 * PAGE_SIZE, "cursor", true, PAGE_SIZE);
> +               if (IS_ERR(cursor_pobj)) {
> +                       gma_crtc->cursor_pobj = NULL;
>                         goto out;
>                 }
> -               gma_crtc->cursor_gt = cursor_gt;
> -               gma_crtc->cursor_addr = dev_priv->stolen_base +
> -                                                       cursor_gt->offset;
> +               gma_crtc->cursor_pobj = cursor_pobj;
> +               gma_crtc->cursor_addr = dev_priv->stolen_base + cursor_pobj->offset;
>         } else {
> -               gma_crtc->cursor_gt = NULL;
> +               gma_crtc->cursor_pobj = NULL;
>         }
>
>  out:
> diff --git a/drivers/gpu/drm/gma500/psb_intel_drv.h b/drivers/gpu/drm/gma500/psb_intel_drv.h
> index 5340225d6997..db3e757328fe 100644
> --- a/drivers/gpu/drm/gma500/psb_intel_drv.h
> +++ b/drivers/gpu/drm/gma500/psb_intel_drv.h
> @@ -140,7 +140,7 @@ struct gma_crtc {
>         int pipe;
>         int plane;
>         uint32_t cursor_addr;
> -       struct gtt_range *cursor_gt;
> +       struct psb_gem_object *cursor_pobj;
>         u8 lut_adj[256];
>         struct psb_intel_framebuffer *fbdev_fb;
>         /* a mode_set for fbdev users on this crtc */
> --
> 2.33.0
>