On Wed, May 23, 2018 at 04:34:06PM +0200, Noralf Trønnes wrote:

This the beginning of an API for in-kernel clients. First out is a way to get a framebuffer backed by a dumb buffer.

Only GEM drivers are supported. The original idea of using an exported dma-buf was dropped because it also creates an anonomous file descriptor which doesn't work when the buffer is created from a kernel thread. The easy way out is to use drm_driver.gem_prime_vmap to get the virtual address, which requires a GEM object. This excludes the vmwgfx driver which is the only non-GEM driver apart from the legacy ones. A solution for vmwgfx will have to be worked out later if it wants to support the client API which it probably will when we have a bootsplash client.

Signed-off-by: Noralf Trønnes noralf@tronnes.org

A few small nits below, with those addressed:

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

Documentation/gpu/drm-client.rst | 12 ++ Documentation/gpu/index.rst | 1 + drivers/gpu/drm/Makefile | 2 +- drivers/gpu/drm/drm_client.c | 267 +++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_drv.c | 1 + include/drm/drm_client.h | 83 ++++++++++++ include/drm/drm_device.h | 7 + 7 files changed, 372 insertions(+), 1 deletion(-) create mode 100644 Documentation/gpu/drm-client.rst create mode 100644 drivers/gpu/drm/drm_client.c create mode 100644 include/drm/drm_client.h

diff --git a/Documentation/gpu/drm-client.rst b/Documentation/gpu/drm-client.rst new file mode 100644 index 000000000000..7e672063e7eb --- /dev/null +++ b/Documentation/gpu/drm-client.rst @@ -0,0 +1,12 @@ +================= +Kernel clients +=================

+.. kernel-doc:: drivers/gpu/drm/drm_client.c

• :doc: overview

+.. kernel-doc:: include/drm/drm_client.h

• :internal:

+.. kernel-doc:: drivers/gpu/drm/drm_client.c

• :export:

diff --git a/Documentation/gpu/index.rst b/Documentation/gpu/index.rst index 00288f34c5a6..1fcf8e851e15 100644 --- a/Documentation/gpu/index.rst +++ b/Documentation/gpu/index.rst @@ -10,6 +10,7 @@ Linux GPU Driver Developer's Guide drm-kms drm-kms-helpers drm-uapi

• drm-client drivers vga-switcheroo vgaarbiter

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index ef9f3dab287f..8c8045147416 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -18,7 +18,7 @@ drm-y := drm_auth.o drm_bufs.o drm_cache.o \ drm_encoder.o drm_mode_object.o drm_property.o \ drm_plane.o drm_color_mgmt.o drm_print.o \ drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \

• drm_syncobj.o drm_lease.o
  

• drm_syncobj.o drm_lease.o drm_client.o
  
  

drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o drm-$(CONFIG_DRM_VM) += drm_vm.o diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c new file mode 100644 index 000000000000..0919aea7dddd --- /dev/null +++ b/drivers/gpu/drm/drm_client.c @@ -0,0 +1,267 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright 2018 Noralf Trønnes
• */

+#include <linux/list.h> +#include <linux/mutex.h> +#include <linux/seq_file.h> +#include <linux/slab.h>

+#include <drm/drm_client.h> +#include <drm/drm_debugfs.h> +#include <drm/drm_device.h> +#include <drm/drm_drv.h> +#include <drm/drm_file.h> +#include <drm/drm_fourcc.h> +#include <drm/drm_gem.h> +#include <drm/drm_mode.h> +#include <drm/drm_print.h> +#include <drm/drmP.h>

+#include "drm_crtc_internal.h" +#include "drm_internal.h"

+/**

• • DOC: overview
• • This library provides support for clients running in the kernel like fbdev and bootsplash.
• • Currently it's only partially implemented, just enough to support fbdev.
• • GEM drivers which provide a GEM based dumb buffer with a virtual address are supported.
• */

+static int drm_client_alloc_file(struct drm_client_dev *client) +{

• struct drm_device *dev = client->dev;
• struct drm_file *file;
• file = drm_file_alloc(dev->primary);
• if (IS_ERR(file))

• return PTR_ERR(file);
  

• drm_dev_get(dev);
• mutex_lock(&dev->filelist_mutex);
• list_add(&file->lhead, &dev->filelist_internal);
• mutex_unlock(&dev->filelist_mutex);
• client->file = file;
• return 0;

+}

+static void drm_client_free_file(struct drm_client_dev *client) +{

• struct drm_device *dev = client->dev;
• mutex_lock(&dev->filelist_mutex);
• list_del(&client->file->lhead);
• mutex_unlock(&dev->filelist_mutex);
• drm_file_free(client->file);
• drm_dev_put(dev);

+}

+/**

• • drm_client_new - Create a DRM client
• • @dev: DRM device
• • Returns:
• • Pointer to a client or an error pointer on failure.
• */

+struct drm_client_dev *drm_client_new(struct drm_device *dev)

Api nitpick:

int drm_client_init(struct drm_device *dev, struct drm_client_dev *client)

and dropping the kzalloc from this structure here. This allows users of this to embed the client struct into their own thing, which means the ->private backpointer isn't necessary. Allowing embedding is the preferred interface in the kernel (since it's strictly more powerful, you can always just kzalloc + _init to get the _new behaviour).

+{

• struct drm_client_dev *client;
• int ret;
• if (!drm_core_check_feature(dev, DRIVER_MODESET) ||

•    !dev->driver->dumb_create || !dev->driver->gem_prime_vmap)
  

• return ERR_PTR(-ENOTSUPP);
  

• client = kzalloc(sizeof(*client), GFP_KERNEL);
• if (!client)

• return ERR_PTR(-ENOMEM);
  

• client->dev = dev;
• ret = drm_client_alloc_file(client);
• if (ret) {

• kfree(client);
  

• return ERR_PTR(ret);
  

• }
• return client;

+} +EXPORT_SYMBOL(drm_client_new);

+/**

• • drm_client_free - Free DRM client resources
• • @client: DRM client
• */

+void drm_client_free(struct drm_client_dev *client)

Similar here, _release instead of _free and drop the kfree.

+{

• drm_client_free_file(client);
• kfree(client);

+} +EXPORT_SYMBOL(drm_client_free);

+static void drm_client_buffer_delete(struct drm_client_buffer *buffer) +{

• struct drm_device *dev;
• if (!buffer)

• return;
  

• dev = buffer->client->dev;
• if (buffer->vaddr && dev->driver->gem_prime_vunmap)

• dev->driver->gem_prime_vunmap(buffer->gem, buffer->vaddr);
  

• if (buffer->gem)

• drm_gem_object_put_unlocked(buffer->gem);
  

• drm_mode_destroy_dumb(dev, buffer->handle, buffer->client->file);
• kfree(buffer);

+}

+static struct drm_client_buffer * +drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u32 format) +{

• struct drm_mode_create_dumb dumb_args = { };
• struct drm_device *dev = client->dev;
• struct drm_client_buffer *buffer;
• struct drm_gem_object *obj;
• void *vaddr;
• int ret;
• buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
• if (!buffer)

• return ERR_PTR(-ENOMEM);
  

• buffer->client = client;
• dumb_args.width = width;
• dumb_args.height = height;
• dumb_args.bpp = drm_format_plane_cpp(format, 0) * 8;
• ret = drm_mode_create_dumb(dev, &dumb_args, client->file);
• if (ret)

• goto err_free;
  

• buffer->handle = dumb_args.handle;
• buffer->pitch = dumb_args.pitch;
• obj = drm_gem_object_lookup(client->file, dumb_args.handle);
• if (!obj) {

• ret = -ENOENT;
  

• goto err_delete;
  

• }
• buffer->gem = obj;

I'm paranoid, I think an

if (WARN_ON(!gem_prime_vmap)) return -EINVAL;

would be cool here.

Also perhaps the following comment:

/* * FIXME: The dependency on GEM here isn't required, we could * convert the driver handle to a dma-buf instead and use the * backend-agnostic dma-buf vmap support instead. This would * require that the handle2fd prime ioctl is reworked to pull the * fd_install step out of the driver backend hooks, to make that * final step optional for internal users. */

• vaddr = dev->driver->gem_prime_vmap(obj);
• if (!vaddr) {

• ret = -ENOMEM;
  

• goto err_delete;
  

• }
• buffer->vaddr = vaddr;
• return buffer;

+err_delete:

• drm_client_buffer_delete(buffer);

+err_free:

• kfree(buffer);
• return ERR_PTR(ret);

+}

+static void drm_client_buffer_rmfb(struct drm_client_buffer *buffer) +{

• int ret;
• if (!buffer || !buffer->fb)

• return;
  

• ret = drm_mode_rmfb(buffer->client->dev, buffer->fb->base.id, buffer->client->file);
• if (ret)

• DRM_DEV_ERROR(buffer->client->dev->dev,
  

• 	      "Error removing FB:%u (%d)\n", buffer->fb->base.id, ret);
  

• buffer->fb = NULL;

+}

+static int drm_client_buffer_addfb(struct drm_client_buffer *buffer,

• 		   u32 width, u32 height, u32 format)
  

+{

• struct drm_client_dev *client = buffer->client;
• struct drm_mode_fb_cmd fb_req = { };
• const struct drm_format_info *info;
• int ret;
• info = drm_format_info(format);
• fb_req.bpp = info->cpp[0] * 8;
• fb_req.depth = info->depth;
• fb_req.width = width;
• fb_req.height = height;
• fb_req.handle = buffer->handle;
• fb_req.pitch = buffer->pitch;
• ret = drm_mode_addfb(client->dev, &fb_req, client->file);
• if (ret)

• return ret;
  

• buffer->fb = drm_framebuffer_lookup(client->dev, buffer->client->file, fb_req.fb_id);
• if (WARN_ON(!buffer->fb))

• return -ENOENT;
  

• /* drop the reference we picked up in framebuffer lookup */
• drm_framebuffer_put(buffer->fb);
• return 0;

+}

+/**

• • drm_client_framebuffer_create - Create a client framebuffer
• • @client: DRM client
• • @width: Framebuffer width
• • @height: Framebuffer height
• • @format: Buffer format
• • This function creates a &drm_client_buffer which consists of a
• • &drm_framebuffer backed by a dumb buffer.
• • Call drm_client_framebuffer_delete() to free the buffer.
• • Returns:
• • Pointer to a client buffer or an error pointer on failure.
• */

+struct drm_client_buffer * +drm_client_framebuffer_create(struct drm_client_dev *client, u32 width, u32 height, u32 format) +{

• struct drm_client_buffer *buffer;
• int ret;
• buffer = drm_client_buffer_create(client, width, height, format);
• if (IS_ERR(buffer))

• return buffer;
  

• ret = drm_client_buffer_addfb(buffer, width, height, format);
• if (ret) {

• drm_client_buffer_delete(buffer);
  

• return ERR_PTR(ret);
  

• }
• return buffer;

+} +EXPORT_SYMBOL(drm_client_framebuffer_create);

+/**

• • drm_client_framebuffer_delete - Delete a client framebuffer
• • @buffer: DRM client buffer
• */

+void drm_client_framebuffer_delete(struct drm_client_buffer *buffer) +{

Style nit: I'd pull the

if (!buffer) return;

Case out of the below 2 functions and into this one here: Only here it's needed, and it makes it a bit more obvious that this function does the right thing for NULL buffers.

Also please explain this in the kerneldoc.

• drm_client_buffer_rmfb(buffer);
• drm_client_buffer_delete(buffer);

+} +EXPORT_SYMBOL(drm_client_framebuffer_delete); diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index f6910ebe4d0e..67ac793a7108 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -505,6 +505,7 @@ int drm_dev_init(struct drm_device *dev, dev->driver = driver;

INIT_LIST_HEAD(&dev->filelist);

• INIT_LIST_HEAD(&dev->filelist_internal); INIT_LIST_HEAD(&dev->ctxlist); INIT_LIST_HEAD(&dev->vmalist); INIT_LIST_HEAD(&dev->maplist);

diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h new file mode 100644 index 000000000000..11379eaf3118 --- /dev/null +++ b/include/drm/drm_client.h @@ -0,0 +1,83 @@ +/* SPDX-License-Identifier: GPL-2.0 */

+#ifndef _DRM_CLIENT_H_ +#define _DRM_CLIENT_H_

+#include <linux/types.h>

+struct drm_device; +struct drm_framebuffer; +struct drm_gem_object; +struct drm_minor;

+/**

• • struct drm_client_dev - DRM client instance
• */

+struct drm_client_dev {

• /**

• * @list:
  

• *
  

• * List of all open client files of a DRM device, linked into
  

• * &drm_device.filelist_internal. Protected by &drm_device.filelist_mutex.
  

• */
  

• struct list_head list;
• /**

• * @dev: DRM device
  

• */
  

• struct drm_device *dev;
• /**

• * @file: DRM file
  

• */
  

• struct drm_file *file;
• /**

• * @private: Optional pointer to client private data
  

• */
  

• void *private;

Not needed if we go with _init and _release because of embedding.

+};

+void drm_client_free(struct drm_client_dev *client); +struct drm_client_dev *drm_client_new(struct drm_device *dev);

+/**

• • struct drm_client_buffer - DRM client buffer
• */

+struct drm_client_buffer {

• /**

• * @client: DRM client
  

• */
  

• struct drm_client_dev *client;
• /**

• * @handle: Buffer handle
  

• */
  

• u32 handle;
• /**

• * @pitch: Buffer pitch
  

• */
  

• u32 pitch;
• /**

• * @gem: GEM object backing this buffer
  

• */
  

• struct drm_gem_object *gem;
• /**

• * @vaddr: Virtual address for the buffer
  

• */
  

• void *vaddr;
• /**

• * @fb: DRM framebuffer
  

• */
  

• struct drm_framebuffer *fb;

+};

+struct drm_client_buffer * +drm_client_framebuffer_create(struct drm_client_dev *client, u32 width, u32 height, u32 format); +void drm_client_framebuffer_delete(struct drm_client_buffer *buffer);

+#endif diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h index 858ba19a3e29..9e29976d4e98 100644 --- a/include/drm/drm_device.h +++ b/include/drm/drm_device.h @@ -74,6 +74,13 @@ struct drm_device { struct mutex filelist_mutex; struct list_head filelist;

• /**

• * @filelist_internal:
  

• *
  

• * List of open DRM files for in-kernel clients. Protected by @filelist_mutex.
  

• */
  

• struct list_head filelist_internal;
• /** \name Memory management */ /*@{ */ struct list_head maplist; /**< Linked list of regions */

-- 2.15.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Wed, May 23, 2018 at 04:34:07PM +0200, Noralf Trønnes wrote:

This is the first step in getting generic fbdev emulation. A drm_fb_helper_funcs.fb_probe function is added which uses the DRM client API to get a framebuffer backed by a dumb buffer.

A transitional hack for tinydrm is needed in order to switch over all CMA helper drivers in a later patch. This hack will be removed when tinydrm moves to vmalloc buffers.

Signed-off-by: Noralf Trønnes noralf@tronnes.org

drivers/gpu/drm/drm_fb_helper.c | 164 ++++++++++++++++++++++++++++++++++++++++ include/drm/drm_fb_helper.h | 26 +++++++ 2 files changed, 190 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 2ee1eaa66188..444c2b4040ea 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -30,11 +30,13 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

#include <linux/console.h> +#include <linux/dma-buf.h> #include <linux/kernel.h> #include <linux/sysrq.h> #include <linux/slab.h> #include <linux/module.h> #include <drm/drmP.h> +#include <drm/drm_client.h> #include <drm/drm_crtc.h> #include <drm/drm_fb_helper.h> #include <drm/drm_crtc_helper.h> @@ -2928,6 +2930,168 @@ void drm_fb_helper_output_poll_changed(struct drm_device *dev) } EXPORT_SYMBOL(drm_fb_helper_output_poll_changed);

+/* @user: 1=userspace, 0=fbcon */ +static int drm_fbdev_fb_open(struct fb_info *info, int user) +{

• struct drm_fb_helper *fb_helper = info->par;
• if (!try_module_get(fb_helper->dev->driver->fops->owner))

• return -ENODEV;
  

• return 0;

+}

+static int drm_fbdev_fb_release(struct fb_info *info, int user) +{

• struct drm_fb_helper *fb_helper = info->par;
• module_put(fb_helper->dev->driver->fops->owner);
• return 0;

+}

Hm, I thought earlier versions of your patch series had this separately, for everyone. What's the reasons for merging this into the fb_probe implementation.

+/*

• • fb_ops.fb_destroy is called by the last put_fb_info() call at the end of
• • unregister_framebuffer() or fb_release().
• */

+static void drm_fbdev_fb_destroy(struct fb_info *info) +{

• struct drm_fb_helper *fb_helper = info->par;
• struct fb_ops *fbops = NULL;
• DRM_DEBUG("\n");
• if (fb_helper->fbdev->fbdefio) {

• fb_deferred_io_cleanup(fb_helper->fbdev);
  

• fbops = fb_helper->fbdev->fbops;
  

• }
• drm_fb_helper_fini(fb_helper);
• drm_client_framebuffer_delete(fb_helper->buffer);
• drm_client_free(fb_helper->client);
• kfree(fb_helper);
• kfree(fbops);

+}

Hm, if we go with the idea that drm_clients could auto-unregister through a callback, then I expect this will lead to some control inversion. But we can fix that later on.

+static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) +{

• struct drm_fb_helper *fb_helper = info->par;
• if (fb_helper->dev->driver->gem_prime_mmap)

• return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma);
  

• else

• return -ENODEV;
  

+}

+static struct fb_ops drm_fbdev_fb_ops = {

• /* No need to set owner, this module is already pinned by the driver. */

I'd still set it, means less thinking since more obviously correct.

• DRM_FB_HELPER_DEFAULT_OPS,
• .fb_open = drm_fbdev_fb_open,
• .fb_release = drm_fbdev_fb_release,
• .fb_destroy = drm_fbdev_fb_destroy,
• .fb_mmap = drm_fbdev_fb_mmap,
• .fb_read = drm_fb_helper_sys_read,
• .fb_write = drm_fb_helper_sys_write,
• .fb_fillrect = drm_fb_helper_sys_fillrect,
• .fb_copyarea = drm_fb_helper_sys_copyarea,
• .fb_imageblit = drm_fb_helper_sys_imageblit,

Hm, some drivers require the cfb versions of these. In practice I guess there's not much of a difference really, at least on x86 and arm.

We might want to document that though.

+};

+static struct fb_deferred_io drm_fbdev_defio = {

• .delay = HZ / 20,
• .deferred_io = drm_fb_helper_deferred_io,

+};

+/*

• • TODO: Remove this when tinydrm is converted to vmalloc buffers.
• */

+static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,

• 			  struct vm_area_struct *vma)
  

+{

• fb_deferred_io_mmap(info, vma);
• vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
• return 0;

+}

Needs kerneldoc here.

+int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,

• 		struct drm_fb_helper_surface_size *sizes)
  

+{

• struct drm_client_dev *client = fb_helper->client;
• struct drm_client_buffer *buffer;
• struct drm_framebuffer *fb;
• struct fb_info *fbi;
• u32 format;
• int ret;
• DRM_DEBUG_KMS("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);
• buffer = drm_client_framebuffer_create(client, sizes->surface_width,

• 			       sizes->surface_height, format);
  

• if (IS_ERR(buffer))

• return PTR_ERR(buffer);
  

• fb_helper->buffer = buffer;
• fb_helper->fb = buffer->fb;
• fb = buffer->fb;
• fbi = drm_fb_helper_alloc_fbi(fb_helper);
• if (IS_ERR(fbi)) {

• ret = PTR_ERR(fbi);
  

• goto err_free_buffer;
  

• }
• fbi->par = fb_helper;
• fbi->fbops = &drm_fbdev_fb_ops;
• fbi->screen_size = fb->height * fb->pitches[0];
• fbi->fix.smem_len = fbi->screen_size;
• fbi->screen_buffer = buffer->vaddr;
• strcpy(fbi->fix.id, "DRM emulated");
• drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth);
• drm_fb_helper_fill_var(fbi, fb_helper, sizes->fb_width, sizes->fb_height);
• if (fb->funcs->dirty) {

• struct fb_ops *fbops;
  

• /*
  

•  * fb_deferred_io_cleanup() clears &fbops->fb_mmap so a per
  

•  * instance version is necessary.
  

•  */
  

• fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
  

• if (!fbops) {
  

• 	ret = -ENOMEM;
  

• 	goto err_fb_info_destroy;
  

• }
  

• *fbops = *fbi->fbops;
  

• fbi->fbops = fbops;
  

• fbi->fbdefio = &drm_fbdev_defio;
  

• /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */
  

• fbi->fix.smem_start = page_to_phys(virt_to_page(buffer->vaddr));
  

• fb_deferred_io_init(fbi);
  

• /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */
  

• fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap;
  

• }

Ugh. Yeah defio and generic allocator through dumb buffers don't mix well. The only true generic solution for this would be to give userspace (and only userspace, for fbcon we can intercept everything) a staging buffer, and then upload things using the dirty callback.

But that introduces another copy step, so isn't cool.

I think a check for is_vmalloc_addr and if that's false, not doing any of the defio mmap setup would be good. Until we have a better idea. And yes that would need to be done after tinydrm is moved over.

• return 0;

+err_fb_info_destroy:

• drm_fb_helper_fini(fb_helper);

+err_free_buffer:

• drm_client_framebuffer_delete(buffer);
• return ret;

+} +EXPORT_SYMBOL(drm_fb_helper_generic_probe);

/* The Kconfig DRM_KMS_HELPER selects FRAMEBUFFER_CONSOLE (if !EXPERT)

• but the module doesn't depend on any fb console symbols. At least
• attempt to load fbcon to avoid leaving the system without a usable console.

diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index b069433e7fc1..bb38469a9502 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -30,6 +30,8 @@ #ifndef DRM_FB_HELPER_H #define DRM_FB_HELPER_H

+struct drm_client_buffer; +struct drm_client_dev; struct drm_fb_helper;

#include <drm/drm_crtc.h> @@ -232,6 +234,20 @@ struct drm_fb_helper { * See also: @deferred_setup */ int preferred_bpp;

• /**

• * @client:
  

• *
  

• * DRM client used by the generic fbdev emulation.
  

• */
  

• struct drm_client_dev *client;
• /**

• * @buffer:
  

• *
  

• * Framebuffer used by the generic fbdev emulation.
  

• */
  

• struct drm_client_buffer *buffer;

};

/** @@ -330,6 +346,9 @@ void drm_fb_helper_fbdev_teardown(struct drm_device *dev);

void drm_fb_helper_lastclose(struct drm_device *dev); void drm_fb_helper_output_poll_changed(struct drm_device *dev);

+int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,

• 		struct drm_fb_helper_surface_size *sizes);
  

#else static inline void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper, @@ -564,6 +583,13 @@ static inline void drm_fb_helper_output_poll_changed(struct drm_device *dev) { }

+static inline int +drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,

• 	    struct drm_fb_helper_surface_size *sizes)
  

+{

• return 0;

+}

Ok, I think this patch looks ok. With the missing kerneldoc added (which also explains the current limitations) this is

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

#endif

static inline int

2.15.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

Den 24.05.2018 11.16, skrev Daniel Vetter:

On Wed, May 23, 2018 at 04:34:07PM +0200, Noralf Trønnes wrote:

...

This is the first step in getting generic fbdev emulation. A drm_fb_helper_funcs.fb_probe function is added which uses the DRM client API to get a framebuffer backed by a dumb buffer.

A transitional hack for tinydrm is needed in order to switch over all CMA helper drivers in a later patch. This hack will be removed when tinydrm moves to vmalloc buffers.

Signed-off-by: Noralf Trønnes noralf@tronnes.org

drivers/gpu/drm/drm_fb_helper.c | 164 ++++++++++++++++++++++++++++++++++++++++ include/drm/drm_fb_helper.h | 26 +++++++ 2 files changed, 190 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 2ee1eaa66188..444c2b4040ea 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -30,11 +30,13 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

#include <linux/console.h> +#include <linux/dma-buf.h> #include <linux/kernel.h> #include <linux/sysrq.h> #include <linux/slab.h> #include <linux/module.h> #include <drm/drmP.h> +#include <drm/drm_client.h> #include <drm/drm_crtc.h> #include <drm/drm_fb_helper.h> #include <drm/drm_crtc_helper.h> @@ -2928,6 +2930,168 @@ void drm_fb_helper_output_poll_changed(struct drm_device *dev) } EXPORT_SYMBOL(drm_fb_helper_output_poll_changed);

+/* @user: 1=userspace, 0=fbcon */ +static int drm_fbdev_fb_open(struct fb_info *info, int user) +{

• struct drm_fb_helper *fb_helper = info->par;
• if (!try_module_get(fb_helper->dev->driver->fops->owner))

• return -ENODEV;
  

• return 0;

+}

