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)
Attachment:
OpenPGP_signature.asc
Description: OpenPGP digital signature
