Den 24.05.2018 10.42, skrev Daniel Vetter:
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
[...]
+/**
- 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);
[...]
+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.
This is already checked in drm_client_init().
Noralf.
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);
+}