+static int drm_fbdev_fb_release(struct fb_info *info, int user) +{

• struct drm_fb_helper *fb_helper = info->par;
• module_put(fb_helper->dev->driver->fops->owner);
• return 0;

+}

Hm, I thought earlier versions of your patch series had this separately, for everyone. What's the reasons for merging this into the fb_probe implementation.

This is only necessary when struct fb_ops is defined in a library where the owner field is the library module and not the driver module. I realised that with this generic version it's highly unlikely that we get another library that defines struct fb_ops, so it's only needed here.

Noralf.

...

+/*

• • fb_ops.fb_destroy is called by the last put_fb_info() call at the end of
• • unregister_framebuffer() or fb_release().
• */

+static void drm_fbdev_fb_destroy(struct fb_info *info) +{

• struct drm_fb_helper *fb_helper = info->par;
• struct fb_ops *fbops = NULL;
• DRM_DEBUG("\n");
• if (fb_helper->fbdev->fbdefio) {

• fb_deferred_io_cleanup(fb_helper->fbdev);
  

• fbops = fb_helper->fbdev->fbops;
  

• }
• drm_fb_helper_fini(fb_helper);
• drm_client_framebuffer_delete(fb_helper->buffer);
• drm_client_free(fb_helper->client);
• kfree(fb_helper);
• kfree(fbops);

+}

Hm, if we go with the idea that drm_clients could auto-unregister through a callback, then I expect this will lead to some control inversion. But we can fix that later on.

...

+static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) +{

• struct drm_fb_helper *fb_helper = info->par;
• if (fb_helper->dev->driver->gem_prime_mmap)

• return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma);
  

• else

• return -ENODEV;
  

+}

+static struct fb_ops drm_fbdev_fb_ops = {

• /* No need to set owner, this module is already pinned by the driver. */

I'd still set it, means less thinking since more obviously correct.

...

• DRM_FB_HELPER_DEFAULT_OPS,
• .fb_open = drm_fbdev_fb_open,
• .fb_release = drm_fbdev_fb_release,
• .fb_destroy = drm_fbdev_fb_destroy,
• .fb_mmap = drm_fbdev_fb_mmap,
• .fb_read = drm_fb_helper_sys_read,
• .fb_write = drm_fb_helper_sys_write,
• .fb_fillrect = drm_fb_helper_sys_fillrect,
• .fb_copyarea = drm_fb_helper_sys_copyarea,
• .fb_imageblit = drm_fb_helper_sys_imageblit,

Hm, some drivers require the cfb versions of these. In practice I guess there's not much of a difference really, at least on x86 and arm.

We might want to document that though.

...

+};

+static struct fb_deferred_io drm_fbdev_defio = {

• .delay = HZ / 20,
• .deferred_io = drm_fb_helper_deferred_io,

+};

+/*

• • TODO: Remove this when tinydrm is converted to vmalloc buffers.
• */

+static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,

• 			  struct vm_area_struct *vma)
  

+{

• fb_deferred_io_mmap(info, vma);
• vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
• return 0;

+}

Needs kerneldoc here.

...

+int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,

• 		struct drm_fb_helper_surface_size *sizes)
  

+{

• struct drm_client_dev *client = fb_helper->client;
• struct drm_client_buffer *buffer;
• struct drm_framebuffer *fb;
• struct fb_info *fbi;
• u32 format;
• int ret;
• DRM_DEBUG_KMS("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);
• buffer = drm_client_framebuffer_create(client, sizes->surface_width,

• 			       sizes->surface_height, format);
  

• if (IS_ERR(buffer))

• return PTR_ERR(buffer);
  

• fb_helper->buffer = buffer;
• fb_helper->fb = buffer->fb;
• fb = buffer->fb;
• fbi = drm_fb_helper_alloc_fbi(fb_helper);
• if (IS_ERR(fbi)) {

• ret = PTR_ERR(fbi);
  

• goto err_free_buffer;
  

• }
• fbi->par = fb_helper;
• fbi->fbops = &drm_fbdev_fb_ops;
• fbi->screen_size = fb->height * fb->pitches[0];
• fbi->fix.smem_len = fbi->screen_size;
• fbi->screen_buffer = buffer->vaddr;
• strcpy(fbi->fix.id, "DRM emulated");
• drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth);
• drm_fb_helper_fill_var(fbi, fb_helper, sizes->fb_width, sizes->fb_height);
• if (fb->funcs->dirty) {

• struct fb_ops *fbops;
  

• /*
  

•  * fb_deferred_io_cleanup() clears &fbops->fb_mmap so a per
  

•  * instance version is necessary.
  

•  */
  

• fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
  

• if (!fbops) {
  

• 	ret = -ENOMEM;
  

• 	goto err_fb_info_destroy;
  

• }
  

• *fbops = *fbi->fbops;
  

• fbi->fbops = fbops;
  

• fbi->fbdefio = &drm_fbdev_defio;
  

• /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */
  

• fbi->fix.smem_start = page_to_phys(virt_to_page(buffer->vaddr));
  

• fb_deferred_io_init(fbi);
  

• /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */
  

• fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap;
  

• }

Ugh. Yeah defio and generic allocator through dumb buffers don't mix well. The only true generic solution for this would be to give userspace (and only userspace, for fbcon we can intercept everything) a staging buffer, and then upload things using the dirty callback.

But that introduces another copy step, so isn't cool.

I think a check for is_vmalloc_addr and if that's false, not doing any of the defio mmap setup would be good. Until we have a better idea. And yes that would need to be done after tinydrm is moved over.

...

• return 0;

+err_fb_info_destroy:

• drm_fb_helper_fini(fb_helper);

+err_free_buffer:

• drm_client_framebuffer_delete(buffer);
• return ret;

+} +EXPORT_SYMBOL(drm_fb_helper_generic_probe);

• /* The Kconfig DRM_KMS_HELPER selects FRAMEBUFFER_CONSOLE (if !EXPERT)
  • but the module doesn't depend on any fb console symbols. At least
  • attempt to load fbcon to avoid leaving the system without a usable console.

diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index b069433e7fc1..bb38469a9502 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -30,6 +30,8 @@ #ifndef DRM_FB_HELPER_H #define DRM_FB_HELPER_H

+struct drm_client_buffer; +struct drm_client_dev; struct drm_fb_helper;

#include <drm/drm_crtc.h> @@ -232,6 +234,20 @@ struct drm_fb_helper { * See also: @deferred_setup */ int preferred_bpp;

• /**

• * @client:
  

• *
  

• * DRM client used by the generic fbdev emulation.
  

• */
  

• struct drm_client_dev *client;

• /**

• * @buffer:
  

• *
  

• * Framebuffer used by the generic fbdev emulation.
  

• */
  

• struct drm_client_buffer *buffer; };

  /**

@@ -330,6 +346,9 @@ void drm_fb_helper_fbdev_teardown(struct drm_device *dev);

void drm_fb_helper_lastclose(struct drm_device *dev); void drm_fb_helper_output_poll_changed(struct drm_device *dev);

+int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,

• 		struct drm_fb_helper_surface_size *sizes);
  

  #else static inline void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,

@@ -564,6 +583,13 @@ static inline void drm_fb_helper_output_poll_changed(struct drm_device *dev) { }

+static inline int +drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,

• 	    struct drm_fb_helper_surface_size *sizes)
  

+{

• return 0;

+}

Ok, I think this patch looks ok. With the missing kerneldoc added (which also explains the current limitations) this is

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

...

• #endif

  static inline int

-- 2.15.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Tue, May 29, 2018 at 9:54 AM Daniel Vetter daniel@ffwll.ch wrote:

On Fri, May 25, 2018 at 02:42:02PM +0200, Noralf Trønnes wrote:

...

Den 24.05.2018 11.16, skrev Daniel Vetter:

...

On Wed, May 23, 2018 at 04:34:07PM +0200, Noralf Trønnes wrote:

...

This is the first step in getting generic fbdev emulation. A drm_fb_helper_funcs.fb_probe function is added which uses the DRM client API to get a framebuffer backed by a dumb buffer.

A transitional hack for tinydrm is needed in order to switch over all CMA helper drivers in a later patch. This hack will be removed when tinydrm moves to vmalloc buffers.

Signed-off-by: Noralf Trønnes noralf@tronnes.org

drivers/gpu/drm/drm_fb_helper.c | 164 ++++++++++++++++++++++++++++++++++++++++ include/drm/drm_fb_helper.h | 26 +++++++ 2 files changed, 190 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 2ee1eaa66188..444c2b4040ea 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -30,11 +30,13 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt #include <linux/console.h> +#include <linux/dma-buf.h> #include <linux/kernel.h> #include <linux/sysrq.h> #include <linux/slab.h> #include <linux/module.h> #include <drm/drmP.h> +#include <drm/drm_client.h> #include <drm/drm_crtc.h> #include <drm/drm_fb_helper.h> #include <drm/drm_crtc_helper.h> @@ -2928,6 +2930,168 @@ void drm_fb_helper_output_poll_changed(struct drm_device *dev) } EXPORT_SYMBOL(drm_fb_helper_output_poll_changed); +/* @user: 1=userspace, 0=fbcon */ +static int drm_fbdev_fb_open(struct fb_info *info, int user) +{

• struct drm_fb_helper *fb_helper = info->par;
• if (!try_module_get(fb_helper->dev->driver->fops->owner))

•     return -ENODEV;
  

• return 0;

+}

+static int drm_fbdev_fb_release(struct fb_info *info, int user) +{

• struct drm_fb_helper *fb_helper = info->par;
• module_put(fb_helper->dev->driver->fops->owner);
• return 0;

+}

Hm, I thought earlier versions of your patch series had this separately, for everyone. What's the reasons for merging this into the fb_probe implementation.

This is only necessary when struct fb_ops is defined in a library where the owner field is the library module and not the driver module. I realised that with this generic version it's highly unlikely that we get another library that defines struct fb_ops, so it's only needed here.

Hm, I'm still not 100% convinced we can fully subsume the tinydrm fbdev code with the generic one, due to the varios slight issues around defio tracking that we still have.

I have a new idea for 100% generic defio support in the fbdev helpers. Haven't tried it thought, but I think this could work around the problem if your mmap isn't struct page backed:

- In the drm_fbdev_fb_mmap helper, if we need defio, change the default page prots to write-protected with something like this:

vma->vm_page_prot = pgprot_wrprotect(vma->vm_page_prot);

- After the driver's mmap function completed, copy the vm_ops structure and WARN_ON if it has an mkwrite and sync callback set. There's no real race here as long as we do all this before we return to userspace.

- Set the mkwrite and fsync callbacks with similar implementions to the core fbdev defio stuff. These should all work on plain ptes, they don't actually require a struct page. uff. These should all work on plain ptes, they don't actually require a struct page.

- We might also need to overwrite the vma_ops fault callback, just forwarding to the underlying vma_ops instead of copying them would be cleaner. The overhead won't matter, especially not for defio drivers.

- Also copypaste all the other bits of the core fbdev defio code we'll need, that would allow us to also clean up the cleanup() warts ...

All of this would ofc only be done if the fb has a ->dirty callback.

We can also just stuff this into todo.rst.

Cheers, Daniel

But it's also easy to export this later on. If you add a comment explaining what you explained here to the commit message I think this is all fine with me as-is. -Daniel

...

Noralf.

...

...

+/*

• • fb_ops.fb_destroy is called by the last put_fb_info() call at the end of
• • unregister_framebuffer() or fb_release().
• */

+static void drm_fbdev_fb_destroy(struct fb_info *info) +{

• struct drm_fb_helper *fb_helper = info->par;
• struct fb_ops *fbops = NULL;
• DRM_DEBUG("\n");
• if (fb_helper->fbdev->fbdefio) {

•     fb_deferred_io_cleanup(fb_helper->fbdev);
  

•     fbops = fb_helper->fbdev->fbops;
  

• }
• drm_fb_helper_fini(fb_helper);
• drm_client_framebuffer_delete(fb_helper->buffer);
• drm_client_free(fb_helper->client);
• kfree(fb_helper);
• kfree(fbops);

+}

