[v2,2/2] drm/i915/gem: Only revoke mmap handlers if active
diff mbox series

Message ID 20200702130605.14747-2-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [v2,1/2] drm/i915/gem: Only revoke the GGTT mmappings on aperture detiling changes
Related show

Commit Message

Chris Wilson July 2, 2020, 1:06 p.m. UTC
Avoid waking up the device and taking stale locks if we know that the
object is not currently mmapped. This is particularly useful as not many
object are actually mmapped and so we can destroy them without waking
the device up, and gives us a little more freedom of workqueue ordering
during shutdown.

v2: Pull the release_mmap() into its single user in freeing the objects,
where there can not be any race with a concurrent user of the freed
object. Or so one hopes!

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
---
 drivers/gpu/drm/i915/gem/i915_gem_mman.c   | 13 -------------
 drivers/gpu/drm/i915/gem/i915_gem_mman.h   |  2 --
 drivers/gpu/drm/i915/gem/i915_gem_object.c | 11 +++++++++++
 3 files changed, 11 insertions(+), 15 deletions(-)

Comments

Chris Wilson July 2, 2020, 1:10 p.m. UTC | #1
Quoting Chris Wilson (2020-07-02 14:06:05)
> Avoid waking up the device and taking stale locks if we know that the
> object is not currently mmapped. This is particularly useful as not many
> object are actually mmapped and so we can destroy them without waking
> the device up, and gives us a little more freedom of workqueue ordering
> during shutdown.
> 
> v2: Pull the release_mmap() into its single user in freeing the objects,
> where there can not be any race with a concurrent user of the freed
> object. Or so one hopes!
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_mman.c   | 13 -------------
>  drivers/gpu/drm/i915/gem/i915_gem_mman.h   |  2 --
>  drivers/gpu/drm/i915/gem/i915_gem_object.c | 11 +++++++++++
>  3 files changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index 7c2650cfb070..b23368529a40 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -507,19 +507,6 @@ void i915_gem_object_release_mmap_offset(struct drm_i915_gem_object *obj)
>         spin_unlock(&obj->mmo.lock);
>  }
>  
> -/**
> - * i915_gem_object_release_mmap - remove physical page mappings
> - * @obj: obj in question
> - *
> - * Preserve the reservation of the mmapping with the DRM core code, but
> - * relinquish ownership of the pages back to the system.
> - */
> -void i915_gem_object_release_mmap(struct drm_i915_gem_object *obj)
> -{
> -       i915_gem_object_release_mmap_gtt(obj);
> -       i915_gem_object_release_mmap_offset(obj);
> -}
> -
>  static struct i915_mmap_offset *
>  lookup_mmo(struct drm_i915_gem_object *obj,
>            enum i915_mmap_type mmap_type)
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.h b/drivers/gpu/drm/i915/gem/i915_gem_mman.h
> index 7c5ccdf59359..efee9e0d2508 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.h
> @@ -24,8 +24,6 @@ int i915_gem_dumb_mmap_offset(struct drm_file *file_priv,
>                               struct drm_device *dev,
>                               u32 handle, u64 *offset);
>  
> -void i915_gem_object_release_mmap(struct drm_i915_gem_object *obj);
> -
>  void __i915_gem_object_release_mmap_gtt(struct drm_i915_gem_object *obj);
>  void i915_gem_object_release_mmap_gtt(struct drm_i915_gem_object *obj);
>  
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index 6b69191c5543..d377c0ef8603 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -171,6 +171,17 @@ static void __i915_gem_free_object_rcu(struct rcu_head *head)
>         atomic_dec(&i915->mm.free_count);
>  }
>  
> +static void i915_gem_object_release_mmap(struct drm_i915_gem_object *obj)

Maybe free_mmaps to try and discourage re-use?

> +{
> +       /* Skip serialisation and waking the device if known to be not used. */
> +
> +       if (obj->userfault_count)
> +               i915_gem_object_release_mmap_gtt(obj);
> +
> +       if (!RB_EMPTY_ROOT(&obj->mmo.offsets))
> +               i915_gem_object_release_mmap_offset(obj);
> +}
Tvrtko Ursulin July 2, 2020, 3:50 p.m. UTC | #2
On 02/07/2020 14:06, Chris Wilson wrote:
> Avoid waking up the device and taking stale locks if we know that the
> object is not currently mmapped. This is particularly useful as not many
> object are actually mmapped and so we can destroy them without waking
> the device up, and gives us a little more freedom of workqueue ordering
> during shutdown.
> 
> v2: Pull the release_mmap() into its single user in freeing the objects,
> where there can not be any race with a concurrent user of the freed
> object. Or so one hopes!
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_mman.c   | 13 -------------
>   drivers/gpu/drm/i915/gem/i915_gem_mman.h   |  2 --
>   drivers/gpu/drm/i915/gem/i915_gem_object.c | 11 +++++++++++
>   3 files changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index 7c2650cfb070..b23368529a40 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -507,19 +507,6 @@ void i915_gem_object_release_mmap_offset(struct drm_i915_gem_object *obj)
>   	spin_unlock(&obj->mmo.lock);
>   }
>   
> -/**
> - * i915_gem_object_release_mmap - remove physical page mappings
> - * @obj: obj in question
> - *
> - * Preserve the reservation of the mmapping with the DRM core code, but
> - * relinquish ownership of the pages back to the system.
> - */
> -void i915_gem_object_release_mmap(struct drm_i915_gem_object *obj)
> -{
> -	i915_gem_object_release_mmap_gtt(obj);
> -	i915_gem_object_release_mmap_offset(obj);
> -}
> -
>   static struct i915_mmap_offset *
>   lookup_mmo(struct drm_i915_gem_object *obj,
>   	   enum i915_mmap_type mmap_type)
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.h b/drivers/gpu/drm/i915/gem/i915_gem_mman.h
> index 7c5ccdf59359..efee9e0d2508 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.h
> @@ -24,8 +24,6 @@ int i915_gem_dumb_mmap_offset(struct drm_file *file_priv,
>   			      struct drm_device *dev,
>   			      u32 handle, u64 *offset);
>   
> -void i915_gem_object_release_mmap(struct drm_i915_gem_object *obj);
> -
>   void __i915_gem_object_release_mmap_gtt(struct drm_i915_gem_object *obj);
>   void i915_gem_object_release_mmap_gtt(struct drm_i915_gem_object *obj);
>   
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index 6b69191c5543..d377c0ef8603 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -171,6 +171,17 @@ static void __i915_gem_free_object_rcu(struct rcu_head *head)
>   	atomic_dec(&i915->mm.free_count);
>   }
>   
> +static void i915_gem_object_release_mmap(struct drm_i915_gem_object *obj)

Yes you can give it a more internal name for a good measure.

> +{
> +	/* Skip serialisation and waking the device if known to be not used. */
> +
> +	if (obj->userfault_count)
> +		i915_gem_object_release_mmap_gtt(obj);
> +
> +	if (!RB_EMPTY_ROOT(&obj->mmo.offsets))
> +		i915_gem_object_release_mmap_offset(obj);
> +}
> +
>   static void __i915_gem_free_objects(struct drm_i915_private *i915,
>   				    struct llist_node *freed)
>   {
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 7c2650cfb070..b23368529a40 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -507,19 +507,6 @@  void i915_gem_object_release_mmap_offset(struct drm_i915_gem_object *obj)
 	spin_unlock(&obj->mmo.lock);
 }
 
-/**
- * i915_gem_object_release_mmap - remove physical page mappings
- * @obj: obj in question
- *
- * Preserve the reservation of the mmapping with the DRM core code, but
- * relinquish ownership of the pages back to the system.
- */
-void i915_gem_object_release_mmap(struct drm_i915_gem_object *obj)
-{
-	i915_gem_object_release_mmap_gtt(obj);
-	i915_gem_object_release_mmap_offset(obj);
-}
-
 static struct i915_mmap_offset *
 lookup_mmo(struct drm_i915_gem_object *obj,
 	   enum i915_mmap_type mmap_type)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.h b/drivers/gpu/drm/i915/gem/i915_gem_mman.h
index 7c5ccdf59359..efee9e0d2508 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.h
@@ -24,8 +24,6 @@  int i915_gem_dumb_mmap_offset(struct drm_file *file_priv,
 			      struct drm_device *dev,
 			      u32 handle, u64 *offset);
 
-void i915_gem_object_release_mmap(struct drm_i915_gem_object *obj);
-
 void __i915_gem_object_release_mmap_gtt(struct drm_i915_gem_object *obj);
 void i915_gem_object_release_mmap_gtt(struct drm_i915_gem_object *obj);
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 6b69191c5543..d377c0ef8603 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -171,6 +171,17 @@  static void __i915_gem_free_object_rcu(struct rcu_head *head)
 	atomic_dec(&i915->mm.free_count);
 }
 
+static void i915_gem_object_release_mmap(struct drm_i915_gem_object *obj)
+{
+	/* Skip serialisation and waking the device if known to be not used. */
+
+	if (obj->userfault_count)
+		i915_gem_object_release_mmap_gtt(obj);
+
+	if (!RB_EMPTY_ROOT(&obj->mmo.offsets))
+		i915_gem_object_release_mmap_offset(obj);
+}
+
 static void __i915_gem_free_objects(struct drm_i915_private *i915,
 				    struct llist_node *freed)
 {