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

[Thomas Zimmermann]

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@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. 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@amd.com/
>>
>> 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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20231010/f6e50431/attachment-0001.sig>