[RFC 2/3] drm/gem: Add drm_gem_object_funcs
Noralf Trønnes
noralf at tronnes.org
Sat Sep 22 16:01:55 UTC 2018
Den 22.09.2018 12.41, skrev Daniel Vetter:
> On Fri, Sep 21, 2018 at 06:42:29PM +0200, Noralf Trønnes wrote:
>> 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.
>>
>> The individual drm_driver callbacks take priority over the GEM attached
>> callbacks.
> I'd do this the other way round, make the new gem callbacks the clearly
> favoured option.
The reason I did it like this is that I thought it would be easier to
convert the CMA helper this way. I've taken a closer look at this, and
the solution is to convert the drivers that override the defaults first,
and then convert the helper as a whole when that's done.
I'll flip it around.
>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
>> ---
>> drivers/gpu/drm/drm_client.c | 12 ++--
>> drivers/gpu/drm/drm_fb_helper.c | 8 ++-
>> drivers/gpu/drm/drm_gem.c | 108 +++++++++++++++++++++++++++++--
>> drivers/gpu/drm/drm_prime.c | 40 ++++++------
>> include/drm/drm_gem.h | 138 ++++++++++++++++++++++++++++++++++++++++
>> 5 files changed, 272 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;
> I guess the ->gem_prime_vmap check got lost, needs to be readded somewhere
> I think.
It happens in drm_client_buffer_create() when drm_gem_vmap() is called.
It returns -EOPNOTSUPP if no callback is set.
>
>>
>> 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_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 8e95d0f7c71d..66969f0facbb 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -38,6 +38,7 @@
>> #include <drm/drmP.h>
>> #include <drm/drm_crtc.h>
>> #include <drm/drm_fb_helper.h>
>> +#include <drm/drm_gem.h>
>> #include <drm/drm_crtc_helper.h>
>> #include <drm/drm_atomic.h>
>> #include <drm/drm_atomic_helper.h>
>> @@ -3010,9 +3011,12 @@ static void drm_fbdev_fb_destroy(struct fb_info *info)
>> static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
>> {
>> struct drm_fb_helper *fb_helper = info->par;
>> + struct drm_gem_object *obj = fb_helper->buffer->gem;
>>
>> - if (fb_helper->dev->driver->gem_prime_mmap)
>> - return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma);
>> + if (obj->dev->driver->gem_prime_mmap)
>> + return obj->dev->driver->gem_prime_mmap(obj, vma);
>> + else if (obj->funcs && obj->funcs->mmap)
>> + return drm_gem_mmap_obj(obj, obj->size, vma);
>> else
>> return -ENODEV;
>> }
>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>> index 512078ebd97b..7da2549667ce 100644
>> --- a/drivers/gpu/drm/drm_gem.c
>> +++ b/drivers/gpu/drm/drm_gem.c
>> @@ -259,6 +259,8 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
>>
>> if (dev->driver->gem_close_object)
>> dev->driver->gem_close_object(obj, file_priv);
>> + else if (obj->funcs && obj->funcs->close)
>> + obj->funcs->close(obj, file_priv);
>>
>> if (drm_core_check_feature(dev, DRIVER_PRIME))
>> drm_gem_remove_prime_handles(obj, file_priv);
>> @@ -414,6 +416,10 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
>> ret = dev->driver->gem_open_object(obj, file_priv);
>> if (ret)
>> goto err_revoke;
>> + } else if (obj->funcs && obj->funcs->open) {
>> + ret = obj->funcs->open(obj, file_priv);
>> + if (ret)
>> + goto err_revoke;
>> }
>>
>> *handlep = handle;
>> @@ -841,6 +847,8 @@ drm_gem_object_free(struct kref *kref)
>> WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>>
>> dev->driver->gem_free_object(obj);
>> + } else if (obj->funcs) {
>> + obj->funcs->free(obj);
>> }
>> }
>> EXPORT_SYMBOL(drm_gem_object_free);
>> @@ -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);
>> @@ -955,20 +963,30 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>> struct vm_area_struct *vma)
>> {
>> struct drm_device *dev = obj->dev;
>> + int ret;
>>
>> /* Check for valid size. */
>> if (obj_size < vma->vm_end - vma->vm_start)
>> return -EINVAL;
>>
>> - if (!dev->driver->gem_vm_ops)
>> + if (dev->driver->gem_vm_ops)
>> + vma->vm_ops = dev->driver->gem_vm_ops;
>> + else if (obj->funcs && obj->funcs->vm_ops)
>> + vma->vm_ops = obj->funcs->vm_ops;
>> + else
>> return -EINVAL;
> A bit a bikeshed, but if we go with these new ops, I'd put them first in
> all the if ladders. Makes it a bit clearer while reading that they're the
> recommended option.
>>
>> 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);
>>
>> + if (obj->funcs && obj->funcs->mmap) {
>> + ret = obj->funcs->mmap(obj, vma);
>> + if (ret)
>> + return ret;
>> + }
> I'm confused about this one here ... this is for prime mmap only iirc.
The smaller drivers that I've looked at combine the DRM mmap and PRIME
mmap paths going into a shared *_mmap_obj() function.
This is what I'm doing:
DRM fd: fops->mmap = drm_gem_mmap
-> drm_gem_mmap_obj
-> gem->funcs->mmap
dmabuf fd: fops->mmap = dma_buf_mmap_internal
-> dmabuf->ops->mmap = drm_gem_dmabuf_mmap
-> drm_gem_mmap_obj
-> gem->funcs->mmap
Is there a difference to the mapping operation itself whether the mmap
happens through DRM dev or through dmabuf?
Noralf.
>> +
>> /* Take a ref for this mapping of the object, so that the fault
>> * handler can dereference the mmap offset's pointer to the object.
>> * This reference is cleaned up by the corresponding vm_close
>> @@ -1068,4 +1086,84 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
>>
>> if (obj->dev->driver->gem_print_info)
>> obj->dev->driver->gem_print_info(p, indent, obj);
>> + else if (obj->funcs && obj->funcs->print_info)
>> + obj->funcs->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->dev->driver->gem_prime_pin)
>> + return obj->dev->driver->gem_prime_pin(obj);
>> + else if (obj->funcs && obj->funcs->pin)
>> + return obj->funcs->pin(obj);
>> + else
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(drm_gem_pin);
>> +
>> +/**
>> + * drm_gem_pin - 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->dev->driver->gem_prime_unpin)
>> + obj->dev->driver->gem_prime_unpin(obj);
>> + else if (obj->funcs && obj->funcs->unpin)
>> + obj->funcs->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->dev->driver->gem_prime_vmap)
>> + vaddr = obj->dev->driver->gem_prime_vmap(obj);
>> + else if (obj->funcs && obj->funcs->vmap)
>> + vaddr = obj->funcs->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->dev->driver->gem_prime_vunmap)
>> + obj->dev->driver->gem_prime_vunmap(obj, vaddr);
>> + else if (obj->funcs && obj->funcs->vunmap)
>> + obj->funcs->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 6721571749ff..7f5c9500136b 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);
> I think if we just check for obj->funcs here and then unconditionally
> call, we avoid a bunch of pointer chasing for the new recommended path :-)
>
>> + if (obj->dev->driver->gem_prime_get_sg_table)
>> + sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
>> + else
>> + sgt = obj->funcs->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);
>>
>> @@ -476,10 +472,12 @@ int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma)
>> struct drm_gem_object *obj = dma_buf->priv;
>> struct drm_device *dev = obj->dev;
>>
>> - if (!dev->driver->gem_prime_mmap)
>> + if (dev->driver->gem_prime_mmap)
>> + return dev->driver->gem_prime_mmap(obj, vma);
>> + else if (obj->funcs && obj->funcs->mmap)
>> + return drm_gem_mmap_obj(obj, obj->size, vma);
>> + else
>> return -ENOSYS;
>> -
>> - return dev->driver->gem_prime_mmap(obj, vma);
>> }
>> EXPORT_SYMBOL(drm_gem_dmabuf_mmap);
>>
>> @@ -561,6 +559,8 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
>>
>> if (dev->driver->gem_prime_export)
>> dmabuf = dev->driver->gem_prime_export(dev, obj, flags);
>> + else if (obj->funcs && obj->funcs->export)
>> + dmabuf = obj->funcs->export(obj, flags);
>> else
>> dmabuf = drm_gem_prime_export(dev, obj, flags);
>> if (IS_ERR(dmabuf)) {
>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>> index 3583b98a1718..3ae3665bf4e7 100644
>> --- a/include/drm/drm_gem.h
>> +++ b/include/drm/drm_gem.h
>> @@ -38,6 +38,130 @@
>>
>> #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_object:
> @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);
>> +
>> + /*
>> + * @mmap:
>> + *
>> + * Setup userspace mapping with the given vma.
>> + *
>> + * This callback is optional.
>> + */
>> + int (*mmap)(struct drm_gem_object *obj, struct vm_area_struct *vma);
>> +
>> + /**
>> + * @vm_ops:
>> + *
>> + * Virtual memory operations used with mmap.
>> + *
>> + * This is optional but necessary for mmap support.
>> + */
>> + const struct vm_operations_struct *vm_ops;
>> +};
> I think fleshing out the docs a bit is a good opportunity here (and adding
> links to the old place that the new ones recommended). But probably best
> in follow-up patches.
>
>> +
>> /**
>> * struct drm_gem_object - GEM buffer object
>> *
>> @@ -146,6 +270,15 @@ 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 if the
>> + * corresponding &drm_driver GEM callback is not set.
> Maybe also here a note that this is recommended over the callbacks in
> &drm_driver.
>
> Aside from all the tiny style suggestions, I really like this.
> -Daniel
>
>> + *
>> + */
>> + const struct drm_gem_object_funcs *funcs;
>> };
>>
>> /**
>> @@ -293,4 +426,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__ */
>> --
>> 2.15.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the dri-devel
mailing list