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@suse.de>
>> ---
> 
> 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