On Fri, Oct 15, 2021 at 10:40 AM Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> ---
> 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, >->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(>->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, >->resource);
>
> /* Reset caching flags */
> set_pages_array_wb(gt->pages, gt->npage);
>
> drm_gem_put_pages(>->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
>
