[PATCH 0/6] drm/gma500: 1 fix + further cleanups

[Patrik Jakobsson]

On Sat, Sep 17, 2022 at 2:31 PM Hans de Goede <hdegoede at redhat.com> wrote:
>
> Hi Patrik,
>
> On 9/14/22 09:50, Patrik Jakobsson wrote:
> > On Fri, Sep 9, 2022 at 1:56 PM Hans de Goede <hdegoede at redhat.com> wrote:
> >>
> >> Hi Patrik,
> >>
> >> Here is another gma500 patch-series with one more bugfix and a bunch
> >> of other cleanups of stuff which I noticed while doing the previous
> >> set of bugfixes.
> >>
> >
> > Hi Hans, nice cleanups!
> >
> > I'm rather busy at the moment so you can commit these yourself to
> > drm-misc-next if you like.
> >
> > "drm/gma500: Wait longer for the GPU to power-down" can go through
> > drm-misc-fixes if you prefer. It fixed the timeout message on two of
> > my CDV machines but I never saw an actual problem from the timeouts.
> >
> > For the entire series:
> > Acked-by: Patrik Jakobsson <patrik.r.jakobsson at gmail.com>
>
> Thanks.
>
> I'm pushing these out to drm-misc-next now, but with some small
> changes:
>
> 1. I have dropped the "drm/gma500: Wait longer for the GPU to power-down"
> patch I'm still seeing timeouts even if I increase the wait time to
> a full seconds. I believe that the actual issue is this line:
>
>         dev_priv->apm_base = CDV_MSG_READ32(domain, PSB_PUNIT_PORT, PSB_APMBA);
>
> sometimes failing. When the timeout happens I see apm_base is set to 0 and
> reading apm_base + cmd / sts offset returns bogus values.

Ugh, that is nasty. Debugging the punit is probably not easy. But the
increased delay did remove the error on two of my 64-bit CDV systems.
The 32-bit system never had an issue to begin with. I can also take a
closer look at this.

>
> I have yet to have a successful boot where the timeout does not happen
> since I have been poking at this (it seems success/fail wrt the timeout
> is random). But I suspect that with a successful boot apm_base will not
> be 0 and that the problem is there. To be continued...

Do you still get working outputs when this happens? Perhaps we can
work around it by simply not touching APM if apm_base is 0.

>
>
> 2. For the "drm/gma500: Rewrite power management code" I noticed the
> following error during further testing (for the actual backlight changes):
>
> [   12.292509] gma500 0000:00:02.0: Unbalanced pm_runtime_enable!
>
> The problem is that pci_pm_init() which the PCI core runs for each
> device already does:
>
>         pm_runtime_forbid(&dev->dev);
>         pm_runtime_set_active(&dev->dev);
>         pm_runtime_enable(&dev->dev);
>
> So the pm_runtime_enable() call in "drm/gma500: Rewrite power management code"
> was a second enable call and as such was not necessary. So I'm going to
> squash in the following small fix while pushing this out:
>
> diff --git a/drivers/gpu/drm/gma500/power.c b/drivers/gpu/drm/gma500/power.c
> index 62d2cc1923f1..0080b692dc3e 100644
> --- a/drivers/gpu/drm/gma500/power.c
> +++ b/drivers/gpu/drm/gma500/power.c
> @@ -61,10 +61,11 @@ void gma_power_init(struct drm_device *dev)
>          * To fix this we need to call pm_runtime_get() once for each active
>          * pipe at boot and then put() / get() for each pipe disable / enable
>          * so that the device gets runtime suspended when no pipes are active.
> +        * Once this is in place the pm_runtime_get() below should be replaced
> +        * by a pm_runtime_allow() call to undo the pm_runtime_forbid() from
> +        * pci_pm_init().
>          */
>         pm_runtime_get(dev->dev);
> -       pm_runtime_set_active(dev->dev); /* Must be done before pm_runtime_enable()! */
> -       pm_runtime_enable(dev->dev);
>
>         dev_priv->pm_initialized = true;
>  }
> @@ -83,7 +83,6 @@ void gma_power_uninit(struct drm_device *dev)
>         if (!dev_priv->pm_initialized)
>                 return;
>
> -       pm_runtime_disable(dev->dev);
>         pm_runtime_put_noidle(dev->dev);
>  }
>
>
> As you can see all the removed lines are already taken care of by the
> PCI core, so this squashed in change really is a no-op (other then
> that it silences the "Unbalanced pm_runtime_enable!" message).
>
> Regards,
>
> Hans
>
>
>
>
> >
> >
> >> Regards,
> >>
> >> Hans
> >>
> >>
> >> Hans de Goede (6):
> >>   drm/gma500: Wait longer for the GPU to power-down
> >>   drm/gma500: Remove runtime_allowed dead code in psb_unlocked_ioctl()
> >>   drm/gma500: Remove never set dev_priv->rpm_enabled flag
> >>   drm/gma500: Remove a couple of not useful function wrappers
> >>   drm/gma500: Rewrite power management code
> >>   drm/gma500: Remove unnecessary suspend/resume wrappers
> >>
> >>  drivers/gpu/drm/gma500/cdv_device.c    |   2 +-
> >>  drivers/gpu/drm/gma500/gma_display.c   |  19 +--
> >>  drivers/gpu/drm/gma500/gma_display.h   |   2 -
> >>  drivers/gpu/drm/gma500/oaktrail_lvds.c |   1 -
> >>  drivers/gpu/drm/gma500/power.c         | 156 +++++--------------------
> >>  drivers/gpu/drm/gma500/power.h         |  18 ---
> >>  drivers/gpu/drm/gma500/psb_drv.c       |  35 +-----
> >>  drivers/gpu/drm/gma500/psb_drv.h       |   7 +-
> >>  drivers/gpu/drm/gma500/psb_irq.c       |  15 ++-
> >>  9 files changed, 41 insertions(+), 214 deletions(-)
> >>
> >> --
> >> 2.37.2
> >>
> >
>