[RFC 2/3] drm/gem: Add drm_gem_object_funcs

Daniel Vetter daniel at ffwll.ch
Mon Sep 24 14:54:52 UTC 2018


On Sat, Sep 22, 2018 at 06:01:55PM +0200, Noralf Trønnes wrote:
> 
> 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.

If it's easier for conversion, then keep as-is (but probably with a plan
to change the defaults later on). Up to you.

> > > 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.

Ah, missed that.

> > 
> > >   	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?

Prime gives you the additional sync ioctl stuff. Which isn't wired up at
all for this gem midlayer, so dunno. Tbh I wouldn't add more functions to
gem_funcs that map to dma_buf functions 1:1, that's just indirection for no
gain (the only reason we have these is EXPORT_SYMBOL_GPL not working well
with the nvidia blob, which I still have a grudge with Dave Airlie about).

I think a default implementation that just remaps dma-buf mmap to gem mmap
for everyone is perfectly fine. For anyone doing write-combined mmap (or
dumb buffers in general) it's the right thing to do.

The problem I'm seeing here is that we now have 2 ways to do gem mmap:
Through your gem_funcs->mmap callback, and through the page fault handling
callback (in vm_ops). That's a bit confusion ... Maybe worth it to tackle
the mmap story separately (both gem_funcs->mmap and dma-buf mmap), so
that it's clearer what you're aiming for, and clearer that it's a win?

On the gem_funcs->mmap itself: Can't you use fops->mmap, with adjusted
offsets? That's what e.g. msm does, afaiui. More callbacks doesn't sound
like a great idea to me ...

Cheers, Daniel

> 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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch


More information about the dri-devel mailing list