On Tue, Oct 10, 2023 at 09:55:46AM +0200, Jocelyn Falempe wrote: > 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. Thanks :) > If possible I still want to have a way for simple drivers like > simpledrm/mgag200 to not allocate a full framebuffer. I guess the split isn't going to be whether the driver is simple or not, but whether it always has access to the buffer used by the scanout. Like for simpledrm, we have that guarantee so it makes sense. For other, if we allow "direct" dma-buf, it's game over and we just can't. Maxime
Attachment:
signature.asc
Description: PGP signature
