[PATCH 0/2] drm/panfrost: Revert drm_gem_map_offset changes

[PATCH 0/2] drm/panfrost: Revert drm_gem_map_offset changes

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

When I started re-applying these I realized they hadn't hit the list.

Sean

Rob Herring (2):
  Revert "drm/gem: Rename drm_gem_dumb_map_offset() to
    drm_gem_map_offset()"
  Revert "drm/panfrost: Use drm_gem_map_offset()"

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

[PATCH 1/2] Revert "drm/gem: Rename drm_gem_dumb_map_offset() to drm_gem_map_offset()"

[Sean Paul]

From: Rob Herring <robh@kernel.org>

This reverts commit 220df83a5394fbf7c1486ba7848794b7b351d598.

Turns out drm_gem_dumb_map_offset really only worked for the dumb buffer
case, so revert the name change.

Signed-off-by: Rob Herring <robh@kernel.org>
Signed-off-by: Sean Paul <sean@poorly.run>
---
 drivers/gpu/drm/drm_dumb_buffers.c      |  4 ++--
 drivers/gpu/drm/drm_gem.c               | 10 +++-------
 drivers/gpu/drm/exynos/exynos_drm_gem.c |  3 ++-
 include/drm/drm_gem.h                   |  4 ++--
 4 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
index b55cfc9e8772..d18a740fe0f1 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_map_offset(). See the callbacks for further details.
+ * drm_gem_dumb_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_map_offset(file_priv, dev, args->handle,
+		return drm_gem_dumb_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 8cbfd60e09c0..afc38cece3f5 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_map_offset - return the fake mmap offset for a gem object
+ * drm_gem_dumb_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,14 +307,10 @@ 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 GEM BO implementations which
- * have same restriction that imported objects cannot be mapped. The
- * shmem backend is one example.
- *
  * Returns:
  * 0 on success or a negative error code on failure.
  */
-int drm_gem_map_offset(struct drm_file *file, struct drm_device *dev,
+int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
 			    u32 handle, u64 *offset)
 {
 	struct drm_gem_object *obj;
@@ -340,7 +336,7 @@ int drm_gem_map_offset(struct drm_file *file, struct drm_device *dev,
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(drm_gem_map_offset);
+EXPORT_SYMBOL_GPL(drm_gem_dumb_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 bf0ad8e5a02b..d734d9d51762 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -273,7 +273,8 @@ int exynos_drm_gem_map_ioctl(struct drm_device *dev, void *data,
 {
 	struct drm_exynos_gem_map *args = data;
 
-	return drm_gem_map_offset(file_priv, dev, args->handle, &args->offset);
+	return drm_gem_dumb_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 0d6445fa9541..ae693c0666cd 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -401,8 +401,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_map_offset(struct drm_file *file, struct drm_device *dev,
-		       u32 handle, u64 *offset);
+int drm_gem_dumb_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);
-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

[PATCH 2/2] Revert "drm/panfrost: Use drm_gem_map_offset()"

[Sean Paul]

From: Rob Herring <robh@kernel.org>

This reverts commit 583bbf46133c726bae277e8f4e32bfba2a528c7f.

Turns out we need mmap to work on imported BOs even if the current code
is buggy.

Signed-off-by: Rob Herring <robh@kernel.org>
Signed-off-by: Sean Paul <sean@poorly.run>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index b2e325e270b7..b187daa4da85 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -291,14 +291,26 @@ 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;
 	}
 
-	return drm_gem_map_offset(file_priv, dev, args->handle,
-				       &args->offset);
+	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;
 }
 
 static int panfrost_ioctl_get_bo_offset(struct drm_device *dev, void *data,
-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

Re: [PATCH 0/2] drm/panfrost: Revert drm_gem_map_offset changes

[Sean Paul]

On Wed, Aug 07, 2019 at 10:52:46AM -0400, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> When I started re-applying these I realized they hadn't hit the list.
> 
> Sean
> 
> Rob Herring (2):
>   Revert "drm/gem: Rename drm_gem_dumb_map_offset() to
>     drm_gem_map_offset()"
>   Revert "drm/panfrost: Use drm_gem_map_offset()"

Note I have the order messed up, so will swap it when I apply (and properly
compile test in between :/)

Sean

> 
>  drivers/gpu/drm/drm_dumb_buffers.c      |  4 ++--
>  drivers/gpu/drm/drm_gem.c               | 10 +++-------
>  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, 23 insertions(+), 14 deletions(-)
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Emil Velikov]

On Wed, 7 Aug 2019 at 15:53, Sean Paul <sean@poorly.run> wrote:
>
> From: Rob Herring <robh@kernel.org>
>
> This reverts commit 583bbf46133c726bae277e8f4e32bfba2a528c7f.
>
> Turns out we need mmap to work on imported BOs even if the current code
> is buggy.
>
Personally I would have mentioned a use case where imported BOs are used.

> Signed-off-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Sean Paul <sean@poorly.run>

Regardless of the above nitpick, with the patch order fixed the series is:
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

... in case you haven't picked it already.

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

Re: [PATCH 0/2] drm/panfrost: Revert drm_gem_map_offset changes

[Sean Paul]

On Wed, Aug 07, 2019 at 10:52:46AM -0400, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> When I started re-applying these I realized they hadn't hit the list.
> 

Applied to drm-misc-next

Sean

> Sean
> 
> Rob Herring (2):
>   Revert "drm/gem: Rename drm_gem_dumb_map_offset() to
>     drm_gem_map_offset()"
>   Revert "drm/panfrost: Use drm_gem_map_offset()"
> 
>  drivers/gpu/drm/drm_dumb_buffers.c      |  4 ++--
>  drivers/gpu/drm/drm_gem.c               | 10 +++-------
>  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, 23 insertions(+), 14 deletions(-)
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Wed, Aug 07, 2019 at 04:59:51PM +0100, Emil Velikov wrote:
> On Wed, 7 Aug 2019 at 15:53, Sean Paul <sean@poorly.run> wrote:
> >
> > From: Rob Herring <robh@kernel.org>
> >
> > This reverts commit 583bbf46133c726bae277e8f4e32bfba2a528c7f.
> >
> > Turns out we need mmap to work on imported BOs even if the current code
> > is buggy.
> >
> Personally I would have mentioned a use case where imported BOs are used.
> 
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Sean Paul <sean@poorly.run>
> 
> Regardless of the above nitpick, with the patch order fixed the series is:
> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
> 
> ... in case you haven't picked it already.

Yeah a follow-up patch to add a comment here about why exactly this went
all kaboom, plus which userspace (since panfrost is moving fast) would be
real nice here.

Atm we need to hope someone does a git blame on this before the break this
again, which seems a bit hopeful ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Wed, Aug 07, 2019 at 10:52:47AM -0400, Sean Paul wrote:
> From: Rob Herring <robh@kernel.org>
> 
> This reverts commit 220df83a5394fbf7c1486ba7848794b7b351d598.
> 
> Turns out drm_gem_dumb_map_offset really only worked for the dumb buffer
> case, so revert the name change.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Sean Paul <sean@poorly.run>

This part of the series seems unecessary to revert?

iow I still like this, and Im trying to sell Gerd Hoffmann on it for some
of his ttm refactoring ... revert of the revert of the revert of the
revert? Probably better if Gerd cherry-picks into his series (if my
suggestion works out) and maybe references this entire chain for
entertainment purposes :-)
-Daniel

> ---
>  drivers/gpu/drm/drm_dumb_buffers.c      |  4 ++--
>  drivers/gpu/drm/drm_gem.c               | 10 +++-------
>  drivers/gpu/drm/exynos/exynos_drm_gem.c |  3 ++-
>  include/drm/drm_gem.h                   |  4 ++--
>  4 files changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
> index b55cfc9e8772..d18a740fe0f1 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_map_offset(). See the callbacks for further details.
> + * drm_gem_dumb_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_map_offset(file_priv, dev, args->handle,
> +		return drm_gem_dumb_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 8cbfd60e09c0..afc38cece3f5 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_map_offset - return the fake mmap offset for a gem object
> + * drm_gem_dumb_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,14 +307,10 @@ 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 GEM BO implementations which
> - * have same restriction that imported objects cannot be mapped. The
> - * shmem backend is one example.
> - *
>   * Returns:
>   * 0 on success or a negative error code on failure.
>   */
> -int drm_gem_map_offset(struct drm_file *file, struct drm_device *dev,
> +int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
>  			    u32 handle, u64 *offset)
>  {
>  	struct drm_gem_object *obj;
> @@ -340,7 +336,7 @@ int drm_gem_map_offset(struct drm_file *file, struct drm_device *dev,
>  
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(drm_gem_map_offset);
> +EXPORT_SYMBOL_GPL(drm_gem_dumb_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 bf0ad8e5a02b..d734d9d51762 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -273,7 +273,8 @@ int exynos_drm_gem_map_ioctl(struct drm_device *dev, void *data,
>  {
>  	struct drm_exynos_gem_map *args = data;
>  
> -	return drm_gem_map_offset(file_priv, dev, args->handle, &args->offset);
> +	return drm_gem_dumb_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 0d6445fa9541..ae693c0666cd 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -401,8 +401,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_map_offset(struct drm_file *file, struct drm_device *dev,
> -		       u32 handle, u64 *offset);
> +int drm_gem_dumb_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);
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Rob Herring]

On Wed, Aug 7, 2019 at 3:01 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Wed, Aug 07, 2019 at 10:52:47AM -0400, Sean Paul wrote:
> > From: Rob Herring <robh@kernel.org>
> >
> > This reverts commit 220df83a5394fbf7c1486ba7848794b7b351d598.
> >
> > Turns out drm_gem_dumb_map_offset really only worked for the dumb buffer
> > case, so revert the name change.
> >
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Sean Paul <sean@poorly.run>
>
> This part of the series seems unecessary to revert?

Already committed, so let me go revert that right now for you. I can
only hope that's enough to get my commit rights revoked too. :)

> iow I still like this, and Im trying to sell Gerd Hoffmann on it for some
> of his ttm refactoring ... revert of the revert of the revert of the
> revert? Probably better if Gerd cherry-picks into his series (if my
> suggestion works out) and maybe references this entire chain for
> entertainment purposes :-)
> -Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel