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

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

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.

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20231006/2aec3390/attachment.sig>