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

[Daniel Vetter]

On Fri, 17 Nov 2023 at 11:07, Thomas Zimmermann <tzimmermann at suse.de> wrote:
>
> Hi
>
> Am 17.11.23 um 10:34 schrieb Maxime Ripard:
> > On Mon, Oct 09, 2023 at 04:06:29PM +0200, Thomas Zimmermann wrote:
> >> DRM's format-conversion helpers require temporary memory. Pass the
> >> buffer from the caller to allow the caller to preallocate the buffer
> >> memory.
> >>
> >> 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 a kernel panic is
> >> fragile. The changes in this patchset enable the DRM panic handler to
> >> preallocate buffer storage before the panic occurs.
> >>
> >> Patch 1 adds struct drm_format_conv_state, a simple interface to pass
> >> around the buffer storage. Patch 2 adds an instance of the struct to
> >> the shadow-plane state. Patch 3 moves the buffer's memory management
> >> from the format helpers into their callers within the DRM drivers. Most
> >> of the affected drivers use the state instance stored in their shadow-
> >> plane state. The shadow-plane code releases the buffer memory automatically.
> >>
> >> Patches 4 to 7 update three drivers to pre-allocate the format-conversion
> >> buffer in their plane's atomic_check function. The drivers thus detect OOM
> >> errors before the display update begins.
> >>
> >> Tested with simpledrm.
> >
> > So, I just discovered that you merged that series.
> >
> > You've complained before about "sneaking patches in", and while I was
> > disagreeing with you then, this particular instance is definitely a
> > strong case for it. You've merged it without telling anyone, and despite
> > our ongoing conversation on the v4 that was active more recently than
> > the v5. And that you never responded to.
> >
> > Awesome.
>
> My apologies. From my point of view, that conversion had ended. I left
> the patch set for a while to wait for further comments or questions, but
> nothing happened. So I merged it.
>
> Revert it if you cannot live with the changes. IIRC you found the
> reduced number of alloc/free cycles to be irrelevant. But even then, the
> patches allow us to move the allocation from atomic_update to
> atomic_check, thus detecting allocation failures early. That's an
> improvement to me.

Just a small comment on this, I didn't read up the full discussion:

Yeah allocating memory in atomic_commit after the point of no return
is no-go. Usually the best spot is the prepare_fb hooks since that
avoids doing expensive allocations for TEST_ONLY commits, but since
the allocation is fairly small it probably doesn't matter overall.
Well the new-ish begin/end_fb_access helpers would be even better I
guess for the atomic flow, but break the panic handler use-case I
think.

Oh and a bikeshed, _copy() feels a bit like a funny postfix for
something that does a state duplication, everywhere else in atomic we
put _duplicate into the name for these functions.

Cheers, Sima

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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch