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

[Thomas Zimmermann]

Hi Javier

Am 29.09.23 um 10:27 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann at 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 at 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 at 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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20231004/b5e5792c/attachment.sig>