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
