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
[PATCH v2 06/10] drm/simpledrm: Move some functionality into fwfb helper library
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
---
Documentation/gpu/drm-kms-helpers.rst | 12 +
MAINTAINERS | 2 +
drivers/gpu/drm/Kconfig | 6 +
drivers/gpu/drm/Makefile | 3 +-
drivers/gpu/drm/drm_fwfb_helper.c | 301 ++
drivers/gpu/drm/tiny/Kconfig | 1 +
drivers/gpu/drm/tiny/simpledrm.c | 163 ++
include/drm/drm_fwfb_helper.h | 51 +
8 files changed, 398 insertions(+), 141 deletions(-)
create mode 100644 drivers/gpu/drm/drm_fwfb_helper.c
create mode 100644 include/drm/drm_fwfb_helper.h
diff --git a/Documentation/gpu/drm-kms-helpers.rst
b/Documentation/gpu/drm-kms-helpers.rst
index 2d473bc64c9f..0a8815b4243e 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -421,6 +421,18 @@ OF/DT Helpers
.. kernel-doc:: drivers/gpu/drm/drm_of.c
:export:
+Firmware Framebuffer Helper Reference
+=
+
+.. kernel-doc:: drivers/gpu/drm/drm_fwfb_helper.c
+ :doc: overview
+
+.. kernel-doc:: include/drm/drm_fwfb_helper.h
+ :internal:
+
+.. kernel-doc:: drivers/gpu/drm/drm_fwfb_helper.c
+ :export:
+
Legacy Plane Helper Reference
=
diff --git a/MAINTAINERS b/MAINTAINERS
index 0f9366144d31..138680415e79 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6572,9 +6572,11 @@ L: dri-de...@lists.freedesktop.org
S: Maintained
T: git git://anongit.freedesktop.org/drm/drm-misc
F: drivers/gpu/drm/drm_aperture.c
+F: drivers/gpu/drm/drm_fwfb_helper.c
F: drivers/gpu/drm/tiny/simpledrm.c
F: drivers/video/aperture.c
F: include/drm/drm_aperture.h
+F: include/drm/drm_fwfb_helper.h
F: include/linux/aperture.h
DRM DRIVER FOR SIS VIDEO CARDS
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 1c91e1e861a5..47a82705e052 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -78,6 +78,12 @@ config DRM_KMS_HELPER
help
CRTC helpers for KMS drivers.
+config DRM_FWFB_HELPER
+ bool
+ depends on DRM_KMS_HELPER
+ help
+ Helpers for firmware-provided framebuffers.
+
config DRM_DEBUG_DP_MST_TOPOLOGY_REFS
bool "Enable refcount backtrace history in the DP MST helpers"
depends on STACKTRACE_SUPPORT
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 25016dcab55e..be51018a3cbf 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -67,8 +67,9 @@ drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o \
drm_gem_framebuffer_helper.o \
drm_atomic_state_helper.o drm_damage_helper.o \
drm_format_helper.o drm_self_refresh_helper.o drm_rect.o
-drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
+drm_kms_helper-$(CONFIG_DRM_FWFB_HELPER) += drm_fwfb_helper.o
+drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
#
diff --git a/drivers/gpu/drm/drm_fwfb_helper.c
b/drivers/gpu/drm/drm_fwfb_helper.c
new file mode 100644
index ..522294f459c8
--- /dev/null
+++ b/drivers/gpu/drm/drm_fwfb_helper.c
@@ -0,0 +1,301 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/**
+ * 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.
+ */
+
+/*
+ * Assume a monitor resolution of 96 dpi to
+ * get a somewhat reasonable screen size.
+ */
+#define RES_MM(d) \
+ (((d) * 254ul) / (96ul * 10ul))
+
+#define DRM_FWFB_MODE(hd, vd) \
+ DRM_SIMPLE_MODE(hd, vd, RES_MM(hd), RES_MM(vd))
+
+static struct drm_display_mode drm_fwfb_mode(unsigned int width, unsigned int
height)
+{
+ struct drm_display_mode mode = { DRM_FWFB_MODE(width, height) };
+
+ mode.clock = mode.hdisplay * mode.vdisplay * 60 / 1000 /* kHz */;
+ drm_mode_set_name();
+
+ return mode;
+}
+
+/**
+ * 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,
+
