Re: [PATCH 08/43] drm/fbdev: Add fbdev-shmem

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Thomas Zimmermann <tzimmermann@suse.de>
Cc: daniel@ffwll.ch, airlied@gmail.com, deller@gmx.de,
	javierm@redhat.com,  linux-fbdev@vger.kernel.org,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 08/43] drm/fbdev: Add fbdev-shmem
Date: Tue, 12 Mar 2024 17:14:46 +0100	[thread overview]
Message-ID: <CAMuHMdVqWF=M=6aPRxZ0MBqPVM_wk2x+Oike6za754riHWusSA@mail.gmail.com> (raw)
In-Reply-To: <20240312154834.26178-9-tzimmermann@suse.de>

Hi Thomas,

On Tue, Mar 12, 2024 at 4:48 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> 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!

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

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

[...]

> +}

> +/**
> + * 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?

> +{
> +       struct drm_fb_helper *fb_helper;
> +       int ret;
> +
> +       drm_WARN(dev, !dev->registered, "Device has not been registered.\n");
> +       drm_WARN(dev, dev->fb_helper, "fb_helper is already set!\n");
> +
> +       fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
> +       if (!fb_helper)
> +               return;
> +       drm_fb_helper_prepare(dev, fb_helper, preferred_bpp, &drm_fbdev_shmem_helper_funcs);
> +
> +       ret = drm_client_init(dev, &fb_helper->client, "fbdev", &drm_fbdev_shmem_client_funcs);
> +       if (ret) {
> +               drm_err(dev, "Failed to register client: %d\n", ret);
> +               goto err_drm_client_init;
> +       }
> +
> +       drm_client_register(&fb_helper->client);
> +
> +       return;
> +
> +err_drm_client_init:
> +       drm_fb_helper_unprepare(fb_helper);
> +       kfree(fb_helper);
> +}
> +EXPORT_SYMBOL(drm_fbdev_shmem_setup);

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