[PATCH 01/10] drm/gma500: Move helpers for struct gtt_range from gtt.c to gem.c
Thomas Zimmermann
tzimmermann at suse.de
Mon Oct 4 06:11:46 UTC 2021
Hi
Am 03.10.21 um 00:13 schrieb Patrik Jakobsson:
> On Tue, Sep 28, 2021 at 10:44 AM Thomas Zimmermann <tzimmermann at 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 at 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(>->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(>->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(>->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, >->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(>->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(>->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, >->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(>->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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20211004/6c8ace76/attachment-0001.sig>
More information about the dri-devel
mailing list