Re: [PATCH v4 0/7] drm: Reuse temporary memory for format conversion

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 3652 bytes --]

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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]