[PATCH] drm/panfrost: Really power off GPU cores in panfrost_gpu_power_off()
AngeloGioacchino Del Regno
angelogioacchino.delregno at collabora.com
Wed Nov 22 10:33:21 UTC 2023
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 at 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 at 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 at 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");
>> }
>
More information about the dri-devel
mailing list