Messages in this thread | | | Date | Wed, 22 Nov 2023 11:23:05 +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:54, Boris Brezillon ha scritto: > Hi Angelo, > > On Wed, 22 Nov 2023 10:06:19 +0100 > AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > 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 🙂 > > 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
| |