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

[Patrik Jakobsson <patrik.r.jakobsson@gmail.com>]

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
>