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

From: Daniel Vetter <daniel@ffwll.ch>
To: "Noralf Trønnes" <noralf@tronnes.org>
Cc: intel-gfx@lists.freedesktop.org,
	laurent.pinchart@ideasonboard.com,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 4/9] drm: Begin an API for in-kernel clients
Date: Thu, 24 May 2018 10:42:13 +0200	[thread overview]
Message-ID: <20180524084213.GG3438@phenom.ffwll.local> (raw)
In-Reply-To: <20180523143411.64070-5-noralf@tronnes.org>

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx