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

[Thomas Zimmermann <tzimmermann@suse.de>]

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

Hi Noralf

Am 07.10.23 um 14:01 schrieb Noralf Trønnes:
> 
> 
> On 10/5/23 11:04, 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.
>>
>> 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. The changes in this patchset enable the DRM panic handler to
>> preallocate buffer storage before the panic occurs.
>>
> 
> I've been thinking about this and afaiu this requires the display hw to
> switch to the new panic buffer for scanout, right?
> I don't think that is possible for any complex hw to do in a panic
> situation. Or are you thinking that the driver should somehow "memcpy"
> this buffer to the actual scanout buffer?

It's all of that, but depends on the situation. In the easy case, we 
have a usable scanout buffer during the panic (e.g., XRGB888). We can 
use it for the panic screen.

If there's no such scanout programmed, we can attempt to set a new 
display mode. I think, it is best to leave these exact steps to the DRM 
driver. Some drivers might not be able to reprogram the display (e.g., 
with USB or I2C hardware).

In the panic handler, we copy glyphs into the scanout buffer and 
possibly convert the color format during that process. Without a working 
scanout buffer, the panic handler does nothing.

Best regards
Thomas


> 
> Noralf.
> 
>> 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.
>>
>> 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 afected 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 preallocate the format-conversion
>> buffer in their plane's atomic_check function. The driver thus detects OOM
>> errors before the display update begins.
>>
>> Tested with simpledrm.
>>
>> v4:
>> 	* rename struct to drm_format_conv_state (Javier)
>> 	* replace ARRAY_SIZE() with sizeof() (Jani)
>> 	* store buffer in shadow-plane state (Javier, Maxime)
>> 	* prealloc in atomic_check in several drivers
>> v3:
>> 	* no changes
>> v2:
>> 	* reserve storage during probing in the drivers
>>
>> [1] https://patchwork.freedesktop.org/series/122244/
>>
>> Thomas Zimmermann (7):
>>    drm/format-helper: Cache buffers with struct drm_format_conv_state
>>    drm/atomic-helper: Add format-conversion state to shadow-plane state
>>    drm/format-helper: Pass format-conversion state to helpers
>>    drm/ofdrm: Preallocate format-conversion buffer in atomic_check
>>    drm/simpledrm: Preallocate format-conversion buffer in atomic_check
>>    drm/ssd130x: Fix atomic_check for disabled planes
>>    drm/ssd130x: Preallocate format-conversion buffer in atomic_check
>>
>>   drivers/gpu/drm/drm_format_helper.c           | 212 +++++++++++++-----
>>   drivers/gpu/drm/drm_gem_atomic_helper.c       |   9 +
>>   drivers/gpu/drm/drm_mipi_dbi.c                |  19 +-
>>   drivers/gpu/drm/gud/gud_pipe.c                |  30 ++-
>>   drivers/gpu/drm/solomon/ssd130x.c             |  36 ++-
>>   .../gpu/drm/tests/drm_format_helper_test.c    |  72 +++---
>>   drivers/gpu/drm/tiny/cirrus.c                 |   3 +-
>>   drivers/gpu/drm/tiny/ili9225.c                |  10 +-
>>   drivers/gpu/drm/tiny/ofdrm.c                  |  16 +-
>>   drivers/gpu/drm/tiny/repaper.c                |   8 +-
>>   drivers/gpu/drm/tiny/simpledrm.c              |  43 +++-
>>   drivers/gpu/drm/tiny/st7586.c                 |  19 +-
>>   include/drm/drm_format_helper.h               |  81 +++++--
>>   include/drm/drm_gem_atomic_helper.h           |  10 +
>>   include/drm/drm_mipi_dbi.h                    |   4 +-
>>   15 files changed, 428 insertions(+), 144 deletions(-)
>>
>>
>> base-commit: 57d3b83a83c5527325efb5bcaf594da09fe4a41b

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