Re: [PATCH V2 1/5] drm/vkms: Add dumb operations

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jul 05, 2018 at 11:21:19PM +0300, Haneen Mohammed wrote:
> On Thu, Jun 21, 2018 at 09:16:13AM -0300, Rodrigo Siqueira wrote:
> > VKMS currently does not handle dumb data, and as a consequence, it does
> > not provide mechanisms for handling gem. This commit adds the necessary
> > support for gem object/handler and the dumb functions.
> > 
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > ---
> 
> This looks good to me except with missing gem_free_object_unlocked callback,
> which causes warning: Memory manager not clean during takedown. 
> 
> Maybe it will be easier if we add this in another patch instead of creating v3 of
> this patchset?

Ah this is the patch series that didn't land yet ...

Rodrigo, can you pls respin with the issue fixed that Haneen spotted?

Haneen, can you pls review the other patches in this series too?

Also, do we have any other patch series that's not yet applied? With two
interns working on the same thing it's a bit harder to keep track of stuff
...

Thanks, Daniel

> 
> >  drivers/gpu/drm/vkms/Makefile   |   2 +-
> >  drivers/gpu/drm/vkms/vkms_drv.c |   9 ++
> >  drivers/gpu/drm/vkms/vkms_drv.h |  21 ++++
> >  drivers/gpu/drm/vkms/vkms_gem.c | 168 ++++++++++++++++++++++++++++++++
> >  4 files changed, 199 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/vkms/vkms_gem.c
> > 
> > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> > index 3f774a6a9c58..986297da51bf 100644
> > --- a/drivers/gpu/drm/vkms/Makefile
> > +++ b/drivers/gpu/drm/vkms/Makefile
> > @@ -1,3 +1,3 @@
> > -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o
> > +vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o
> >  
> >  obj-$(CONFIG_DRM_VKMS) += vkms.o
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > index 740a4cbfed91..638bab9083b5 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > @@ -37,6 +37,12 @@ static const struct file_operations vkms_driver_fops = {
> >  	.release	= drm_release,
> >  };
> >  
> > +static const struct vm_operations_struct vkms_gem_vm_ops = {
> > +	.fault = vkms_gem_fault,
> > +	.open = drm_gem_vm_open,
> > +	.close = drm_gem_vm_close,
> > +};
> > +
> >  static void vkms_release(struct drm_device *dev)
> >  {
> >  	struct vkms_device *vkms = container_of(dev, struct vkms_device, drm);
> > @@ -50,6 +56,9 @@ static struct drm_driver vkms_driver = {
> >  	.driver_features	= DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
> >  	.release		= vkms_release,
> >  	.fops			= &vkms_driver_fops,
> > +	.dumb_create		= vkms_dumb_create,
> > +	.dumb_map_offset	= vkms_dumb_map,
> > +	.gem_vm_ops		= &vkms_gem_vm_ops,
> >  
> >  	.name			= DRIVER_NAME,
> >  	.desc			= DRIVER_DESC,
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > index b0f9d2e61a42..54bb3dd2b2c1 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -3,6 +3,7 @@
> >  
> >  #include <drm/drmP.h>
> >  #include <drm/drm.h>
> > +#include <drm/drm_gem.h>
> >  #include <drm/drm_encoder.h>
> >  
> >  static const u32 vkms_formats[] = {
> > @@ -21,6 +22,12 @@ struct vkms_device {
> >  	struct vkms_output output;
> >  };
> >  
> > +struct vkms_gem_object {
> > +	struct drm_gem_object gem;
> > +	struct mutex pages_lock; /* Page lock used in page fault handler */
> > +	struct page **pages;
> > +};
> > +
> >  int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> >  		   struct drm_plane *primary, struct drm_plane *cursor);
> >  
> > @@ -28,4 +35,18 @@ int vkms_output_init(struct vkms_device *vkmsdev);
> >  
> >  struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev);
> >  
> > +/* Gem stuff */
> > +struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
> > +				       struct drm_file *file,
> > +				       u32 *handle,
> > +				       u64 size);
> > +
> > +int vkms_gem_fault(struct vm_fault *vmf);
> > +
> > +int vkms_dumb_create(struct drm_file *file, struct drm_device *dev,
> > +		     struct drm_mode_create_dumb *args);
> > +
> > +int vkms_dumb_map(struct drm_file *file, struct drm_device *dev,
> > +		  u32 handle, u64 *offset);
> > +
> >  #endif /* _VKMS_DRV_H_ */
> > diff --git a/drivers/gpu/drm/vkms/vkms_gem.c b/drivers/gpu/drm/vkms/vkms_gem.c
> > new file mode 100644
> > index 000000000000..9f820f56b9e0
> > --- /dev/null
> > +++ b/drivers/gpu/drm/vkms/vkms_gem.c
> > @@ -0,0 +1,168 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#include <linux/shmem_fs.h>
> > +
> > +#include "vkms_drv.h"
> > +
> > +static struct vkms_gem_object *__vkms_gem_create(struct drm_device *dev,
> > +						 u64 size)
> > +{
> > +	struct vkms_gem_object *obj;
> > +	int ret;
> > +
> > +	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
> > +	if (!obj)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	size = roundup(size, PAGE_SIZE);
> > +	ret = drm_gem_object_init(dev, &obj->gem, size);
> > +	if (ret) {
> > +		kfree(obj);
> > +		return ERR_PTR(ret);
> > +	}
> > +
> > +	mutex_init(&obj->pages_lock);
> > +
> > +	return obj;
> > +}
> > +
> > +int vkms_gem_fault(struct vm_fault *vmf)
> > +{
> > +	struct vm_area_struct *vma = vmf->vma;
> > +	struct vkms_gem_object *obj = vma->vm_private_data;
> > +	unsigned long vaddr = vmf->address;
> > +	pgoff_t page_offset;
> > +	loff_t num_pages;
> > +	int ret;
> > +
> > +	page_offset = (vaddr - vma->vm_start) >> PAGE_SHIFT;
> > +	num_pages = DIV_ROUND_UP(obj->gem.size, PAGE_SIZE);
> > +
> > +	if (page_offset > num_pages)
> > +		return VM_FAULT_SIGBUS;
> > +
> > +	ret = -ENOENT;
> > +	mutex_lock(&obj->pages_lock);
> > +	if (obj->pages) {
> > +		get_page(obj->pages[page_offset]);
> > +		vmf->page = obj->pages[page_offset];
> > +		ret = 0;
> > +	}
> > +	mutex_unlock(&obj->pages_lock);
> > +	if (ret) {
> > +		struct page *page;
> > +		struct address_space *mapping;
> > +
> > +		mapping = file_inode(obj->gem.filp)->i_mapping;
> > +		page = shmem_read_mapping_page(mapping, page_offset);
> > +
> > +		if (!IS_ERR(page)) {
> > +			vmf->page = page;
> > +			ret = 0;
> > +		} else {
> > +			switch (PTR_ERR(page)) {
> > +			case -ENOSPC:
> > +			case -ENOMEM:
> > +				ret = VM_FAULT_OOM;
> > +				break;
> > +			case -EBUSY:
> > +				ret = VM_FAULT_RETRY;
> > +				break;
> > +			case -EFAULT:
> > +			case -EINVAL:
> > +				ret = VM_FAULT_SIGBUS;
> > +				break;
> > +			default:
> > +				WARN_ON(PTR_ERR(page));
> > +				ret = VM_FAULT_SIGBUS;
> > +				break;
> > +			}
> > +		}
> > +	}
> > +	return ret;
> > +}
> > +
> > +struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
> > +				       struct drm_file *file,
> > +				       u32 *handle,
> > +				       u64 size)
> > +{
> > +	struct vkms_gem_object *obj;
> > +	int ret;
> > +
> > +	if (!file || !dev || !handle)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	obj = __vkms_gem_create(dev, size);
> > +	if (IS_ERR(obj))
> > +		return ERR_CAST(obj);
> > +
> > +	ret = drm_gem_handle_create(file, &obj->gem, handle);
> > +	drm_gem_object_put_unlocked(&obj->gem);
> > +	if (ret) {
> > +		drm_gem_object_release(&obj->gem);
> > +		kfree(obj);
> > +		return ERR_PTR(ret);
> > +	}
> > +
> > +	return &obj->gem;
> > +}
> > +
> > +int vkms_dumb_create(struct drm_file *file, struct drm_device *dev,
> > +		     struct drm_mode_create_dumb *args)
> > +{
> > +	struct drm_gem_object *gem_obj;
> > +	u64 pitch, size;
> > +
> > +	if (!args || !dev || !file)
> > +		return -EINVAL;
> > +
> > +	pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
> > +	size = pitch * args->height;
> > +
> > +	if (!size)
> > +		return -EINVAL;
> > +
> > +	gem_obj = vkms_gem_create(dev, file, &args->handle, size);
> > +	if (IS_ERR(gem_obj))
> > +		return PTR_ERR(gem_obj);
> > +
> > +	args->size = gem_obj->size;
> > +	args->pitch = pitch;
> > +
> > +	DRM_DEBUG_DRIVER("Created object of size %lld\n", size);
> > +
> > +	return 0;
> > +}
> > +
> > +int vkms_dumb_map(struct drm_file *file, struct drm_device *dev,
> > +		  u32 handle, u64 *offset)
> > +{
> > +	struct drm_gem_object *obj;
> > +	int ret;
> > +
> > +	obj = drm_gem_object_lookup(file, handle);
> > +	if (!obj)
> > +		return -ENOENT;
> > +
> > +	if (!obj->filp) {
> > +		ret = -EINVAL;
> > +		goto unref;
> > +	}
> > +
> > +	ret = drm_gem_create_mmap_offset(obj);
> > +	if (ret)
> > +		goto unref;
> > +
> > +	*offset = drm_vma_node_offset_addr(&obj->vma_node);
> > +unref:
> > +	drm_gem_object_put_unlocked(obj);
> > +
> > +	return ret;
> > +}
> > -- 
> > 2.17.1
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel