[PATCH v4 2/4] drm/panic: Add a drm panic handler

[Jocelyn Falempe]

On 10/10/2023 15:05, Thomas Zimmermann wrote:
> Hi
> 
> Am 10.10.23 um 11:33 schrieb Maxime Ripard:
> [...]
>>> We also have discussions about kexec/kdump support. Here we'd need to
>>> retrieve the scanout address, forward it to the kexec kernel and put
>>> simpledrm onto that framebuffer until the regular driver takes over.
>>
>> Generically speaking, there's strictly no guarantee that the current
>> scanout buffer is accessible by the CPU. It's not even a driver issue,
>> it's a workload issue, so it will affect 100% of the times some users,
>> and some 0% of the time.
> 
> And I'd be OK with that. It's all best effort anyway.
> 
>>
>>> An interface like get_scanout_buffer will be helpful for this use
>>> case. So it makes sense to use it for the panic handler as well.
>>
>> It won't be helpful because the vast majority of the ARM drivers (and
>> thus the vast majority of the KMS drivers) won't be able to implement it
>> properly and reliably.
> 
> The barrier for firmware-based drivers is low: a pre-configured display 
> and mmap'able framebuffer memory. And it's assumed that a callback for 
> kexec would attempt to configure hardware accordingly. If a system 
> really cannot provide this, so be it.
> 
>>
>>>>> run the atomic_update() on it, and return this commit's framebuffer ?
>>>>>
>>>>> That way each driver have a better control on what the panic 
>>>>> handler will
>>>>> do.
>>>>> It can even call directly its internal functions, to avoid the 
>>>>> locks of the
>>>>> drm generic functions, and make sure it will only change the format 
>>>>> and base
>>>>> address.
>>>>> That's a bit more work for each driver, but should be more reliable 
>>>>> I think.
>>>>
>>>> I don't think that better control there is a good idea, it's a path 
>>>> that
>>>> won't get tested much so we'd be better off not allowing drivers to
>>>> deviate too much from the "ideal" design.
>>>>
>>>> What I had in mind is something like:
>>>>
>>>>     - Add a panic hook in drm_mode_config_funcs, with a
>>>>       drm_atomic_helper_panic helper;
>>>>
>>>>     - Provide an atomic_panic hook or something in 
>>>> drm_plane_helper_funcs;
>>>>
>>>>     - If they are set, we register the drm_panic handler;
>>>>
>>>>     - The handler will call drm_mode_config_funcs.panic, which will 
>>>> take
>>>>       its prepared state, fill the framebuffer it allocated with the
>>>>       penguin and backtrace, call 
>>>> drm_plane_helper_funcs.atomic_panic().
>>>>
>>>>     - The driver now updates the format and fb address.
>>>>
>>>>     - Halt and catch fire
>>>>
>>>> Does that make sense?
>>>
>>> Please see my other replies. I find this fragile, and unnecessary for 
>>> cases
>>> where there already is a working scanout buffer in place.
>>
>> And please see my other replies. Depending on a working scanout buffer
>> in place being accessible by the CPU is incredibly limiting. Ignoring it
>> when I'm pointing that out won't get us to a solution acceptable for
>> everyone.
>>
>>> It's something a driver could implement internally to provide a
>>> scanout buffer if none has been set up already.
>>
>> Some drivers need extra resources when setting up a plane. VC4 for
>> example need for every plane to allocate multiple internal SRAM buffers.
>> Just allocating an extra framebuffer won't cut it.
>>
>> This is tied to the state so far, so I guess we would need to allocate a
>> new state. Oh, and if we have several drivers that need to allocate that
>> extra framebuffer and state, we could make it part of the core or turn
>> it into a helper?
> 
> I mentioned that even the simple drivers hold locks during the atomic 
> commit. Some of the drivers use vmap/vunmap, which might make problems 
> as well. It's used in the context of damage handling, which also makes 
> no sense here. So the regular atomic helpers most likely won't work.
> 
> It sounds to me as if you're essentially asking for something like a 
> flush or sync operation.
> 
> Stepping back from get_scanout_buffer for a moment and adopting your 
> proposal from above, I can see something like this working:
> 
>    - the driver provides a panic callback in struct drm_driver
> 
>    - DRM registers a panic handler to invokes the callback
> 
>    - the panic callback has a scanout buffer from somewhere (currently 
> active, prepared, from firmware, plain memory, etc)
> 
>    - we provide a helper that fills the scanout buffer with information
> 
>    - the driver then updates the hardware from/with the scanout buffer; 
> details depend on the hardware
> 
> The final step is like a flush or commit operation. To give some 
> examples: The simple drivers of this patchset probably don't have to do 
> anything. Drivers with command/packet queues could attempt to send the 
> necessary commands to the device.

Yes that was the second question in the cover letter.
Some drivers probably want a flush_scanout_buffer() to send the 
framebuffer to the hardware.
That callback can be in struct drm_driver as well.

-- 

Jocelyn

> 
> Best regards
> Thomas
> 
>>
>> Maxime
>