Re: [PATCH] drm/panfrost: Really power off GPU cores in panfrost_gpu_power_off()

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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