Hi Am 08.03.22 um 13:07 schrieb Patrik Jakobsson:
On Tue, Mar 8, 2022 at 9:48 AM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote: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?In theory these values can change if you hibernate, make changes in the bios and then resume. I think I've seen bioses that can change the stolen size and/or aperture size. Not sure if that would also change the value in eg. pge_ctl or the size of the gtt. I would have to do some testing on real hardware to figure this out. But as you say, the above scenario is already broken. For the time being, can we perhaps just fail gracefully?
Ah, thanks for the explanation. I'll leave the current logic as-is. Best regards Thomas -- 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
