[PATCH v2 0/3] drm/panfrost: drm_gem_map_offset() helper

[PATCH v2 0/3] drm/panfrost: drm_gem_map_offset() helper

[Steven Price]

Panfrost has a re-implementation of drm_gem_dumb_map_offset() with an
extra bug regarding the handling of imported buffers. However we don't
really want Panfrost calling _dumb functions because it's not a KMS
driver.

This series renames drm_gem_dumb_map_offset() to drop the '_dumb' and
introduces a shmem helper to wrap it. This means that the shmem
implementation can be kept in sync with the semantics the
drm_gem_shmem_mmap() callback provides.

v1: https://lore.kernel.org/lkml/20190513143244.16478-1-steven.price@arm.com/
Changes since v1:
 * Rename drm_gem_dumb_map_offset to drop _dumb
 * Add a shmem helper

Steven Price (3):
  drm/gem: Rename drm_gem_dumb_map_offset() to drm_gem_map_offset()
  drm: shmem: Add drm_gem_shmem_map_offset() wrapper
  drm/panfrost: Use drm_gem_shmem_map_offset()

 drivers/gpu/drm/drm_dumb_buffers.c      |  4 ++--
 drivers/gpu/drm/drm_gem.c               |  6 +++---
 drivers/gpu/drm/drm_gem_shmem_helper.c  | 20 ++++++++++++++++++++
 drivers/gpu/drm/exynos/exynos_drm_gem.c |  3 +--
 drivers/gpu/drm/panfrost/panfrost_drv.c | 16 ++--------------
 include/drm/drm_gem.h                   |  4 ++--
 include/drm/drm_gem_shmem_helper.h      |  2 ++
 7 files changed, 32 insertions(+), 23 deletions(-)

-- 
2.20.1

[PATCH v2 1/3] drm/gem: Rename drm_gem_dumb_map_offset() to drm_gem_map_offset()

[Steven Price]

drm_gem_dumb_map_offset() is a useful helper for non-dumb clients, so
rename it to remove the _dumb.

Signed-off-by: Steven Price <steven.price@arm.com>
---
 drivers/gpu/drm/drm_dumb_buffers.c      | 4 ++--
 drivers/gpu/drm/drm_gem.c               | 6 +++---
 drivers/gpu/drm/exynos/exynos_drm_gem.c | 3 +--
 include/drm/drm_gem.h                   | 4 ++--
 4 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
index 81dfdd33753a..956665464296 100644
--- a/drivers/gpu/drm/drm_dumb_buffers.c
+++ b/drivers/gpu/drm/drm_dumb_buffers.c
@@ -46,7 +46,7 @@
  * To support dumb objects drivers must implement the &drm_driver.dumb_create
  * operation. &drm_driver.dumb_destroy defaults to drm_gem_dumb_destroy() if
  * not set and &drm_driver.dumb_map_offset defaults to
- * drm_gem_dumb_map_offset(). See the callbacks for further details.
+ * drm_gem_map_offset(). See the callbacks for further details.
  *
  * Note that dumb objects may not be used for gpu acceleration, as has been
  * attempted on some ARM embedded platforms. Such drivers really must have
@@ -125,7 +125,7 @@ int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
 						    args->handle,
 						    &args->offset);
 	else
-		return drm_gem_dumb_map_offset(file_priv, dev, args->handle,
+		return drm_gem_map_offset(file_priv, dev, args->handle,
 					       &args->offset);
 }
 
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 50de138c89e0..99bb7f79a70b 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -294,7 +294,7 @@ drm_gem_handle_delete(struct drm_file *filp, u32 handle)
 EXPORT_SYMBOL(drm_gem_handle_delete);
 
 /**
- * drm_gem_dumb_map_offset - return the fake mmap offset for a gem object
+ * drm_gem_map_offset - return the fake mmap offset for a gem object
  * @file: drm file-private structure containing the gem object
  * @dev: corresponding drm_device
  * @handle: gem object handle
@@ -306,7 +306,7 @@ EXPORT_SYMBOL(drm_gem_handle_delete);
  * Returns:
  * 0 on success or a negative error code on failure.
  */
