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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

Am 10.10.23 um 11:33 schrieb Maxime Ripard:
[...]
We also have discussions about kexec/kdump support. Here we'd need to
retrieve the scanout address, forward it to the kexec kernel and put
simpledrm onto that framebuffer until the regular driver takes over.

Generically speaking, there's strictly no guarantee that the current
scanout buffer is accessible by the CPU. It's not even a driver issue,
it's a workload issue, so it will affect 100% of the times some users,
and some 0% of the time.

And I'd be OK with that. It's all best effort anyway.


An interface like get_scanout_buffer will be helpful for this use
case. So it makes sense to use it for the panic handler as well.

It won't be helpful because the vast majority of the ARM drivers (and
thus the vast majority of the KMS drivers) won't be able to implement it
properly and reliably.

The barrier for firmware-based drivers is low: a pre-configured display and mmap'able framebuffer memory. And it's assumed that a callback for kexec would attempt to configure hardware accordingly. If a system really cannot provide this, so be it.


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?

Please see my other replies. I find this fragile, and unnecessary for cases
where there already is a working scanout buffer in place.

And please see my other replies. Depending on a working scanout buffer
in place being accessible by the CPU is incredibly limiting. Ignoring it
when I'm pointing that out won't get us to a solution acceptable for
everyone.

It's something a driver could implement internally to provide a
scanout buffer if none has been set up already.

Some drivers need extra resources when setting up a plane. VC4 for
example need for every plane to allocate multiple internal SRAM buffers.
Just allocating an extra framebuffer won't cut it.

This is tied to the state so far, so I guess we would need to allocate a
new state. Oh, and if we have several drivers that need to allocate that
extra framebuffer and state, we could make it part of the core or turn
it into a helper?

I mentioned that even the simple drivers hold locks during the atomic commit. Some of the drivers use vmap/vunmap, which might make problems as well. It's used in the context of damage handling, which also makes no sense here. So the regular atomic helpers most likely won't work.

It sounds to me as if you're essentially asking for something like a flush or sync operation.

Stepping back from get_scanout_buffer for a moment and adopting your proposal from above, I can see something like this working:

  - the driver provides a panic callback in struct drm_driver

  - DRM registers a panic handler to invokes the callback

- the panic callback has a scanout buffer from somewhere (currently active, prepared, from firmware, plain memory, etc)

  - we provide a helper that fills the scanout buffer with information

- the driver then updates the hardware from/with the scanout buffer; details depend on the hardware

The final step is like a flush or commit operation. To give some examples: The simple drivers of this patchset probably don't have to do anything. Drivers with command/packet queues could attempt to send the necessary commands to the device.

Best regards
Thomas


Maxime

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux