Il 22/11/23 10:54, Boris Brezillon ha scritto:
Hi Angelo,
On Wed, 22 Nov 2023 10:06:19 +0100
AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
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@xxxxxxxxxx>
[ 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 🙂
It's not directly related to your commit, it's just a side effect.
Indeed!
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.
It's not the power_off() call that's problematic, it's when it happens
(the last thing called in panfrost_device_runtime_suspend()), and the
fact it generates interrupts. Yes, you don't explicitly gate the clocks
in panfrost_device_runtime_suspend(), but the PM layer does interact
directly with power domains, and shutting down a power domain might
result in other clks/components being gated, which might make the
register bank inaccessible from the CPU.
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,
Really? In practice I'd expect no pending interrupts, other than the
power transition ones, which are purely and simply ignored by the
handler. If we had any other pending interrupts on suspend, we would
have faced this problem before. To sum-up, I'd expect the extra latency
to just be the overhead of the synchronize_irq() call, which, after
looking at the code, shouldn't be such a big deal.
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,
Yes, that's an option, but I don't think that's enough (see below).
or clearing interrupts,
The handler might have been called already when you clear the
interrupt, and you'd still need to make sure the handler has returned
before returning from panfrost_device_runtime_suspend() if you want to
guarantee no one is touching the registers when the power domains are
shutdown.
which
may not work if those are generated after the execution of the poweroff function.
They are generated while you poll the register, but that doesn't
guarantee they will be processed by the time you return from your
power_off() function, which I think is exactly the problem we're facing
here.
Or we could simply disable the irq after power_off, but that'd be hacky (as well).
If by disabling the interrupt you mean calling disable_irq(), that
would work if the irq lines were not declared as shared (IRQF_SHARED
flag passed at request time). Beside, the latency of disable_irq()
should be pretty much the same as synchronize_irq(), given
synchronize_irq() from there.
If by disabling the interrupt, you mean masking it with _INT_MASK,
then, as said above, that's not enough. You need to make sure any
handler that may have been called as a result of this interrupt,
returns before you return from the suspend function, so you need some
kind of synchronization.
Your reasons are totally valid and I see the point.
That's what I'll do as a follow-up Fixes patch:
- gpu_write(pfdev, GPU_INT_MASK, 0);
- gpu_write(pfdev, GPU_INT_CLEAR, GPU_IRQ_MASK_ALL);
- synchronize_irq()
- poweroff *all* shaders/tilers/l2 (without caring about core_mask)
- *No* INT_MASK restore, as we rely on soft_reset() to do that for us
once we resume the GPU.
Let's see if asking to poweroff *everything* works:
It might slightly change the timing, making this problem disappear by
chance (if the interrupt gets processed before power_off() returns),
but it doesn't make the suspend logic more robust. We really have to
guarantee that no one will touch the registers when we enter suspend,
be it some interrupt handler, or any kind of deferred work.
Again, none of this is a direct result of your patch, it's just that
your patch uncovered the problem, and I think now is a good time to fix
it properly.
Yes, I am well aware of that and I was trying to make that clear in the first
place - I'm sorry if I gave the impression of having any kind of doubt around
that, or any other.
Cheers!
Angelo
