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

[PATCH v4 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.

Previous versions:

v3: https://lore.kernel.org/lkml/20190520092306.27633-1-steven.price@arm.com
Changes since v3:
 * Add a comment to drm_gem_map_offset() explaining that it can be used
   with shmem clients as well as dumb clients.

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_map_offset()

 drivers/gpu/drm/drm_dumb_buffers.c      |  4 ++--
 drivers/gpu/drm/drm_gem.c               |  9 ++++++---
 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, 13 insertions(+), 23 deletions(-)

-- 
2.20.1

[PATCH v4 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 and add a comment that it can be used by
shmem clients.

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

diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
index d18a740fe0f1..b55cfc9e8772 100644
--- a/drivers/gpu/drm/drm_dumb_buffers.c
+++ b/drivers/gpu/drm/drm_dumb_buffers.c
@@ -48,7 +48,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
@@ -127,7 +127,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 a8c4468f03d9..62842b7701bb 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -298,7 +298,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
@@ -307,10 +307,13 @@ EXPORT_SYMBOL(drm_gem_handle_delete);
  * This implements the &drm_driver.dumb_map_offset kms driver callback for
  * drivers which use gem to manage their backing storage.
  *
+ * It can also be used by drivers using the shmem backend as they have the
+ * same restriction that imported objects cannot be mapped.
+ *
  * 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;
@@ -336,7 +339,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 d8f1fe9b68d8..3b8cffc7a8e0 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -272,8 +272,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 a9121fe66ea2..65b884930583 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 v4 2/2] drm/panfrost: Use drm_gem_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.

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 e34e86a7378a..1498b9b07a8f 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -259,26 +259,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 v4 1/2] drm/gem: Rename drm_gem_dumb_map_offset() to drm_gem_map_offset()

[Rob Herring]

On Thu, Jun 27, 2019 at 9:53 AM Steven Price <steven.price@arm.com> wrote:
>
> drm_gem_dumb_map_offset() is a useful helper for non-dumb clients, so
> rename it to remove the _dumb and add a comment that it can be used by
> shmem clients.
>
> Signed-off-by: Steven Price <steven.price@arm.com>
> ---
>  drivers/gpu/drm/drm_dumb_buffers.c      | 4 ++--
>  drivers/gpu/drm/drm_gem.c               | 9 ++++++---
>  drivers/gpu/drm/exynos/exynos_drm_gem.c | 3 +--
>  include/drm/drm_gem.h                   | 4 ++--
>  4 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
> index d18a740fe0f1..b55cfc9e8772 100644
> --- a/drivers/gpu/drm/drm_dumb_buffers.c
> +++ b/drivers/gpu/drm/drm_dumb_buffers.c
> @@ -48,7 +48,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
> @@ -127,7 +127,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 a8c4468f03d9..62842b7701bb 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -298,7 +298,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
> @@ -307,10 +307,13 @@ EXPORT_SYMBOL(drm_gem_handle_delete);
>   * This implements the &drm_driver.dumb_map_offset kms driver callback for
>   * drivers which use gem to manage their backing storage.
>   *
> + * It can also be used by drivers using the shmem backend as they have the
> + * same restriction that imported objects cannot be mapped.

Maybe better not to say 'shmem' explicitly or just mention it as an
example so when we have a 2nd case we don't have to update the
comment.

...drivers with GEM BO implementations which have the same...

I can fix up and apply. Some other acks would be nice first.

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 1/2] drm/gem: Rename drm_gem_dumb_map_offset() to drm_gem_map_offset()

[Daniel Vetter]

On Thu, Jun 27, 2019 at 11:57:34AM -0600, Rob Herring wrote:
> On Thu, Jun 27, 2019 at 9:53 AM Steven Price <steven.price@arm.com> wrote:
> >
> > drm_gem_dumb_map_offset() is a useful helper for non-dumb clients, so
> > rename it to remove the _dumb and add a comment that it can be used by
> > shmem clients.
> >
> > Signed-off-by: Steven Price <steven.price@arm.com>
> > ---
> >  drivers/gpu/drm/drm_dumb_buffers.c      | 4 ++--
> >  drivers/gpu/drm/drm_gem.c               | 9 ++++++---
> >  drivers/gpu/drm/exynos/exynos_drm_gem.c | 3 +--
> >  include/drm/drm_gem.h                   | 4 ++--
> >  4 files changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
> > index d18a740fe0f1..b55cfc9e8772 100644
> > --- a/drivers/gpu/drm/drm_dumb_buffers.c
> > +++ b/drivers/gpu/drm/drm_dumb_buffers.c
> > @@ -48,7 +48,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
> > @@ -127,7 +127,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 a8c4468f03d9..62842b7701bb 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -298,7 +298,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
> > @@ -307,10 +307,13 @@ EXPORT_SYMBOL(drm_gem_handle_delete);
> >   * This implements the &drm_driver.dumb_map_offset kms driver callback for
> >   * drivers which use gem to manage their backing storage.
> >   *
> > + * It can also be used by drivers using the shmem backend as they have the
> > + * same restriction that imported objects cannot be mapped.
> 
> Maybe better not to say 'shmem' explicitly or just mention it as an
> example so when we have a 2nd case we don't have to update the
> comment.
> 
> ...drivers with GEM BO implementations which have the same...
> 
> I can fix up and apply. Some other acks would be nice first.

Yeah kerneldoc is a bit suboptimal, with that polished, a-b: me. Yours
should be good enough though.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v4 2/2] drm/panfrost: Use drm_gem_map_offset()

[Rob Herring]

On Thu, Jun 27, 2019 at 9:53 AM Steven Price <steven.price@arm.com> wrote:
>
> 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.

While it may have been a bug, it worked (by some definition of
worked). Now mesa breaks on importing buffers which always get
mmapped. So we need to revert this, get import mmaps to work, or drop
mmapping of imports and backport that to 19.1. I don't think there
should be any need to mmap imports.

Rob