Re: [PATCH 2/6] drm/i915: Remove walk over obj->vma_list for the shrinker

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On Sat, 2017-08-12 at 12:51 +0100, Chris Wilson wrote:
> In the next patch, we want to reduce the lock coverage within the
> shrinker, and one of the dangerous walks we have is over obj->vma_list.
> We are only walking the obj->vma_list in order to check whether it has
> been permanently pinned by HW access, typically via use on the scanout.
> But we have a couple of other long term pins, the context objects for
> which we currently have to check the individual vma pin_count. If we
> instead mark these using obj->pin_display, we can forgo the dangerous
> and sometimes slow list iteration.

s/pin_display/pin_permanent/g

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

>  static bool swap_available(void)
>  {
>  	return get_nr_swap_pages() > 0;
> @@ -115,7 +96,13 @@ static bool can_release_pages(struct drm_i915_gem_object *obj)
>  	if (atomic_read(&obj->mm.pages_pin_count) > obj->bind_count)
>  		return false;
>  
> -	if (any_vma_pinned(obj))
> +	/* If any vma are permanently pinned, it will prevent us from reclaiming

Oh, it sounds so much better here when the variable is 'pin_permanent'.

> +	 * the obj->mm.pages. We only allow scanout objects to claim a permanent
> +	 * pin, along with a few others like the reserved context object. To
> +	 * simplify the scan, and to avoid walking the list of vma under the
> +	 * object, we just check the count of its permanently pinned.
> +	 */
> +	if (obj->pin_display)
>  		return false;
>  
>  	/* We can only return physical pages to the system if we can either
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index b0738d2b2a7f..874562bd59ae 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -866,6 +866,7 @@ execlists_context_pin(struct intel_engine_cs *engine,
>  		i915_ggtt_offset(ce->ring->vma);
>  
>  	ce->state->obj->mm.dirty = true;
> +	ce->state->obj->pin_display++;

This should be closer to intel_ring_pin (my preference).

>  
>  	i915_gem_context_get(ctx);
>  out:
> @@ -892,6 +893,7 @@ static void execlists_context_unpin(struct intel_engine_cs *engine,
>  		return;
>  
>  	intel_ring_unpin(ce->ring);
> +	ce->state->obj->pin_display--;

Or this should be closer to i915_gem_context_put. Just make it
symmetrict.
 
> @@ -1515,6 +1517,7 @@ intel_ring_context_pin(struct intel_engine_cs *engine,
>  		if (ret)
>  			goto err;
>  
> +		ce->state->obj->pin_display++;
>  		ce->state->obj->mm.dirty = true;

This is rather symmetric, the above can have mm.dirty after it, too.

As 'pin_permanent', this is;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx