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

[Maxime Ripard]

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?
Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20231009/afb57005/attachment.sig>