On 10/10/2023 14:59, Daniel Vetter wrote:
On Tue, Oct 10, 2023 at 02:15:47PM +0200, Maxime Ripard wrote:
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@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.
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.
I think largely it should still be ok, because the idea behind the draw_xy
callback was that the driver could take care of anything special, like
- tiling
- clearing compression bits so that just writing the raw pixels works (if
your compression format allows for that)
- handling any differences in how the pixels need to be written, like
cache flushing, mmio_write vs normal memory, amd also has peek/poke
registers to be able to write even into unmappable memory
What would probably be a good idea is to do an s/void */uinptr_t/ over my
interface proposal, or maybe an even more opaque cookie since really the
only thing you can do is get the void * that ->panic_vmap returns and pass
it into ->panic_draw_xy.
I thought (but I didn't dig through details) that the panic fb struct is
essentially meant to serve this purpose in the current series?
Yes, in this series there is a struct drm_scanout_buffer, that the
driver has to provide when a panic occurs:
struct drm_scanout_buffer {
const struct drm_format_info *format;
struct iosys_map map;
unsigned int pitch;
unsigned int width;
unsigned int height;
};
That works well for CPU accessible, linear format.
It should be possible to support YUV, or simple tiling with that, but
for the more complex case, I proposed to add a draw_pixel() callback.
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?
So panic teardown would be for testing in CI, to make it non-destructive
and clean up anything panic_vmap (or _enter or whatever you call it) has
done. I thought John Oggness was also looking into how the new panic
handlers/consoles could be made testable in the panic paths, including
lock stealing and getting called from nmi.
Thanks, I was also wondering why a panic teardown would be necessary,
since after the panic handler has run, the system will probably halt.
(or maybe run kdump and reboot).
-Sima