Hm, if we go with the idea that drm_clients could auto-unregister through a callback, then I expect this will lead to some control inversion. But we can fix that later on.

...

+static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) +{

• struct drm_fb_helper *fb_helper = info->par;
• if (fb_helper->dev->driver->gem_prime_mmap)

•     return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma);
  

• else

•     return -ENODEV;
  

+}

+static struct fb_ops drm_fbdev_fb_ops = {

• /* No need to set owner, this module is already pinned by the driver. */

I'd still set it, means less thinking since more obviously correct.

...

• DRM_FB_HELPER_DEFAULT_OPS,
• .fb_open = drm_fbdev_fb_open,
• .fb_release = drm_fbdev_fb_release,
• .fb_destroy = drm_fbdev_fb_destroy,
• .fb_mmap = drm_fbdev_fb_mmap,
• .fb_read = drm_fb_helper_sys_read,
• .fb_write = drm_fb_helper_sys_write,
• .fb_fillrect = drm_fb_helper_sys_fillrect,
• .fb_copyarea = drm_fb_helper_sys_copyarea,
• .fb_imageblit = drm_fb_helper_sys_imageblit,

Hm, some drivers require the cfb versions of these. In practice I guess there's not much of a difference really, at least on x86 and arm.

We might want to document that though.

...

+};

+static struct fb_deferred_io drm_fbdev_defio = {

• .delay = HZ / 20,
• .deferred_io = drm_fb_helper_deferred_io,

+};

+/*

• • TODO: Remove this when tinydrm is converted to vmalloc buffers.
• */

+static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,

•                               struct vm_area_struct *vma)
  

+{

• fb_deferred_io_mmap(info, vma);
• vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
• return 0;

+}

Needs kerneldoc here.

...

+int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,

•                     struct drm_fb_helper_surface_size *sizes)
  

+{

• struct drm_client_dev *client = fb_helper->client;
• struct drm_client_buffer *buffer;
• struct drm_framebuffer *fb;
• struct fb_info *fbi;
• u32 format;
• int ret;
• DRM_DEBUG_KMS("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);
• buffer = drm_client_framebuffer_create(client, sizes->surface_width,

•                                    sizes->surface_height, format);
  

• if (IS_ERR(buffer))

•     return PTR_ERR(buffer);
  

• fb_helper->buffer = buffer;
• fb_helper->fb = buffer->fb;
• fb = buffer->fb;
• fbi = drm_fb_helper_alloc_fbi(fb_helper);
• if (IS_ERR(fbi)) {

•     ret = PTR_ERR(fbi);
  

•     goto err_free_buffer;
  

• }
• fbi->par = fb_helper;
• fbi->fbops = &drm_fbdev_fb_ops;
• fbi->screen_size = fb->height * fb->pitches[0];
• fbi->fix.smem_len = fbi->screen_size;
• fbi->screen_buffer = buffer->vaddr;
• strcpy(fbi->fix.id, "DRM emulated");
• drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth);
• drm_fb_helper_fill_var(fbi, fb_helper, sizes->fb_width, sizes->fb_height);
• if (fb->funcs->dirty) {

•     struct fb_ops *fbops;
  

•     /*
  

•      * fb_deferred_io_cleanup() clears &fbops->fb_mmap so a per
  

•      * instance version is necessary.
  

•      */
  

•     fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
  

•     if (!fbops) {
  

•             ret = -ENOMEM;
  

•             goto err_fb_info_destroy;
  

•     }
  

•     *fbops = *fbi->fbops;
  

•     fbi->fbops = fbops;
  

•     fbi->fbdefio = &drm_fbdev_defio;
  

•     /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */
  

•     fbi->fix.smem_start = page_to_phys(virt_to_page(buffer->vaddr));
  

•     fb_deferred_io_init(fbi);
  

•     /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */
  

•     fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap;
  

• }

Ugh. Yeah defio and generic allocator through dumb buffers don't mix well. The only true generic solution for this would be to give userspace (and only userspace, for fbcon we can intercept everything) a staging buffer, and then upload things using the dirty callback.

But that introduces another copy step, so isn't cool.

I think a check for is_vmalloc_addr and if that's false, not doing any of the defio mmap setup would be good. Until we have a better idea. And yes that would need to be done after tinydrm is moved over.

...

• return 0;

+err_fb_info_destroy:

• drm_fb_helper_fini(fb_helper);

+err_free_buffer:

• drm_client_framebuffer_delete(buffer);
• return ret;

+} +EXPORT_SYMBOL(drm_fb_helper_generic_probe);

• /* The Kconfig DRM_KMS_HELPER selects FRAMEBUFFER_CONSOLE (if !EXPERT)
  • but the module doesn't depend on any fb console symbols. At least
  • attempt to load fbcon to avoid leaving the system without a usable console.

diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index b069433e7fc1..bb38469a9502 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -30,6 +30,8 @@ #ifndef DRM_FB_HELPER_H #define DRM_FB_HELPER_H +struct drm_client_buffer; +struct drm_client_dev; struct drm_fb_helper; #include <drm/drm_crtc.h> @@ -232,6 +234,20 @@ struct drm_fb_helper { * See also: @deferred_setup */ int preferred_bpp;

• /**
• • @client:
• • DRM client used by the generic fbdev emulation.
• */
• struct drm_client_dev *client;
• /**
• • @buffer:
• • Framebuffer used by the generic fbdev emulation.
• */
• struct drm_client_buffer *buffer; }; /**

@@ -330,6 +346,9 @@ void drm_fb_helper_fbdev_teardown(struct drm_device *dev); void drm_fb_helper_lastclose(struct drm_device *dev); void drm_fb_helper_output_poll_changed(struct drm_device *dev);

+int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,

•                     struct drm_fb_helper_surface_size *sizes);
  

  #else static inline void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,

@@ -564,6 +583,13 @@ static inline void drm_fb_helper_output_poll_changed(struct drm_device *dev) { } +static inline int +drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,

•                 struct drm_fb_helper_surface_size *sizes)
  

+{

• return 0;

+}

Ok, I think this patch looks ok. With the missing kerneldoc added (which also explains the current limitations) this is

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

...

• #endif static inline int

-- 2.15.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

On Thu, Jan 03, 2019 at 06:06:53PM +0100, Noralf Trønnes wrote:

Den 28.12.2018 21.38, skrev Daniel Vetter:

...

On Tue, May 29, 2018 at 9:54 AM Daniel Vetter daniel@ffwll.ch wrote:

...

On Fri, May 25, 2018 at 02:42:02PM +0200, Noralf Trønnes wrote:

...

Den 24.05.2018 11.16, skrev Daniel Vetter:

...

On Wed, May 23, 2018 at 04:34:07PM +0200, Noralf Trønnes wrote:

...

This is the first step in getting generic fbdev emulation. A drm_fb_helper_funcs.fb_probe function is added which uses the DRM client API to get a framebuffer backed by a dumb buffer.

A transitional hack for tinydrm is needed in order to switch over all CMA helper drivers in a later patch. This hack will be removed when tinydrm moves to vmalloc buffers.

Signed-off-by: Noralf Trønnes noralf@tronnes.org

drivers/gpu/drm/drm_fb_helper.c | 164 ++++++++++++++++++++++++++++++++++++++++ include/drm/drm_fb_helper.h | 26 +++++++ 2 files changed, 190 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 2ee1eaa66188..444c2b4040ea 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -30,11 +30,13 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt #include <linux/console.h> +#include <linux/dma-buf.h> #include <linux/kernel.h> #include <linux/sysrq.h> #include <linux/slab.h> #include <linux/module.h> #include <drm/drmP.h> +#include <drm/drm_client.h> #include <drm/drm_crtc.h> #include <drm/drm_fb_helper.h> #include <drm/drm_crtc_helper.h> @@ -2928,6 +2930,168 @@ void drm_fb_helper_output_poll_changed(struct drm_device *dev) } EXPORT_SYMBOL(drm_fb_helper_output_poll_changed); +/* @user: 1=userspace, 0=fbcon */ +static int drm_fbdev_fb_open(struct fb_info *info, int user) +{

• struct drm_fb_helper *fb_helper = info->par;
• if (!try_module_get(fb_helper->dev->driver->fops->owner))

•     return -ENODEV;
  

• return 0;

+}

+static int drm_fbdev_fb_release(struct fb_info *info, int user) +{

• struct drm_fb_helper *fb_helper = info->par;
• module_put(fb_helper->dev->driver->fops->owner);
• return 0;

+}

Hm, I thought earlier versions of your patch series had this separately, for everyone. What's the reasons for merging this into the fb_probe implementation.

This is only necessary when struct fb_ops is defined in a library where the owner field is the library module and not the driver module. I realised that with this generic version it's highly unlikely that we get another library that defines struct fb_ops, so it's only needed here.

Hm, I'm still not 100% convinced we can fully subsume the tinydrm fbdev code with the generic one, due to the varios slight issues around defio tracking that we still have.

I have a new idea for 100% generic defio support in the fbdev helpers. Haven't tried it thought, but I think this could work around the problem if your mmap isn't struct page backed:

• In the drm_fbdev_fb_mmap helper, if we need defio, change the

default page prots to write-protected with something like this:

vma->vm_page_prot = pgprot_wrprotect(vma->vm_page_prot);

• After the driver's mmap function completed, copy the vm_ops

structure and WARN_ON if it has an mkwrite and sync callback set. There's no real race here as long as we do all this before we return to userspace.

• Set the mkwrite and fsync callbacks with similar implementions to

the core fbdev defio stuff. These should all work on plain ptes, they don't actually require a struct page. uff. These should all work on plain ptes, they don't actually require a struct page.

• We might also need to overwrite the vma_ops fault callback, just

forwarding to the underlying vma_ops instead of copying them would be cleaner. The overhead won't matter, especially not for defio drivers.

• Also copypaste all the other bits of the core fbdev defio code we'll

need, that would allow us to also clean up the cleanup() warts ...

All of this would ofc only be done if the fb has a ->dirty callback.

We can also just stuff this into todo.rst.

Hmm, do you think it's worth spending more time on fbdev? The point would be to speed it up, right? Avoid the blitting.

My hope is that when I'm done with DRM client, David Herrmann will pick up and finish his DRM/KMS console. At that point fbdev is only needed for legacy userspace.

This is only about the legacy mmap stuff, fbcon doesn't use that. And the current defio fbdev mmap stuff is fairly horrible and very driver-specific. Doing the above would be a neat cleanup I think, since it avoids that new drivers have to care about fbdev.

Aside: Not sure David is still interested in the drm console stuff. I haven't chatted with him since ages about this at least ...

I suck at estimating how long it takes to do stuff, but I really hope I'm done with DRM client by the end of this year.

It's going to be a sizeable chunk of work I think, and might not work. Was really just an idea, I think better to stuff it into todo.rst. I'll do a patch.

Cheers, Daniel

Noralf.

...

Cheers, Daniel

...

But it's also easy to export this later on. If you add a comment explaining what you explained here to the commit message I think this is all fine with me as-is. -Daniel

...

Noralf.

...

...

+/*

• • fb_ops.fb_destroy is called by the last put_fb_info() call at the end of
• • unregister_framebuffer() or fb_release().
• */

+static void drm_fbdev_fb_destroy(struct fb_info *info) +{

• struct drm_fb_helper *fb_helper = info->par;
• struct fb_ops *fbops = NULL;
• DRM_DEBUG("\n");
• if (fb_helper->fbdev->fbdefio) {

•     fb_deferred_io_cleanup(fb_helper->fbdev);
  

•     fbops = fb_helper->fbdev->fbops;
  

• }
• drm_fb_helper_fini(fb_helper);
• drm_client_framebuffer_delete(fb_helper->buffer);
• drm_client_free(fb_helper->client);
• kfree(fb_helper);
• kfree(fbops);

+}

Hm, if we go with the idea that drm_clients could auto-unregister through a callback, then I expect this will lead to some control inversion. But we can fix that later on.

...

