Re: [PATCH 06/10] drm/gma500: Move GTT resume logic out of psb_gtt_init()

[Sam Ravnborg <sam@ravnborg.org>]

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@suse.de>
> ---
>  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?

> +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