Re: [PATCH v2 06/10] drm/simpledrm: Move some functionality into fwfb helper library
On 7/27/22 10:24, Thomas Zimmermann wrote:
> 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
>>> ---
>>
>> 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.
>
>>
>> [...]
>>
>>> +
>>> +/**
>>> + * 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.
[...]
>> 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.
>>
>> [...]
>>
>>> +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
Re: [PATCH v2 06/10] drm/simpledrm: Move some functionality into fwfb helper library
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
---
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
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v2 06/10] drm/simpledrm: Move some functionality into fwfb helper library
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
> ---
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
