On 7/27/22 10:24, Thomas Zimmermann wrote:

Hi

Am 25.07.22 um 18:23 schrieb Javier Martinez Canillas:

quoted

On 7/20/22 16:27, Thomas Zimmermann wrote:

quoted

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.

Yes, I think is a step in the right direction and guess it would be even
more useful once/if a 3rd firmware-provided framebuffer driver is added.

quoted

[...]

quoted

+
+/**
+ * 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. :)

LOL.

Would 'back buffer' (in contrast to 'shadow buffer') be clear?

Back buffer is more clear indeed.

[...]

quoted

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.

I see. Makes sense.

quoted

[...]

quoted

+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.

OK.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat