diff mbox series

[v4,3/9] drm/panfrost: Restructure the GEM object creation

Message ID 20190808222200.13176-4-robh@kernel.org
State New
Headers show
Series drm/panfrost: Add heap and no execute buffer allocation | expand

Commit Message

Rob Herring (Arm) Aug. 8, 2019, 10:21 p.m. UTC 
Setting the GPU VA when creating the GEM object doesn't allow for any
conditional adjustments to the mapping. In preparation to support
adjusting the mapping and per FD address spaces, restructure the GEM
object creation to map and unmap the GEM object in the GEM object .open()
and .close() hooks.

While panfrost_gem_free_object() and panfrost_gem_prime_import_sg_table()
are not really needed after this commit, keep them as we'll need them in
subsequent commits.

Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Reviewed-by: Steven Price <steven.price@arm.com>
Acked-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
Signed-off-by: Rob Herring <robh@kernel.org>
---
Steven, Alyssa, I kept your tags, but please take another look as things
moved around a bit here.

 drivers/gpu/drm/panfrost/panfrost_drv.c |  9 ----
 drivers/gpu/drm/panfrost/panfrost_gem.c | 67 ++++++++++++++-----------
 2 files changed, 37 insertions(+), 39 deletions(-)

--
2.20.1

Comments

Steven Price Aug. 9, 2019, 10:11 a.m. UTC | #1 
On 08/08/2019 23:21, Rob Herring wrote:
> Setting the GPU VA when creating the GEM object doesn't allow for any
> conditional adjustments to the mapping. In preparation to support
> adjusting the mapping and per FD address spaces, restructure the GEM
> object creation to map and unmap the GEM object in the GEM object .open()
> and .close() hooks.
> 
> While panfrost_gem_free_object() and panfrost_gem_prime_import_sg_table()
> are not really needed after this commit, keep them as we'll need them in
> subsequent commits.
> 
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Reviewed-by: Steven Price <steven.price@arm.com>
> Acked-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> Steven, Alyssa, I kept your tags, but please take another look as things
> moved around a bit here.

Sadly this doesn't compile (bisection is broken), see below:

>  drivers/gpu/drm/panfrost/panfrost_drv.c |  9 ----
>  drivers/gpu/drm/panfrost/panfrost_gem.c | 67 ++++++++++++++-----------
>  2 files changed, 37 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 926d021ee202..2894cfbbce2b 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -78,7 +78,6 @@ static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct
>  static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
>  		struct drm_file *file)
>  {
> -	int ret;
>  	struct drm_gem_shmem_object *shmem;
>  	struct drm_panfrost_create_bo *args = data;
> 
> @@ -90,17 +89,9 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
>  	if (IS_ERR(shmem))
>  		return PTR_ERR(shmem);
> 
> -	ret = panfrost_mmu_map(to_panfrost_bo(&shmem->base));
> -	if (ret)
> -		goto err_free;
> -
>  	args->offset = to_panfrost_bo(&shmem->base)->node.start << PAGE_SHIFT;
> 
>  	return 0;
> -
> -err_free:
> -	drm_gem_handle_delete(file, args->handle);
> -	return ret;
>  }
> 
>  /**
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 67d374184340..3933f83ba6b0 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -15,6 +15,39 @@
>   * BO.
>   */
>  static void panfrost_gem_free_object(struct drm_gem_object *obj)
> +{
> +	mutex_lock(&pfdev->shrinker_lock);
> +	if (!list_empty(&bo->base.madv_list))
> +		list_del(&bo->base.madv_list);
> +	mutex_unlock(&pfdev->shrinker_lock);
> +
> +	drm_gem_shmem_free_object(obj);
> +}

'pfdev' and 'bo' have't been defined yet.

Steve
Alyssa Rosenzweig Aug. 9, 2019, 9:38 p.m. UTC | #2 
Still A-b :)
On Thu, Aug 08, 2019 at 04:21:54PM -0600, Rob Herring wrote:
> Setting the GPU VA when creating the GEM object doesn't allow for any

> conditional adjustments to the mapping. In preparation to support

> adjusting the mapping and per FD address spaces, restructure the GEM

> object creation to map and unmap the GEM object in the GEM object .open()

> and .close() hooks.

> 

> While panfrost_gem_free_object() and panfrost_gem_prime_import_sg_table()

> are not really needed after this commit, keep them as we'll need them in

> subsequent commits.

> 

> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>

> Cc: Boris Brezillon <boris.brezillon@collabora.com>

> Cc: Robin Murphy <robin.murphy@arm.com>

> Reviewed-by: Steven Price <steven.price@arm.com>

> Acked-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

> Signed-off-by: Rob Herring <robh@kernel.org>

> ---

> Steven, Alyssa, I kept your tags, but please take another look as things

> moved around a bit here.

> 

>  drivers/gpu/drm/panfrost/panfrost_drv.c |  9 ----

>  drivers/gpu/drm/panfrost/panfrost_gem.c | 67 ++++++++++++++-----------

>  2 files changed, 37 insertions(+), 39 deletions(-)

> 

> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c

> index 926d021ee202..2894cfbbce2b 100644

> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c

> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c

> @@ -78,7 +78,6 @@ static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct

>  static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,

>  		struct drm_file *file)

>  {

> -	int ret;

>  	struct drm_gem_shmem_object *shmem;

>  	struct drm_panfrost_create_bo *args = data;

> 

> @@ -90,17 +89,9 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,

>  	if (IS_ERR(shmem))

>  		return PTR_ERR(shmem);

> 

> -	ret = panfrost_mmu_map(to_panfrost_bo(&shmem->base));

> -	if (ret)

> -		goto err_free;

> -

>  	args->offset = to_panfrost_bo(&shmem->base)->node.start << PAGE_SHIFT;

> 

>  	return 0;

> -

> -err_free:

> -	drm_gem_handle_delete(file, args->handle);

> -	return ret;

>  }

> 

>  /**

> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c

> index 67d374184340..3933f83ba6b0 100644

> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c

> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c

> @@ -15,6 +15,39 @@

>   * BO.

>   */

>  static void panfrost_gem_free_object(struct drm_gem_object *obj)

> +{

> +	mutex_lock(&pfdev->shrinker_lock);

> +	if (!list_empty(&bo->base.madv_list))

> +		list_del(&bo->base.madv_list);

> +	mutex_unlock(&pfdev->shrinker_lock);

> +

> +	drm_gem_shmem_free_object(obj);

> +}

> +

> +static int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv)

> +{

> +	int ret;

> +	size_t size = obj->size;

> +	u64 align = size >= SZ_2M ? SZ_2M >> PAGE_SHIFT : 0;

> +	struct panfrost_gem_object *bo = to_panfrost_bo(obj);

> +	struct panfrost_device *pfdev = obj->dev->dev_private;

> +

> +	spin_lock(&pfdev->mm_lock);

> +	ret = drm_mm_insert_node_generic(&pfdev->mm, &bo->node,

> +					 size >> PAGE_SHIFT, align, 0, 0);

> +	if (ret)

> +		goto out;

> +

> +	ret = panfrost_mmu_map(bo);

> +	if (ret)

> +		drm_mm_remove_node(&bo->node);

> +

> +out:

> +	spin_unlock(&pfdev->mm_lock);

> +	return ret;

> +}

> +

> +static void panfrost_gem_close(struct drm_gem_object *obj, struct drm_file *file_priv)

>  {

>  	struct panfrost_gem_object *bo = to_panfrost_bo(obj);

>  	struct panfrost_device *pfdev = obj->dev->dev_private;

> @@ -23,19 +56,15 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)

>  		panfrost_mmu_unmap(bo);

> 

>  	spin_lock(&pfdev->mm_lock);

> -	drm_mm_remove_node(&bo->node);

> +	if (drm_mm_node_allocated(&bo->node))

> +		drm_mm_remove_node(&bo->node);

>  	spin_unlock(&pfdev->mm_lock);

> -

> -	mutex_lock(&pfdev->shrinker_lock);

> -	if (!list_empty(&bo->base.madv_list))

> -		list_del(&bo->base.madv_list);

> -	mutex_unlock(&pfdev->shrinker_lock);

> -

> -	drm_gem_shmem_free_object(obj);

>  }

> 

>  static const struct drm_gem_object_funcs panfrost_gem_funcs = {

>  	.free = panfrost_gem_free_object,

> +	.open = panfrost_gem_open,

> +	.close = panfrost_gem_close,

>  	.print_info = drm_gem_shmem_print_info,

>  	.pin = drm_gem_shmem_pin,

>  	.unpin = drm_gem_shmem_unpin,

> @@ -55,10 +84,7 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = {

>   */

>  struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t size)

>  {

> -	int ret;

> -	struct panfrost_device *pfdev = dev->dev_private;

>  	struct panfrost_gem_object *obj;

> -	u64 align;

> 

>  	obj = kzalloc(sizeof(*obj), GFP_KERNEL);

>  	if (!obj)

> @@ -66,21 +92,7 @@ struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t

> 

>  	obj->base.base.funcs = &panfrost_gem_funcs;

> 

> -	size = roundup(size, PAGE_SIZE);

> -	align = size >= SZ_2M ? SZ_2M >> PAGE_SHIFT : 0;

> -

> -	spin_lock(&pfdev->mm_lock);

> -	ret = drm_mm_insert_node_generic(&pfdev->mm, &obj->node,

> -					 size >> PAGE_SHIFT, align, 0, 0);

> -	spin_unlock(&pfdev->mm_lock);

> -	if (ret)

> -		goto free_obj;

> -

>  	return &obj->base.base;

> -

> -free_obj:

> -	kfree(obj);

> -	return ERR_PTR(ret);

>  }

> 

>  struct drm_gem_object *

> @@ -89,15 +101,10 @@ panfrost_gem_prime_import_sg_table(struct drm_device *dev,

>  				   struct sg_table *sgt)

>  {

>  	struct drm_gem_object *obj;

> -	struct panfrost_gem_object *pobj;

> 

>  	obj = drm_gem_shmem_prime_import_sg_table(dev, attach, sgt);

>  	if (IS_ERR(obj))

>  		return ERR_CAST(obj);

> 

> -	pobj = to_panfrost_bo(obj);

> -

> -	panfrost_mmu_map(pobj);

> -

>  	return obj;

>  }

> --

> 2.20.1
diff mbox series

Patch

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 926d021ee202..2894cfbbce2b 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -78,7 +78,6 @@  static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct
 static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
 		struct drm_file *file)
 {
-	int ret;
 	struct drm_gem_shmem_object *shmem;
 	struct drm_panfrost_create_bo *args = data;

@@ -90,17 +89,9 @@  static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
 	if (IS_ERR(shmem))
 		return PTR_ERR(shmem);

-	ret = panfrost_mmu_map(to_panfrost_bo(&shmem->base));
-	if (ret)
-		goto err_free;
-
 	args->offset = to_panfrost_bo(&shmem->base)->node.start << PAGE_SHIFT;

 	return 0;
-
-err_free:
-	drm_gem_handle_delete(file, args->handle);
-	return ret;
 }

 /**
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index 67d374184340..3933f83ba6b0 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -15,6 +15,39 @@ 
  * BO.
  */
 static void panfrost_gem_free_object(struct drm_gem_object *obj)
