Hi Am 25.07.22 um 18:23 schrieb Javier Martinez Canillas:
On 7/20/22 16:27, Thomas Zimmermann wrote:Move some of simpledrm's functionality into a helper library. Other drivers for firmware-provided framebuffers will also need functions to handle fixed modes and color formats, or update the back buffer. Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx> ---Nice patch!
TBH it took me 3 tries to get something done for this library and I'm still not happy with the result. I want to share code between simpledrm and ofdrm, but that turns out to be harder then expected. A good part of this code appears to belong into other libraries (you also mentioned this below).
I don't want to duplicated code between simpledrm and ofdrm without reason, but I expect that this library will somewhen be refactored and dissolved into existing libraries.
[...]+ +/** + * DOC: overview + * + * The Firmware Framebuffer library FWFB provides helpers for devices with + * fixed-mode backing storage. It helps drivers to export a display mode of + * te correct size and copy updates to the backing storage.the it is "backing storage" or "backing store" ? I always thought that storage was used for non-volatile media while "store" could be volatile and non-volatile.
Why store? Isn't that a little shop for fashion or groceries? I'm no native speaker; I can't tell if either implies that we're sending pictures to a warehouse or bakery. :)
Would 'back buffer' (in contrast to 'shadow buffer') be clear?
[...]+/** + * drm_fwfb_init - Initializes an fwfb buffer + * @fwfb: fwfb buffer + * @screen_base: Address of the backing buffer in kernel address space + * @width: Number of pixels per scanline + * @height: Number of scanlines + * @format: Color format + * @pitch: Distance between two consecutive scanlines in bytes + * + * Returns: + * 0 on success, or a negative errno code otherwise. + */ +int drm_fwfb_init(struct drm_fwfb *fwfb, struct iosys_map *screen_base, + unsigned int width, unsigned int height, + const struct drm_format_info *format, unsigned int pitch) +{ + fwfb->screen_base = *screen_base; + fwfb->mode = drm_fwfb_mode(width, height); + fwfb->format = format;It seems a little bit arbitrary to me that format is the only field that's a pointer and the other ones are embedded into the struct drm_fwfb. Any reason for that or is just a consequence of how types were used by the simpledrm_device_create() function before that code moved into helpers ?
Format is constant and comes from statically initialized memory in drm_fourcc.c. I'd expect to be able to compare formats by comparing the pointers. Copying the format here would break the assumption.
[...]+static bool is_listed_fourcc(const uint32_t *fourccs, size_t nfourccs, uint32_t fourcc) +{ + const uint32_t *fourccs_end = fourccs + nfourccs; + + while (fourccs < fourccs_end) { + if (*fourccs == fourcc) + return true; + ++fourccs; + } + return false; +}This seems a helper that could be useful besides the drm_fwfb_helper.c file. I believe patches 1-6 shouldn't wait for the others in this series and could just be merged when ready. Patches 7-10 can follow later.
Yeah, I'd like to move patches 1 to 5 into a new series for merging. Patch 6 is only useful for ofdrm and as I said, maybe there's a better solution then this library. I'd rather keep it here for now.
Best regards Thomas
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
