Hi Maxime Am 06.10.23 um 16:49 schrieb Maxime Ripard:
Hi, On Thu, Oct 05, 2023 at 11:04:20AM +0200, Thomas Zimmermann wrote:DRM's format-conversion helpers require temporary memory. Pass the buffer from the caller and keep it allocated over several calls. Allow the caller to preallocate the buffer memory.I'm sorry... but why? Why do you need to keep it allocated over several calls and preallocate the buffer? It's not clear to me at all.The motivation for this patchset is the recent work on a DRM panic handler. [1] The panic handler requires format conversion to display an error to the screen. But allocating memory during kernel panics is fragile.We agree that we shouldn't allocate memory during the panic. I still have concerns about how the panic handler will handle the driver currently set up for a plane that isn't using an RGB format, or a buffer not accessible by the kernel or CPU. You can't expect to get away with just a copy to the current active buffer.
In our current design, the panic handler calls get_scanout_buffer from struct drm_driver to retrieve a scanout buffer. What happens within that callback depends on the driver and hardware. Here are some of the expected scenarios:
* simpledrm or ofdrm can return the firmware-provided scanout buffer. No further action is required.
* Devices on a PCI-like bus:* With a working mode in RGB colors, drivers can return the current scanout buffer as well. * Without a working mode, drivers likely attempt to program a common display mode with RGB colors.
* Drivers for devices behind other busses, such as USB, will probably not be able to reprogram during a panic or provide a useful scanout buffer at all.
* The scanout buffer has to be mapped into kernel address space. This operation might be fragile during a panic. So drivers could set aside a slice of graphics memory and pre-map it; then use it during panic (requires some mode programming).
I expect that we will eventually have helpers for the various scenarios. Drivers will be able to implement their get_scanout_buffer with these helpers.
The font glyphs are 1-bit bitmaps. So we have to convert them to the scanout buffer's format in any case. We want to use the existing format-conversion helpers were possible.
If that's the assumption that underlines that patch series, then I don't know why we need it at all, because that assumption is wrong to begin with, and way too restrictive.The changes in this patchset enable the DRM panic handler to preallocate buffer storage before the panic occurs. As an additonal benefit, drivers can now keep the temporary storage across multiple updates. Avoiding memory allocation slightly reduces the CPU overhead of the format helpers.I'm sorry to go over that again, but you can't write a performance improvement mechanism without some kind of benchmark. kmalloc has built-in caching, why do we absolutely need our own cache on top of it? If you never measured it, for all we know, we simply don't need it and kmalloc is good enough.
I'll remove that paragraph if you find it so annoying. Let me just say again that overhead is not the primary motivation behind these patches.
Best regards Thomas
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
