| Message ID | 20220909115646.99920-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/gma500: 1 fix + further cleanups | expand |
On Fri, Sep 9, 2022 at 1:56 PM Hans de Goede <hdegoede@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@gmail.com> > 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 >
Hi, On 9/14/22 08:50, Patrik Jakobsson wrote: > On Fri, Sep 9, 2022 at 1:56 PM Hans de Goede <hdegoede@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. Ok, I'm at Linux Plumbers atm, so I will take care of this in a couple of days. > "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@gmail.com> Thanks. 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 >> >
Hi Patrik, On 9/14/22 09:50, Patrik Jakobsson wrote: > On Fri, Sep 9, 2022 at 1:56 PM Hans de Goede <hdegoede@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@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. 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... 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 >> >
On Sat, Sep 17, 2022 at 2:31 PM Hans de Goede <hdegoede@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@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@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 > >> > > >
Hi Patrik, On 9/18/22 20:45, Patrik Jakobsson wrote: > On Sat, Sep 17, 2022 at 2:31 PM Hans de Goede <hdegoede@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@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@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. It seemed to fix things on my 64 bit CDV system too, but it is really hit and miss for me. I suspect that if you do a couple of full poweroff + boot again cycles it will be back some of the time for you too... > The 32-bit system never had an issue to begin with. I can also take a > closer look at this. If you feel like verifying my dev_priv->apm_base = 0; theory that would be great. Looking at the less hacky iosf-mbi access code from: arch/x86/platform/intel/iosf_mbi.c What might help is adding this: diff --git a/drivers/gpu/drm/gma500/cdv_device.c b/drivers/gpu/drm/gma500/cdv_device.c index 3065596257e9..70ce06ee3c1c 100644 --- a/drivers/gpu/drm/gma500/cdv_device.c +++ b/drivers/gpu/drm/gma500/cdv_device.c @@ -145,7 +145,7 @@ static int cdv_backlight_init(struct drm_device *dev) static inline u32 CDV_MSG_READ32(int domain, uint port, uint offset) { - int mcr = (0x10<<24) | (port << 16) | (offset << 8); + int mcr = (0x10<<24) | (port << 16) | (offset << 8) | 0xF0; uint32_t ret_val = 0; struct pci_dev *pci_root = pci_get_domain_bus_and_slot(domain, 0, 0); pci_write_config_dword(pci_root, 0xD0, mcr); the gma500 cdv code already does this in the write path, but the: arch/x86/platform/intel/iosf_mbi.c code does it both the write and read paths. Also we should probably just add the PCI root-bridge product-id to: drivers/gpu/drm/gma500/cdv_device.c and start using the iosf_mbi_read() / iosf_mbi_write() helpers from there. >> 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. The non working outputs thing is something weirdly timing related, I have only seen this happen when the gma500_gfx module is not in the initrd (so it gets loaded later). So I need to retry once we have a proper fix for this, atm I am using the default Fedora config of having the module inside the initrd. Not doing random outl to IO address 0 and 0x04 seems like a good idea regardless though :) Regards, Hans > >> >> >> 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 >>>> >>> >> >