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
Attachment:
signature.asc
Description: PGP signature
