Hi Sam and Patrik Am 07.03.22 um 22:02 schrieb Patrik Jakobsson:
On Mon, Mar 7, 2022 at 8:07 PM Sam Ravnborg <sam@xxxxxxxxxxxx> wrote:Hi Thomas, One comment below. On Sun, Mar 06, 2022 at 09:36:15PM +0100, Thomas Zimmermann wrote:The current implementation of psb_gtt_init() also does resume handling. Move the resume code into its own helper. Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx> --- drivers/gpu/drm/gma500/gtt.c | 122 ++++++++++++++++++++++++++----- drivers/gpu/drm/gma500/gtt.h | 2 +- drivers/gpu/drm/gma500/psb_drv.c | 2 +- 3 files changed, 104 insertions(+), 22 deletions(-) diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c index acd50ee26b03..43ad3ec38c80 100644 --- a/drivers/gpu/drm/gma500/gtt.c +++ b/drivers/gpu/drm/gma500/gtt.c @@ -209,7 +209,7 @@ static void psb_gtt_populate_resources(struct drm_psb_private *pdev) drm_dbg(dev, "Restored %u of %u gtt ranges (%u KB)", restored, total, (size / 1024)); } -int psb_gtt_init(struct drm_device *dev, int resume) +int psb_gtt_init(struct drm_device *dev) { struct drm_psb_private *dev_priv = to_drm_psb_private(dev); struct pci_dev *pdev = to_pci_dev(dev->dev); @@ -218,10 +218,8 @@ int psb_gtt_init(struct drm_device *dev, int resume) struct psb_gtt *pg; int ret = 0; - if (!resume) { - mutex_init(&dev_priv->gtt_mutex); - mutex_init(&dev_priv->mmap_mutex); - } + mutex_init(&dev_priv->gtt_mutex); + mutex_init(&dev_priv->mmap_mutex); pg = &dev_priv->gtt; @@ -290,13 +288,6 @@ int psb_gtt_init(struct drm_device *dev, int resume) dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n", dev_priv->stolen_base, vram_stolen_size / 1024); - if (resume && (gtt_pages != pg->gtt_pages) && - (stolen_size != pg->stolen_size)) { - dev_err(dev->dev, "GTT resume error.\n"); - ret = -EINVAL; - goto out_err; - } - pg->gtt_pages = gtt_pages; pg->stolen_size = stolen_size; dev_priv->vram_stolen_size = vram_stolen_size; @@ -304,19 +295,14 @@ int psb_gtt_init(struct drm_device *dev, int resume) /* * Map the GTT and the stolen memory area */ - if (!resume) - dev_priv->gtt_map = ioremap(pg->gtt_phys_start, - gtt_pages << PAGE_SHIFT); + dev_priv->gtt_map = ioremap(pg->gtt_phys_start, gtt_pages << PAGE_SHIFT); if (!dev_priv->gtt_map) { dev_err(dev->dev, "Failure to map gtt.\n"); ret = -ENOMEM; goto out_err; } - if (!resume) - dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base, - stolen_size); - + dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base, stolen_size); if (!dev_priv->vram_addr) { dev_err(dev->dev, "Failure to map stolen base.\n"); ret = -ENOMEM; @@ -333,11 +319,107 @@ int psb_gtt_init(struct drm_device *dev, int resume) return ret; }The below is a lot of duplicated complex code. Can you add one more helper for this?
That makes a lot of sense.
I was thinking the same but figured it would be an easy follow up patch. But sure, why not fix it here already. While at it, the gtt enable/disable code could be put in helpers as well.
Patrik, do you know why the code re-reads all these values during resume? Do the values change? The driver never seems to do anything with the updated values. For example, it doesn't update the GTT mapping if gtt_phys_start changes. Can we remove all that code from the resume function?
Best regards Thomas
+static int psb_gtt_resume(struct drm_device *dev) +{ + struct drm_psb_private *dev_priv = to_drm_psb_private(dev); + struct pci_dev *pdev = to_pci_dev(dev->dev); + unsigned int gtt_pages; + unsigned long stolen_size, vram_stolen_size; + struct psb_gtt *pg; + int ret = 0; + + pg = &dev_priv->gtt;static void psb_enable_gtt(..) {+ + /* Enable the GTT */ + pci_read_config_word(pdev, PSB_GMCH_CTRL, &dev_priv->gmch_ctrl); + pci_write_config_word(pdev, PSB_GMCH_CTRL, + dev_priv->gmch_ctrl | _PSB_GMCH_ENABLED); + + dev_priv->pge_ctl = PSB_RVDC32(PSB_PGETBL_CTL); + PSB_WVDC32(dev_priv->pge_ctl | _PSB_PGETBL_ENABLED, PSB_PGETBL_CTL); + (void) PSB_RVDC32(PSB_PGETBL_CTL); + + /* The root resource we allocate address space from */ + dev_priv->gtt_initialized = 1; + + pg->gtt_phys_start = dev_priv->pge_ctl & PAGE_MASK; + + /* + * The video mmu has a hw bug when accessing 0x0D0000000. + * Make gatt start at 0x0e000,0000. This doesn't actually + * matter for us but may do if the video acceleration ever + * gets opened up. + */ + pg->mmu_gatt_start = 0xE0000000; + + pg->gtt_start = pci_resource_start(pdev, PSB_GTT_RESOURCE); + gtt_pages = pci_resource_len(pdev, PSB_GTT_RESOURCE) >> PAGE_SHIFT; + /* CDV doesn't report this. In which case the system has 64 gtt pages */ + if (pg->gtt_start == 0 || gtt_pages == 0) { + dev_dbg(dev->dev, "GTT PCI BAR not initialized.\n"); + gtt_pages = 64; + pg->gtt_start = dev_priv->pge_ctl; + } + + pg->gatt_start = pci_resource_start(pdev, PSB_GATT_RESOURCE); + pg->gatt_pages = pci_resource_len(pdev, PSB_GATT_RESOURCE) >> PAGE_SHIFT; + dev_priv->gtt_mem = &pdev->resource[PSB_GATT_RESOURCE]; + + if (pg->gatt_pages == 0 || pg->gatt_start == 0) { + static struct resource fudge; /* Preferably peppermint */ + /* + * This can occur on CDV systems. Fudge it in this case. We + * really don't care what imaginary space is being allocated + * at this point. + */ + dev_dbg(dev->dev, "GATT PCI BAR not initialized.\n"); + pg->gatt_start = 0x40000000; + pg->gatt_pages = (128 * 1024 * 1024) >> PAGE_SHIFT; + /* + * This is a little confusing but in fact the GTT is providing + * a view from the GPU into memory and not vice versa. As such + * this is really allocating space that is not the same as the + * CPU address space on CDV. + */ + fudge.start = 0x40000000; + fudge.end = 0x40000000 + 128 * 1024 * 1024 - 1; + fudge.name = "fudge"; + fudge.flags = IORESOURCE_MEM; + dev_priv->gtt_mem = &fudge; + } + + pci_read_config_dword(pdev, PSB_BSM, &dev_priv->stolen_base); + vram_stolen_size = pg->gtt_phys_start - dev_priv->stolen_base - PAGE_SIZE; + stolen_size = vram_stolen_size; + + dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n", + dev_priv->stolen_base, vram_stolen_size / 1024);} And then use this helper in both init and resume?+ + if ((gtt_pages != pg->gtt_pages) && (stolen_size != pg->stolen_size)) { + dev_err(dev->dev, "GTT resume error.\n"); + ret = -EINVAL; + goto out_err; + } ++ pg->gtt_pages = gtt_pages; + pg->stolen_size = stolen_size;Not needed for resume, we just checked them.+ dev_priv->vram_stolen_size = vram_stolen_size; + + psb_gtt_clear(dev_priv); + psb_gtt_populate_stolen(dev_priv); + + return 0; + +out_err: + psb_gtt_takedown(dev); + return ret; +} + int psb_gtt_restore(struct drm_device *dev) { struct drm_psb_private *dev_priv = to_drm_psb_private(dev); - psb_gtt_init(dev, 1); + psb_gtt_resume(dev); psb_gtt_populate_resources(dev_priv); diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h index 31500533ac45..cb270ea40794 100644 --- a/drivers/gpu/drm/gma500/gtt.h +++ b/drivers/gpu/drm/gma500/gtt.h @@ -25,7 +25,7 @@ struct psb_gtt { }; /* Exported functions */ -extern int psb_gtt_init(struct drm_device *dev, int resume); +int psb_gtt_init(struct drm_device *dev); extern void psb_gtt_takedown(struct drm_device *dev); extern int psb_gtt_restore(struct drm_device *dev);A cleanup patch to remove all extern would be nice. But that's not related to this series. Sam
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