+static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) +{

• struct drm_fb_helper *fb_helper = info->par;
• if (fb_helper->dev->driver->gem_prime_mmap)

•     return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma);
  

• else

•     return -ENODEV;
  

+}

+static struct fb_ops drm_fbdev_fb_ops = {

• /* No need to set owner, this module is already pinned by the driver. */

I'd still set it, means less thinking since more obviously correct.

...

• DRM_FB_HELPER_DEFAULT_OPS,
• .fb_open = drm_fbdev_fb_open,
• .fb_release = drm_fbdev_fb_release,
• .fb_destroy = drm_fbdev_fb_destroy,
• .fb_mmap = drm_fbdev_fb_mmap,
• .fb_read = drm_fb_helper_sys_read,
• .fb_write = drm_fb_helper_sys_write,
• .fb_fillrect = drm_fb_helper_sys_fillrect,
• .fb_copyarea = drm_fb_helper_sys_copyarea,
• .fb_imageblit = drm_fb_helper_sys_imageblit,

Hm, some drivers require the cfb versions of these. In practice I guess there's not much of a difference really, at least on x86 and arm.

We might want to document that though.

...

+};

+static struct fb_deferred_io drm_fbdev_defio = {

• .delay = HZ / 20,
• .deferred_io = drm_fb_helper_deferred_io,

+};

+/*

• • TODO: Remove this when tinydrm is converted to vmalloc buffers.
• */

+static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,

•                               struct vm_area_struct *vma)
  

+{

• fb_deferred_io_mmap(info, vma);
• vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
• return 0;

+}

Needs kerneldoc here.

...

+int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,

•                     struct drm_fb_helper_surface_size *sizes)
  

+{

• struct drm_client_dev *client = fb_helper->client;
• struct drm_client_buffer *buffer;
• struct drm_framebuffer *fb;
• struct fb_info *fbi;
• u32 format;
• int ret;
• DRM_DEBUG_KMS("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);
• buffer = drm_client_framebuffer_create(client, sizes->surface_width,

•                                    sizes->surface_height, format);
  

• if (IS_ERR(buffer))

•     return PTR_ERR(buffer);
  

• fb_helper->buffer = buffer;
• fb_helper->fb = buffer->fb;
• fb = buffer->fb;
• fbi = drm_fb_helper_alloc_fbi(fb_helper);
• if (IS_ERR(fbi)) {

•     ret = PTR_ERR(fbi);
  

•     goto err_free_buffer;
  

• }
• fbi->par = fb_helper;
• fbi->fbops = &drm_fbdev_fb_ops;
• fbi->screen_size = fb->height * fb->pitches[0];
• fbi->fix.smem_len = fbi->screen_size;
• fbi->screen_buffer = buffer->vaddr;
• strcpy(fbi->fix.id, "DRM emulated");
• drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth);
• drm_fb_helper_fill_var(fbi, fb_helper, sizes->fb_width, sizes->fb_height);
• if (fb->funcs->dirty) {

•     struct fb_ops *fbops;
  

•     /*
  

•      * fb_deferred_io_cleanup() clears &fbops->fb_mmap so a per
  

•      * instance version is necessary.
  

•      */
  

•     fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
  

•     if (!fbops) {
  

•             ret = -ENOMEM;
  

•             goto err_fb_info_destroy;
  

•     }
  

•     *fbops = *fbi->fbops;
  

•     fbi->fbops = fbops;
  

•     fbi->fbdefio = &drm_fbdev_defio;
  

•     /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */
  

•     fbi->fix.smem_start = page_to_phys(virt_to_page(buffer->vaddr));
  

•     fb_deferred_io_init(fbi);
  

•     /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */
  

•     fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap;
  

• }

Ugh. Yeah defio and generic allocator through dumb buffers don't mix well. The only true generic solution for this would be to give userspace (and only userspace, for fbcon we can intercept everything) a staging buffer, and then upload things using the dirty callback.

But that introduces another copy step, so isn't cool.

I think a check for is_vmalloc_addr and if that's false, not doing any of the defio mmap setup would be good. Until we have a better idea. And yes that would need to be done after tinydrm is moved over.

...

• return 0;

+err_fb_info_destroy:

• drm_fb_helper_fini(fb_helper);

+err_free_buffer:

• drm_client_framebuffer_delete(buffer);
• return ret;

+} +EXPORT_SYMBOL(drm_fb_helper_generic_probe);

• /* The Kconfig DRM_KMS_HELPER selects FRAMEBUFFER_CONSOLE (if !EXPERT)
  • but the module doesn't depend on any fb console symbols. At least
  • attempt to load fbcon to avoid leaving the system without a usable console.

diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index b069433e7fc1..bb38469a9502 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -30,6 +30,8 @@ #ifndef DRM_FB_HELPER_H #define DRM_FB_HELPER_H +struct drm_client_buffer; +struct drm_client_dev; struct drm_fb_helper; #include <drm/drm_crtc.h> @@ -232,6 +234,20 @@ struct drm_fb_helper { * See also: @deferred_setup */ int preferred_bpp;

• /**
• • @client:
• • DRM client used by the generic fbdev emulation.
• */
• struct drm_client_dev *client;
• /**
• • @buffer:
• • Framebuffer used by the generic fbdev emulation.
• */
• struct drm_client_buffer *buffer; }; /**

@@ -330,6 +346,9 @@ void drm_fb_helper_fbdev_teardown(struct drm_device *dev); void drm_fb_helper_lastclose(struct drm_device *dev); void drm_fb_helper_output_poll_changed(struct drm_device *dev);

+int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,

•                     struct drm_fb_helper_surface_size *sizes);
  

  #else static inline void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,

@@ -564,6 +583,13 @@ static inline void drm_fb_helper_output_poll_changed(struct drm_device *dev) { } +static inline int +drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,

•                 struct drm_fb_helper_surface_size *sizes)
  

+{

• return 0;

+}

Ok, I think this patch looks ok. With the missing kerneldoc added (which also explains the current limitations) this is

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

...

• #endif static inline int

-- 2.15.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

On Wed, May 23, 2018 at 04:34:09PM +0200, Noralf Trønnes wrote:

This switches the CMA helper drivers that use its fbdev emulation over to the generic fbdev emulation. It's the first phase of using generic fbdev. A later phase will use DRM client callbacks for the lastclose/hotplug/remove callbacks.

There are currently 2 fbdev init/fini functions:

• drm_fb_cma_fbdev_init/drm_fb_cma_fbdev_fini
• drm_fbdev_cma_init/drm_fbdev_cma_fini

This is because the work on generic fbdev came up during a fbdev refactoring and thus wasn't completed. No point in completing that refactoring when drivers will soon move to drm_fb_helper_generic_probe().

tinydrm uses drm_fb_cma_fbdev_init_with_funcs().

Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Noralf Trønnes noralf@tronnes.org

---

drivers/gpu/drm/drm_fb_cma_helper.c | 365 +++++-------------------------------

Yay for the diffstat! Has my ack, but would be good to get Laurent's review. -Daniel

include/drm/drm_fb_cma_helper.h | 3 - 2 files changed, 47 insertions(+), 321 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 186d00adfb5f..76e2f7977779 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -18,6 +18,7 @@ */

#include <drm/drmP.h> +#include <drm/drm_client.h> #include <drm/drm_fb_helper.h> #include <drm/drm_framebuffer.h> #include <drm/drm_gem_cma_helper.h> @@ -26,11 +27,8 @@ #include <drm/drm_print.h> #include <linux/module.h>

-#define DEFAULT_FBDEFIO_DELAY_MS 50

struct drm_fbdev_cma { struct drm_fb_helper fb_helper;

• const struct drm_framebuffer_funcs *fb_funcs;

};

/** @@ -44,36 +42,6 @@ struct drm_fbdev_cma {

• An fbdev framebuffer backed by cma is also available by calling
• drm_fb_cma_fbdev_init(). drm_fb_cma_fbdev_fini() tears it down.

• • If the &drm_framebuffer_funcs.dirty callback is set, fb_deferred_io will be
• • set up automatically. &drm_framebuffer_funcs.dirty is called by
• • drm_fb_helper_deferred_io() in process context (&struct delayed_work).
• • Example fbdev deferred io code::

• • static int driver_fb_dirty(struct drm_framebuffer *fb,
    

• •                            struct drm_file *file_priv,
    

• •                            unsigned flags, unsigned color,
    

• •                            struct drm_clip_rect *clips,
    

• •                            unsigned num_clips)
    

• • {
    

• •     struct drm_gem_cma_object *cma = drm_fb_cma_get_gem_obj(fb, 0);
    

• •     ... push changes ...
    

• •     return 0;
    

• • }
    

• • static struct drm_framebuffer_funcs driver_fb_funcs = {
    

• •     .destroy       = drm_gem_fb_destroy,
    

• •     .create_handle = drm_gem_fb_create_handle,
    

• •     .dirty         = driver_fb_dirty,
    

• • };
    

• • Initialize::

• • fbdev = drm_fb_cma_fbdev_init_with_funcs(dev, 16,
    

• •                                       dev->mode_config.num_crtc,
    

• •                                       dev->mode_config.num_connector,
    

• •                                       &driver_fb_funcs);
    

• */

static inline struct drm_fbdev_cma *to_fbdev_cma(struct drm_fb_helper *helper) @@ -131,153 +99,6 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, } EXPORT_SYMBOL_GPL(drm_fb_cma_get_gem_addr);

-static int drm_fb_cma_mmap(struct fb_info *info, struct vm_area_struct *vma) -{

• return dma_mmap_writecombine(info->device, vma, info->screen_base,

• 		     info->fix.smem_start, info->fix.smem_len);
  

-}

-static struct fb_ops drm_fbdev_cma_ops = {

• .owner = THIS_MODULE,
• DRM_FB_HELPER_DEFAULT_OPS,
• .fb_fillrect = drm_fb_helper_sys_fillrect,
• .fb_copyarea = drm_fb_helper_sys_copyarea,
• .fb_imageblit = drm_fb_helper_sys_imageblit,
• .fb_mmap = drm_fb_cma_mmap,

-};

-static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,

• 			  struct vm_area_struct *vma)
  

-{

• fb_deferred_io_mmap(info, vma);
• vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
• return 0;

-}

-static int drm_fbdev_cma_defio_init(struct fb_info *fbi,

• 		    struct drm_gem_cma_object *cma_obj)
  

-{

• struct fb_deferred_io *fbdefio;
• struct fb_ops *fbops;
• /*

• * Per device structures are needed because:
  

• * fbops: fb_deferred_io_cleanup() clears fbops.fb_mmap
  

• * fbdefio: individual delays
  

• */
  

• fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
• fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
• if (!fbdefio || !fbops) {

• kfree(fbdefio);
  

• kfree(fbops);
  

• return -ENOMEM;
  

• }
• /* can't be offset from vaddr since dirty() uses cma_obj */
• fbi->screen_buffer = cma_obj->vaddr;
• /* fb_deferred_io_fault() needs a physical address */
• fbi->fix.smem_start = page_to_phys(virt_to_page(fbi->screen_buffer));
• *fbops = *fbi->fbops;
• fbi->fbops = fbops;
• fbdefio->delay = msecs_to_jiffies(DEFAULT_FBDEFIO_DELAY_MS);
• fbdefio->deferred_io = drm_fb_helper_deferred_io;
• fbi->fbdefio = fbdefio;
• fb_deferred_io_init(fbi);
• fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap;
• return 0;

-}

-static void drm_fbdev_cma_defio_fini(struct fb_info *fbi) -{

• if (!fbi->fbdefio)

• return;
  

• fb_deferred_io_cleanup(fbi);
• kfree(fbi->fbdefio);
• kfree(fbi->fbops);

-}

-static int -drm_fbdev_cma_create(struct drm_fb_helper *helper,

• struct drm_fb_helper_surface_size *sizes)

-{

• struct drm_fbdev_cma *fbdev_cma = to_fbdev_cma(helper);
• struct drm_device *dev = helper->dev;
• struct drm_gem_cma_object *obj;
• struct drm_framebuffer *fb;
• unsigned int bytes_per_pixel;
• unsigned long offset;
• struct fb_info *fbi;
• size_t size;
• int ret;
• DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n",

• 	sizes->surface_width, sizes->surface_height,
  

• 	sizes->surface_bpp);
  

• bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8);
• size = sizes->surface_width * sizes->surface_height * bytes_per_pixel;
• obj = drm_gem_cma_create(dev, size);
• if (IS_ERR(obj))

• return -ENOMEM;
  

• fbi = drm_fb_helper_alloc_fbi(helper);
• if (IS_ERR(fbi)) {

• ret = PTR_ERR(fbi);
  

• goto err_gem_free_object;
  

• }
• fb = drm_gem_fbdev_fb_create(dev, sizes, 0, &obj->base,

• 		     fbdev_cma->fb_funcs);
  

• if (IS_ERR(fb)) {

• dev_err(dev->dev, "Failed to allocate DRM framebuffer.\n");
  

• ret = PTR_ERR(fb);
  

• goto err_fb_info_destroy;
  

• }
• helper->fb = fb;
• fbi->par = helper;
• fbi->flags = FBINFO_FLAG_DEFAULT;
• fbi->fbops = &drm_fbdev_cma_ops;
• drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth);
• drm_fb_helper_fill_var(fbi, helper, sizes->fb_width, sizes->fb_height);
• offset = fbi->var.xoffset * bytes_per_pixel;
• offset += fbi->var.yoffset * fb->pitches[0];
• dev->mode_config.fb_base = (resource_size_t)obj->paddr;
• fbi->screen_base = obj->vaddr + offset;
• fbi->fix.smem_start = (unsigned long)(obj->paddr + offset);
• fbi->screen_size = size;
• fbi->fix.smem_len = size;
• if (fb->funcs->dirty) {

• ret = drm_fbdev_cma_defio_init(fbi, obj);
  

• if (ret)
  

• 	goto err_cma_destroy;
  

• }
• return 0;

-err_cma_destroy:

• drm_framebuffer_remove(fb);

-err_fb_info_destroy:

• drm_fb_helper_fini(helper);

-err_gem_free_object:

• drm_gem_object_put_unlocked(&obj->base);
• return ret;

-}

-static const struct drm_fb_helper_funcs drm_fb_cma_helper_funcs = {

• .fb_probe = drm_fbdev_cma_create,

-};

/**

• drm_fb_cma_fbdev_init_with_funcs() - Allocate and initialize fbdev emulation
• @dev: DRM device

@@ -295,53 +116,7 @@ int drm_fb_cma_fbdev_init_with_funcs(struct drm_device *dev, unsigned int preferred_bpp, unsigned int max_conn_count, const struct drm_framebuffer_funcs *funcs) {

• struct drm_fbdev_cma *fbdev_cma;
• struct drm_fb_helper *fb_helper;
• int ret;
• if (!preferred_bpp)

• preferred_bpp = dev->mode_config.preferred_depth;
  

• if (!preferred_bpp)

• preferred_bpp = 32;
  

• if (!max_conn_count)

• max_conn_count = dev->mode_config.num_connector;
  

• fbdev_cma = kzalloc(sizeof(*fbdev_cma), GFP_KERNEL);
• if (!fbdev_cma)

• return -ENOMEM;
  

• fbdev_cma->fb_funcs = funcs;
• fb_helper = &fbdev_cma->fb_helper;
• drm_fb_helper_prepare(dev, fb_helper, &drm_fb_cma_helper_funcs);
• ret = drm_fb_helper_init(dev, fb_helper, max_conn_count);
• if (ret < 0) {

• DRM_DEV_ERROR(dev->dev, "Failed to initialize fbdev helper.\n");
  

• goto err_free;
  

• }
• ret = drm_fb_helper_single_add_all_connectors(fb_helper);
• if (ret < 0) {

• DRM_DEV_ERROR(dev->dev, "Failed to add connectors.\n");
  

• goto err_drm_fb_helper_fini;
  

• }
• ret = drm_fb_helper_initial_config(fb_helper, preferred_bpp);
• if (ret < 0) {

• DRM_DEV_ERROR(dev->dev, "Failed to set fbdev configuration.\n");
  

• goto err_drm_fb_helper_fini;
  

• }
• return 0;

-err_drm_fb_helper_fini:

• drm_fb_helper_fini(fb_helper);

-err_free:

• kfree(fbdev_cma);
• return ret;

• return drm_fb_cma_fbdev_init(dev, preferred_bpp, max_conn_count);

} EXPORT_SYMBOL_GPL(drm_fb_cma_fbdev_init_with_funcs);

@@ -359,8 +134,14 @@ EXPORT_SYMBOL_GPL(drm_fb_cma_fbdev_init_with_funcs); int drm_fb_cma_fbdev_init(struct drm_device *dev, unsigned int preferred_bpp, unsigned int max_conn_count) {

• return drm_fb_cma_fbdev_init_with_funcs(dev, preferred_bpp,

• 				max_conn_count, NULL);
  

• struct drm_fbdev_cma *fbdev_cma;
• /* dev->fb_helper will indirectly point to fbdev_cma after this call */
• fbdev_cma = drm_fbdev_cma_init(dev, preferred_bpp, max_conn_count);
• if (IS_ERR(fbdev_cma))

• return PTR_ERR(fbdev_cma);
  

• return 0;

} EXPORT_SYMBOL_GPL(drm_fb_cma_fbdev_init);

@@ -370,87 +151,13 @@ EXPORT_SYMBOL_GPL(drm_fb_cma_fbdev_init); */ void drm_fb_cma_fbdev_fini(struct drm_device *dev) {

• struct drm_fb_helper *fb_helper = dev->fb_helper;
• if (!fb_helper)

• return;
  

• /* Unregister if it hasn't been done already */
• if (fb_helper->fbdev && fb_helper->fbdev->dev)

• drm_fb_helper_unregister_fbi(fb_helper);
  

• if (fb_helper->fbdev)

• drm_fbdev_cma_defio_fini(fb_helper->fbdev);
  

• if (fb_helper->fb)

• drm_framebuffer_remove(fb_helper->fb);
  

• drm_fb_helper_fini(fb_helper);
• kfree(to_fbdev_cma(fb_helper));

• if (dev->fb_helper)

• drm_fbdev_cma_fini(to_fbdev_cma(dev->fb_helper));
  

} EXPORT_SYMBOL_GPL(drm_fb_cma_fbdev_fini);

-/**

• • drm_fbdev_cma_init_with_funcs() - Allocate and initializes a drm_fbdev_cma struct
• • @dev: DRM device
• • @preferred_bpp: Preferred bits per pixel for the device
• • @max_conn_count: Maximum number of connectors
• • @funcs: fb helper functions, in particular a custom dirty() callback
• • Returns a newly allocated drm_fbdev_cma struct or a ERR_PTR.
• */

-struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev,

• unsigned int preferred_bpp, unsigned int max_conn_count,
• const struct drm_framebuffer_funcs *funcs)

-{

• struct drm_fbdev_cma *fbdev_cma;
• struct drm_fb_helper *helper;
• int ret;
• fbdev_cma = kzalloc(sizeof(*fbdev_cma), GFP_KERNEL);
• if (!fbdev_cma) {

• dev_err(dev->dev, "Failed to allocate drm fbdev.\n");
  

• return ERR_PTR(-ENOMEM);
  

• }
• fbdev_cma->fb_funcs = funcs;
• helper = &fbdev_cma->fb_helper;
• drm_fb_helper_prepare(dev, helper, &drm_fb_cma_helper_funcs);
• ret = drm_fb_helper_init(dev, helper, max_conn_count);
• if (ret < 0) {

• dev_err(dev->dev, "Failed to initialize drm fb helper.\n");
  

• goto err_free;
  

• }
• ret = drm_fb_helper_single_add_all_connectors(helper);
• if (ret < 0) {

• dev_err(dev->dev, "Failed to add connectors.\n");
  

• goto err_drm_fb_helper_fini;
  

• }
• ret = drm_fb_helper_initial_config(helper, preferred_bpp);
• if (ret < 0) {

• dev_err(dev->dev, "Failed to set initial hw configuration.\n");
  

• goto err_drm_fb_helper_fini;
  

• }
• return fbdev_cma;

-err_drm_fb_helper_fini:

• drm_fb_helper_fini(helper);

-err_free:

• kfree(fbdev_cma);
• return ERR_PTR(ret);

-} -EXPORT_SYMBOL_GPL(drm_fbdev_cma_init_with_funcs);

-static const struct drm_framebuffer_funcs drm_fb_cma_funcs = {

• .destroy = drm_gem_fb_destroy,
• .create_handle = drm_gem_fb_create_handle,

+static const struct drm_fb_helper_funcs drm_fb_cma_helper_funcs = {

• .fb_probe = drm_fb_helper_generic_probe,

};

/** @@ -464,9 +171,38 @@ static const struct drm_framebuffer_funcs drm_fb_cma_funcs = { struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev, unsigned int preferred_bpp, unsigned int max_conn_count) {

• return drm_fbdev_cma_init_with_funcs(dev, preferred_bpp,

• 			     max_conn_count,
  

• 			     &drm_fb_cma_funcs);
  

• struct drm_fbdev_cma *fbdev_cma;
• struct drm_fb_helper *fb_helper;
• struct drm_client_dev *client;
• int ret;
• fbdev_cma = kzalloc(sizeof(*fbdev_cma), GFP_KERNEL);
• if (!fbdev_cma)

• return ERR_PTR(-ENOMEM);
  

• client = drm_client_new(dev);
• if (IS_ERR(client)) {

• ret = PTR_ERR(client);
  

• goto err_free;
  

• }
• fb_helper = &fbdev_cma->fb_helper;
• client->private = fb_helper;
• fb_helper->client = client;
• ret = drm_fb_helper_fbdev_setup(dev, fb_helper, &drm_fb_cma_helper_funcs,

• 			preferred_bpp, max_conn_count);
  

• if (ret)

• goto err_client_free;
  

• return fbdev_cma;

+err_client_free:

• drm_client_free(client);

+err_free:

• kfree(fbdev_cma);
• return ERR_PTR(ret);

} EXPORT_SYMBOL_GPL(drm_fbdev_cma_init);

@@ -477,14 +213,7 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_init); void drm_fbdev_cma_fini(struct drm_fbdev_cma *fbdev_cma) { drm_fb_helper_unregister_fbi(&fbdev_cma->fb_helper);

• if (fbdev_cma->fb_helper.fbdev)

• drm_fbdev_cma_defio_fini(fbdev_cma->fb_helper.fbdev);
  

• if (fbdev_cma->fb_helper.fb)

• drm_framebuffer_remove(fbdev_cma->fb_helper.fb);
  

• drm_fb_helper_fini(&fbdev_cma->fb_helper);
• kfree(fbdev_cma);

• /* All resources have now been freed by drm_fbdev_fb_destroy() */

} EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini);

diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h index d532f88a8d55..a0546c3451f9 100644 --- a/include/drm/drm_fb_cma_helper.h +++ b/include/drm/drm_fb_cma_helper.h @@ -23,9 +23,6 @@ int drm_fb_cma_fbdev_init(struct drm_device *dev, unsigned int preferred_bpp, unsigned int max_conn_count); void drm_fb_cma_fbdev_fini(struct drm_device *dev);

-struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev,

• unsigned int preferred_bpp, unsigned int max_conn_count,
• const struct drm_framebuffer_funcs *funcs);

struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev, unsigned int preferred_bpp, unsigned int max_conn_count); void drm_fbdev_cma_fini(struct drm_fbdev_cma *fbdev_cma); -- 2.15.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Wed, May 23, 2018 at 04:34:10PM +0200, Noralf Trønnes wrote:

Add client callbacks and hook them up. Add a list of clients per drm_device.

Patch nit, but I think the debugfs stuff would be nice if split out.

Signed-off-by: Noralf Trønnes noralf@tronnes.org

drivers/gpu/drm/drm_client.c | 246 +++++++++++++++++++++++++++++++++++- drivers/gpu/drm/drm_debugfs.c | 7 + drivers/gpu/drm/drm_drv.c | 8 ++ drivers/gpu/drm/drm_fb_cma_helper.c | 2 +- drivers/gpu/drm/drm_file.c | 3 + drivers/gpu/drm/drm_probe_helper.c | 3 + include/drm/drm_client.h | 65 +++++++++- include/drm/drm_device.h | 14 ++ 8 files changed, 345 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c index 0919aea7dddd..c495fcee2058 100644 --- a/drivers/gpu/drm/drm_client.c +++ b/drivers/gpu/drm/drm_client.c @@ -66,11 +66,13 @@ static void drm_client_free_file(struct drm_client_dev *client) /**

• drm_client_new - Create a DRM client
• @dev: DRM device

• • @funcs: DRM client functions
  • Returns:
  • Pointer to a client or an error pointer on failure.
  */