+{
+	mutex_lock(&pfdev->shrinker_lock);
+	if (!list_empty(&bo->base.madv_list))
+		list_del(&bo->base.madv_list);
+	mutex_unlock(&pfdev->shrinker_lock);
+
+	drm_gem_shmem_free_object(obj);
+}
+
+static int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv)
+{
+	int ret;
+	size_t size = obj->size;
+	u64 align = size >= SZ_2M ? SZ_2M >> PAGE_SHIFT : 0;
+	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
+	struct panfrost_device *pfdev = obj->dev->dev_private;
+
+	spin_lock(&pfdev->mm_lock);
+	ret = drm_mm_insert_node_generic(&pfdev->mm, &bo->node,
+					 size >> PAGE_SHIFT, align, 0, 0);
+	if (ret)
+		goto out;
+
+	ret = panfrost_mmu_map(bo);
+	if (ret)
+		drm_mm_remove_node(&bo->node);
+
+out:
+	spin_unlock(&pfdev->mm_lock);
+	return ret;
+}
+
+static void panfrost_gem_close(struct drm_gem_object *obj, struct drm_file *file_priv)
 {
 	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
 	struct panfrost_device *pfdev = obj->dev->dev_private;
@@ -23,19 +56,15 @@  static void panfrost_gem_free_object(struct drm_gem_object *obj)
 		panfrost_mmu_unmap(bo);

 	spin_lock(&pfdev->mm_lock);
-	drm_mm_remove_node(&bo->node);
+	if (drm_mm_node_allocated(&bo->node))
+		drm_mm_remove_node(&bo->node);
 	spin_unlock(&pfdev->mm_lock);
-
-	mutex_lock(&pfdev->shrinker_lock);
-	if (!list_empty(&bo->base.madv_list))
-		list_del(&bo->base.madv_list);
-	mutex_unlock(&pfdev->shrinker_lock);
-
-	drm_gem_shmem_free_object(obj);
 }

 static const struct drm_gem_object_funcs panfrost_gem_funcs = {
 	.free = panfrost_gem_free_object,
+	.open = panfrost_gem_open,
+	.close = panfrost_gem_close,
 	.print_info = drm_gem_shmem_print_info,
 	.pin = drm_gem_shmem_pin,
 	.unpin = drm_gem_shmem_unpin,
@@ -55,10 +84,7 @@  static const struct drm_gem_object_funcs panfrost_gem_funcs = {
  */
 struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t size)
 {
-	int ret;
-	struct panfrost_device *pfdev = dev->dev_private;
 	struct panfrost_gem_object *obj;
-	u64 align;

 	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
 	if (!obj)
@@ -66,21 +92,7 @@  struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t

 	obj->base.base.funcs = &panfrost_gem_funcs;

-	size = roundup(size, PAGE_SIZE);
-	align = size >= SZ_2M ? SZ_2M >> PAGE_SHIFT : 0;
-
-	spin_lock(&pfdev->mm_lock);
-	ret = drm_mm_insert_node_generic(&pfdev->mm, &obj->node,
-					 size >> PAGE_SHIFT, align, 0, 0);
-	spin_unlock(&pfdev->mm_lock);
-	if (ret)
-		goto free_obj;
-
 	return &obj->base.base;
-
-free_obj:
-	kfree(obj);
-	return ERR_PTR(ret);
 }

 struct drm_gem_object *
@@ -89,15 +101,10 @@  panfrost_gem_prime_import_sg_table(struct drm_device *dev,
 				   struct sg_table *sgt)
 {
 	struct drm_gem_object *obj;
-	struct panfrost_gem_object *pobj;

 	obj = drm_gem_shmem_prime_import_sg_table(dev, attach, sgt);
 	if (IS_ERR(obj))
 		return ERR_CAST(obj);

-	pobj = to_panfrost_bo(obj);
-
-	panfrost_mmu_map(pobj);
-
 	return obj;
 }