[v2,1/5] drm: Add reservation_object to drm_gem_object

Submitted by Rob Herring on Feb. 2, 2019, 3:41 p.m.

Details

Message ID 20190202154158.10443-2-robh@kernel.org
State New
Headers show
Series "Add reservation_object to drm_gem_object" ( rev: 2 ) in DRI devel

Not browsing as part of any series.

Commit Message

Rob Herring Feb. 2, 2019, 3:41 p.m.
Many users of drm_gem_object embed a struct reservation_object into
their subclassed struct, so let's add one to struct drm_gem_object.
This will allow removing the reservation object from the subclasses
and removing the ->gem_prime_res_obj callback.

With the addition, add a drm_gem_reservation_object_wait() helper
function for drivers to use in wait ioctls.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: Sean Paul <sean@poorly.run>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Rob Herring <robh@kernel.org>
---
v2:
 - Fix missing kerneldoc
 - Reword todo with what is left todo.
 - Fix timeout error handling (added to drm_gem_reservation_object_wait)

 Documentation/gpu/todo.rst  |  8 +++----
 drivers/gpu/drm/drm_gem.c   | 43 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_prime.c |  1 +
 include/drm/drm_gem.h       | 21 ++++++++++++++++++
 4 files changed, 69 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 14191b64446d..c9ed55605641 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -209,12 +209,12 @@  Would be great to refactor this all into a set of small common helpers.
 
 Contact: Daniel Vetter
 
-Put a reservation_object into drm_gem_object
+Remove the ->gem_prime_res_obj callback
 --------------------------------------------
 
-This would remove the need for the ->gem_prime_res_obj callback. It would also
-allow us to implement generic helpers for waiting for a bo, allowing for quite a
-bit of refactoring in the various wait ioctl implementations.
+The ->gem_prime_res_obj callback can be removed from drivers by using the
+reservation_object in the drm_gem_object. It may also be possible to use the
+generic drm_gem_reservation_object_wait helper for waiting for a bo.
 
 Contact: Daniel Vetter
 
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 8b55ece97967..b09acbc5a655 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -170,6 +170,10 @@  void drm_gem_private_object_init(struct drm_device *dev,
 	kref_init(&obj->refcount);
 	obj->handle_count = 0;
 	obj->size = size;
+	reservation_object_init(&obj->_resv);
+	if (!obj->resv)
+		obj->resv = &obj->_resv;
+
 	drm_vma_node_reset(&obj->vma_node);
 }
 EXPORT_SYMBOL(drm_gem_private_object_init);
@@ -657,6 +661,44 @@  drm_gem_object_lookup(struct drm_file *filp, u32 handle)
 }
 EXPORT_SYMBOL(drm_gem_object_lookup);
 
+/**
+ * drm_gem_reservation_object_wait - Wait on GEM object's reservation's objects
+ * shared and/or exclusive fences.
+ * @filep: DRM file private date
+ * @handle: userspace handle
+ * @wait_all: if true, wait on all fences, else wait on just exclusive fence
+ * @timeout: timeout value in jiffies or zero to return immediately
+ *
+ * Returns:
+ *
+ * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or
+ * greater than 0 on success.
+ */
+long drm_gem_reservation_object_wait(struct drm_file *filep, u32 handle,
+				    bool wait_all, unsigned long timeout)
+{
+	long ret;
+	struct drm_gem_object *obj;
+
+	obj = drm_gem_object_lookup(filep, handle);
+	if (!obj) {
+		DRM_DEBUG("Failed to look up GEM BO %d\n", handle);
+		return -EINVAL;
+	}
+
+	ret = reservation_object_wait_timeout_rcu(obj->resv, wait_all,
+						  true, timeout);
+	if (ret == 0)
+		ret = -ETIME;
+	else if (ret > 0)
+		ret = 0;
+
+	drm_gem_object_put_unlocked(obj);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_gem_reservation_object_wait);
+
 /**
  * drm_gem_close_ioctl - implementation of the GEM_CLOSE ioctl
  * @dev: drm_device
@@ -821,6 +863,7 @@  drm_gem_object_release(struct drm_gem_object *obj)
 	if (obj->filp)
 		fput(obj->filp);
 
+	reservation_object_fini(&obj->_resv);
 	drm_gem_free_mmap_offset(obj);
 }
 EXPORT_SYMBOL(drm_gem_object_release);
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 231e3f6d5f41..dc079efb3b0f 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -504,6 +504,7 @@  struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
 		.size = obj->size,
 		.flags = flags,
 		.priv = obj,
+		.resv = obj->resv,
 	};
 
 	if (dev->driver->gem_prime_res_obj)
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index c95727425284..25f1ff2df464 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -35,6 +35,7 @@ 
  */
 
 #include <linux/kref.h>
