On Tue, Oct 5, 2021 at 10:11 AM Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxx>
> ---
> 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 37b61334ade2..425d183c76ca 100644
> --- a/drivers/gpu/drm/gma500/gem.c
> +++ b/drivers/gpu/drm/gma500/gem.c
> @@ -91,30 +91,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(>->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(>->resource);
> + kfree(gt);
> }
>
> static const struct vm_operations_struct psb_gem_vm_ops = {
> @@ -128,59 +120,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, >->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 = >->gem;
>
> + /* GTT resource */
> +
> + ret = psb_gtt_allocate_resource(dev_priv, >->resource, name, size, align, stolen,
> + >->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) {
> @@ -188,7 +156,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);
> @@ -196,8 +164,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(>->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])
My concern with [static 1] here is not how it's used with arrays. In
this case offset isn't an array so should just be "u32 *offset".
> +{
> + 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
>
