Re: [PATCH v2 0/5] drm: Reuse temporary memory for format conversion

[Thomas Zimmermann <tzimmermann@suse.de>]

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

Hi

Am 29.09.23 um 14:11 schrieb Maxime Ripard:
> On Wed, Sep 20, 2023 at 04:24:26PM +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.
>>
>> The motivation for this patchset is the recent work on a DRM panic
>> handler. 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.
>>
>> As an additonal benefit, drivers can now keep the temporary storage
>> across multiple display updates. Avoiding memory allocation reduces
>> the CPU overhead of the format helpers.
> 
> This argument is getting a bit tiring. The main reason is that it isn't
> one, and:

CPU overhead isn't the driver behind this patchset, but if it is 
affected, why not say that in the commit message? There's a alloc/free 
pair for each updated scanline. For a full-screen updates, that's quite 
a bit.

> 
>    - we allocate something in the 10-20 range objects at a given commit,
>      so another small one is not insignificant.
> 
>    - If it was, it would be trivial to produce a benchmark, but no-one
>      ever actually showed a workload and numbers where there's actually
>      any difference.
> 
>    - Also, the CPU overhead is indeed (even if marginally) decreased, but
>      the memory overhead is increased. I don't know whether that's a good
>      trade-off or not, see the point above.
> 
> It really sounds like an empty statement to me: "But just think of the
> CPU!".
> 
> That being said, I also have more fundamental doubts about this series.
> 
> The first one is that storing the buffer pointer in the device instead
> of the state makes it harder to reason about. When you have a state, the
> framework provides the guarantee at commit time that there's only going
> to be one at a given time. And since the buffer is stored in that state
> object, you can't access it by mistake. The buffer size also depends on
> the state, so that all makes sense from a logical PoV.

Yes. I discussed this with Javier already. Putting this into the state 
is the clean solution.

> 
> If we store the buffer into the device, then suddenly you have to think
> about whether there's multiple CRTCs or not (since commits aren't
> serialised if they affect different CRTCs), whether the buffer size you
> allocated is large enough now for your current resolution and format,
> etc. It adds a decent chunk of complexity on something that was quite
> complex already.

It's in the device because it's good enough for these simple drivers. 
The next best place would be a dedicated plane structure in each driver. 
The per-plane cache would then be clearly attributed to a single plane.

> 
> I understand that the motivation is for drm_panic to have a buffer ready
> when it has to kick in. But why don't we just allocate (and check) a
> spare commit at probe time so we just have to commit it when we panic.

DRM panic doesn't commit anything. It picks up whatever the current 
scanout buffer is and draws into that. If the DRM driver cannot provide 
a scanout buffer, DRM panic does nothing.

Best regards
Thomas

> 
> That would fall nicely into the rest of the atomic modesetting code, and
> since we pretty much require not to allocate anything during
> atomic_commit, we have that constraints already figured out.
> 
> 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)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]