-struct drm_client_dev *drm_client_new(struct drm_device *dev) +struct drm_client_dev * +drm_client_new(struct drm_device *dev, const struct drm_client_funcs *funcs) { struct drm_client_dev *client; int ret; @@ -84,6 +86,7 @@ struct drm_client_dev *drm_client_new(struct drm_device *dev) return ERR_PTR(-ENOMEM);

client->dev = dev;

• client->funcs = funcs;

  ret = drm_client_alloc_file(client); if (ret) {

@@ -91,21 +94,231 @@ struct drm_client_dev *drm_client_new(struct drm_device *dev) return ERR_PTR(ret); }

• /*

• * TODO:
  

• * Temporary hack until all CMA drivers have been moved over to using
  

• * drm_fbdev_generic_setup().
  

• */
  

I think clients without callbacks make perfect sense, imo no need for a TODO. Just explain in the kernel-doc that the callbacks are optional.

• if (!funcs)

• return client;
  

• mutex_lock(&dev->clientlist_mutex);
• list_add(&client->list, &dev->clientlist);
• mutex_unlock(&dev->clientlist_mutex);
• return client;

} EXPORT_SYMBOL(drm_client_new);

+/**

• • drm_client_new_from_id - Create a DRM client from a minor id
• • @minor_id: DRM minor id
• • @funcs: DRM client functions
• • Returns:
• • Pointer to a client or an error pointer on failure.
• */

+struct drm_client_dev * +drm_client_new_from_id(unsigned int minor_id, const struct drm_client_funcs *funcs) +{

• struct drm_client_dev *client;
• struct drm_minor *minor;
• minor = drm_minor_acquire(minor_id);
• if (IS_ERR(minor))

• return ERR_CAST(minor);
  

• client = drm_client_new(minor->dev, funcs);
• drm_minor_release(minor);
• return client;

+} +EXPORT_SYMBOL(drm_client_new_from_id);

I don't get why we need this new function ...

/**

• drm_client_free - Free DRM client resources
• @client: DRM client

• • This is called automatically on client removal unless the client returns
• • non-zero in the &drm_client_funcs->remove callback. The fbdev client does
• • this when it can't close &drm_file because userspace has an open fd.
• • Note:
• • If the client can't release it's resources on remove, it needs to hold a
• • reference on the driver module to prevent the code from going away.

We need a full reference on the drm_device I think, not just the code. I thought drm_file provides that for any drm_client?

*/ void drm_client_free(struct drm_client_dev *client) {

• DRM_DEV_DEBUG_KMS(client->dev->dev, "\n");

Both above bits probably should be in the patch that introduces these?

drm_client_free_file(client); kfree(client); } EXPORT_SYMBOL(drm_client_free);

+static void drm_client_remove_locked(struct drm_client_dev *client) +{

• list_del(&client->list);
• if (!client->funcs->remove || !client->funcs->remove(client))

• drm_client_free(client);
  

Ugh, I don't like this semantics of ->remove. Imo you really only want an ->unregister here, an _not want to automatically free the client. Automaticlly freeing the client prevents embedded, and all around smells a bit like midlayer mistake.

Without the automatic freeing you also don't need the return value, and you can have a more standard void return type for your unregister function.

We might need to make sure the drm_dev_unregister doesn't free up the drm_device while we do that, so might need a temporary reference. But I don't think we need anything else to avoid locking headaches.

+}

+static void drm_client_remove_safe(struct drm_device *dev,

• 		   struct drm_client_dev *client)
  

+{

• struct drm_client_dev *iter;
• mutex_lock(&dev->clientlist_mutex);
• list_for_each_entry(iter, &dev->clientlist, list) {

• if (iter == client) {
  

• 	drm_client_remove_locked(client);
  

• 	break;
  

• }
  

• }
• mutex_unlock(&dev->clientlist_mutex);

+}

+/**

• • drm_client_remove - Remove client
• • @client: Client
• • Remove the DRM client.
• */

+void drm_client_remove(struct drm_client_dev *client) +{

• struct drm_device *dev;
• if (!client)

• return;
  

• dev = client->dev;
• /* Hold a reference since the client might drop the last one */
• drm_dev_get(dev);
• drm_client_remove_safe(dev, client);
• drm_dev_put(dev);

+} +EXPORT_SYMBOL(drm_client_remove);

No idea what's this for, seems unused. Please remove - we can readd later on.

+struct drm_client_remove_defer {

• struct list_head list;
• struct drm_device *dev;
• struct drm_client_dev *client;

+};

+static LIST_HEAD(drm_client_remove_defer_list); +static DEFINE_MUTEX(drm_client_remove_defer_list_lock);

+static void drm_client_remove_defer_work_fn(struct work_struct *work) +{

• struct drm_client_remove_defer *defer, *tmp;
• mutex_lock(&drm_client_remove_defer_list_lock);
• list_for_each_entry_safe(defer, tmp, &drm_client_remove_defer_list, list) {

• drm_client_remove_safe(defer->dev, defer->client);
  

• drm_dev_put(defer->dev);
  

• list_del(&defer->list);
  

• kfree(defer);
  

• }
• mutex_unlock(&drm_client_remove_defer_list_lock);

+}

+static DECLARE_WORK(drm_client_remove_defer_work, drm_client_remove_defer_work_fn);

+/**

• • drm_client_remove_defer - Deferred client removal
• • @client: Client
• • Defer client removal to a worker. This makes it possible for a client running
• • in a worker to remove itself.
• • Returns:
• • Zero on success, or -ENOMEM on allocation failure.
• */

+int drm_client_remove_defer(struct drm_client_dev *client) +{

• struct drm_client_remove_defer *defer;
• if (!client)

• return 0;
  

• defer = kzalloc(sizeof(*defer), GFP_KERNEL);
• if (!defer)

• return -ENOMEM;
  

• defer->dev = client->dev;
• defer->client = client;
• drm_dev_get(client->dev);
• mutex_lock(&drm_client_remove_defer_list_lock);
• list_add(&defer->list, &drm_client_remove_defer_list);
• mutex_unlock(&drm_client_remove_defer_list_lock);
• schedule_work(&drm_client_remove_defer_work);
• return 0;

+} +EXPORT_SYMBOL(drm_client_remove_defer);

+void drm_client_exit(void) +{

• flush_work(&drm_client_remove_defer_work);

+}

All the above deferred client removal seems unused. And I can't come up with a reason for why we need this. Please remove, we can add it when we need it later on.

+void drm_client_dev_unregister(struct drm_device *dev) +{

• struct drm_client_dev *client, *tmp;
• if (!drm_core_check_feature(dev, DRIVER_MODESET))

• return;
  

• mutex_lock(&dev->clientlist_mutex);
• list_for_each_entry_safe(client, tmp, &dev->clientlist, list)

• drm_client_remove_locked(client);
  

• mutex_unlock(&dev->clientlist_mutex);

+}

+void drm_client_dev_hotplug(struct drm_device *dev) +{

• struct drm_client_dev *client;
• int ret;
• if (!drm_core_check_feature(dev, DRIVER_MODESET))

• return;
  

• mutex_lock(&dev->clientlist_mutex);
• list_for_each_entry(client, &dev->clientlist, list) {

• if (!client->funcs->hotplug)
  

• 	continue;
  

• ret = client->funcs->hotplug(client);
  

• DRM_DEV_DEBUG_KMS(dev->dev, "%s: ret=%d\n", client->funcs->name, ret);
  

• }
• mutex_unlock(&dev->clientlist_mutex);

+} +EXPORT_SYMBOL(drm_client_dev_hotplug);

+void drm_client_dev_lastclose(struct drm_device *dev) +{

• struct drm_client_dev *client;
• int ret;
• if (!drm_core_check_feature(dev, DRIVER_MODESET))

• return;
  

• mutex_lock(&dev->clientlist_mutex);
• list_for_each_entry(client, &dev->clientlist, list) {

• if (!client->funcs->lastclose)
  

• 	continue;
  

• ret = client->funcs->lastclose(client);
  

Hm, it's called from lastclose context, but conceptually this isn't what's happening at all. I'd call both the callback and the function here ->restore respectively drm_client_dev_restore. Makes it much clearer what's going on, and why this one here is special.

• DRM_DEV_DEBUG_KMS(dev->dev, "%s: ret=%d\n", client->funcs->name, ret);
  

• if (!ret) /* The first one to return zero gets the privilege to restore */
  

• 	break;
  

• }
• mutex_unlock(&dev->clientlist_mutex);

+}

static void drm_client_buffer_delete(struct drm_client_buffer *buffer) { struct drm_device *dev; @@ -218,6 +431,9 @@ static int drm_client_buffer_addfb(struct drm_client_buffer *buffer, /* drop the reference we picked up in framebuffer lookup */ drm_framebuffer_put(buffer->fb);

• if (client->funcs)

• strscpy(buffer->fb->comm, client->funcs->name, TASK_COMM_LEN);
  

Hm, small nit: I'd keep the client name in drm_client_dev, not in the funcs structure. That way you could dynamically assign it if needed. The important part is to keep the functions separate from anything that can change, so that we can put it into protected ro memory.

Feel free to ignore this :-)

• return 0;

}

@@ -265,3 +481,31 @@ void drm_client_framebuffer_delete(struct drm_client_buffer *buffer) drm_client_buffer_delete(buffer); } EXPORT_SYMBOL(drm_client_framebuffer_delete);

+#ifdef CONFIG_DEBUG_FS +static int drm_client_debugfs_internal_clients(struct seq_file *m, void *data) +{

• struct drm_info_node *node = m->private;
• struct drm_device *dev = node->minor->dev;
• struct drm_printer p = drm_seq_file_printer(m);
• struct drm_client_dev *client;
• mutex_lock(&dev->clientlist_mutex);
• list_for_each_entry(client, &dev->clientlist, list)

• drm_printf(&p, "%s\n", client->funcs->name);
  

• mutex_unlock(&dev->clientlist_mutex);
• return 0;

+}

+static const struct drm_info_list drm_client_debugfs_list[] = {

• { "internal_clients", drm_client_debugfs_internal_clients, 0 },

+};

+int drm_client_debugfs_init(struct drm_minor *minor) +{

• return drm_debugfs_create_files(drm_client_debugfs_list,

• 			ARRAY_SIZE(drm_client_debugfs_list),
  

• 			minor->debugfs_root, minor);
  

+} +#endif diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c index b2482818fee8..50a20bfc07ea 100644 --- a/drivers/gpu/drm/drm_debugfs.c +++ b/drivers/gpu/drm/drm_debugfs.c @@ -28,6 +28,7 @@ #include <linux/slab.h> #include <linux/export.h>

+#include <drm/drm_client.h> #include <drm/drm_debugfs.h> #include <drm/drm_edid.h> #include <drm/drm_atomic.h> @@ -164,6 +165,12 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id, DRM_ERROR("Failed to create framebuffer debugfs file\n"); return ret; }

• ret = drm_client_debugfs_init(minor);
  

