[PATCH 09/10] drm/gma500: Rewrite GTT page insert/remove without struct gtt_range
Thomas Zimmermann
tzimmermann at suse.de
Mon Oct 4 06:31:30 UTC 2021
Hi
Am 03.10.21 um 00:15 schrieb Patrik Jakobsson:
> On Tue, Sep 28, 2021 at 10:44 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.
>
> We could also treat the GTT as registers and save/restore it that way.
> OFC that approach would waste a bit of memory.
For me, the next step would be to split-off the GEM code from the
remaining GTT code. That would be an opportunity to improve this. I
think the current code with its call to pin might be broken anyway and
messes up the reference counting during resume.
If the driver ever switches to atomic modesetting, we could use the
suspend helpers and have the mode reprogrammed during resume. That
should resolve the whole issue.
Best regards
Thomas
>
>
>>
>> Rename the functions to psb_gtt_insert_pages() and psb_gtt_remove_pages()
>> to reflect their similarity to MMU interfaces.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> ---
>> drivers/gpu/drm/gma500/gem.c | 13 ++----
>> drivers/gpu/drm/gma500/gtt.c | 87 ++++++++++++------------------------
>> drivers/gpu/drm/gma500/gtt.h | 5 ++-
>> 3 files changed, 35 insertions(+), 70 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
>> index a88d51a3aa2a..fd556ba2c044 100644
>> --- a/drivers/gpu/drm/gma500/gem.c
>> +++ b/drivers/gpu/drm/gma500/gem.c
>> @@ -23,7 +23,6 @@
>>
>> 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;
>> @@ -43,10 +42,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);
>> @@ -59,10 +55,6 @@ int psb_gem_pin(struct gtt_range *gt)
>> mutex_unlock(&dev_priv->gtt_mutex);
>>
>> return 0;
>> -
>> -err_drm_gem_put_pages:
>> - drm_gem_put_pages(>->gem, pages, true, false);
>> - return ret;
>> }
>>
>> void psb_gem_unpin(struct gtt_range *gt)
>> @@ -82,13 +74,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 244de618e612..cf71a2396c16 100644
>> --- a/drivers/gpu/drm/gma500/gtt.c
>> +++ b/drivers/gpu/drm/gma500/gtt.c
>> @@ -66,85 +66,51 @@ 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.
>> - */
>> -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.
>> - */
>> -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 +300,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 7af71617e0c5..6a28e24a18b7 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[static 1]);
>>
>> -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
>>
--
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/1467dba0/attachment-0001.sig>
More information about the dri-devel
mailing list