Hi Thomas,

On Wed, Mar 13, 2024 at 9:19 AM Thomas Zimmermann [off-list ref] wrote:

Am 12.03.24 um 17:14 schrieb Geert Uytterhoeven:

quoted

On Tue, Mar 12, 2024 at 4:48 PM Thomas Zimmermann [off-list ref] wrote:

quoted

Add an fbdev emulation for SHMEM-based memory managers. The code is
similar to fbdev-generic, but does not require an addition shadow
buffer for mmap(). Fbdev-shmem operates directly on the buffer object's
SHMEM pages. Fbdev's deferred-I/O mechanism updates the hardware state
on write operations.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Thanks for your patch!

quoted

--- /dev/null
+++ b/drivers/gpu/drm/drm_fbdev_shmem.c
+static int drm_fbdev_shmem_helper_fb_probe(struct drm_fb_helper *fb_helper,
+                                          struct drm_fb_helper_surface_size *sizes)
+{
+       struct drm_client_dev *client = &fb_helper->client;
+       struct drm_device *dev = fb_helper->dev;
+       struct drm_client_buffer *buffer;
+       struct drm_gem_shmem_object *shmem;
+       struct drm_framebuffer *fb;
+       struct fb_info *info;
+       u32 format;
+       struct iosys_map map;
+       int ret;
+
+       drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n",
+                   sizes->surface_width, sizes->surface_height,
+                   sizes->surface_bpp);
+
+       format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth);

Oops, one more caller of the imprecise
let's-guess-the-format-from-bpp-and-depth machinery to get rid of...

Right, that has been discussed in another thread. I'll change this call
to the drm_driver_() function.

You mean drm_driver_legacy_fb_format()? That has the same issues.

quoted

quoted

+       buffer = drm_client_framebuffer_create(client, sizes->surface_width,
+                                              sizes->surface_height, format);

[...]

quoted

+}
+/**
+ * drm_fbdev_shmem_setup() - Setup fbdev emulation for GEM SHMEM helpers
+ * @dev: DRM device
+ * @preferred_bpp: Preferred bits per pixel for the device.
+ *                 32 is used if this is zero.
+ *
+ * This function sets up fbdev emulation for GEM DMA drivers that support
+ * dumb buffers with a virtual address and that can be mmap'ed.
+ * drm_fbdev_shmem_setup() shall be called after the DRM driver registered
+ * the new DRM device with drm_dev_register().
+ *
+ * Restore, hotplug events and teardown are all taken care of. Drivers that do
+ * suspend/resume need to call drm_fb_helper_set_suspend_unlocked() themselves.
+ * Simple drivers might use drm_mode_config_helper_suspend().
+ *
+ * This function is safe to call even when there are no connectors present.
+ * Setup will be retried on the next hotplug event.
+ *
+ * The fbdev is destroyed by drm_dev_unregister().
+ */
+void drm_fbdev_shmem_setup(struct drm_device *dev, unsigned int preferred_bpp)

As this is a completely new function, can we please get a
preferred_format parameter instead?

An understandable question. But as it is, the patchset has a trivial
change in each driver. And the preferred_bpp parameter has the same
meaning as the bpp value in the video= parameter. So it's ok-ish for now.

OK.

Using a format parameter here is really a much larger update and touches
the internals of the fbdev emulation. I'm not even sure that we should
have a parameter at all. Since in-kernel clients should behave like
userspace clients, we could try to figure out the format from the
driver's primary planes. That's a patchset of its own.

How do you figure out "the" format from the driver's primary plane?
Isn't that a list of formats (always including XR24) , so the driver
still needs to specify a preferred format?

A while ago, I had a look into replacing preferred_{depth,bpp} by
preferred_format, but I was held back by the inconsistencies in some
drivers (e.g. depth 24 vs. 32).  Perhaps an incremental approach
(use preferred_format if available, else fall back to legacy
preferred_{depth,bpp} handling) would be more suitable?

FTR, my main use-case is letting fbdev emulation distinguish between
DRM_FORMAT_Rx and DRM_FORMAT_Cx, which share the values of depth
and bpp.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds