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

Patrik Jakobsson patrik.r.jakobsson at gmail.com
Mon Oct 18 12:27:59 UTC 2021


On Fri, Oct 15, 2021 at 10:40 AM Thomas Zimmermann <tzimmermann at 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.
>
> Rename the functions to psb_gtt_insert_pages() and psb_gtt_remove_pages()
> to reflect their similarity to MMU interfaces.
>
> v3:
>         * restore the comments about locking rules (Patrik)
>

That should be the last of them. Nice cleanups! Thanks.

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


> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> ---
>  drivers/gpu/drm/gma500/gem.c | 12 ++---
>  drivers/gpu/drm/gma500/gtt.c | 93 ++++++++++++++----------------------
>  drivers/gpu/drm/gma500/gtt.h |  5 +-
>  3 files changed, 44 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
> index def26d9ce89d..c93d09e1921e 100644
> --- a/drivers/gpu/drm/gma500/gem.c
> +++ b/drivers/gpu/drm/gma500/gem.c
> @@ -23,12 +23,12 @@
>
>  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;
> +       int ret;
>
>         mutex_lock(&dev_priv->gtt_mutex);
>
> @@ -45,10 +45,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);
> @@ -62,8 +59,6 @@ int psb_gem_pin(struct gtt_range *gt)
>
>         return 0;
>
> -err_drm_gem_put_pages:
> -       drm_gem_put_pages(&gt->gem, pages, true, false);
>  err_mutex_unlock:
>         mutex_unlock(&dev_priv->gtt_mutex);
>         return ret;
> @@ -86,13 +81,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 3a716a970a8a..0d70f63c3267 100644
> --- a/drivers/gpu/drm/gma500/gtt.c
> +++ b/drivers/gpu/drm/gma500/gtt.c
> @@ -66,85 +66,61 @@ 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.
> +/*
> + * 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.
> +/*
> + * 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 +310,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 ddb4f3af93f7..b967dcf6bef1 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);
>
> -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
>


More information about the dri-devel mailing list