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

[Maxime Ripard]

On Tue, Oct 10, 2023 at 01:29:52PM +0200, Noralf Trønnes wrote:
> 
> 
> On 10/10/23 11:25, Maxime Ripard wrote:
> > 
> > 
> > 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.
> > 
> 
> Why can't we use the model[1] suggested by Daniel using a draw_pixel
> callback giving drivers full control on how they can put a pixel on the
> display?

I share kind of the same general ideas/conclusions: "qthe idea is that
all the fb selection and lookup is handled in shared code (and with
proper locking, but only for atomic drivers)."

2016 is a bit old though and multiple developments happened since
(secure playback is a thing now, framebuffer compression too), so I
still think that their expectation that the framebuffer is accessible to
/ writable by the CPU no longer holds true.

> This will even work for the AMD debug interface.
> In the linear CPU accessible buffer case, we can provide a helper for
> that, maybe we can do helpers for other common cases as well.

Yeah, their idea of a panic_draw would work great for that.

> Adding to that we would need a panic_setup/enter and panic_teardown/exit
> callback.

What for?

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/870bd720/attachment.sig>