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

[Patrik Jakobsson]

On Tue, Mar 8, 2022 at 9:48 AM Thomas Zimmermann <tzimmermann at suse.de> 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 at ravnborg.org> 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 at 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?
> >
>
> 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?