Hi Patrik,
On 9/18/22 20:45, Patrik Jakobsson wrote:
> On Sat, Sep 17, 2022 at 2:31 PM Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxx>
>>
>> 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
>>>>
>>>
>>
>
