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

[Maxime Ripard]

On Mon, Oct 09, 2023 at 10:23:02AM +0200, Thomas Zimmermann wrote:
> 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.

Nothing guarantees that this is true. Even if in RGB, the buffer could
be unaccessible by the CPU, or still in an opaque format (when using
AFBC for example).

>      * Without a working mode, drivers likely attempt to program a common
> display mode with RGB colors.

Which would potentially require extra allocations, computations, etc.
that probably aren't doable in a panic handler path.

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

What I'm trying to say is that it's not just about providing new
helpers. Sure, we can make drivers do whatever they want and provide a
scanout buffer. We still have to put that buffer into an active plane at
some point. The current design doesn't provide any way to do that
properly.

That's what I'd like to see addressed, and I will disagree with any
proposal that just ignores it.

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

I mean, I don't want to sweep the code under the rug but keep it. I want
to know why we need that code in the first place. If there's no reason
then we just shouldn't have that caching at all.

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/20231010/50c20113/attachment-0001.sig>