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!
[...]
> +
> +/**
> + * 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.
[...]
> +/**
> + * 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 ?
[...]
> +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.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
