Re: [PATCH v2 1/5] drm/format-helper: Add struct drm_xfrm_buf to cache format conversion

[Thomas Zimmermann <tzimmermann@suse.de>]

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

Hi Javier

Am 29.09.23 um 10:27 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
>> Hold temporary memory for format conversion in an instance of struct
>> drm_xfrm_buf. Update internal helpers of DRM's format-conversion code
>> accordingly. Drivers will later be able to keep this cache across
>> display updates.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> [...]
> 
>> +int drmm_xfrm_buf_init(struct drm_device *dev, struct drm_xfrm_buf *buf)
>> +{
>> +	buf->mem = NULL;
>> +	buf->size = 0;
>> +	buf->preallocated = false;
>> +
>> +	return drmm_add_action_or_reset(dev, drm_xfrm_buf_init_release, buf);
>> +}
>> +EXPORT_SYMBOL(drmm_xfrm_buf_init);
>> +
> 
> Can we find a better name than xfrm? I know that this is what's used in
> the internal drm_format_helper.c helpers but if we are exposing this to
> drivers, then I think that the naming is not self explanatory.
> 
>> +/**
>> + * drm_xfrm_buf_reserve - Allocates storage in an xfrm buffer
>> + * @buf: The xfrm buffer
> 
> At least in the kernel-doc we can say "The buffer used for pixel format
> conversion" or something along those lines.
> 
> [...]
> 
>> +/**
>> + * struct drm_xfrm_buf - Stores transformation and conversion state
>> + *
>> + * DRM helpers for format conversion store temporary state in
>> + * struct drm_xfrm_buf. The buffer's resources can be reused
> 
> And same here. Maybe struct drm_fmt_conversion_buf ?

I find this name to be unpleasant to read. Can we use 
drm_format_conv_state or drm_fmtcnv_state?

In the discussion about the panic handler, I mentioned that the struct 
can be used to store more inforamtion, such as palette entries or fg/bg 
colors. That would enable support for converting indexed formats, hence 
the _state postfix.

In the longer term, I'd also like to replace the drm_framebuffer from 
the API and then rename the functions to something like 
drm_fmtcnv_<x>_to_<y>(). The framebuffer really doesn't make much sense 
any longer.

Best regards
Thomas

> 
> Other than this nit, the patch looks good to me.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 

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