Re: [PATCH] drm: base prime/dma-buf support

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Mar 26, 2012 at 04:02:55PM +0100, Dave Airlie wrote:
<snip>
> +int drm_gem_prime_handle_to_fd(struct drm_device *dev,
> +		struct drm_file *file_priv, uint32_t handle, uint32_t flags,
> +		int *prime_fd)
> +{
> +	struct drm_gem_object *obj;
> +
> +	obj = drm_gem_object_lookup(dev, file_priv, handle);
> +	if (!obj)
> +		return -ENOENT;
> +
> +	/* don't allow imported buffers to be re-exported */
> +	if (obj->import_attach) {
> +		drm_gem_object_unreference_unlocked(obj);
> +		return -EINVAL;
> +	}
> +
> +	if (obj->export_dma_buf) {
> +		get_file(obj->export_dma_buf->file);
> +		*prime_fd = dma_buf_fd(obj->export_dma_buf, flags);
> +		drm_gem_object_unreference_unlocked(obj);
> +	} else {
> +		obj->export_dma_buf =
> +				dev->driver->gem_prime_export(dev, obj, flags);
> +		if (IS_ERR_OR_NULL(obj->export_dma_buf)) {
> +			/* normally the created dma-buf takes ownership of the ref,
> +			 * but if that fails then drop the ref
> +			 */
> +			drm_gem_object_unreference_unlocked(obj);
> +			return PTR_ERR(obj->export_dma_buf);

PTR_ERR(NULL) seems like a bad idea. Also, you're accessing 'obj' after
dropping the reference.

> +		}
> +		*prime_fd = dma_buf_fd(obj->export_dma_buf, flags);
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
> +
> +int drm_gem_prime_fd_to_handle(struct drm_device *dev,
> +		struct drm_file *file_priv, int prime_fd, uint32_t *handle)
> +{
> +	struct dma_buf *dma_buf;
> +	struct drm_gem_object *obj;
> +	int ret;
> +
> +	dma_buf = dma_buf_get(prime_fd);
> +	if (IS_ERR(dma_buf))
> +		return PTR_ERR(dma_buf);
> +
> +	ret = drm_prime_lookup_fd_handle_mapping(&file_priv->prime,
> +			dma_buf, handle);
> +	if (!ret) {
> +		dma_buf_put(dma_buf);
> +		return 0;
> +	}
> +
> +	/* never seen this one, need to import */
> +	obj = dev->driver->gem_prime_import(dev, dma_buf);
> +	if (IS_ERR_OR_NULL(obj)) {
> +		ret = PTR_ERR(obj);

PTR_ERR(NULL) again.

> +		goto fail_put;
> +	}
> +
> +	ret = drm_gem_handle_create(file_priv, obj, handle);
> +	drm_gem_object_unreference_unlocked(obj);
> +	if (ret)
> +		goto fail_put;
> +
> +	ret = drm_prime_insert_fd_handle_mapping(&file_priv->prime,
> +			dma_buf, *handle);
> +	if (ret)
> +		goto fail;
> +
> +	return 0;
> +
> +fail:
> +	/* hmm, if driver attached, we are relying on the free-object path
> +	 * to detach.. which seems ok..
> +	 */
> +	drm_gem_object_handle_unreference_unlocked(obj);
> +fail_put:
> +	dma_buf_put(dma_buf);
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_gem_prime_fd_to_handle);
> +
> +int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
> +				 struct drm_file *file_priv)
> +{
> +	struct drm_prime_handle *args = data;
> +	uint32_t flags;
> +
> +	if (!drm_core_check_feature(dev, DRIVER_PRIME))
> +		return -EINVAL;
> +
> +	if (!dev->driver->prime_handle_to_fd)
> +		return -ENOSYS;
> +
> +	/* we only want to pass DRM_CLOEXEC which is == O_CLOEXEC */
> +	flags = args->flags & DRM_CLOEXEC;

Check that the unused flags are 0? If you allow broken userspace to
pass uninitialized 'flags' to the kernel, you may have compatibility
problems if/when you want to add more flags.

> +
> +	return dev->driver->prime_handle_to_fd(dev, file_priv,
> +			args->handle, flags, &args->fd);
> +}
> +
> +int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data,
> +				 struct drm_file *file_priv)
> +{
> +	struct drm_prime_handle *args = data;
> +
> +	if (!drm_core_check_feature(dev, DRIVER_PRIME))
> +		return -EINVAL;
> +
> +	if (!dev->driver->prime_fd_to_handle)
> +		return -ENOSYS;
> +
> +	return dev->driver->prime_fd_to_handle(dev, file_priv,
> +			args->fd, &args->handle);
> +}
> +
> +/*
> + * drm_prime_pages_to_sg
> + *
> + * this helper creates an sg table object from a set of pages
> + * the driver is responsible for mapping the pages into the
> + * importers address space
> + */
> +struct sg_table *drm_prime_pages_to_sg(struct page **pages, int nr_pages)
> +{
> +	struct sg_table *sg = NULL;
> +	struct scatterlist *iter;
> +	int i;
> +	int ret;
> +
> +	sg = kzalloc(sizeof(struct sg_table), GFP_KERNEL);

kmalloc() would be enough. sg_alloc_table() already does a memset().

> +	if (!sg)
> +		goto out;
> +
> +	ret = sg_alloc_table(sg, nr_pages, GFP_KERNEL);
> +	if (ret)
> +		goto out;
> +
> +	for_each_sg(sg->sgl, iter, nr_pages, i)
> +		sg_set_page(iter, pages[i], PAGE_SIZE, 0);
> +
> +	return sg;
> +out:
> +	kfree(sg);
> +	return NULL;
> +}
> +EXPORT_SYMBOL(drm_prime_pages_to_sg);

-- 
Ville Syrjälä
Intel OTC