• if (ret) {
  

• 	DRM_ERROR("Failed to create client debugfs file\n");
  

• 	return ret;
  

• }
  

  }

  if (dev->driver->debugfs_init) {

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 67ac793a7108..d7c1ceebd0de 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -34,6 +34,7 @@ #include <linux/slab.h> #include <linux/srcu.h>

+#include <drm/drm_client.h> #include <drm/drm_drv.h> #include <drm/drmP.h>

@@ -506,6 +507,7 @@ int drm_dev_init(struct drm_device *dev,

INIT_LIST_HEAD(&dev->filelist); INIT_LIST_HEAD(&dev->filelist_internal);

• INIT_LIST_HEAD(&dev->clientlist); INIT_LIST_HEAD(&dev->ctxlist); INIT_LIST_HEAD(&dev->vmalist); INIT_LIST_HEAD(&dev->maplist);

@@ -515,6 +517,7 @@ int drm_dev_init(struct drm_device *dev, spin_lock_init(&dev->event_lock); mutex_init(&dev->struct_mutex); mutex_init(&dev->filelist_mutex);

• mutex_init(&dev->clientlist_mutex); mutex_init(&dev->ctxlist_mutex); mutex_init(&dev->master_mutex);

@@ -570,6 +573,7 @@ int drm_dev_init(struct drm_device *dev, err_free: mutex_destroy(&dev->master_mutex); mutex_destroy(&dev->ctxlist_mutex);

• mutex_destroy(&dev->clientlist_mutex); mutex_destroy(&dev->filelist_mutex); mutex_destroy(&dev->struct_mutex); return ret;

@@ -604,6 +608,7 @@ void drm_dev_fini(struct drm_device *dev)

mutex_destroy(&dev->master_mutex); mutex_destroy(&dev->ctxlist_mutex);

• mutex_destroy(&dev->clientlist_mutex); mutex_destroy(&dev->filelist_mutex); mutex_destroy(&dev->struct_mutex); kfree(dev->unique);

@@ -854,6 +859,8 @@ void drm_dev_unregister(struct drm_device *dev) { struct drm_map_list *r_list, *list_temp;

• drm_client_dev_unregister(dev);
• if (drm_core_check_feature(dev, DRIVER_LEGACY)) drm_lastclose(dev);

@@ -959,6 +966,7 @@ static const struct file_operations drm_stub_fops = {

static void drm_core_exit(void) {

• drm_client_exit(); unregister_chrdev(DRM_MAJOR, "drm"); debugfs_remove(drm_debugfs_root); drm_sysfs_destroy();

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 76e2f7977779..aff3c215d9e5 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -180,7 +180,7 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev, if (!fbdev_cma) return ERR_PTR(-ENOMEM);

• client = drm_client_new(dev);

• client = drm_client_new(dev, NULL); if (IS_ERR(client)) { ret = PTR_ERR(client); goto err_free;

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index 55505378df47..bcc688e58776 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -35,6 +35,7 @@ #include <linux/slab.h> #include <linux/module.h>

+#include <drm/drm_client.h> #include <drm/drm_file.h> #include <drm/drmP.h>

@@ -443,6 +444,8 @@ void drm_lastclose(struct drm_device * dev)

if (drm_core_check_feature(dev, DRIVER_LEGACY)) drm_legacy_dev_reinit(dev);

• drm_client_dev_lastclose(dev);

}

/** diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 527743394150..26be57e28a9d 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -33,6 +33,7 @@ #include <linux/moduleparam.h>

#include <drm/drmP.h> +#include <drm/drm_client.h> #include <drm/drm_crtc.h> #include <drm/drm_fourcc.h> #include <drm/drm_crtc_helper.h> @@ -563,6 +564,8 @@ void drm_kms_helper_hotplug_event(struct drm_device *dev) drm_sysfs_hotplug_event(dev); if (dev->mode_config.funcs->output_poll_changed) dev->mode_config.funcs->output_poll_changed(dev);

• drm_client_dev_hotplug(dev);

} EXPORT_SYMBOL(drm_kms_helper_hotplug_event);

diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h index 11379eaf3118..011a87ad3406 100644 --- a/include/drm/drm_client.h +++ b/include/drm/drm_client.h @@ -5,11 +5,56 @@

#include <linux/types.h>

+struct drm_client_dev; struct drm_device; struct drm_framebuffer; struct drm_gem_object; struct drm_minor;

+/**

• • struct drm_client_funcs - DRM client callbacks
• */

+struct drm_client_funcs {

• /**

• * @name:
  

• *
  

• * Name of the client. Mandatory.
  

• */
  

• const char *name;
• /**

• * @remove:
  

• *
  

• * Called when a &drm_device is unregistered or the client is
  

• * unregistered. If zero is returned drm_client_free() is called
  

• * automatically. If the client can't drop it's resources it should
  

• * return non-zero and call drm_client_free() later.
  

• *
  

• * This callback is optional.
  

• */
  

• int (*remove)(struct drm_client_dev *client);

s/remove/unregister/ and void return type. See above.

Also we should document here that once ->unregister is call, we will no longer call ->restore nor ->hotplug.

• /**

• * @lastclose:
  

• *
  

• * Called on drm_lastclose(). The first client instance in the list
  

• * that returns zero gets the privilege to restore and no more clients
  

• * are called.
  

• *
  

• * This callback is optional.
  

• */
  

• int (*lastclose)(struct drm_client_dev *client);

I'd call this restore. Makes it more obvious what it's for.

• /**

• * @hotplug:
  

• *
  

• * Called on drm_kms_helper_hotplug_event().
  

• *
  

• * This callback is optional.
  

• */
  

• int (*hotplug)(struct drm_client_dev *client);

+};

/**

• struct drm_client_dev - DRM client instance

*/ @@ -27,6 +72,11 @@ struct drm_client_dev { */ struct drm_device *dev;

• /**

• * @funcs: DRM client functions
  

• */
  

• const struct drm_client_funcs *funcs;
• /**
  • @file: DRM file
  */

@@ -39,7 +89,20 @@ struct drm_client_dev { };

void drm_client_free(struct drm_client_dev *client); -struct drm_client_dev *drm_client_new(struct drm_device *dev); +struct drm_client_dev * +drm_client_new(struct drm_device *dev, const struct drm_client_funcs *funcs); +struct drm_client_dev * +drm_client_new_from_id(unsigned int minor_id, const struct drm_client_funcs *funcs); +void drm_client_remove(struct drm_client_dev *client); +int drm_client_remove_defer(struct drm_client_dev *client);

+void drm_client_exit(void);

+void drm_client_dev_unregister(struct drm_device *dev); +void drm_client_dev_hotplug(struct drm_device *dev); +void drm_client_dev_lastclose(struct drm_device *dev);

+int drm_client_debugfs_init(struct drm_minor *minor);

/**

• struct drm_client_buffer - DRM client buffer

diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h index 9e29976d4e98..f9c6e0e3aec7 100644 --- a/include/drm/drm_device.h +++ b/include/drm/drm_device.h @@ -81,6 +81,20 @@ struct drm_device { */ struct list_head filelist_internal;

• /**

• * @clientlist_mutex:
  

• *
  

• * Protects @clientlist access.
  

• */
  

• struct mutex clientlist_mutex;
• /**

• * @clientlist:
  

• *
  

• * List of in-kernel clients. Protected by @clientlist_mutex.
  

• */
  

• struct list_head clientlist;
• /** \name Memory management */ /*@{ */ struct list_head maplist; /**< Linked list of regions */

lgtm overall, just a few nits wrt naming stuff, and splitting things out that we dont (yet) need. Plus the unregister semantics don't look fully thought out to me yet. -Daniel

-- 2.15.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Wed, May 23, 2018 at 04:34:11PM +0200, Noralf Trønnes wrote:

This adds a drm_fbdev_generic_setup() function that sets up generic fbdev emulation with client callbacks for lastclose, hotplug and remove/unregister.

Signed-off-by: Noralf Trønnes noralf@tronnes.org

drivers/gpu/drm/drm_fb_helper.c | 123 ++++++++++++++++++++++++++++++++++++++++ include/drm/drm_fb_helper.h | 7 +++ 2 files changed, 130 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 444c2b4040ea..bc826457eb84 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -68,6 +68,9 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);

• helper functions used by many drivers to implement the kernel mode setting
• interfaces.

• • Drivers that support a dumb buffer with a virtual address and mmap support,
• • should try and use the generic fbdev emulation using drm_fbdev_generic_setup().
• • Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it
  • down by calling drm_fb_helper_fbdev_teardown().

@@ -3092,6 +3095,126 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, } EXPORT_SYMBOL(drm_fb_helper_generic_probe);

+static const struct drm_fb_helper_funcs drm_fb_helper_generic_funcs = {

• .fb_probe = drm_fb_helper_generic_probe,

+};

+static int drm_fbdev_client_remove(struct drm_client_dev *client) +{

• struct drm_fb_helper *fb_helper = client->private;
• /* Maybe setup was tried but failed */
• if (!fb_helper)

• return 0;
  

• /* Maybe drm_fb_helper_fbdev_setup() hasn't run yet */
• if (!fb_helper->dev) {

• kfree(fb_helper);
  

• return 0;
  

• }
• /* Maybe drm_fb_helper_generic_probe() hasn't run yet */
• if (!fb_helper->fbdev) {

• drm_fb_helper_fini(fb_helper);
  

• kfree(fb_helper);
  

• return 0;
  

• }
• unregister_framebuffer(fb_helper->fbdev);

Hm, I'd expect nice problems with recursion and locking here, since we clean up a lot of drm stuff from deeply within fbdev callbacks. I think moving that cleanup code from your fb_destroy callback to here would look a bit cleaner, and avoids the risk of going boom while holding the console_lock. Which is always really, really nasty.

• /*

• * If userspace is closed the client is now freed by drm_fbdev_fb_destroy(),
  

• * otherwise it will be freed on the last close.
  

• * Return 1 to tell that freeing is taken care of.
  

• */
  

• return 1;

Yeah that's definitely magic midlayer, we don't want the core to free stuff for us and then pass around return values to tell it not to.

+}

+static int drm_fbdev_client_lastclose(struct drm_client_dev *client) +{

• drm_fb_helper_restore_fbdev_mode_unlocked(client->private);
• return 0;

+}

+static int drm_fbdev_client_hotplug(struct drm_client_dev *client) +{

• struct drm_fb_helper *fb_helper = client->private;
• struct drm_device *dev = client->dev;
• int ret;
• if (dev->fb_helper)

• return drm_fb_helper_hotplug_event(dev->fb_helper);
  

• if (!dev->mode_config.num_connector || !fb_helper)

• return 0;
  

• ret = drm_fb_helper_fbdev_setup(dev, fb_helper, &drm_fb_helper_generic_funcs,

• 			fb_helper->preferred_bpp, 0);
  

• if (ret) {

• /* Setup is tried only once */
  

• client->private = NULL;
  

• kfree(fb_helper);
  

• return ret;
  

• }
• return 0;

+}

+static const struct drm_client_funcs drm_fbdev_client_funcs = {

• .name = "fbdev",
• .remove = drm_fbdev_client_remove,
• .lastclose = drm_fbdev_client_lastclose,
• .hotplug = drm_fbdev_client_hotplug,

+};

+/**

• • drm_fb_helper_generic_fbdev_setup() - Setup generic fbdev emulation
• • @dev: DRM device
• • @preferred_bpp: Preferred bits per pixel for the device.

• •             @dev->mode_config.preferred_depth is used if this is zero.
    

• • This function sets up generic fbdev emulation for drivers that supports
• • dumb buffers with a virtual address and that can be mmap'ed.
• • Restore, hotplug events and teardown are all taken care of. Drivers that does
• • 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.

I think we should spend a few more words on the limitations of the generic fbdev support. Probably best to reference the generic_probe callback for that (and make sure all the limitations, like defio vs. not vmalloced buffers) are documented there.

lgtm otherwise. With the destroy story cleanup up and the kerneldoc polished also has my

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

• • Returns:
• • Zero on success or negative error code on failure.
• */

+int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp) +{

• struct drm_fb_helper *fb_helper;
• struct drm_client_dev *client;
• if (!drm_fbdev_emulation)

• return 0;
  

• fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
• if (!fb_helper)

• return -ENOMEM;
  

• client = drm_client_new(dev, &drm_fbdev_client_funcs);
• if (IS_ERR(client)) {

• kfree(fb_helper);
  

• return PTR_ERR(client);
  

• }
• client->private = fb_helper;
• fb_helper->client = client;
• fb_helper->preferred_bpp = preferred_bpp;
• drm_fbdev_client_hotplug(client);
• return 0;

+} +EXPORT_SYMBOL(drm_fbdev_generic_setup);

/* The Kconfig DRM_KMS_HELPER selects FRAMEBUFFER_CONSOLE (if !EXPERT)

• but the module doesn't depend on any fb console symbols. At least
• attempt to load fbcon to avoid leaving the system without a usable console.

diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index bb38469a9502..884eabbc54d5 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -349,6 +349,7 @@ void drm_fb_helper_output_poll_changed(struct drm_device *dev);

int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, struct drm_fb_helper_surface_size *sizes); +int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp); #else static inline void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper, @@ -590,6 +591,12 @@ drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, return 0; }

+static inline int +drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp) +{

• return 0;

+}

#endif

static inline int

2.15.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel