Re: [Intel-gfx] [PATCH] i915: add dmabuf/prime buffer sharing support.

[Chris Wilson <chris@chris-wilson.co.uk>]

On Thu, 10 May 2012 15:11:15 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> This adds handle->fd and fd->handle support to i915, this is to allow
> for offloading of rendering in one direction and outputs in the other.
> 
> v2 from Daniel Vetter:
> - fixup conflicts with the prepare/finish gtt prep work.
> - implement ppgtt binding support.
> 
> Note that we have squat i-g-t testcoverage for any of the lifetime and
> access rules dma_buf/prime support brings along. And there are quite a
> few intricate situations here.
> 
> Also note that the integration with the existing code is a bit
> hackish, especially around get_gtt_pages and put_gtt_pages. It imo
> would be easier with the prep code from Chris Wilson's unbound series,
> but that is for 3.6.
> 
> Also note that I didn't bother to put the new prepare/finish gtt hooks
> to good use by moving the dma_buf_map/unmap_attachment calls in there
> (like we've originally planned for).
> 
> Last but not least this patch is only compile-tested, but I've changed
> very little compared to Dave Airlie's version. So there's a decent
> chance v2 on drm-next works as well as v1 on 3.4-rc.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/Makefile          |    3 +-
>  drivers/gpu/drm/i915/i915_drv.c        |    8 +-
>  drivers/gpu/drm/i915/i915_drv.h        |   11 +++
>  drivers/gpu/drm/i915/i915_gem.c        |   12 ++-
>  drivers/gpu/drm/i915/i915_gem_dmabuf.c |  149 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_gem_gtt.c    |   17 +++-
>  6 files changed, 193 insertions(+), 7 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/i915_gem_dmabuf.c
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 8b8bbc7..7b7ecb8 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -37,7 +37,8 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o \
>  	  dvo_ch7017.o \
>  	  dvo_ivch.o \
>  	  dvo_tfp410.o \
> -	  dvo_sil164.o
> +	  dvo_sil164.o \
> +	  i915_gem_dmabuf.o
>  
>  i915-$(CONFIG_COMPAT)   += i915_ioc32.o
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 1ccfc23..ac13c2c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1034,7 +1034,7 @@ static struct drm_driver driver = {
>  	 */
>  	.driver_features =
>  	    DRIVER_USE_AGP | DRIVER_REQUIRE_AGP | /* DRIVER_USE_MTRR |*/
> -	    DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM,
> +	    DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM | DRIVER_PRIME,
>  	.load = i915_driver_load,
>  	.unload = i915_driver_unload,
>  	.open = i915_driver_open,
> @@ -1057,6 +1057,12 @@ static struct drm_driver driver = {
>  	.gem_init_object = i915_gem_init_object,
>  	.gem_free_object = i915_gem_free_object,
>  	.gem_vm_ops = &i915_gem_vm_ops,
> +
> +	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> +	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> +	.gem_prime_export = i915_gem_prime_export,
> +	.gem_prime_import = i915_gem_prime_import,

Maybe a .gem_prime_ops = &i915_gem_prime_ops.

> +
>  	.dumb_create = i915_gem_dumb_create,
>  	.dumb_map_offset = i915_gem_mmap_gtt,
>  	.dumb_destroy = i915_gem_dumb_destroy,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e03a4f8..751f25c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -935,6 +935,8 @@ struct drm_i915_gem_object {
>  	struct scatterlist *sg_list;
>  	int num_sg;
>  
> +	/* prime dma-buf support */
> +	struct sg_table *sg_table;

This looks like a kludge which with a little more work could integrate
neatly into the existing sg code without having to add aditional special
cases.

> +struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachment,
> +				      enum dma_data_direction dir)
> +{
> +	struct drm_i915_gem_object *obj = attachment->dmabuf->priv;
> +	struct drm_device *dev = obj->base.dev;
> +	int npages = obj->base.size / PAGE_SIZE;
> +	struct sg_table *sg = NULL;
> +	int ret;
> +	int nents;
> +
> +	ret = i915_mutex_lock_interruptible(dev);
> +	if (ret)
> +		return NULL;

Not impressed by the lack of error reporting through this interface.

> +struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
> +				struct dma_buf *dma_buf)
> +{
> +	struct dma_buf_attachment *attach;
> +	struct sg_table *sg;
> +	struct drm_i915_gem_object *obj;
> +	int npages;
> +	int size;
> +	int ret;
> +
> +	/* is this one of own objects? */
> +	if (dma_buf->ops == &i915_dmabuf_ops) {
> +		obj = dma_buf->priv;
> +		/* is it from our device? */
> +		if (obj->base.dev == dev) {
> +			drm_gem_object_reference(&obj->base);
> +			return &obj->base;
> +		}
> +	}
> +
> +	/* need to attach */
> +	attach = dma_buf_attach(dma_buf, dev->dev);
> +	if (IS_ERR(attach))
> +		return ERR_PTR(-EINVAL);
Return the original error.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre