[PATCH v2 1/2] drm: Add library for shmem backed GEM objects

[Noralf Trønnes]

Den 02.09.2018 22.56, skrev Sam Ravnborg:
> Hi Noralf.
>
> Only nitpicks, I have not the background
> to review the actual implmentation.
> So no tags from me to put on the commit.
>
> 	Sam
>
>> +/**
>> + * drm_gem_shmem_create - Allocate an object with the given size
>> + * @dev: DRM device
>> + * @size: Size of the object to allocate
>> + *
>> + * This function creates a shmem GEM object. The default cache mode is
>> + * DRM_GEM_SHMEM_BO_CACHED. The &drm_driver->gem_create_object callback can be
>> + * used override this.
> used to override this.
>       ^^
>
>> + *
>> + * Returns:
>> + * A struct drm_gem_shmem_object * on success or an ERR_PTR()-encoded negative
>> + * error code on failure.
>> + */
>> +struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t size)
>> +{
>> +	struct drm_gem_shmem_object *shmem;
>> +	struct drm_gem_object *obj;
>> +	int ret;
>> +
>> +	size = PAGE_ALIGN(size);
>> +
>> +	if (dev->driver->gem_create_object)
>> +		obj = dev->driver->gem_create_object(dev, size);
>> +	else
>> +		obj = kzalloc(sizeof(*shmem), GFP_KERNEL);
>> +	if (!obj)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	shmem = to_drm_gem_shmem_obj(obj);
>> +
>> +	if (!dev->driver->gem_create_object)
>> +		shmem->cache_mode = DRM_GEM_SHMEM_BO_CACHED;
>> +
>> +	ret = drm_gem_object_init(dev, obj, size);
>> +	if (ret)
>> +		goto err_free;
> Some users of drm_gem_object_init() calls drm_gem_object_put_unlocked(obj)
> when there is an error. Others call kfree() liek in this case.

I could also have done that if I initialized the mutexes earlier to match
up with the destroy in *_free_object. But it doesn't buy me anything,
so I prefer to stay with how the majority does it.

I really like to use Elixir when tracking down how drivers use the
various DRM functions:
https://elixir.bootlin.com/linux/latest/ident/drm_gem_object_init

>> +
>> +	ret = drm_gem_create_mmap_offset(obj);
>> +	if (ret)
>> +		goto err_release;
>> +
>> +	mutex_init(&shmem->pages_lock);
>> +	mutex_init(&shmem->vmap_lock);
>> +
>> +	return shmem;
>> +
>> +err_release:
>> +	drm_gem_object_release(obj);
>> +err_free:
>> +	kfree(shmem);
>> +
>> +	return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
>> +
>> +
>> +static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem)
>> +{
>> +	struct drm_gem_object *obj = &shmem->base;
>> +	struct page **pages;
>> +
>> +	if (shmem->pages_use_count++ > 0)
>> +		return 0;
>> +
>> +	pages = drm_gem_get_pages(obj);
>> +	if (IS_ERR(pages)) {
>> +		DRM_DEBUG_KMS("Failed to get pages (%ld)\n", PTR_ERR(pages));
>> +		shmem->pages_use_count = 0;
>> +		return PTR_ERR(pages);
>> +	}
>> +
>> +	shmem->pages = pages;
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * drm_gem_shmem_get_pages - Allocate backing pages for a shmem GEM object
>> + * @shmem: shmem GEM object
>> + *
>> + * This function makes sure that backing pages exists for the shmem GEM object
>> + * and increases the use count.
>> + *
>> + * Returns:
>> + * 0 on success or a negative error code on failure.
>> + */
>> +int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
>> +{
>> +	int ret;
>> +
>> +	ret = mutex_lock_interruptible(&shmem->pages_lock);
>> +	if (ret)
>> +		return ret;
>> +	ret = drm_gem_shmem_get_pages_locked(shmem);
>> +	mutex_unlock(&shmem->pages_lock);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(drm_gem_shmem_get_pages);
>> +
> The functions is named *_unlocked, but called with a lock held.
> Inconsistent?

Indeed it is.

Thanks,
Noralf

>> +static void drm_gem_shmem_put_pages_unlocked(struct drm_gem_shmem_object *shmem)
>> +{
>> +	struct drm_gem_object *obj = &shmem->base;
>> +
>> +	if (WARN_ON_ONCE(!shmem->pages_use_count))
>> +		return;
>> +
>> +	if (--shmem->pages_use_count > 0)
>> +		return;
>> +
>> +	drm_gem_put_pages(obj, shmem->pages,
>> +			  shmem->pages_mark_dirty_on_put,
>> +			  shmem->pages_mark_accessed_on_put);
>> +	shmem->pages = NULL;
>> +}
>> +
>> +/*
>> + * drm_gem_shmem_put_pages - Decrease use count on the backing pages for a shmem GEM object
>> + * @shmem: shmem GEM object
>> + *
>> + * This function decreases the use count and puts the backing pages when use drops to zero.
>> + */
>> +void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
>> +{
>> +	mutex_lock(&shmem->pages_lock);
>> +	drm_gem_shmem_put_pages_unlocked(shmem);
>> +	mutex_unlock(&shmem->pages_lock);
>> +}
>> +EXPORT_SYMBOL(drm_gem_shmem_put_pages);
>> +
>> +static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem)
>> +{
>> +	struct drm_gem_object *obj = &shmem->base;
>> +	int ret;
>> +
>> +	if (shmem->vmap_use_count++ > 0)
>> +		return 0;
>> +
>> +	ret = drm_gem_shmem_get_pages(shmem);
>> +	if (ret)
>> +		goto err_zero_use;
>> +
>> +	if (obj->import_attach) {
>> +		shmem->vaddr = dma_buf_vmap(obj->import_attach->dmabuf);
>> +	} else {
>> +		pgprot_t prot;
>> +
>> +		switch (shmem->cache_mode) {
>> +		case DRM_GEM_SHMEM_BO_UNKNOWN:
> No printout to help the coder that did not set this?
>
>> +			ret = -EINVAL;
>> +			goto err_put_pages;
>> +
>> +		case DRM_GEM_SHMEM_BO_WRITECOMBINED:
>> +			prot = pgprot_writecombine(PAGE_KERNEL);
>> +			break;
>> +
>> +		case DRM_GEM_SHMEM_BO_UNCACHED:
>> +			prot = pgprot_noncached(PAGE_KERNEL);
>> +			break;
>> +
>> +		case DRM_GEM_SHMEM_BO_CACHED:
>> +			prot = PAGE_KERNEL;
>> +			break;
>> +		}
>> +
>> +		shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT, VM_MAP, prot);
>> +	}
>> +
>> +	if (!shmem->vaddr) {
>> +		DRM_DEBUG_KMS("Failed to vmap pages\n");
>> +		ret = -ENOMEM;
>> +		goto err_put_pages;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_put_pages:
>> +	drm_gem_shmem_put_pages(shmem);
>> +err_zero_use:
>> +	shmem->vmap_use_count = 0;
>> +
>> +	return ret;
>> +}
>> +
>> +/*
>> + * drm_gem_shmem_vmap - Create a virtual mapping for a shmem GEM object
>> + * @shmem: shmem GEM object
>> + *
>> + * This function makes sure that a virtual address exists for the buffer backing
>> + * the shmem GEM object.
>> + *
>> + * Returns:
>> + * 0 on success or a negative error code on failure.
>> + */
>> +int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem)
>> +{
>> +	int ret;
>> +
>> +	ret = mutex_lock_interruptible(&shmem->vmap_lock);
>> +	if (ret)
>> +		return ret;
>> +	ret = drm_gem_shmem_vmap_locked(shmem);
>> +	mutex_unlock(&shmem->vmap_lock);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(drm_gem_shmem_vmap);
>> +
>> +static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem)
>> +{
>> +	struct drm_gem_object *obj = &shmem->base;
>> +
>> +	if (WARN_ON_ONCE(!shmem->vmap_use_count))
>> +		return;
>> +
>> +	if (--shmem->vmap_use_count > 0)
>> +		return;
>> +
>> +	if (obj->import_attach)
>> +		dma_buf_vunmap(obj->import_attach->dmabuf, shmem->vaddr);
>> +	else
>> +		vunmap(shmem->vaddr);
>> +
>> +	shmem->vaddr = NULL;
>> +	drm_gem_shmem_put_pages(shmem);
>> +}
>> +
>> +/*
>> + * drm_gem_shmem_vunmap - Unmap a virtual mapping fo a shmem GEM object
>> + * @shmem: shmem GEM object
>> + *
>> + * This function removes the virtual address when use count drops to zero.
>> + */
>> +void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem)
>> +{
>> +	mutex_lock(&shmem->vmap_lock);
>> +	drm_gem_shmem_vunmap_locked(shmem);
>> +	mutex_unlock(&shmem->vmap_lock);
>> +}
>> +EXPORT_SYMBOL(drm_gem_shmem_vunmap);
>> +
>> +static struct drm_gem_shmem_object *
>> +drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
>> +				 struct drm_device *dev, size_t size,
>> +				 uint32_t *handle)
>> +{
>> +	struct drm_gem_shmem_object *shmem;
>> +	int ret;
>> +
>> +	shmem = drm_gem_shmem_create(dev, size);
>> +	if (IS_ERR(shmem))
>> +		return shmem;
>> +
>> +	/*
>> +	 * Allocate an id of idr table where the obj is registered
>> +	 * and handle has the id what user can see.
>> +	 */
>> +	ret = drm_gem_handle_create(file_priv, &shmem->base, handle);
>> +	/* drop reference from allocate - handle holds it now. */
>> +	drm_gem_object_put_unlocked(&shmem->base);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +
>> +	return shmem;
>> +}
>> +
>> +/**
>> + * drm_gem_shmem_dumb_create - Create a dumb shmem buffer object
>> + * @file: DRM file structure to create the dumb buffer for
>> + * @dev: DRM device
>> + * @args: IOCTL data
>> + *
>> + * This function computes the pitch of the dumb buffer and rounds it up to an
>> + * integer number of bytes per pixel. Drivers for hardware that doesn't have
>> + * any additional restrictions on the pitch can directly use this function as
>> + * their &drm_driver.dumb_create callback.
>> + *
>> + * For hardware with additional restrictions, drivers can adjust the fields
>> + * set up by userspace before calling into this function.
>> + *
>> + * Returns:
>> + * 0 on success or a negative error code on failure.
>> + */
>> +int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
>> +			      struct drm_mode_create_dumb *args)
>> +{
>> +	u32 min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
>> +	struct drm_gem_shmem_object *shmem;
>> +
>> +	if (!args->pitch || !args->size) {
>> +		args->pitch = min_pitch;
>> +		args->size = args->pitch * args->height;
>> +	} else {
>> +		/* ensure sane minimum values */
>> +		if (args->pitch < min_pitch)
>> +			args->pitch = min_pitch;
>> +		if (args->size < args->pitch * args->height)
>> +			args->size = args->pitch * args->height;
>> +	}
>> +
>> +	shmem = drm_gem_shmem_create_with_handle(file, dev, args->size, &args->handle);
>> +
>> +	return PTR_ERR_OR_ZERO(shmem);
>> +}
>> +EXPORT_SYMBOL_GPL(drm_gem_shmem_dumb_create);
>> +
>> +static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
>> +{
>> +	struct vm_area_struct *vma = vmf->vma;
>> +	struct drm_gem_object *obj = vma->vm_private_data;
>> +	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>> +	/* We don't use vmf->pgoff since that has the fake offset: */
>> +	pgoff_t pgoff = (vmf->address - vma->vm_start) >> PAGE_SHIFT;
>> +	loff_t num_pages = obj->size >> PAGE_SHIFT;
>> +	struct page *page;
>> +
>> +	if (pgoff > num_pages || WARN_ON_ONCE(!shmem->pages))
>> +		return VM_FAULT_SIGBUS;
>> +
>> +	page = shmem->pages[pgoff];
>> +
>> +	return vmf_insert_page(vma, vmf->address, page);
>> +}
>> +
>> +static void drm_gem_shmem_vm_close(struct vm_area_struct *vma)
>> +{
>> +	struct drm_gem_object *obj = vma->vm_private_data;
>> +	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>> +
>> +	drm_gem_shmem_put_pages(shmem);
>> +	drm_gem_vm_close(vma);
>> +}
>> +
>> +const struct vm_operations_struct drm_gem_shmem_vm_ops = {
>> +	.fault = drm_gem_shmem_fault,
>> +	.open = drm_gem_vm_open,
>> +	.close = drm_gem_shmem_vm_close,
>> +};
>> +EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
>> +
>> +static int drm_gem_shmem_mmap_obj(struct drm_gem_shmem_object *shmem,
>> +				  struct vm_area_struct *vma)
>> +{
>> +	int ret;
>> +
>> +	ret = drm_gem_shmem_get_pages(shmem);
>> +	if (ret)
>> +		goto err_close;
>> +
>> +	/* VM_PFNMAP was set by drm_gem_mmap() */
>> +	vma->vm_flags &= ~VM_PFNMAP;
>> +	vma->vm_flags |= VM_MIXEDMAP;
>> +
>> +	switch (shmem->cache_mode) {
>> +	case DRM_GEM_SHMEM_BO_UNKNOWN:
>> +		ret = -EINVAL;
> Print to help the programmer?
>
>> +		goto err_put_pages;
>> +
>> +	case DRM_GEM_SHMEM_BO_WRITECOMBINED:
>> +		vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
>> +		break;
>> +
>> +	case DRM_GEM_SHMEM_BO_UNCACHED:
>> +		vma->vm_page_prot = pgprot_noncached(vm_get_page_prot(vma->vm_flags));
>> +		break;
>> +
>> +	case DRM_GEM_SHMEM_BO_CACHED:
>> +		/*
>> +		 * Shunt off cached objs to shmem file so they have their own
>> +		 * address_space (so unmap_mapping_range does what we want,
>> +		 * in particular in the case of mmap'd dmabufs)
>> +		 */
>> +		fput(vma->vm_file);
>> +		get_file(shmem->base.filp);
>> +		vma->vm_pgoff = 0;
>> +		vma->vm_file  = shmem->base.filp;
>> +		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
>> +		break;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_put_pages:
>> +	drm_gem_shmem_put_pages(shmem);
>> +err_close:
>> +	drm_gem_vm_close(vma);
>> +
>> +	return ret;
>> +}
>> +