[PATCH v3 0/2] drm/panfrost: drm_gem_map_offset() helper

[PATCH v3 0/2] 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
updates Panfrost to use it rather than it's own implementation.

v2: https://lore.kernel.org/lkml/20190516141447.46839-1-steven.price@arm.com/
Changes since v2:
 * Drop the shmem helper

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 (2):
  drm/gem: Rename drm_gem_dumb_map_offset() to drm_gem_map_offset()
  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/exynos/exynos_drm_gem.c |  3 +--
 drivers/gpu/drm/panfrost/panfrost_drv.c | 16 ++--------------
 include/drm/drm_gem.h                   |  4 ++--
 5 files changed, 10 insertions(+), 23 deletions(-)

-- 
2.20.1

[PATCH v3 1/2] 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>
Acked-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
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

[PATCH v3 2/2] drm/panfrost: Use drm_gem_shmem_map_offset()

[Steven Price]

panfrost_ioctl_mmap_bo() contains a reimplementation of
drm_gem_map_offset() but with a bug - it allows mapping imported objects
(without going through the exporter). Fix this by switching to use the
newly renamed drm_gem_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>
Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
---
 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 d11e2281dde6..8be0cd5d6c7a 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -255,26 +255,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_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 v3 2/2] drm/panfrost: Use drm_gem_shmem_map_offset()

[Rob Herring]

On Mon, May 20, 2019 at 4:23 AM Steven Price <steven.price@arm.com> wrote:
>

You forgot to update the subject. I can fixup when applying, but I'd
like an ack from Chris on patch 1.

> panfrost_ioctl_mmap_bo() contains a reimplementation of
> drm_gem_map_offset() but with a bug - it allows mapping imported objects
> (without going through the exporter). Fix this by switching to use the
> newly renamed drm_gem_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>
> Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>

Re: [PATCH v3 2/2] drm/panfrost: Use drm_gem_shmem_map_offset()

[Chris Wilson]

Quoting Rob Herring (2019-05-21 16:24:27)
> On Mon, May 20, 2019 at 4:23 AM Steven Price <steven.price@arm.com> wrote:
> >
> 
> You forgot to update the subject. I can fixup when applying, but I'd
> like an ack from Chris on patch 1.

I still think it is incorrect as the limitation is purely an issue with
the shmem backend and not a generic GEM limitation. It matters if you
have a history of passing names and want a consistent api across objects
when the apps themselves no longer can tell the difference, and
certainly do not have access to the dmabuf fd. i915 provides an indirect
mmap interface that uses the dma mapping regardless of source.
-Chris

Re: [PATCH v3 2/2] drm/panfrost: Use drm_gem_shmem_map_offset()

[Steven Price]

On 21/05/2019 19:23, Chris Wilson wrote:
> Quoting Rob Herring (2019-05-21 16:24:27)
>> On Mon, May 20, 2019 at 4:23 AM Steven Price <steven.price@arm.com> wrote:
>>>
>>
>> You forgot to update the subject. I can fixup when applying, but I'd
>> like an ack from Chris on patch 1.

Sorry about that - I'll try to be more careful in the future.

> I still think it is incorrect as the limitation is purely an issue with
> the shmem backend and not a generic GEM limitation. It matters if you

Do you prefer the previous version of this series[1] with the shmem helper?

[1]
https://lore.kernel.org/lkml/20190516141447.46839-1-steven.price@arm.com/

Although this isn't a generic GEM limitation it's currently the same
limitation that applies to the 'dumb' drivers as well as shmem backend,
so I'd prefer not implementing two identical functions purely because
this limitation could be removed in the future.

> have a history of passing names and want a consistent api across objects
> when the apps themselves no longer can tell the difference, and
> certainly do not have access to the dmabuf fd. i915 provides an indirect
> mmap interface that uses the dma mapping regardless of source.

I don't understand the i915 driver, but from a quick look at the source
of i915_gem_mmap_ioctl():

	/* prime objects have no backing filp to GEM mmap
	 * pages from.
	 */
	if (!obj->base.filp) {
		addr = -ENXIO;
		goto err;
	}

it looks to me that an attempt to map an imported dmabuf from user space
using the ioctl will fail. Am I missing something?

exynos_drm_gem_mmap() is the only (mainline[2]) instance I can see where
a transparent mapping of a dma_buf is supported.

[2] mali_kbase does this too - but I'm not convinced it was a good idea.

I could instead add support in shmem/panfrost for transparently passing
the request to the exporter (i.e. call dma_buf_mmap()) - but I'm not
sure we want to implement this if there isn't going to be a user of the
support.

Steve

Re: [PATCH v3 2/2] drm/panfrost: Use drm_gem_shmem_map_offset()

[Rob Herring]

On Wed, May 22, 2019 at 6:39 AM Steven Price <steven.price@arm.com> wrote:
>
> On 21/05/2019 19:23, Chris Wilson wrote:
> > Quoting Rob Herring (2019-05-21 16:24:27)
> >> On Mon, May 20, 2019 at 4:23 AM Steven Price <steven.price@arm.com> wrote:
> >>>
> >>
> >> You forgot to update the subject. I can fixup when applying, but I'd
> >> like an ack from Chris on patch 1.
>
> Sorry about that - I'll try to be more careful in the future.
>
> > I still think it is incorrect as the limitation is purely an issue with
> > the shmem backend and not a generic GEM limitation. It matters if you
>
> Do you prefer the previous version of this series[1] with the shmem helper?
>
> [1]
> https://lore.kernel.org/lkml/20190516141447.46839-1-steven.price@arm.com/
>
> Although this isn't a generic GEM limitation it's currently the same
> limitation that applies to the 'dumb' drivers as well as shmem backend,
> so I'd prefer not implementing two identical functions purely because
> this limitation could be removed in the future.

In interest of moving this forward, how about some comments in
drm_gem_map_offset() explaining the limitations and when it is
appropriate to use the function.

Rob