Re: [PATCH 1/5] drm: Add reservation_object to drm_gem_object

On Thu, Jan 31, 2019 at 06:50:53PM -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

---

Documentation/gpu/todo.rst | 9 --------- drivers/gpu/drm/drm_gem.c | 39 +++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_prime.c | 1 + include/drm/drm_gem.h | 7 +++++++ 4 files changed, 47 insertions(+), 9 deletions(-)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index 14191b64446d..6e0a37d0bf6d 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -209,15 +209,6 @@ 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

-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.

-Contact: Daniel Vetter

idr_init_base()

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 8b55ece97967..91dd06c1b3a8 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;

• if (!obj->resv) {

• obj->resv = &obj->_resv;
  

• reservation_object_init(obj->resv);
  

You _fini unconditionally, but don't _init unconditionally. I think simplest to just always init (and only assign the pointer if nothing's been assigned to it yet).

• } drm_vma_node_reset(&obj->vma_node);

} EXPORT_SYMBOL(drm_gem_private_object_init); @@ -657,6 +661,40 @@ drm_gem_object_lookup(struct drm_file *filp, u32 handle) } EXPORT_SYMBOL(drm_gem_object_lookup);

+/**

• • drm_gem_object_lookup - Wait on GEM object's reservation's objects
• • shared and/or exclusive fences.
• • @filp: 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);
  

• 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 +859,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..f450a5b6038e 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,10 @@ struct drm_gem_object { */ struct dma_buf_attachment *import_attach;

• /* normally (resv == &_resv) except for imported bo's */
• struct reservation_object *resv;
• struct reservation_object _resv;

Some kerneldoc here would be neat too.

Otherwise looks good to me.. -Daniel

• /**
  • @funcs:

@@ -363,6 +368,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