-int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
+int drm_gem_map_offset(struct drm_file *file, struct drm_device *dev,
 			    u32 handle, u64 *offset)
 {
 	struct drm_gem_object *obj;
@@ -332,7 +332,7 @@ int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(drm_gem_dumb_map_offset);
+EXPORT_SYMBOL_GPL(drm_gem_map_offset);
 
 /**
  * drm_gem_dumb_destroy - dumb fb callback helper for gem based drivers
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
index a55f5ac41bf3..5e3aa9e4a096 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -276,8 +276,7 @@ int exynos_drm_gem_map_ioctl(struct drm_device *dev, void *data,
 {
 	struct drm_exynos_gem_map *args = data;
 
-	return drm_gem_dumb_map_offset(file_priv, dev, args->handle,
-				       &args->offset);
+	return drm_gem_map_offset(file_priv, dev, args->handle, &args->offset);
 }
 
 struct exynos_drm_gem *exynos_drm_gem_get(struct drm_file *filp,
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 5047c7ee25f5..91b07c2325e9 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -395,8 +395,8 @@ int drm_gem_fence_array_add(struct xarray *fence_array,
 int drm_gem_fence_array_add_implicit(struct xarray *fence_array,
 				     struct drm_gem_object *obj,
 				     bool write);
-int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
-			    u32 handle, u64 *offset);
+int drm_gem_map_offset(struct drm_file *file, struct drm_device *dev,
+		       u32 handle, u64 *offset);
 int drm_gem_dumb_destroy(struct drm_file *file,
 			 struct drm_device *dev,
 			 uint32_t handle);
-- 
2.20.1

[PATCH v2 2/3] drm: shmem: Add drm_gem_shmem_map_offset() wrapper

[Steven Price]

Provide a wrapper for drm_gem_map_offset() for clients of shmem. This
wrapper provides the correct semantics for the drm_gem_shmem_mmap()
callback.

Signed-off-by: Steven Price <steven.price@arm.com>
---
 drivers/gpu/drm/drm_gem_shmem_helper.c | 20 ++++++++++++++++++++
 include/drm/drm_gem_shmem_helper.h     |  2 ++
 2 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 1ee208c2c85e..9dbebc4897d1 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -400,6 +400,26 @@ int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
 }
 EXPORT_SYMBOL_GPL(drm_gem_shmem_dumb_create);
 
+/**
+ * drm_gem_map_offset - return the fake mmap offset for a gem object
+ * @file: drm file-private structure containing the gem object
+ * @dev: corresponding drm_device
+ * @handle: gem object handle
+ * @offset: return location for the fake mmap offset
+ *
+ * This provides an offset suitable for user space to return to the
+ * drm_gem_shmem_mmap() callback via an mmap() call.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+int drm_gem_shmem_map_offset(struct drm_file *file, struct drm_device *dev,
+			     u32 handle, u64 *offset)
+{
+	return drm_gem_map_offset(file, dev, handle, offset);
+}
+EXPORT_SYMBOL_GPL(drm_gem_shmem_map_offset);
+
 static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
index 038b6d313447..4239ddaaaa4f 100644
--- a/include/drm/drm_gem_shmem_helper.h
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -128,6 +128,8 @@ drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
 int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
 			      struct drm_mode_create_dumb *args);
 
+int drm_gem_shmem_map_offset(struct drm_file *file, struct drm_device *dev,
+			     u32 handle, u64 *offset);
 int drm_gem_shmem_mmap(struct file *filp, struct vm_area_struct *vma);
 
 extern const struct vm_operations_struct drm_gem_shmem_vm_ops;
-- 
2.20.1

[PATCH v2 3/3] drm/panfrost: Use drm_gem_shmem_map_offset()

[Steven Price]

panfrost_ioctl_mmap_bo() contains a reimplementation of
drm_gem_shmem_map_offset() but with a bug - it allows mapping imported
objects (without going through the exporter). Fix this by switching to
use the new drm_gem_shmem_map_offset() function instead which has
the bonus of simplifying the code.

CC: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Signed-off-by: Steven Price <steven.price@arm.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 94b0819ad50b..a261b59208d0 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -254,26 +254,14 @@ static int panfrost_ioctl_mmap_bo(struct drm_device *dev, void *data,
 		      struct drm_file *file_priv)
 {
 	struct drm_panfrost_mmap_bo *args = data;
-	struct drm_gem_object *gem_obj;
-	int ret;
 
 	if (args->flags != 0) {
 		DRM_INFO("unknown mmap_bo flags: %d\n", args->flags);
 		return -EINVAL;
 	}
 
-	gem_obj = drm_gem_object_lookup(file_priv, args->handle);
-	if (!gem_obj) {
-		DRM_DEBUG("Failed to look up GEM BO %d\n", args->handle);
-		return -ENOENT;
-	}
-
-	ret = drm_gem_create_mmap_offset(gem_obj);
-	if (ret == 0)
-		args->offset = drm_vma_node_offset_addr(&gem_obj->vma_node);
-	drm_gem_object_put_unlocked(gem_obj);
-
-	return ret;
+	return drm_gem_shmem_map_offset(file_priv, dev, args->handle,
+				       &args->offset);
 }
 
 static int panfrost_ioctl_get_bo_offset(struct drm_device *dev, void *data,
-- 
2.20.1

Re: [PATCH v2 0/3] drm/panfrost: drm_gem_map_offset() helper

[Alyssa Rosenzweig]

Providing maintainers more aware of the substance review it and ok it,
patches 1-2 are:

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

Patch 3 should be:

	Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>

Re: [PATCH v2 1/3] drm/gem: Rename drm_gem_dumb_map_offset() to drm_gem_map_offset()

[Daniel Vetter]

On Thu, May 16, 2019 at 03:14:45PM +0100, Steven Price wrote:
> drm_gem_dumb_map_offset() is a useful helper for non-dumb clients, so
> rename it to remove the _dumb.
> 
> Signed-off-by: Steven Price <steven.price@arm.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_dumb_buffers.c      | 4 ++--
>  drivers/gpu/drm/drm_gem.c               | 6 +++---
>  drivers/gpu/drm/exynos/exynos_drm_gem.c | 3 +--
>  include/drm/drm_gem.h                   | 4 ++--
>  4 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
> index 81dfdd33753a..956665464296 100644
> --- a/drivers/gpu/drm/drm_dumb_buffers.c
> +++ b/drivers/gpu/drm/drm_dumb_buffers.c
> @@ -46,7 +46,7 @@
>   * To support dumb objects drivers must implement the &drm_driver.dumb_create
>   * operation. &drm_driver.dumb_destroy defaults to drm_gem_dumb_destroy() if
>   * not set and &drm_driver.dumb_map_offset defaults to
> - * drm_gem_dumb_map_offset(). See the callbacks for further details.
> + * drm_gem_map_offset(). See the callbacks for further details.
>   *
>   * Note that dumb objects may not be used for gpu acceleration, as has been
>   * attempted on some ARM embedded platforms. Such drivers really must have
> @@ -125,7 +125,7 @@ int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
>  						    args->handle,
>  						    &args->offset);
>  	else
> -		return drm_gem_dumb_map_offset(file_priv, dev, args->handle,
> +		return drm_gem_map_offset(file_priv, dev, args->handle,
>  					       &args->offset);
>  }
>  
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 50de138c89e0..99bb7f79a70b 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -294,7 +294,7 @@ drm_gem_handle_delete(struct drm_file *filp, u32 handle)
>  EXPORT_SYMBOL(drm_gem_handle_delete);
>  
>  /**
> - * drm_gem_dumb_map_offset - return the fake mmap offset for a gem object
> + * drm_gem_map_offset - return the fake mmap offset for a gem object
>   * @file: drm file-private structure containing the gem object
>   * @dev: corresponding drm_device
>   * @handle: gem object handle
> @@ -306,7 +306,7 @@ EXPORT_SYMBOL(drm_gem_handle_delete);
>   * Returns:
>   * 0 on success or a negative error code on failure.
>   */
> -int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
> +int drm_gem_map_offset(struct drm_file *file, struct drm_device *dev,
>  			    u32 handle, u64 *offset)
>  {
>  	struct drm_gem_object *obj;
> @@ -332,7 +332,7 @@ int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
>  
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(drm_gem_dumb_map_offset);
> +EXPORT_SYMBOL_GPL(drm_gem_map_offset);
>  
>  /**
>   * drm_gem_dumb_destroy - dumb fb callback helper for gem based drivers
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> index a55f5ac41bf3..5e3aa9e4a096 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -276,8 +276,7 @@ int exynos_drm_gem_map_ioctl(struct drm_device *dev, void *data,
>  {
>  	struct drm_exynos_gem_map *args = data;
>  
> -	return drm_gem_dumb_map_offset(file_priv, dev, args->handle,
> -				       &args->offset);
> +	return drm_gem_map_offset(file_priv, dev, args->handle, &args->offset);
>  }
>  
>  struct exynos_drm_gem *exynos_drm_gem_get(struct drm_file *filp,
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 5047c7ee25f5..91b07c2325e9 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -395,8 +395,8 @@ int drm_gem_fence_array_add(struct xarray *fence_array,
>  int drm_gem_fence_array_add_implicit(struct xarray *fence_array,
>  				     struct drm_gem_object *obj,
>  				     bool write);
> -int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
> -			    u32 handle, u64 *offset);
> +int drm_gem_map_offset(struct drm_file *file, struct drm_device *dev,
> +		       u32 handle, u64 *offset);
>  int drm_gem_dumb_destroy(struct drm_file *file,
>  			 struct drm_device *dev,
>  			 uint32_t handle);
> -- 
> 2.20.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 2/3] drm: shmem: Add drm_gem_shmem_map_offset() wrapper

[Daniel Vetter]

On Thu, May 16, 2019 at 03:14:46PM +0100, Steven Price wrote:
> Provide a wrapper for drm_gem_map_offset() for clients of shmem. This
> wrapper provides the correct semantics for the drm_gem_shmem_mmap()
> callback.
> 
> Signed-off-by: Steven Price <steven.price@arm.com>
> ---
>  drivers/gpu/drm/drm_gem_shmem_helper.c | 20 ++++++++++++++++++++
>  include/drm/drm_gem_shmem_helper.h     |  2 ++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 1ee208c2c85e..9dbebc4897d1 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -400,6 +400,26 @@ int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_shmem_dumb_create);
>  
> +/**
> + * drm_gem_map_offset - return the fake mmap offset for a gem object
> + * @file: drm file-private structure containing the gem object
> + * @dev: corresponding drm_device
> + * @handle: gem object handle
> + * @offset: return location for the fake mmap offset
> + *
> + * This provides an offset suitable for user space to return to the
> + * drm_gem_shmem_mmap() callback via an mmap() call.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +int drm_gem_shmem_map_offset(struct drm_file *file, struct drm_device *dev,
> +			     u32 handle, u64 *offset)
> +{
> +	return drm_gem_map_offset(file, dev, handle, offset);
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_shmem_map_offset);

Not seeing the point of this mapper, since drm_gem_shmem_map_offset isn't
speficic at all. It works for dumb, shmem, cma and private objects all
equally well. I'd drop this and just directly call the underlying thing,
no need to layer helpers.
-Daniel

> +
>  static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
>  {
>  	struct vm_area_struct *vma = vmf->vma;
> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
> index 038b6d313447..4239ddaaaa4f 100644
> --- a/include/drm/drm_gem_shmem_helper.h
> +++ b/include/drm/drm_gem_shmem_helper.h
> @@ -128,6 +128,8 @@ drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
>  int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
>  			      struct drm_mode_create_dumb *args);
>  
> +int drm_gem_shmem_map_offset(struct drm_file *file, struct drm_device *dev,
> +			     u32 handle, u64 *offset);
>  int drm_gem_shmem_mmap(struct file *filp, struct vm_area_struct *vma);
>  
>  extern const struct vm_operations_struct drm_gem_shmem_vm_ops;
> -- 
> 2.20.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 2/3] drm: shmem: Add drm_gem_shmem_map_offset() wrapper

[Steven Price]

On 16/05/2019 21:26, Daniel Vetter wrote:
> On Thu, May 16, 2019 at 03:14:46PM +0100, Steven Price wrote:
>> Provide a wrapper for drm_gem_map_offset() for clients of shmem. This
>> wrapper provides the correct semantics for the drm_gem_shmem_mmap()
>> callback.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>>  drivers/gpu/drm/drm_gem_shmem_helper.c | 20 ++++++++++++++++++++
>>  include/drm/drm_gem_shmem_helper.h     |  2 ++
>>  2 files changed, 22 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> index 1ee208c2c85e..9dbebc4897d1 100644
>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> @@ -400,6 +400,26 @@ int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
>>  }
>>  EXPORT_SYMBOL_GPL(drm_gem_shmem_dumb_create);
>>  
>> +/**
>> + * drm_gem_map_offset - return the fake mmap offset for a gem object
>> + * @file: drm file-private structure containing the gem object
>> + * @dev: corresponding drm_device
>> + * @handle: gem object handle
>> + * @offset: return location for the fake mmap offset
>> + *
>> + * This provides an offset suitable for user space to return to the
>> + * drm_gem_shmem_mmap() callback via an mmap() call.
>> + *
>> + * Returns:
>> + * 0 on success or a negative error code on failure.
>> + */
>> +int drm_gem_shmem_map_offset(struct drm_file *file, struct drm_device *dev,
>> +			     u32 handle, u64 *offset)
>> +{
>> +	return drm_gem_map_offset(file, dev, handle, offset);
>> +}
>> +EXPORT_SYMBOL_GPL(drm_gem_shmem_map_offset);
> 
> Not seeing the point of this mapper, since drm_gem_shmem_map_offset isn't
> speficic at all. It works for dumb, shmem, cma and private objects all
> equally well. I'd drop this and just directly call the underlying thing,
> no need to layer helpers.
> -Daniel

Ok, I'll drop it. I may have misunderstood, but I think Chris Wilson was
asking for it because shmem is the source of the particular requirements
of what can be mmap()d. But I think a helper can be added very easily if
anything changes, so this patch is probably premature.

I'll resend the series with this patch dropped.

Thanks,
Steve