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

[Maxime Ripard]

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@suse.de/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
-------------- 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/20231010/da12e504/attachment-0001.sig>