| Message ID | 20180523143411.64070-5-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Noralf. A couple of issues below: On 05/23/2018 04:34 PM, 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. Couldn't you add vmap() and vunmap() to the dumb buffer API for in-kernel use rather than using GEM directly? But the main issue is pinning. It looks like the buffers are going to be vmapped() for a long time, which requires pinning, and that doesn't work for some drivers when they bind the framebuffer to a plane, since that might require pinning in another memory region and the vmap would have to be torn down. Besides, buffer pinning should really be avoided if possible: Since we can't page-fault vmaps, and setting up / tearing down vmaps is potentially an expensive operation, could we perhaps have a mapping api that allows the driver to cache vmaps? vmap() // Indicates that we want to map a bo begin_access() // Returns a virtual address which may vary between calls. Allows access. A fast operation. Behind the lines pins / reserves the bo and returns a cached vmap if the bo didn't move since last begin_access(), which is the typical case. end_access() // Disable access. Unpins / unreserves the bo. vunmap_cached() //Indicates that the map is no longer needed. The driver can release the cached map. The idea is that the API client would wrap all bo map accesses with begin_access() end_access(), allowing for the bo to be moved in between. /Thomas
On Wed, May 23, 2018 at 11:45:00PM +0200, Thomas Hellstrom wrote: > Hi, Noralf. > > A couple of issues below: > > On 05/23/2018 04:34 PM, 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. > > Couldn't you add vmap() and vunmap() to the dumb buffer API for in-kernel > use rather than using GEM directly? > > But the main issue is pinning. It looks like the buffers are going to be > vmapped() for a long time, which requires pinning, and that doesn't work for > some drivers when they bind the framebuffer to a plane, since that might > require pinning in another memory region and the vmap would have to be torn > down. Besides, buffer pinning should really be avoided if possible: > > Since we can't page-fault vmaps, and setting up / tearing down vmaps is > potentially an expensive operation, could we perhaps have a mapping api that > allows the driver to cache vmaps? > > vmap() // Indicates that we want to map a bo > begin_access() // Returns a virtual address which may vary between calls. > Allows access. A fast operation. Behind the lines pins / reserves the bo and > returns a cached vmap if the bo didn't move since last begin_access(), which > is the typical case. > end_access() // Disable access. Unpins / unreserves the bo. > vunmap_cached() //Indicates that the map is no longer needed. The driver can > release the cached map. > > The idea is that the API client would wrap all bo map accesses with > begin_access() end_access(), allowing for the bo to be moved in between. So originally my ideas for the cpu side dma-buf interfaces where all meant to handle this. But then the first implementations bothered with none of this, but instead expected that stuff is pinned, and vmap Just Works. Which yeah doesn't work for vmwgfx and is a pain in a few other cases. I agree it'd be nice to fix all this, but it's also not a problem that this patch set here started. And since it's all optional (and vmwgfx isn't even using the current fb helper code) I think it's reasonable to address this post-merge (if someone gets around to it ever). What we'd need is is a fallback for when vmap doesn't exist (for fbdev that probably means a vmalloc'ed buffer + manual uploads, because fbdev), plus making sure dma-buf implementations actually implement it. Wrt tying this to gem hooks: I don't think this should be needed, I thought we've discussed a way to get at the handle2fd logic without having to end up with an fd, but directly return the dma-buf instead. Then we could use the dma-buf vmap interfaces instead of the gem ones. But getting there means we need to rework all the dma-buf export functions to move the fd_install into core code, so it's possible to make it optional. Not difficult work, but I think not necessary for the first merged version either since fairly auxiliary. Also right now there's no demand for this, since vmwgfx isn't using the fb helpers (yet). But we do have a solid plan to remove the gem dependency at least. -Daniel
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 05/24/2018 10:32 AM, Daniel Vetter wrote: > On Wed, May 23, 2018 at 11:45:00PM +0200, Thomas Hellstrom wrote: >> Hi, Noralf. >> >> A couple of issues below: >> >> On 05/23/2018 04:34 PM, 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. >> Couldn't you add vmap() and vunmap() to the dumb buffer API for in-kernel >> use rather than using GEM directly? >> >> But the main issue is pinning. It looks like the buffers are going to be >> vmapped() for a long time, which requires pinning, and that doesn't work for >> some drivers when they bind the framebuffer to a plane, since that might >> require pinning in another memory region and the vmap would have to be torn >> down. Besides, buffer pinning should really be avoided if possible: >> >> Since we can't page-fault vmaps, and setting up / tearing down vmaps is >> potentially an expensive operation, could we perhaps have a mapping api that >> allows the driver to cache vmaps? >> >> vmap() // Indicates that we want to map a bo >> begin_access() // Returns a virtual address which may vary between calls. >> Allows access. A fast operation. Behind the lines pins / reserves the bo and >> returns a cached vmap if the bo didn't move since last begin_access(), which >> is the typical case. >> end_access() // Disable access. Unpins / unreserves the bo. >> vunmap_cached() //Indicates that the map is no longer needed. The driver can >> release the cached map. >> >> The idea is that the API client would wrap all bo map accesses with >> begin_access() end_access(), allowing for the bo to be moved in between. > So originally my ideas for the cpu side dma-buf interfaces where all meant > to handle this. But then the first implementations bothered with none of > this, but instead expected that stuff is pinned, and vmap Just Works. > > Which yeah doesn't work for vmwgfx and is a pain in a few other cases. > > I agree it'd be nice to fix all this, but it's also not a problem that > this patch set here started. And since it's all optional (and vmwgfx isn't > even using the current fb helper code) I think it's reasonable to address > this post-merge (if someone gets around to it ever). What we'd need is is > a fallback for when vmap doesn't exist (for fbdev that probably means a > vmalloc'ed buffer + manual uploads, because fbdev), plus making sure > dma-buf implementations actually implement it. My argument here is that, If I understand Noralf, this is intended to be an API exported outside of drm. In that case we shouldn't replicate the assumed behaviour of incomplete dma-buf implementations in a new API. Also the fact that vmwgfx currently isn't using the fbdev helpers isn't a good argument to design an API so that vmwgfx can _never_ use the fbdev helpers. The reason we aren't using them is that the kms implementation was so old that we didn't implement the necessary helper callbacks... Also, I might be misunderstanding the code a bit, but I doubt that vmwgfx is the only hardware with pinning restrictions on the framebuffer? I was under the assumption that most discrete hardware required the framebuffer to be pinned in VRAM? So the important question is, Is this a set of helpers for shared-memory GEM drivers to implement fbdev? Then I wouldn't bother, If it's intended to become an API for clients outside of DRM, then I would have to insist on the API being changed to reflect that. /Thomas
On Thu, May 24, 2018 at 11:25:04AM +0200, Thomas Hellstrom wrote: > On 05/24/2018 10:32 AM, Daniel Vetter wrote: > > On Wed, May 23, 2018 at 11:45:00PM +0200, Thomas Hellstrom wrote: > > > Hi, Noralf. > > > > > > A couple of issues below: > > > > > > On 05/23/2018 04:34 PM, 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. > > > Couldn't you add vmap() and vunmap() to the dumb buffer API for in-kernel > > > use rather than using GEM directly? > > > > > > But the main issue is pinning. It looks like the buffers are going to be > > > vmapped() for a long time, which requires pinning, and that doesn't work for > > > some drivers when they bind the framebuffer to a plane, since that might > > > require pinning in another memory region and the vmap would have to be torn > > > down. Besides, buffer pinning should really be avoided if possible: > > > > > > Since we can't page-fault vmaps, and setting up / tearing down vmaps is > > > potentially an expensive operation, could we perhaps have a mapping api that > > > allows the driver to cache vmaps? > > > > > > vmap() // Indicates that we want to map a bo > > > begin_access() // Returns a virtual address which may vary between calls. > > > Allows access. A fast operation. Behind the lines pins / reserves the bo and > > > returns a cached vmap if the bo didn't move since last begin_access(), which > > > is the typical case. > > > end_access() // Disable access. Unpins / unreserves the bo. > > > vunmap_cached() //Indicates that the map is no longer needed. The driver can > > > release the cached map. > > > > > > The idea is that the API client would wrap all bo map accesses with > > > begin_access() end_access(), allowing for the bo to be moved in between. > > So originally my ideas for the cpu side dma-buf interfaces where all meant > > to handle this. But then the first implementations bothered with none of > > this, but instead expected that stuff is pinned, and vmap Just Works. > > > > Which yeah doesn't work for vmwgfx and is a pain in a few other cases. > > > > I agree it'd be nice to fix all this, but it's also not a problem that > > this patch set here started. And since it's all optional (and vmwgfx isn't > > even using the current fb helper code) I think it's reasonable to address > > this post-merge (if someone gets around to it ever). What we'd need is is > > a fallback for when vmap doesn't exist (for fbdev that probably means a > > vmalloc'ed buffer + manual uploads, because fbdev), plus making sure > > dma-buf implementations actually implement it. > > My argument here is that, If I understand Noralf, this is intended to be an > API exported outside of drm. In that case we shouldn't replicate the assumed > behaviour of incomplete dma-buf implementations in a new API. Also the fact > that vmwgfx currently isn't using the fbdev helpers isn't a good argument to > design an API so that vmwgfx can _never_ use the fbdev helpers. The reason > we aren't using them is that the kms implementation was so old that we > didn't implement the necessary helper callbacks... > > Also, I might be misunderstanding the code a bit, but I doubt that vmwgfx is > the only hardware with pinning restrictions on the framebuffer? I was under > the assumption that most discrete hardware required the framebuffer to be > pinned in VRAM? > > So the important question is, Is this a set of helpers for shared-memory GEM > drivers to implement fbdev? Then I wouldn't bother, If it's intended to > become an API for clients outside of DRM, then I would have to insist on the > API being changed to reflect that. This is definitely not an api for anything outside of drm. Just an attempt to consolidate kernel-internal drm access like fbdev, a simple bootsplash or an emergency console would need to do. Having some limitations in the initial versions, as long as we have some idea how to handle them, seems perfectly fine to me. This isn't meant to be a mandatory replacement for anything - no intentions of exporting this to userspace. And yes the pinning aspect is really ugly, but that's by far not the only annoying aspect of trying to emulate fbdev on top of kms. defio handling is another really problematic thing. I think we won't ever reach a 100% solution on this. That's why I asked Noralf to rework his series to tie into the existing ->fb_probe callback, since converting everyone over to this isn't likely. -Daniel
On 05/24/2018 12:14 PM, Daniel Vetter wrote: > On Thu, May 24, 2018 at 11:25:04AM +0200, Thomas Hellstrom wrote: >> On 05/24/2018 10:32 AM, Daniel Vetter wrote: >>> On Wed, May 23, 2018 at 11:45:00PM +0200, Thomas Hellstrom wrote: >>>> Hi, Noralf. >>>> >>>> A couple of issues below: >>>> >>>> On 05/23/2018 04:34 PM, 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. >>>> Couldn't you add vmap() and vunmap() to the dumb buffer API for in-kernel >>>> use rather than using GEM directly? >>>> >>>> But the main issue is pinning. It looks like the buffers are going to be >>>> vmapped() for a long time, which requires pinning, and that doesn't work for >>>> some drivers when they bind the framebuffer to a plane, since that might >>>> require pinning in another memory region and the vmap would have to be torn >>>> down. Besides, buffer pinning should really be avoided if possible: >>>> >>>> Since we can't page-fault vmaps, and setting up / tearing down vmaps is >>>> potentially an expensive operation, could we perhaps have a mapping api that >>>> allows the driver to cache vmaps? >>>> >>>> vmap() // Indicates that we want to map a bo >>>> begin_access() // Returns a virtual address which may vary between calls. >>>> Allows access. A fast operation. Behind the lines pins / reserves the bo and >>>> returns a cached vmap if the bo didn't move since last begin_access(), which >>>> is the typical case. >>>> end_access() // Disable access. Unpins / unreserves the bo. >>>> vunmap_cached() //Indicates that the map is no longer needed. The driver can >>>> release the cached map. >>>> >>>> The idea is that the API client would wrap all bo map accesses with >>>> begin_access() end_access(), allowing for the bo to be moved in between. >>> So originally my ideas for the cpu side dma-buf interfaces where all meant >>> to handle this. But then the first implementations bothered with none of >>> this, but instead expected that stuff is pinned, and vmap Just Works. >>> >>> Which yeah doesn't work for vmwgfx and is a pain in a few other cases. >>> >>> I agree it'd be nice to fix all this, but it's also not a problem that >>> this patch set here started. And since it's all optional (and vmwgfx isn't >>> even using the current fb helper code) I think it's reasonable to address >>> this post-merge (if someone gets around to it ever). What we'd need is is >>> a fallback for when vmap doesn't exist (for fbdev that probably means a >>> vmalloc'ed buffer + manual uploads, because fbdev), plus making sure >>> dma-buf implementations actually implement it. >> My argument here is that, If I understand Noralf, this is intended to be an >> API exported outside of drm. In that case we shouldn't replicate the assumed >> behaviour of incomplete dma-buf implementations in a new API. Also the fact >> that vmwgfx currently isn't using the fbdev helpers isn't a good argument to >> design an API so that vmwgfx can _never_ use the fbdev helpers. The reason >> we aren't using them is that the kms implementation was so old that we >> didn't implement the necessary helper callbacks... >> >> Also, I might be misunderstanding the code a bit, but I doubt that vmwgfx is >> the only hardware with pinning restrictions on the framebuffer? I was under >> the assumption that most discrete hardware required the framebuffer to be >> pinned in VRAM? >> >> So the important question is, Is this a set of helpers for shared-memory GEM >> drivers to implement fbdev? Then I wouldn't bother, If it's intended to >> become an API for clients outside of DRM, then I would have to insist on the >> API being changed to reflect that. > This is definitely not an api for anything outside of drm. Just an attempt > to consolidate kernel-internal drm access like fbdev, a simple bootsplash > or an emergency console would need to do. Having some limitations in the > initial versions, as long as we have some idea how to handle them, seems > perfectly fine to me. This isn't meant to be a mandatory replacement for > anything - no intentions of exporting this to userspace. > OK, yeah my concern is really for generic code and that there in the end would be too much code to change if we wanted to support this, but at least the generic code would be somewhat contained. But it seems like we're at least in agreement on the problematic areas, and as long as they are open for change I guess we can live with that. /Thomas
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); >> +}
On Mon, May 28, 2018 at 03:26:48PM +0200, Noralf Trønnes wrote: > > 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(). Yeah I noticed later on. I think rechecking for safety here can't hurt, but up to you. -Daniel > > 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); > > > +} > >
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) +{ + 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) +{ + 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; + + 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) +{ + 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; +}; + +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 */
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> --- 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