Hi Am 09.10.23 um 13:33 schrieb Maxime Ripard: [...]
And you don't need to support all kinds of tiling, YUV or RGB variants.
We should indeed not use YUV at all. For RGB, we already have plenty of conversion code from XRGB8888-to-<whatever>, so we are more flexible there.
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. 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.
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.
Best regards Thomas
Also how many drivers would need this ? Currently I was mostly considering x86 platform, so: simpledrm/ast/mgag200 which works well with the get_scanout_buffer(). i915/amdgpu/nouveau, which are quite complex, and will need to do their own thing anyway.I guess we're not entirely aligned there then. I would expect that mechanism to work with any atomic KMS driver. You are right that i915, amdgpu and nouveau are special enough that some extra internal plumbing is going to be required, but I'd expect it to be easy to support with any other driver for a memory-mapped device. I guess what I'm trying to say is, even though it's totally fine that you only support those drivers at first, supporting in vc4 for example shouldn't require to rewrite the whole thing.I'm more in favor of an emergency function, that each driver has to implement, and use what the hardware can do to display a simple frame quickly. get_scanout_buffer() is a good start for simple driver, but will need refactoring for the more complex case, like adding a callback to write pixels one by one, if there is no memory mapped buffer available.Sorry, I'm not quite sure what you mean there, where would you write the pixel to?It was mentioned by Noralf, about the amdgpu driver: https://lore.kernel.org/dri-devel/d233c376-ed07-2127-6084-8292d313dac7@xxxxxxx/ They have a slow "debug interface" that you can write to, and can be used for the panic handler. It's not memory mapped, so you have to write pixels one by one. So for the struct drm_scanout_buffer, I can add a function pointer to a write_pixel(u32 x, u32 y, u32 color) So if the iosys map is null, it will use that instead.It would be nice to support indeed, but it's a fairly rare feature afaik so I'd rather focus on getting something that can work for everyone first 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
