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

[Thomas Zimmermann]

Hi

Am 10.10.23 um 14:59 schrieb Daniel Vetter:
[...]
>>>>
>>>
>>> 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?

I have one detail about the code: While working on the format-conversion 
code, I've always found struct drm_framebuffer to be clunky to work 
with. It's build around the idea of managing GEM buffers, but not 
accessing them.

So I've been thinking about something like a drm_pixmap that contains 
size, color format and data pointers in one place. Reads and writes 
would be callbacks. It could abstract access to any kind of buffer. 
IIRC Jocelyn had something like that in the very first patchset. or 
maybe fb_pixmap could be repurposed.

Best regards
Thomas

> 
>>> 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.
> -Sima

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