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

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi Javier

Am 29.09.23 um 10:27 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxx>
> > ---
>
> [...]
>
> > +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@xxxxxxxxxx>

--
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: OpenPGP_signature.asc
Description: OpenPGP digital signature