On Fri, 17 Nov 2023 at 11:07, Thomas Zimmermann <tzimmerm...@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
