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

[Jocelyn Falempe]

On 09/10/2023 18:07, Maxime Ripard wrote:
> On Mon, Oct 09, 2023 at 04:05:19PM +0200, Jocelyn Falempe wrote:
>>>> - I find it risky to completely reconfigure the hardware in a panic handler.
>>>
>>> I would expect to only change the format and base address of the
>>> framebuffer. I guess it can fail, but it doesn't seem that different to
>>> the async plane update we already have and works well.
>>>
>> In this case it can work, but by using generic drm api, it's hard to know
>> what the driver will do.
> 
> We should document extensively what we expect drivers to do in those
> hooks, and possibly call cant_sleep() in the framework function to have
> some reporting at least.
> 
>>>> Also how many drivers would need this ?
>>>>
>>>> Currently I was mostly considering x86 platform, so:
>>>>
>>>> simpledrm/ast/mgag200 which works well with the get_scanout_buffer().
>>>>
>>>> i915/amdgpu/nouveau, which are quite complex, and will need to do their own
>>>> thing anyway.
>>>
>>> I guess we're not entirely aligned there then. I would expect that
>>> mechanism to work with any atomic KMS driver. You are right that i915,
>>> amdgpu and nouveau are special enough that some extra internal plumbing
>>> is going to be required, but I'd expect it to be easy to support with
>>> any other driver for a memory-mapped device.
>>>
>>> I guess what I'm trying to say is, even though it's totally fine that
>>> you only support those drivers at first, supporting in vc4 for example
>>> shouldn't require to rewrite the whole thing.
>>
>> Would that work for you to put that in a drm_panic_helper.c,
>> so that drivers can opt-in ?
>>
>> So the driver can call a drm_panic_helper_prepare_commit() at
>> initialization, and then in the get_scanout_buffer() function
> 
> If we have a full blown commit with a new framebuffer, why do we need
> get_scanout_buffer? It should be either the framebuffer itself, or in
> the plane state if you have a conversion.
> 
>> 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?

Yes, I will do some experiment with that, and see if I can make it work 
this way.
If possible I still want to have a way for simple drivers like 
simpledrm/mgag200 to not allocate a full framebuffer.

> Maxime

-- 

Jocelyn