Messages in this thread | | | Date | Wed, 22 Nov 2023 11:33:21 +0100 | Subject | Re: [PATCH] drm/panfrost: Really power off GPU cores in panfrost_gpu_power_off() | From | AngeloGioacchino Del Regno <> |
| |
Il 22/11/23 10:48, Steven Price ha scritto: > On 22/11/2023 09:06, AngeloGioacchino Del Regno wrote: >> Il 21/11/23 18:08, Krzysztof Kozlowski ha scritto: >>> On 21/11/2023 17:55, Boris Brezillon wrote: >>>> On Tue, 21 Nov 2023 17:11:42 +0100 >>>> AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >>>> wrote: >>>> >>>>> Il 21/11/23 16:34, Krzysztof Kozlowski ha scritto: >>>>>> On 08/11/2023 14:20, Steven Price wrote: >>>>>>> On 02/11/2023 14:15, AngeloGioacchino Del Regno wrote: >>>>>>>> The layout of the registers {TILER,SHADER,L2}_PWROFF_LO, used to >>>>>>>> request >>>>>>>> powering off cores, is the same as the {TILER,SHADER,L2}_PWRON_LO >>>>>>>> ones: >>>>>>>> this means that in order to request poweroff of cores, we are >>>>>>>> supposed >>>>>>>> to write a bitmask of cores that should be powered off! >>>>>>>> This means that the panfrost_gpu_power_off() function has always >>>>>>>> been >>>>>>>> doing nothing. >>>>>>>> >>>>>>>> Fix powering off the GPU by writing a bitmask of the cores to >>>>>>>> poweroff >>>>>>>> to the relevant PWROFF_LO registers and then check that the >>>>>>>> transition >>>>>>>> (from ON to OFF) has finished by polling the relevant PWRTRANS_LO >>>>>>>> registers. >>>>>>>> >>>>>>>> While at it, in order to avoid code duplication, move the core mask >>>>>>>> logic from panfrost_gpu_power_on() to a new panfrost_get_core_mask() >>>>>>>> function, used in both poweron and poweroff. >>>>>>>> >>>>>>>> Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver") >>>>>>>> Signed-off-by: AngeloGioacchino Del Regno >>>>>>>> <angelogioacchino.delregno@collabora.com> >>>>>> >>>>>> >>>>>> Hi, >>>>>> >>>>>> This commit was added to next recently but it causes "external >>>>>> abort on >>>>>> non-linefetch" during boot of my Odroid HC1 board. >>>>>> >>>>>> At least bisect points to it. >>>>>> >>>>>> If fixed, please add: >>>>>> >>>>>> Reported-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>>>> >>>>>> [ 4.861683] 8<--- cut here --- >>>>>> [ 4.863429] Unhandled fault: external abort on non-linefetch >>>>>> (0x1008) at 0xf0c8802c >>>>>> [ 4.871018] [f0c8802c] *pgd=433ed811, *pte=11800653, *ppte=11800453 >>>>>> ... >>>>>> [ 5.164010] panfrost_gpu_irq_handler from >>>>>> __handle_irq_event_percpu+0xcc/0x31c >>>>>> [ 5.171276] __handle_irq_event_percpu from >>>>>> handle_irq_event+0x38/0x80 >>>>>> [ 5.177765] handle_irq_event from handle_fasteoi_irq+0x9c/0x250 >>>>>> [ 5.183743] handle_fasteoi_irq from >>>>>> generic_handle_domain_irq+0x28/0x38 >>>>>> [ 5.190417] generic_handle_domain_irq from >>>>>> gic_handle_irq+0x88/0xa8 >>>>>> [ 5.196741] gic_handle_irq from generic_handle_arch_irq+0x34/0x44 >>>>>> [ 5.202893] generic_handle_arch_irq from __irq_svc+0x8c/0xd0 >>>>>> >>>>>> Full log: >>>>>> https://krzk.eu/#/builders/21/builds/4392/steps/11/logs/serial0 >>>>>> >>>>> >>>>> Hey Krzysztof, >>>>> >>>>> This is interesting. It might be about the cores that are missing >>>>> from the partial >>>>> core_mask raising interrupts, but an external abort on non-linefetch >>>>> is strange to >>>>> see here. >>>> >>>> I've seen such external aborts in the past, and the fault type has >>>> often been misleading. It's unlikely to have anything to do with a >>> >>> Yeah, often accessing device with power or clocks gated. >>> >> >> Except my commit does *not* gate SoC power, nor SoC clocks 🙂 >> >> What the "Really power off ..." commit does is to ask the GPU to >> internally power >> off the shaders, tilers and L2, that's why I say that it is strange to >> see that >> kind of abort. >> >> The GPU_INT_CLEAR GPU_INT_STAT, GPU_FAULT_STATUS and >> GPU_FAULT_ADDRESS_{HI/LO} >> registers should still be accessible even with shaders, tilers and cache >> OFF. >> >> Anyway, yes, synchronizing IRQs before calling the poweroff sequence >> would also >> work, but that'd add up quite a bit of latency on the runtime_suspend() >> call, so >> in this case I'd be more for avoiding to execute any register r/w in the >> handler >> by either checking if the GPU is supposed to be OFF, or clearing >> interrupts, which >> may not work if those are generated after the execution of the poweroff >> function. >> Or we could simply disable the irq after power_off, but that'd be hacky >> (as well). >> >> >> Let's see if asking to poweroff *everything* works: >> >> >> --- >> drivers/gpu/drm/panfrost/panfrost_gpu.c | 14 +++++++++++--- >> 1 file changed, 11 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c >> b/drivers/gpu/drm/panfrost/panfrost_gpu.c >> index 09f5e1563ebd..1c7276aaa182 100644 >> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c >> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c >> @@ -429,21 +429,29 @@ void panfrost_gpu_power_off(struct panfrost_device >> *pfdev) >> int ret; >> u32 val; >> >> - gpu_write(pfdev, SHADER_PWROFF_LO, pfdev->features.shader_present & >> core_mask); >> + gpu_write(pfdev, SHADER_PWROFF_LO, pfdev->features.shader_present); > > Hopefully this one line change, and... > >> + gpu_write(pfdev, SHADER_PWROFF_HI, U32_MAX); >> ret = readl_relaxed_poll_timeout(pfdev->iomem + SHADER_PWRTRANS_LO, >> val, !val, 1, 1000); >> if (ret) >> dev_err(pfdev->dev, "shader power transition timeout"); >> >> gpu_write(pfdev, TILER_PWROFF_LO, pfdev->features.tiler_present); >> + gpu_write(pfdev, TILER_PWROFF_HI, U32_MAX); >> ret = readl_relaxed_poll_timeout(pfdev->iomem + TILER_PWRTRANS_LO, >> val, !val, 1, 1000); >> if (ret) >> dev_err(pfdev->dev, "tiler power transition timeout"); >> >> - gpu_write(pfdev, L2_PWROFF_LO, pfdev->features.l2_present & >> core_mask); >> + gpu_write(pfdev, L2_PWROFF_LO, pfdev->features.l2_present); > > ... this one are all that are actually needed - the rest should be > ignored as they affect cores that aren't present. >
Honestly - when I wrote that diff, I didn't care at all whether the HI registers were powering off cores that weren't present, because I knew that the GPU would have handled that gracefully anyway.
What I wanted to do was to reduce Krzysztof's testing effort to a minimum, actually preventing to send more than one patch to try... but with that, you bought me a bit of precious time that I would've spent with research, so, hats off! Thank you!
> The Exynos 5422 SoC has a T628 MP6 - so two core groups which isn't a > particularly well supported configuration. But I'm not sure how we're > ending up with the second core group being powered up in the first > place. Even if it was left powered by something previous (e.g. the > bootloader) then the soft-reset during probe should cause them to power > down. >
Hm. I didn't know that soft_reset is supposed to (and will) power down cores. This is clarifying some things I didn't really have an explanation for... so thanks again :-)
> But it seems like a good idea to power off everything when powering > down, even if we didn't expect the cores to be on. > > Boris also has a point that before cutting the power/clocks we should > really be synchronising with the IRQs - but that affects the follow on > patches not this one. >
...which gives me some more ideas to try... in the near future. But it's out of context for this fix anyway.
Cheers, Angelo
> Steve > >> ret = readl_poll_timeout(pfdev->iomem + L2_PWRTRANS_LO, >> - val, !val, 0, 1000); >> + val, !val, 0, 1000); >> + if (ret) >> + dev_err(pfdev->dev, "l2_low power transition timeout"); >> + >> + gpu_write(pfdev, L2_PWROFF_HI, U32_MAX); >> + ret = readl_poll_timeout(pfdev->iomem + L2_PWRTRANS_HI, >> + val, !val, 0, 1000); >> if (ret) >> dev_err(pfdev->dev, "l2 power transition timeout"); >> } >
| |