+#include <linux/reservation.h>
 
 #include <drm/drm_vma_manager.h>
 
@@ -262,6 +263,24 @@  struct drm_gem_object {
 	 */
 	struct dma_buf_attachment *import_attach;
 
+	/**
+	 * @resv:
+	 *
+	 * Pointer to reservation object associated with the this GEM object.
+	 *
+	 * Normally (@resv == &@_resv) except for imported GEM objects.
+	 */
+	struct reservation_object *resv;
+
+	/**
+	 * @_resv:
+	 *
+	 * A reservation object for this GEM object.
+	 *
+	 * This is unused for imported GEM objects.
+	 */
+	struct reservation_object _resv;
+
 	/**
 	 * @funcs:
 	 *
@@ -363,6 +382,8 @@  void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
 		bool dirty, bool accessed);
 
 struct drm_gem_object *drm_gem_object_lookup(struct drm_file *filp, u32 handle);
+long drm_gem_reservation_object_wait(struct drm_file *filep, u32 handle,
+				    bool wait_all, unsigned long timeout);
 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,

Comments

On Sat, Feb 02, 2019 at 09:41:54AM -0600, Rob Herring wrote:
> Many users of drm_gem_object embed a struct reservation_object into
> their subclassed struct, so let's add one to struct drm_gem_object.
> This will allow removing the reservation object from the subclasses
> and removing the ->gem_prime_res_obj callback.
> 
> With the addition, add a drm_gem_reservation_object_wait() helper
> function for drivers to use in wait ioctls.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v2:
>  - Fix missing kerneldoc
>  - Reword todo with what is left todo.
>  - Fix timeout error handling (added to drm_gem_reservation_object_wait)

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
>  Documentation/gpu/todo.rst  |  8 +++----
>  drivers/gpu/drm/drm_gem.c   | 43 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_prime.c |  1 +
>  include/drm/drm_gem.h       | 21 ++++++++++++++++++
>  4 files changed, 69 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 14191b64446d..c9ed55605641 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -209,12 +209,12 @@ Would be great to refactor this all into a set of small common helpers.
>  
>  Contact: Daniel Vetter
>  
> -Put a reservation_object into drm_gem_object
> +Remove the ->gem_prime_res_obj callback
>  --------------------------------------------
>  
> -This would remove the need for the ->gem_prime_res_obj callback. It would also
> -allow us to implement generic helpers for waiting for a bo, allowing for quite a
> -bit of refactoring in the various wait ioctl implementations.
> +The ->gem_prime_res_obj callback can be removed from drivers by using the
> +reservation_object in the drm_gem_object. It may also be possible to use the
> +generic drm_gem_reservation_object_wait helper for waiting for a bo.
>  
>  Contact: Daniel Vetter
>  
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 8b55ece97967..b09acbc5a655 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -170,6 +170,10 @@ void drm_gem_private_object_init(struct drm_device *dev,
>  	kref_init(&obj->refcount);
>  	obj->handle_count = 0;
>  	obj->size = size;
> +	reservation_object_init(&obj->_resv);
> +	if (!obj->resv)
> +		obj->resv = &obj->_resv;
> +
>  	drm_vma_node_reset(&obj->vma_node);
>  }
>  EXPORT_SYMBOL(drm_gem_private_object_init);
> @@ -657,6 +661,44 @@ drm_gem_object_lookup(struct drm_file *filp, u32 handle)
>  }
>  EXPORT_SYMBOL(drm_gem_object_lookup);
>  
> +/**
> + * drm_gem_reservation_object_wait - Wait on GEM object's reservation's objects
> + * shared and/or exclusive fences.
> + * @filep: DRM file private date
> + * @handle: userspace handle
> + * @wait_all: if true, wait on all fences, else wait on just exclusive fence
> + * @timeout: timeout value in jiffies or zero to return immediately
> + *
> + * Returns:
> + *
> + * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or
> + * greater than 0 on success.
> + */
> +long drm_gem_reservation_object_wait(struct drm_file *filep, u32 handle,
> +				    bool wait_all, unsigned long timeout)
> +{
> +	long ret;
> +	struct drm_gem_object *obj;
> +
> +	obj = drm_gem_object_lookup(filep, handle);
> +	if (!obj) {
> +		DRM_DEBUG("Failed to look up GEM BO %d\n", handle);
> +		return -EINVAL;
> +	}
> +
> +	ret = reservation_object_wait_timeout_rcu(obj->resv, wait_all,
> +						  true, timeout);
> +	if (ret == 0)
> +		ret = -ETIME;
> +	else if (ret > 0)
> +		ret = 0;
> +
> +	drm_gem_object_put_unlocked(obj);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_gem_reservation_object_wait);
> +
>  /**
>   * drm_gem_close_ioctl - implementation of the GEM_CLOSE ioctl
>   * @dev: drm_device
> @@ -821,6 +863,7 @@ drm_gem_object_release(struct drm_gem_object *obj)
>  	if (obj->filp)
>  		fput(obj->filp);
>  
> +	reservation_object_fini(&obj->_resv);
>  	drm_gem_free_mmap_offset(obj);
>  }
>  EXPORT_SYMBOL(drm_gem_object_release);
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 231e3f6d5f41..dc079efb3b0f 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -504,6 +504,7 @@ struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
>  		.size = obj->size,
>  		.flags = flags,
>  		.priv = obj,
> +		.resv = obj->resv,
>  	};
>  
>  	if (dev->driver->gem_prime_res_obj)
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index c95727425284..25f1ff2df464 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -35,6 +35,7 @@
>   */
>  
>  #include <linux/kref.h>
> +#include <linux/reservation.h>
>  
>  #include <drm/drm_vma_manager.h>
>  
> @@ -262,6 +263,24 @@ struct drm_gem_object {
>  	 */
>  	struct dma_buf_attachment *import_attach;
>  
> +	/**
> +	 * @resv:
> +	 *
> +	 * Pointer to reservation object associated with the this GEM object.
> +	 *
> +	 * Normally (@resv == &@_resv) except for imported GEM objects.
> +	 */
> +	struct reservation_object *resv;
> +
> +	/**
> +	 * @_resv:
> +	 *
> +	 * A reservation object for this GEM object.
> +	 *
> +	 * This is unused for imported GEM objects.
> +	 */
> +	struct reservation_object _resv;
> +
>  	/**
>  	 * @funcs:
>  	 *
> @@ -363,6 +382,8 @@ void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
>  		bool dirty, bool accessed);
>  
>  struct drm_gem_object *drm_gem_object_lookup(struct drm_file *filp, u32 handle);
> +long drm_gem_reservation_object_wait(struct drm_file *filep, u32 handle,
> +				    bool wait_all, unsigned long timeout);
>  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,
> -- 
> 2.19.1
>
Rob Herring <robh@kernel.org> writes:

> Many users of drm_gem_object embed a struct reservation_object into
> their subclassed struct, so let's add one to struct drm_gem_object.
> This will allow removing the reservation object from the subclasses
> and removing the ->gem_prime_res_obj callback.
>
> With the addition, add a drm_gem_reservation_object_wait() helper
> function for drivers to use in wait ioctls.

1, 4, 5 are:

Reviewed-by: Eric Anholt <eric@anholt.net>
Am Sa., 2. Feb. 2019 um 16:42 Uhr schrieb Rob Herring <robh@kernel.org>:
>
> Many users of drm_gem_object embed a struct reservation_object into
> their subclassed struct, so let's add one to struct drm_gem_object.
> This will allow removing the reservation object from the subclasses
> and removing the ->gem_prime_res_obj callback.
>
> With the addition, add a drm_gem_reservation_object_wait() helper
> function for drivers to use in wait ioctls.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Rob Herring <robh@kernel.org>

Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>

> ---
> v2:
>  - Fix missing kerneldoc
>  - Reword todo with what is left todo.
>  - Fix timeout error handling (added to drm_gem_reservation_object_wait)
>
>  Documentation/gpu/todo.rst  |  8 +++----
>  drivers/gpu/drm/drm_gem.c   | 43 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_prime.c |  1 +
>  include/drm/drm_gem.h       | 21 ++++++++++++++++++
>  4 files changed, 69 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 14191b64446d..c9ed55605641 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -209,12 +209,12 @@ Would be great to refactor this all into a set of small common helpers.
>
>  Contact: Daniel Vetter
>
> -Put a reservation_object into drm_gem_object
> +Remove the ->gem_prime_res_obj callback
>  --------------------------------------------
>
> -This would remove the need for the ->gem_prime_res_obj callback. It would also
> -allow us to implement generic helpers for waiting for a bo, allowing for quite a
> -bit of refactoring in the various wait ioctl implementations.
> +The ->gem_prime_res_obj callback can be removed from drivers by using the
> +reservation_object in the drm_gem_object. It may also be possible to use the
> +generic drm_gem_reservation_object_wait helper for waiting for a bo.
>
>  Contact: Daniel Vetter
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 8b55ece97967..b09acbc5a655 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -170,6 +170,10 @@ void drm_gem_private_object_init(struct drm_device *dev,
>         kref_init(&obj->refcount);
>         obj->handle_count = 0;
>         obj->size = size;
> +       reservation_object_init(&obj->_resv);
> +       if (!obj->resv)
> +               obj->resv = &obj->_resv;
> +
>         drm_vma_node_reset(&obj->vma_node);
>  }
>  EXPORT_SYMBOL(drm_gem_private_object_init);
> @@ -657,6 +661,44 @@ drm_gem_object_lookup(struct drm_file *filp, u32 handle)
>  }
>  EXPORT_SYMBOL(drm_gem_object_lookup);
>
> +/**
> + * drm_gem_reservation_object_wait - Wait on GEM object's reservation's objects
> + * shared and/or exclusive fences.
> + * @filep: DRM file private date
> + * @handle: userspace handle
> + * @wait_all: if true, wait on all fences, else wait on just exclusive fence
> + * @timeout: timeout value in jiffies or zero to return immediately
> + *
> + * Returns:
> + *
> + * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or
> + * greater than 0 on success.
> + */
> +long drm_gem_reservation_object_wait(struct drm_file *filep, u32 handle,
> +                                   bool wait_all, unsigned long timeout)
> +{
> +       long ret;
> +       struct drm_gem_object *obj;
> +
> +       obj = drm_gem_object_lookup(filep, handle);
> +       if (!obj) {
> +               DRM_DEBUG("Failed to look up GEM BO %d\n", handle);
> +               return -EINVAL;
> +       }
> +
> +       ret = reservation_object_wait_timeout_rcu(obj->resv, wait_all,
> +                                                 true, timeout);
> +       if (ret == 0)
> +               ret = -ETIME;
> +       else if (ret > 0)
> +               ret = 0;
> +
> +       drm_gem_object_put_unlocked(obj);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL(drm_gem_reservation_object_wait);
> +
>  /**
>   * drm_gem_close_ioctl - implementation of the GEM_CLOSE ioctl
>   * @dev: drm_device
> @@ -821,6 +863,7 @@ drm_gem_object_release(struct drm_gem_object *obj)
>         if (obj->filp)
>                 fput(obj->filp);
>
> +       reservation_object_fini(&obj->_resv);
>         drm_gem_free_mmap_offset(obj);
>  }
>  EXPORT_SYMBOL(drm_gem_object_release);
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 231e3f6d5f41..dc079efb3b0f 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -504,6 +504,7 @@ struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
>                 .size = obj->size,
>                 .flags = flags,
>                 .priv = obj,
> +               .resv = obj->resv,
>         };
>
>         if (dev->driver->gem_prime_res_obj)
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index c95727425284..25f1ff2df464 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -35,6 +35,7 @@
>   */
>
>  #include <linux/kref.h>
> +#include <linux/reservation.h>
>
>  #include <drm/drm_vma_manager.h>
>
> @@ -262,6 +263,24 @@ struct drm_gem_object {
>          */
>         struct dma_buf_attachment *import_attach;
>
> +       /**
> +        * @resv:
> +        *
> +        * Pointer to reservation object associated with the this GEM object.
> +        *
> +        * Normally (@resv == &@_resv) except for imported GEM objects.
> +        */
> +       struct reservation_object *resv;
> +
> +       /**
> +        * @_resv:
> +        *
> +        * A reservation object for this GEM object.
> +        *
> +        * This is unused for imported GEM objects.
> +        */
> +       struct reservation_object _resv;
> +
>         /**
>          * @funcs:
>          *
> @@ -363,6 +382,8 @@ void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
>                 bool dirty, bool accessed);
>
>  struct drm_gem_object *drm_gem_object_lookup(struct drm_file *filp, u32 handle);
> +long drm_gem_reservation_object_wait(struct drm_file *filep, u32 handle,
> +                                   bool wait_all, unsigned long timeout);
>  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,
> --
> 2.19.1
>
On Fri, Feb 08, 2019 at 08:23:44AM +0100, Christian Gmeiner wrote:
> Am Sa., 2. Feb. 2019 um 16:42 Uhr schrieb Rob Herring <robh@kernel.org>:
> >
> > Many users of drm_gem_object embed a struct reservation_object into
> > their subclassed struct, so let's add one to struct drm_gem_object.
> > This will allow removing the reservation object from the subclasses
> > and removing the ->gem_prime_res_obj callback.
> >
> > With the addition, add a drm_gem_reservation_object_wait() helper
> > function for drivers to use in wait ioctls.
> >
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Rob Herring <robh@kernel.org>
> 
> Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>

Does this extend to 2/5 to update etnaviv?
-Daniel

> 
> > ---
> > v2:
> >  - Fix missing kerneldoc
> >  - Reword todo with what is left todo.
> >  - Fix timeout error handling (added to drm_gem_reservation_object_wait)
> >
> >  Documentation/gpu/todo.rst  |  8 +++----
> >  drivers/gpu/drm/drm_gem.c   | 43 +++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/drm_prime.c |  1 +
> >  include/drm/drm_gem.h       | 21 ++++++++++++++++++
> >  4 files changed, 69 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> > index 14191b64446d..c9ed55605641 100644
> > --- a/Documentation/gpu/todo.rst
> > +++ b/Documentation/gpu/todo.rst
> > @@ -209,12 +209,12 @@ Would be great to refactor this all into a set of small common helpers.
> >
> >  Contact: Daniel Vetter
> >
> > -Put a reservation_object into drm_gem_object
> > +Remove the ->gem_prime_res_obj callback
> >  --------------------------------------------
> >
> > -This would remove the need for the ->gem_prime_res_obj callback. It would also
> > -allow us to implement generic helpers for waiting for a bo, allowing for quite a
> > -bit of refactoring in the various wait ioctl implementations.
> > +The ->gem_prime_res_obj callback can be removed from drivers by using the
> > +reservation_object in the drm_gem_object. It may also be possible to use the
> > +generic drm_gem_reservation_object_wait helper for waiting for a bo.
> >
> >  Contact: Daniel Vetter
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 8b55ece97967..b09acbc5a655 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -170,6 +170,10 @@ void drm_gem_private_object_init(struct drm_device *dev,
> >         kref_init(&obj->refcount);
> >         obj->handle_count = 0;
> >         obj->size = size;
> > +       reservation_object_init(&obj->_resv);
> > +       if (!obj->resv)
> > +               obj->resv = &obj->_resv;
> > +
> >         drm_vma_node_reset(&obj->vma_node);
> >  }
> >  EXPORT_SYMBOL(drm_gem_private_object_init);
> > @@ -657,6 +661,44 @@ drm_gem_object_lookup(struct drm_file *filp, u32 handle)
> >  }
> >  EXPORT_SYMBOL(drm_gem_object_lookup);
> >
> > +/**
> > + * drm_gem_reservation_object_wait - Wait on GEM object's reservation's objects
> > + * shared and/or exclusive fences.
> > + * @filep: DRM file private date
> > + * @handle: userspace handle
> > + * @wait_all: if true, wait on all fences, else wait on just exclusive fence
> > + * @timeout: timeout value in jiffies or zero to return immediately
> > + *
> > + * Returns:
> > + *
> > + * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or
> > + * greater than 0 on success.
> > + */
> > +long drm_gem_reservation_object_wait(struct drm_file *filep, u32 handle,
> > +                                   bool wait_all, unsigned long timeout)
> > +{
> > +       long ret;
> > +       struct drm_gem_object *obj;
> > +
> > +       obj = drm_gem_object_lookup(filep, handle);
> > +       if (!obj) {
> > +               DRM_DEBUG("Failed to look up GEM BO %d\n", handle);
> > +               return -EINVAL;
> > +       }
> > +
> > +       ret = reservation_object_wait_timeout_rcu(obj->resv, wait_all,
> > +                                                 true, timeout);
> > +       if (ret == 0)
> > +               ret = -ETIME;
> > +       else if (ret > 0)
> > +               ret = 0;
> > +
> > +       drm_gem_object_put_unlocked(obj);
> > +
> > +       return ret;
> > +}
> > +EXPORT_SYMBOL(drm_gem_reservation_object_wait);
> > +
> >  /**
> >   * drm_gem_close_ioctl - implementation of the GEM_CLOSE ioctl
> >   * @dev: drm_device
> > @@ -821,6 +863,7 @@ drm_gem_object_release(struct drm_gem_object *obj)
> >         if (obj->filp)
> >                 fput(obj->filp);
> >
> > +       reservation_object_fini(&obj->_resv);
> >         drm_gem_free_mmap_offset(obj);
> >  }
> >  EXPORT_SYMBOL(drm_gem_object_release);
> > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> > index 231e3f6d5f41..dc079efb3b0f 100644
> > --- a/drivers/gpu/drm/drm_prime.c
> > +++ b/drivers/gpu/drm/drm_prime.c
> > @@ -504,6 +504,7 @@ struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
> >                 .size = obj->size,
> >                 .flags = flags,
> >                 .priv = obj,
> > +               .resv = obj->resv,
> >         };
> >
> >         if (dev->driver->gem_prime_res_obj)
> > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > index c95727425284..25f1ff2df464 100644
> > --- a/include/drm/drm_gem.h
> > +++ b/include/drm/drm_gem.h
> > @@ -35,6 +35,7 @@
> >   */
> >
> >  #include <linux/kref.h>
> > +#include <linux/reservation.h>
> >
> >  #include <drm/drm_vma_manager.h>
> >
> > @@ -262,6 +263,24 @@ struct drm_gem_object {
> >          */
> >         struct dma_buf_attachment *import_attach;
> >
> > +       /**
> > +        * @resv:
> > +        *
> > +        * Pointer to reservation object associated with the this GEM object.
> > +        *
> > +        * Normally (@resv == &@_resv) except for imported GEM objects.
> > +        */
> > +       struct reservation_object *resv;
> > +
> > +       /**
> > +        * @_resv:
> > +        *
> > +        * A reservation object for this GEM object.
> > +        *
> > +        * This is unused for imported GEM objects.
> > +        */
> > +       struct reservation_object _resv;
> > +
> >         /**
> >          * @funcs:
> >          *
> > @@ -363,6 +382,8 @@ void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
> >                 bool dirty, bool accessed);
> >
> >  struct drm_gem_object *drm_gem_object_lookup(struct drm_file *filp, u32 handle);
> > +long drm_gem_reservation_object_wait(struct drm_file *filep, u32 handle,
> > +                                   bool wait_all, unsigned long timeout);
> >  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,
> > --
> > 2.19.1
> >
> 
> 
> -- 
> greets
> --
> Christian Gmeiner, MSc
> 
> https://christian-gmeiner.info
Am Di., 12. Feb. 2019 um 15:16 Uhr schrieb Daniel Vetter <daniel@ffwll.ch>:
>
> On Fri, Feb 08, 2019 at 08:23:44AM +0100, Christian Gmeiner wrote:
> > Am Sa., 2. Feb. 2019 um 16:42 Uhr schrieb Rob Herring <robh@kernel.org>:
> > >
> > > Many users of drm_gem_object embed a struct reservation_object into
> > > their subclassed struct, so let's add one to struct drm_gem_object.
> > > This will allow removing the reservation object from the subclasses
> > > and removing the ->gem_prime_res_obj callback.
> > >
> > > With the addition, add a drm_gem_reservation_object_wait() helper
> > > function for drivers to use in wait ioctls.
> > >
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> > > Cc: Sean Paul <sean@poorly.run>
> > > Cc: David Airlie <airlied@linux.ie>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Signed-off-by: Rob Herring <robh@kernel.org>
> >
> > Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>
>
> Does this extend to 2/5 to update etnaviv?

oopps... yes it does - see my reply to 2/5.

> -Daniel
>
> >
> > > ---
> > > v2:
> > >  - Fix missing kerneldoc
> > >  - Reword todo with what is left todo.
> > >  - Fix timeout error handling (added to drm_gem_reservation_object_wait)
> > >
> > >  Documentation/gpu/todo.rst  |  8 +++----
> > >  drivers/gpu/drm/drm_gem.c   | 43 +++++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/drm_prime.c |  1 +
> > >  include/drm/drm_gem.h       | 21 ++++++++++++++++++
> > >  4 files changed, 69 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> > > index 14191b64446d..c9ed55605641 100644
> > > --- a/Documentation/gpu/todo.rst
> > > +++ b/Documentation/gpu/todo.rst
> > > @@ -209,12 +209,12 @@ Would be great to refactor this all into a set of small common helpers.
> > >
> > >  Contact: Daniel Vetter
> > >
> > > -Put a reservation_object into drm_gem_object
> > > +Remove the ->gem_prime_res_obj callback
> > >  --------------------------------------------
> > >
> > > -This would remove the need for the ->gem_prime_res_obj callback. It would also
> > > -allow us to implement generic helpers for waiting for a bo, allowing for quite a
> > > -bit of refactoring in the various wait ioctl implementations.
> > > +The ->gem_prime_res_obj callback can be removed from drivers by using the
> > > +reservation_object in the drm_gem_object. It may also be possible to use the
> > > +generic drm_gem_reservation_object_wait helper for waiting for a bo.
> > >
> > >  Contact: Daniel Vetter
> > >
> > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > index 8b55ece97967..b09acbc5a655 100644
> > > --- a/drivers/gpu/drm/drm_gem.c
> > > +++ b/drivers/gpu/drm/drm_gem.c
> > > @@ -170,6 +170,10 @@ void drm_gem_private_object_init(struct drm_device *dev,
> > >         kref_init(&obj->refcount);
> > >         obj->handle_count = 0;
> > >         obj->size = size;
> > > +       reservation_object_init(&obj->_resv);
> > > +       if (!obj->resv)
> > > +               obj->resv = &obj->_resv;
> > > +
> > >         drm_vma_node_reset(&obj->vma_node);
> > >  }
> > >  EXPORT_SYMBOL(drm_gem_private_object_init);
> > > @@ -657,6 +661,44 @@ drm_gem_object_lookup(struct drm_file *filp, u32 handle)
> > >  }
> > >  EXPORT_SYMBOL(drm_gem_object_lookup);
> > >
> > > +/**
> > > + * drm_gem_reservation_object_wait - Wait on GEM object's reservation's objects
> > > + * shared and/or exclusive fences.
> > > + * @filep: DRM file private date
> > > + * @handle: userspace handle
> > > + * @wait_all: if true, wait on all fences, else wait on just exclusive fence
> > > + * @timeout: timeout value in jiffies or zero to return immediately
> > > + *
> > > + * Returns:
> > > + *
> > > + * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or
> > > + * greater than 0 on success.
> > > + */
> > > +long drm_gem_reservation_object_wait(struct drm_file *filep, u32 handle,
> > > +                                   bool wait_all, unsigned long timeout)
> > > +{
> > > +       long ret;
> > > +       struct drm_gem_object *obj;
> > > +
> > > +       obj = drm_gem_object_lookup(filep, handle);
> > > +       if (!obj) {
> > > +               DRM_DEBUG("Failed to look up GEM BO %d\n", handle);
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       ret = reservation_object_wait_timeout_rcu(obj->resv, wait_all,
> > > +                                                 true, timeout);
> > > +       if (ret == 0)
> > > +               ret = -ETIME;
> > > +       else if (ret > 0)
> > > +               ret = 0;
> > > +
> > > +       drm_gem_object_put_unlocked(obj);
> > > +
> > > +       return ret;
> > > +}
> > > +EXPORT_SYMBOL(drm_gem_reservation_object_wait);
> > > +
> > >  /**
> > >   * drm_gem_close_ioctl - implementation of the GEM_CLOSE ioctl
> > >   * @dev: drm_device
> > > @@ -821,6 +863,7 @@ drm_gem_object_release(struct drm_gem_object *obj)
> > >         if (obj->filp)
> > >                 fput(obj->filp);
> > >
> > > +       reservation_object_fini(&obj->_resv);
> > >         drm_gem_free_mmap_offset(obj);
> > >  }
> > >  EXPORT_SYMBOL(drm_gem_object_release);
> > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> > > index 231e3f6d5f41..dc079efb3b0f 100644
> > > --- a/drivers/gpu/drm/drm_prime.c
> > > +++ b/drivers/gpu/drm/drm_prime.c
> > > @@ -504,6 +504,7 @@ struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
> > >                 .size = obj->size,
> > >                 .flags = flags,
> > >                 .priv = obj,
> > > +               .resv = obj->resv,
> > >         };
> > >
> > >         if (dev->driver->gem_prime_res_obj)
> > > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > > index c95727425284..25f1ff2df464 100644
> > > --- a/include/drm/drm_gem.h
> > > +++ b/include/drm/drm_gem.h
> > > @@ -35,6 +35,7 @@
> > >   */
> > >
> > >  #include <linux/kref.h>
> > > +#include <linux/reservation.h>
> > >
> > >  #include <drm/drm_vma_manager.h>
> > >
> > > @@ -262,6 +263,24 @@ struct drm_gem_object {
> > >          */
> > >         struct dma_buf_attachment *import_attach;
> > >
> > > +       /**
> > > +        * @resv:
> > > +        *
> > > +        * Pointer to reservation object associated with the this GEM object.
> > > +        *
> > > +        * Normally (@resv == &@_resv) except for imported GEM objects.
> > > +        */
> > > +       struct reservation_object *resv;
> > > +
> > > +       /**
> > > +        * @_resv:
> > > +        *
> > > +        * A reservation object for this GEM object.
> > > +        *
> > > +        * This is unused for imported GEM objects.
> > > +        */
> > > +       struct reservation_object _resv;
> > > +
> > >         /**
> > >          * @funcs:
> > >          *
> > > @@ -363,6 +382,8 @@ void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
> > >                 bool dirty, bool accessed);
> > >
> > >  struct drm_gem_object *drm_gem_object_lookup(struct drm_file *filp, u32 handle);
> > > +long drm_gem_reservation_object_wait(struct drm_file *filep, u32 handle,
> > > +                                   bool wait_all, unsigned long timeout);
> > >  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,
> > > --
> > > 2.19.1
> > >
> >
> >
> > --
> > greets
> > --
> > Christian Gmeiner, MSc
> >
> > https://christian-gmeiner.info
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch