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

[Thomas Zimmermann]

Hi

Am 08.03.22 um 13:07 schrieb 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?

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
-------------- 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/20220308/32828894/attachment-0001.sig>