Re: [PATCH 4/9] drm: Begin an API for in-kernel clients

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