On Tue, Oct 10, 2023 at 10:55:09AM +0200, Thomas Zimmermann wrote: > > > So if I understand correctly, drm_panic would pre-allocate a plane/commit, > > > and use that when a panic occurs ? > > > > And have it checked already, yes. We would only wait for a panic to > > happen to pull the trigger on the commit. > > > > > I have two concern about this approach: > > > - How much memory would be allocated for this ? a whole framebuffer can be > > > big for just this use case. > > As I outlined in my email at [1], there are a number of different scenarios. > The question of atomic state and commits is entirely separate from the DRM > panic handler. We should not throw them together. Whatever is necessary is > get a scanout buffer, should happen on the driver side of > get_scanout_buffer, not on the drm_panic side. > > [1] https://lore.kernel.org/dri-devel/39bd4c35-8a61-42ee-8675-ccea4f5d4ac6@xxxxxxx/T/#m22f116e9438e00a5f0a9dc43987d4153424f8be1 > > > > > I'dd expect a whole framebuffer for the current > > configuration/resolution. It would be typically 4MB for a full-HD system > > which isn't a lot really and I guess we can always add an option to > > disable the mechanism if needed. > > > > > - 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. > > The one thing I don't understand is: Why should we use atomic commits in the > first place? It doesn't make sense for firmware-based drivers. Because this is generic infrastructure that is valuable for any drivers and not only firmware-based drivers? > In some drivers, even the simple ast, we hold locks during the regular > commit. Trying to run the panic commit concurrently will likely give a > deadlock. You're in the middle of a panic. Don't take any locks and you won't deadlock. > In the end it's a per-driver decision, but in most cases, the driver can > easily switch to a default mode with some ad-hoc code. When was the last time a per-driver decision has been a good thing? I'm sorry, but the get_scanout_buffer approach buffer won't work for any driver out there. I'm fine with discussing alternatives if you don't like the ones I suggested, but they must allow the panic handler infrastructure to work with any driver we have, not just 4. Maxime
Attachment:
signature.asc
Description: PGP signature
