On Mon, Oct 22, 2018 at 02:57:28PM +0200, Christian König wrote:
> Am 17.10.18 um 15:04 schrieb Noralf Trønnes:
> > This adds an optional function table on GEM objects.
> > The main benefit is for drivers that support more than one type of
> > memory (shmem,vram,cma) for their buffers depending on the hardware it
> > runs on. With the callbacks attached to the GEM object itself, it is
> > easier to have core helpers for the the various buffer types. The driver
> > only has to make the decision about buffer type on GEM object creation
> > and all other callbacks can be handled by the chosen helper.
> >
> > drm_driver->gem_prime_res_obj has not been added since there's a todo to
> > put a reservation_object into drm_gem_object.
> >
> > v2: Drop drm_gem_object_funcs->prime_mmap in favour of
> > drm_gem_prime_mmap() (Daniel Vetter)
> >
> > v1:
> > - drm_gem_object_funcs.map -> .prime_map let it only do PRIME mmap like
> > the function it superseeds (Daniel Vetter)
> > - Flip around the if ladders and make obj->funcs the first choice
> > highlighting the fact that this the new default way of doing it
> > (Daniel Vetter)
>
> Well could we please make this mandatory and convert drivers bit by bit?
Yeah I'm all for adding a new entry to todo.rst and getting this rolling.
If there's support (which seems so).
-Daniel
>
> Christian.
>
> >
> > Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> > Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > ---
> > drivers/gpu/drm/drm_client.c | 12 ++--
> > drivers/gpu/drm/drm_gem.c | 109 ++++++++++++++++++++++++++++++++---
> > drivers/gpu/drm/drm_prime.c | 34 ++++++-----
> > include/drm/drm_gem.h | 131 +++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 252 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> > index 17d9a64e885e..eca7331762e4 100644
> > --- a/drivers/gpu/drm/drm_client.c
> > +++ b/drivers/gpu/drm/drm_client.c
> > @@ -80,8 +80,7 @@ int 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)
> > + if (!drm_core_check_feature(dev, DRIVER_MODESET) || !dev->driver->dumb_create)
> > return -EOPNOTSUPP;
> > if (funcs && !try_module_get(funcs->owner))
> > @@ -212,8 +211,7 @@ static void drm_client_buffer_delete(struct drm_client_buffer *buffer)
> > {
> > struct drm_device *dev = buffer->client->dev;
> > - if (buffer->vaddr && dev->driver->gem_prime_vunmap)
> > - dev->driver->gem_prime_vunmap(buffer->gem, buffer->vaddr);
> > + drm_gem_vunmap(buffer->gem, buffer->vaddr);
> > if (buffer->gem)
> > drm_gem_object_put_unlocked(buffer->gem);
> > @@ -266,9 +264,9 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u
> > * 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;
> > + vaddr = drm_gem_vmap(obj);
> > + if (IS_ERR(vaddr)) {
> > + ret = PTR_ERR(vaddr);
> > goto err_delete;
> > }
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 512078ebd97b..8b55ece97967 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -257,7 +257,9 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
> > struct drm_gem_object *obj = ptr;
> > struct drm_device *dev = obj->dev;
> > - if (dev->driver->gem_close_object)
> > + if (obj->funcs && obj->funcs->close)
> > + obj->funcs->close(obj, file_priv);
> > + else if (dev->driver->gem_close_object)
> > dev->driver->gem_close_object(obj, file_priv);
> > if (drm_core_check_feature(dev, DRIVER_PRIME))
> > @@ -410,7 +412,11 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
> > if (ret)
> > goto err_remove;
> > - if (dev->driver->gem_open_object) {
> > + if (obj->funcs && obj->funcs->open) {
> > + ret = obj->funcs->open(obj, file_priv);
> > + if (ret)
> > + goto err_revoke;
> > + } else if (dev->driver->gem_open_object) {
> > ret = dev->driver->gem_open_object(obj, file_priv);
> > if (ret)
> > goto err_revoke;
> > @@ -835,7 +841,9 @@ drm_gem_object_free(struct kref *kref)
> > container_of(kref, struct drm_gem_object, refcount);
> > struct drm_device *dev = obj->dev;
> > - if (dev->driver->gem_free_object_unlocked) {
> > + if (obj->funcs) {
> > + obj->funcs->free(obj);
> > + } else if (dev->driver->gem_free_object_unlocked) {
> > dev->driver->gem_free_object_unlocked(obj);
> > } else if (dev->driver->gem_free_object) {
> > WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> > @@ -864,13 +872,13 @@ drm_gem_object_put_unlocked(struct drm_gem_object *obj)
> > dev = obj->dev;
> > - if (dev->driver->gem_free_object_unlocked) {
> > - kref_put(&obj->refcount, drm_gem_object_free);
> > - } else {
> > + if (dev->driver->gem_free_object) {
> > might_lock(&dev->struct_mutex);
> > if (kref_put_mutex(&obj->refcount, drm_gem_object_free,
> > &dev->struct_mutex))
> > mutex_unlock(&dev->struct_mutex);
> > + } else {
> > + kref_put(&obj->refcount, drm_gem_object_free);
> > }
> > }
> > EXPORT_SYMBOL(drm_gem_object_put_unlocked);
> > @@ -960,11 +968,14 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
> > if (obj_size < vma->vm_end - vma->vm_start)
> > return -EINVAL;
> > - if (!dev->driver->gem_vm_ops)
> > + if (obj->funcs && obj->funcs->vm_ops)
> > + vma->vm_ops = obj->funcs->vm_ops;
> > + else if (dev->driver->gem_vm_ops)
> > + vma->vm_ops = dev->driver->gem_vm_ops;
> > + else
> > return -EINVAL;
> > vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
> > - vma->vm_ops = dev->driver->gem_vm_ops;
> > vma->vm_private_data = obj;
> > vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> > vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> > @@ -1066,6 +1077,86 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
> > drm_printf_indent(p, indent, "imported=%s\n",
> > obj->import_attach ? "yes" : "no");
> > - if (obj->dev->driver->gem_print_info)
> > + if (obj->funcs && obj->funcs->print_info)
> > + obj->funcs->print_info(p, indent, obj);
> > + else if (obj->dev->driver->gem_print_info)
> > obj->dev->driver->gem_print_info(p, indent, obj);
> > }
> > +
> > +/**
> > + * drm_gem_pin - Pin backing buffer in memory
> > + * @obj: GEM object
> > + *
> > + * Make sure the backing buffer is pinned in memory.
> > + *
> > + * Returns:
> > + * 0 on success or a negative error code on failure.
> > + */
> > +int drm_gem_pin(struct drm_gem_object *obj)
> > +{
> > + if (obj->funcs && obj->funcs->pin)
> > + return obj->funcs->pin(obj);
> > + else if (obj->dev->driver->gem_prime_pin)
> > + return obj->dev->driver->gem_prime_pin(obj);
> > + else
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(drm_gem_pin);
> > +
> > +/**
> > + * drm_gem_unpin - Unpin backing buffer from memory
> > + * @obj: GEM object
> > + *
> > + * Relax the requirement that the backing buffer is pinned in memory.
> > + */
> > +void drm_gem_unpin(struct drm_gem_object *obj)
> > +{
> > + if (obj->funcs && obj->funcs->unpin)
> > + obj->funcs->unpin(obj);
> > + else if (obj->dev->driver->gem_prime_unpin)
> > + obj->dev->driver->gem_prime_unpin(obj);
> > +}
> > +EXPORT_SYMBOL(drm_gem_unpin);
> > +
> > +/**
> > + * drm_gem_vmap - Map buffer into kernel virtual address space
> > + * @obj: GEM object
> > + *
> > + * Returns:
> > + * A virtual pointer to a newly created GEM object or an ERR_PTR-encoded negative
> > + * error code on failure.
> > + */
> > +void *drm_gem_vmap(struct drm_gem_object *obj)
> > +{
> > + void *vaddr;
> > +
> > + if (obj->funcs && obj->funcs->vmap)
> > + vaddr = obj->funcs->vmap(obj);
> > + else if (obj->dev->driver->gem_prime_vmap)
> > + vaddr = obj->dev->driver->gem_prime_vmap(obj);
> > + else
> > + vaddr = ERR_PTR(-EOPNOTSUPP);
> > +
> > + if (!vaddr)
> > + vaddr = ERR_PTR(-ENOMEM);
> > +
> > + return vaddr;
> > +}
> > +EXPORT_SYMBOL(drm_gem_vmap);
> > +
> > +/**
> > + * drm_gem_vunmap - Remove buffer mapping from kernel virtual address space
> > + * @obj: GEM object
> > + * @vaddr: Virtual address (can be NULL)
> > + */
> > +void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr)
> > +{
> > + if (!vaddr)
> > + return;
> > +
> > + if (obj->funcs && obj->funcs->vunmap)
> > + obj->funcs->vunmap(obj, vaddr);
> > + else if (obj->dev->driver->gem_prime_vunmap)
> > + obj->dev->driver->gem_prime_vunmap(obj, vaddr);
> > +}
> > +EXPORT_SYMBOL(drm_gem_vunmap);
> > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> > index 42abf98c1d4a..e0ab5213efa7 100644
> > --- a/drivers/gpu/drm/drm_prime.c
> > +++ b/drivers/gpu/drm/drm_prime.c
> > @@ -199,7 +199,6 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
> > {
> > struct drm_prime_attachment *prime_attach;
> > struct drm_gem_object *obj = dma_buf->priv;
> > - struct drm_device *dev = obj->dev;
> > prime_attach = kzalloc(sizeof(*prime_attach), GFP_KERNEL);
> > if (!prime_attach)
> > @@ -208,10 +207,7 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
> > prime_attach->dir = DMA_NONE;
> > attach->priv = prime_attach;
> > - if (!dev->driver->gem_prime_pin)
> > - return 0;
> > -
> > - return dev->driver->gem_prime_pin(obj);
> > + return drm_gem_pin(obj);
> > }
> > EXPORT_SYMBOL(drm_gem_map_attach);
> > @@ -228,7 +224,6 @@ void drm_gem_map_detach(struct dma_buf *dma_buf,
> > {
> > struct drm_prime_attachment *prime_attach = attach->priv;
> > struct drm_gem_object *obj = dma_buf->priv;
> > - struct drm_device *dev = obj->dev;
> > if (prime_attach) {
> > struct sg_table *sgt = prime_attach->sgt;
> > @@ -247,8 +242,7 @@ void drm_gem_map_detach(struct dma_buf *dma_buf,
> > attach->priv = NULL;
> > }
> > - if (dev->driver->gem_prime_unpin)
> > - dev->driver->gem_prime_unpin(obj);
> > + drm_gem_unpin(obj);
> > }
> > EXPORT_SYMBOL(drm_gem_map_detach);
> > @@ -310,7 +304,10 @@ struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
> > if (WARN_ON(prime_attach->dir != DMA_NONE))
> > return ERR_PTR(-EBUSY);
> > - sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
> > + if (obj->funcs)
> > + sgt = obj->funcs->get_sg_table(obj);
> > + else
> > + sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
> > if (!IS_ERR(sgt)) {
> > if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
> > @@ -406,12 +403,13 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release);
> > void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf)
> > {
> > struct drm_gem_object *obj = dma_buf->priv;
> > - struct drm_device *dev = obj->dev;
> > + void *vaddr;
> > - if (dev->driver->gem_prime_vmap)
> > - return dev->driver->gem_prime_vmap(obj);
> > - else
> > - return NULL;
> > + vaddr = drm_gem_vmap(obj);
> > + if (IS_ERR(vaddr))
> > + vaddr = NULL;
> > +
> > + return vaddr;
> > }
> > EXPORT_SYMBOL(drm_gem_dmabuf_vmap);
> > @@ -426,10 +424,8 @@ EXPORT_SYMBOL(drm_gem_dmabuf_vmap);
> > void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
> > {
> > struct drm_gem_object *obj = dma_buf->priv;
> > - struct drm_device *dev = obj->dev;
> > - if (dev->driver->gem_prime_vunmap)
> > - dev->driver->gem_prime_vunmap(obj, vaddr);
> > + drm_gem_vunmap(obj, vaddr);
> > }
> > EXPORT_SYMBOL(drm_gem_dmabuf_vunmap);
> > @@ -529,7 +525,9 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
> > return dmabuf;
> > }
> > - if (dev->driver->gem_prime_export)
> > + if (obj->funcs && obj->funcs->export)
> > + dmabuf = obj->funcs->export(obj, flags);
> > + else if (dev->driver->gem_prime_export)
> > dmabuf = dev->driver->gem_prime_export(dev, obj, flags);
> > else
> > dmabuf = drm_gem_prime_export(dev, obj, flags);
> > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > index 3583b98a1718..f466ce5bde0e 100644
> > --- a/include/drm/drm_gem.h
> > +++ b/include/drm/drm_gem.h
> > @@ -38,6 +38,121 @@
> > #include <drm/drm_vma_manager.h>
> > +struct drm_gem_object;
> > +
> > +/**
> > + * struct drm_gem_object_funcs - GEM object functions
> > + */
> > +struct drm_gem_object_funcs {
> > + /**
> > + * @free:
> > + *
> > + * Deconstructor for drm_gem_objects.
> > + *
> > + * This callback is mandatory.
> > + */
> > + void (*free)(struct drm_gem_object *obj);
> > +
> > + /**
> > + * @open:
> > + *
> > + * Called upon GEM handle creation.
> > + *
> > + * This callback is optional.
> > + */
> > + int (*open)(struct drm_gem_object *obj, struct drm_file *file);
> > +
> > + /**
> > + * @close:
> > + *
> > + * Called upon GEM handle release.
> > + *
> > + * This callback is optional.
> > + */
> > + void (*close)(struct drm_gem_object *obj, struct drm_file *file);
> > +
> > + /**
> > + * @print_info:
> > + *
> > + * If driver subclasses struct &drm_gem_object, it can implement this
> > + * optional hook for printing additional driver specific info.
> > + *
> > + * drm_printf_indent() should be used in the callback passing it the
> > + * indent argument.
> > + *
> > + * This callback is called from drm_gem_print_info().
> > + *
> > + * This callback is optional.
> > + */
> > + void (*print_info)(struct drm_printer *p, unsigned int indent,
> > + const struct drm_gem_object *obj);
> > +
> > + /**
> > + * @export:
> > + *
> > + * Export backing buffer as a &dma_buf.
> > + * If this is not set drm_gem_prime_export() is used.
> > + *
> > + * This callback is optional.
> > + */
> > + struct dma_buf *(*export)(struct drm_gem_object *obj, int flags);
> > +
> > + /**
> > + * @pin:
> > + *
> > + * Pin backing buffer in memory.
> > + *
> > + * This callback is optional.
> > + */
> > + int (*pin)(struct drm_gem_object *obj);
> > +
> > + /**
> > + * @unpin:
> > + *
> > + * Unpin backing buffer.
> > + *
> > + * This callback is optional.
> > + */
> > + void (*unpin)(struct drm_gem_object *obj);
> > +
> > + /**
> > + * @get_sg_table:
> > + *
> > + * Returns a Scatter-Gather table representation of the buffer.
> > + * Used when exporting a buffer.
> > + *
> > + * This callback is mandatory if buffer export is supported.
> > + */
> > + struct sg_table *(*get_sg_table)(struct drm_gem_object *obj);
> > +
> > + /**
> > + * @vmap:
> > + *
> > + * Returns a virtual address for the buffer.
> > + *
> > + * This callback is optional.
> > + */
> > + void *(*vmap)(struct drm_gem_object *obj);
> > +
> > + /**
> > + * @vunmap:
> > + *
> > + * Releases the the address previously returned by @vmap.
> > + *
> > + * This callback is optional.
> > + */
> > + void (*vunmap)(struct drm_gem_object *obj, void *vaddr);
> > +
> > + /**
> > + * @vm_ops:
> > + *
> > + * Virtual memory operations used with mmap.
> > + *
> > + * This is optional but necessary for mmap support.
> > + */
> > + const struct vm_operations_struct *vm_ops;
> > +};
> > +
> > /**
> > * struct drm_gem_object - GEM buffer object
> > *
> > @@ -146,6 +261,17 @@ struct drm_gem_object {
> > * simply leave it as NULL.
> > */
> > struct dma_buf_attachment *import_attach;
> > +
> > + /**
> > + * @funcs:
> > + *
> > + * Optional GEM object functions. If this is set, it will be used instead of the
> > + * corresponding &drm_driver GEM callbacks.
> > + *
> > + * New drivers should use this.
> > + *
> > + */
> > + const struct drm_gem_object_funcs *funcs;
> > };
> > /**
> > @@ -293,4 +419,9 @@ int drm_gem_dumb_destroy(struct drm_file *file,
> > struct drm_device *dev,
> > uint32_t handle);
> > +int drm_gem_pin(struct drm_gem_object *obj);
> > +void drm_gem_unpin(struct drm_gem_object *obj);
> > +void *drm_gem_vmap(struct drm_gem_object *obj);
> > +void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr);
> > +
> > #endif /* __DRM_GEM_H__ */
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
