[Intel-gfx] [PATCH v2 2/2] drm/i915/gem: Only revoke mmap handlers if active

[Tvrtko Ursulin]

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 at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at 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 at intel.com>

Regards,

Tvrtko