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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 07/11, Daniel Vetter wrote:
> 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@xxxxxxxxx>
> > > ---
> > 
> > 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 ...

Hi, thanks for all the feedbacks.
 
> Rodrigo, can you pls respin with the issue fixed that Haneen spotted?

Yes,

I will prepare a new version of this patchset based on all the comments.
Just one question, should I create a new version of the patchset that
includes the patch related with the hrtimer simulation or keep it
separate?
 
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux