diff mbox

[v3,1/5] drm/vkms: Add functions to map/unmap GEM backing storage

Message ID f1e25df28917993930ffc5db596771fe6de0fa59.1531955948.git.hamohammed.sa@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Haneen Mohammed July 19, 2018, 12:18 a.m. UTC
This patch add the necessary functions to map/unmap GEM
backing memory to the kernel's virtual address space.

Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
---
Changes in v2:
- add vkms_gem_vunmap
- use vmap_count to guard against multiple prepare_fb calls on the same
  fb
Changes in v3:
- change vkms_gem_vmap to retrun int
- cleanup if vmap failed
- increment vmap_count after successful vmap

 drivers/gpu/drm/vkms/vkms_drv.h |  9 ++++
 drivers/gpu/drm/vkms/vkms_gem.c | 80 ++++++++++++++++++++++++++++++++-
 2 files changed, 88 insertions(+), 1 deletion(-)

Comments

Sean Paul July 23, 2018, 1:51 p.m. UTC | #1
On Thu, Jul 19, 2018 at 03:18:03AM +0300, Haneen Mohammed wrote:
> This patch add the necessary functions to map/unmap GEM
> backing memory to the kernel's virtual address space.
> 
> Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
> ---
> Changes in v2:
> - add vkms_gem_vunmap
> - use vmap_count to guard against multiple prepare_fb calls on the same
>   fb
> Changes in v3:
> - change vkms_gem_vmap to retrun int
> - cleanup if vmap failed
> - increment vmap_count after successful vmap
> 
>  drivers/gpu/drm/vkms/vkms_drv.h |  9 ++++
>  drivers/gpu/drm/vkms/vkms_gem.c | 80 ++++++++++++++++++++++++++++++++-
>  2 files changed, 88 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 07be29f2dc44..47048f707566 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -39,6 +39,8 @@ struct vkms_gem_object {
>  	struct drm_gem_object gem;
>  	struct mutex pages_lock; /* Page lock used in page fault handler */
>  	struct page **pages;
> +	unsigned int vmap_count;
> +	void *vaddr;
>  };
>  
>  #define drm_crtc_to_vkms_output(target) \
> @@ -47,6 +49,9 @@ struct vkms_gem_object {
>  #define drm_device_to_vkms_device(target) \
>  	container_of(target, struct vkms_device, drm)
>  
> +#define drm_gem_to_vkms_gem(target)\
> +	container_of(target, struct vkms_gem_object, gem)
> +
>  /* CRTC */
>  int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>  		   struct drm_plane *primary, struct drm_plane *cursor);
> @@ -75,4 +80,8 @@ int vkms_dumb_map(struct drm_file *file, struct drm_device *dev,
>  
>  void vkms_gem_free_object(struct drm_gem_object *obj);
>  
> +int vkms_gem_vmap(struct drm_gem_object *obj);
> +
> +void vkms_gem_vunmap(struct drm_gem_object *obj);
> +
>  #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_gem.c b/drivers/gpu/drm/vkms/vkms_gem.c
> index c7e38368602b..2e55c906d9b2 100644
> --- a/drivers/gpu/drm/vkms/vkms_gem.c
> +++ b/drivers/gpu/drm/vkms/vkms_gem.c
> @@ -37,7 +37,9 @@ void vkms_gem_free_object(struct drm_gem_object *obj)
>  	struct vkms_gem_object *gem = container_of(obj, struct vkms_gem_object,
>  						   gem);
>  
> -	kvfree(gem->pages);
> +	WARN_ON(gem->pages);
> +	WARN_ON(gem->vaddr);
> +
>  	mutex_destroy(&gem->pages_lock);
>  	drm_gem_object_release(obj);
>  	kfree(gem);
> @@ -177,3 +179,79 @@ int vkms_dumb_map(struct drm_file *file, struct drm_device *dev,
>  
>  	return ret;
>  }
> +
> +static struct page **_get_pages(struct vkms_gem_object *vkms_obj)
> +{
> +	struct drm_gem_object *gem_obj = &vkms_obj->gem;
> +
> +	if (!vkms_obj->pages) {
> +		struct page **pages = drm_gem_get_pages(gem_obj);
> +
> +		if (IS_ERR(pages))
> +			return pages;
> +
> +		if (cmpxchg(&vkms_obj->pages, NULL, pages))
> +			drm_gem_put_pages(gem_obj, pages, false, true);
> +	}
> +
> +	return vkms_obj->pages;
> +}
> +
> +void vkms_gem_vunmap(struct drm_gem_object *obj)
> +{
> +	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(obj);
> +
> +	mutex_lock(&vkms_obj->pages_lock);
> +	if (vkms_obj->vmap_count < 1) {
> +		WARN_ON(vkms_obj->vaddr);
> +		WARN_ON(vkms_obj->pages);
> +		mutex_unlock(&vkms_obj->pages_lock);
> +		return;
> +	}
> +
> +	vkms_obj->vmap_count--;
> +
> +	if (vkms_obj->vmap_count == 0) {
> +		vunmap(vkms_obj->vaddr);
> +		vkms_obj->vaddr = NULL;
> +		drm_gem_put_pages(obj, vkms_obj->pages, false, true);
> +		vkms_obj->pages = NULL;
> +	}
> +
> +	mutex_unlock(&vkms_obj->pages_lock);
> +}
> +
> +int vkms_gem_vmap(struct drm_gem_object *obj)
> +{
> +	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(obj);
> +	int ret = 0;
> +
> +	mutex_lock(&vkms_obj->pages_lock);
> +
> +	if (!vkms_obj->vaddr) {
> +		unsigned int n_pages = obj->size >> PAGE_SHIFT;
> +		struct page **pages = _get_pages(vkms_obj);
> +
> +		if (IS_ERR(pages)) {
> +			ret = PTR_ERR(pages);
> +			goto fail;
> +		}
> +
> +		vkms_obj->vaddr = vmap(pages, n_pages, VM_MAP, PAGE_KERNEL);
> +		if (!vkms_obj->vaddr) {
> +			ret = -ENOMEM;
> +			drm_gem_put_pages(obj, vkms_obj->pages, false, true);
> +			vkms_obj->pages = NULL;
> +			goto fail;
> +		}
> +
> +		vkms_obj->vmap_count++;
> +	}
> +
> +	mutex_unlock(&vkms_obj->pages_lock);
> +	return 0;
> +
> +fail:

Since fail does the same as the success path, there are 2 ways of handling this
that would be (IMO) better:

1- Rename fail to out and remove the redundant 2 lines above
2- Add a new label above fail called err_vmap and do the ret/put_pages/pages=NULL
   cleanup there, dropping into the unlock and return after.

With either of these changes,

Reviewed-by: Sean Paul <seanpaul@chromium.org>


Sean

> +	mutex_unlock(&vkms_obj->pages_lock);
> +	return ret;
> +}
> -- 
> 2.17.1
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 07be29f2dc44..47048f707566 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -39,6 +39,8 @@  struct vkms_gem_object {
 	struct drm_gem_object gem;
 	struct mutex pages_lock; /* Page lock used in page fault handler */
 	struct page **pages;
+	unsigned int vmap_count;
+	void *vaddr;
 };
 
 #define drm_crtc_to_vkms_output(target) \
@@ -47,6 +49,9 @@  struct vkms_gem_object {
 #define drm_device_to_vkms_device(target) \
 	container_of(target, struct vkms_device, drm)
 
+#define drm_gem_to_vkms_gem(target)\
+	container_of(target, struct vkms_gem_object, gem)
+
 /* CRTC */
 int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		   struct drm_plane *primary, struct drm_plane *cursor);
@@ -75,4 +80,8 @@  int vkms_dumb_map(struct drm_file *file, struct drm_device *dev,
 
 void vkms_gem_free_object(struct drm_gem_object *obj);
 
+int vkms_gem_vmap(struct drm_gem_object *obj);
+
+void vkms_gem_vunmap(struct drm_gem_object *obj);
+
 #endif /* _VKMS_DRV_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_gem.c b/drivers/gpu/drm/vkms/vkms_gem.c
index c7e38368602b..2e55c906d9b2 100644
--- a/drivers/gpu/drm/vkms/vkms_gem.c
+++ b/drivers/gpu/drm/vkms/vkms_gem.c
@@ -37,7 +37,9 @@  void vkms_gem_free_object(struct drm_gem_object *obj)
 	struct vkms_gem_object *gem = container_of(obj, struct vkms_gem_object,
 						   gem);
 
-	kvfree(gem->pages);
+	WARN_ON(gem->pages);
+	WARN_ON(gem->vaddr);
+
 	mutex_destroy(&gem->pages_lock);
 	drm_gem_object_release(obj);
 	kfree(gem);
@@ -177,3 +179,79 @@  int vkms_dumb_map(struct drm_file *file, struct drm_device *dev,
 
 	return ret;
 }
+
+static struct page **_get_pages(struct vkms_gem_object *vkms_obj)
+{
+	struct drm_gem_object *gem_obj = &vkms_obj->gem;
+
+	if (!vkms_obj->pages) {
+		struct page **pages = drm_gem_get_pages(gem_obj);
+
+		if (IS_ERR(pages))
+			return pages;
+
+		if (cmpxchg(&vkms_obj->pages, NULL, pages))
+			drm_gem_put_pages(gem_obj, pages, false, true);
+	}
+
+	return vkms_obj->pages;
+}
+
+void vkms_gem_vunmap(struct drm_gem_object *obj)
+{
+	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(obj);
+
+	mutex_lock(&vkms_obj->pages_lock);
+	if (vkms_obj->vmap_count < 1) {
+		WARN_ON(vkms_obj->vaddr);
+		WARN_ON(vkms_obj->pages);
+		mutex_unlock(&vkms_obj->pages_lock);
+		return;
+	}
+
+	vkms_obj->vmap_count--;
+
+	if (vkms_obj->vmap_count == 0) {
+		vunmap(vkms_obj->vaddr);
+		vkms_obj->vaddr = NULL;
+		drm_gem_put_pages(obj, vkms_obj->pages, false, true);
+		vkms_obj->pages = NULL;
+	}
+
+	mutex_unlock(&vkms_obj->pages_lock);
+}
+
+int vkms_gem_vmap(struct drm_gem_object *obj)
+{
+	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(obj);
+	int ret = 0;
+
+	mutex_lock(&vkms_obj->pages_lock);
+
+	if (!vkms_obj->vaddr) {
+		unsigned int n_pages = obj->size >> PAGE_SHIFT;
+		struct page **pages = _get_pages(vkms_obj);
+
+		if (IS_ERR(pages)) {
+			ret = PTR_ERR(pages);
+			goto fail;
+		}
+
+		vkms_obj->vaddr = vmap(pages, n_pages, VM_MAP, PAGE_KERNEL);
+		if (!vkms_obj->vaddr) {
+			ret = -ENOMEM;
+			drm_gem_put_pages(obj, vkms_obj->pages, false, true);
+			vkms_obj->pages = NULL;
+			goto fail;
+		}
+
+		vkms_obj->vmap_count++;
+	}
+
+	mutex_unlock(&vkms_obj->pages_lock);
+	return 0;
+
+fail:
+	mutex_unlock(&vkms_obj->pages_lock);
+	return ret;
+}